diff --git a/changelog/unreleased/settings-permissions-check.md b/changelog/unreleased/settings-permissions-check.md new file mode 100644 index 000000000..10b302d04 --- /dev/null +++ b/changelog/unreleased/settings-permissions-check.md @@ -0,0 +1,5 @@ +Bugfix: Fix permission check in settings service + +Added a check of the stored roles as a fallback if no roles are contained in the context. + +https://github.com/owncloud/ocis/pull/4890 diff --git a/services/settings/pkg/service/v0/service.go b/services/settings/pkg/service/v0/service.go index e75fdac24..93673f527 100644 --- a/services/settings/pkg/service/v0/service.go +++ b/services/settings/pkg/service/v0/service.go @@ -51,6 +51,8 @@ func NewService(cfg *config.Config, logger log.Logger) Service { return service } +// CheckPermission implements the CS3 API Permssions service. +// It's used to check if a subject (user or group) has a permission. func (g Service) CheckPermission(ctx context.Context, req *permissions.CheckPermissionRequest) (*permissions.CheckPermissionResponse, error) { spec := req.SubjectRef.Spec @@ -390,13 +392,13 @@ func (g Service) ListRoleAssignments(ctx context.Context, req *settingssvc.ListR // AssignRoleToUser implements the RoleServiceHandler interface func (g Service) AssignRoleToUser(ctx context.Context, req *settingssvc.AssignRoleToUserRequest, res *settingssvc.AssignRoleToUserResponse) error { - if err := g.checkStaticPermissionsByBundleType(ctx, settingsmsg.Bundle_TYPE_ROLE); err != nil { - return err + if !g.canManageRoles(ctx) { + return merrors.Forbidden(g.id, "user has no role management permission") } req.AccountUuid = getValidatedAccountUUID(ctx, req.AccountUuid) if validationError := validateAssignRoleToUser(req); validationError != nil { - return merrors.BadRequest(g.id, "%s", validationError) + return merrors.BadRequest(g.id, validationError.Error()) } ownAccountUUID, ok := metadata.Get(ctx, middleware.AccountID) @@ -406,12 +408,12 @@ func (g Service) AssignRoleToUser(ctx context.Context, req *settingssvc.AssignRo } if ownAccountUUID == req.AccountUuid { g.logger.Debug().Str("id", g.id).Msg("Changing own role assignment forbidden") - return merrors.Forbidden(g.id, "%s", "Changing own role assignment forbidden") + return merrors.Forbidden(g.id, "Changing own role assignment forbidden") } r, err := g.manager.WriteRoleAssignment(req.AccountUuid, req.RoleId) if err != nil { - return merrors.BadRequest(g.id, "%s", err) + return merrors.BadRequest(g.id, err.Error()) } res.Assignment = r return nil @@ -419,8 +421,12 @@ func (g Service) AssignRoleToUser(ctx context.Context, req *settingssvc.AssignRo // RemoveRoleFromUser implements the RoleServiceHandler interface func (g Service) RemoveRoleFromUser(ctx context.Context, req *settingssvc.RemoveRoleFromUserRequest, _ *emptypb.Empty) error { - if err := g.checkStaticPermissionsByBundleType(ctx, settingsmsg.Bundle_TYPE_ROLE); err != nil { - return err + if !g.canManageRoles(ctx) { + return merrors.Forbidden(g.id, "user has no role management permission") + } + + if validationError := validateRemoveRoleFromUser(req); validationError != nil { + return merrors.BadRequest(g.id, validationError.Error()) } ownAccountUUID, ok := metadata.Get(ctx, middleware.AccountID) @@ -432,22 +438,18 @@ func (g Service) RemoveRoleFromUser(ctx context.Context, req *settingssvc.Remove al, err := g.manager.ListRoleAssignments(ownAccountUUID) if err != nil { g.logger.Debug().Err(err).Str("id", g.id).Msg("ListRoleAssignments failed") - return merrors.InternalServerError(g.id, "%s", err) + return merrors.InternalServerError(g.id, err.Error()) } for _, a := range al { if a.Id == req.Id { g.logger.Debug().Str("id", g.id).Msg("Removing own role assignment forbidden") - return merrors.Forbidden(g.id, "%s", "Removing own role assignment forbidden") + return merrors.Forbidden(g.id, "Removing own role assignment forbidden") } } - if validationError := validateRemoveRoleFromUser(req); validationError != nil { - return merrors.BadRequest(g.id, "%s", validationError) - } - if err := g.manager.RemoveRoleAssignment(req.Id); err != nil { - return merrors.BadRequest(g.id, "%s", err) + return merrors.BadRequest(g.id, err.Error()) } return nil } @@ -547,16 +549,24 @@ func (g Service) getValueWithIdentifier(value *settingsmsg.Value) (*settingsmsg. func (g Service) hasStaticPermission(ctx context.Context, permissionID string) bool { roleIDs, ok := roles.ReadRoleIDsFromContext(ctx) if !ok { - /** - * FIXME: with this we are skipping permission checks on all requests that are coming in without roleIDs in the - * metadata context. This is a huge security impairment, as that's the case not only for grpc requests but also - * for unauthenticated http requests and http requests coming in without hitting the ocis-proxy first. - */ // TODO add system role for internal requests. // - at least the proxy needs to look up account info // - glauth needs to make bind requests // tracked as OCIS-454 - return true + + accountID, ok := metadata.Get(ctx, middleware.AccountID) + if !ok { + return false + } + assignments, err := g.manager.ListRoleAssignments(accountID) + if err != nil { + return false + } + + roleIDs = make([]string, 0, len(assignments)) + for _, a := range assignments { + roleIDs = append(roleIDs, a.GetRoleId()) + } } p, err := g.manager.ReadPermissionByID(permissionID, roleIDs) return err == nil && p != nil @@ -590,3 +600,7 @@ func (g Service) isCurrentUser(ctx context.Context, accountID string) bool { } return accountID == ownAccountID } + +func (g Service) canManageRoles(ctx context.Context) bool { + return g.hasStaticPermission(ctx, RoleManagementPermissionID) +} diff --git a/services/settings/pkg/service/v0/service_test.go b/services/settings/pkg/service/v0/service_test.go index eb93ad33c..d75366480 100644 --- a/services/settings/pkg/service/v0/service_test.go +++ b/services/settings/pkg/service/v0/service_test.go @@ -66,6 +66,19 @@ func TestGetValidatedAccountUUID(t *testing.T) { func TestEditOwnRoleAssignment(t *testing.T) { manager := &mocks.Manager{} + a := []*settingsmsg.UserRoleAssignment{ + { + Id: "00000000-0000-0000-0000-000000000001", + AccountUuid: "61445573-4dbe-4d56-88dc-88ab47aceba7", + RoleId: "aceb15b8-7486-479f-ae32-c91118e07a39", + }, + } + editRolePermission := &settingsmsg.Permission{ + Operation: settingsmsg.Permission_OPERATION_READWRITE, + Constraint: settingsmsg.Permission_CONSTRAINT_ALL, + } + manager.On("ListRoleAssignments", mock.Anything).Return(a, nil) + manager.On("ReadPermissionByID", mock.Anything, mock.Anything).Return(editRolePermission, nil) svc := Service{ manager: manager, } @@ -99,6 +112,11 @@ func TestRemoveOwnRoleAssignment(t *testing.T) { RoleId: "aceb15b8-7486-479f-ae32-c91118e07a39", }, } + editRolePermission := &settingsmsg.Permission{ + Operation: settingsmsg.Permission_OPERATION_READWRITE, + Constraint: settingsmsg.Permission_CONSTRAINT_ALL, + } + manager.On("ReadPermissionByID", mock.Anything, mock.Anything).Return(editRolePermission, nil) manager.On("ListRoleAssignments", mock.Anything).Return(a, nil) svc := Service{ manager: manager, @@ -114,6 +132,7 @@ func TestRemoveOwnRoleAssignment(t *testing.T) { manager = &mocks.Manager{} manager.On("ListRoleAssignments", mock.Anything).Return(nil, nil) manager.On("RemoveRoleAssignment", mock.Anything).Return(nil) + manager.On("ReadPermissionByID", mock.Anything, mock.Anything).Return(editRolePermission, nil) svc = Service{ manager: manager, }