diff --git a/api4/group.go b/api4/group.go index b4470e1888..568f851e2b 100644 --- a/api4/group.go +++ b/api4/group.go @@ -137,12 +137,32 @@ func patchGroup(c *Context, w http.ResponseWriter, r *http.Request) { auditRec.AddMeta("group", group) if groupPatch.AllowReference != nil && *groupPatch.AllowReference { - tmp := model.NewId() if groupPatch.Name == nil { - tmp = strings.ReplaceAll(strings.ToLower(group.DisplayName), " ", "-") + tmp := strings.ReplaceAll(strings.ToLower(group.DisplayName), " ", "-") + groupPatch.Name = &tmp + } else { + if *groupPatch.Name == model.USER_NOTIFY_ALL || *groupPatch.Name == model.CHANNEL_MENTIONS_NOTIFY_PROP || *groupPatch.Name == model.USER_NOTIFY_HERE { + c.Err = model.NewAppError("Api4.patchGroup", "api.ldap_groups.existing_reserved_name_error", nil, "", http.StatusNotImplemented) + return + } + //check if a user already has this group name + user, _ := c.App.GetUserByUsername(*groupPatch.Name) + if user != nil { + c.Err = model.NewAppError("Api4.patchGroup", "api.ldap_groups.existing_user_name_error", nil, "", http.StatusNotImplemented) + return + } + //check if a mentionable group already has this name + searchOpts := model.GroupSearchOpts{ + FilterAllowReference: true, + } + existingGroup, _ := c.App.GetGroupByName(*groupPatch.Name, searchOpts) + if existingGroup != nil { + c.Err = model.NewAppError("Api4.patchGroup", "api.ldap_groups.existing_group_name_error", nil, "", http.StatusNotImplemented) + return + } } - groupPatch.Name = &tmp } + group.Patch(groupPatch) group, err = c.App.UpdateGroup(group) diff --git a/app/app_iface.go b/app/app_iface.go index 9de2a74a43..c17a2593e7 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -537,7 +537,7 @@ type AppIface interface { GetFlaggedPostsForChannel(userId, channelId string, offset int, limit int) (*model.PostList, *model.AppError) GetFlaggedPostsForTeam(userId, teamId string, offset int, limit int) (*model.PostList, *model.AppError) GetGroup(id string) (*model.Group, *model.AppError) - GetGroupByName(name string) (*model.Group, *model.AppError) + GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) GetGroupByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, *model.AppError) GetGroupChannel(userIds []string) (*model.Channel, *model.AppError) GetGroupMemberUsers(groupID string) ([]*model.User, *model.AppError) diff --git a/app/group.go b/app/group.go index 9bed101e6f..d30da09c89 100644 --- a/app/group.go +++ b/app/group.go @@ -11,8 +11,8 @@ func (a *App) GetGroup(id string) (*model.Group, *model.AppError) { return a.Srv().Store.Group().Get(id) } -func (a *App) GetGroupByName(name string) (*model.Group, *model.AppError) { - return a.Srv().Store.Group().GetByName(name) +func (a *App) GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { + return a.Srv().Store.Group().GetByName(name, opts) } func (a *App) GetGroupByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, *model.AppError) { diff --git a/app/opentracing_layer.go b/app/opentracing_layer.go index dbb8e3318a..52114f272e 100644 --- a/app/opentracing_layer.go +++ b/app/opentracing_layer.go @@ -5236,7 +5236,7 @@ func (a *OpenTracingAppLayer) GetGroup(id string) (*model.Group, *model.AppError return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) GetGroupByName(name string) (*model.Group, *model.AppError) { +func (a *OpenTracingAppLayer) GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetGroupByName") @@ -5248,7 +5248,7 @@ func (a *OpenTracingAppLayer) GetGroupByName(name string) (*model.Group, *model. }() defer span.Finish() - resultVar0, resultVar1 := a.app.GetGroupByName(name) + resultVar0, resultVar1 := a.app.GetGroupByName(name, opts) if resultVar1 != nil { span.LogFields(spanlog.Error(resultVar1)) diff --git a/app/plugin_api.go b/app/plugin_api.go index c1653e426c..14a6028236 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -459,8 +459,8 @@ func (api *PluginAPI) GetGroup(groupId string) (*model.Group, *model.AppError) { return api.app.GetGroup(groupId) } -func (api *PluginAPI) GetGroupByName(name string) (*model.Group, *model.AppError) { - return api.app.GetGroupByName(name) +func (api *PluginAPI) GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { + return api.app.GetGroupByName(name, opts) } func (api *PluginAPI) GetGroupsForUser(userId string) ([]*model.Group, *model.AppError) { diff --git a/i18n/en.json b/i18n/en.json index 1540928821..f3491e69f0 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1420,6 +1420,18 @@ "id": "api.ldap_group.not_found", "translation": "ldap group not found" }, + { + "id": "api.ldap_groups.existing_group_name_error", + "translation": "group name already exists" + }, + { + "id": "api.ldap_groups.existing_reserved_name_error", + "translation": "group name already exists as a reserved name" + }, + { + "id": "api.ldap_groups.existing_user_name_error", + "translation": "group name already exists as a user name" + }, { "id": "api.ldap_groups.license_error", "translation": "your license does not support ldap groups" @@ -5110,6 +5122,10 @@ "id": "model.group.name.app_error", "translation": "invalid name property for group." }, + { + "id": "model.group.name.invalid_chars.app_error", + "translation": "invalid characters in the name property for group" + }, { "id": "model.group.remote_id.app_error", "translation": "invalid remote id property for group." diff --git a/model/group.go b/model/group.go index aceddcf104..daf5a3cb54 100644 --- a/model/group.go +++ b/model/group.go @@ -7,6 +7,7 @@ import ( "encoding/json" "io" "net/http" + "regexp" ) const ( @@ -103,8 +104,9 @@ func (group *Group) Patch(patch *GroupPatch) { } func (group *Group) IsValidForCreate() *AppError { - if l := len(group.Name); l == 0 || l > GroupNameMaxLength { - return NewAppError("Group.IsValidForCreate", "model.group.name.app_error", map[string]interface{}{"GroupNameMaxLength": GroupNameMaxLength}, "", http.StatusBadRequest) + err := group.IsValidName() + if err != nil { + return err } if l := len(group.DisplayName); l == 0 || l > GroupDisplayNameMaxLength { @@ -158,6 +160,20 @@ func (group *Group) IsValidForUpdate() *AppError { return nil } +var validGroupnameChars = regexp.MustCompile(`^[a-z0-9\.\-_]+$`) + +func (group *Group) IsValidName() *AppError { + if l := len(group.Name); l == 0 || l > GroupNameMaxLength { + return NewAppError("Group.IsValidName", "model.group.name.app_error", map[string]interface{}{"GroupNameMaxLength": GroupNameMaxLength}, "", http.StatusBadRequest) + } + + if !validGroupnameChars.MatchString(group.Name) { + return NewAppError("Group.IsValidName", "model.group.name.invalid_chars.app_error", nil, "", http.StatusBadRequest) + } + + return nil +} + func GroupFromJson(data io.Reader) *Group { var group *Group json.NewDecoder(data).Decode(&group) diff --git a/model/user.go b/model/user.go index 91ec785954..168605adb2 100644 --- a/model/user.go +++ b/model/user.go @@ -25,6 +25,7 @@ import ( const ( ME = "me" USER_NOTIFY_ALL = "all" + USER_NOTIFY_HERE = "here" USER_NOTIFY_MENTION = "mention" USER_NOTIFY_NONE = "none" DESKTOP_NOTIFY_PROP = "desktop" diff --git a/plugin/api.go b/plugin/api.go index 7334165c27..df46dc02f8 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -490,8 +490,8 @@ type API interface { // GetGroupByName gets a group by name. // // @tag Group - // Minimum server version: 5.18 - GetGroupByName(name string) (*model.Group, *model.AppError) + // Minimum server version: 5.24 + GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) // GetGroupsForUser gets the groups a user is in. // diff --git a/plugin/api_timer_layer_generated.go b/plugin/api_timer_layer_generated.go index 4943062a70..8036931144 100644 --- a/plugin/api_timer_layer_generated.go +++ b/plugin/api_timer_layer_generated.go @@ -532,9 +532,9 @@ func (api *apiTimerLayer) GetGroup(groupId string) (*model.Group, *model.AppErro return _returnsA, _returnsB } -func (api *apiTimerLayer) GetGroupByName(name string) (*model.Group, *model.AppError) { +func (api *apiTimerLayer) GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { startTime := timePkg.Now() - _returnsA, _returnsB := api.apiImpl.GetGroupByName(name) + _returnsA, _returnsB := api.apiImpl.GetGroupByName(name, opts) api.recordTime(startTime, "GetGroupByName", true) return _returnsA, _returnsB } diff --git a/plugin/client_rpc_generated.go b/plugin/client_rpc_generated.go index 7c2ae442e5..17a259a3e4 100644 --- a/plugin/client_rpc_generated.go +++ b/plugin/client_rpc_generated.go @@ -2559,6 +2559,7 @@ func (s *apiRPCServer) GetGroup(args *Z_GetGroupArgs, returns *Z_GetGroupReturns type Z_GetGroupByNameArgs struct { A string + B model.GroupSearchOpts } type Z_GetGroupByNameReturns struct { @@ -2566,8 +2567,8 @@ type Z_GetGroupByNameReturns struct { B *model.AppError } -func (g *apiRPCClient) GetGroupByName(name string) (*model.Group, *model.AppError) { - _args := &Z_GetGroupByNameArgs{name} +func (g *apiRPCClient) GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { + _args := &Z_GetGroupByNameArgs{name, opts} _returns := &Z_GetGroupByNameReturns{} if err := g.client.Call("Plugin.GetGroupByName", _args, _returns); err != nil { log.Printf("RPC call to GetGroupByName API failed: %s", err.Error()) @@ -2577,9 +2578,9 @@ func (g *apiRPCClient) GetGroupByName(name string) (*model.Group, *model.AppErro func (s *apiRPCServer) GetGroupByName(args *Z_GetGroupByNameArgs, returns *Z_GetGroupByNameReturns) error { if hook, ok := s.impl.(interface { - GetGroupByName(name string) (*model.Group, *model.AppError) + GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) }); ok { - returns.A, returns.B = hook.GetGroupByName(args.A) + returns.A, returns.B = hook.GetGroupByName(args.A, args.B) } else { return encodableError(fmt.Errorf("API GetGroupByName called but not implemented.")) } diff --git a/plugin/plugintest/api.go b/plugin/plugintest/api.go index 51fa5b3416..8bf095173f 100644 --- a/plugin/plugintest/api.go +++ b/plugin/plugintest/api.go @@ -1073,13 +1073,13 @@ func (_m *API) GetGroup(groupId string) (*model.Group, *model.AppError) { return r0, r1 } -// GetGroupByName provides a mock function with given fields: name -func (_m *API) GetGroupByName(name string) (*model.Group, *model.AppError) { - ret := _m.Called(name) +// GetGroupByName provides a mock function with given fields: name, opts +func (_m *API) GetGroupByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { + ret := _m.Called(name, opts) var r0 *model.Group - if rf, ok := ret.Get(0).(func(string) *model.Group); ok { - r0 = rf(name) + if rf, ok := ret.Get(0).(func(string, model.GroupSearchOpts) *model.Group); ok { + r0 = rf(name, opts) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.Group) @@ -1087,8 +1087,8 @@ func (_m *API) GetGroupByName(name string) (*model.Group, *model.AppError) { } var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { - r1 = rf(name) + if rf, ok := ret.Get(1).(func(string, model.GroupSearchOpts) *model.AppError); ok { + r1 = rf(name, opts) } else { if ret.Get(1) != nil { r1 = ret.Get(1).(*model.AppError) diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index 08752b2593..020e08778f 100644 --- a/store/opentracing_layer.go +++ b/store/opentracing_layer.go @@ -3180,7 +3180,7 @@ func (s *OpenTracingLayerGroupStore) GetByIDs(groupIDs []string) ([]*model.Group return resultVar0, resultVar1 } -func (s *OpenTracingLayerGroupStore) GetByName(name string) (*model.Group, *model.AppError) { +func (s *OpenTracingLayerGroupStore) GetByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "GroupStore.GetByName") s.Root.Store.SetContext(newCtx) @@ -3189,7 +3189,7 @@ func (s *OpenTracingLayerGroupStore) GetByName(name string) (*model.Group, *mode }() defer span.Finish() - resultVar0, resultVar1 := s.GroupStore.GetByName(name) + resultVar0, resultVar1 := s.GroupStore.GetByName(name, opts) if resultVar1 != nil { span.LogFields(spanlog.Error(resultVar1)) ext.Error.Set(span, true) diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index 98cbee181a..008e5bf99c 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -125,9 +125,19 @@ func (s *SqlGroupStore) Get(groupId string) (*model.Group, *model.AppError) { return group, nil } -func (s *SqlGroupStore) GetByName(name string) (*model.Group, *model.AppError) { +func (s *SqlGroupStore) GetByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { var group *model.Group - if err := s.GetReplica().SelectOne(&group, "SELECT * from UserGroups WHERE Name = :Name", map[string]interface{}{"Name": name}); err != nil { + query := s.getQueryBuilder().Select("*").From("UserGroups").Where(sq.Eq{"Name": name}) + if opts.FilterAllowReference { + query = query.Where("AllowReference = true") + } + + queryString, args, err := query.ToSql() + + if err != nil { + return nil, model.NewAppError("SqlGroupStore.GetByName", "store.sql_group.app_error", nil, err.Error(), http.StatusInternalServerError) + } + if err := s.GetReplica().SelectOne(&group, queryString, args...); err != nil { if err == sql.ErrNoRows { return nil, model.NewAppError("SqlGroupStore.GroupGetByName", "store.sql_group.no_rows", nil, err.Error(), http.StatusNotFound) } @@ -216,6 +226,9 @@ func (s *SqlGroupStore) Update(group *model.Group) (*model.Group, *model.AppErro rowsChanged, err := s.GetMaster().Update(group) if err != nil { + if IsUniqueConstraintError(err, []string{"Name", "groups_name_key"}) { + return nil, model.NewAppError("SqlGroupStore.GroupUpdate", "store.sql_group.unique_constraint", nil, err.Error(), http.StatusInternalServerError) + } return nil, model.NewAppError("SqlGroupStore.GroupUpdate", "store.update_error", nil, err.Error(), http.StatusInternalServerError) } if rowsChanged != 1 { @@ -993,8 +1006,8 @@ func (s *SqlGroupStore) getGroupsAssociatedToChannelsByTeam(st model.GroupSyncab query := s.getQueryBuilder(). Select("gc.ChannelId, ug.*, gc.SchemeAdmin AS SyncableSchemeAdmin"). From("UserGroups ug"). - LeftJoin(fmt.Sprintf(`( - SELECT + LeftJoin(` + (SELECT GroupChannels.GroupId, GroupChannels.ChannelId, GroupChannels.DeleteAt, GroupChannels.SchemeAdmin FROM GroupChannels @@ -1003,8 +1016,7 @@ func (s *SqlGroupStore) getGroupsAssociatedToChannelsByTeam(st model.GroupSyncab WHERE GroupChannels.DeleteAt = 0 AND Channels.DeleteAt = 0 - AND Channels.TeamId = ?) AS gc - ON gc.GroupId = ug.Id`), teamID). + AND Channels.TeamId = ?) AS gc ON gc.GroupId = ug.Id`, teamID). Where("ug.DeleteAt = 0 AND gc.DeleteAt = 0"). OrderBy("ug.DisplayName") @@ -1012,8 +1024,8 @@ func (s *SqlGroupStore) getGroupsAssociatedToChannelsByTeam(st model.GroupSyncab query = s.getQueryBuilder(). Select("gc.ChannelId, ug.*, coalesce(Members.MemberCount, 0) AS MemberCount, gc.SchemeAdmin AS SyncableSchemeAdmin"). From("UserGroups ug"). - LeftJoin(fmt.Sprintf(`( - SELECT + LeftJoin(` + (SELECT GroupChannels.ChannelId, GroupChannels.DeleteAt, GroupChannels.GroupId, GroupChannels.SchemeAdmin FROM GroupChannels @@ -1022,8 +1034,7 @@ func (s *SqlGroupStore) getGroupsAssociatedToChannelsByTeam(st model.GroupSyncab WHERE GroupChannels.DeleteAt = 0 AND Channels.DeleteAt = 0 - AND Channels.TeamId = ?) AS gc - ON gc.GroupId = ug.Id`), teamID). + AND Channels.TeamId = ?) AS gc ON gc.GroupId = ug.Id`, teamID). LeftJoin(`( SELECT GroupMembers.GroupId, COUNT(*) AS MemberCount diff --git a/store/store.go b/store/store.go index 4c22c31bb1..814ee7d392 100644 --- a/store/store.go +++ b/store/store.go @@ -624,7 +624,7 @@ type UserTermsOfServiceStore interface { type GroupStore interface { Create(group *model.Group) (*model.Group, *model.AppError) Get(groupID string) (*model.Group, *model.AppError) - GetByName(name string) (*model.Group, *model.AppError) + GetByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) GetByIDs(groupIDs []string) ([]*model.Group, *model.AppError) GetByRemoteID(remoteID string, groupSource model.GroupSource) (*model.Group, *model.AppError) GetAllBySource(groupSource model.GroupSource) ([]*model.Group, *model.AppError) diff --git a/store/storetest/group_store.go b/store/storetest/group_store.go index 6874cde2d4..42999dbd46 100644 --- a/store/storetest/group_store.go +++ b/store/storetest/group_store.go @@ -177,6 +177,16 @@ func testGroupStoreCreate(t *testing.T, ss store.Store) { RemoteId: model.NewId(), } require.Equal(t, g6.IsValidForCreate().Id, "model.group.source.app_error") + + //must use valid characters + g7 := &model.Group{ + Name: "%^#@$$", + DisplayName: model.NewId(), + Description: model.NewId(), + Source: model.GroupSourceLdap, + RemoteId: model.NewId(), + } + require.Equal(t, g7.IsValidForCreate().Id, "model.group.name.invalid_chars.app_error") } func testGroupStoreGet(t *testing.T, ss store.Store) { @@ -219,12 +229,16 @@ func testGroupStoreGetByName(t *testing.T, ss store.Store) { Source: model.GroupSourceLdap, RemoteId: model.NewId(), } + g1Opts := model.GroupSearchOpts{ + FilterAllowReference: false, + } + d1, err := ss.Group().Create(g1) require.Nil(t, err) require.Len(t, d1.Id, 26) // Get the group - d2, err := ss.Group().GetByName(d1.Name) + d2, err := ss.Group().GetByName(d1.Name, g1Opts) require.Nil(t, err) require.Equal(t, d1.Id, d2.Id) require.Equal(t, d1.Name, d2.Name) @@ -236,7 +250,7 @@ func testGroupStoreGetByName(t *testing.T, ss store.Store) { require.Equal(t, d1.DeleteAt, d2.DeleteAt) // Get an invalid group - _, err = ss.Group().GetByName(model.NewId()) + _, err = ss.Group().GetByName(model.NewId(), g1Opts) require.NotNil(t, err) require.Equal(t, err.Id, "store.sql_group.no_rows") } @@ -490,7 +504,7 @@ func testGroupStoreUpdate(t *testing.T, ss store.Store) { Description: model.NewId(), RemoteId: model.NewId(), }) - require.Equal(t, err.Id, "store.update_error") + require.Equal(t, err.Id, "store.sql_group.unique_constraint") // Cannot update CreateAt someVal := model.GetMillis() diff --git a/store/storetest/mocks/GroupStore.go b/store/storetest/mocks/GroupStore.go index 746c298caf..a329116a90 100644 --- a/store/storetest/mocks/GroupStore.go +++ b/store/storetest/mocks/GroupStore.go @@ -454,13 +454,13 @@ func (_m *GroupStore) GetByIDs(groupIDs []string) ([]*model.Group, *model.AppErr return r0, r1 } -// GetByName provides a mock function with given fields: name -func (_m *GroupStore) GetByName(name string) (*model.Group, *model.AppError) { - ret := _m.Called(name) +// GetByName provides a mock function with given fields: name, opts +func (_m *GroupStore) GetByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { + ret := _m.Called(name, opts) var r0 *model.Group - if rf, ok := ret.Get(0).(func(string) *model.Group); ok { - r0 = rf(name) + if rf, ok := ret.Get(0).(func(string, model.GroupSearchOpts) *model.Group); ok { + r0 = rf(name, opts) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*model.Group) @@ -468,8 +468,8 @@ func (_m *GroupStore) GetByName(name string) (*model.Group, *model.AppError) { } var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string) *model.AppError); ok { - r1 = rf(name) + if rf, ok := ret.Get(1).(func(string, model.GroupSearchOpts) *model.AppError); ok { + r1 = rf(name, opts) } else { if ret.Get(1) != nil { r1 = ret.Get(1).(*model.AppError) diff --git a/store/timer_layer.go b/store/timer_layer.go index 04a0888406..ddb9191190 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -2910,10 +2910,10 @@ func (s *TimerLayerGroupStore) GetByIDs(groupIDs []string) ([]*model.Group, *mod return resultVar0, resultVar1 } -func (s *TimerLayerGroupStore) GetByName(name string) (*model.Group, *model.AppError) { +func (s *TimerLayerGroupStore) GetByName(name string, opts model.GroupSearchOpts) (*model.Group, *model.AppError) { start := timemodule.Now() - resultVar0, resultVar1 := s.GroupStore.GetByName(name) + resultVar0, resultVar1 := s.GroupStore.GetByName(name, opts) elapsed := float64(timemodule.Since(start)) / float64(timemodule.Second) if s.Root.Metrics != nil {