From 04ac120dca9ada4476c8b946f5b88a40fe3b8444 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 11 Nov 2026 13:32:56 +0100 Subject: [PATCH 1/2] fix(graph): apply the role allowlist to permission updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating a permission or a space root permission did not check the roles an administrator enabled with GRAPH_AVAILABLE_ROLES. An invite with a disabled role was rejected, but the same role could be set by updating an existing share. We now validate updates against the allowlist, same as invites. The three "without enabling it" scenarios now expect a 400. Their removed lines are padded with blank lines so the line numbers of the scenarios stay the same. ported from `owncloud/ocis#12540`. The reva half (`owncloud/reva#653`) is https://github.com/opencloud-eu/reva/pull/993 Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Jörn Friedrich Dreyer --- .../service/v0/api_driveitem_permissions.go | 6 +- .../graph/pkg/validate/libregraph_test.go | 22 ++ .../updateShareInvitations.feature | 292 +++++++++--------- 3 files changed, 172 insertions(+), 148 deletions(-) diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 411a3aae66..75f31df2ff 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -1007,7 +1007,8 @@ func (api DriveItemPermissionsApi) UpdatePermission(w http.ResponseWriter, r *ht return } - ctx := r.Context() + // enforce the admin role allowlist on updates, same as on invites + ctx := validate.ContextWithAllowedRoleIDs(r.Context(), api.config.UnifiedRoles.AvailableRoles) if err = validate.StructCtx(ctx, permission); err != nil { api.logger.Debug().Err(err).Interface("Body", r.Body).Msg("invalid request body") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) @@ -1046,7 +1047,8 @@ func (api DriveItemPermissionsApi) UpdateSpaceRootPermission(w http.ResponseWrit return } - ctx := r.Context() + // enforce the admin role allowlist on updates, same as on invites + ctx := validate.ContextWithAllowedRoleIDs(r.Context(), api.config.UnifiedRoles.AvailableRoles) if err = validate.StructCtx(ctx, permission); err != nil { api.logger.Debug().Err(err).Interface("Body", r.Body).Msg("invalid request body") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) diff --git a/services/graph/pkg/validate/libregraph_test.go b/services/graph/pkg/validate/libregraph_test.go index 52e0624ebb..48855312c9 100644 --- a/services/graph/pkg/validate/libregraph_test.go +++ b/services/graph/pkg/validate/libregraph_test.go @@ -125,6 +125,28 @@ var _ = Describe("libregraph", func() { }), ) + // Permission is validated when updating a permission (PATCH). It must honour the + // admin role allowlist just like DriveItemInvite does. + DescribeTable("Permission", + func(factory validatableFactory[libregraph.Permission]) { + s, pass := factory() + switch err := validate.StructCtx(ctx, s); pass { + case false: + Expect(err).To(HaveOccurred()) + default: + Expect(err).ToNot(HaveOccurred()) + } + }, + Entry("fail: disabled role rejected when allowlist is set", func() (libregraph.Permission, bool) { + ctx = validate.ContextWithAllowedRoleIDs(ctx, []string{unifiedrole.UnifiedRoleEditorID}) + return libregraph.Permission{Roles: []string{unifiedrole.UnifiedRoleSecureViewerID}}, false + }), + Entry("succeed: allowed role accepted when allowlist is set", func() (libregraph.Permission, bool) { + ctx = validate.ContextWithAllowedRoleIDs(ctx, []string{unifiedrole.UnifiedRoleEditorID}) + return libregraph.Permission{Roles: []string{unifiedrole.UnifiedRoleEditorID}}, true + }), + ) + DescribeTable("DriveRecipient", func(factories ...validatableFactory[libregraph.DriveRecipient]) { for _, factory := range factories { diff --git a/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature b/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature index ad024c0069..e6bfc9e1fe 100644 --- a/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature +++ b/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature @@ -1183,7 +1183,7 @@ Feature: Update permission of a share | Manager | Space Editor | @issue-10768 - Scenario Outline: sharer updates share permissions role of a resource to Secure Viewer without enabling it + Scenario Outline: sharer tries to update share permissions role of a resource to Secure Viewer without enabling it Given user "Alice" has created folder "folderToShare" And user "Alice" has uploaded file with content "hello world" to "textfile.txt" And user "Alice" has sent the following resource share invitation: @@ -1196,67 +1196,26 @@ Feature: Update permission of a share | permissionsRole | Secure Viewer | | space | Personal | | resource | | - Then the HTTP status code should be "200" + Then the HTTP status code should be "400" And the JSON data of the response should match """ { "type": "object", - "required": [ - "@libre.graph.permissions.actions", - "grantedToV2", - "id", - "invitation" - ], + "required": ["error"], "properties": { - "@libre.graph.permissions.actions": { - "const": [ - "libre.graph/driveItem/path/read", - "libre.graph/driveItem/children/read", - "libre.graph/driveItem/basic/read" - ] - }, - "grantedToV2": { + "error": { "type": "object", - "required": ["user"], + "required": ["code", "innererror", "message"], "properties": { - "user": { + "code": { + "const": "invalidRequest" + }, + "innererror": { "type": "object", - "required": ["displayName", "id"], - "properties": { - "displayName": { - "const": "Brian Murphy" - }, - "id": { - "pattern": "^%user_id_pattern%$" - } - } - } - } - }, - "id": { - "pattern": "^%permissions_id_pattern%$" - }, - "invitation": { - "type": "object", - "required": ["invitedBy"], - "properties": { - "invitedBy": { - "type": "object", - "required": ["user"], - "properties": { - "user": { - "type": "object", - "required": ["displayName", "id"], - "properties": { - "displayName": { - "const": "Alice Hansen" - }, - "id": { - "pattern": "^%user_id_pattern%$" - } - } - } - } + "required": ["date", "request-id"] + }, + "message": { + "const": "Key: 'Permission.Roles' Error:Field validation for 'Roles' failed on the 'available_role' tag" } } } @@ -1271,8 +1230,49 @@ Feature: Update permission of a share | textfile.txt | File Editor | | textfile.txt | Viewer | + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @issue-10768 - Scenario Outline: sharer updates share permissions role of a resource to Denied without enabling it + Scenario Outline: sharer tries to update share permissions role of a resource to Denied without enabling it Given user "Alice" has created folder "folderToShare" And user "Alice" has sent the following resource share invitation: | resource | folderToShare | @@ -1284,63 +1284,26 @@ Feature: Update permission of a share | permissionsRole | Denied | | space | Personal | | resource | folderToShare | - Then the HTTP status code should be "200" + Then the HTTP status code should be "400" And the JSON data of the response should match """ { "type": "object", - "required": [ - "@libre.graph.permissions.actions", - "grantedToV2", - "id", - "invitation" - ], + "required": ["error"], "properties": { - "@libre.graph.permissions.actions": { - "const": ["none"] - }, - "grantedToV2": { - "type": "object", - "required": ["user"], - "properties": { - "user": { - "type": "object", - "required": ["displayName", "id"], - "properties": { - "displayName": { - "const": "Brian Murphy" - }, - "id": { - "pattern": "^%user_id_pattern%$" - } - } - } - } - }, - "id": { - "pattern": "^%permissions_id_pattern%$" - }, - "invitation": { + "error": { "type": "object", - "required": ["invitedBy"], + "required": ["code", "innererror", "message"], "properties": { - "invitedBy": { + "code": { + "const": "invalidRequest" + }, + "innererror": { "type": "object", - "required": ["user"], - "properties": { - "user": { - "type": "object", - "required": ["displayName", "id"], - "properties": { - "displayName": { - "const": "Alice Hansen" - }, - "id": { - "pattern": "^%user_id_pattern%$" - } - } - } - } + "required": ["date", "request-id"] + }, + "message": { + "const": "Key: 'Permission.Roles' Error:Field validation for 'Roles' failed on the 'available_role' tag" } } } @@ -1353,8 +1316,45 @@ Feature: Update permission of a share | Editor | | Viewer | + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + @issue-10768 - Scenario Outline: sharer updates share permissions role of space to Space Editor Without Versions without enabling it + Scenario Outline: sharer tries to update share permissions role of space to Space Editor Without Versions without enabling it Given using spaces DAV path And the administrator has assigned the role "Space Admin" to user "Alice" using the Graph API And user "Alice" has created a space "new-space" with the default quota using the Graph API @@ -1368,56 +1368,28 @@ Feature: Update permission of a share | space | new-space | | shareType | user | | sharee | Brian | - Then the HTTP status code should be "200" + Then the HTTP status code should be "400" And the JSON data of the response should match """ { "type": "object", - "required": [ - "@libre.graph.permissions.actions", - "grantedToV2", - "id" - ], + "required": ["error"], "properties": { - "@libre.graph.permissions.actions": { - "const": [ - "libre.graph/driveItem/children/create", - "libre.graph/driveItem/standard/delete", - "libre.graph/driveItem/path/read", - "libre.graph/driveItem/quota/read", - "libre.graph/driveItem/content/read", - "libre.graph/driveItem/upload/create", - "libre.graph/driveItem/permissions/read", - "libre.graph/driveItem/children/read", - "libre.graph/driveItem/deleted/read", - "libre.graph/driveItem/path/update", - "libre.graph/driveItem/deleted/update", - "libre.graph/driveItem/basic/read" - ] - }, - "grantedToV2": { + "error": { "type": "object", - "required": ["user"], - "properties":{ - "user": { + "required": ["code", "innererror", "message"], + "properties": { + "code": { + "const": "invalidRequest" + }, + "innererror": { "type": "object", - "required": [ - "displayName", - "id" - ], - "properties": { - "displayName": { - "const": "Brian Murphy" - }, - "id": { - "pattern": "^%user_id_pattern%$" - } - } + "required": ["date", "request-id"] + }, + "message": { + "const": "Key: 'Permission.Roles' Error:Field validation for 'Roles' failed on the 'available_role' tag" } } - }, - "id": { - "pattern": "^%permissions_id_pattern%$" } } } @@ -1427,3 +1399,31 @@ Feature: Update permission of a share | Space Viewer | | Space Editor | | Manager | + + + + + + + + + + + + + + + + + + + + + + + + + + + + From f1271ef2fe458e36dc6678f79708de740c7a1dbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 11 Nov 2026 15:05:18 +0100 Subject: [PATCH 2/2] test: drop the blank line padding in updateShareInvitations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The padding kept the scenario line numbers stable but trips the newline_before_scenario gherlint rule. The three scenarios are the last ones in the file and nothing refers to their line numbers, so they can just move up. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: Jörn Friedrich Dreyer --- .../updateShareInvitations.feature | 106 ------------------ 1 file changed, 106 deletions(-) diff --git a/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature b/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature index e6bfc9e1fe..475366e664 100644 --- a/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature +++ b/tests/acceptance/features/apiSharingNgShareInvitation/updateShareInvitations.feature @@ -1230,47 +1230,6 @@ Feature: Update permission of a share | textfile.txt | File Editor | | textfile.txt | Viewer | - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @issue-10768 Scenario Outline: sharer tries to update share permissions role of a resource to Denied without enabling it Given user "Alice" has created folder "folderToShare" @@ -1316,43 +1275,6 @@ Feature: Update permission of a share | Editor | | Viewer | - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - @issue-10768 Scenario Outline: sharer tries to update share permissions role of space to Space Editor Without Versions without enabling it Given using spaces DAV path @@ -1399,31 +1321,3 @@ Feature: Update permission of a share | Space Viewer | | Space Editor | | Manager | - - - - - - - - - - - - - - - - - - - - - - - - - - - -