From cc89435b9d3e2fdfca8937f21fe63cdcb86ae3a8 Mon Sep 17 00:00:00 2001 From: Denis Arh Date: Wed, 15 May 2019 11:13:38 +0200 Subject: [PATCH] Improve permission rules flush procedure --- internal/permissions/repository.go | 9 +- internal/permissions/rule.go | 18 ++++ internal/permissions/ruleset_checks_test.go | 30 +++--- internal/permissions/ruleset_utils.go | 51 +++++---- internal/permissions/ruleset_utils_test.go | 112 ++++++++++---------- internal/permissions/service.go | 18 +++- 6 files changed, 140 insertions(+), 98 deletions(-) diff --git a/internal/permissions/repository.go b/internal/permissions/repository.go index b2dac2f2b..683d03495 100644 --- a/internal/permissions/repository.go +++ b/internal/permissions/repository.go @@ -62,9 +62,16 @@ func (r *repository) Load() (RuleSet, error) { } func (r *repository) Store(deleteSet, updateSet RuleSet) (err error) { + if len(deleteSet) == 0 && len(updateSet) == 0 { + return + } + return r.dbh.Transaction(func() error { if len(deleteSet) > 0 { - err = r.dbh.Delete(r.dbTable, deleteSet, "rel_role", "resource", "operation") + err = deleteSet.Walk(func(rule *Rule) error { + return r.dbh.Delete(r.dbTable, rule, "rel_role", "resource", "operation") + }) + if err != nil { return err } diff --git a/internal/permissions/rule.go b/internal/permissions/rule.go index e1f6e7105..163019a9c 100644 --- a/internal/permissions/rule.go +++ b/internal/permissions/rule.go @@ -10,6 +10,9 @@ type ( Resource Resource `json:"resource" db:"resource"` Operation Operation `json:"operation" db:"operation"` Access Access `json:"access,string" db:"access"` + + // Do we need to flush it to storage? + dirty bool } ) @@ -37,3 +40,18 @@ func (r Rule) Equals(cmp *Rule) bool { r.Resource == cmp.Resource && r.Operation == cmp.Operation } + +// AllowRule helper func to create allow rule +func AllowRule(id uint64, r Resource, o Operation) *Rule { + return &Rule{id, r, o, Allow, false} +} + +// DenyRule helper func to create deny rule +func DenyRule(id uint64, r Resource, o Operation) *Rule { + return &Rule{id, r, o, Deny, false} +} + +// InheritRule helper func to create inherit rule +func InheritRule(id uint64, r Resource, o Operation) *Rule { + return &Rule{id, r, o, Inherit, false} +} diff --git a/internal/permissions/ruleset_checks_test.go b/internal/permissions/ruleset_checks_test.go index 05ba81084..46b6514bb 100644 --- a/internal/permissions/ruleset_checks_test.go +++ b/internal/permissions/ruleset_checks_test.go @@ -27,9 +27,9 @@ func TestRuleSet_check(t *testing.T) { assert = test.Assert rr = RuleSet{ - &Rule{role1, resThing42, opRead, Allow}, - &Rule{role1, resThing13, opWrite, Deny}, - &Rule{role2, resThing13, opWrite, Allow}, + AllowRule(role1, resThing42, opRead), + DenyRule(role1, resThing13, opWrite), + AllowRule(role2, resThing13, opWrite), } sCases = []struct { @@ -80,7 +80,7 @@ func TestRuleSet_checkResource(t *testing.T) { }{ { RuleSet{ - &Rule{role1, resService1, opAccess, Allow}, + AllowRule(role1, resService1, opAccess), }, []uint64{role1}, resService1, @@ -89,7 +89,7 @@ func TestRuleSet_checkResource(t *testing.T) { }, { RuleSet{ - &Rule{role1, resThingWc, opAccess, Allow}, + AllowRule(role1, resThingWc, opAccess), }, []uint64{role1}, resThing42, @@ -98,8 +98,8 @@ func TestRuleSet_checkResource(t *testing.T) { }, { // deny wc and explictly allow 42 RuleSet{ - &Rule{role1, resThingWc, opAccess, Deny}, - &Rule{role1, resThing42, opAccess, Allow}, + DenyRule(role1, resThingWc, opAccess), + AllowRule(role1, resThing42, opAccess), }, []uint64{role1}, resThing42, @@ -108,8 +108,8 @@ func TestRuleSet_checkResource(t *testing.T) { }, { // deny wc and explictly allow 42 RuleSet{ - &Rule{role1, resThingWc, opAccess, Deny}, - &Rule{role1, resThing42, opAccess, Allow}, + DenyRule(role1, resThingWc, opAccess), + AllowRule(role1, resThing42, opAccess), }, []uint64{role1}, resThing13, @@ -130,14 +130,14 @@ func TestRuleSet_Check(t *testing.T) { var ( rr = RuleSet{ // 1st level - &Rule{role1, resService1, opAccess, Allow}, - &Rule{role2, resService1, opAccess, Deny}, + AllowRule(role1, resService1, opAccess), + DenyRule(role2, resService1, opAccess), // 2nd level - &Rule{EveryoneRoleID, resService2, opAccess, Deny}, - &Rule{role1, resService2, opAccess, Allow}, + DenyRule(EveryoneRoleID, resService2, opAccess), + AllowRule(role1, resService2, opAccess), // 3rd level - &Rule{EveryoneRoleID, resThingWc, opAccess, Deny}, - &Rule{role1, resThing42, opAccess, Allow}, + DenyRule(EveryoneRoleID, resThingWc, opAccess), + AllowRule(role1, resThing42, opAccess), } assert = test.Assert diff --git a/internal/permissions/ruleset_utils.go b/internal/permissions/ruleset_utils.go index 8c095e61a..919c58b37 100644 --- a/internal/permissions/ruleset_utils.go +++ b/internal/permissions/ruleset_utils.go @@ -1,27 +1,25 @@ package permissions -func (set RuleSet) merge(rules ...*Rule) (out RuleSet, err error) { +// merge applies new rules (changes) to existing set and mark all changes as dirty +func (set RuleSet) merge(rules ...*Rule) (out RuleSet) { var ( o int olen = len(set) - - skipInherited = func(r *Rule) (b bool, e error) { - return r != nil, nil - } - - merged = set ) if olen == 0 { - // Nothing exists yet, just assign - merged = rules + // Nothing exists yet + return rules } else { + out = set + newRules: for _, rule := range rules { - for ; o < olen; o++ { - // Never go beyond the last old rule - if merged[o].Equals(rule) { - merged[o].Access = rule.Access + // Never go beyond the last old rule (olen) + for o = 0; o < olen; o++ { + if out[o].Equals(rule) { + out[o].dirty = out[o].Access != rule.Access + out[o].Access = rule.Access // only one rule can match so proceed with next new rule continue newRules @@ -29,26 +27,37 @@ func (set RuleSet) merge(rules ...*Rule) (out RuleSet, err error) { } // none of the old rules matched, append - merged = append(merged, rule) + var c = *rule + c.dirty = true + + out = append(out, &c) } } - // Filter out all rules with access = inherit - return merged.Filter(skipInherited) + return } -func (set RuleSet) split() (inherited, rest RuleSet) { +// dirty returns list of changed (dirty==true) and deleted (Access==Inherit) rules +func (set RuleSet) dirty() (inherited, rest RuleSet) { inherited, rest = RuleSet{}, RuleSet{} for _, r := range set { + var c = *r if r.Access == Inherit { - inherited = append(inherited, r) - } else { - rest = append(rest, r) - + inherited = append(inherited, &c) + } else if r.dirty { + rest = append(rest, &c) } } return } + +// reset dirty flag +func (set RuleSet) clear() { + _ = set.Walk(func(rule *Rule) error { + rule.dirty = false + return nil + }) +} diff --git a/internal/permissions/ruleset_utils_test.go b/internal/permissions/ruleset_utils_test.go index f1a047453..6fc6df948 100644 --- a/internal/permissions/ruleset_utils_test.go +++ b/internal/permissions/ruleset_utils_test.go @@ -14,77 +14,75 @@ func TestRuleSet_merge(t *testing.T) { sCases = []struct { old RuleSet - in RuleSet - exp RuleSet + new RuleSet + del RuleSet + upd RuleSet }{ + { + RuleSet{AllowRule(role1, resService1, opAccess)}, + RuleSet{AllowRule(role1, resService1, opAccess)}, + RuleSet{}, + RuleSet{}, + }, + { + RuleSet{AllowRule(role1, resService1, opAccess)}, + RuleSet{DenyRule(role1, resService1, opAccess)}, + RuleSet{}, + RuleSet{DenyRule(role1, resService1, opAccess)}, + }, + { + RuleSet{AllowRule(role1, resService1, opAccess)}, + RuleSet{InheritRule(role1, resService1, opAccess)}, + RuleSet{InheritRule(role1, resService1, opAccess)}, + RuleSet{}, + }, + { + RuleSet{AllowRule(role1, resService1, opAccess)}, + RuleSet{AllowRule(role1, resService1, opAccess)}, + RuleSet{}, + RuleSet{}, + }, { RuleSet{ - &Rule{role1, resService1, opAccess, Allow}, - &Rule{role2, resService1, opAccess, Deny}, - &Rule{EveryoneRoleID, resService2, opAccess, Deny}, - &Rule{role1, resService2, opAccess, Allow}, + AllowRule(role1, resService1, opAccess), + DenyRule(role2, resService1, opAccess), + DenyRule(EveryoneRoleID, resService2, opAccess), + AllowRule(role1, resService2, opAccess), + AllowRule(role2, resThing42, opAccess), }, RuleSet{ - &Rule{EveryoneRoleID, resThingWc, opAccess, Deny}, - &Rule{role1, resThing42, opAccess, Allow}, - &Rule{role1, resThing42, opAccess, Inherit}, + DenyRule(EveryoneRoleID, resThingWc, opAccess), + AllowRule(role1, resService2, opAccess), + AllowRule(role1, resThing42, opAccess), + InheritRule(role2, resThing42, opAccess), }, RuleSet{ - &Rule{role1, resService1, opAccess, Allow}, - &Rule{role2, resService1, opAccess, Deny}, - &Rule{EveryoneRoleID, resService2, opAccess, Deny}, - &Rule{role1, resService2, opAccess, Allow}, - &Rule{EveryoneRoleID, resThingWc, opAccess, Deny}, - &Rule{role1, resThing42, opAccess, Allow}, - &Rule{role1, resThing42, opAccess, Inherit}, + InheritRule(role2, resThing42, opAccess), + }, + RuleSet{ + // AllowRule(role1, resService1, opAccess), + // DenyRule(role2, resService1, opAccess), + // DenyRule(EveryoneRoleID, resService2, opAccess), + // AllowRule(role1, resService2, opAccess), + DenyRule(EveryoneRoleID, resThingWc, opAccess), + AllowRule(role1, resThing42, opAccess), }, }, } ) for c, sc := range sCases { - out, _ := sc.old.merge(sc.in...) + // Apply changed and get update candidates + mrg := sc.old.merge(sc.new...) + del, upd := mrg.dirty() - assert(t, len(out) == len(sc.exp), "Check test #%d failed, expected length %d, got %d", c, len(out), len(sc.exp)) - assert(t, reflect.DeepEqual(out, sc.exp), "Check test #%d failed, reflect.DeepEqual == false", c) - - } -} - -// Test role inheritance -func TestRuleSet_split(t *testing.T) { - var ( - assert = test.Assert - - sCases = []struct { - set RuleSet - i RuleSet - r RuleSet - }{ - { - RuleSet{ - &Rule{role1, resService1, opAccess, Allow}, - &Rule{role2, resService1, opAccess, Deny}, - &Rule{EveryoneRoleID, resService2, opAccess, Inherit}, - }, - RuleSet{ - &Rule{EveryoneRoleID, resService2, opAccess, Inherit}, - }, - RuleSet{ - &Rule{role1, resService1, opAccess, Allow}, - &Rule{role2, resService1, opAccess, Deny}, - }, - }, - } - ) - - for c, sc := range sCases { - i, r := sc.set.split() - - assert(t, len(i) == len(sc.i), "Check test #%d failed, expected length %d, got %d", c, len(i), len(sc.i)) - assert(t, len(r) == len(sc.r), "Check test #%d failed, expected length %d, got %d", c, len(r), len(sc.r)) - assert(t, reflect.DeepEqual(i, sc.i), "Check test #%d failed, reflect.DeepEqual == false", c) - assert(t, reflect.DeepEqual(r, sc.r), "Check test #%d failed, reflect.DeepEqual == false", c) + // Clear dirty flag so that we do not confuse DeepEqual + del.clear() + upd.clear() + assert(t, len(del) == len(sc.del), "Check test #%d failed, expected delete list length %d, got %d", c, len(sc.del), len(del)) + assert(t, len(upd) == len(sc.upd), "Check test #%d failed, expected update list length %d, got %d", c, len(sc.upd), len(upd)) + assert(t, reflect.DeepEqual(del, sc.del), "Check test #%d failed for delete list, reflect.DeepEqual == false", c) + assert(t, reflect.DeepEqual(upd, sc.upd), "Check test #%d failed for update list, reflect.DeepEqual == false", c) } } diff --git a/internal/permissions/service.go b/internal/permissions/service.go index f1e833810..8950d52d3 100644 --- a/internal/permissions/service.go +++ b/internal/permissions/service.go @@ -103,9 +103,7 @@ func (svc *service) Grant(ctx context.Context, wl Whitelist, rules ...*Rule) (er } } - if svc.rules, err = svc.rules.merge(rules...); err != nil { - return - } + svc.rules = svc.rules.merge(rules...) return svc.flush(ctx) } @@ -131,6 +129,17 @@ func (svc service) Watch(ctx context.Context) { svc.logger.Debug("watcher initialized") } +func (svc service) FindRulesByRoleID(roleID uint64) (rr RuleSet) { + svc.l.Lock() + defer svc.l.Unlock() + + rr, _ = svc.rules.Filter(func(rule *Rule) (b bool, e error) { + return rule.RoleID == roleID, nil + }) + + return +} + func (svc *service) Reload(ctx context.Context) { svc.l.Lock() defer svc.l.Unlock() @@ -149,13 +158,14 @@ func (svc *service) Reload(ctx context.Context) { } func (svc service) flush(ctx context.Context) (err error) { - d, u := svc.rules.split() + d, u := svc.rules.dirty() err = svc.repository.With(ctx).Store(d, u) if err != nil { return } + u.clear() svc.rules = u svc.logger.Info("flushed rules", zap.Int("updated", len(u)),