Merge pull request #4615 from rhafer/issue/3713

Restrict admins from self-removal

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

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
 		}

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

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

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

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