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 <silvan.reusser@gmail.com>
This commit is contained in:
Livio Spring 2024-08-15 07:39:54 +02:00 committed by GitHub
parent 5fab533e37
commit 0af37d45e9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 99 additions and 47 deletions

View File

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

View File

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

View File

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