From 4638280d21b819e7dbcbb8cee0442cead8d052e4 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Tue, 6 Aug 2024 11:55:51 +0200 Subject: [PATCH] enhancement: add allowed role validation to the go-playground validator --- .../pkg/config/defaults/defaultconfig.go | 3 ++ .../service/v0/api_driveitem_permissions.go | 22 ++++++---- .../v0/api_driveitem_permissions_test.go | 40 ++++++++++++++++++- .../graph/pkg/service/v0/rolemanagement.go | 14 +++---- services/graph/pkg/service/v0/service.go | 2 +- services/graph/pkg/unifiedrole/errors.go | 2 +- services/graph/pkg/validate/libregraph.go | 38 ++++++++++++------ 7 files changed, 88 insertions(+), 33 deletions(-) diff --git a/services/graph/pkg/config/defaults/defaultconfig.go b/services/graph/pkg/config/defaults/defaultconfig.go index 4cd513bb82..1d13d88c93 100644 --- a/services/graph/pkg/config/defaults/defaultconfig.go +++ b/services/graph/pkg/config/defaults/defaultconfig.go @@ -114,6 +114,9 @@ func DefaultConfig() *config.Config { Cluster: "ocis-cluster", EnableTLS: false, }, + UnifiedRoles: config.UnifiedRoles{ + AvailableRoles: nil, // will be populated with defaults in EnsureDefaults + }, } } diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 0c571cbec3..6497b7f7b5 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -73,13 +73,13 @@ const ( ) // NewDriveItemPermissionsService creates a new DriveItemPermissionsService -func NewDriveItemPermissionsService(logger log.Logger, gatewaySelector pool.Selectable[gateway.GatewayAPIClient], identityCache identity.IdentityCache, config *config.Config) (DriveItemPermissionsService, error) { +func NewDriveItemPermissionsService(logger log.Logger, gatewaySelector pool.Selectable[gateway.GatewayAPIClient], identityCache identity.IdentityCache, c *config.Config) (DriveItemPermissionsService, error) { return DriveItemPermissionsService{ BaseGraphService: BaseGraphService{ logger: &log.Logger{Logger: logger.With().Str("graph api", "DrivesDriveItemService").Logger()}, gatewaySelector: gatewaySelector, identityCache: identityCache, - config: config, + config: c, }, }, nil } @@ -104,6 +104,11 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId *sto unifiedRolePermissions := []*libregraph.UnifiedRolePermission{{AllowedResourceActions: invite.LibreGraphPermissionsActions}} 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 + } + role, err := unifiedrole.NewUnifiedRoleFromID(roleID) if err != nil { s.logger.Debug().Err(err).Interface("role", invite.GetRoles()[0]).Msg("unable to convert requested role") @@ -544,18 +549,20 @@ func (s DriveItemPermissionsService) UpdateSpaceRootPermission(ctx context.Conte return s.UpdatePermission(ctx, rootResourceID, permissionID, newPermission) } -// DriveItemPermissionsService is the api that registers the http endpoints which expose needed operation to the graph api. +// DriveItemPermissionsApi is the api that registers the http endpoints which expose needed operation to the graph api. // the business logic is delegated to the permissions service and further down to the cs3 client. type DriveItemPermissionsApi struct { logger log.Logger driveItemPermissionsService DriveItemPermissionsProvider + config *config.Config } // NewDriveItemPermissionsApi creates a new DriveItemPermissionsApi -func NewDriveItemPermissionsApi(driveItemPermissionService DriveItemPermissionsProvider, logger log.Logger) (DriveItemPermissionsApi, error) { +func NewDriveItemPermissionsApi(driveItemPermissionService DriveItemPermissionsProvider, logger log.Logger, c *config.Config) (DriveItemPermissionsApi, error) { return DriveItemPermissionsApi{ logger: log.Logger{Logger: logger.With().Str("graph api", "DrivesDriveItemApi").Logger()}, driveItemPermissionsService: driveItemPermissionService, + config: c, }, nil } @@ -575,15 +582,14 @@ func (api DriveItemPermissionsApi) Invite(w http.ResponseWriter, r *http.Request return } - ctx := r.Context() - + ctx := validate.ContextWithAllowedRoleIDs(r.Context(), api.config.UnifiedRoles.AvailableRoles) if err = validate.StructCtx(ctx, driveItemInvite); err != nil { api.logger.Debug().Err(err).Interface("Body", r.Body).Msg("invalid request body") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) return } - permission, err := api.driveItemPermissionsService.Invite(ctx, &itemID, *driveItemInvite) + permission, err := api.driveItemPermissionsService.Invite(ctx, &itemID, *driveItemInvite) if err != nil { errorcode.RenderError(w, r, err) return @@ -609,7 +615,7 @@ func (api DriveItemPermissionsApi) SpaceRootInvite(w http.ResponseWriter, r *htt return } - ctx := r.Context() + ctx := validate.ContextWithAllowedRoleIDs(r.Context(), api.config.UnifiedRoles.AvailableRoles) if err = validate.StructCtx(ctx, driveItemInvite); err != nil { api.logger.Debug().Err(err).Interface("Body", r.Body).Msg("invalid request body") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) 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 c2bae8e99e..c3eca5d559 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go @@ -245,6 +245,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(permission.GetLibreGraphPermissionsActions()).To(HaveLen(1)) Expect(permission.GetLibreGraphPermissionsActions()[0]).To(Equal(unifiedrole.DriveItemContentRead)) }) + It("fails with a missing driveritem", func() { statResponse.Status = status.NewNotFound(context.Background(), "not found") permission, err := driveItemPermissionsService.Invite(context.Background(), driveItemId, driveItemInvite) @@ -252,6 +253,12 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(err).To(MatchError(errorcode.New(errorcode.ItemNotFound, "not found").WithOrigin(errorcode.ErrorOriginCS3))) Expect(permission).To(BeZero()) }) + + It("fails with unknown or disable role", func() { + driveItemInvite.Roles = []string{unifiedrole.UnifiedRoleViewerID, unifiedrole.UnifiedRoleSecureViewerID} + _, err := driveItemPermissionsService.Invite(context.Background(), driveItemId, driveItemInvite) + Expect(err).To(MatchError(unifiedrole.ErrUnknownUnifiedRole)) + }) }) Describe("SpaceRootInvite", func() { var ( @@ -1015,7 +1022,7 @@ var _ = Describe("DriveItemPermissionsApi", func() { logger := log.NewLogger() mockProvider = mocks.NewDriveItemPermissionsProvider(GinkgoT()) - api, err := svc.NewDriveItemPermissionsApi(mockProvider, logger) + api, err := svc.NewDriveItemPermissionsApi(mockProvider, logger, defaults.FullDefaultConfig()) Expect(err).ToNot(HaveOccurred()) httpAPI = api @@ -1097,6 +1104,22 @@ var _ = Describe("DriveItemPermissionsApi", func() { Expect(responseRecorder.Code).To(Equal(http.StatusOK)) }) + + It("fails with unknown or disable role", func() { + rCTX.URLParams.Add("itemID", "1$2!3") + responseRecorder := httptest.NewRecorder() + invite.Roles = []string{unifiedrole.UnifiedRoleViewerID, unifiedrole.UnifiedRoleSecureViewerID} + inviteJson, err := json.Marshal(invite) + Expect(err).ToNot(HaveOccurred()) + + request := httptest.NewRequest(http.MethodPost, "/", bytes.NewBuffer(inviteJson)). + WithContext( + context.WithValue(context.Background(), chi.RouteCtxKey, rCTX), + ) + httpAPI.Invite(responseRecorder, request) + + Expect(responseRecorder.Code).To(Equal(http.StatusBadRequest)) + }) }) Describe("SpaceRootInvite", func() { It("call the Invite provider with the correct arguments", func() { @@ -1132,6 +1155,21 @@ var _ = Describe("DriveItemPermissionsApi", func() { Expect(responseRecorder.Code).To(Equal(http.StatusUnprocessableEntity)) }) + + It("fails with unknown or disable role", func() { + responseRecorder := httptest.NewRecorder() + invite.Roles = []string{unifiedrole.UnifiedRoleViewerID, unifiedrole.UnifiedRoleSecureViewerID} + inviteJson, err := json.Marshal(invite) + Expect(err).ToNot(HaveOccurred()) + + request := httptest.NewRequest(http.MethodPost, "/", bytes.NewBuffer(inviteJson)). + WithContext( + context.WithValue(context.Background(), chi.RouteCtxKey, rCTX), + ) + httpAPI.SpaceRootInvite(responseRecorder, request) + + Expect(responseRecorder.Code).To(Equal(http.StatusBadRequest)) + }) }) Describe("ListPermissions", func() { It("calls the ListPermissions provider with the correct arguments", func() { diff --git a/services/graph/pkg/service/v0/rolemanagement.go b/services/graph/pkg/service/v0/rolemanagement.go index d1511221ae..3fdfd4ef39 100644 --- a/services/graph/pkg/service/v0/rolemanagement.go +++ b/services/graph/pkg/service/v0/rolemanagement.go @@ -1,7 +1,6 @@ package svc import ( - "fmt" "net/http" "net/url" @@ -16,8 +15,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) - // fixMe: should we consider all roles or only the ones that are enabled? - render.JSON(w, r, unifiedrole.GetBuiltinRoleDefinitionList()) + render.JSON(w, r, unifiedrole.GetBuiltinRoleDefinitionList(unifiedrole.RoleFilterIDs(g.config.UnifiedRoles.AvailableRoles...))) } // GetRoleDefinition a permission role than can be used when sharing with users or groups @@ -29,7 +27,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 := getRoleDefinition(roleID) + role, err := getRoleDefinition(roleID, g.config.UnifiedRoles.AvailableRoles) if err != nil { logger.Debug().Str("roleID", roleID).Msg("could not get role: not found") errorcode.ItemNotFound.Render(w, r, http.StatusNotFound, err.Error()) @@ -39,13 +37,11 @@ func (g Graph) GetRoleDefinition(w http.ResponseWriter, r *http.Request) { render.JSON(w, r, role) } -func getRoleDefinition(roleID string) (*libregraph.UnifiedRoleDefinition, error) { - // fixMe: should we consider all roles or only the ones that are enabled? - roleList := unifiedrole.GetBuiltinRoleDefinitionList() - for _, role := range roleList { +func getRoleDefinition(roleID string, availableRoles []string) (*libregraph.UnifiedRoleDefinition, error) { + for _, role := range unifiedrole.GetBuiltinRoleDefinitionList(unifiedrole.RoleFilterIDs(availableRoles...)) { if role != nil && role.Id != nil && *role.Id == roleID { return role, nil } } - return nil, fmt.Errorf("role not found") + return nil, unifiedrole.ErrUnknownUnifiedRole } diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 57db3b0d86..2d566f21e6 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -214,7 +214,7 @@ func NewService(opts ...Option) (Graph, error) { //nolint:maintidx return svc, err } - driveItemPermissionsApi, err := NewDriveItemPermissionsApi(driveItemPermissionsService, options.Logger) + driveItemPermissionsApi, err := NewDriveItemPermissionsApi(driveItemPermissionsService, options.Logger, options.Config) if err != nil { return svc, err } diff --git a/services/graph/pkg/unifiedrole/errors.go b/services/graph/pkg/unifiedrole/errors.go index e29580167d..04b007ba4e 100644 --- a/services/graph/pkg/unifiedrole/errors.go +++ b/services/graph/pkg/unifiedrole/errors.go @@ -6,5 +6,5 @@ import ( var ( // ErrUnknownUnifiedRole is returned when an unknown unified role is requested. - ErrUnknownUnifiedRole = errors.New("unknown unified role") + ErrUnknownUnifiedRole = errors.New("unknown unified role, check if the role is enabled") ) diff --git a/services/graph/pkg/validate/libregraph.go b/services/graph/pkg/validate/libregraph.go index d4084989a0..d5007836b5 100644 --- a/services/graph/pkg/validate/libregraph.go +++ b/services/graph/pkg/validate/libregraph.go @@ -1,6 +1,7 @@ package validate import ( + "context" "slices" "github.com/go-playground/validator/v10" @@ -9,6 +10,10 @@ import ( "github.com/owncloud/ocis/v2/services/graph/pkg/unifiedrole" ) +var ( + _contextRoleIDsValueKey = "roleFilterIDs" +) + // initLibregraph initializes libregraph validation func initLibregraph(v *validator.Validate) { for _, f := range []func(*validator.Validate){ @@ -30,10 +35,9 @@ func libregraphDriveItemInvite(v *validator.Validate) { "ExpirationDateTime": "omitnil,gt", }, s) - v.RegisterStructValidation(func(sl validator.StructLevel) { + v.RegisterStructValidationCtx(func(ctx context.Context, sl validator.StructLevel) { driveItemInvite := sl.Current().Interface().(libregraph.DriveItemInvite) - - rolesAndActions(sl, driveItemInvite.Roles, driveItemInvite.LibreGraphPermissionsActions, false) + rolesAndActions(ctx, sl, driveItemInvite.Roles, driveItemInvite.LibreGraphPermissionsActions, false) }, s) } @@ -52,7 +56,7 @@ func libregraphPermission(v *validator.Validate) { v.RegisterStructValidationMapRules(map[string]string{ "Roles": "max=1", }, s) - v.RegisterStructValidation(func(sl validator.StructLevel) { + v.RegisterStructValidationCtx(func(ctx context.Context, sl validator.StructLevel) { permission := sl.Current().Interface().(libregraph.Permission) if _, ok := permission.GetIdOk(); ok { @@ -63,14 +67,13 @@ func libregraphPermission(v *validator.Validate) { sl.ReportError(permission.HasPassword, "hasPassword", "HasPassword", "readonly", "") } - rolesAndActions(sl, permission.Roles, permission.LibreGraphPermissionsActions, true) + rolesAndActions(ctx, sl, permission.Roles, permission.LibreGraphPermissionsActions, true) }, s) } -func rolesAndActions(sl validator.StructLevel, roles, actions []string, allowEmpty bool) { +func rolesAndActions(ctx context.Context, sl validator.StructLevel, roles, actions []string, allowEmpty bool) { totalRoles := len(roles) totalActions := len(actions) - switch { case allowEmpty && totalRoles == 0 && totalActions == 0: break @@ -83,12 +86,16 @@ func rolesAndActions(sl validator.StructLevel, roles, actions []string, allowEmp var availableRoles []string var availableActions []string - for _, definition := range append( - // fixMe: why twice!? - // fixMe: should we consider all roles or only the ones that are enabled? - unifiedrole.GetBuiltinRoleDefinitionList(), - unifiedrole.GetBuiltinRoleDefinitionList()..., - ) { + var definitions []*libregraph.UnifiedRoleDefinition + + switch roles, ok := ctx.Value(_contextRoleIDsValueKey).([]string); { + case ok: + definitions = unifiedrole.GetBuiltinRoleDefinitionList(unifiedrole.RoleFilterIDs(roles...)) + default: + definitions = unifiedrole.GetBuiltinRoleDefinitionList() + } + + for _, definition := range definitions { if slices.Contains(availableRoles, definition.GetId()) { continue } @@ -122,3 +129,8 @@ func rolesAndActions(sl validator.StructLevel, roles, actions []string, allowEmp sl.ReportError(actions, "LibreGraphPermissionsActions", "LibreGraphPermissionsActions", "available_action", "") } } + +// ContextWithAllowedRoleIDs returns a new context which includes the allowed role IDs. +func ContextWithAllowedRoleIDs(ctx context.Context, rolesIds []string) context.Context { + return context.WithValue(ctx, _contextRoleIDsValueKey, rolesIds) +}