fix: fix deny access to prevent a regression

owncloud · Nov 22, 2024 · 028fab2 · 028fab2
1 parent 2714557
commit 028fab2
Show file tree

Hide file tree

Showing 10 changed files with 81 additions and 3 deletions.
diff --git a/changelog/unreleased/fix-denied.md 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
@@ -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
@@ -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
@@ -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, that is a full denial
+		if len(actions) == 0 {
+			actions = []string{"access denied"}
+		}
 		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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -65,6 +65,10 @@ var themeDefaults = KV{
 				"label":    "UnifiedRoleSecureView",
 				"iconName": "shield",
 			},
+			unifiedrole.UnifiedRoleDeniedID: KV{
+				"label":    "UnifiedRoleFullDenial",
+				"iconName": "stop-circle",
+			},
 		},
 	},
 }