From 7db02045b401a8b4a6dd7a823b47c83283ec639a Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Mon, 14 Dec 2020 17:31:28 +0100 Subject: [PATCH 1/3] Introduce permission checks for WRITE access via http --- ocis-pkg/middleware/account.go | 3 +- settings/go.sum | 2 + .../pkg/proto/v0/settings.pb.micro_test.go | 4 +- settings/pkg/service/v0/service.go | 45 ++++++++++++++++--- settings/pkg/service/v0/settings.go | 24 ++++++++++ 5 files changed, 70 insertions(+), 8 deletions(-) diff --git a/ocis-pkg/middleware/account.go b/ocis-pkg/middleware/account.go index 0a1e907753..50136668d7 100644 --- a/ocis-pkg/middleware/account.go +++ b/ocis-pkg/middleware/account.go @@ -46,7 +46,8 @@ func ExtractAccountUUID(opts ...account.Option) func(http.Handler) http.Handler return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { token := r.Header.Get("x-access-token") if len(token) == 0 { - next.ServeHTTP(w, r) + ctx := metadata.Set(r.Context(), RoleIDs, "") + next.ServeHTTP(w, r.WithContext(ctx)) return } diff --git a/settings/go.sum b/settings/go.sum index 79e799b944..041c04dad0 100644 --- a/settings/go.sum +++ b/settings/go.sum @@ -196,6 +196,7 @@ github.com/cs3org/go-cs3apis v0.0.0-20200730121022-c4f3d4f7ddfd/go.mod h1:UXha4T github.com/cs3org/go-cs3apis v0.0.0-20200810113633-b00aca449666/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= github.com/cs3org/go-cs3apis v0.0.0-20201007120910-416ed6cf8b00 h1:LVl25JaflluOchVvaHWtoCynm5OaM+VNai0IYkcCSe0= github.com/cs3org/go-cs3apis v0.0.0-20201007120910-416ed6cf8b00/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= +github.com/cs3org/go-cs3apis v0.0.0-20201118090759-87929f5bae21 h1:mZpylrgnCgSeaZ5EznvHIPIKuaQHMHZDi2wkJtk4M8Y= github.com/cs3org/go-cs3apis v0.0.0-20201118090759-87929f5bae21/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= github.com/cs3org/reva v1.1.0 h1:Gih6ECHvMMGSx523SFluFlDmNMuhYelXYShdWvjvW38= github.com/cs3org/reva v1.1.0/go.mod h1:fBzTrNuAKdQ62ybjpdu8nyhBin90/3/3s6DGQDCdBp4= @@ -1332,6 +1333,7 @@ golang.org/x/sys v0.0.0-20200720211630-cb9d2d5c5666/go.mod h1:h1NjWce9XRLGQEsW7w golang.org/x/sys v0.0.0-20200909081042-eff7692f9009/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200918174421-af09f7315aff h1:1CPUrky56AcgSpxz/KfgzQWzfG09u5YOL8MvPYBlrL8= golang.org/x/sys v0.0.0-20200918174421-af09f7315aff/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20201101102859-da207088b7d1 h1:a/mKvvZr9Jcc8oKfcmgzyp7OwF73JPWsQLvH1z2Kxck= golang.org/x/sys v0.0.0-20201101102859-da207088b7d1/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= diff --git a/settings/pkg/proto/v0/settings.pb.micro_test.go b/settings/pkg/proto/v0/settings.pb.micro_test.go index 6f76d468f6..0ca180049c 100644 --- a/settings/pkg/proto/v0/settings.pb.micro_test.go +++ b/settings/pkg/proto/v0/settings.pb.micro_test.go @@ -382,7 +382,7 @@ func TestGetBundleHavingFullPermissionsOnAnotherRole(t *testing.T) { defer teardown() ctx := metadata.Set(context.Background(), middleware.AccountID, testAccountID) - ctx = metadata.Set(ctx, middleware.RoleIDs, getRoleIDAsJSON(svc.BundleUUIDRoleUser)) + ctx = metadata.Set(ctx, middleware.RoleIDs, getRoleIDAsJSON(svc.BundleUUIDRoleAdmin)) saveRequest := proto.SaveBundleRequest{ Bundle: &bundleStub, @@ -392,6 +392,8 @@ func TestGetBundleHavingFullPermissionsOnAnotherRole(t *testing.T) { assert.Equal(t, bundleStub.Id, saveResponse.Bundle.Id) setFullReadWriteOnBundleForAdmin(ctx, t, bundleStub.Id) + + ctx = metadata.Set(ctx, middleware.RoleIDs, getRoleIDAsJSON(svc.BundleUUIDRoleUser)) getRequest := proto.GetBundleRequest{BundleId: bundleStub.Id} getResponse, err := bundleService.GetBundle(ctx, &getRequest) assert.Empty(t, getResponse) diff --git a/settings/pkg/service/v0/service.go b/settings/pkg/service/v0/service.go index 23d8f9ea2e..114066cbe5 100644 --- a/settings/pkg/service/v0/service.go +++ b/settings/pkg/service/v0/service.go @@ -72,9 +72,13 @@ func (g Service) RegisterDefaultRoles() { // SaveBundle implements the BundleServiceHandler interface func (g Service) SaveBundle(ctx context.Context, req *proto.SaveBundleRequest, res *proto.SaveBundleResponse) error { cleanUpResource(ctx, req.Bundle.Resource) + if err := g.checkStaticPermissionsByBundleType(ctx, req.Bundle.Type); err != nil { + return err + } if validationError := validateSaveBundle(req); validationError != nil { return merrors.BadRequest(g.id, "%s", validationError) } + r, err := g.manager.WriteBundle(req.Bundle) if err != nil { return merrors.BadRequest(g.id, "%s", err) @@ -164,9 +168,13 @@ func (g Service) getFilteredBundle(roleIDs []string, bundle *proto.Bundle) *prot // AddSettingToBundle implements the BundleServiceHandler interface func (g Service) AddSettingToBundle(ctx context.Context, req *proto.AddSettingToBundleRequest, res *proto.AddSettingToBundleResponse) error { cleanUpResource(ctx, req.Setting.Resource) + if err := g.checkStaticPermissionsByBundleID(ctx, req.BundleId); err != nil { + return err + } if validationError := validateAddSettingToBundle(req); validationError != nil { return merrors.BadRequest(g.id, "%s", validationError) } + r, err := g.manager.AddSettingToBundle(req.BundleId, req.Setting) if err != nil { return merrors.BadRequest(g.id, "%s", err) @@ -177,9 +185,13 @@ func (g Service) AddSettingToBundle(ctx context.Context, req *proto.AddSettingTo // RemoveSettingFromBundle implements the BundleServiceHandler interface func (g Service) RemoveSettingFromBundle(ctx context.Context, req *proto.RemoveSettingFromBundleRequest, _ *empty.Empty) error { + if err := g.checkStaticPermissionsByBundleID(ctx, req.BundleId); err != nil { + return err + } if validationError := validateRemoveSettingFromBundle(req); validationError != nil { return merrors.BadRequest(g.id, "%s", validationError) } + if err := g.manager.RemoveSettingFromBundle(req.BundleId, req.SettingId); err != nil { return merrors.BadRequest(g.id, "%s", err) } @@ -297,8 +309,8 @@ func (g Service) ListRoleAssignments(ctx context.Context, req *proto.ListRoleAss // AssignRoleToUser implements the RoleServiceHandler interface func (g Service) AssignRoleToUser(ctx context.Context, req *proto.AssignRoleToUserRequest, res *proto.AssignRoleToUserResponse) error { - if !g.hasRoleManagementPermission(ctx) { - return merrors.Forbidden(g.id, "the user is not allowed to assign roles") + if err := g.checkStaticPermissionsByBundleType(ctx, proto.Bundle_TYPE_ROLE); err != nil { + return err } req.AccountUuid = getValidatedAccountUUID(ctx, req.AccountUuid) @@ -315,8 +327,8 @@ func (g Service) AssignRoleToUser(ctx context.Context, req *proto.AssignRoleToUs // RemoveRoleFromUser implements the RoleServiceHandler interface func (g Service) RemoveRoleFromUser(ctx context.Context, req *proto.RemoveRoleFromUserRequest, _ *empty.Empty) error { - if !g.hasRoleManagementPermission(ctx) { - return merrors.Forbidden(g.id, "the user is not allowed to assign roles") + if err := g.checkStaticPermissionsByBundleType(ctx, proto.Bundle_TYPE_ROLE); err != nil { + return err } if validationError := validateRemoveRoleFromUser(req); validationError != nil { @@ -406,7 +418,7 @@ func (g Service) getValueWithIdentifier(value *proto.Value) (*proto.ValueWithIde }, nil } -func (g Service) hasRoleManagementPermission(ctx context.Context) bool { +func (g Service) hasStaticPermission(ctx context.Context, permissionID string) bool { roleIDs, ok := roles.ReadRoleIDsFromContext(ctx) if !ok { /** @@ -420,6 +432,27 @@ func (g Service) hasRoleManagementPermission(ctx context.Context) bool { // tracked as OCIS-454 return true } - p, err := g.manager.ReadPermissionByID(RoleManagementPermissionID, roleIDs) + p, err := g.manager.ReadPermissionByID(permissionID, roleIDs) return err == nil && p != nil } + +func (g Service) checkStaticPermissionsByBundleID(ctx context.Context, bundleID string) error { + bundle, err := g.manager.ReadBundle(bundleID) + if err != nil { + return merrors.NotFound(g.id, "bundle not found: %s", err) + } + return g.checkStaticPermissionsByBundleType(ctx, bundle.Type) +} + +func (g Service) checkStaticPermissionsByBundleType(ctx context.Context, bundleType proto.Bundle_Type) error { + if bundleType == proto.Bundle_TYPE_ROLE { + if !g.hasStaticPermission(ctx, RoleManagementPermissionID) { + return merrors.Forbidden(g.id, "user has no role management permission") + } + return nil + } + if !g.hasStaticPermission(ctx, SettingsManagementPermissionID) { + return merrors.Forbidden(g.id, "user has no settings management permission") + } + return nil +} diff --git a/settings/pkg/service/v0/settings.go b/settings/pkg/service/v0/settings.go index cc8212ea3c..249c53b8ea 100644 --- a/settings/pkg/service/v0/settings.go +++ b/settings/pkg/service/v0/settings.go @@ -16,6 +16,11 @@ const ( RoleManagementPermissionID string = "a53e601e-571f-4f86-8fec-d4576ef49c62" // RoleManagementPermissionName is the hardcoded setting name for the role management permission RoleManagementPermissionName string = "role-management" + + // SettingsManagementPermissionID is the hardcoded setting UUID for the settings management permission + SettingsManagementPermissionID string = "79e13b30-3e22-11eb-bc51-0b9f0bad9a58" + // SettingsManagementPermissionName is the hardcoded setting name for the settings management permission + SettingsManagementPermissionName string = "settings-management" ) // generateBundlesDefaultRoles bootstraps the default roles. @@ -90,5 +95,24 @@ func generatePermissionRequests() []*settings.AddSettingToBundleRequest { }, }, }, + { + BundleId: BundleUUIDRoleAdmin, + Setting: &settings.Setting{ + Id: SettingsManagementPermissionID, + Name: SettingsManagementPermissionName, + DisplayName: "Settings Management", + Description: "This permission gives full access to everything that is related to settings management.", + Resource: &settings.Resource{ + Type: settings.Resource_TYPE_USER, + Id: "all", + }, + Value: &settings.Setting_PermissionValue{ + PermissionValue: &settings.Permission{ + Operation: settings.Permission_OPERATION_READWRITE, + Constraint: settings.Permission_CONSTRAINT_ALL, + }, + }, + }, + }, } } From df391d5966eaa0cfcc703a05678a20f12d38efcb Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Mon, 14 Dec 2020 23:21:53 +0100 Subject: [PATCH 2/3] Changelog --- changelog/unreleased/settings-permissions.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changelog/unreleased/settings-permissions.md diff --git a/changelog/unreleased/settings-permissions.md b/changelog/unreleased/settings-permissions.md new file mode 100644 index 0000000000..32f0876feb --- /dev/null +++ b/changelog/unreleased/settings-permissions.md @@ -0,0 +1,7 @@ +Bugfix: Permission checks for settings write access + +Tags: settings + +There were several endpoints with write access to the settings service that were not protected by permission checks. We introduced a generic settings management permission to fix this for now. Will be more fine grained later on. + +https://github.com/owncloud/ocis/pull/1092 From 633391e30dc8352953008b4ac7c690d602f19de9 Mon Sep 17 00:00:00 2001 From: Benedikt Kulmann Date: Mon, 14 Dec 2020 23:49:56 +0100 Subject: [PATCH 3/3] Set empty role ids array --- ocis-pkg/middleware/account.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ocis-pkg/middleware/account.go b/ocis-pkg/middleware/account.go index 50136668d7..c02ca9f30a 100644 --- a/ocis-pkg/middleware/account.go +++ b/ocis-pkg/middleware/account.go @@ -2,6 +2,7 @@ package middleware import ( "context" + "encoding/json" "net/http" "github.com/cs3org/reva/pkg/token/manager/jwt" @@ -46,7 +47,8 @@ func ExtractAccountUUID(opts ...account.Option) func(http.Handler) http.Handler return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { token := r.Header.Get("x-access-token") if len(token) == 0 { - ctx := metadata.Set(r.Context(), RoleIDs, "") + roleIDsJSON, _ := json.Marshal([]string{}) + ctx := metadata.Set(r.Context(), RoleIDs, string(roleIDsJSON)) next.ServeHTTP(w, r.WithContext(ctx)) return }