fix: proper errors for double-submit

ory · Oct 16, 2023 · 606c405 · 606c405
1 parent 0806721
commit 606c405
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 2 deletions.
diff --git a/consent/strategy_default.go b/consent/strategy_default.go
@@ -464,6 +464,9 @@ func (s *DefaultStrategy) verifyAuthentication(
 
 		loginSession.IdentityProviderSessionID = sqlxx.NullString(session.IdentityProviderSessionID)
 		if err := s.r.ConsentManager().ConfirmLoginSession(ctx, loginSession, sessionID, time.Time(session.AuthenticatedAt), session.Subject, session.Remember); err != nil {
+			if errors.Is(err, sqlcon.ErrUniqueViolation) {
+				return nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The login verifier has already been used."))
+			}
 			return nil, err
 		}
 	}
@@ -659,7 +662,9 @@ func (s *DefaultStrategy) verifyConsent(ctx context.Context, _ http.ResponseWrit
 	}
 
 	session, err := s.r.ConsentManager().VerifyAndInvalidateConsentRequest(ctx, verifier)
-	if errors.Is(err, sqlcon.ErrNoRows) {
+	if errors.Is(err, sqlcon.ErrUniqueViolation) {
+		return nil, nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The consent verifier has already been used."))
+	} else if errors.Is(err, sqlcon.ErrNoRows) {
 		return nil, nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The consent verifier has already been used, has not been granted, or is invalid."))
 	} else if err != nil {
 		return nil, nil, err

diff --git a/consent/strategy_oauth_test.go b/consent/strategy_oauth_test.go
@@ -202,6 +202,76 @@ func TestStrategyLoginConsentNext(t *testing.T) {
 		makeRequestAndExpectError(t, nil, c, url.Values{}, "expect-reject-consent")
 	})
 
+	t.Run("suite=double-submit", func(t *testing.T) {
+		ctx := context.Background()
+		c := createDefaultClient(t)
+		hc := testhelpers.NewEmptyJarClient(t)
+		var loginChallenge, consentChallenge string
+
+		testhelpers.NewLoginConsentUI(t, reg.Config(),
+			func(w http.ResponseWriter, r *http.Request) {
+				res, _, err := adminClient.OAuth2Api.GetOAuth2LoginRequest(ctx).
+					LoginChallenge(r.URL.Query().Get("login_challenge")).
+					Execute()
+				require.NoError(t, err)
+				loginChallenge = res.Challenge
+
+				v, _, err := adminClient.OAuth2Api.AcceptOAuth2LoginRequest(ctx).
+					LoginChallenge(loginChallenge).
+					AcceptOAuth2LoginRequest(hydra.AcceptOAuth2LoginRequest{Subject: "aeneas-rekkas"}).
+					Execute()
+				require.NoError(t, err)
+				require.NotEmpty(t, v.RedirectTo)
+				http.Redirect(w, r, v.RedirectTo, http.StatusFound)
+			},
+			func(w http.ResponseWriter, r *http.Request) {
+				res, _, err := adminClient.OAuth2Api.GetOAuth2ConsentRequest(ctx).
+					ConsentChallenge(r.URL.Query().Get("consent_challenge")).
+					Execute()
+				require.NoError(t, err)
+				consentChallenge = res.Challenge
+
+				v, _, err := adminClient.OAuth2Api.AcceptOAuth2ConsentRequest(ctx).
+					ConsentChallenge(consentChallenge).
+					AcceptOAuth2ConsentRequest(hydra.AcceptOAuth2ConsentRequest{}).
+					Execute()
+				require.NoError(t, err)
+				require.NotEmpty(t, v.RedirectTo)
+				http.Redirect(w, r, v.RedirectTo, http.StatusFound)
+			})
+
+		makeRequestAndExpectCode(t, hc, c, url.Values{})
+
+		t.Run("case=double-submit login verifier", func(t *testing.T) {
+			v, _, err := adminClient.OAuth2Api.AcceptOAuth2LoginRequest(ctx).
+				LoginChallenge(loginChallenge).
+				AcceptOAuth2LoginRequest(hydra.AcceptOAuth2LoginRequest{Subject: "aeneas-rekkas"}).
+				Execute()
+			require.NoError(t, err)
+			res, err := hc.Get(v.RedirectTo)
+			require.NoError(t, err)
+			q := res.Request.URL.Query()
+			assert.Equal(t,
+				"The resource owner or authorization server denied the request. The login verifier has already been used.",
+				q.Get("error_description"), q)
+		})
+
+		t.Run("case=double-submit consent verifier", func(t *testing.T) {
+			v, _, err := adminClient.OAuth2Api.AcceptOAuth2ConsentRequest(ctx).
+				ConsentChallenge(consentChallenge).
+				AcceptOAuth2ConsentRequest(hydra.AcceptOAuth2ConsentRequest{}).
+				Execute()
+			require.NoError(t, err)
+			res, err := hc.Get(v.RedirectTo)
+			require.NoError(t, err)
+			q := res.Request.URL.Query()
+			assert.Equal(t,
+				"The resource owner or authorization server denied the request. The consent verifier has already been used.",
+				q.Get("error_description"), q)
+		})
+
+	})
+
 	t.Run("case=should pass and set acr values properly", func(t *testing.T) {
 		c := createDefaultClient(t)
 		testhelpers.NewLoginConsentUI(t, reg.Config(),

diff --git a/persistence/sql/persister_consent.go b/persistence/sql/persister_consent.go
@@ -385,7 +385,11 @@ func (p *Persister) ConfirmLoginSession(ctx context.Context, session *flow.Login
 		session.Subject = subject
 		session.Remember = remember
 
-		return p.CreateWithNetwork(ctx, session)
+		err := p.CreateWithNetwork(ctx, session)
+		if err != nil {
+			return sqlcon.HandleError(err)
+		}
+		return nil
 	}
 
 	// In some unit tests, we still confirm the login session without data from the cookie. We can remove this case