From 5c317cdbdf0c534c484901ab3a6a9d4ee8f39ec6 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Thu, 26 May 2022 20:56:20 +0200 Subject: [PATCH] Add proper support and access control for record owner --- compose/record.cue | 1 + compose/service/access_control.gen.go | 19 +++- compose/service/record.go | 104 +++++++++++++++---- compose/service/record_test.go | 115 ++++++++++++++++++++++ pkg/envoy/store/compose_record_marshal.go | 3 +- 5 files changed, 217 insertions(+), 25 deletions(-) diff --git a/compose/record.cue b/compose/record.cue index 03484d05b..f5dd2426f 100644 --- a/compose/record.cue +++ b/compose/record.cue @@ -42,6 +42,7 @@ record: schema.#Resource & { "read": {} "update": {} "delete": {} + "owner.manage": {} } } diff --git a/compose/service/access_control.gen.go b/compose/service/access_control.gen.go index 523a52e29..8392b57d7 100644 --- a/compose/service/access_control.gen.go +++ b/compose/service/access_control.gen.go @@ -184,6 +184,11 @@ func (svc accessControl) List() (out []map[string]string) { "any": types.RecordRbacResource(0, 0, 0), "op": "delete", }, + { + "type": types.RecordResourceType, + "any": types.RecordRbacResource(0, 0, 0), + "op": "owner.manage", + }, { "type": types.ComponentResourceType, "any": types.ComponentRbacResource(), @@ -469,6 +474,13 @@ func (svc accessControl) CanDeleteRecord(ctx context.Context, r *types.Record) b return svc.can(ctx, "delete", r) } +// CanManageOwnerOnRecord checks if current user can owner.manage +// +// This function is auto-generated +func (svc accessControl) CanManageOwnerOnRecord(ctx context.Context, r *types.Record) bool { + return svc.can(ctx, "owner.manage", r) +} + // CanGrant checks if current user can manage compose permissions // // This function is auto-generated @@ -586,9 +598,10 @@ func rbacResourceOperations(r string) map[string]bool { } case types.RecordResourceType: return map[string]bool{ - "read": true, - "update": true, - "delete": true, + "read": true, + "update": true, + "delete": true, + "owner.manage": true, } case types.ComponentResourceType: return map[string]bool{ diff --git a/compose/service/record.go b/compose/service/record.go index 518b27a70..6887aeb80 100644 --- a/compose/service/record.go +++ b/compose/service/record.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/cortezaproject/corteza-server/pkg/locale" "regexp" "sort" "strconv" @@ -70,6 +71,10 @@ type ( CanUpdateRecordValueOnModuleField(context.Context, *types.ModuleField) bool } + recordManageOwnerAccessController interface { + CanManageOwnerOnRecord(context.Context, *types.Record) bool + } + recordAccessController interface { CanCreateRecordOnModule(context.Context, *types.Module) bool CanSearchRecordsOnModule(context.Context, *types.Module) bool @@ -79,6 +84,7 @@ type ( CanUpdateRecord(context.Context, *types.Record) bool CanDeleteRecord(context.Context, *types.Record) bool + recordManageOwnerAccessController recordValueAccessController } @@ -617,29 +623,68 @@ func RecordValueSanitization(m *types.Module, vv types.RecordValueSet) (err erro return } -func RecordUpdateOwner(invokerID uint64, r, old *types.Record) *types.Record { - if old == nil { - if r.OwnedBy == 0 { - // If od owner is not set, make current user - // the owner of the record - r.OwnedBy = invokerID +func SetRecordOwner(ctx context.Context, ac recordManageOwnerAccessController, s store.Storer, old, upd *types.Record, invoker uint64) *types.RecordValueErrorSet { + if upd == nil { + // no-op + return nil + } + + var ( + curOwner uint64 + updOwner = upd.OwnedBy + ) + + if old != nil { + curOwner = old.OwnedBy + } + + updOwner = CalcRecordOwner(curOwner, updOwner, invoker) + + var ( + mkError = func(kind, tkey string) *types.RecordValueErrorSet { + return &types.RecordValueErrorSet{Set: []types.RecordValueError{{ + Kind: kind, + Meta: map[string]interface{}{"field": "", "value": updOwner}, + Message: locale.Global().T(ctx, "compose", tkey), + }}} } - } else { - if r.OwnedBy == 0 { - if old.OwnedBy > 0 { - // Owner not set/send in the payload - // - // Fallback to old owner (if set) - r.OwnedBy = old.OwnedBy - } else { - // If od owner is not set, make current user - // the owner of the record - r.OwnedBy = invokerID - } + ) + + if (old != nil && curOwner != updOwner) || (old == nil && updOwner != invoker) { + // check if ownership can be changed when: + // a) updating (old != nil) and ownership changed + // b) creating (old == nil) and ownership id not set to the invoking user + if !ac.CanManageOwnerOnRecord(ctx, upd) { + return mkError("accessDenied", "record.errors.ownershipChangeDenied") } } - return r + if _, err := store.LookupUserByID(ctx, s, updOwner); err != nil { + if errors.IsNotFound(err) { + return mkError("invalidValue", "record.errors.invalidOwner") + } else { + return mkError("internal", "record.errors.store") + } + } + + upd.OwnedBy = updOwner + return nil +} + +func CalcRecordOwner(current, new, invoker uint64) uint64 { + if invoker == 0 { + // invoker is, for some reason 0, + // use current owner as invoker + invoker = current + } + + if new == 0 { + // if new owner is not set, use invoker + return invoker + } + + // keep owner unchanged + return new } func RecordValueUpdateOpCheck(ctx context.Context, ac recordValueAccessController, m *types.Module, vv types.RecordValueSet) *types.RecordValueErrorSet { @@ -873,7 +918,22 @@ func (svc record) procCreate(ctx context.Context, invokerID uint64, m *types.Mod new.DeletedAt = nil new.DeletedBy = 0 - new = RecordUpdateOwner(invokerID, new, nil) + if new.OwnedBy == 0 { + new.OwnedBy = invokerID + } + + if new.OwnedBy != invokerID { + // we're creating record here and this check goes against current + // RBAC implementation logic on record creation + if !svc.ac.CanManageOwnerOnRecord(ctx, new) { + // not permitted to change the owner + + } + } + + if err := SetRecordOwner(ctx, svc.ac, svc.store, nil, new, invokerID); err != nil { + return err + } if rve = RecordValueUpdateOpCheck(ctx, svc.ac, m, new.Values); !rve.IsValid() { return @@ -924,7 +984,9 @@ func (svc record) procUpdate(ctx context.Context, invokerID uint64, m *types.Mod upd.DeletedAt = old.DeletedAt upd.DeletedBy = old.DeletedBy - upd = RecordUpdateOwner(invokerID, upd, old) + if err := SetRecordOwner(ctx, svc.ac, svc.store, old, upd, invokerID); err != nil { + return err + } upd.Values = old.Values.Merge(m.Fields, upd.Values, func(f *types.ModuleField) bool { return svc.ac.CanUpdateRecordValueOnModuleField(ctx, m.Fields.FindByName(f.Name)) diff --git a/compose/service/record_test.go b/compose/service/record_test.go index 9d720201e..7840dc11f 100644 --- a/compose/service/record_test.go +++ b/compose/service/record_test.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "github.com/davecgh/go-spew/spew" "testing" "github.com/cortezaproject/corteza-server/compose/service/values" @@ -856,3 +857,117 @@ func TestRecord_contextualRolesAccessControl(t *testing.T) { hits, _, err = svc.Find(ctx, f) req.Len(hits, 9) } + +func TestSetRecordOwner(t *testing.T) { + var ( + req = require.New(t) + + // uncomment to enable sql conn debugging + //ctx = logger.ContextWithValue(context.Background(), logger.MakeDebugLogger()) + ctx = context.Background() + s, err = sqlite.ConnectInMemoryWithDebug(ctx) + ) + + req.NoError(err) + req.NoError(store.Upgrade(ctx, zap.NewNop(), s)) + req.NoError(store.TruncateComposeNamespaces(ctx, s)) + req.NoError(store.TruncateComposeModules(ctx, s)) + req.NoError(store.TruncateComposeRecords(ctx, s, nil)) + req.NoError(store.TruncateRbacRules(ctx, s)) + + var ( + rbacService = rbac.NewService( + //zap.NewNop(), + logger.MakeDebugLogger(), + nil, + ) + ac = &accessControl{rbac: rbacService} + + invoker = &sysTypes.User{ID: 1001} + originalOwner = &sysTypes.User{ID: 2001} + alternativeOwner = &sysTypes.User{ID: 2002} + + role = &sysTypes.Role{Name: "role-with-ownership-change-permission", ID: 3000} + + old, upd *types.Record + rvse *types.RecordValueErrorSet + ) + + rbacService.UpdateRoles( + rbac.CommonRole.Make(role.ID, role.Handle), + ) + + req.NoError(rbacService.Grant(ctx, rbac.AllowRule(role.ID, types.RecordRbacResource(0, 0, 0), "owner.manage"))) + spew.Dump(rbacService.Rules()) + req.NoError(store.CreateUser(ctx, s, invoker, originalOwner, alternativeOwner)) + + t.Run("invalid input", func(t *testing.T) { + old, upd = nil, nil + require.Nil(t, SetRecordOwner(ctx, ac, s, old, upd, invoker.ID)) + }) + + t.Run("new record owner is invoker", func(t *testing.T) { + old, upd = nil, &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3} + + // this must work, invoker can always set owner to self on a new record + if rvse = SetRecordOwner(ctx, ac, s, old, upd, invoker.ID); !rvse.IsValid() { + t.Fatalf("errors: %v", rvse.Set) + } + + req.Equal(upd.OwnedBy, invoker.ID) + }) + + t.Run("deny setting to alternative owner on create", func(t *testing.T) { + old, upd = nil, &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: alternativeOwner.ID} + + // this must work, invoker can always set owner to self on a new record + if rvse = SetRecordOwner(ctx, ac, s, old, upd, invoker.ID); rvse.IsValid() { + t.Fatalf("expecting error") + } + }) + + t.Run("deny setting to alternative owner on update", func(t *testing.T) { + old = &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: originalOwner.ID} + upd = &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: alternativeOwner.ID} + + // this must work, invoker can always set owner to self on a new record + if rvse = SetRecordOwner(ctx, ac, s, old, upd, invoker.ID); rvse.IsValid() { + t.Fatalf("expecting error") + } + }) + + t.Run("allow setting to alternative owner on create", func(t *testing.T) { + ctx = auth.SetIdentityToContext(ctx, auth.Authenticated(invoker.ID, role.ID)) + old, upd = nil, &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: alternativeOwner.ID} + + if rvse = SetRecordOwner(ctx, ac, s, old, upd, invoker.ID); !rvse.IsValid() { + t.Fatalf("errors: %v", rvse.Set) + } + + req.Equal(upd.OwnedBy, alternativeOwner.ID) + }) + + t.Run("allow setting to alternative owner on update", func(t *testing.T) { + ctx = auth.SetIdentityToContext(ctx, auth.Authenticated(invoker.ID, role.ID)) + old = &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: originalOwner.ID} + upd = &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: alternativeOwner.ID} + + if rvse = SetRecordOwner(ctx, ac, s, old, upd, invoker.ID); !rvse.IsValid() { + t.Fatalf("errors: %v", rvse.Set) + } + + req.Equal(upd.OwnedBy, alternativeOwner.ID) + }) + + t.Run("allow setting to new owner to zerp", func(t *testing.T) { + ctx = auth.SetIdentityToContext(ctx, auth.Authenticated(invoker.ID, role.ID)) + old = &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: originalOwner.ID} + upd = &types.Record{ID: 1, NamespaceID: 2, ModuleID: 3, OwnedBy: 0} + + if rvse = SetRecordOwner(ctx, ac, s, old, upd, invoker.ID); !rvse.IsValid() { + t.Fatalf("errors: %v", rvse.Set) + } + + req.Equal(upd.OwnedBy, invoker.ID) + }) +} diff --git a/pkg/envoy/store/compose_record_marshal.go b/pkg/envoy/store/compose_record_marshal.go index 713c691f3..d8dc57299 100644 --- a/pkg/envoy/store/compose_record_marshal.go +++ b/pkg/envoy/store/compose_record_marshal.go @@ -327,7 +327,8 @@ func (n *composeRecord) Encode(ctx context.Context, pl *payload) (err error) { rec.OwnedBy = ux[r.Us.OwnedBy.Ref] } } - service.RecordUpdateOwner(pl.invokerID, rec, old) + + rec.OwnedBy = service.CalcRecordOwner(old.OwnedBy, rec.OwnedBy, pl.invokerID) rvs := make(composeTypes.RecordValueSet, 0, len(r.Values)) for k, v := range r.Values {