From bac2256c5f8f9f2af3391c561bfa992bd3df6150 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 --- changelog/unreleased/fix-denied.md | 5 ++ .../pkg/config/defaults/defaultconfig.go | 1 + .../service/v0/api_driveitem_permissions.go | 2 +- .../v0/api_driveitem_permissions_test.go | 77 ++++++++++++++++++- services/graph/pkg/service/v0/base.go | 6 +- services/graph/pkg/unifiedrole/conversion.go | 2 + .../graph/pkg/unifiedrole/conversion_test.go | 2 + services/graph/pkg/unifiedrole/export_test.go | 1 + services/graph/pkg/unifiedrole/roles.go | 29 ++++++- services/graph/pkg/unifiedrole/roles_test.go | 1 + services/web/pkg/theme/theme.go | 4 + 11 files changed, 126 insertions(+), 4 deletions(-) create mode 100644 changelog/unreleased/fix-denied.md 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/api_driveitem_permissions_test.go b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go index 5ac3cc43e5a..d08ea120bca 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go @@ -21,6 +21,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" libregraph "github.com/owncloud/libre-graph-api-go" + "github.com/owncloud/ocis/v2/services/graph/pkg/config" "github.com/stretchr/testify/mock" "github.com/tidwall/gjson" "google.golang.org/grpc" @@ -59,6 +60,7 @@ var _ = Describe("DriveItemPermissionsService", func() { statResponse *provider.StatResponse driveItemId *provider.ResourceId ctx context.Context + cfg *config.Config ) BeforeEach(func() { @@ -70,7 +72,7 @@ var _ = Describe("DriveItemPermissionsService", func() { cache = identity.NewIdentityCache(identity.IdentityCacheWithGatewaySelector(gatewaySelector)) - cfg := defaults.FullDefaultConfig() + cfg = defaults.FullDefaultConfig() service, err := svc.NewDriveItemPermissionsService(logger, gatewaySelector, cache, cfg) Expect(err).ToNot(HaveOccurred()) driveItemPermissionsService = service @@ -436,6 +438,79 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(len(permissions.LibreGraphPermissionsRolesAllowedValues)).ToNot(BeZero()) Expect(len(permissions.Value)).To(Equal(2)) }) + It("returns role denied", func() { + statResponse.Info.PermissionSet = roleconversions.NewManagerRole().CS3ResourcePermissions() + cfg.UnifiedRoles.AvailableRoles = []string{unifiedrole.UnifiedRoleViewerID, unifiedrole.UnifiedRoleDeniedID, unifiedrole.UnifiedRoleManagerID} + listSharesResponse.Shares = []*collaboration.Share{ + { + Id: &collaboration.ShareId{OpaqueId: "1"}, + Permissions: &collaboration.SharePermissions{ + Permissions: roleconversions.NewDeniedRole().CS3ResourcePermissions(), + }, + ResourceId: &provider.ResourceId{ + StorageId: "1", + SpaceId: "2", + OpaqueId: "3", + }, + Grantee: &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_USER, + Id: &provider.Grantee_UserId{ + UserId: &userpb.UserId{ + OpaqueId: "user-id", + }, + }, + }, + }, + } + listPublicSharesResponse.Share = []*link.PublicShare{} + + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(listPublicSharesResponse, nil) + permissions, err := driveItemPermissionsService.ListPermissions(context.Background(), itemID, false, false) + Expect(err).ToNot(HaveOccurred()) + Expect(len(permissions.LibreGraphPermissionsActionsAllowedValues)).ToNot(BeZero()) + Expect(len(permissions.LibreGraphPermissionsRolesAllowedValues)).ToNot(BeZero()) + Expect(len(permissions.Value)).To(Equal(1)) + Expect(permissions.Value[0].GetRoles()[0]).To(Equal(unifiedrole.UnifiedRoleDeniedID)) + }) + It("returns access denied when no role and no actions are set (full denial)", func() { + statResponse.Info.PermissionSet = roleconversions.NewManagerRole().CS3ResourcePermissions() + listSharesResponse.Shares = []*collaboration.Share{ + { + Id: &collaboration.ShareId{OpaqueId: "1"}, + Permissions: &collaboration.SharePermissions{ + Permissions: roleconversions.NewDeniedRole().CS3ResourcePermissions(), + }, + ResourceId: &provider.ResourceId{ + StorageId: "1", + SpaceId: "2", + OpaqueId: "3", + }, + Grantee: &provider.Grantee{ + Type: provider.GranteeType_GRANTEE_TYPE_USER, + Id: &provider.Grantee_UserId{ + UserId: &userpb.UserId{ + OpaqueId: "user-id", + }, + }, + }, + }, + } + listPublicSharesResponse.Share = []*link.PublicShare{} + + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(listPublicSharesResponse, nil) + permissions, err := driveItemPermissionsService.ListPermissions(context.Background(), itemID, false, false) + Expect(err).ToNot(HaveOccurred()) + Expect(len(permissions.LibreGraphPermissionsActionsAllowedValues)).ToNot(BeZero()) + Expect(len(permissions.LibreGraphPermissionsRolesAllowedValues)).ToNot(BeZero()) + Expect(len(permissions.Value)).To(Equal(1)) + Expect(permissions.Value[0].GetLibreGraphPermissionsActions()[0]).To(Equal("none")) + }) }) Describe("ListSpaceRootPermissions", func() { var ( diff --git a/services/graph/pkg/service/v0/base.go b/services/graph/pkg/service/v0/base.go index 6992a2e994f..d670aff5475 100644 --- a/services/graph/pkg/service/v0/base.go +++ b/services/graph/pkg/service/v0/base.go @@ -482,6 +482,10 @@ func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *c perm.SetRoles([]string{role.GetId()}) } else { actions := unifiedrole.CS3ResourcePermissionsToLibregraphActions(share.GetPermissions().GetPermissions()) + // neither a role nor actions are set, we need to return "none" as a hint in the actions + if len(actions) == 0 { + actions = []string{"none"} + } perm.SetLibreGraphPermissionsActions(actions) perm.SetRoles(nil) } @@ -1079,7 +1083,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..99730c34442 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,7 @@ 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: {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..4768348f5ca 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,22 @@ 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(UnifiedRoleConditionFolder), + }, + }, + LibreGraphWeight: proto.Int32(0), + } + }() ) // GetRoles returns a role filter that matches the provided resources @@ -484,7 +509,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..5fe83a00301 100644 --- a/services/graph/pkg/unifiedrole/roles_test.go +++ b/services/graph/pkg/unifiedrole/roles_test.go @@ -172,6 +172,7 @@ func TestGetRolesByPermissions(t *testing.T) { givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleDenied, unifiedrole.RoleSecureViewer, unifiedrole.RoleViewer, unifiedrole.RoleViewerListGrants, 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", + }, }, }, }