diff --git a/changelog/unreleased/fix-denied.md b/changelog/unreleased/fix-denied.md new file mode 100644 index 00000000000..e4cab79b1ae --- /dev/null +++ b/changelog/unreleased/fix-denied.md @@ -0,0 +1,5 @@ +Bugfix: Fix deny access for graph roles + +We added a unified role "Cannot access" to prevent a regression when switching the share implementation to the graph API. This role is now used to deny access to a resource.The new role is not enabled by default. The whole deny feature is still experimental. + +https://github.com/owncloud/ocis/pull/10627 diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go index 4cd0680b0d8..8bcb6921337 100644 --- a/services/graph/pkg/config/defaults/defaultconfig.go +++ b/services/graph/pkg/config/defaults/defaultconfig.go @@ -21,6 +21,7 @@ var ( unifiedrole.UnifiedRoleViewerListGrantsID, unifiedrole.UnifiedRoleEditorListGrantsID, unifiedrole.UnifiedRoleFileEditorListGrantsID, + unifiedrole.UnifiedRoleDeniedID, } ) diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 223a41d660a..dfdb3567a11 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -119,7 +119,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto } allowedResourceActions := unifiedrole.GetAllowedResourceActions(role, condition) - if len(allowedResourceActions) == 0 { + if len(allowedResourceActions) == 0 && role.GetId() != unifiedrole.UnifiedRoleDeniedID { return libregraph.Permission{}, errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource") } diff --git a/services/graph/pkg/service/v0/base.go b/services/graph/pkg/service/v0/base.go index 6992a2e994f..1fcb31e53ef 100644 --- a/services/graph/pkg/service/v0/base.go +++ b/services/graph/pkg/service/v0/base.go @@ -1079,7 +1079,7 @@ func (g BaseGraphService) updateUserShare(ctx context.Context, permissionID stri } allowedResourceActions = unifiedrole.GetAllowedResourceActions(role, condition) - if len(allowedResourceActions) == 0 { + if len(allowedResourceActions) == 0 && role.GetId() != unifiedrole.UnifiedRoleDeniedID { return nil, errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource") } } diff --git a/services/graph/pkg/unifiedrole/conversion.go b/services/graph/pkg/unifiedrole/conversion.go index 6b4e8770143..9bace85cfd0 100644 --- a/services/graph/pkg/unifiedrole/conversion.go +++ b/services/graph/pkg/unifiedrole/conversion.go @@ -226,6 +226,8 @@ func cs3RoleToDisplayName(role *conversions.Role) string { return _managerUnifiedRoleDisplayName case conversions.RoleSecureViewer: return _secureViewerUnifiedRoleDisplayName + case conversions.RoleDenied: + return _deniedUnifiedRoleDisplayName default: return "" } diff --git a/services/graph/pkg/unifiedrole/conversion_test.go b/services/graph/pkg/unifiedrole/conversion_test.go index 56a361dcaed..cda6c3a36ca 100644 --- a/services/graph/pkg/unifiedrole/conversion_test.go +++ b/services/graph/pkg/unifiedrole/conversion_test.go @@ -27,6 +27,7 @@ func TestPermissionsToCS3ResourcePermissions(t *testing.T) { cs3Conversions.RoleFileEditorListGrants: {cs3Conversions.NewFileEditorListGrantsRole(), unifiedrole.RoleFileEditorListGrants, true}, cs3Conversions.RoleManager: {cs3Conversions.NewManagerRole(), unifiedrole.RoleManager, true}, cs3Conversions.RoleSecureViewer: {cs3Conversions.NewSecureViewerRole(), unifiedrole.RoleSecureViewer, true}, + cs3Conversions.RoleDenied: {cs3Conversions.NewDeniedRole(), unifiedrole.RoleDenied, true}, "no match": {cs3Conversions.NewFileEditorRole(), unifiedrole.RoleManager, false}, } @@ -66,6 +67,8 @@ func TestCS3ResourcePermissionsToRole(t *testing.T) { cs3Conversions.RoleSpaceEditor: {cs3Conversions.NewSpaceEditorRole().CS3ResourcePermissions(), unifiedrole.RoleSpaceEditor, unifiedrole.UnifiedRoleConditionDrive}, cs3Conversions.RoleSecureViewer + "1": {cs3Conversions.NewSecureViewerRole().CS3ResourcePermissions(), unifiedrole.RoleSecureViewer, unifiedrole.UnifiedRoleConditionFile}, cs3Conversions.RoleSecureViewer + "2": {cs3Conversions.NewSecureViewerRole().CS3ResourcePermissions(), unifiedrole.RoleSecureViewer, unifiedrole.UnifiedRoleConditionFolder}, + cs3Conversions.RoleDenied + "1": {cs3Conversions.NewDeniedRole().CS3ResourcePermissions(), unifiedrole.RoleDenied, unifiedrole.UnifiedRoleConditionFile}, + cs3Conversions.RoleDenied + "2": {cs3Conversions.NewDeniedRole().CS3ResourcePermissions(), unifiedrole.RoleDenied, unifiedrole.UnifiedRoleConditionFolder}, "custom 1": {&provider.ResourcePermissions{GetPath: true}, nil, unifiedrole.UnifiedRoleConditionFolder}, } diff --git a/services/graph/pkg/unifiedrole/export_test.go b/services/graph/pkg/unifiedrole/export_test.go index 9b3b182b3f9..bca190a00a3 100644 --- a/services/graph/pkg/unifiedrole/export_test.go +++ b/services/graph/pkg/unifiedrole/export_test.go @@ -13,6 +13,7 @@ var ( RoleEditorLite = roleEditorLite RoleManager = roleManager RoleSecureViewer = roleSecureViewer + RoleDenied = roleDenied BuildInRoles = buildInRoles diff --git a/services/graph/pkg/unifiedrole/roles.go b/services/graph/pkg/unifiedrole/roles.go index 2b24100b31a..5d2d1369e06 100644 --- a/services/graph/pkg/unifiedrole/roles.go +++ b/services/graph/pkg/unifiedrole/roles.go @@ -38,6 +38,8 @@ const ( UnifiedRoleManagerID = "312c0871-5ef7-4b3a-85b6-0e4074c64049" // UnifiedRoleSecureViewerID Unified role secure viewer id. UnifiedRoleSecureViewerID = "aa97fe03-7980-45ac-9e50-b325749fd7e6" + // UnifiedRoleDeniedID Unified role to deny all access. + UnifiedRoleDeniedID = "63e64e19-8d43-42ec-a738-2b6af2610efa" // Wile the below conditions follow the SDDL syntax, they are not parsed anywhere. We use them as strings to // represent the constraints that a role definition applies to. For the actual syntax, see the SDDL documentation @@ -161,6 +163,12 @@ var ( // UnifiedRole SecureViewer, Role DisplayName (resolves directly) _secureViewerUnifiedRoleDisplayName = l10n.Template("Can view (secure)") + // UnifiedRole FullDenial, Role Description (resolves directly) + _deniedUnifiedRoleDescription = l10n.Template("Deny all access.") + + // UnifiedRole FullDenial, Role DisplayName (resolves directly) + _deniedUnifiedRoleDisplayName = l10n.Template("Cannot access.") + // legacyNames contains the legacy role names. legacyNames = map[string]string{ UnifiedRoleViewerID: conversions.RoleViewer, @@ -190,6 +198,7 @@ var ( roleEditorLite, roleManager, roleSecureViewer, + roleDenied, } // roleViewer creates a viewer role. @@ -439,6 +448,26 @@ var ( LibreGraphWeight: proto.Int32(0), } }() + // roleDenied creates a secure viewer role + roleDenied = func() *libregraph.UnifiedRoleDefinition { + r := conversions.NewDeniedRole() + return &libregraph.UnifiedRoleDefinition{ + Id: proto.String(UnifiedRoleDeniedID), + Description: proto.String(_deniedUnifiedRoleDescription), + DisplayName: proto.String(cs3RoleToDisplayName(r)), + RolePermissions: []libregraph.UnifiedRolePermission{ + { + AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), + Condition: proto.String(UnifiedRoleConditionFile), + }, + { + AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), + Condition: proto.String(UnifiedRoleConditionFolder), + }, + }, + LibreGraphWeight: proto.Int32(0), + } + }() ) // GetRoles returns a role filter that matches the provided resources @@ -484,7 +513,9 @@ func GetRolesByPermissions(roleSet []*libregraph.UnifiedRoleDefinition, actions match = true } } - + if role.GetId() == UnifiedRoleDeniedID && slices.Contains(actions, DriveItemPermissionsDeny) { + match = true + } if match { break } diff --git a/services/graph/pkg/unifiedrole/roles_test.go b/services/graph/pkg/unifiedrole/roles_test.go index 31690a1e368..eb1e4f98b19 100644 --- a/services/graph/pkg/unifiedrole/roles_test.go +++ b/services/graph/pkg/unifiedrole/roles_test.go @@ -161,6 +161,7 @@ func TestGetRolesByPermissions(t *testing.T) { givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleDenied, unifiedrole.RoleSecureViewer, unifiedrole.RoleViewer, unifiedrole.RoleViewerListGrants, @@ -172,6 +173,7 @@ func TestGetRolesByPermissions(t *testing.T) { givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleDenied, unifiedrole.RoleSecureViewer, unifiedrole.RoleViewer, unifiedrole.RoleViewerListGrants, @@ -203,6 +205,31 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleEditorLite, }, }, + "All Roles | file": { + givenActions: getRoleActions(unifiedrole.RoleManager), + constraints: unifiedrole.UnifiedRoleConditionFile, + unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleDenied, + unifiedrole.RoleSecureViewer, + unifiedrole.RoleViewer, + unifiedrole.RoleViewerListGrants, + unifiedrole.RoleFileEditor, + unifiedrole.RoleFileEditorListGrants, + }, + }, + "All Roles | folder": { + givenActions: getRoleActions(unifiedrole.RoleManager), + constraints: unifiedrole.UnifiedRoleConditionFolder, + unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleDenied, + unifiedrole.RoleSecureViewer, + unifiedrole.RoleViewer, + unifiedrole.RoleViewerListGrants, + unifiedrole.RoleEditorLite, + unifiedrole.RoleEditor, + unifiedrole.RoleEditorListGrants, + }, + }, } for name, tc := range tests { diff --git a/services/web/pkg/theme/theme.go b/services/web/pkg/theme/theme.go index 35f84220624..3c1f46d0974 100644 --- a/services/web/pkg/theme/theme.go +++ b/services/web/pkg/theme/theme.go @@ -65,6 +65,10 @@ var themeDefaults = KV{ "label": "UnifiedRoleSecureView", "iconName": "shield", }, + unifiedrole.UnifiedRoleDeniedID: KV{ + "label": "UnifiedRoleFullDenial", + "iconName": "stop-circle", + }, }, }, }