Repository: owncloud/ocis
Pull request #4615 (2022-09-20 11:14:04)

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

Fixes: #3713 


Patch:
diff --git a/changelog/unreleased/forbid-admin-self-removal.md b/changelog/unreleased/forbid-admin-self-removal.md
new file mode 100644
index 00000000000..41d488b477e
--- /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 45ca10dfa8c..0efcf0f92b7 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 2c08461aed5..17daf1a49f3 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/settings/mocks/Manager.go b/services/settings/pkg/settings/mocks/Manager.go
new file mode 100644
index 00000000000..ca216d97804
--- /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 19658ed0e43..737585834fc 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


Test patch:
diff --git a/services/settings/pkg/service/v0/service_test.go b/services/settings/pkg/service/v0/service_test.go
index 2e766b5bf03..eb93ad33cf7 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)
+}