From d254828d476861e7c9f09c182c4f4159f244a11e Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Thu, 30 May 2024 09:06:32 +0200 Subject: [PATCH] fix: session idp intent check (#8040) # Which Problems Are Solved If an IdP intent succeeded with the user was not linked yet, the IdP link was then added, the following IdP check on the session API would then fail with `Intent meant for another user (COMMAND-O8xk3w)`. This issue was introduced with when allowing IdP intents from other organizations (https://github.com/zitadel/zitadel/pull/7871) # How the Problems Are Solved The IdP link is now correctly checked in the session API (using the user's organization instead of the one from the intent). # Additional Changes - Improved the corresponding integration test to cover the exact bahvior. - Tests, which had to be updated with newer cases where additionally changed to use expectEventstore instead of deprecated eventstoreExpect and the two eventstore mocks of the session_tests.go where combined. # Additional Context - Relates to #7871 - This issue was reported by a customer. - will be back ported to 2.52.x --- .../session/v2/session_integration_test.go | 32 +-- internal/command/session.go | 2 +- internal/command/session_test.go | 237 +++++++++++------- 3 files changed, 163 insertions(+), 108 deletions(-) diff --git a/internal/api/grpc/session/v2/session_integration_test.go b/internal/api/grpc/session/v2/session_integration_test.go index 9423eea9ae..689893319b 100644 --- a/internal/api/grpc/session/v2/session_integration_test.go +++ b/internal/api/grpc/session/v2/session_integration_test.go @@ -429,6 +429,14 @@ func TestServer_CreateSession_successfulIntent_instant(t *testing.T) { func TestServer_CreateSession_successfulIntentUnknownUserID(t *testing.T) { idpID := Tester.AddGenericOAuthProvider(t, CTX) + // successful intent without known / linked user + idpUserID := "id" + intentID, token, _, _ := Tester.CreateSuccessfulOAuthIntent(t, CTX, idpID, "", idpUserID) + + // link the user (with info from intent) + Tester.CreateUserIDPlink(CTX, User.GetUserId(), idpUserID, idpID, User.GetUserId()) + + // session with intent check must now succeed createResp, err := Client.CreateSession(CTX, &session.CreateSessionRequest{ Checks: &session.Checks{ User: &session.CheckUser{ @@ -436,28 +444,6 @@ func TestServer_CreateSession_successfulIntentUnknownUserID(t *testing.T) { UserId: User.GetUserId(), }, }, - }, - }) - require.NoError(t, err) - verifyCurrentSession(t, createResp.GetSessionId(), createResp.GetSessionToken(), createResp.GetDetails().GetSequence(), time.Minute, nil, nil, 0, User.GetUserId()) - - idpUserID := "id" - intentID, token, _, _ := Tester.CreateSuccessfulOAuthIntent(t, CTX, idpID, "", idpUserID) - updateResp, err := Client.SetSession(CTX, &session.SetSessionRequest{ - SessionId: createResp.GetSessionId(), - Checks: &session.Checks{ - IdpIntent: &session.CheckIDPIntent{ - IdpIntentId: intentID, - IdpIntentToken: token, - }, - }, - }) - require.Error(t, err) - 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(), - Checks: &session.Checks{ IdpIntent: &session.CheckIDPIntent{ IdpIntentId: intentID, IdpIntentToken: token, @@ -465,7 +451,7 @@ func TestServer_CreateSession_successfulIntentUnknownUserID(t *testing.T) { }, }) require.NoError(t, err) - verifyCurrentSession(t, createResp.GetSessionId(), updateResp.GetSessionToken(), updateResp.GetDetails().GetSequence(), time.Minute, nil, nil, 0, User.GetUserId(), wantUserFactor, wantIntentFactor) + verifyCurrentSession(t, createResp.GetSessionId(), createResp.GetSessionToken(), createResp.GetDetails().GetSequence(), time.Minute, nil, nil, 0, User.GetUserId(), wantUserFactor, wantIntentFactor) } func TestServer_CreateSession_startedIntentFalseToken(t *testing.T) { diff --git a/internal/command/session.go b/internal/command/session.go index c9fd29ac46..d6fcb072ad 100644 --- a/internal/command/session.go +++ b/internal/command/session.go @@ -123,7 +123,7 @@ func CheckIntent(intentID, token string) SessionCommand { return zerrors.ThrowPreconditionFailed(nil, "COMMAND-O8xk3w", "Errors.Intent.OtherUser") } } else { - linkWriteModel := NewUserIDPLinkWriteModel(cmd.sessionWriteModel.UserID, cmd.intentWriteModel.IDPID, cmd.intentWriteModel.IDPUserID, cmd.intentWriteModel.ResourceOwner) + linkWriteModel := NewUserIDPLinkWriteModel(cmd.sessionWriteModel.UserID, cmd.intentWriteModel.IDPID, cmd.intentWriteModel.IDPUserID, cmd.sessionWriteModel.UserResourceOwner) err := cmd.eventstore.FilterToQueryReducer(ctx, linkWriteModel) if err != nil { return err diff --git a/internal/command/session_test.go b/internal/command/session_test.go index b41a886aa2..feaf395c22 100644 --- a/internal/command/session_test.go +++ b/internal/command/session_test.go @@ -31,7 +31,7 @@ func TestSessionCommands_getHumanWriteModel(t *testing.T) { userAggr := &user.NewAggregate("user1", "org1").Aggregate type fields struct { - eventstore *eventstore.Eventstore + eventstore func(*testing.T) *eventstore.Eventstore sessionWriteModel *SessionWriteModel } type res struct { @@ -46,7 +46,7 @@ func TestSessionCommands_getHumanWriteModel(t *testing.T) { { name: "missing UID", fields: fields{ - eventstore: &eventstore.Eventstore{}, + eventstore: expectEventstore(), sessionWriteModel: &SessionWriteModel{}, }, res: res{ @@ -57,7 +57,7 @@ func TestSessionCommands_getHumanWriteModel(t *testing.T) { { name: "filter error", fields: fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( expectFilterError(io.ErrClosedPipe), ), sessionWriteModel: &SessionWriteModel{ @@ -72,7 +72,7 @@ func TestSessionCommands_getHumanWriteModel(t *testing.T) { { name: "removed user", fields: fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), @@ -101,7 +101,7 @@ func TestSessionCommands_getHumanWriteModel(t *testing.T) { { name: "ok", fields: fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( expectFilter( eventFromEventPusher( user.NewHumanAddedEvent(context.Background(), @@ -133,7 +133,7 @@ func TestSessionCommands_getHumanWriteModel(t *testing.T) { } for _, tt := range tests { s := &SessionCommands{ - eventstore: tt.fields.eventstore, + eventstore: tt.fields.eventstore(t), sessionWriteModel: tt.fields.sessionWriteModel, } got, err := s.gethumanWriteModel(context.Background()) @@ -271,7 +271,7 @@ func TestCommands_CreateSession(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c := &Commands{ - eventstore: eventstoreExpect(t, tt.expect...), + eventstore: expectEventstore(tt.expect...)(t), idGenerator: tt.fields.idGenerator, sessionTokenCreator: tt.fields.tokenCreator, } @@ -284,7 +284,7 @@ func TestCommands_CreateSession(t *testing.T) { func TestCommands_UpdateSession(t *testing.T) { type fields struct { - eventstore *eventstore.Eventstore + eventstore func(*testing.T) *eventstore.Eventstore tokenVerifier func(ctx context.Context, sessionToken, sessionID, tokenID string) (err error) } type args struct { @@ -307,7 +307,7 @@ func TestCommands_UpdateSession(t *testing.T) { { "eventstore failed", fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( expectFilterError(zerrors.ThrowInternal(nil, "id", "filter failed")), ), }, @@ -321,7 +321,7 @@ func TestCommands_UpdateSession(t *testing.T) { { "no change", fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( expectFilter( eventFromEventPusher( session.NewAddedEvent(context.Background(), @@ -361,7 +361,7 @@ func TestCommands_UpdateSession(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c := &Commands{ - eventstore: tt.fields.eventstore, + eventstore: tt.fields.eventstore(t), sessionTokenVerifier: tt.fields.tokenVerifier, } got, err := c.UpdateSession(tt.args.ctx, tt.args.sessionID, tt.args.checks, tt.args.metadata, tt.args.lifetime) @@ -387,7 +387,7 @@ func TestCommands_updateSession(t *testing.T) { testNow := time.Now() type fields struct { - eventstore *eventstore.Eventstore + eventstore func(*testing.T) *eventstore.Eventstore } type args struct { ctx context.Context @@ -408,7 +408,7 @@ func TestCommands_updateSession(t *testing.T) { { "terminated", fields{ - eventstore: eventstoreExpect(t), + eventstore: expectEventstore(), }, args{ ctx: context.Background(), @@ -423,7 +423,7 @@ func TestCommands_updateSession(t *testing.T) { { "check failed", fields{ - eventstore: eventstoreExpect(t), + eventstore: expectEventstore(), }, args{ ctx: context.Background(), @@ -443,7 +443,7 @@ func TestCommands_updateSession(t *testing.T) { { "no change", fields{ - eventstore: eventstoreExpect(t), + eventstore: expectEventstore(), }, args{ ctx: authz.NewMockContext("instance1", "", ""), @@ -463,14 +463,13 @@ func TestCommands_updateSession(t *testing.T) { { "negative lifetime", fields{ - eventstore: eventstoreExpect(t), + eventstore: expectEventstore(), }, args{ ctx: authz.NewMockContext("instance1", "", ""), checks: &SessionCommands{ sessionWriteModel: NewSessionWriteModel("sessionID", "instance1"), sessionCommands: []SessionCommand{}, - eventstore: eventstoreExpect(t), createToken: func(sessionID string) (string, string, error) { return "tokenID", "token", @@ -489,7 +488,7 @@ func TestCommands_updateSession(t *testing.T) { { "lifetime set", fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( expectPush( session.NewLifetimeSetEvent(context.Background(), &session.NewAggregate("sessionID", "instance1").Aggregate, 10*time.Minute, @@ -505,7 +504,6 @@ func TestCommands_updateSession(t *testing.T) { checks: &SessionCommands{ sessionWriteModel: NewSessionWriteModel("sessionID", "instance1"), sessionCommands: []SessionCommand{}, - eventstore: eventstoreExpect(t), createToken: func(sessionID string) (string, string, error) { return "tokenID", "token", @@ -530,7 +528,17 @@ func TestCommands_updateSession(t *testing.T) { { "set user, password, metadata and token", fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), + ), + eventFromEventPusher( + user.NewHumanPasswordChangedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "$plain$x$password", false, ""), + ), + ), expectPush( session.NewUserCheckedEvent(context.Background(), &session.NewAggregate("sessionID", "instance1").Aggregate, "userID", "org1", testNow, &language.Afrikaans, @@ -555,18 +563,6 @@ func TestCommands_updateSession(t *testing.T) { CheckUser("userID", "org1", &language.Afrikaans), CheckPassword("password"), }, - eventstore: eventstoreExpect(t, - expectFilter( - eventFromEventPusher( - user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, - "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), - ), - eventFromEventPusher( - user.NewHumanPasswordChangedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, - "$plain$x$password", false, ""), - ), - ), - ), createToken: func(sessionID string) (string, string, error) { return "tokenID", "token", @@ -594,7 +590,14 @@ func TestCommands_updateSession(t *testing.T) { { "set user, intent not successful", fields{ - eventstore: eventstoreExpect(t), + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), + ), + ), + ), }, args{ ctx: authz.NewMockContext("instance1", "", ""), @@ -604,14 +607,6 @@ func TestCommands_updateSession(t *testing.T) { CheckUser("userID", "org1", &language.Afrikaans), CheckIntent("intent", "aW50ZW50"), }, - eventstore: eventstoreExpect(t, - expectFilter( - eventFromEventPusher( - user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, - "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), - ), - ), - ), createToken: func(sessionID string) (string, string, error) { return "tokenID", "token", @@ -633,7 +628,25 @@ func TestCommands_updateSession(t *testing.T) { { "set user, intent not for user", fields{ - eventstore: eventstoreExpect(t), + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), + ), + eventFromEventPusher( + idpintent.NewSucceededEvent(context.Background(), + &idpintent.NewAggregate("id", "instance1").Aggregate, + nil, + "idpUserID", + "idpUserName", + "userID2", + nil, + "", + ), + ), + ), + ), }, args{ ctx: authz.NewMockContext("instance1", "", ""), @@ -643,25 +656,6 @@ func TestCommands_updateSession(t *testing.T) { CheckUser("userID", "org1", &language.Afrikaans), CheckIntent("intent", "aW50ZW50"), }, - eventstore: eventstoreExpect(t, - expectFilter( - eventFromEventPusher( - user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, - "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), - ), - eventFromEventPusher( - idpintent.NewSucceededEvent(context.Background(), - &idpintent.NewAggregate("id", "instance1").Aggregate, - nil, - "idpUserID", - "idpUserName", - "userID2", - nil, - "", - ), - ), - ), - ), createToken: func(sessionID string) (string, string, error) { return "tokenID", "token", @@ -683,7 +677,7 @@ func TestCommands_updateSession(t *testing.T) { { "set user, intent incorrect token", fields{ - eventstore: eventstoreExpect(t), + eventstore: expectEventstore(), }, args{ ctx: authz.NewMockContext("instance1", "", ""), @@ -693,7 +687,6 @@ func TestCommands_updateSession(t *testing.T) { CheckUser("userID", "org1", &language.Afrikaans), CheckIntent("intent2", "aW50ZW50"), }, - eventstore: eventstoreExpect(t), createToken: func(sessionID string) (string, string, error) { return "tokenID", "token", @@ -715,7 +708,24 @@ func TestCommands_updateSession(t *testing.T) { { "set user, intent, metadata and token", fields{ - eventstore: eventstoreExpect(t, + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), + ), + eventFromEventPusher( + idpintent.NewSucceededEvent(context.Background(), + &idpintent.NewAggregate("id", "instance1").Aggregate, + nil, + "idpUserID", + "idpUsername", + "userID", + nil, + "", + ), + ), + ), expectPush( session.NewUserCheckedEvent(context.Background(), &session.NewAggregate("sessionID", "instance1").Aggregate, "userID", "org1", testNow, &language.Afrikaans), @@ -736,25 +746,6 @@ func TestCommands_updateSession(t *testing.T) { CheckUser("userID", "org1", &language.Afrikaans), CheckIntent("intent", "aW50ZW50"), }, - eventstore: eventstoreExpect(t, - expectFilter( - eventFromEventPusher( - user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, - "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), - ), - eventFromEventPusher( - idpintent.NewSucceededEvent(context.Background(), - &idpintent.NewAggregate("id", "instance1").Aggregate, - nil, - "idpUserID", - "idpUsername", - "userID", - nil, - "", - ), - ), - ), - ), createToken: func(sessionID string) (string, string, error) { return "tokenID", "token", @@ -779,12 +770,90 @@ func TestCommands_updateSession(t *testing.T) { }, }, }, + { + "set user, intent (user not linked yet)", + fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "username", "", "", "", "", language.English, domain.GenderUnspecified, "", false), + ), + eventFromEventPusher( + idpintent.NewStartedEvent(context.Background(), + &idpintent.NewAggregate("id", "instance1").Aggregate, + nil, + nil, + "idpID", + ), + ), + eventFromEventPusher( + idpintent.NewSucceededEvent(context.Background(), + &idpintent.NewAggregate("id", "instance1").Aggregate, + nil, + "idpUserID", + "idpUsername", + "", + nil, + "", + ), + ), + ), + expectFilter( + eventFromEventPusher( + user.NewUserIDPLinkAddedEvent(context.Background(), &user.NewAggregate("userID", "org1").Aggregate, + "idpID", + "idpUsername", + "idpUserID", + ), + ), + ), + expectPush( + session.NewUserCheckedEvent(context.Background(), &session.NewAggregate("sessionID", "instance1").Aggregate, + "userID", "org1", testNow, &language.Afrikaans), + session.NewIntentCheckedEvent(context.Background(), &session.NewAggregate("sessionID", "instance1").Aggregate, + testNow), + session.NewTokenSetEvent(context.Background(), &session.NewAggregate("sessionID", "instance1").Aggregate, + "tokenID"), + ), + ), + }, + args{ + ctx: authz.NewMockContext("instance1", "", ""), + checks: &SessionCommands{ + sessionWriteModel: NewSessionWriteModel("sessionID", "instance1"), + sessionCommands: []SessionCommand{ + CheckUser("userID", "org1", &language.Afrikaans), + CheckIntent("intent", "aW50ZW50"), + }, + createToken: func(sessionID string) (string, string, error) { + return "tokenID", + "token", + nil + }, + intentAlg: decryption(nil), + now: func() time.Time { + return testNow + }, + }, + }, + res{ + want: &SessionChanged{ + ObjectDetails: &domain.ObjectDetails{ + ResourceOwner: "instance1", + }, + ID: "sessionID", + NewToken: "token", + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c := &Commands{ - eventstore: tt.fields.eventstore, + eventstore: tt.fields.eventstore(t), } + tt.args.checks.eventstore = c.eventstore got, err := c.updateSession(tt.args.ctx, tt.args.checks, tt.args.metadata, tt.args.lifetime) require.ErrorIs(t, err, tt.res.err) assert.Equal(t, tt.res.want, got)