From 0af37d45e9ad1c6b8fe22f4b4f4629d57d22b58b Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Thu, 15 Aug 2024 07:39:54 +0200 Subject: [PATCH] fix: handle user remove correctly in v1 sessions for login (#8432) # Which Problems Are Solved In case a user was deleted and recreated with the same id, they would never be able to authenticate through the login UI, since it would return an error "User not active". This was due to the check in the auth request / session handling for the login UI, where the user removed event would terminate an further event check and ignore the newly added user. # How the Problems Are Solved - The user removed event no longer returns an error, but is handled as a session termination event. (A user removed event will already delete the user and the preceding `activeUserById` function will deny the authentication.) # Additional Changes Updated tests to be able to handle multiple events in the mocks. # Additional Context closes https://github.com/zitadel/zitadel/issues/8201 Co-authored-by: Silvan --- .../eventsourcing/eventstore/auth_request.go | 2 - .../eventstore/auth_request_test.go | 141 ++++++++++++------ .../repository/view/model/user_session.go | 3 +- 3 files changed, 99 insertions(+), 47 deletions(-) diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request.go b/internal/auth/repository/eventsourcing/eventstore/auth_request.go index dda1a5fdb..bef602ff0 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request.go @@ -1615,8 +1615,6 @@ func userSessionByIDs(ctx context.Context, provider userSessionViewProvider, eve if userAgentID != agentID { continue } - case user_repo.UserRemovedType: - return nil, zerrors.ThrowPreconditionFailed(nil, "EVENT-dG2fe", "Errors.User.NotActive") } err := sessionCopy.AppendEvent(event) logging.WithFields("traceID", tracing.TraceIDFromCtx(ctx)).OnError(err).Warn("error appending event") diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go index c99ddc6f1..7308d6ce1 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request_test.go @@ -110,15 +110,12 @@ func (m *mockViewNoUser) UserByID(context.Context, string, string) (*user_view_m } type mockEventUser struct { - Event eventstore.Event + Events []eventstore.Event CodeExists bool } func (m *mockEventUser) UserEventsByID(ctx context.Context, id string, changeDate time.Time, types []eventstore.EventType) ([]eventstore.Event, error) { - if m.Event != nil { - return []eventstore.Event{m.Event}, nil - } - return nil, nil + return m.Events, nil } func (m *mockEventUser) PasswordCodeExists(ctx context.Context, userID string) (bool, error) { @@ -725,9 +722,11 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { fields{ userViewProvider: &mockViewUser{}, userEventProvider: &mockEventUser{ - Event: &es_models.Event{ - AggregateType: user_repo.AggregateType, - Typ: user_repo.UserDeactivatedType, + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserDeactivatedType, + }, }, }, orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, @@ -747,9 +746,11 @@ func TestAuthRequestRepo_nextSteps(t *testing.T) { fields{ userViewProvider: &mockViewUser{}, userEventProvider: &mockEventUser{ - Event: &es_models.Event{ - AggregateType: user_repo.AggregateType, - Typ: user_repo.UserLockedType, + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserLockedType, + }, }, }, orgViewProvider: &mockViewOrg{State: domain.OrgStateActive}, @@ -2290,10 +2291,12 @@ func Test_userSessionByIDs(t *testing.T) { agentID: "agentID", user: &user_model.UserView{ID: "id", HumanView: &user_model.HumanView{FirstName: "FirstName"}}, eventProvider: &mockEventUser{ - Event: &es_models.Event{ - AggregateType: user_repo.AggregateType, - Typ: user_repo.UserV1MFAOTPCheckSucceededType, - CreationDate: testNow, + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserV1MFAOTPCheckSucceededType, + CreationDate: testNow, + }, }, }, }, @@ -2313,14 +2316,16 @@ func Test_userSessionByIDs(t *testing.T) { agentID: "agentID", user: &user_model.UserView{ID: "id"}, eventProvider: &mockEventUser{ - Event: &es_models.Event{ - AggregateType: user_repo.AggregateType, - Typ: user_repo.UserV1MFAOTPCheckSucceededType, - CreationDate: testNow, - Data: func() []byte { - data, _ := json.Marshal(&user_es_model.AuthRequest{UserAgentID: "otherID"}) - return data - }(), + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserV1MFAOTPCheckSucceededType, + CreationDate: testNow, + Data: func() []byte { + data, _ := json.Marshal(&user_es_model.AuthRequest{UserAgentID: "otherID"}) + return data + }(), + }, }, }, }, @@ -2340,14 +2345,16 @@ func Test_userSessionByIDs(t *testing.T) { agentID: "agentID", user: &user_model.UserView{ID: "id", HumanView: &user_model.HumanView{FirstName: "FirstName"}}, eventProvider: &mockEventUser{ - Event: &es_models.Event{ - AggregateType: user_repo.AggregateType, - Typ: user_repo.UserV1MFAOTPCheckSucceededType, - CreationDate: testNow, - Data: func() []byte { - data, _ := json.Marshal(&user_es_model.AuthRequest{UserAgentID: "agentID"}) - return data - }(), + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserV1MFAOTPCheckSucceededType, + CreationDate: testNow, + Data: func() []byte { + data, _ := json.Marshal(&user_es_model.AuthRequest{UserAgentID: "agentID"}) + return data + }(), + }, }, }, }, @@ -2359,7 +2366,7 @@ func Test_userSessionByIDs(t *testing.T) { nil, }, { - "new user events (user deleted), precondition failed error", + "new user events (user deleted), session terminated", args{ userProvider: &mockViewUserSession{ PasswordVerification: testNow, @@ -2367,14 +2374,57 @@ func Test_userSessionByIDs(t *testing.T) { agentID: "agentID", user: &user_model.UserView{ID: "id"}, eventProvider: &mockEventUser{ - Event: &es_models.Event{ - AggregateType: user_repo.AggregateType, - Typ: user_repo.UserRemovedType, + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserRemovedType, + CreationDate: testNow, + }, }, }, }, + &user_model.UserSessionView{ + ChangeDate: testNow, + State: domain.UserSessionStateTerminated, + }, + nil, + }, + { + "new user events (user deleted, readded and password checked)", + args{ + userProvider: &mockViewUserSession{ + PasswordVerification: testNow, + }, + agentID: "agentID", + user: &user_model.UserView{ID: "id"}, + eventProvider: &mockEventUser{ + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserRemovedType, + }, + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.HumanAddedType, + }, + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.HumanPasswordCheckSucceededType, + CreationDate: testNow, + Data: func() []byte { + data, _ := json.Marshal(&user_es_model.AuthRequest{UserAgentID: "agentID"}) + return data + }(), + }, + }, + }, + }, + &user_model.UserSessionView{ + ChangeDate: testNow, + PasswordVerification: testNow, + State: domain.UserSessionStateActive, + }, nil, - zerrors.IsPreconditionFailed, }, } for _, tt := range tests { @@ -2456,14 +2506,16 @@ func Test_userByID(t *testing.T) { PasswordChangeRequired: true, }, eventProvider: &mockEventUser{ - Event: &es_models.Event{ - AggregateType: user_repo.AggregateType, - Typ: user_repo.UserV1PasswordChangedType, - CreationDate: testNow, - Data: func() []byte { - data, _ := json.Marshal(user_es_model.Password{ChangeRequired: false, Secret: &crypto.CryptoValue{}}) - return data - }(), + Events: []eventstore.Event{ + &es_models.Event{ + AggregateType: user_repo.AggregateType, + Typ: user_repo.UserV1PasswordChangedType, + CreationDate: testNow, + Data: func() []byte { + data, _ := json.Marshal(user_es_model.Password{ChangeRequired: false, Secret: &crypto.CryptoValue{}}) + return data + }(), + }, }, }, }, @@ -2552,6 +2604,7 @@ func TestAuthRequestRepo_VerifyPassword_IgnoreUnknownUsernames(t *testing.T) { a.SetPolicyOrgID("instance1") return a } + type fields struct { AuthRequests func(*testing.T, string) cache.AuthRequestCache UserViewProvider userViewProvider diff --git a/internal/user/repository/view/model/user_session.go b/internal/user/repository/view/model/user_session.go index d76159255..48eafd50a 100644 --- a/internal/user/repository/view/model/user_session.go +++ b/internal/user/repository/view/model/user_session.go @@ -199,7 +199,8 @@ func (v *UserSessionView) AppendEvent(event eventstore.Event) error { case user.UserV1SignedOutType, user.HumanSignedOutType, user.UserLockedType, - user.UserDeactivatedType: + user.UserDeactivatedType, + user.UserRemovedType: v.PasswordlessVerification = sql.NullTime{Time: time.Time{}, Valid: true} v.PasswordVerification = sql.NullTime{Time: time.Time{}, Valid: true} v.SecondFactorVerification = sql.NullTime{Time: time.Time{}, Valid: true}