sevenhill/QSfera
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[full-ci]fallback to stored roles (#4890)

Browse Source 
* fallback to stored roles

If there are no roles in the current context load the user's roles from the storage.

* some code clean up
This commit is contained in:
David Christofas
2022-11-03 13:25:21 +01:00
committed by GitHub
parent 5602668634
commit 3125459415
3 changed files with 58 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files
changelog/unreleased/settings-permissions-check.md
+5
View File
@@ -0,0 +1,5 @@
Bugfix: Fix permission check in settings service
Added a check of the stored roles as a fallback if no roles are contained in the context.
https://github.com/owncloud/ocis/pull/4890
services/settings/pkg/service/v0/service.go
+34 -20
View File
@@ -51,6 +51,8 @@ func NewService(cfg *config.Config, logger log.Logger) Service {
return service
}
// CheckPermission implements the CS3 API Permssions service.
// It's used to check if a subject (user or group) has a permission.
func (g Service) CheckPermission(ctx context.Context, req *permissions.CheckPermissionRequest) (*permissions.CheckPermissionResponse, error) {
spec := req.SubjectRef.Spec
@@ -390,13 +392,13 @@ 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 err := g.checkStaticPermissionsByBundleType(ctx, settingsmsg.Bundle_TYPE_ROLE); err != nil {
return err
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, "%s", validationError)
return merrors.BadRequest(g.id, validationError.Error())
}
ownAccountUUID, ok := metadata.Get(ctx, middleware.AccountID)
@@ -406,12 +408,12 @@ func (g Service) AssignRoleToUser(ctx context.Context, req *settingssvc.AssignRo
}
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")
return merrors.Forbidden(g.id, "Changing own role assignment forbidden")
}
r, err := g.manager.WriteRoleAssignment(req.AccountUuid, req.RoleId)
if err != nil {
return merrors.BadRequest(g.id, "%s", err)
return merrors.BadRequest(g.id, err.Error())
}
res.Assignment = r
return nil
@@ -419,8 +421,12 @@ func (g Service) AssignRoleToUser(ctx context.Context, req *settingssvc.AssignRo
// RemoveRoleFromUser implements the RoleServiceHandler interface
func (g Service) RemoveRoleFromUser(ctx context.Context, req *settingssvc.RemoveRoleFromUserRequest, _ *emptypb.Empty) error {
if err := g.checkStaticPermissionsByBundleType(ctx, settingsmsg.Bundle_TYPE_ROLE); err != nil {
return err
if !g.canManageRoles(ctx) {
return merrors.Forbidden(g.id, "user has no role management permission")
}
if validationError := validateRemoveRoleFromUser(req); validationError != nil {
return merrors.BadRequest(g.id, validationError.Error())
}
ownAccountUUID, ok := metadata.Get(ctx, middleware.AccountID)
@@ -432,22 +438,18 @@ func (g Service) RemoveRoleFromUser(ctx context.Context, req *settingssvc.Remove
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)
return merrors.InternalServerError(g.id, err.Error())
}
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")
return merrors.Forbidden(g.id, "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)
return merrors.BadRequest(g.id, err.Error())
}
return nil
}
@@ -547,16 +549,24 @@ func (g Service) getValueWithIdentifier(value *settingsmsg.Value) (*settingsmsg.
func (g Service) hasStaticPermission(ctx context.Context, permissionID string) bool {
roleIDs, ok := roles.ReadRoleIDsFromContext(ctx)
if !ok {
/**
* FIXME: with this we are skipping permission checks on all requests that are coming in without roleIDs in the
* metadata context. This is a huge security impairment, as that's the case not only for grpc requests but also
* for unauthenticated http requests and http requests coming in without hitting the ocis-proxy first.
*/
// TODO add system role for internal requests.
// - at least the proxy needs to look up account info
// - glauth needs to make bind requests
// tracked as OCIS-454
return true
accountID, ok := metadata.Get(ctx, middleware.AccountID)
if !ok {
return false
}
assignments, err := g.manager.ListRoleAssignments(accountID)
if err != nil {
return false
}
roleIDs = make([]string, 0, len(assignments))
for _, a := range assignments {
roleIDs = append(roleIDs, a.GetRoleId())
}
}
p, err := g.manager.ReadPermissionByID(permissionID, roleIDs)
return err == nil && p != nil
@@ -590,3 +600,7 @@ func (g Service) isCurrentUser(ctx context.Context, accountID string) bool {
}
return accountID == ownAccountID
}
func (g Service) canManageRoles(ctx context.Context) bool {
return g.hasStaticPermission(ctx, RoleManagementPermissionID)
}
services/settings/pkg/service/v0/service_test.go
+19
View File
@@ -66,6 +66,19 @@ func TestGetValidatedAccountUUID(t *testing.T) {
func TestEditOwnRoleAssignment(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",
},
}
editRolePermission := &settingsmsg.Permission{
Operation: settingsmsg.Permission_OPERATION_READWRITE,
Constraint: settingsmsg.Permission_CONSTRAINT_ALL,
}
manager.On("ListRoleAssignments", mock.Anything).Return(a, nil)
manager.On("ReadPermissionByID", mock.Anything, mock.Anything).Return(editRolePermission, nil)
svc := Service{
manager: manager,
}
@@ -99,6 +112,11 @@ func TestRemoveOwnRoleAssignment(t *testing.T) {
RoleId: "aceb15b8-7486-479f-ae32-c91118e07a39",
},
}
editRolePermission := &settingsmsg.Permission{
Operation: settingsmsg.Permission_OPERATION_READWRITE,
Constraint: settingsmsg.Permission_CONSTRAINT_ALL,
}
manager.On("ReadPermissionByID", mock.Anything, mock.Anything).Return(editRolePermission, nil)
manager.On("ListRoleAssignments", mock.Anything).Return(a, nil)
svc := Service{
manager: manager,
@@ -114,6 +132,7 @@ func TestRemoveOwnRoleAssignment(t *testing.T) {
manager = &mocks.Manager{}
manager.On("ListRoleAssignments", mock.Anything).Return(nil, nil)
manager.On("RemoveRoleAssignment", mock.Anything).Return(nil)
manager.On("ReadPermissionByID", mock.Anything, mock.Anything).Return(editRolePermission, nil)
svc = Service{
manager: manager,
}