diff --git a/changelog/unreleased/enhancement-unified-roles-management.md b/changelog/unreleased/enhancement-unified-roles-management.md index 5763a580c2c..da1faec415d 100644 --- a/changelog/unreleased/enhancement-unified-roles-management.md +++ b/changelog/unreleased/enhancement-unified-roles-management.md @@ -1,6 +1,8 @@ Enhancement: Unified Roles Management Improved management of unified roles with the introduction of default enabled/disabled states and a new command for listing available roles. +It is important to note that a disabled role does not lose previously assigned permissions; +it only means that the role is not available for new assignments. The following roles are now enabled by default: diff --git a/services/graph/pkg/command/unified_roles.go b/services/graph/pkg/command/unified_roles.go index df1b1d7ef9f..338956c84fc 100644 --- a/services/graph/pkg/command/unified_roles.go +++ b/services/graph/pkg/command/unified_roles.go @@ -44,7 +44,8 @@ func unifiedRolesStatus(cfg *config.Config) *cli.Command { var data [][]string - for _, definition := range unifiedrole.GetDefinitions(unifiedrole.RoleFilterAll()) { + // fixMe: should we use all definitions? + for _, definition := range unifiedrole.GetRoles(unifiedrole.RoleFilterAll()) { data = append(data, []string{"", definition.GetId(), definition.GetDescription()}) } diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go index 77e5183c198..57aaed57be6 100644 --- a/services/graph/pkg/config/defaults/defaultconfig.go +++ b/services/graph/pkg/config/defaults/defaultconfig.go @@ -177,7 +177,7 @@ func EnsureDefaults(cfg *config.Config) { // set default roles, if no roles are defined, we need to take care and provide all the default roles if len(cfg.UnifiedRoles.AvailableRoles) == 0 { - for _, definition := range unifiedrole.GetDefinitions( + for _, definition := range unifiedrole.GetRoles( // filter out the roles that are disabled by default unifiedrole.RoleFilterInvert(unifiedrole.RoleFilterIDs(_disabledByDefaultUnifiedRoleRoleIDs...)), ) { diff --git a/services/graph/pkg/config/parser/parse.go b/services/graph/pkg/config/parser/parse.go index 2638ba2f80c..3bcd06f4f72 100644 --- a/services/graph/pkg/config/parser/parse.go +++ b/services/graph/pkg/config/parser/parse.go @@ -79,9 +79,9 @@ func Validate(cfg *config.Config) error { for _, uid := range cfg.UnifiedRoles.AvailableRoles { // check if the role is known - if len(unifiedrole.GetDefinitions(unifiedrole.RoleFilterIDs(uid))) == 0 { + if len(unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(uid))) == 0 { // collect all possible errors to return them all at once - err = errors.Join(err, fmt.Errorf("%w: %s", unifiedrole.ErrUnknownUnifiedRole, uid)) + err = errors.Join(err, fmt.Errorf("%w: %s", unifiedrole.ErrUnknownRole, uid)) } } diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 1a1744414df..9f8a260fed9 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -104,10 +104,10 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto for _, roleID := range invite.GetRoles() { // only allow roles that are enabled in the config if !slices.Contains(s.config.UnifiedRoles.AvailableRoles, roleID) { - return libregraph.Permission{}, unifiedrole.ErrUnknownUnifiedRole + return libregraph.Permission{}, unifiedrole.ErrUnknownRole } - role, err := unifiedrole.GetDefinition(unifiedrole.RoleFilterIDs(roleID)) + role, err := unifiedrole.GetRole(unifiedrole.RoleFilterIDs(roleID)) if err != nil { s.logger.Debug().Err(err).Interface("role", invite.GetRoles()[0]).Msg("unable to convert requested role") return libregraph.Permission{}, err @@ -127,7 +127,8 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto cs3ResourcePermissions := unifiedrole.PermissionsToCS3ResourcePermissions(unifiedRolePermissions) permission := &libregraph.Permission{} - if role := unifiedrole.CS3ResourcePermissionsToDefinition(cs3ResourcePermissions, condition); role != nil { + availableRoles := unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(s.config.UnifiedRoles.AvailableRoles...)) + if role := unifiedrole.CS3ResourcePermissionsToRole(availableRoles, cs3ResourcePermissions, condition); role != nil { permission.Roles = []string{role.GetId()} } @@ -223,7 +224,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto } else if IsSpaceRoot(statResponse.GetInfo().GetId()) { // permissions on a space root are not handled by a share manager so // they don't get a share-id - permission.SetId(identitySetToSpacePermissionID(permission.GetGrantedToV2())) + permission.SetId(identitySetToSpacePermissionID(permission.GetGrantedToV2(), s.config.UnifiedRoles.AvailableRoles)) } if expiration != nil { @@ -348,6 +349,7 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID LibreGraphPermissionsActionsAllowedValues: allowedActions, LibreGraphPermissionsRolesAllowedValues: conversions.ToValueSlice( unifiedrole.GetRolesByPermissions( + unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(s.config.UnifiedRoles.AvailableRoles...)), allowedActions, condition, false, 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 453cb5794e6..1598f48b2f2 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go @@ -7,6 +7,7 @@ import ( "errors" "net/http" "net/http/httptest" + "slices" "time" gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" @@ -55,6 +56,7 @@ var _ = Describe("DriveItemPermissionsService", func() { OpaqueId: "user", }, } + cache identity.IdentityCache statResponse *provider.StatResponse driveItemId *provider.ResourceId ctx context.Context @@ -67,7 +69,7 @@ var _ = Describe("DriveItemPermissionsService", func() { gatewaySelector = mocks.NewSelectable[gateway.GatewayAPIClient](GinkgoT()) gatewaySelector.On("Next").Return(gatewayClient, nil) - cache := identity.NewIdentityCache(identity.IdentityCacheWithGatewaySelector(gatewaySelector)) + cache = identity.NewIdentityCache(identity.IdentityCacheWithGatewaySelector(gatewaySelector)) cfg := defaults.FullDefaultConfig() service, err := svc.NewDriveItemPermissionsService(logger, gatewaySelector, cache, cfg) @@ -256,9 +258,17 @@ var _ = Describe("DriveItemPermissionsService", func() { }) It("fails with unknown or disable role", func() { + cfg := defaults.FullDefaultConfig() + slices.DeleteFunc(cfg.UnifiedRoles.AvailableRoles, func(s string) bool { + // SecureViewer is enabled in ci, we need to remove it in the unit test + return s != unifiedrole.UnifiedRoleSecureViewerID + }) + service, err := svc.NewDriveItemPermissionsService(log.NewLogger(), gatewaySelector, cache, cfg) + Expect(err).ToNot(HaveOccurred()) + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID, unifiedrole.UnifiedRoleSecureViewerID} - _, err := driveItemPermissionsService.Invite(context.Background(), driveItemId, driveItemInvite) - Expect(err).To(MatchError(unifiedrole.ErrUnknownUnifiedRole)) + _, err = service.Invite(context.Background(), driveItemId, driveItemInvite) + Expect(err).To(MatchError(unifiedrole.ErrUnknownRole)) }) }) Describe("SpaceRootInvite", func() { diff --git a/services/graph/pkg/service/v0/base.go b/services/graph/pkg/service/v0/base.go index 6d7dabbd186..630f1127b25 100644 --- a/services/graph/pkg/service/v0/base.go +++ b/services/graph/pkg/service/v0/base.go @@ -85,7 +85,8 @@ func (g BaseGraphService) CS3ReceivedSharesToDriveItems(ctx context.Context, rec return nil, err } - return cs3ReceivedSharesToDriveItems(ctx, g.logger, gatewayClient, g.identityCache, receivedShares) + availableRoles := unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(g.config.UnifiedRoles.AvailableRoles...)) + return cs3ReceivedSharesToDriveItems(ctx, g.logger, gatewayClient, g.identityCache, receivedShares, availableRoles) } func (g BaseGraphService) CS3ReceivedOCMSharesToDriveItems(ctx context.Context, receivedShares []*ocm.ReceivedShare) ([]libregraph.DriveItem, error) { @@ -183,7 +184,7 @@ func (g BaseGraphService) cs3SpacePermissionsToLibreGraph(ctx context.Context, s } else { identitySet.SetUser(identity) } - p.SetId(identitySetToSpacePermissionID(identitySet)) + p.SetId(identitySetToSpacePermissionID(identitySet, g.config.UnifiedRoles.AvailableRoles)) p.SetGrantedToV2(identitySet) } @@ -191,10 +192,15 @@ func (g BaseGraphService) cs3SpacePermissionsToLibreGraph(ctx context.Context, s p.SetExpirationDateTime(time.Unix(int64(exp.GetSeconds()), int64(exp.GetNanos()))) } - if role := unifiedrole.CS3ResourcePermissionsToDefinition(perm, unifiedrole.UnifiedRoleConditionDrive); role != nil { + availableRoles := unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(g.config.UnifiedRoles.AvailableRoles...)) + if role := unifiedrole.CS3ResourcePermissionsToRole( + availableRoles, + perm, + unifiedrole.UnifiedRoleConditionDrive, + ); role != nil { switch apiVersion { case APIVersion_1: - if r := unifiedrole.GetLegacyDefinitionName(*role); r != "" { + if r := unifiedrole.GetLegacyRoleName(*role); r != "" { p.SetRoles([]string{r}) } case APIVersion_1_Beta_1: @@ -392,7 +398,9 @@ func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *c if share.GetCtime() != nil { perm.SetCreatedDateTime(cs3TimestampToTime(share.GetCtime())) } - role := unifiedrole.CS3ResourcePermissionsToDefinition( + // fixMe: should we use all roles? + role := unifiedrole.CS3ResourcePermissionsToRole( + unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(g.config.UnifiedRoles.AvailableRoles...)), share.GetPermissions().GetPermissions(), roleCondition, ) @@ -695,7 +703,7 @@ func (g BaseGraphService) updateUserShare(ctx context.Context, permissionID stri var permissionsUpdated, ok bool if roles, ok = newPermission.GetRolesOk(); ok && len(roles) > 0 { for _, roleID := range roles { - role, err := unifiedrole.GetDefinition(unifiedrole.RoleFilterIDs(roleID)) + role, err := unifiedrole.GetRole(unifiedrole.RoleFilterIDs(roleID)) if err != nil { g.logger.Debug().Err(err).Interface("role", role).Msg("unable to convert requested role") return nil, err diff --git a/services/graph/pkg/service/v0/rolemanagement.go b/services/graph/pkg/service/v0/rolemanagement.go index d1eb4299978..6f769136267 100644 --- a/services/graph/pkg/service/v0/rolemanagement.go +++ b/services/graph/pkg/service/v0/rolemanagement.go @@ -14,7 +14,7 @@ import ( // GetRoleDefinitions a list of permission roles than can be used when sharing with users or groups func (g Graph) GetRoleDefinitions(w http.ResponseWriter, r *http.Request) { render.Status(r, http.StatusOK) - render.JSON(w, r, unifiedrole.GetDefinitions(unifiedrole.RoleFilterIDs(g.config.UnifiedRoles.AvailableRoles...))) + render.JSON(w, r, unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(g.config.UnifiedRoles.AvailableRoles...))) } // GetRoleDefinition a permission role than can be used when sharing with users or groups @@ -26,7 +26,7 @@ func (g Graph) GetRoleDefinition(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "unescaping role id failed") return } - role, err := unifiedrole.GetDefinition(unifiedrole.RoleFilterIDs(roleID)) + role, err := unifiedrole.GetRole(unifiedrole.RoleFilterIDs(roleID)) if err != nil { logger.Debug().Str("roleID", roleID).Msg("could not get role: not found") errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, err.Error()) diff --git a/services/graph/pkg/service/v0/sharedwithme.go b/services/graph/pkg/service/v0/sharedwithme.go index d369e508205..385995f700f 100644 --- a/services/graph/pkg/service/v0/sharedwithme.go +++ b/services/graph/pkg/service/v0/sharedwithme.go @@ -10,6 +10,7 @@ import ( libregraph "github.com/owncloud/libre-graph-api-go" "github.com/owncloud/ocis/v2/services/graph/pkg/errorcode" + "github.com/owncloud/ocis/v2/services/graph/pkg/unifiedrole" ) // ListSharedWithMe lists the files shared with the current user. @@ -39,7 +40,8 @@ func (g Graph) listSharedWithMe(ctx context.Context) ([]libregraph.DriveItem, er g.logger.Error().Err(err).Msg("listing shares failed") return nil, err } - driveItems, err := cs3ReceivedSharesToDriveItems(ctx, g.logger, gatewayClient, g.identityCache, listReceivedSharesResponse.GetShares()) + availableRoles := unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(g.config.UnifiedRoles.AvailableRoles...)) + driveItems, err := cs3ReceivedSharesToDriveItems(ctx, g.logger, gatewayClient, g.identityCache, listReceivedSharesResponse.GetShares(), availableRoles) if err != nil { g.logger.Error().Err(err).Msg("could not convert received shares to drive items") return nil, err diff --git a/services/graph/pkg/service/v0/utils.go b/services/graph/pkg/service/v0/utils.go index 91a4ed48cbf..6e10213dd6b 100644 --- a/services/graph/pkg/service/v0/utils.go +++ b/services/graph/pkg/service/v0/utils.go @@ -130,7 +130,7 @@ func groupIdToIdentity(ctx context.Context, cache identity.IdentityCache, groupI // As permissions on space to not map to a cs3 share we need something else of the ids. So we just // construct the id for the id of the user or group that the permission applies to and prefix that // with a "u:" for userids and "g:" for group ids. -func identitySetToSpacePermissionID(identitySet libregraph.SharePointIdentitySet) (id string) { +func identitySetToSpacePermissionID(identitySet libregraph.SharePointIdentitySet, allowedRoleIDs []string) (id string) { switch { case identitySet.HasUser(): id = "u:" + identitySet.User.GetId() @@ -144,7 +144,9 @@ func cs3ReceivedSharesToDriveItems(ctx context.Context, logger *log.Logger, gatewayClient gateway.GatewayAPIClient, identityCache identity.IdentityCache, - receivedShares []*collaboration.ReceivedShare) ([]libregraph.DriveItem, error) { + receivedShares []*collaboration.ReceivedShare, + availableRoles []*libregraph.UnifiedRoleDefinition, +) ([]libregraph.DriveItem, error) { group := new(errgroup.Group) // Set max concurrency @@ -186,7 +188,7 @@ func cs3ReceivedSharesToDriveItems(ctx context.Context, return errCode } - driveItem, err := fillDriveItemPropertiesFromReceivedShare(ctx, logger, identityCache, receivedShares, shareStat.GetInfo()) + driveItem, err := fillDriveItemPropertiesFromReceivedShare(ctx, logger, identityCache, receivedShares, shareStat.GetInfo(), availableRoles) if err != nil { return err } @@ -323,7 +325,7 @@ func cs3ReceivedSharesToDriveItems(ctx context.Context, func fillDriveItemPropertiesFromReceivedShare(ctx context.Context, logger *log.Logger, identityCache identity.IdentityCache, receivedShares []*collaboration.ReceivedShare, - resourceInfo *storageprovider.ResourceInfo) (*libregraph.DriveItem, error) { + resourceInfo *storageprovider.ResourceInfo, availableRoles []*libregraph.UnifiedRoleDefinition) (*libregraph.DriveItem, error) { driveItem := libregraph.NewDriveItem() permissions := make([]libregraph.Permission, 0, len(receivedShares)) @@ -337,7 +339,7 @@ func fillDriveItemPropertiesFromReceivedShare(ctx context.Context, logger *log.L oldestReceivedShare = receivedShare } - permission, err := cs3ReceivedShareToLibreGraphPermissions(ctx, logger, identityCache, receivedShare, resourceInfo) + permission, err := cs3ReceivedShareToLibreGraphPermissions(ctx, logger, identityCache, receivedShare, resourceInfo, availableRoles) if err != nil { return driveItem, err } @@ -398,7 +400,7 @@ func fillDriveItemPropertiesFromReceivedShare(ctx context.Context, logger *log.L func cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, logger *log.Logger, identityCache identity.IdentityCache, receivedShare *collaboration.ReceivedShare, - resourceInfo *storageprovider.ResourceInfo) (*libregraph.Permission, error) { + resourceInfo *storageprovider.ResourceInfo, availableRoles []*libregraph.UnifiedRoleDefinition) (*libregraph.Permission, error) { permission := libregraph.NewPermission() if id := receivedShare.GetShare().GetId().GetOpaqueId(); id != "" { permission.SetId(id) @@ -417,8 +419,12 @@ func cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, logger *log.Lo if err != nil { return nil, err } - role := unifiedrole.CS3ResourcePermissionsToDefinition(permissionSet, condition) + role := unifiedrole.CS3ResourcePermissionsToRole( + availableRoles, + permissionSet, + condition, + ) if role != nil { permission.SetRoles([]string{role.GetId()}) } diff --git a/services/graph/pkg/unifiedrole/conversion.go b/services/graph/pkg/unifiedrole/conversion.go index 0f73ac7e2e0..18724777fd4 100644 --- a/services/graph/pkg/unifiedrole/conversion.go +++ b/services/graph/pkg/unifiedrole/conversion.go @@ -142,17 +142,15 @@ func CS3ResourcePermissionsToLibregraphActions(p *provider.ResourcePermissions) return actions } -// CS3ResourcePermissionsToDefinition tries to find the UnifiedRoleDefinition that matches the supplied -// CS3 ResourcePermissions and constraints. -func _CS3ResourcePermissionsToDefinition(p *provider.ResourcePermissions, constraints string) *libregraph.UnifiedRoleDefinition { - a := CS3ResourcePermissionsToLibregraphActions(p) +// CS3ResourcePermissionsToRole converts the provided cs3 ResourcePermissions to a libregraph UnifiedRoleDefinition +func CS3ResourcePermissionsToRole(roleSet []*libregraph.UnifiedRoleDefinition, p *provider.ResourcePermissions, constraints string) *libregraph.UnifiedRoleDefinition { actionSet := map[string]struct{}{} - for _, action := range a { + for _, action := range CS3ResourcePermissionsToLibregraphActions(p) { actionSet[action] = struct{}{} } var res *libregraph.UnifiedRoleDefinition - for _, uRole := range GetDefinitions(RoleFilterAll()) { + for _, uRole := range roleSet { matchFound := false for _, uPerm := range uRole.GetRolePermissions() { if uPerm.GetCondition() != constraints { @@ -174,6 +172,7 @@ func _CS3ResourcePermissionsToDefinition(p *provider.ResourcePermissions, constr return res } +// resourceActionsEqual checks if the provided actions are equal to the actions defined for a resource func resourceActionsEqual(targetActionSet map[string]struct{}, actions []string) bool { if len(targetActionSet) != len(actions) { return false @@ -187,16 +186,6 @@ func resourceActionsEqual(targetActionSet map[string]struct{}, actions []string) return true } -func CS3ResourcePermissionsToDefinition(p *provider.ResourcePermissions, constraints string) *libregraph.UnifiedRoleDefinition { - var res *libregraph.UnifiedRoleDefinition - matches := GetDefinitions(RoleFilterPermission(RoleFilterMatchExact, constraints, CS3ResourcePermissionsToLibregraphActions(p)...)) - if len(matches) >= 1 { - res = matches[0] - } - - return res -} - // cs3RoleToDisplayName converts a CS3 role to a human-readable display name func cs3RoleToDisplayName(role *conversions.Role) string { if role == nil { diff --git a/services/graph/pkg/unifiedrole/conversion_test.go b/services/graph/pkg/unifiedrole/conversion_test.go index eec08b2045b..a09c3dc378d 100644 --- a/services/graph/pkg/unifiedrole/conversion_test.go +++ b/services/graph/pkg/unifiedrole/conversion_test.go @@ -48,7 +48,7 @@ func TestPermissionsToCS3ResourcePermissions(t *testing.T) { } } -func TestCS3ResourcePermissionsToDefinition(t *testing.T) { +func TestCS3ResourcePermissionsToRole(t *testing.T) { tests := map[string]struct { cs3ResourcePermissions *provider.ResourcePermissions unifiedRoleDefinition *libregraph.UnifiedRoleDefinition @@ -69,7 +69,7 @@ func TestCS3ResourcePermissionsToDefinition(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { NewWithT(t).Expect( - unifiedrole.CS3ResourcePermissionsToDefinition(tc.cs3ResourcePermissions, tc.constraints), + unifiedrole.CS3ResourcePermissionsToRole(unifiedrole.BuildInRoles, tc.cs3ResourcePermissions, tc.constraints), ).To(Equal(tc.unifiedRoleDefinition)) }) } diff --git a/services/graph/pkg/unifiedrole/errors.go b/services/graph/pkg/unifiedrole/errors.go index e4f095e7a4b..3033503073f 100644 --- a/services/graph/pkg/unifiedrole/errors.go +++ b/services/graph/pkg/unifiedrole/errors.go @@ -5,9 +5,6 @@ import ( ) var ( - // ErrUnknownUnifiedRole is returned when an unknown unified role is requested. - ErrUnknownUnifiedRole = errors.New("unknown unified role, check if the role is enabled") - - // ErrTooManyResults is returned when a filter returns too many results. - ErrTooManyResults = errors.New("too many results, consider using a more specific filter") + // ErrUnknownRole is returned when an unknown unified role is requested. + ErrUnknownRole = errors.New("unknown role, check if the role is enabled") ) diff --git a/services/graph/pkg/unifiedrole/export_test.go b/services/graph/pkg/unifiedrole/export_test.go index d402ddc44d1..f834bcc554f 100644 --- a/services/graph/pkg/unifiedrole/export_test.go +++ b/services/graph/pkg/unifiedrole/export_test.go @@ -1,7 +1,6 @@ package unifiedrole var ( - // roles RoleViewer = roleViewer RoleSpaceViewer = roleSpaceViewer RoleEditor = roleEditor @@ -11,6 +10,7 @@ var ( RoleManager = roleManager RoleSecureViewer = roleSecureViewer - // functions - WeightDefinitions = weightDefinitions + BuildInRoles = buildInRoles + + WeightDefinitions = weightRoles ) diff --git a/services/graph/pkg/unifiedrole/filter.go b/services/graph/pkg/unifiedrole/filter.go index 7e7f020cba0..52a61c91c2d 100644 --- a/services/graph/pkg/unifiedrole/filter.go +++ b/services/graph/pkg/unifiedrole/filter.go @@ -8,18 +8,7 @@ import ( type ( // RoleFilter is used to filter role collections - RoleFilter func(r *libregraph.UnifiedRoleDefinition) bool - - // RoleFilterMatch defines the match behavior of a role filter - RoleFilterMatch int -) - -const ( - // RoleFilterMatchExact is the behavior for role filters that require an exact match - RoleFilterMatchExact RoleFilterMatch = iota - - // RoleFilterMatchSome is the behavior for role filters that require some match - RoleFilterMatchSome + RoleFilter func(*libregraph.UnifiedRoleDefinition) bool ) // RoleFilterInvert inverts the provided role filter @@ -44,44 +33,12 @@ func RoleFilterIDs(ids ...string) RoleFilter { } } -// RoleFilterPermission returns a role filter that matches the provided condition and actions pair -func RoleFilterPermission(matchBehavior RoleFilterMatch, condition string, wantActions ...string) RoleFilter { - return func(r *libregraph.UnifiedRoleDefinition) bool { - for _, permission := range r.GetRolePermissions() { - if permission.GetCondition() != condition { - continue - } - - givenActions := permission.GetAllowedResourceActions() - - switch { - case matchBehavior == RoleFilterMatchExact && slices.Equal(givenActions, wantActions): - return true - case matchBehavior == RoleFilterMatchSome: - matches := 0 - - for _, action := range givenActions { - if !slices.Contains(wantActions, action) { - break - } - - matches++ - } - - return len(givenActions) == matches - } - } - - return false - } -} - // filterRoles filters the provided roles by the provided filter -func filterRoles(roles []*libregraph.UnifiedRoleDefinition, filter RoleFilter) []*libregraph.UnifiedRoleDefinition { +func filterRoles(roles []*libregraph.UnifiedRoleDefinition, f RoleFilter) []*libregraph.UnifiedRoleDefinition { return slices.DeleteFunc( slices.Clone(roles), func(r *libregraph.UnifiedRoleDefinition) bool { - return !filter(r) + return !f(r) }, ) } diff --git a/services/graph/pkg/unifiedrole/filter_test.go b/services/graph/pkg/unifiedrole/filter_test.go index c2b28a63985..ce30d644216 100644 --- a/services/graph/pkg/unifiedrole/filter_test.go +++ b/services/graph/pkg/unifiedrole/filter_test.go @@ -5,7 +5,6 @@ import ( . "github.com/onsi/gomega" libregraph "github.com/owncloud/libre-graph-api-go" - "google.golang.org/protobuf/proto" "github.com/owncloud/ocis/v2/services/graph/pkg/unifiedrole" ) @@ -22,7 +21,9 @@ func TestRoleFilterIDs(t *testing.T) { func TestRoleFilterInvert(t *testing.T) { NewWithT(t).Expect( unifiedrole.RoleFilterInvert( - unifiedrole.RoleFilterAll(), + func(_ *libregraph.UnifiedRoleDefinition) bool { + return true + }, )(unifiedrole.RoleEditorLite), ).To(BeFalse()) } @@ -32,98 +33,3 @@ func TestRoleFilterAll(t *testing.T) { unifiedrole.RoleFilterAll()(unifiedrole.RoleEditorLite), ).To(BeTrue()) } - -func TestRoleFilterPermissions(t *testing.T) { - tests := map[string]struct { - unifiedRolePermission []libregraph.UnifiedRolePermission - filterCondition string - filterActions []string - filterMatch bool - }{ - "true | single": { - unifiedRolePermission: []libregraph.UnifiedRolePermission{ - { - Condition: proto.String(unifiedrole.UnifiedRoleConditionDrive), - AllowedResourceActions: []string{ - unifiedrole.DriveItemPermissionsCreate, - }, - }, - }, - filterCondition: unifiedrole.UnifiedRoleConditionDrive, - filterActions: []string{ - unifiedrole.DriveItemPermissionsCreate, - }, - filterMatch: true, - }, - "true | multiple": { - unifiedRolePermission: []libregraph.UnifiedRolePermission{ - { - Condition: proto.String(unifiedrole.UnifiedRoleConditionFolder), - AllowedResourceActions: []string{ - unifiedrole.DriveItemDeletedRead, - }, - }, - { - Condition: proto.String(unifiedrole.UnifiedRoleConditionDrive), - AllowedResourceActions: []string{ - unifiedrole.DriveItemPermissionsCreate, - }, - }, - }, - filterCondition: unifiedrole.UnifiedRoleConditionDrive, - filterActions: []string{ - unifiedrole.DriveItemPermissionsCreate, - }, - filterMatch: true, - }, - "false | cross match": { - unifiedRolePermission: []libregraph.UnifiedRolePermission{ - { - Condition: proto.String(unifiedrole.UnifiedRoleConditionDrive), - AllowedResourceActions: []string{ - unifiedrole.DriveItemDeletedRead, - }, - }, - { - Condition: proto.String(unifiedrole.UnifiedRoleConditionFolder), - AllowedResourceActions: []string{ - unifiedrole.DriveItemPermissionsCreate, - }, - }, - }, - filterCondition: unifiedrole.UnifiedRoleConditionDrive, - filterActions: []string{unifiedrole.DriveItemPermissionsCreate}, - filterMatch: false, - }, - "false | too many actions": { - unifiedRolePermission: []libregraph.UnifiedRolePermission{ - { - Condition: proto.String(unifiedrole.UnifiedRoleConditionDrive), - AllowedResourceActions: []string{ - unifiedrole.DriveItemDeletedRead, - unifiedrole.DriveItemPermissionsCreate, - }, - }, - }, - filterCondition: unifiedrole.UnifiedRoleConditionDrive, - filterActions: []string{ - unifiedrole.DriveItemPermissionsCreate, - }, - filterMatch: false, - }, - } - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - NewWithT(t).Expect( - unifiedrole.RoleFilterPermission( - unifiedrole.RoleFilterMatchExact, - tc.filterCondition, - tc.filterActions..., - )(&libregraph.UnifiedRoleDefinition{ - RolePermissions: tc.unifiedRolePermission, - }), - ).To(Equal(tc.filterMatch)) - }) - } -} diff --git a/services/graph/pkg/unifiedrole/roles.go b/services/graph/pkg/unifiedrole/roles.go index 6718e5b1fa6..169b1a982ec 100644 --- a/services/graph/pkg/unifiedrole/roles.go +++ b/services/graph/pkg/unifiedrole/roles.go @@ -228,41 +228,68 @@ var ( }() ) -// GetDefinitions returns a role filter that matches the provided resources -func GetDefinitions(filter RoleFilter) []*libregraph.UnifiedRoleDefinition { - return filterRoles(buildInRoles, filter) +// GetRoles returns a role filter that matches the provided resources +func GetRoles(f RoleFilter) []*libregraph.UnifiedRoleDefinition { + return filterRoles(buildInRoles, f) } -// GetDefinition returns a role filter that matches the provided resources -func GetDefinition(filter RoleFilter) (*libregraph.UnifiedRoleDefinition, error) { - definitions := filterRoles(buildInRoles, filter) - if len(definitions) == 0 { - return nil, ErrUnknownUnifiedRole +// GetRole returns a role filter that matches the provided resources +func GetRole(f RoleFilter) (*libregraph.UnifiedRoleDefinition, error) { + roles := filterRoles(buildInRoles, f) + if len(roles) == 0 { + return nil, ErrUnknownRole } - return definitions[0], nil + return roles[0], nil } // GetRolesByPermissions returns a list of role definitions // that match the provided actions and constraints -func GetRolesByPermissions(actions []string, constraints string, descending bool) []*libregraph.UnifiedRoleDefinition { - roles := GetDefinitions(RoleFilterPermission(RoleFilterMatchSome, constraints, actions...)) - roles = weightDefinitions(roles, constraints, descending) +func GetRolesByPermissions(roleSet []*libregraph.UnifiedRoleDefinition, actions []string, constraints string, descending bool) []*libregraph.UnifiedRoleDefinition { + roles := make([]*libregraph.UnifiedRoleDefinition, 0, len(roleSet)) - return roles + for _, role := range roleSet { + var match bool + + for _, permission := range role.GetRolePermissions() { + if permission.GetCondition() != constraints { + continue + } + + for i, action := range permission.GetAllowedResourceActions() { + if !slices.Contains(actions, action) { + break + } + if i == len(permission.GetAllowedResourceActions())-1 { + match = true + } + } + + if match { + break + } + } + + if match { + roles = append(roles, role) + } + + } + + return weightRoles(roles, constraints, descending) } -// GetLegacyDefinitionName returns the legacy role name for the provided role -func GetLegacyDefinitionName(definition libregraph.UnifiedRoleDefinition) string { - return legacyNames[definition.GetId()] +// GetLegacyRoleName returns the legacy role name for the provided role +func GetLegacyRoleName(role libregraph.UnifiedRoleDefinition) string { + return legacyNames[role.GetId()] } -// weightDefinitions sorts the provided role definitions by the number of permissions[n].actions they grant, +// weightRoles sorts the provided role definitions by the number of permissions[n].actions they grant, // the implementation is optimistic and assumes that the weight relies on the number of available actions. // descending - false - sorts the roles from least to most permissions // descending - true - sorts the roles from most to least permissions -func weightDefinitions(definitions []*libregraph.UnifiedRoleDefinition, constraints string, descending bool) []*libregraph.UnifiedRoleDefinition { - slices.SortFunc(definitions, func(i, j *libregraph.UnifiedRoleDefinition) int { +func weightRoles(roleSet []*libregraph.UnifiedRoleDefinition, constraints string, descending bool) []*libregraph.UnifiedRoleDefinition { + slices.SortFunc(roleSet, func(i, j *libregraph.UnifiedRoleDefinition) int { var ia []string for _, rp := range i.GetRolePermissions() { if rp.GetCondition() == constraints { @@ -285,12 +312,12 @@ func weightDefinitions(definitions []*libregraph.UnifiedRoleDefinition, constrai } }) - for i, definition := range definitions { - definition.LibreGraphWeight = libregraph.PtrInt32(int32(i) + 1) + for i, role := range roleSet { + role.LibreGraphWeight = libregraph.PtrInt32(int32(i) + 1) } // return for the sake of consistency, optional because the slice is modified in place - return definitions + return roleSet } // GetAllowedResourceActions returns the allowed resource actions for the provided role by condition diff --git a/services/graph/pkg/unifiedrole/roles_test.go b/services/graph/pkg/unifiedrole/roles_test.go index b51fb24ff10..76a070629b3 100644 --- a/services/graph/pkg/unifiedrole/roles_test.go +++ b/services/graph/pkg/unifiedrole/roles_test.go @@ -27,14 +27,14 @@ func TestGetDefinition(t *testing.T) { }, "fail unknown": { ids: []string{"unknown"}, - expectError: unifiedrole.ErrUnknownUnifiedRole, + expectError: unifiedrole.ErrUnknownRole, }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { g := NewWithT(t) - definition, err := unifiedrole.GetDefinition(unifiedrole.RoleFilterIDs(tc.ids...)) + definition, err := unifiedrole.GetRole(unifiedrole.RoleFilterIDs(tc.ids...)) if tc.expectError != nil { g.Expect(err).To(MatchError(tc.expectError)) @@ -96,7 +96,7 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedRoleDefinition []*libregraph.UnifiedRoleDefinition }{ "ViewerUnifiedRole": { - givenActions: rolesToAction(unifiedrole.RoleViewer), + givenActions: getRoleActions(unifiedrole.RoleViewer), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSecureViewer, @@ -104,7 +104,7 @@ func TestGetRolesByPermissions(t *testing.T) { }, }, "ViewerUnifiedRole | share": { - givenActions: rolesToAction(unifiedrole.RoleViewer), + givenActions: getRoleActions(unifiedrole.RoleViewer), constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSecureViewer, @@ -112,7 +112,7 @@ func TestGetRolesByPermissions(t *testing.T) { }, }, "NewFileEditorUnifiedRole": { - givenActions: rolesToAction(unifiedrole.RoleFileEditor), + givenActions: getRoleActions(unifiedrole.RoleFileEditor), constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSecureViewer, @@ -121,7 +121,7 @@ func TestGetRolesByPermissions(t *testing.T) { }, }, "NewEditorUnifiedRole": { - givenActions: rolesToAction(unifiedrole.RoleEditor), + givenActions: getRoleActions(unifiedrole.RoleEditor), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSecureViewer, @@ -131,7 +131,7 @@ func TestGetRolesByPermissions(t *testing.T) { }, }, "GetRoles 1": { - givenActions: rolesToAction(unifiedrole.GetDefinitions(unifiedrole.RoleFilterAll())...), + givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionFile, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSecureViewer, @@ -140,7 +140,7 @@ func TestGetRolesByPermissions(t *testing.T) { }, }, "GetRoles 2": { - givenActions: rolesToAction(unifiedrole.GetDefinitions(unifiedrole.RoleFilterAll())...), + givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSecureViewer, @@ -150,7 +150,7 @@ func TestGetRolesByPermissions(t *testing.T) { }, }, "GetRoles 3": { - givenActions: rolesToAction(unifiedrole.GetDefinitions(unifiedrole.RoleFilterAll())...), + givenActions: getRoleActions(unifiedrole.BuildInRoles...), constraints: unifiedrole.UnifiedRoleConditionDrive, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSpaceViewer, @@ -164,7 +164,7 @@ func TestGetRolesByPermissions(t *testing.T) { unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{}, }, "mixed": { - givenActions: append(rolesToAction(unifiedrole.RoleEditorLite), unifiedrole.DriveItemQuotaRead), + givenActions: append(getRoleActions(unifiedrole.RoleEditorLite), unifiedrole.DriveItemQuotaRead), constraints: unifiedrole.UnifiedRoleConditionFolder, unifiedRoleDefinition: []*libregraph.UnifiedRoleDefinition{ unifiedrole.RoleSecureViewer, @@ -176,7 +176,7 @@ func TestGetRolesByPermissions(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { g := NewWithT(t) - generatedDefinitions := unifiedrole.GetRolesByPermissions(tc.givenActions, tc.constraints, false) + generatedDefinitions := unifiedrole.GetRolesByPermissions(unifiedrole.BuildInRoles, tc.givenActions, tc.constraints, false) g.Expect(len(generatedDefinitions)).To(Equal(len(tc.unifiedRoleDefinition))) @@ -185,7 +185,7 @@ func TestGetRolesByPermissions(t *testing.T) { g.Expect(*generatedDefinition.LibreGraphWeight).To(Equal(int32(i + 1))) } - generatedActions := rolesToAction(generatedDefinitions...) + generatedActions := getRoleActions(generatedDefinitions...) g.Expect(len(tc.givenActions) >= len(generatedActions)).To(BeTrue()) for _, generatedAction := range generatedActions { diff --git a/services/graph/pkg/unifiedrole/unifiedrole_suite_test.go b/services/graph/pkg/unifiedrole/unifiedrole_suite_test.go index 098870b3cee..1b5760fe9ae 100644 --- a/services/graph/pkg/unifiedrole/unifiedrole_suite_test.go +++ b/services/graph/pkg/unifiedrole/unifiedrole_suite_test.go @@ -6,7 +6,7 @@ import ( libregraph "github.com/owncloud/libre-graph-api-go" ) -func rolesToAction(definitions ...*libregraph.UnifiedRoleDefinition) []string { +func getRoleActions(definitions ...*libregraph.UnifiedRoleDefinition) []string { var actions []string for _, definition := range definitions { diff --git a/services/graph/pkg/validate/libregraph.go b/services/graph/pkg/validate/libregraph.go index 8e863dbfc7d..4a3dce46df9 100644 --- a/services/graph/pkg/validate/libregraph.go +++ b/services/graph/pkg/validate/libregraph.go @@ -90,9 +90,10 @@ func rolesAndActions(ctx context.Context, sl validator.StructLevel, roles, actio switch roles, ok := ctx.Value(_contextRoleIDsValueKey).([]string); { case ok: - definitions = unifiedrole.GetDefinitions(unifiedrole.RoleFilterIDs(roles...)) + definitions = unifiedrole.GetRoles(unifiedrole.RoleFilterIDs(roles...)) default: - definitions = unifiedrole.GetDefinitions(unifiedrole.RoleFilterAll()) + // it the ctx does not contain the allowed role IDs, we need to fall back to all roles + definitions = unifiedrole.GetRoles(unifiedrole.RoleFilterAll()) } for _, definition := range definitions { @@ -121,8 +122,8 @@ func rolesAndActions(ctx context.Context, sl validator.StructLevel, roles, actio sl.ReportError(roles, "Roles", "Roles", "available_role", "") } - for _, role := range actions { - if slices.Contains(availableActions, role) { + for _, action := range actions { + if slices.Contains(availableActions, action) { continue } diff --git a/services/graph/pkg/validate/libregraph_test.go b/services/graph/pkg/validate/libregraph_test.go index dfb195b7efa..ecf3cfdd83f 100644 --- a/services/graph/pkg/validate/libregraph_test.go +++ b/services/graph/pkg/validate/libregraph_test.go @@ -16,10 +16,12 @@ import ( type validatableFactory[T any] func() (T, bool) var _ = Describe("libregraph", func() { + var ctx context.Context var driveItemInvite libregraph.DriveItemInvite var driveRecipient libregraph.DriveRecipient BeforeEach(func() { + ctx = context.Background() driveRecipient = libregraph.DriveRecipient{ ObjectId: conversions.ToPointer("1"), LibreGraphRecipientType: conversions.ToPointer("user"), @@ -27,18 +29,17 @@ var _ = Describe("libregraph", func() { driveItemInvite = libregraph.DriveItemInvite{ Recipients: []libregraph.DriveRecipient{driveRecipient}, - Roles: []string{role.UnifiedRoleEditorID}, - LibreGraphPermissionsActions: []string{role.DriveItemVersionsUpdate}, + Roles: []string{unifiedrole.UnifiedRoleEditorID}, + LibreGraphPermissionsActions: []string{unifiedrole.DriveItemVersionsUpdate}, ExpirationDateTime: libregraph.PtrTime(time.Now().Add(time.Hour)), } - }) DescribeTable("DriveItemInvite", func(factories ...validatableFactory[libregraph.DriveItemInvite]) { for _, factory := range factories { s, pass := factory() - switch err := validate.StructCtx(context.Background(), s); pass { + switch err := validate.StructCtx(ctx, s); pass { case false: Expect(err).To(HaveOccurred()) default: @@ -61,8 +62,8 @@ var _ = Describe("libregraph", func() { }), Entry("fail: multiple role assignment", func() (libregraph.DriveItemInvite, bool) { driveItemInvite.Roles = []string{ - role.UnifiedRoleEditorID, - role.UnifiedRoleManagerID, + unifiedrole.UnifiedRoleEditorID, + unifiedrole.UnifiedRoleManagerID, } driveItemInvite.LibreGraphPermissionsActions = nil return driveItemInvite, false @@ -72,6 +73,14 @@ var _ = Describe("libregraph", func() { driveItemInvite.LibreGraphPermissionsActions = nil return driveItemInvite, false }), + Entry("fail: disabled role", func() (libregraph.DriveItemInvite, bool) { + ctx = validate.ContextWithAllowedRoleIDs(ctx, []string{unifiedrole.UnifiedRoleEditorID}) + driveItemInvite.Roles = []string{ + unifiedrole.UnifiedRoleSecureViewerID, + } + driveItemInvite.LibreGraphPermissionsActions = nil + return driveItemInvite, false + }), Entry("fail: unknown action", func() (libregraph.DriveItemInvite, bool) { driveItemInvite.Roles = nil driveItemInvite.LibreGraphPermissionsActions = []string{"foo"} @@ -84,8 +93,8 @@ var _ = Describe("libregraph", func() { }), Entry("fail: different number of roles and actions", func() (libregraph.DriveItemInvite, bool) { driveItemInvite.LibreGraphPermissionsActions = []string{ - role.DriveItemVersionsUpdate, - role.DriveItemChildrenCreate, + unifiedrole.DriveItemVersionsUpdate, + unifiedrole.DriveItemChildrenCreate, } return driveItemInvite, false }), diff --git a/services/web/pkg/theme/service_test.go b/services/web/pkg/theme/service_test.go index eab4959c38e..9b8efcc037d 100644 --- a/services/web/pkg/theme/service_test.go +++ b/services/web/pkg/theme/service_test.go @@ -70,5 +70,5 @@ func TestService_Get(t *testing.T) { // brandingTheme assert.Equal(t, jsonData.Get("_branding").String(), "_branding") // themeDefaults - assert.Equal(t, jsonData.Get("common.shareRoles."+role.UnifiedRoleViewerID+".name").String(), "UnifiedRoleViewer") + assert.Equal(t, jsonData.Get("common.shareRoles."+unifiedrole.UnifiedRoleViewerID+".name").String(), "UnifiedRoleViewer") }