feat: remove login session cookie during consent flow (#3667)

.github/workflows/ci.yaml

@@ -25,7 +25,7 @@ jobs:
           fetch-depth: 2
       - uses: actions/setup-go@v3
         with:
-          go-version: "1.20"
+          go-version: "1.21"
       - name: Start service
         run: ./test/conformance/start.sh
       - name: Run tests
@@ -82,7 +82,7 @@ jobs:
           key: ${{ needs.sdk-generate.outputs.sdk-cache-key }}
       - uses: actions/setup-go@v4
         with:
-          go-version: "1.20"
+          go-version: "1.21"
       - run: go list -json > go.list
       - name: Run nancy
         uses: sonatype-nexus-community/nancy-github-action@v1.0.2
@@ -125,7 +125,7 @@ jobs:
           key: ${{ needs.sdk-generate.outputs.sdk-cache-key }}
       - uses: actions/setup-go@v3
         with:
-          go-version: "1.20"
+          go-version: "1.21"
       - name: Setup HSM libs and packages
         run: |
           sudo apt install -y softhsm opensc
@@ -176,7 +176,7 @@ jobs:
       - uses: ory/ci/checkout@master
       - uses: actions/setup-go@v3
         with:
-          go-version: "1.20"
+          go-version: "1.21"
       - uses: actions/cache@v2
         with:
           path: ./test/e2e/hydra

.github/workflows/codeql-analysis.yml

@@ -41,9 +41,14 @@ jobs:
       - run: git checkout HEAD^2
         if: ${{ github.event_name == 'pull_request' }}
 
+      - uses: actions/setup-go@v4
+        with:
+          go-version: "1.21"
+      - run: go version
+
       # Initializes the CodeQL tools for scanning.
       - name: Initialize CodeQL
-        uses: github/codeql-action/init@v1
+        uses: github/codeql-action/init@v2
         with:
           languages: ${{ matrix.language }}
           # If you wish to specify custom queries, you can do so here or in a config file.
@@ -54,7 +59,7 @@ jobs:
       # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
       # If this step fails, then you should remove it and run the build manually (see below)
       - name: Autobuild
-        uses: github/codeql-action/autobuild@v1
+        uses: github/codeql-action/autobuild@v2
 
       # ℹ️ Command-line programs to run using the OS shell.
       # 📚 https://git.io/JvXDl
@@ -68,4 +73,4 @@ jobs:
       #   make release
 
       - name: Perform CodeQL Analysis
-        uses: github/codeql-action/analyze@v1
+        uses: github/codeql-action/analyze@v2

.github/workflows/format.yml

@@ -11,7 +11,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v3
         with:
-          go-version: "1.20"
+          go-version: "1.21"
       - run: make format
       - name: Indicate formatting issues
         run: git diff HEAD --exit-code --color

.github/workflows/licenses.yml

@@ -14,7 +14,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-go@v3
         with:
-          go-version: "1.20"
+          go-version: "1.21"
       - uses: actions/setup-node@v2
         with:
           node-version: "18"

.gitignore

@@ -31,3 +31,4 @@ persistence/sql/migrations/schema.sql
 cypress/videos
 cypress/screenshots
 BENCHMARKS.md
+*.sqlite

consent/manager.go

@@ -5,7 +5,6 @@ package consent
 
 import (
 	"context"
-	"time"
 
 	"github.com/gofrs/uuid"
 
@@ -43,7 +42,7 @@ type (
 		CreateLoginSession(ctx context.Context, session *flow.LoginSession) error
 		DeleteLoginSession(ctx context.Context, id string) (deletedSession *flow.LoginSession, err error)
 		RevokeSubjectLoginSession(ctx context.Context, user string) error
-		ConfirmLoginSession(ctx context.Context, session *flow.LoginSession, id string, authTime time.Time, subject string, remember bool) error
+		ConfirmLoginSession(ctx context.Context, loginSession *flow.LoginSession) error
 
 		CreateLoginRequest(ctx context.Context, req *flow.LoginRequest) (*flow.Flow, error)
 		GetLoginRequest(ctx context.Context, challenge string) (*flow.LoginRequest, error)

consent/manager_test_helpers.go

@@ -322,8 +322,8 @@ func TestHelperNID(r interface {
 		require.Error(t, err)
 		_, err = t1ValidNID.HandleLoginRequest(ctx, f, testLR.ID, &testHLR)
 		require.NoError(t, err)
-		require.Error(t, t2InvalidNID.ConfirmLoginSession(ctx, &testLS, testLS.ID, time.Now(), testLS.Subject, true))
-		require.NoError(t, t1ValidNID.ConfirmLoginSession(ctx, &testLS, testLS.ID, time.Now(), testLS.Subject, true))
+		require.Error(t, t2InvalidNID.ConfirmLoginSession(ctx, &testLS))
+		require.NoError(t, t1ValidNID.ConfirmLoginSession(ctx, &testLS))
 		ls, err := t2InvalidNID.DeleteLoginSession(ctx, testLS.ID)
 		require.Error(t, err)
 		assert.Nil(t, ls)
@@ -356,7 +356,7 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
 					Subject:         fmt.Sprintf("subject-%s", k),
 				}
 				require.NoError(t, m.CreateLoginSession(ctx, loginSession))
-				require.NoError(t, m.ConfirmLoginSession(ctx, loginSession, loginSession.ID, time.Now().Round(time.Second).UTC(), loginSession.Subject, true))
+				require.NoError(t, m.ConfirmLoginSession(ctx, loginSession))
 
 				lr[k] = &flow.LoginRequest{
 					ID:              makeID("fk-login-challenge", network, k),
@@ -392,6 +392,7 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
 					},
 				},
 			} {
+				tc := tc
 				t.Run("case=create-get-"+tc.s.ID, func(t *testing.T) {
 					_, err := m.GetRememberedLoginSession(ctx, &tc.s, tc.s.ID)
 					require.EqualError(t, err, x.ErrNotFound.Error(), "%#v", err)
@@ -403,17 +404,28 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
 					require.EqualError(t, err, x.ErrNotFound.Error())
 
 					updatedAuth := time.Time(tc.s.AuthenticatedAt).Add(time.Second)
-					require.NoError(t, m.ConfirmLoginSession(ctx, &tc.s, tc.s.ID, updatedAuth, tc.s.Subject, true))
+					tc.s.AuthenticatedAt = sqlxx.NullTime(updatedAuth)
+					require.NoError(t, m.ConfirmLoginSession(ctx, &flow.LoginSession{
+						ID:              tc.s.ID,
+						AuthenticatedAt: sqlxx.NullTime(updatedAuth),
+						Subject:         tc.s.Subject,
+						Remember:        true,
+					}))
 
 					got, err := m.GetRememberedLoginSession(ctx, nil, tc.s.ID)
 					require.NoError(t, err)
 					assert.EqualValues(t, tc.s.ID, got.ID)
-					assert.Equal(t, updatedAuth.Unix(), time.Time(got.AuthenticatedAt).Unix()) // this was updated from confirm...
+					assert.Equal(t, tc.s.AuthenticatedAt, got.AuthenticatedAt) // this was updated from confirm...
 					assert.EqualValues(t, tc.s.Subject, got.Subject)
 
 					// Make sure AuthAt does not equal...
 					updatedAuth2 := updatedAuth.Add(1 * time.Second).UTC()
-					require.NoError(t, m.ConfirmLoginSession(ctx, nil, tc.s.ID, updatedAuth2, "some-other-subject", true))
+					require.NoError(t, m.ConfirmLoginSession(ctx, &flow.LoginSession{
+						ID:              tc.s.ID,
+						AuthenticatedAt: sqlxx.NullTime(updatedAuth2),
+						Subject:         "some-other-subject",
+						Remember:        true,
+					}))
 
 					got2, err := m.GetRememberedLoginSession(ctx, nil, tc.s.ID)
 					require.NoError(t, err)
@@ -902,7 +914,8 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
 							Subject:         subject,
 						}
 						require.NoError(t, m.CreateLoginSession(ctx, ls))
-						require.NoError(t, m.ConfirmLoginSession(ctx, ls, ls.ID, time.Now(), ls.Subject, true))
+						ls.Remember = true
+						require.NoError(t, m.ConfirmLoginSession(ctx, ls))
 
 						cl := &client.Client{ID: uuid.New().String()}
 						switch k % 4 {
@@ -1042,7 +1055,7 @@ func ManagerTests(deps Deps, m Manager, clientManager client.Manager, fositeMana
 			}
 
 			require.NoError(t, m.CreateLoginSession(ctx, &s))
-			require.NoError(t, m.ConfirmLoginSession(ctx, &s, s.ID, time.Time(s.AuthenticatedAt), s.Subject, false))
+			require.NoError(t, m.ConfirmLoginSession(ctx, &s))
 
 			lr := &flow.LoginRequest{
 				ID:              uuid.New().String(),

consent/sdk_test.go

@@ -104,7 +104,7 @@ func TestSDK(t *testing.T) {
 
 	loginSession3 := &LoginSession{ID: cr3.LoginSessionID.String()}
 	require.NoError(t, m.CreateLoginSession(context.Background(), loginSession3))
-	require.NoError(t, m.ConfirmLoginSession(context.Background(), loginSession3, loginSession3.ID, time.Now(), cr3.Subject, true))
+	require.NoError(t, m.ConfirmLoginSession(context.Background(), loginSession3))
 	cr3Flow, err := m.CreateLoginRequest(context.Background(), &LoginRequest{
 		ID:          cr3.LoginChallenge.String(),
 		Subject:     cr3.Subject,
@@ -117,7 +117,7 @@ func TestSDK(t *testing.T) {
 
 	loginSession4 := &LoginSession{ID: cr4.LoginSessionID.String()}
 	require.NoError(t, m.CreateLoginSession(context.Background(), loginSession4))
-	require.NoError(t, m.ConfirmLoginSession(context.Background(), loginSession4, loginSession4.ID, time.Now(), cr4.Subject, true))
+	require.NoError(t, m.ConfirmLoginSession(context.Background(), loginSession4))
 	cr4Flow, err := m.CreateLoginRequest(context.Background(), &LoginRequest{
 		ID:        cr4.LoginChallenge.String(),
 		Client:    cr4.Client,

consent/strategy_default.go

@@ -108,8 +108,7 @@ func (s *DefaultStrategy) authenticationSession(ctx context.Context, _ http.Resp
 		return nil, errorsx.WithStack(ErrNoAuthenticationSessionFound)
 	}
 
-	sessionFromCookie := s.loginSessionFromCookie(r)
-	session, err := s.r.ConsentManager().GetRememberedLoginSession(r.Context(), sessionFromCookie, sessionID)
+	session, err := s.r.ConsentManager().GetRememberedLoginSession(r.Context(), nil, sessionID)
 	if errors.Is(err, x.ErrNotFound) {
 		s.r.Logger().WithRequest(r).WithError(err).
 			Debug("User logout skipped because cookie exists and session value exist but are not remembered any more.")
@@ -232,15 +231,6 @@ func (s *DefaultStrategy) forwardAuthenticationRequest(ctx context.Context, w ht
 	sessionID := uuid.New()
 	if session != nil {
 		sessionID = session.ID
-	} else {
-		// Create a stub session so that we can later update it.
-		loginSession := &flow.LoginSession{ID: sessionID}
-		if err := s.r.ConsentManager().CreateLoginSession(ctx, loginSession); err != nil {
-			return err
-		}
-		if err := flowctx.SetCookie(ctx, w, s.r, flowctx.LoginSessionCookie(flowctx.SuffixForClient(ar.GetClient())), loginSession); err != nil {
-			return err
-		}
 	}
 
 	// Set the session
@@ -454,16 +444,17 @@ func (s *DefaultStrategy) verifyAuthentication(
 	if !session.LoginRequest.Skip {
 		if time.Time(session.AuthenticatedAt).IsZero() {
 			return nil, errorsx.WithStack(fosite.ErrServerError.WithHint(
-				"Expected the handled login request to contain a valid authenticated_at value but it was zero. This is a bug which should be reported to https://github.com/ory/hydra."))
+				"Expected the handled login request to contain a valid authenticated_at value but it was zero. " +
+					"This is a bug which should be reported to https://github.com/ory/hydra."))
 		}
 
-		loginSession := s.loginSessionFromCookie(r)
-		if loginSession == nil {
-			return nil, fosite.ErrAccessDenied.WithHint("The login session cookie was not found or malformed.")
-		}
-
-		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 err := s.r.ConsentManager().ConfirmLoginSession(ctx, &flow.LoginSession{
+			ID:                        sessionID,
+			AuthenticatedAt:           session.AuthenticatedAt,
+			Subject:                   session.Subject,
+			IdentityProviderSessionID: sqlxx.NullString(session.IdentityProviderSessionID),
+			Remember:                  session.Remember,
+		}); err != nil {
 			if errors.Is(err, sqlcon.ErrUniqueViolation) {
 				return nil, errorsx.WithStack(fosite.ErrAccessDenied.WithHint("The login verifier has already been used."))
 			}
@@ -633,6 +624,10 @@ func (s *DefaultStrategy) forwardConsentRequest(
 		return err
 	}
 
+	if f.Client.GetID() != cl.GetID() {
+		return errorsx.WithStack(fosite.ErrInvalidClient.WithHint("The flow client id does not match the authorize request client id."))
+	}
+
 	clientSpecificCookieNameConsentCSRF := fmt.Sprintf("%s_%s", s.r.Config().CookieNameConsentCSRF(ctx), cl.CookieSuffix())
 	if err := createCsrfSession(w, r, s.r.Config(), store, clientSpecificCookieNameConsentCSRF, csrf, s.c.ConsentRequestMaxAge(ctx)); err != nil {
 		return errorsx.WithStack(err)
@@ -970,8 +965,7 @@ func (s *DefaultStrategy) issueLogoutVerifier(ctx context.Context, w http.Respon
 
 	// We do not really want to verify if the user (from id token hint) has a session here because it doesn't really matter.
 	// Instead, we'll check this when we're actually revoking the cookie!
-	sessionFromCookie := s.loginSessionFromCookie(r)
-	session, err := s.r.ConsentManager().GetRememberedLoginSession(r.Context(), sessionFromCookie, hintSid)
+	session, err := s.r.ConsentManager().GetRememberedLoginSession(r.Context(), nil, hintSid)
 	if errors.Is(err, x.ErrNotFound) {
 		// Such a session does not exist - maybe it has already been revoked? In any case, we can't do much except
 		// leaning back and redirecting back.
@@ -1101,8 +1095,7 @@ func (s *DefaultStrategy) HandleOpenIDConnectLogout(ctx context.Context, w http.
 }
 
 func (s *DefaultStrategy) HandleHeadlessLogout(ctx context.Context, _ http.ResponseWriter, r *http.Request, sid string) error {
-	sessionFromCookie := s.loginSessionFromCookie(r)
-	loginSession, lsErr := s.r.ConsentManager().GetRememberedLoginSession(ctx, sessionFromCookie, sid)
+	loginSession, lsErr := s.r.ConsentManager().GetRememberedLoginSession(ctx, nil, sid)
 
 	if errors.Is(lsErr, x.ErrNotFound) {
 		// This is ok (session probably already revoked), do nothing!

consent/strategy_logout_test.go

@@ -512,11 +512,11 @@ func TestLogoutFlows(t *testing.T) {
 				defer wg.Done()
 				require.NoError(t, err)
 				assert.False(t, res.Skip)
-				return hydra.AcceptOAuth2LoginRequest{Remember: pointerx.Bool(true)}
+				return hydra.AcceptOAuth2LoginRequest{Remember: pointerx.Ptr(true)}
 			}),
 			checkAndAcceptConsentHandler(t, adminApi, func(t *testing.T, res *hydra.OAuth2ConsentRequest, err error) hydra.AcceptOAuth2ConsentRequest {
 				require.NoError(t, err)
-				return hydra.AcceptOAuth2ConsentRequest{Remember: pointerx.Bool(true)}
+				return hydra.AcceptOAuth2ConsentRequest{Remember: pointerx.Ptr(true)}
 			}))
 
 		// Make an oauth 2 request to trigger the login check.

consent/strategy_oauth_test.go

@@ -19,8 +19,6 @@ import (
 	"golang.org/x/exp/slices"
 	"golang.org/x/oauth2"
 
-	"github.com/ory/hydra/v2/aead"
-	"github.com/ory/hydra/v2/consent"
 	"github.com/ory/x/pointerx"
 
 	"github.com/tidwall/gjson"
@@ -114,7 +112,7 @@ func TestStrategyLoginConsentNext(t *testing.T) {
 	t.Run("case=should fail because a login verifier was given that doesn't exist in the store", func(t *testing.T) {
 		testhelpers.NewLoginConsentUI(t, reg.Config(), testhelpers.HTTPServerNoExpectedCallHandler(t), testhelpers.HTTPServerNoExpectedCallHandler(t))
 		c := createDefaultClient(t)
-		hc := newHTTPClientWithFlowCookie(t, ctx, reg, c)
+		hc := testhelpers.NewEmptyJarClient(t)
 
 		makeRequestAndExpectError(
 			t, hc, c, url.Values{"login_verifier": {"does-not-exist"}},
@@ -128,7 +126,7 @@ func TestStrategyLoginConsentNext(t *testing.T) {
 		// - This should fail because a consent verifier was given but no login verifier
 		testhelpers.NewLoginConsentUI(t, reg.Config(), testhelpers.HTTPServerNoExpectedCallHandler(t), testhelpers.HTTPServerNoExpectedCallHandler(t))
 		c := createDefaultClient(t)
-		hc := newHTTPClientWithFlowCookie(t, ctx, reg, c)
+		hc := testhelpers.NewEmptyJarClient(t)
 
 		makeRequestAndExpectError(
 			t, hc, c, url.Values{"consent_verifier": {"does-not-exist"}},
@@ -252,7 +250,7 @@ func TestStrategyLoginConsentNext(t *testing.T) {
 			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.",
+				"The resource owner or authorization server denied the request. The consent verifier has already been used.",
 				q.Get("error_description"), q)
 		})
 
@@ -1122,15 +1120,3 @@ func (d *dropCSRFCookieJar) SetCookies(u *url.URL, cookies []*http.Cookie) {
 func (d *dropCSRFCookieJar) Cookies(u *url.URL) []*http.Cookie {
 	return d.jar.Cookies(u)
 }
-
-// TODO(hperl): rename
-func newHTTPClientWithFlowCookie(t *testing.T, ctx context.Context, reg interface {
-	ConsentManager() consent.Manager
-	Config() *config.DefaultProvider
-	FlowCipher() *aead.XChaCha20Poly1305
-}, c *client.Client) *http.Client {
-
-	hc := testhelpers.NewEmptyJarClient(t)
-
-	return hc
-}