Merge pull request #10627 from owncloud/fix-denials-graph

fix: fix deny access to prevent a regression

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

services/graph/pkg/config/defaults/defaultconfig.go

@@ -21,6 +21,7 @@ var (
 		unifiedrole.UnifiedRoleViewerListGrantsID,
 		unifiedrole.UnifiedRoleEditorListGrantsID,
 		unifiedrole.UnifiedRoleFileEditorListGrantsID,
+		unifiedrole.UnifiedRoleDeniedID,
 	}
 )
 

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")
 		}
 

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 (

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")
 			}
 		}

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 ""
 	}

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},
 	}
 

services/graph/pkg/unifiedrole/export_test.go

@@ -13,6 +13,7 @@ var (
 	RoleEditorLite                 = roleEditorLite
 	RoleManager                    = roleManager
 	RoleSecureViewer               = roleSecureViewer
+	RoleDenied                     = roleDenied
 
 	BuildInRoles = buildInRoles
 

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
 			}

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,

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",
+			},
 		},
 	},
 }