From 21f1d2aabc9bf2821f4d70207f541ad23d5d2cff Mon Sep 17 00:00:00 2001 From: mkraft Date: Tue, 8 Mar 2022 09:14:22 -0500 Subject: [PATCH] MM-42201: Enforce group is not the same as username. (#19660) * MM-42201: Enforce group is not the same as username. * MM-42201: Adjust test for change to translation id. --- api4/group_test.go | 10 ++++++++++ app/group.go | 30 ++++++++++++++++++++++++++++++ app/group_test.go | 19 +++++++++++++++++++ app/user.go | 27 +++++++++++++++++++++++++++ app/user_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ i18n/en.json | 16 ++++++++++++++++ model/group.go | 7 +++++++ 7 files changed, 149 insertions(+) diff --git a/api4/group_test.go b/api4/group_test.go index 6f870d698c..14688fc474 100644 --- a/api4/group_test.go +++ b/api4/group_test.go @@ -119,6 +119,16 @@ func TestCreateGroup(t *testing.T) { require.NoError(t, err) CheckCreatedStatus(t, response) + usernameGroup := &model.Group{ + DisplayName: "dn_" + model.NewId(), + Name: &th.BasicUser.Username, + Source: model.GroupSourceCustom, + AllowReference: true, + } + _, response, err = th.SystemAdminClient.CreateGroup(usernameGroup) + require.Error(t, err) + CheckBadRequestStatus(t, response) + unReferenceableCustomGroup := &model.Group{ DisplayName: "dn_" + model.NewId(), Name: model.NewString("name" + model.NewId()), diff --git a/app/group.go b/app/group.go index f0f9e8482f..b00a017fa3 100644 --- a/app/group.go +++ b/app/group.go @@ -85,6 +85,11 @@ func (a *App) GetGroupsByUserId(userID string) ([]*model.Group, *model.AppError) } func (a *App) CreateGroup(group *model.Group) (*model.Group, *model.AppError) { + if err := a.isUniqueToUsernames(group.GetName()); err != nil { + err.Where = "CreateGroup" + return nil, err + } + group, err := a.Srv().Store.Group().Create(group) if err != nil { var invErr *store.ErrInvalidInput @@ -102,7 +107,27 @@ func (a *App) CreateGroup(group *model.Group) (*model.Group, *model.AppError) { return group, nil } +func (a *App) isUniqueToUsernames(val string) *model.AppError { + if val == "" { + return nil + } + var notFoundErr *store.ErrNotFound + user, err := a.Srv().Store.User().GetByUsername(val) + if err != nil && !errors.As(err, ¬FoundErr) { + return model.NewAppError("", "app.group.get_by_username_failure", nil, err.Error(), http.StatusInternalServerError) + } + if user != nil { + return model.NewAppError("", "app.group.username_conflict", nil, "", http.StatusBadRequest) + } + return nil +} + func (a *App) CreateGroupWithUserIds(group *model.GroupWithUserIds) (*model.Group, *model.AppError) { + if err := a.isUniqueToUsernames(group.GetName()); err != nil { + err.Where = "CreateGroupWithUserIds" + return nil, err + } + newGroup, err := a.Srv().Store.Group().CreateWithUserIds(group) if err != nil { var invErr *store.ErrInvalidInput @@ -137,6 +162,11 @@ func (a *App) CreateGroupWithUserIds(group *model.GroupWithUserIds) (*model.Grou } func (a *App) UpdateGroup(group *model.Group) (*model.Group, *model.AppError) { + if err := a.isUniqueToUsernames(group.GetName()); err != nil { + err.Where = "UpdateGroup" + return nil, err + } + updatedGroup, err := a.Srv().Store.Group().Update(group) if err == nil { diff --git a/app/group_test.go b/app/group_test.go index 2438689d58..90b6161c9a 100644 --- a/app/group_test.go +++ b/app/group_test.go @@ -83,6 +83,18 @@ func TestCreateGroup(t *testing.T) { g, err = th.App.CreateGroup(group) require.NotNil(t, err) require.Nil(t, g) + + user := th.CreateUser() + usernameGroup := &model.Group{ + DisplayName: "dn_" + model.NewId(), + Name: &user.Username, + Source: model.GroupSourceLdap, + RemoteId: model.NewString(model.NewId()), + } + g, err = th.App.CreateGroup(usernameGroup) + require.NotNil(t, err) + require.Equal(t, "app.group.username_conflict", err.Id) + require.Nil(t, g) } func TestUpdateGroup(t *testing.T) { @@ -94,6 +106,13 @@ func TestUpdateGroup(t *testing.T) { g, err := th.App.UpdateGroup(group) require.Nil(t, err) require.NotNil(t, g) + + user := th.CreateUser() + g.Name = &user.Username + g, err = th.App.UpdateGroup(g) + require.NotNil(t, err) + require.Equal(t, "app.group.username_conflict", err.Id) + require.Nil(t, g) } func TestDeleteGroup(t *testing.T) { diff --git a/app/user.go b/app/user.go index 1c90505f77..0b5b3b0551 100644 --- a/app/user.go +++ b/app/user.go @@ -219,6 +219,11 @@ func (a *App) CreateGuest(c *request.Context, user *model.User) (*model.User, *m } func (a *App) createUserOrGuest(c *request.Context, user *model.User, guest bool) (*model.User, *model.AppError) { + if err := a.isUniqueToGroupNames(user.Username); err != nil { + err.Where = "createUserOrGuest" + return nil, err + } + ruser, nErr := a.ch.srv.userService.CreateUser(user, users.UserCreateOptions{Guest: guest}) if nErr != nil { var appErr *model.AppError @@ -1053,6 +1058,21 @@ func (a *App) sendUpdatedUserEvent(user model.User) { a.Publish(sourceUserMessage) } +func (a *App) isUniqueToGroupNames(val string) *model.AppError { + if val == "" { + return nil + } + var notFoundErr *store.ErrNotFound + group, err := a.Srv().Store.Group().GetByName(val, model.GroupSearchOpts{}) + if err != nil && !errors.As(err, ¬FoundErr) { + return model.NewAppError("", "app.user.get_by_name_failure", nil, err.Error(), http.StatusInternalServerError) + } + if group != nil { + return model.NewAppError("", "app.user.group_name_conflict", nil, "", http.StatusBadRequest) + } + return nil +} + func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User, *model.AppError) { prev, err := a.ch.srv.userService.GetUser(user.Id) if err != nil { @@ -1065,6 +1085,13 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User, } } + if user.Username != prev.Username { + if err := a.isUniqueToGroupNames(user.Username); err != nil { + err.Where = "UpdateUser" + return nil, err + } + } + var newEmail string if user.Email != prev.Email { if !users.CheckUserDomain(user, *a.Config().TeamSettings.RestrictCreationToDomains) { diff --git a/app/user_test.go b/app/user_test.go index 07da29f972..85b50bc910 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -180,6 +180,46 @@ func TestUpdateUserToRestrictedDomain(t *testing.T) { }) } +func TestUpdateUser(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + user := th.CreateUser() + group := th.CreateGroup() + + t.Run("fails if the username matches a group name", func(t *testing.T) { + user.Username = *group.Name + u, err := th.App.UpdateUser(user, false) + require.NotNil(t, err) + require.Equal(t, "app.user.group_name_conflict", err.Id) + require.Nil(t, u) + }) +} + +func TestCreateUser(t *testing.T) { + th := Setup(t) + defer th.TearDown() + + group := th.CreateGroup() + + id := model.NewId() + user := &model.User{ + Email: "success+" + id + "@simulator.amazonses.com", + Username: *group.Name, + Nickname: "nn_" + id, + Password: "Password1", + EmailVerified: true, + } + + t.Run("fails if the username matches a group name", func(t *testing.T) { + user.Username = *group.Name + u, err := th.App.CreateUser(th.Context, user) + require.NotNil(t, err) + require.Equal(t, "app.user.group_name_conflict", err.Id) + require.Nil(t, u) + }) +} + func TestUpdateUserActive(t *testing.T) { th := Setup(t) defer th.TearDown() diff --git a/i18n/en.json b/i18n/en.json index b367792d7a..d7febf48ed 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4931,6 +4931,10 @@ "id": "app.group.crud_permission", "translation": "Unable to perform operation for that source type." }, + { + "id": "app.group.get_by_username_failure", + "translation": " " + }, { "id": "app.group.group_syncable_already_deleted", "translation": "group syncable was already deleted" @@ -4951,6 +4955,10 @@ "id": "app.group.uniqueness_error", "translation": "group member already exists" }, + { + "id": "app.group.username_conflict", + "translation": " " + }, { "id": "app.import.attachment.bad_file.error", "translation": "Error reading the file at: \"{{.FilePath}}\"" @@ -6467,6 +6475,10 @@ "id": "app.user.get_by_auth.other.app_error", "translation": "We encountered an error trying to find the account by authentication type." }, + { + "id": "app.user.get_by_name_failure", + "translation": " " + }, { "id": "app.user.get_by_username.app_error", "translation": "Unable to find an existing account matching your username for this team. This team may require an invite from the team owner to join." @@ -6523,6 +6535,10 @@ "id": "app.user.get_users_batch_for_indexing.get_users.app_error", "translation": "Unable to get the users batch for indexing." }, + { + "id": "app.user.group_name_conflict", + "translation": " " + }, { "id": "app.user.missing_account.const", "translation": "Unable to find the user." diff --git a/model/group.go b/model/group.go index 1e56dc283a..428c431a67 100644 --- a/model/group.go +++ b/model/group.go @@ -211,6 +211,13 @@ func (group *Group) IsValidName() *AppError { return nil } +func (group *Group) GetName() string { + if group.Name == nil { + return "" + } + return *group.Name +} + func (group *Group) GetRemoteId() string { if group.RemoteId == nil { return ""