TheArchive/zitadel
1
0
Fork 0
mirror of https://github.com/zitadel/zitadel.git synced 2024-12-13 11:34:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: session idp intent check (#8040)

Browse Source 
# 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

(cherry picked from commit d254828d47)
This commit is contained in:
Livio Spring 2024-05-30 09:06:32 +02:00
parent 0b82fc1ed0
commit 4f114e4bf3
No known key found for this signature in database
GPG Key ID: 26BB1C2FA5952CF0
3 changed files with 163 additions and 108 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
internal/api/grpc/session/v2/session_integration_test.go
View File

@ -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) {

2
internal/command/session.go
View File

@ -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

237
internal/command/session_test.go
View File

@ -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)