diff --git a/changelog/unreleased/forbid-admin-self-removal.md b/changelog/unreleased/forbid-admin-self-removal.md new file mode 100644 index 0000000000..41d488b477 --- /dev/null +++ b/changelog/unreleased/forbid-admin-self-removal.md @@ -0,0 +1,8 @@ +Enhancement: Restrict admins from self-removal + +Admin users are no longer allowed to remove their own account or +to edit their own role assigments. By this restriction we try to +prevent situation where no administrative users is available +in the system anymore + +https://github.com/owncloud/ocis/issues/3713 diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 45ca10dfa8..0efcf0f92b 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -255,6 +255,7 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { userID := chi.URLParam(r, "userID") userID, err := url.PathUnescape(userID) if err != nil { + g.logger.Debug().Err(err).Msg("unescaping user id failed") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "unescaping user id failed") return } @@ -265,6 +266,7 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { } user, err := g.identityBackend.GetUser(r.Context(), userID, r.URL.Query()) if err != nil { + g.logger.Debug().Err(err).Str("userID", userID).Msg("failed to get user") var errcode errorcode.Error if errors.As(err, &errcode) { errcode.Render(w, r) @@ -276,6 +278,12 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { currentUser := ctxpkg.ContextMustGetUser(r.Context()) + if currentUser.GetId().GetOpaqueId() == user.GetId() { + g.logger.Debug().Msg("self deletion forbidden") + errorcode.NotAllowed.Render(w, r, http.StatusForbidden, "self deletion forbidden") + return + } + opaque := utils.AppendPlainToOpaque(nil, "unrestricted", "T") f := listStorageSpacesUserFilter(user.GetId()) lspr, err := g.gatewayClient.ListStorageSpaces(r.Context(), &storageprovider.ListStorageSpacesRequest{ @@ -283,6 +291,7 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { Filters: []*storageprovider.ListStorageSpacesRequest_Filter{f}, }) if err != nil { + g.logger.Debug().Err(err).Msg("could not read spaces") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "could not read spaces") return } @@ -303,6 +312,7 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { }, }) if err != nil { + g.logger.Debug().Err(err).Msg("could not disable homespace") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "could not disable homespace") return } @@ -315,6 +325,7 @@ func (g Graph) DeleteUser(w http.ResponseWriter, r *http.Request) { }, }) if err != nil { + g.logger.Debug().Err(err).Msg("could not delete homespace") errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "could not delete homespace") return } diff --git a/services/settings/pkg/service/v0/service.go b/services/settings/pkg/service/v0/service.go index 2c08461aed..17daf1a49f 100644 --- a/services/settings/pkg/service/v0/service.go +++ b/services/settings/pkg/service/v0/service.go @@ -385,6 +385,17 @@ func (g Service) AssignRoleToUser(ctx context.Context, req *settingssvc.AssignRo if validationError := validateAssignRoleToUser(req); validationError != nil { return merrors.BadRequest(g.id, "%s", validationError) } + + ownAccountUUID, ok := metadata.Get(ctx, middleware.AccountID) + if !ok { + g.logger.Debug().Str("id", g.id).Msg("user not in context") + return merrors.InternalServerError(g.id, "user not in context") + } + 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") + } + r, err := g.manager.WriteRoleAssignment(req.AccountUuid, req.RoleId) if err != nil { return merrors.BadRequest(g.id, "%s", err) @@ -399,9 +410,29 @@ func (g Service) RemoveRoleFromUser(ctx context.Context, req *settingssvc.Remove return err } + ownAccountUUID, ok := metadata.Get(ctx, middleware.AccountID) + if !ok { + g.logger.Debug().Str("id", g.id).Msg("user not in context") + return merrors.InternalServerError(g.id, "user not in context") + } + + 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) + } + + 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") + } + } + 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) } diff --git a/services/settings/pkg/service/v0/service_test.go b/services/settings/pkg/service/v0/service_test.go index 2e766b5bf0..eb93ad33cf 100644 --- a/services/settings/pkg/service/v0/service_test.go +++ b/services/settings/pkg/service/v0/service_test.go @@ -5,7 +5,11 @@ import ( "testing" "github.com/owncloud/ocis/v2/ocis-pkg/middleware" + settingsmsg "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0" + v0 "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" + "github.com/owncloud/ocis/v2/services/settings/pkg/settings/mocks" "github.com/stretchr/testify/assert" + "github.com/test-go/testify/mock" "go-micro.dev/v4/metadata" ) @@ -59,3 +63,64 @@ func TestGetValidatedAccountUUID(t *testing.T) { }) } } + +func TestEditOwnRoleAssignment(t *testing.T) { + manager := &mocks.Manager{} + svc := Service{ + manager: manager, + } + + // Creating an self assignment is expect to fail + req := v0.AssignRoleToUserRequest{ + AccountUuid: "61445573-4dbe-4d56-88dc-88ab47aceba7", + RoleId: "aceb15b8-7486-479f-ae32-c91118e07a39", + } + res := v0.AssignRoleToUserResponse{} + err := svc.AssignRoleToUser(ctxWithUUID, &req, &res) + assert.NotNil(t, err) + + manager.On("WriteRoleAssignment", mock.Anything, mock.Anything).Return(nil, nil) + // Creating an self assignment is expect to fail + req = v0.AssignRoleToUserRequest{ + AccountUuid: "00000000-0000-0000-0000-000000000000", + RoleId: "aceb15b8-7486-479f-ae32-c91118e07a39", + } + res = v0.AssignRoleToUserResponse{} + err = svc.AssignRoleToUser(ctxWithUUID, &req, &res) + assert.Nil(t, err) +} + +func TestRemoveOwnRoleAssignment(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", + }, + } + manager.On("ListRoleAssignments", mock.Anything).Return(a, nil) + svc := Service{ + manager: manager, + } + + // Removing a role for oneself is expected to fail + req := v0.RemoveRoleFromUserRequest{ + Id: "00000000-0000-0000-0000-000000000001", + } + err := svc.RemoveRoleFromUser(ctxWithUUID, &req, nil) + assert.NotNil(t, err) + + manager = &mocks.Manager{} + manager.On("ListRoleAssignments", mock.Anything).Return(nil, nil) + manager.On("RemoveRoleAssignment", mock.Anything).Return(nil) + svc = Service{ + manager: manager, + } + // Removing a role for someone else is expected to fail + req = v0.RemoveRoleFromUserRequest{ + Id: "00000000-0000-0000-0000-000000000002", + } + err = svc.RemoveRoleFromUser(ctxWithUUID, &req, nil) + assert.Nil(t, err) +} diff --git a/services/settings/pkg/settings/mocks/Manager.go b/services/settings/pkg/settings/mocks/Manager.go new file mode 100644 index 0000000000..ca216d9780 --- /dev/null +++ b/services/settings/pkg/settings/mocks/Manager.go @@ -0,0 +1,364 @@ +// Code generated by mockery v2.10.4. DO NOT EDIT. + +package mocks + +import ( + mock "github.com/stretchr/testify/mock" + + v0 "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0" +) + +// Manager is an autogenerated mock type for the Manager type +type Manager struct { + mock.Mock +} + +// AddSettingToBundle provides a mock function with given fields: bundleID, setting +func (_m *Manager) AddSettingToBundle(bundleID string, setting *v0.Setting) (*v0.Setting, error) { + ret := _m.Called(bundleID, setting) + + var r0 *v0.Setting + if rf, ok := ret.Get(0).(func(string, *v0.Setting) *v0.Setting); ok { + r0 = rf(bundleID, setting) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Setting) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, *v0.Setting) error); ok { + r1 = rf(bundleID, setting) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ListBundles provides a mock function with given fields: bundleType, bundleIDs +func (_m *Manager) ListBundles(bundleType v0.Bundle_Type, bundleIDs []string) ([]*v0.Bundle, error) { + ret := _m.Called(bundleType, bundleIDs) + + var r0 []*v0.Bundle + if rf, ok := ret.Get(0).(func(v0.Bundle_Type, []string) []*v0.Bundle); ok { + r0 = rf(bundleType, bundleIDs) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v0.Bundle) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(v0.Bundle_Type, []string) error); ok { + r1 = rf(bundleType, bundleIDs) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ListPermissionsByResource provides a mock function with given fields: resource, roleIDs +func (_m *Manager) ListPermissionsByResource(resource *v0.Resource, roleIDs []string) ([]*v0.Permission, error) { + ret := _m.Called(resource, roleIDs) + + var r0 []*v0.Permission + if rf, ok := ret.Get(0).(func(*v0.Resource, []string) []*v0.Permission); ok { + r0 = rf(resource, roleIDs) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v0.Permission) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*v0.Resource, []string) error); ok { + r1 = rf(resource, roleIDs) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ListRoleAssignments provides a mock function with given fields: accountUUID +func (_m *Manager) ListRoleAssignments(accountUUID string) ([]*v0.UserRoleAssignment, error) { + ret := _m.Called(accountUUID) + + var r0 []*v0.UserRoleAssignment + if rf, ok := ret.Get(0).(func(string) []*v0.UserRoleAssignment); ok { + r0 = rf(accountUUID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v0.UserRoleAssignment) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(accountUUID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ListValues provides a mock function with given fields: bundleID, accountUUID +func (_m *Manager) ListValues(bundleID string, accountUUID string) ([]*v0.Value, error) { + ret := _m.Called(bundleID, accountUUID) + + var r0 []*v0.Value + if rf, ok := ret.Get(0).(func(string, string) []*v0.Value); ok { + r0 = rf(bundleID, accountUUID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*v0.Value) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(bundleID, accountUUID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ReadBundle provides a mock function with given fields: bundleID +func (_m *Manager) ReadBundle(bundleID string) (*v0.Bundle, error) { + ret := _m.Called(bundleID) + + var r0 *v0.Bundle + if rf, ok := ret.Get(0).(func(string) *v0.Bundle); ok { + r0 = rf(bundleID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Bundle) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(bundleID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ReadPermissionByID provides a mock function with given fields: permissionID, roleIDs +func (_m *Manager) ReadPermissionByID(permissionID string, roleIDs []string) (*v0.Permission, error) { + ret := _m.Called(permissionID, roleIDs) + + var r0 *v0.Permission + if rf, ok := ret.Get(0).(func(string, []string) *v0.Permission); ok { + r0 = rf(permissionID, roleIDs) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Permission) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, []string) error); ok { + r1 = rf(permissionID, roleIDs) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ReadPermissionByName provides a mock function with given fields: name, roleIDs +func (_m *Manager) ReadPermissionByName(name string, roleIDs []string) (*v0.Permission, error) { + ret := _m.Called(name, roleIDs) + + var r0 *v0.Permission + if rf, ok := ret.Get(0).(func(string, []string) *v0.Permission); ok { + r0 = rf(name, roleIDs) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Permission) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, []string) error); ok { + r1 = rf(name, roleIDs) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ReadSetting provides a mock function with given fields: settingID +func (_m *Manager) ReadSetting(settingID string) (*v0.Setting, error) { + ret := _m.Called(settingID) + + var r0 *v0.Setting + if rf, ok := ret.Get(0).(func(string) *v0.Setting); ok { + r0 = rf(settingID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Setting) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(settingID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ReadValue provides a mock function with given fields: valueID +func (_m *Manager) ReadValue(valueID string) (*v0.Value, error) { + ret := _m.Called(valueID) + + var r0 *v0.Value + if rf, ok := ret.Get(0).(func(string) *v0.Value); ok { + r0 = rf(valueID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Value) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string) error); ok { + r1 = rf(valueID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// ReadValueByUniqueIdentifiers provides a mock function with given fields: accountUUID, settingID +func (_m *Manager) ReadValueByUniqueIdentifiers(accountUUID string, settingID string) (*v0.Value, error) { + ret := _m.Called(accountUUID, settingID) + + var r0 *v0.Value + if rf, ok := ret.Get(0).(func(string, string) *v0.Value); ok { + r0 = rf(accountUUID, settingID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Value) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(accountUUID, settingID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// RemoveRoleAssignment provides a mock function with given fields: assignmentID +func (_m *Manager) RemoveRoleAssignment(assignmentID string) error { + ret := _m.Called(assignmentID) + + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { + r0 = rf(assignmentID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// RemoveSettingFromBundle provides a mock function with given fields: bundleID, settingID +func (_m *Manager) RemoveSettingFromBundle(bundleID string, settingID string) error { + ret := _m.Called(bundleID, settingID) + + var r0 error + if rf, ok := ret.Get(0).(func(string, string) error); ok { + r0 = rf(bundleID, settingID) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// WriteBundle provides a mock function with given fields: bundle +func (_m *Manager) WriteBundle(bundle *v0.Bundle) (*v0.Bundle, error) { + ret := _m.Called(bundle) + + var r0 *v0.Bundle + if rf, ok := ret.Get(0).(func(*v0.Bundle) *v0.Bundle); ok { + r0 = rf(bundle) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Bundle) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*v0.Bundle) error); ok { + r1 = rf(bundle) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// WriteRoleAssignment provides a mock function with given fields: accountUUID, roleID +func (_m *Manager) WriteRoleAssignment(accountUUID string, roleID string) (*v0.UserRoleAssignment, error) { + ret := _m.Called(accountUUID, roleID) + + var r0 *v0.UserRoleAssignment + if rf, ok := ret.Get(0).(func(string, string) *v0.UserRoleAssignment); ok { + r0 = rf(accountUUID, roleID) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.UserRoleAssignment) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(string, string) error); ok { + r1 = rf(accountUUID, roleID) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// WriteValue provides a mock function with given fields: value +func (_m *Manager) WriteValue(value *v0.Value) (*v0.Value, error) { + ret := _m.Called(value) + + var r0 *v0.Value + if rf, ok := ret.Get(0).(func(*v0.Value) *v0.Value); ok { + r0 = rf(value) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*v0.Value) + } + } + + var r1 error + if rf, ok := ret.Get(1).(func(*v0.Value) error); ok { + r1 = rf(value) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} diff --git a/services/settings/pkg/settings/settings.go b/services/settings/pkg/settings/settings.go index 19658ed0e4..737585834f 100644 --- a/services/settings/pkg/settings/settings.go +++ b/services/settings/pkg/settings/settings.go @@ -18,6 +18,8 @@ var ( // RegisterFunc stores store constructors type RegisterFunc func(*config.Config) Manager +//go:generate mockery --name=Manager + // Manager combines service interfaces for abstraction of storage implementations type Manager interface { BundleManager