auth-ui: Auto delete Hydra login challenge cookie after use
Fixes failed logins
Change-Id: I891acf511c5cdb83c8ad24d209172724125b501f
diff --git a/core/auth/ui/e2e/hydra_test.go b/core/auth/ui/e2e/hydra_test.go
index 84e4e38..1b78c8c 100644
--- a/core/auth/ui/e2e/hydra_test.go
+++ b/core/auth/ui/e2e/hydra_test.go
@@ -22,44 +22,7 @@
session, callbacks := newHydraTestSession(t)
client := newDirectAPIClient(testStack.KratosURL, testStack.KratosAdmin)
defer client.close()
- username, password := uniqueKratosCredentials(t)
-
- status, body := postIdentityJSON(t, client, username, password)
- if status != http.StatusOK {
- t.Fatalf("create unique OAuth user returned status %d: %s", status, sanitizedResponseDiagnostic(body))
- }
- var identity identityAPIResponse
- if err := json.Unmarshal(body, &identity); err != nil || identity.ID == "" {
- t.Fatal("create unique OAuth user did not return a non-empty identity id")
- }
-
- clientToken, err := randomSecret(12)
- if err != nil {
- t.Fatal("generate unique OAuth client id")
- }
- clientSecret, err := randomSecret(24)
- if err != nil {
- t.Fatal("generate unique OAuth client secret")
- }
- clientID := "e2e-client-" + clientToken
- requestedClient := hydraOAuthClient{
- ClientID: clientID,
- ClientSecret: clientSecret,
- RedirectURIs: []string{callbacks.URI()},
- GrantTypes: []string{"authorization_code"},
- ResponseTypes: []string{"code"},
- Scope: "openid",
- TokenEndpointAuthMethod: "client_secret_basic",
- }
- ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
- created, err := client.createHydraOAuthClient(ctx, testStack.HydraAdmin, requestedClient)
- cancel()
- if err != nil {
- t.Fatal(err)
- }
- if created.ClientID != clientID {
- t.Fatal("Hydra Admin did not create the unique confidential client")
- }
+ username, password, clientID, clientSecret := createHydraBrowserUserAndClient(t, client, callbacks.URI())
state := uniqueOAuthValue(t, "state")
nonce := uniqueOAuthValue(t, "nonce")
@@ -136,6 +99,102 @@
_ = exchangeAndValidateHydraCode(t, client, clientID, clientSecret, callbacks.URI(), secondCode, username, secondNonce)
}
+func TestHydraStaleLoginChallengeDoesNotBreakDirectLogin(t *testing.T) {
+ session, callbacks := newHydraTestSession(t)
+ client := newDirectAPIClient(testStack.KratosURL, testStack.KratosAdmin)
+ defer client.close()
+ username, password, clientID, clientSecret := createHydraBrowserUserAndClient(t, client, callbacks.URI())
+
+ state := uniqueOAuthValue(t, "state")
+ nonce := uniqueOAuthValue(t, "nonce")
+ authAttempt, err := callbacks.Begin()
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, err := session.Page.Goto(hydraAuthorizationURL(testStack.HydraURL, clientID, callbacks.URI(), state, nonce)); err != nil {
+ t.Fatal("navigate to Hydra authorization request before direct-login regression")
+ }
+ assertKratosForm(t, session.Page, "/login")
+ fillCredentials(t, session.Page, username, password)
+ clickButton(t, session.Page, "login")
+ callback, err := authAttempt.Wait(context.Background())
+ if err != nil {
+ t.Fatal(err)
+ }
+ code, err := validateOAuthCallback(callback, state)
+ if err != nil {
+ t.Fatal(err)
+ }
+ assertCallbackPage(t, session, callbacks)
+ if err := authAttempt.Finish(context.Background()); err != nil {
+ t.Fatal(err)
+ }
+ _ = exchangeAndValidateHydraCode(t, client, clientID, clientSecret, callbacks.URI(), code, username, nonce)
+ assertAcceptedKratosSession(t, client, session)
+
+ if _, err := session.Page.Goto(testStack.UIURL + "/logout"); err != nil {
+ t.Fatal("log out after completing the Hydra authorization")
+ }
+ assertKratosForm(t, session.Page, "/login")
+ assertNoAcceptedKratosSession(t, client, session)
+
+ fillCredentials(t, session.Page, username, password)
+ clickButton(t, session.Page, "login")
+ assertAcceptedKratosSession(t, client, session)
+
+ current, err := url.Parse(session.Page.URL())
+ if err != nil || current.Scheme+"://"+current.Host != testStack.UIURL || current.Path != "/" {
+ symptom := "redirected away from the account page"
+ if body, bodyErr := session.Page.Locator("body").InnerText(); bodyErr == nil && strings.TrimSpace(body) == "Not Found" {
+ symptom = "returned Not Found"
+ }
+ t.Fatalf("direct login established a Kratos session but %s after reusing the consumed Hydra login challenge; final URL: %s", symptom, sanitizeFinalURL(session.Page.URL()))
+ }
+ assertGreeting(t, session.Page, username)
+}
+
+func createHydraBrowserUserAndClient(t *testing.T, client *directAPIClient, callbackURI string) (username, password, clientID, clientSecret string) {
+ t.Helper()
+ username, password = uniqueKratosCredentials(t)
+ status, body := postIdentityJSON(t, client, username, password)
+ if status != http.StatusOK {
+ t.Fatalf("create unique OAuth user returned status %d: %s", status, sanitizedResponseDiagnostic(body))
+ }
+ var identity identityAPIResponse
+ if err := json.Unmarshal(body, &identity); err != nil || identity.ID == "" {
+ t.Fatal("create unique OAuth user did not return a non-empty identity id")
+ }
+
+ clientToken, err := randomSecret(12)
+ if err != nil {
+ t.Fatal("generate unique OAuth client id")
+ }
+ clientSecret, err = randomSecret(24)
+ if err != nil {
+ t.Fatal("generate unique OAuth client secret")
+ }
+ clientID = "e2e-client-" + clientToken
+ requestedClient := hydraOAuthClient{
+ ClientID: clientID,
+ ClientSecret: clientSecret,
+ RedirectURIs: []string{callbackURI},
+ GrantTypes: []string{"authorization_code"},
+ ResponseTypes: []string{"code"},
+ Scope: "openid",
+ TokenEndpointAuthMethod: "client_secret_basic",
+ }
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ created, err := client.createHydraOAuthClient(ctx, testStack.HydraAdmin, requestedClient)
+ cancel()
+ if err != nil {
+ t.Fatal(err)
+ }
+ if created.ClientID != clientID {
+ t.Fatal("Hydra Admin did not create the unique confidential client")
+ }
+ return username, password, clientID, clientSecret
+}
+
func newHydraTestSession(t *testing.T) (*browserSession, *callbackCapture) {
t.Helper()
callbacks, err := startCallbackCapture()
diff --git a/core/auth/ui/main.go b/core/auth/ui/main.go
index 978ddc7..55dc963 100644
--- a/core/auth/ui/main.go
+++ b/core/auth/ui/main.go
@@ -240,12 +240,28 @@
// Login flow
+func clearLoginChallengeCookie(w http.ResponseWriter) {
+ http.SetCookie(w, &http.Cookie{
+ Name: "login_challenge",
+ Value: "",
+ Path: "/",
+ MaxAge: -1,
+ HttpOnly: true,
+ SameSite: http.SameSiteLaxMode,
+ })
+}
+
func (s *Server) loginInitiate(w http.ResponseWriter, r *http.Request) {
if err := r.ParseForm(); err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
- if challenge, ok := r.Form["login_challenge"]; ok {
+ challenge, hasChallenge := r.Form["login_challenge"]
+ flow, hasFlow := r.Form["flow"]
+ if !hasChallenge && !hasFlow {
+ clearLoginChallengeCookie(w)
+ }
+ if hasChallenge {
_, username, err := getWhoAmIFromKratos(r.Cookies())
if err != nil && err != ErrNotLoggedIn {
http.Error(w, err.Error(), http.StatusInternalServerError)
@@ -257,6 +273,7 @@
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
+ clearLoginChallengeCookie(w)
http.Redirect(w, r, redirectTo, http.StatusSeeOther)
return
}
@@ -264,15 +281,16 @@
http.SetCookie(w, &http.Cookie{
Name: "login_challenge",
Value: challenge[0],
+ Path: "/",
HttpOnly: true,
+ SameSite: http.SameSiteLaxMode,
})
}
returnTo := r.FormValue("return_to")
if returnTo == "" && s.defaultReturnTo != "" {
returnTo = s.defaultReturnTo
}
- flow, ok := r.Form["flow"]
- if !ok {
+ if !hasFlow {
addr := s.kratos + "/self-service/login/browser"
if returnTo != "" {
addr += fmt.Sprintf("?return_to=%s", returnTo)
@@ -471,6 +489,7 @@
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
+ clearLoginChallengeCookie(w)
http.Redirect(w, r, redirectTo, http.StatusSeeOther)
return
}
@@ -491,6 +510,7 @@
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
+ clearLoginChallengeCookie(w)
http.Redirect(w, r, redirectTo, http.StatusSeeOther)
return
}