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..475366e664 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": { - "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" } } } @@ -1272,7 +1231,7 @@ Feature: Update permission of a share | 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 +1243,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" } } } @@ -1354,7 +1276,7 @@ Feature: Update permission of a share | 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 +1290,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%$" } } }