diff --git a/changelog/unreleased/fix-group-name-changes.md b/changelog/unreleased/fix-group-name-changes.md new file mode 100644 index 0000000000..93eced4cc9 --- /dev/null +++ b/changelog/unreleased/fix-group-name-changes.md @@ -0,0 +1,4 @@ +Bugfix: Fix so that PATCH requests for groups actually updates the group name + +https://github.com/owncloud/ocis/pull/5949 + diff --git a/services/graph/pkg/identity/backend.go b/services/graph/pkg/identity/backend.go index 903dfc65b2..1ee6b10824 100644 --- a/services/graph/pkg/identity/backend.go +++ b/services/graph/pkg/identity/backend.go @@ -33,6 +33,8 @@ type Backend interface { CreateGroup(ctx context.Context, group libregraph.Group) (*libregraph.Group, error) // DeleteGroup deletes a given group, identified by id DeleteGroup(ctx context.Context, id string) error + // UpdateGroupName updates the group name + UpdateGroupName(ctx context.Context, groupID string, groupName string) error GetGroup(ctx context.Context, nameOrID string, queryParam url.Values) (*libregraph.Group, error) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregraph.Group, error) // GetGroupMembers list all members of a group diff --git a/services/graph/pkg/identity/cs3.go b/services/graph/pkg/identity/cs3.go index 331ea1574f..1f03170d12 100644 --- a/services/graph/pkg/identity/cs3.go +++ b/services/graph/pkg/identity/cs3.go @@ -192,6 +192,11 @@ func (i *CS3) DeleteGroup(ctx context.Context, id string) error { return errorcode.New(errorcode.NotSupported, "not implemented") } +// UpdateGroupName implements the Backend Interface. It's currently not supported for the CS3 backend +func (i *CS3) UpdateGroupName(ctx context.Context, groupID string, groupName string) error { + return errorcode.New(errorcode.NotSupported, "not implemented") +} + // GetGroupMembers implements the Backend Interface. It's currently not supported for the CS3 backend func (i *CS3) GetGroupMembers(ctx context.Context, groupID string, _ *godata.GoDataRequest) ([]*libregraph.User, error) { return nil, errorcode.New(errorcode.NotSupported, "not implemented") diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go index 01a9d143ca..040ac9423d 100644 --- a/services/graph/pkg/identity/ldap_group.go +++ b/services/graph/pkg/identity/ldap_group.go @@ -2,6 +2,7 @@ package identity import ( "context" + "errors" "fmt" "net/url" "strings" @@ -214,6 +215,43 @@ func (i *LDAP) DeleteGroup(ctx context.Context, id string) error { return nil } +// UpdateGroupName implements the Backend Interface. +func (i *LDAP) UpdateGroupName(ctx context.Context, groupID string, groupName string) error { + logger := i.logger.SubloggerWithRequestID(ctx) + logger.Debug().Str("backend", "ldap").Msg("AddMembersToGroup") + + ge, err := i.getLDAPGroupByID(groupID, true) + if err != nil { + return err + } + + if ge.GetEqualFoldAttributeValue(i.groupAttributeMap.name) == groupName { + return nil + } + + attributeTypeAndValue := ldap.AttributeTypeAndValue{ + Type: i.groupAttributeMap.name, + Value: groupName, + } + newDNString := attributeTypeAndValue.String() + + logger.Debug().Str("originalDN", ge.DN).Str("newDN", newDNString).Msg("Modifying DN") + mrdn := ldap.NewModifyDNRequest(ge.DN, newDNString, true, "") + + if err := i.conn.ModifyDN(mrdn); err != nil { + var lerr *ldap.Error + logger.Debug().Str("originalDN", ge.DN).Str("newDN", newDNString).Err(err).Msg("Failed to modify DN") + if errors.As(err, &lerr) { + if lerr.ResultCode == ldap.LDAPResultEntryAlreadyExists { + err = errorcode.New(errorcode.NameAlreadyExists, "Group name already in use") + } + } + return err + } + + return nil +} + // AddMembersToGroup implements the Backend Interface for the LDAP backend. // Currently it is limited to adding Users as Group members. Adding other groups // as members is not yet implemented diff --git a/services/graph/pkg/identity/ldap_group_test.go b/services/graph/pkg/identity/ldap_group_test.go index d375d1cef0..585b5e65af 100644 --- a/services/graph/pkg/identity/ldap_group_test.go +++ b/services/graph/pkg/identity/ldap_group_test.go @@ -8,6 +8,7 @@ import ( "github.com/go-ldap/ldap/v3" "github.com/owncloud/ocis/v2/services/graph/mocks" + "github.com/stretchr/testify/assert" "github.com/test-go/testify/mock" ) @@ -224,3 +225,138 @@ func TestGetGroups(t *testing.T) { } } } +func TestUpdateGroupName(t *testing.T) { + groupDn := "cn=TheGroup,ou=groups,dc=owncloud,dc=com" + + type args struct { + groupId string + groupName string + newName string + } + + type mockInputs struct { + funcName string + args []interface{} + returns []interface{} + } + + tests := []struct { + name string + args args + assertion assert.ErrorAssertionFunc + ldapMocks []mockInputs + }{ + { + name: "Test with no name change", + args: args{ + groupId: "some-uuid-string", + newName: "TheGroup", + }, + assertion: func(t assert.TestingT, err error, args ...interface{}) bool { + return assert.Nil(t, err, args...) + }, + ldapMocks: []mockInputs{ + { + funcName: "Search", + args: []interface{}{ + ldap.NewSearchRequest( + "ou=groups,dc=test", + ldap.ScopeWholeSubtree, + ldap.NeverDerefAliases, 1, 0, false, + "(&(objectClass=groupOfNames)(entryUUID=some-uuid-string))", + []string{"cn", "entryUUID", "member"}, + nil, + ), + }, + returns: []interface{}{ + &ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: groupDn, + Attributes: []*ldap.EntryAttribute{ + { + Name: "cn", + Values: []string{"TheGroup"}, + }, + }, + }, + }, + }, + nil, + }, + }, + }, + }, + { + name: "Test with name change", + args: args{ + groupId: "some-uuid-string", + newName: "TheGroupWithShinyNewName", + }, + assertion: func(t assert.TestingT, err error, args ...interface{}) bool { + return assert.Nil(t, err, args...) + }, + ldapMocks: []mockInputs{ + { + funcName: "Search", + args: []interface{}{ + ldap.NewSearchRequest( + "ou=groups,dc=test", + ldap.ScopeWholeSubtree, + ldap.NeverDerefAliases, 1, 0, false, + "(&(objectClass=groupOfNames)(entryUUID=some-uuid-string))", + []string{"cn", "entryUUID", "member"}, + nil, + ), + }, + returns: []interface{}{ + &ldap.SearchResult{ + Entries: []*ldap.Entry{ + { + DN: "cn=TheGroup,ou=groups,dc=owncloud,dc=com", + Attributes: []*ldap.EntryAttribute{ + { + Name: "cn", + Values: []string{"TheGroup"}, + }, + }, + }, + }, + }, + nil, + }, + }, + { + funcName: "ModifyDN", + args: []interface{}{ + &ldap.ModifyDNRequest{ + DN: groupDn, + NewRDN: "cn=TheGroupWithShinyNewName", + DeleteOldRDN: true, + NewSuperior: "", + Controls: []ldap.Control(nil), + }, + }, + returns: []interface{}{ + nil, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lm := &mocks.Client{} + for _, mock := range tt.ldapMocks { + lm.On(mock.funcName, mock.args...).Return(mock.returns...) + } + + ldapConfig := lconfig + i, _ := getMockedBackend(lm, ldapConfig, &logger) + + err := i.UpdateGroupName(context.Background(), tt.args.groupId, tt.args.newName) + tt.assertion(t, err) + }) + } +} diff --git a/services/graph/pkg/identity/mocks/backend.go b/services/graph/pkg/identity/mocks/backend.go index 89586889df..69e876002a 100644 --- a/services/graph/pkg/identity/mocks/backend.go +++ b/services/graph/pkg/identity/mocks/backend.go @@ -1,4 +1,4 @@ -// Code generated by mockery v2.14.1. DO NOT EDIT. +// Code generated by mockery v2.22.1. DO NOT EDIT. package mocks @@ -38,6 +38,10 @@ func (_m *Backend) CreateGroup(ctx context.Context, group libregraph.Group) (*li ret := _m.Called(ctx, group) var r0 *libregraph.Group + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, libregraph.Group) (*libregraph.Group, error)); ok { + return rf(ctx, group) + } if rf, ok := ret.Get(0).(func(context.Context, libregraph.Group) *libregraph.Group); ok { r0 = rf(ctx, group) } else { @@ -46,7 +50,6 @@ func (_m *Backend) CreateGroup(ctx context.Context, group libregraph.Group) (*li } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, libregraph.Group) error); ok { r1 = rf(ctx, group) } else { @@ -61,6 +64,10 @@ func (_m *Backend) CreateUser(ctx context.Context, user libregraph.User) (*libre ret := _m.Called(ctx, user) var r0 *libregraph.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, libregraph.User) (*libregraph.User, error)); ok { + return rf(ctx, user) + } if rf, ok := ret.Get(0).(func(context.Context, libregraph.User) *libregraph.User); ok { r0 = rf(ctx, user) } else { @@ -69,7 +76,6 @@ func (_m *Backend) CreateUser(ctx context.Context, user libregraph.User) (*libre } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, libregraph.User) error); ok { r1 = rf(ctx, user) } else { @@ -112,6 +118,10 @@ func (_m *Backend) GetGroup(ctx context.Context, nameOrID string, queryParam url ret := _m.Called(ctx, nameOrID, queryParam) var r0 *libregraph.Group + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string, url.Values) (*libregraph.Group, error)); ok { + return rf(ctx, nameOrID, queryParam) + } if rf, ok := ret.Get(0).(func(context.Context, string, url.Values) *libregraph.Group); ok { r0 = rf(ctx, nameOrID, queryParam) } else { @@ -120,7 +130,6 @@ func (_m *Backend) GetGroup(ctx context.Context, nameOrID string, queryParam url } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, string, url.Values) error); ok { r1 = rf(ctx, nameOrID, queryParam) } else { @@ -135,6 +144,10 @@ func (_m *Backend) GetGroupMembers(ctx context.Context, id string, oreq *godata. ret := _m.Called(ctx, id, oreq) var r0 []*libregraph.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string, *godata.GoDataRequest) ([]*libregraph.User, error)); ok { + return rf(ctx, id, oreq) + } if rf, ok := ret.Get(0).(func(context.Context, string, *godata.GoDataRequest) []*libregraph.User); ok { r0 = rf(ctx, id, oreq) } else { @@ -143,7 +156,6 @@ func (_m *Backend) GetGroupMembers(ctx context.Context, id string, oreq *godata. } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, string, *godata.GoDataRequest) error); ok { r1 = rf(ctx, id, oreq) } else { @@ -158,6 +170,10 @@ func (_m *Backend) GetGroups(ctx context.Context, queryParam url.Values) ([]*lib ret := _m.Called(ctx, queryParam) var r0 []*libregraph.Group + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, url.Values) ([]*libregraph.Group, error)); ok { + return rf(ctx, queryParam) + } if rf, ok := ret.Get(0).(func(context.Context, url.Values) []*libregraph.Group); ok { r0 = rf(ctx, queryParam) } else { @@ -166,7 +182,6 @@ func (_m *Backend) GetGroups(ctx context.Context, queryParam url.Values) ([]*lib } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, url.Values) error); ok { r1 = rf(ctx, queryParam) } else { @@ -181,6 +196,10 @@ func (_m *Backend) GetUser(ctx context.Context, nameOrID string, oreq *godata.Go ret := _m.Called(ctx, nameOrID, oreq) var r0 *libregraph.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string, *godata.GoDataRequest) (*libregraph.User, error)); ok { + return rf(ctx, nameOrID, oreq) + } if rf, ok := ret.Get(0).(func(context.Context, string, *godata.GoDataRequest) *libregraph.User); ok { r0 = rf(ctx, nameOrID, oreq) } else { @@ -189,7 +208,6 @@ func (_m *Backend) GetUser(ctx context.Context, nameOrID string, oreq *godata.Go } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, string, *godata.GoDataRequest) error); ok { r1 = rf(ctx, nameOrID, oreq) } else { @@ -204,6 +222,10 @@ func (_m *Backend) GetUsers(ctx context.Context, oreq *godata.GoDataRequest) ([] ret := _m.Called(ctx, oreq) var r0 []*libregraph.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, *godata.GoDataRequest) ([]*libregraph.User, error)); ok { + return rf(ctx, oreq) + } if rf, ok := ret.Get(0).(func(context.Context, *godata.GoDataRequest) []*libregraph.User); ok { r0 = rf(ctx, oreq) } else { @@ -212,7 +234,6 @@ func (_m *Backend) GetUsers(ctx context.Context, oreq *godata.GoDataRequest) ([] } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, *godata.GoDataRequest) error); ok { r1 = rf(ctx, oreq) } else { @@ -236,11 +257,29 @@ func (_m *Backend) RemoveMemberFromGroup(ctx context.Context, groupID string, me return r0 } +// UpdateGroupName provides a mock function with given fields: ctx, groupID, groupName +func (_m *Backend) UpdateGroupName(ctx context.Context, groupID string, groupName string) error { + ret := _m.Called(ctx, groupID, groupName) + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string) error); ok { + r0 = rf(ctx, groupID, groupName) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // UpdateUser provides a mock function with given fields: ctx, nameOrID, user func (_m *Backend) UpdateUser(ctx context.Context, nameOrID string, user libregraph.User) (*libregraph.User, error) { ret := _m.Called(ctx, nameOrID, user) var r0 *libregraph.User + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string, libregraph.User) (*libregraph.User, error)); ok { + return rf(ctx, nameOrID, user) + } if rf, ok := ret.Get(0).(func(context.Context, string, libregraph.User) *libregraph.User); ok { r0 = rf(ctx, nameOrID, user) } else { @@ -249,7 +288,6 @@ func (_m *Backend) UpdateUser(ctx context.Context, nameOrID string, user libregr } } - var r1 error if rf, ok := ret.Get(1).(func(context.Context, string, libregraph.User) error); ok { r1 = rf(ctx, nameOrID, user) } else { diff --git a/services/graph/pkg/service/v0/groups.go b/services/graph/pkg/service/v0/groups.go index 7ddfd995e2..5fa9210486 100644 --- a/services/graph/pkg/service/v0/groups.go +++ b/services/graph/pkg/service/v0/groups.go @@ -126,6 +126,11 @@ func (g Graph) PatchGroup(w http.ResponseWriter, r *http.Request) { return } + if changes.HasDisplayName() { + groupName := changes.GetDisplayName() + err = g.identityBackend.UpdateGroupName(r.Context(), groupID, groupName) + } + if memberRefs, ok := changes.GetMembersodataBindOk(); ok { // The spec defines a limit of 20 members maxium per Request if len(memberRefs) > g.config.API.GroupMembersPatchLimit { diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go index 16f7aa572e..298e1922aa 100644 --- a/services/graph/pkg/service/v0/groups_test.go +++ b/services/graph/pkg/service/v0/groups_test.go @@ -283,7 +283,6 @@ var _ = Describe("Groups", func() { It("fails when the number of users is exceeded - spec says 20 max", func() { updatedGroup := libregraph.NewGroup() - updatedGroup.SetDisplayName("group1 updated") updatedGroup.SetMembersodataBind([]string{ "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", @@ -306,7 +305,6 @@ var _ = Describe("Groups", func() { It("succeeds when the number of users is over 20 but the limit is raised to 21", func() { updatedGroup := libregraph.NewGroup() - updatedGroup.SetDisplayName("group1 updated") updatedGroup.SetMembersodataBind([]string{ "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", @@ -337,7 +335,6 @@ var _ = Describe("Groups", func() { It("fails on invalid user refs", func() { updatedGroup := libregraph.NewGroup() - updatedGroup.SetDisplayName("group1 updated") updatedGroup.SetMembersodataBind([]string{"invalid"}) updatedGroupJson, err := json.Marshal(updatedGroup) Expect(err).ToNot(HaveOccurred()) @@ -353,7 +350,6 @@ var _ = Describe("Groups", func() { It("fails when the adding non-users users", func() { updatedGroup := libregraph.NewGroup() - updatedGroup.SetDisplayName("group1 updated") updatedGroup.SetMembersodataBind([]string{"/non-users/user1"}) updatedGroupJson, err := json.Marshal(updatedGroup) Expect(err).ToNot(HaveOccurred()) @@ -371,7 +367,6 @@ var _ = Describe("Groups", func() { identityBackend.On("AddMembersToGroup", mock.Anything, mock.Anything, mock.Anything).Return(nil) updatedGroup := libregraph.NewGroup() - updatedGroup.SetDisplayName("group1 updated") updatedGroup.SetMembersodataBind([]string{"/users/user1"}) updatedGroupJson, err := json.Marshal(updatedGroup) Expect(err).ToNot(HaveOccurred()) @@ -385,6 +380,25 @@ var _ = Describe("Groups", func() { Expect(rr.Code).To(Equal(http.StatusNoContent)) identityBackend.AssertNumberOfCalls(GinkgoT(), "AddMembersToGroup", 1) }) + + It("updates the group name", func() { + identityBackend.On("UpdateGroupName", mock.Anything, mock.Anything, mock.Anything).Return(nil) + + updatedGroup := libregraph.NewGroup() + updatedGroup.SetDisplayName("group1 updated") + updatedGroupJson, err := json.Marshal(updatedGroup) + Expect(err).ToNot(HaveOccurred()) + + r := httptest.NewRequest(http.MethodPatch, "/graph/v1.0/groups", bytes.NewBuffer(updatedGroupJson)) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("groupID", *newGroup.Id) + r = r.WithContext(context.WithValue(revactx.ContextSetUser(ctx, currentUser), chi.RouteCtxKey, rctx)) + svc.PatchGroup(rr, r) + + Expect(rr.Code).To(Equal(http.StatusNoContent)) + identityBackend.AssertNumberOfCalls(GinkgoT(), "UpdateGroupName", 1) + }) + }) }) diff --git a/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md b/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md index 13bab45523..65056b0654 100644 --- a/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md +++ b/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md @@ -62,13 +62,8 @@ The expected failures in this file are from features in the owncloud/ocis repo. - [apiGraph/deleteGroup.feature:68](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/deleteGroup.feature#L68) #### [Updating group displayName request seems OK but group is not being renamed](https://github.com/owncloud/ocis/issues/5099) -- [apiGraph/editGroup.feature:20](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L20) -- [apiGraph/editGroup.feature:21](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L21) -- [apiGraph/editGroup.feature:22](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L22) - [apiGraph/editGroup.feature:23](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L23) -- [apiGraph/editGroup.feature:24](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L24) - [apiGraph/editGroup.feature:25](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L25) -- [apiGraph/editGroup.feature:40](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L40) #### [CORS headers are not identical with oC10 headers](https://github.com/owncloud/ocis/issues/5195) - [apiCors/cors.feature:25](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiCors/cors.feature#L25)