From d4b65664b041ec7bd53e081ed1f4c08609007211 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Fri, 22 Nov 2024 09:54:12 +0100 Subject: [PATCH] fix: fix deny access to prevent a regression --- .../pkg/config/defaults/defaultconfig.go | 1 + .../service/v0/api_driveitem_permissions.go | 2 +- services/graph/pkg/service/v0/base.go | 2 +- services/graph/pkg/unifiedrole/conversion.go | 2 ++ .../graph/pkg/unifiedrole/conversion_test.go | 3 ++ services/graph/pkg/unifiedrole/export_test.go | 1 + services/graph/pkg/unifiedrole/roles.go | 33 ++++++++++++++++++- services/graph/pkg/unifiedrole/roles_test.go | 25 ++++++++++++++ services/web/pkg/theme/theme.go | 4 +++ 9 files changed, 70 insertions(+), 3 deletions(-) diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go index 4cd0680b0d8..719f4681558 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.UnifiedRoleFullDenialID, } ) diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 223a41d660a..c90c121e10b 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.UnifiedRoleFullDenialID { 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..85307da4893 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.UnifiedRoleFullDenialID { 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..de236c50e12 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 _fullDenialUnifiedRoleDisplayName 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..0307d1b3395 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" + // UnifiedRoleFullDenialID Unified role to deny all access. + UnifiedRoleFullDenialID = "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) + _fullDenialUnifiedRoleDescription = l10n.Template("Deny all access.") + + // UnifiedRole FullDenial, Role DisplayName (resolves directly) + _fullDenialUnifiedRoleDisplayName = 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(UnifiedRoleFullDenialID), + Description: proto.String(_fullDenialUnifiedRoleDescription), + 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() == UnifiedRoleFullDenialID && 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..ef1493359d7 100644 --- a/services/graph/pkg/unifiedrole/roles_test.go +++ b/services/graph/pkg/unifiedrole/roles_test.go @@ -203,6 +203,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..e7ce8c322fb 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.UnifiedRoleFullDenialID: KV{ + "label": "UnifiedRoleFullDenial", + "iconName": "stop-circle", + }, }, }, }