From 5b1160de1e2a9d46df9e257e3ebb07c10a444cf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Wed, 22 May 2024 07:56:11 +0200 Subject: [PATCH] feat(session): allow update of session without token (#7963) # Which Problems Are Solved The session update requires the current session token as argument. Since this adds extra complexity but no real additional security and prevents case like magic links, we want to remove this requirement. We still require the session token on other resouces / endpoints, e.g. for finalizing the auth request or on idp intents. # How the Problems Are Solved - Removed the session token verifier in the Update Session GRPc call. - Removed the session token from login UI examples session update calls # Additional Changes - none # Additional Context - Closes #7883 --- .../login-ui/_update_session_webauthn.mdx | 1 - docs/docs/guides/integrate/login-ui/mfa.mdx | 3 - .../integrate/login-ui/username-password.mdx | 3 +- internal/api/grpc/session/v2/session.go | 6 +- .../session/v2/session_integration_test.go | 75 +++++++------------ internal/command/session.go | 5 +- internal/command/session_test.go | 49 ++---------- internal/integration/client.go | 3 +- .../session/v2beta/session_service.proto | 8 +- 9 files changed, 42 insertions(+), 111 deletions(-) diff --git a/docs/docs/guides/integrate/login-ui/_update_session_webauthn.mdx b/docs/docs/guides/integrate/login-ui/_update_session_webauthn.mdx index 0acbf805f..430a31b33 100644 --- a/docs/docs/guides/integrate/login-ui/_update_session_webauthn.mdx +++ b/docs/docs/guides/integrate/login-ui/_update_session_webauthn.mdx @@ -13,7 +13,6 @@ curl --request PATCH \ --header 'Authorization: Bearer '"$TOKEN"''\ --header 'Content-Type: application/json' \ --data '{ - "sessionToken": "yMDi6uVPJAcphbbz0LaxC07ihWkNTe7m0Xqch8SzfM5Cz3HSIQIDZ65x1f5Qal0jxz0MEyo-_zYcUg", "checks": { "webAuthN": { "credentialAssertionData": {} diff --git a/docs/docs/guides/integrate/login-ui/mfa.mdx b/docs/docs/guides/integrate/login-ui/mfa.mdx index 65ee8da47..8f6fe60e3 100644 --- a/docs/docs/guides/integrate/login-ui/mfa.mdx +++ b/docs/docs/guides/integrate/login-ui/mfa.mdx @@ -140,7 +140,6 @@ curl --request PATCH \ --header 'Accept: application/json' \ --header 'Content-Type: application/json' \ --data '{ - "sessionToken": "W3mEoesTiYOsiR1LYUCRw3vaEwXKLGDTsqOV_bkOhlah_-ZbuiLgvnzADwe_iYMusbwkMhp7VfMn8j", "checks": { "totp": { "code": "323764" @@ -270,7 +269,6 @@ curl --request PATCH \ --header 'Authorization: Bearer '"$TOKEN"'' \ --header 'Content-Type: application/json' \ --data '{ - "sessionToken": "W3mEoesTiYOsiR1LYUCRw3WaFwXKLGDRsqOV_bkOhlah_-ZpuiLgvnzADwe_iYMusbwkMhp7VfMn8g", "checks": { "otpSms": { "code": "3237642" @@ -359,7 +357,6 @@ curl --request PATCH \ --header 'Authorization: Bearer '"$TOKEN"'' \ --header 'Content-Type: application/json' \ --data '{ - "sessionToken": "W3mEoesTiYOsiR1LYUCRw3WaFwXKLGDRsqOV_bkOhlah_-ZpuiLgvnzADwe_iYMusbwkMhp7VfMn8g", "checks": { "otpEmail": { "code": "3237642" diff --git a/docs/docs/guides/integrate/login-ui/username-password.mdx b/docs/docs/guides/integrate/login-ui/username-password.mdx index 2f1dcf4be..1be65bebc 100644 --- a/docs/docs/guides/integrate/login-ui/username-password.mdx +++ b/docs/docs/guides/integrate/login-ui/username-password.mdx @@ -163,7 +163,7 @@ Checkout how to handle [session validation](./session-validation). Your session already has a username check. The next step is to check the password. -To update an existing session, add the session ID to the URL and the session token you got in the previous step to the request body. +To update an existing session, add the session ID you got in the previous step to the URL. ### Request @@ -174,7 +174,6 @@ curl --request PATCH \ --header 'Authorization: Bearer '"$TOKEN"''\ --header 'Content-Type: application/json' \ --data '{ - "sessionToken": "yMDi6uVPJAcphbbz0LaxC07ihWkNTe7m0Xqch8SzfM5Cz3HSIQIDZ65x1f5Qal0jxz0MEyo-_zYcUg", "checks": { "password": { "password": "Secr3tP4ssw0rd!" diff --git a/internal/api/grpc/session/v2/session.go b/internal/api/grpc/session/v2/session.go index 6fac6a872..7af87798e 100644 --- a/internal/api/grpc/session/v2/session.go +++ b/internal/api/grpc/session/v2/session.go @@ -89,14 +89,10 @@ func (s *Server) SetSession(ctx context.Context, req *session.SetSessionRequest) return nil, err } - set, err := s.command.UpdateSession(ctx, req.GetSessionId(), req.GetSessionToken(), cmds, req.GetMetadata(), req.GetLifetime().AsDuration()) + set, err := s.command.UpdateSession(ctx, req.GetSessionId(), cmds, req.GetMetadata(), req.GetLifetime().AsDuration()) if err != nil { return nil, err } - // if there's no new token, just return the current - if set.NewToken == "" { - set.NewToken = req.GetSessionToken() - } return &session.SetSessionResponse{ Details: object.DomainToDetailsPb(set.ObjectDetails), SessionToken: set.NewToken, diff --git a/internal/api/grpc/session/v2/session_integration_test.go b/internal/api/grpc/session/v2/session_integration_test.go index 645db7673..9423eea9a 100644 --- a/internal/api/grpc/session/v2/session_integration_test.go +++ b/internal/api/grpc/session/v2/session_integration_test.go @@ -366,8 +366,7 @@ func TestServer_CreateSession_webauthn(t *testing.T) { // update the session with webauthn assertion data updateResp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: createResp.GetSessionToken(), + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ WebAuthN: &session.CheckWebAuthN{ CredentialAssertionData: assertionData, @@ -394,8 +393,7 @@ func TestServer_CreateSession_successfulIntent(t *testing.T) { intentID, token, _, _ := Tester.CreateSuccessfulOAuthIntent(t, CTX, idpID, User.GetUserId(), "id") updateResp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: createResp.GetSessionToken(), + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ IdpIntent: &session.CheckIDPIntent{ IdpIntentId: intentID, @@ -446,8 +444,7 @@ func TestServer_CreateSession_successfulIntentUnknownUserID(t *testing.T) { idpUserID := "id" intentID, token, _, _ := Tester.CreateSuccessfulOAuthIntent(t, CTX, idpID, "", idpUserID) updateResp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: createResp.GetSessionToken(), + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ IdpIntent: &session.CheckIDPIntent{ IdpIntentId: intentID, @@ -459,8 +456,7 @@ func TestServer_CreateSession_successfulIntentUnknownUserID(t *testing.T) { Tester.CreateUserIDPlink(CTX, User.GetUserId(), idpUserID, idpID, User.GetUserId()) intentID, token, _, _ = Tester.CreateSuccessfulOAuthIntent(t, CTX, idpID, User.GetUserId(), idpUserID) updateResp, err = Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: createResp.GetSessionToken(), + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ IdpIntent: &session.CheckIDPIntent{ IdpIntentId: intentID, @@ -489,8 +485,7 @@ func TestServer_CreateSession_startedIntentFalseToken(t *testing.T) { intentID := Tester.CreateIntent(t, CTX, idpID) _, err = Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: createResp.GetSessionToken(), + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ IdpIntent: &session.CheckIDPIntent{ IdpIntentId: intentID, @@ -543,8 +538,7 @@ func TestServer_SetSession_flow_totp(t *testing.T) { t.Run("check user", func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ User: &session.CheckUser{ Search: &session.CheckUser_UserId{ @@ -560,8 +554,7 @@ func TestServer_SetSession_flow_totp(t *testing.T) { t.Run("check webauthn, user verified (passkey)", func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Challenges: &session.RequestChallenges{ WebAuthN: &session.RequestChallenges_WebAuthN{ Domain: Tester.Config.ExternalDomain, @@ -577,8 +570,7 @@ func TestServer_SetSession_flow_totp(t *testing.T) { require.NoError(t, err) resp, err = Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ WebAuthN: &session.CheckWebAuthN{ CredentialAssertionData: assertionData, @@ -600,8 +592,7 @@ func TestServer_SetSession_flow_totp(t *testing.T) { code, err := totp.GenerateCode(totpSecret, time.Now()) require.NoError(t, err) resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ Totp: &session.CheckTOTP{ Code: code, @@ -621,8 +612,7 @@ func TestServer_SetSession_flow_totp(t *testing.T) { t.Run("check user", func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createRespImport.GetSessionId(), - SessionToken: sessionTokenImport, + SessionId: createRespImport.GetSessionId(), Checks: &session.Checks{ User: &session.CheckUser{ Search: &session.CheckUser_UserId{ @@ -639,8 +629,7 @@ func TestServer_SetSession_flow_totp(t *testing.T) { code, err := totp.GenerateCode(totpSecret, time.Now()) require.NoError(t, err) resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createRespImport.GetSessionId(), - SessionToken: sessionTokenImport, + SessionId: createRespImport.GetSessionId(), Checks: &session.Checks{ Totp: &session.CheckTOTP{ Code: code, @@ -662,8 +651,7 @@ func TestServer_SetSession_flow(t *testing.T) { t.Run("check user", func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ User: &session.CheckUser{ Search: &session.CheckUser_UserId{ @@ -679,8 +667,7 @@ func TestServer_SetSession_flow(t *testing.T) { t.Run("check webauthn, user verified (passkey)", func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Challenges: &session.RequestChallenges{ WebAuthN: &session.RequestChallenges_WebAuthN{ Domain: Tester.Config.ExternalDomain, @@ -696,8 +683,7 @@ func TestServer_SetSession_flow(t *testing.T) { require.NoError(t, err) resp, err = Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ WebAuthN: &session.CheckWebAuthN{ CredentialAssertionData: assertionData, @@ -723,8 +709,7 @@ func TestServer_SetSession_flow(t *testing.T) { } { t.Run(userVerificationRequirement.String(), func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Challenges: &session.RequestChallenges{ WebAuthN: &session.RequestChallenges_WebAuthN{ Domain: Tester.Config.ExternalDomain, @@ -740,8 +725,7 @@ func TestServer_SetSession_flow(t *testing.T) { require.NoError(t, err) resp, err = Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ WebAuthN: &session.CheckWebAuthN{ CredentialAssertionData: assertionData, @@ -759,8 +743,7 @@ func TestServer_SetSession_flow(t *testing.T) { code, err := totp.GenerateCode(totpSecret, time.Now()) require.NoError(t, err) resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ Totp: &session.CheckTOTP{ Code: code, @@ -774,8 +757,7 @@ func TestServer_SetSession_flow(t *testing.T) { t.Run("check OTP SMS", func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Challenges: &session.RequestChallenges{ OtpSms: &session.RequestChallenges_OTPSMS{ReturnCode: true}, }, @@ -788,8 +770,7 @@ func TestServer_SetSession_flow(t *testing.T) { require.NotEmpty(t, otp) resp, err = Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ OtpSms: &session.CheckOTP{ Code: otp, @@ -803,8 +784,7 @@ func TestServer_SetSession_flow(t *testing.T) { t.Run("check OTP Email", func(t *testing.T) { resp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Challenges: &session.RequestChallenges{ OtpEmail: &session.RequestChallenges_OTPEmail{ DeliveryType: &session.RequestChallenges_OTPEmail_ReturnCode_{}, @@ -819,8 +799,7 @@ func TestServer_SetSession_flow(t *testing.T) { require.NotEmpty(t, otp) resp, err = Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionToken, + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ OtpEmail: &session.CheckOTP{ Code: otp, @@ -840,19 +819,17 @@ func TestServer_SetSession_expired(t *testing.T) { require.NoError(t, err) // test session token works - sessionResp, err := Tester.Client.SessionV2.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: createResp.GetSessionToken(), - Lifetime: durationpb.New(20 * time.Second), + _, err = Tester.Client.SessionV2.SetSession(CTX, &session.SetSessionRequest{ + SessionId: createResp.GetSessionId(), + Lifetime: durationpb.New(20 * time.Second), }) require.NoError(t, err) // ensure session expires and does not work anymore time.Sleep(20 * time.Second) _, err = Tester.Client.SessionV2.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: sessionResp.GetSessionToken(), - Lifetime: durationpb.New(20 * time.Second), + SessionId: createResp.GetSessionId(), + Lifetime: durationpb.New(20 * time.Second), }) require.Error(t, err) } diff --git a/internal/command/session.go b/internal/command/session.go index 843d2be39..c9fd29ac4 100644 --- a/internal/command/session.go +++ b/internal/command/session.go @@ -312,15 +312,12 @@ func (c *Commands) CreateSession(ctx context.Context, cmds []SessionCommand, met return c.updateSession(ctx, cmd, metadata, lifetime) } -func (c *Commands) UpdateSession(ctx context.Context, sessionID, sessionToken string, cmds []SessionCommand, metadata map[string][]byte, lifetime time.Duration) (set *SessionChanged, err error) { +func (c *Commands) UpdateSession(ctx context.Context, sessionID string, cmds []SessionCommand, metadata map[string][]byte, lifetime time.Duration) (set *SessionChanged, err error) { sessionWriteModel := NewSessionWriteModel(sessionID, authz.GetInstance(ctx).InstanceID()) err = c.eventstore.FilterToQueryReducer(ctx, sessionWriteModel) if err != nil { return nil, err } - if err := c.sessionTokenVerifier(ctx, sessionToken, sessionWriteModel.AggregateID, sessionWriteModel.TokenID); err != nil { - return nil, err - } cmd := c.NewSessionCommands(cmds, sessionWriteModel) return c.updateSession(ctx, cmd, metadata, lifetime) } diff --git a/internal/command/session_test.go b/internal/command/session_test.go index 998463efe..b41a886aa 100644 --- a/internal/command/session_test.go +++ b/internal/command/session_test.go @@ -288,12 +288,11 @@ func TestCommands_UpdateSession(t *testing.T) { tokenVerifier func(ctx context.Context, sessionToken, sessionID, tokenID string) (err error) } type args struct { - ctx context.Context - sessionID string - sessionToken string - checks []SessionCommand - metadata map[string][]byte - lifetime time.Duration + ctx context.Context + sessionID string + checks []SessionCommand + metadata map[string][]byte + lifetime time.Duration } type res struct { want *SessionChanged @@ -319,37 +318,6 @@ func TestCommands_UpdateSession(t *testing.T) { err: zerrors.ThrowInternal(nil, "id", "filter failed"), }, }, - { - "invalid session token", - fields{ - eventstore: eventstoreExpect(t, - expectFilter( - eventFromEventPusher( - session.NewAddedEvent(context.Background(), - &session.NewAggregate("sessionID", "instance1").Aggregate, - &domain.UserAgent{ - FingerprintID: gu.Ptr("fp1"), - IP: net.ParseIP("1.2.3.4"), - Description: gu.Ptr("firefox"), - Header: http.Header{"foo": []string{"bar"}}, - }, - )), - eventFromEventPusher( - session.NewTokenSetEvent(context.Background(), &session.NewAggregate("sessionID", "instance1").Aggregate, - "tokenID")), - ), - ), - tokenVerifier: newMockTokenVerifierInvalid(), - }, - args{ - ctx: context.Background(), - sessionID: "sessionID", - sessionToken: "invalid", - }, - res{ - err: zerrors.ThrowPermissionDenied(nil, "COMMAND-sGr42", "Errors.Session.Token.Invalid"), - }, - }, { "no change", fields{ @@ -375,9 +343,8 @@ func TestCommands_UpdateSession(t *testing.T) { }, }, args{ - ctx: context.Background(), - sessionID: "sessionID", - sessionToken: "token", + ctx: context.Background(), + sessionID: "sessionID", }, res{ want: &SessionChanged{ @@ -397,7 +364,7 @@ func TestCommands_UpdateSession(t *testing.T) { eventstore: tt.fields.eventstore, sessionTokenVerifier: tt.fields.tokenVerifier, } - got, err := c.UpdateSession(tt.args.ctx, tt.args.sessionID, tt.args.sessionToken, tt.args.checks, tt.args.metadata, tt.args.lifetime) + got, err := c.UpdateSession(tt.args.ctx, tt.args.sessionID, tt.args.checks, tt.args.metadata, tt.args.lifetime) require.ErrorIs(t, err, tt.res.err) assert.Equal(t, tt.res.want, got) }) diff --git a/internal/integration/client.go b/internal/integration/client.go index 4b843f68e..21410bec9 100644 --- a/internal/integration/client.go +++ b/internal/integration/client.go @@ -521,8 +521,7 @@ func (s *Tester) CreateVerifiedWebAuthNSessionWithLifetime(t *testing.T, ctx con require.NoError(t, err) updateResp, err := s.Client.SessionV2.SetSession(ctx, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - SessionToken: createResp.GetSessionToken(), + SessionId: createResp.GetSessionId(), Checks: &session.Checks{ WebAuthN: &session.CheckWebAuthN{ CredentialAssertionData: assertion, diff --git a/proto/zitadel/session/v2beta/session_service.proto b/proto/zitadel/session/v2beta/session_service.proto index 94efe5a7c..6a2e731a9 100644 --- a/proto/zitadel/session/v2beta/session_service.proto +++ b/proto/zitadel/session/v2beta/session_service.proto @@ -296,7 +296,7 @@ message CreateSessionResponse{ ]; string session_token = 3 [ (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { - description: "\"The current token of the session, which is required for further updates of the session or the request other resources.\""; + description: "\"The current token of the session, which is required for delete session, get session or the request of other resources.\""; } ]; Challenges challenges = 4; @@ -313,11 +313,11 @@ message SetSessionRequest{ } ]; string session_token = 2 [ - (validate.rules).string = {min_len: 1, max_len: 200}, + (validate.rules).string = {min_len: 0, max_len: 200}, (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { min_length: 1; max_length: 200; - description: "\"The current token of the session, previously returned on the create / update request.\""; + description: "\"DEPRECATED: this field is ignored.\""; } ]; Checks checks = 3[ @@ -344,7 +344,7 @@ message SetSessionResponse{ zitadel.object.v2beta.Details details = 1; string session_token = 2 [ (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { - description: "\"The current token of the session, which is required for further updates of the session or to request other resources.\""; + description: "\"The current token of the session, which is required for delete session, get session or the request of other resources.\""; } ]; Challenges challenges = 3;