Skip to content

Commit

Permalink
Forbid to edit/remove own role assignements
Browse files Browse the repository at this point in the history
And admin user is no longer allowed to edit/remove its own assignments.
This to prevent admins from locking themselves out.

Fixes: owncloud#3713
  • Loading branch information
rhafer committed Sep 23, 2022
1 parent ca66a9f commit 87eaf72
Show file tree
Hide file tree
Showing 4 changed files with 462 additions and 0 deletions.
31 changes: 31 additions & 0 deletions services/settings/pkg/service/v0/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
Expand Down
65 changes: 65 additions & 0 deletions services/settings/pkg/service/v0/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
Loading

0 comments on commit 87eaf72

Please sign in to comment.