enhancement: only use allowed roles for the graph service

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:
 

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

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...)),
 		) {

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

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,

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() {

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,18 +184,23 @@ 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)
 		}
 
 		if exp := permissionsExpirations[id]; exp != nil {
 			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

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())

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

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

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 {

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

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

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
 )