From cc612fed07de3acece5f93cb4ae6122340550680 Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Fri, 19 Aug 2022 15:00:14 +0200 Subject: [PATCH] fix: trim spaces for usernames and organization names (#4217) --- .../eventsourcing/eventstore/auth_request.go | 4 +- internal/command/org.go | 5 +- internal/command/org_test.go | 135 +++++++++++++++++- internal/command/user.go | 2 + internal/command/user_human.go | 7 +- internal/command/user_human_test.go | 70 +++++++++ internal/command/user_test.go | 110 ++++++++++++++ internal/domain/org.go | 8 +- 8 files changed, 334 insertions(+), 7 deletions(-) diff --git a/internal/auth/repository/eventsourcing/eventstore/auth_request.go b/internal/auth/repository/eventsourcing/eventstore/auth_request.go index 3bbc75078b..568fac2a09 100644 --- a/internal/auth/repository/eventsourcing/eventstore/auth_request.go +++ b/internal/auth/repository/eventsourcing/eventstore/auth_request.go @@ -2,6 +2,7 @@ package eventstore import ( "context" + "strings" "time" "github.com/zitadel/logging" @@ -628,6 +629,7 @@ func (repo *AuthRequestRepo) tryUsingOnlyUserSession(request *domain.AuthRequest func (repo *AuthRequestRepo) checkLoginName(ctx context.Context, request *domain.AuthRequest, loginName string) (err error) { var user *user_view_model.UserView + loginName = strings.TrimSpace(loginName) preferredLoginName := loginName if request.RequestedOrgID != "" { if request.RequestedOrgID != "" { @@ -662,7 +664,7 @@ func (repo *AuthRequestRepo) checkLoginName(ctx context.Context, request *domain return nil } -func (repo AuthRequestRepo) checkLoginPolicyWithResourceOwner(ctx context.Context, request *domain.AuthRequest, user *user_view_model.UserView) error { +func (repo *AuthRequestRepo) checkLoginPolicyWithResourceOwner(ctx context.Context, request *domain.AuthRequest, user *user_view_model.UserView) error { loginPolicy, idpProviders, err := repo.getLoginPolicyAndIDPProviders(ctx, user.ResourceOwner) if err != nil { return err diff --git a/internal/command/org.go b/internal/command/org.go index d40c7eec36..76d60c1c4f 100644 --- a/internal/command/org.go +++ b/internal/command/org.go @@ -135,7 +135,7 @@ func (c *Commands) AddOrgWithID(ctx context.Context, name, userID, resourceOwner } func (c *Commands) AddOrg(ctx context.Context, name, userID, resourceOwner string, claimedUserIDs []string) (*domain.Org, error) { - if name == "" { + if name = strings.TrimSpace(name); name == "" { return nil, caos_errs.ThrowInvalidArgument(nil, "EVENT-Mf9sd", "Errors.Org.Invalid") } @@ -174,6 +174,7 @@ func (c *Commands) addOrgWithIDAndMember(ctx context.Context, name, userID, reso } func (c *Commands) ChangeOrg(ctx context.Context, orgID, name string) (*domain.ObjectDetails, error) { + name = strings.TrimSpace(name) if orgID == "" || name == "" { return nil, caos_errs.ThrowInvalidArgument(nil, "EVENT-Mf9sd", "Errors.Org.Invalid") } @@ -186,7 +187,7 @@ func (c *Commands) ChangeOrg(ctx context.Context, orgID, name string) (*domain.O return nil, caos_errs.ThrowNotFound(nil, "ORG-1MRds", "Errors.Org.NotFound") } if orgWriteModel.Name == name { - return nil, caos_errs.ThrowNotFound(nil, "ORG-4VSdf", "Errors.Org.NotChanged") + return nil, caos_errs.ThrowPreconditionFailed(nil, "ORG-4VSdf", "Errors.Org.NotChanged") } orgAgg := OrgAggregateFromWriteModel(&orgWriteModel.WriteModel) events := make([]eventstore.Command, 0) diff --git a/internal/command/org_test.go b/internal/command/org_test.go index ab9f90f8eb..1f7e04f4b3 100644 --- a/internal/command/org_test.go +++ b/internal/command/org_test.go @@ -106,6 +106,23 @@ func TestCommandSide_AddOrg(t *testing.T) { err: errors.IsErrorInvalidArgument, }, }, + { + name: "invalid org (spaces), error", + fields: fields{ + eventstore: eventstoreExpect( + t, + ), + }, + args: args{ + ctx: context.Background(), + userID: "user1", + resourceOwner: "org1", + name: " ", + }, + res: res{ + err: errors.IsErrorInvalidArgument, + }, + }, { name: "user removed, error", fields: fields{ @@ -362,6 +379,82 @@ func TestCommandSide_AddOrg(t *testing.T) { }, }, }, + { + name: "add org (remove spaces), no error", + fields: fields{ + eventstore: eventstoreExpect( + t, + expectFilterOrgDomainNotFound(), + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username1", + "firstname1", + "lastname1", + "nickname1", + "displayname1", + language.German, + domain.GenderMale, + "email1", + true, + ), + ), + ), + expectFilterOrgMemberNotFound(), + expectPush( + []*repository.Event{ + eventFromEventPusher(org.NewOrgAddedEvent(context.Background(), + &org.NewAggregate("org2").Aggregate, + "Org", + )), + eventFromEventPusher(org.NewDomainAddedEvent(context.Background(), + &org.NewAggregate("org2").Aggregate, "org.iam-domain", + )), + eventFromEventPusher(org.NewDomainVerifiedEvent(context.Background(), + &org.NewAggregate("org2").Aggregate, + "org.iam-domain", + )), + eventFromEventPusher(org.NewDomainPrimarySetEvent(context.Background(), + &org.NewAggregate("org2").Aggregate, + "org.iam-domain", + )), + eventFromEventPusher(org.NewMemberAddedEvent(context.Background(), + &org.NewAggregate("org2").Aggregate, + "user1", + domain.RoleOrgOwner, + )), + }, + uniqueConstraintsFromEventConstraint(org.NewAddOrgNameUniqueConstraint("Org")), + uniqueConstraintsFromEventConstraint(org.NewAddOrgDomainUniqueConstraint("org.iam-domain")), + uniqueConstraintsFromEventConstraint(member.NewAddMemberUniqueConstraint("org2", "user1")), + ), + ), + idGenerator: id_mock.NewIDGeneratorExpectIDs(t, "org2"), + zitadelRoles: []authz.RoleMapping{ + { + Role: "ORG_OWNER", + }, + }, + }, + args: args{ + ctx: authz.WithRequestedDomain(context.Background(), "iam-domain"), + name: " Org ", + userID: "user1", + resourceOwner: "org1", + }, + res: res{ + want: &domain.Org{ + ObjectRoot: models.ObjectRoot{ + AggregateID: "org2", + ResourceOwner: "org2", + }, + Name: "Org", + State: domain.OrgStateActive, + PrimaryDomain: "org.iam-domain", + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -394,8 +487,7 @@ func TestCommandSide_ChangeOrg(t *testing.T) { name string } type res struct { - want *domain.Org - err func(error) bool + err func(error) bool } tests := []struct { name string @@ -418,6 +510,22 @@ func TestCommandSide_ChangeOrg(t *testing.T) { err: errors.IsErrorInvalidArgument, }, }, + { + name: "empty name (spaces), invalid argument error", + fields: fields{ + eventstore: eventstoreExpect( + t, + ), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + name: " ", + }, + res: res{ + err: errors.IsErrorInvalidArgument, + }, + }, { name: "org not found, error", fields: fields{ @@ -435,6 +543,29 @@ func TestCommandSide_ChangeOrg(t *testing.T) { err: errors.IsNotFound, }, }, + { + name: "no change (spaces), error", + fields: fields{ + eventstore: eventstoreExpect( + t, + expectFilter( + eventFromEventPusher( + org.NewOrgAddedEvent(context.Background(), + &org.NewAggregate("org1").Aggregate, + "org"), + ), + ), + ), + }, + args: args{ + ctx: authz.WithRequestedDomain(context.Background(), "zitadel.ch"), + orgID: "org1", + name: " org ", + }, + res: res{ + err: errors.IsPreconditionFailed, + }, + }, { name: "push failed, error", fields: fields{ diff --git a/internal/command/user.go b/internal/command/user.go index d31727f150..a956138f97 100644 --- a/internal/command/user.go +++ b/internal/command/user.go @@ -3,6 +3,7 @@ package command import ( "context" "fmt" + "strings" "time" "github.com/zitadel/logging" @@ -19,6 +20,7 @@ import ( ) func (c *Commands) ChangeUsername(ctx context.Context, orgID, userID, userName string) (*domain.ObjectDetails, error) { + userName = strings.TrimSpace(userName) if orgID == "" || userID == "" || userName == "" { return nil, caos_errs.ThrowInvalidArgument(nil, "COMMAND-2N9fs", "Errors.IDMissing") } diff --git a/internal/command/user_human.go b/internal/command/user_human.go index b2efd78d42..fa7c985b5f 100644 --- a/internal/command/user_human.go +++ b/internal/command/user_human.go @@ -416,7 +416,10 @@ func (c *Commands) importHuman(ctx context.Context, orgID string, human *domain. } func (c *Commands) registerHuman(ctx context.Context, orgID string, human *domain.Human, link *domain.UserIDPLink, domainPolicy *domain.DomainPolicy, pwPolicy *domain.PasswordComplexityPolicy, initCodeGenerator crypto.Generator, phoneCodeGenerator crypto.Generator) ([]eventstore.Command, *HumanWriteModel, error) { - if human != nil && human.Username == "" { + if human == nil { + return nil, nil, errors.ThrowInvalidArgument(nil, "COMMAND-JKefw", "Errors.User.Invalid") + } + if human.Username = strings.TrimSpace(human.Username); human.Username == "" { human.Username = human.EmailAddress } if orgID == "" || !human.IsValid() || link == nil && (human.Password == nil || human.Password.SecretString == "") { @@ -432,6 +435,8 @@ func (c *Commands) createHuman(ctx context.Context, orgID string, human *domain. if err := human.CheckDomainPolicy(domainPolicy); err != nil { return nil, nil, err } + human.Username = strings.TrimSpace(human.Username) + human.EmailAddress = strings.TrimSpace(human.EmailAddress) if !domainPolicy.UserLoginMustBeDomain { usernameSplit := strings.Split(human.Username, "@") if len(usernameSplit) != 2 { diff --git a/internal/command/user_human_test.go b/internal/command/user_human_test.go index f618ba58c0..a84a09fa03 100644 --- a/internal/command/user_human_test.go +++ b/internal/command/user_human_test.go @@ -410,6 +410,76 @@ func TestCommandSide_AddHuman(t *testing.T) { }, }, }, + { + name: "add human email verified, trim spaces, ok", + fields: fields{ + eventstore: eventstoreExpect( + t, + expectFilter( + eventFromEventPusher( + org.NewDomainPolicyAddedEvent(context.Background(), + &userAgg.Aggregate, + true, + true, + true, + ), + ), + ), + expectFilter( + eventFromEventPusher( + org.NewPasswordComplexityPolicyAddedEvent(context.Background(), + &userAgg.Aggregate, + 1, + false, + false, + false, + false, + ), + ), + ), + expectPush( + []*repository.Event{ + eventFromEventPusher( + newAddHumanEvent("password", true, ""), + ), + eventFromEventPusher( + user.NewHumanEmailVerifiedEvent(context.Background(), + &userAgg.Aggregate), + ), + }, + uniqueConstraintsFromEventConstraint(user.NewAddUsernameUniqueConstraint("username", "org1", true)), + ), + ), + idGenerator: id_mock.NewIDGeneratorExpectIDs(t, "user1"), + userPasswordAlg: crypto.CreateMockHashAlg(gomock.NewController(t)), + codeAlg: crypto.CreateMockEncryptionAlg(gomock.NewController(t)), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + human: &AddHuman{ + Username: " username ", + Password: "password", + FirstName: "firstname", + LastName: "lastname", + Email: Email{ + Address: "email@test.ch", + Verified: true, + }, + PreferredLanguage: language.English, + PasswordChangeRequired: true, + }, + secretGenerator: GetMockSecretGenerator(t), + }, + res: res{ + want: &domain.HumanDetails{ + ID: "user1", + ObjectDetails: domain.ObjectDetails{ + ResourceOwner: "org1", + }, + }, + }, + }, { name: "add human (with phone), ok", fields: fields{ diff --git a/internal/command/user_test.go b/internal/command/user_test.go index 383d7205df..aa43a5a0c6 100644 --- a/internal/command/user_test.go +++ b/internal/command/user_test.go @@ -96,6 +96,23 @@ func TestCommandSide_UsernameChange(t *testing.T) { err: caos_errs.IsErrorInvalidArgument, }, }, + { + name: "username only spaces, invalid argument error", + fields: fields{ + eventstore: eventstoreExpect( + t, + ), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + userID: "user1", + username: " ", + }, + res: res{ + err: caos_errs.IsErrorInvalidArgument, + }, + }, { name: "user removed, not found error", fields: fields{ @@ -147,6 +164,39 @@ func TestCommandSide_UsernameChange(t *testing.T) { err: caos_errs.IsPreconditionFailed, }, }, + { + name: "username not changed (spaces), precondition error", + fields: fields{ + eventstore: eventstoreExpect( + t, + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "firstname", + "lastname", + "nickname", + "displayname", + language.German, + domain.GenderUnspecified, + "email@test.ch", + true, + ), + ), + ), + ), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + userID: "user1", + username: "username ", + }, + res: res{ + err: caos_errs.IsPreconditionFailed, + }, + }, { name: "org iam policy not found, precondition error", fields: fields{ @@ -284,6 +334,66 @@ func TestCommandSide_UsernameChange(t *testing.T) { }, }, }, + { + name: "change username (remove spaces), ok", + fields: fields{ + eventstore: eventstoreExpect( + t, + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "firstname", + "lastname", + "nickname", + "displayname", + language.German, + domain.GenderUnspecified, + "email@test.ch", + true, + ), + ), + ), + expectFilter(), + expectFilter( + eventFromEventPusher( + instance.NewDomainPolicyAddedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + true, + true, + true, + ), + ), + ), + expectPush( + []*repository.Event{ + eventFromEventPusher( + user.NewUsernameChangedEvent(context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "username", + "username1", + true, + ), + ), + }, + uniqueConstraintsFromEventConstraint(user.NewRemoveUsernameUniqueConstraint("username", "org1", true)), + uniqueConstraintsFromEventConstraint(user.NewAddUsernameUniqueConstraint("username1", "org1", true)), + ), + ), + }, + args: args{ + ctx: context.Background(), + orgID: "org1", + userID: "user1", + username: "username1 ", + }, + res: res{ + want: &domain.ObjectDetails{ + ResourceOwner: "org1", + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/domain/org.go b/internal/domain/org.go index e45143ba2f..c5375e83cd 100644 --- a/internal/domain/org.go +++ b/internal/domain/org.go @@ -1,6 +1,8 @@ package domain import ( + "strings" + "github.com/zitadel/zitadel/internal/eventstore/v1/models" ) @@ -15,7 +17,11 @@ type Org struct { } func (o *Org) IsValid() bool { - return o != nil && o.Name != "" + if o == nil { + return false + } + o.Name = strings.TrimSpace(o.Name) + return o.Name != "" } func (o *Org) AddIAMDomain(iamDomain string) {