From 4f2678351d155b5eadb80d4ace844ebebd46c00e Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Wed, 21 Aug 2024 14:10:46 +0200 Subject: [PATCH] fix: cherry pick unified-role federation changes --- services/graph/pkg/unifiedrole/roles.go | 53 +++++++++++++++++++- services/graph/pkg/unifiedrole/roles_test.go | 47 +++++++++++++---- 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/services/graph/pkg/unifiedrole/roles.go b/services/graph/pkg/unifiedrole/roles.go index 8cef535139..99c15e2a45 100644 --- a/services/graph/pkg/unifiedrole/roles.go +++ b/services/graph/pkg/unifiedrole/roles.go @@ -3,6 +3,7 @@ package unifiedrole import ( "cmp" "slices" + "strings" libregraph "github.com/owncloud/libre-graph-api-go" "google.golang.org/protobuf/proto" @@ -29,6 +30,18 @@ const ( UnifiedRoleManagerID = "312c0871-5ef7-4b3a-85b6-0e4074c64049" // UnifiedRoleSecureViewerID Unified role secure viewer id. UnifiedRoleSecureViewerID = "aa97fe03-7980-45ac-9e50-b325749fd7e6" + // UnifiedRoleFederatedViewerID Unified role federated viewer id. + UnifiedRoleFederatedViewerID = "be531789-063c-48bf-a9fe-857e6fbee7da" + // UnifiedRoleFederatedEditorID Unified role federated editor id. + UnifiedRoleFederatedEditorID = "36279a93-e4e3-4bbb-8a23-53b05b560963" + + // 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 + // at https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-definition-language-for-conditional-aces-#conditional-expressions + + // Some roles apply to a specific type of resource, for example, a role that applies to a file or a folder. + // @Resource is the placeholder for the resource that the role is applied to + // .Root, .Folder and .File are facets of the driveItem resource that indicate the type of the resource if they are present. // UnifiedRoleConditionDrive defines constraint that matches a Driveroot/Spaceroot UnifiedRoleConditionDrive = "exists @Resource.Root" @@ -37,6 +50,19 @@ const ( // UnifiedRoleConditionFile defines a constraint that matches a DriveItem representing a File UnifiedRoleConditionFile = "exists @Resource.File" + // Some roles apply to a specific type of user, for example, a role that applies to a federated user. + // @Subject is the placeholder for the subject that the role is applied to. For sharing roles this is the user that the resource is shared with. + // .UserType is the type of the user: 'Member' for a member of the organization, 'Guest' for a guest user, 'Federated' for a federated user. + + // UnifiedRoleConditionFederatedUser defines a constraint that matches a federated user + UnifiedRoleConditionFederatedUser = "@Subject.UserType==\"Federated\"" + + // For federated sharing we need roles that combine the constraints for the resource and the user. + // UnifiedRoleConditionFileFederatedUser defines a constraint that matches a File and a federated user + UnifiedRoleConditionFileFederatedUser = UnifiedRoleConditionFile + " && " + UnifiedRoleConditionFederatedUser + // UnifiedRoleConditionFolderFederatedUser defines a constraint that matches a Folder and a federated user + UnifiedRoleConditionFolderFederatedUser = UnifiedRoleConditionFolder + " && " + UnifiedRoleConditionFederatedUser + DriveItemPermissionsCreate = "libre.graph/driveItem/permissions/create" DriveItemChildrenCreate = "libre.graph/driveItem/children/create" DriveItemStandardDelete = "libre.graph/driveItem/standard/delete" @@ -149,6 +175,14 @@ var ( AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), Condition: proto.String(UnifiedRoleConditionFolder), }, + { + AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), + Condition: proto.String(UnifiedRoleConditionFileFederatedUser), + }, + { + AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), + Condition: proto.String(UnifiedRoleConditionFolderFederatedUser), + }, }, LibreGraphWeight: proto.Int32(0), } @@ -183,6 +217,10 @@ var ( AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), Condition: proto.String(UnifiedRoleConditionFolder), }, + { + AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), + Condition: proto.String(UnifiedRoleConditionFolderFederatedUser), + }, }, LibreGraphWeight: proto.Int32(0), } @@ -217,6 +255,10 @@ var ( AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), Condition: proto.String(UnifiedRoleConditionFile), }, + { + AllowedResourceActions: CS3ResourcePermissionsToLibregraphActions(r.CS3ResourcePermissions()), + Condition: proto.String(UnifiedRoleConditionFileFederatedUser), + }, }, LibreGraphWeight: proto.Int32(0), } @@ -295,14 +337,21 @@ func GetRole(f RoleFilter) (*libregraph.UnifiedRoleDefinition, error) { // GetRolesByPermissions returns a list of role definitions // that match the provided actions and constraints -func GetRolesByPermissions(roleSet []*libregraph.UnifiedRoleDefinition, actions []string, constraints string, descending bool) []*libregraph.UnifiedRoleDefinition { +func GetRolesByPermissions(roleSet []*libregraph.UnifiedRoleDefinition, actions []string, constraints string, listFederatedRoles, descending bool) []*libregraph.UnifiedRoleDefinition { roles := make([]*libregraph.UnifiedRoleDefinition, 0, len(roleSet)) for _, role := range roleSet { var match bool for _, permission := range role.GetRolePermissions() { - if permission.GetCondition() != constraints { + // this is a dirty comparison because we are not really parsing the SDDL, but as long as we && the conditions we are good + isFederatedRole := strings.Contains(permission.GetCondition(), UnifiedRoleConditionFederatedUser) + switch { + case !strings.Contains(permission.GetCondition(), constraints): + continue + case listFederatedRoles && !isFederatedRole: + continue + case !listFederatedRoles && isFederatedRole: continue } diff --git a/services/graph/pkg/unifiedrole/roles_test.go b/services/graph/pkg/unifiedrole/roles_test.go index 76a070629b..2d1a4e8d8d 100644 --- a/services/graph/pkg/unifiedrole/roles_test.go +++ b/services/graph/pkg/unifiedrole/roles_test.go @@ -93,9 +93,10 @@ func TestGetRolesByPermissions(t *testing.T) { tests := map[string]struct { givenActions []string constraints string + listFederatedRoles bool unifiedRoleDefinition []*libregraph.UnifiedRoleDefinition }{ - "ViewerUnifiedRole": { + "RoleViewer | folder": { givenActions: getRoleActions(unifiedrole.RoleViewer), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -103,7 +104,7 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleViewer, }, }, - "ViewerUnifiedRole | share": { + "RoleViewer | file": { givenActions: getRoleActions(unifiedrole.RoleViewer), constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -111,7 +112,15 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleViewer, }, }, - "NewFileEditorUnifiedRole": { + "RoleViewer | file | federated": { + givenActions: getRoleActions(unifiedrole.RoleViewer), + constraints: unifiedrole.UnifiedRoleConditionFile, + listFederatedRoles: true, + unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleViewer, + }, + }, + "RoleFileEditor | file": { givenActions: getRoleActions(unifiedrole.RoleFileEditor), constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -120,7 +129,7 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleFileEditor, }, }, - "NewEditorUnifiedRole": { + "RoleEditor | folder": { givenActions: getRoleActions(unifiedrole.RoleEditor), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -130,7 +139,25 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleEditor, }, }, - "GetRoles 1": { + "RoleEditor | folder | federated": { + givenActions: getRoleActions(unifiedrole.RoleEditor), + constraints: unifiedrole.UnifiedRoleConditionFolder, + listFederatedRoles: true, + unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleViewer, + unifiedrole.RoleEditor, + }, + }, + "RoleEditor | file | federated": { + givenActions: getRoleActions(unifiedrole.RoleEditor), + constraints: unifiedrole.UnifiedRoleConditionFile, + listFederatedRoles: true, + unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ + unifiedrole.RoleViewer, + unifiedrole.RoleFileEditor, + }, + }, + "BuildInRoles | file": { givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -139,7 +166,7 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleFileEditor, }, }, - "GetRoles 2": { + "BuildInRoles | folder": { givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -149,7 +176,7 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleEditor, }, }, - "GetRoles 3": { + "BuildInRoles | drive": { givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionDrive, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -158,12 +185,12 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedrole.RoleManager, }, }, - "single": { + "custom | file": { givenActions: []string{unifiedrole.DriveItemQuotaRead}, constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{}, }, - "mixed": { + "RoleEditorLite and custom | folder": { givenActions: append(getRoleActions(unifiedrole.RoleEditorLite), unifiedrole.DriveItemQuotaRead), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ @@ -176,7 +203,7 @@ func TestGetRolesByPermissions(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { g := NewWithT(t) - generatedDefinitions := unifiedrole.GetRolesByPermissions(unifiedrole.BuildInRoles, tc.givenActions, tc.constraints, false) + generatedDefinitions := unifiedrole.GetRolesByPermissions(unifiedrole.BuildInRoles, tc.givenActions, tc.constraints, tc.listFederatedRoles, false) g.Expect(len(generatedDefinitions)).To(Equal(len(tc.unifiedRoleDefinition)))