Merge pull request #5949 from owncloud/excds/feature/Update_group_names

graph: Allow updating of group name via PATCH request
2026-03-05 15:57:01 -05:00 · 2023-03-29 12:48:25 +02:00
parent bfe8ed8db5 555ca582b1
commit 053ce7a7cd
9 changed files with 256 additions and 19 deletions
--- a/changelog/unreleased/fix-group-name-changes.md
+++ 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
+
--- 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
--- 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")
--- 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
--- 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)
+		})
+	}
+}
--- 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 {
--- 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 {
--- 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)
+			})
+
 		})
 	})

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