Improve permission rules flush procedure

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
 			}

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}
+}

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

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
+	})
+}

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)
 	}
 }

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)),