Skip to content

Commit

Permalink
Allow initial self-assignemnt of UserRole
Browse files Browse the repository at this point in the history
When using an external user management we need to allow users to self-assign
the default role. This adds an explicit check for that to the settings service.
This also means we no longer need to fiddle with the account id in the proxy
upon first login.

Fixes: #5045
  • Loading branch information
rhafer committed Nov 16, 2022
1 parent 2ee2f71 commit 07a7828
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 20 deletions.
5 changes: 5 additions & 0 deletions changelog/unreleased/initial-role-assignment.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Bugfix: Initial role assingment with external IDM

We've the initial user role assignment when using an external LDAP server.

https://github.com/owncloud/ocis/issues/5045
4 changes: 1 addition & 3 deletions services/proxy/pkg/user/backend/cs3.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@ func (c *cs3backend) GetUserByClaims(ctx context.Context, claim, value string, w
// https://github.com/owncloud/ocis/v2/issues/1825 for more context.
if user.Id.Type == cs3.UserType_USER_TYPE_PRIMARY {
c.logger.Info().Str("userid", user.Id.OpaqueId).Msg("user has no role assigned, assigning default user role")
// Updating context to have the Account-ID field and suffixing with _init
// so that the safety check for setting users' own role doesn't fail
ctx = metadata.Set(ctx, middleware.AccountID, user.Id.OpaqueId+"_init")
ctx = metadata.Set(ctx, middleware.AccountID, user.Id.OpaqueId)
_, err := c.settingsRoleService.AssignRoleToUser(ctx, &settingssvc.AssignRoleToUserRequest{
AccountUuid: user.Id.OpaqueId,
RoleId: settingsService.BundleUUIDRoleUser,
Expand Down
23 changes: 16 additions & 7 deletions services/settings/pkg/service/v0/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0"
"github.com/owncloud/ocis/v2/services/settings/pkg/config"
"github.com/owncloud/ocis/v2/services/settings/pkg/settings"
"github.com/owncloud/ocis/v2/services/settings/pkg/store/defaults"
filestore "github.com/owncloud/ocis/v2/services/settings/pkg/store/filesystem"
metastore "github.com/owncloud/ocis/v2/services/settings/pkg/store/metadata"
merrors "go-micro.dev/v4/errors"
Expand Down Expand Up @@ -392,10 +393,6 @@ 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 !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, validationError.Error())
Expand All @@ -406,9 +403,21 @@ func (g Service) AssignRoleToUser(ctx context.Context, req *settingssvc.AssignRo
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, "Changing own role assignment forbidden")

switch {
case ownAccountUUID == req.AccountUuid:
// Allow users to assign themself to the user role
// deny any other attempt to change the user's own assignment
if r, err := g.manager.ListRoleAssignments(req.AccountUuid); err == nil && len(r) > 0 {
return merrors.Forbidden(g.id, "Changing own role assignment forbidden")
}
if req.RoleId != defaults.BundleUUIDRoleUser {
return merrors.Forbidden(g.id, "Changing own role assignment forbidden")
}
g.logger.Debug().Str("userid", ownAccountUUID).Msg("Self-assignment for default 'user' role permitted")
case g.canManageRoles(ctx):
default:
return merrors.Forbidden(g.id, "user has no role management permission")
}

r, err := g.manager.WriteRoleAssignment(req.AccountUuid, req.RoleId)
Expand Down
46 changes: 36 additions & 10 deletions services/settings/pkg/service/v0/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
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/owncloud/ocis/v2/services/settings/pkg/store/defaults"
"github.com/stretchr/testify/assert"
"github.com/test-go/testify/mock"
"go-micro.dev/v4/metadata"
Expand Down Expand Up @@ -66,7 +67,35 @@ func TestGetValidatedAccountUUID(t *testing.T) {

func TestEditOwnRoleAssignment(t *testing.T) {
manager := &mocks.Manager{}
a := []*settingsmsg.UserRoleAssignment{
svc := Service{
manager: manager,
}
a := []*settingsmsg.UserRoleAssignment{}
manager.On("ListRoleAssignments", mock.Anything).Return(a, nil)
manager.On("WriteRoleAssignment", mock.Anything, mock.Anything).Return(nil, nil)
// Creating an initial self assignment is expected to succeed for UserRole when no assignment exists yet
req := v0.AssignRoleToUserRequest{
AccountUuid: "61445573-4dbe-4d56-88dc-88ab47aceba7",
RoleId: defaults.BundleUUIDRoleUser,
}
res := v0.AssignRoleToUserResponse{}
err := svc.AssignRoleToUser(ctxWithUUID, &req, &res)
assert.Nil(t, err)

// Creating an initial self assignment is expected to fail for non UserRole when no assignment exists yet
req = v0.AssignRoleToUserRequest{
AccountUuid: "61445573-4dbe-4d56-88dc-88ab47aceba7",
RoleId: defaults.BundleUUIDRoleAdmin,
}
res = v0.AssignRoleToUserResponse{}
err = svc.AssignRoleToUser(ctxWithUUID, &req, &res)
assert.NotNil(t, err)

manager = &mocks.Manager{}
svc = Service{
manager: manager,
}
a = []*settingsmsg.UserRoleAssignment{
{
Id: "00000000-0000-0000-0000-000000000001",
AccountUuid: "61445573-4dbe-4d56-88dc-88ab47aceba7",
Expand All @@ -79,21 +108,18 @@ func TestEditOwnRoleAssignment(t *testing.T) {
}
manager.On("ListRoleAssignments", mock.Anything).Return(a, nil)
manager.On("ReadPermissionByID", mock.Anything, mock.Anything).Return(editRolePermission, nil)
svc := Service{
manager: manager,
}

// Creating an self assignment is expect to fail
req := v0.AssignRoleToUserRequest{
// Creating an self assignment is expect to fail if there is already an assingment
req = v0.AssignRoleToUserRequest{
AccountUuid: "61445573-4dbe-4d56-88dc-88ab47aceba7",
RoleId: "aceb15b8-7486-479f-ae32-c91118e07a39",
RoleId: defaults.BundleUUIDRoleUser,
}
res := v0.AssignRoleToUserResponse{}
err := svc.AssignRoleToUser(ctxWithUUID, &req, &res)
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
// Creating an assignment for somebody else is expected to succeed, give the right permissions
req = v0.AssignRoleToUserRequest{
AccountUuid: "00000000-0000-0000-0000-000000000000",
RoleId: "aceb15b8-7486-479f-ae32-c91118e07a39",
Expand Down

0 comments on commit 07a7828

Please sign in to comment.