diff --git a/enforcer_cached.go b/enforcer_cached.go index fc8883ba..a9b4be40 100644 --- a/enforcer_cached.go +++ b/enforcer_cached.go @@ -114,8 +114,8 @@ func (e *CachedEnforcer) RemovePolicy(params ...interface{}) (bool, error) { func (e *CachedEnforcer) RemovePolicies(rules [][]string) (bool, error) { if len(rules) != 0 { if atomic.LoadInt32(&e.enableCache) != 0 { - irule := make([]interface{}, len(rules[0])) for _, rule := range rules { + irule := make([]interface{}, len(rule)) for i, param := range rule { irule[i] = param } diff --git a/enforcer_cached_synced.go b/enforcer_cached_synced.go index cc46afed..3ae26134 100644 --- a/enforcer_cached_synced.go +++ b/enforcer_cached_synced.go @@ -167,8 +167,8 @@ func (e *SyncedCachedEnforcer) checkOneAndRemoveCache(params ...interface{}) (bo func (e *SyncedCachedEnforcer) checkManyAndRemoveCache(rules [][]string) (bool, error) { if len(rules) != 0 { if atomic.LoadInt32(&e.enableCache) != 0 { - irule := make([]interface{}, len(rules[0])) for _, rule := range rules { + irule := make([]interface{}, len(rule)) for i, param := range rule { irule[i] = param } diff --git a/enforcer_cached_synced_test.go b/enforcer_cached_synced_test.go index 1c7da267..04ac1fdc 100644 --- a/enforcer_cached_synced_test.go +++ b/enforcer_cached_synced_test.go @@ -83,3 +83,37 @@ func TestSyncCache(t *testing.T) { testSyncEnforceCache(t, e, "alice", "data2", "read", true) testSyncEnforceCache(t, e, "alice", "data2", "write", true) } + +// TestSyncRemovePoliciesCacheRaggedRulesStaleEntry verifies that RemovePolicies +// invalidates the cached decision of every rule in the batch even when the +// rules have different lengths. The key buffer used to be sized from the first +// rule and reused, so a later shorter rule produced a key with a stale tail and +// the cached decision of a removed rule survived the removal. +func TestSyncRemovePoliciesCacheRaggedRulesStaleEntry(t *testing.T) { + e, _ := NewSyncedCachedEnforcer("examples/rbac_model.conf", "examples/rbac_policy.csv") + + _, _ = e.AddPolicies([][]string{{"bob", "data2", "write", "extra"}}) + testSyncEnforceCache(t, e, "alice", "data1", "read", true) + + _, _ = e.RemovePolicies([][]string{ + {"bob", "data2", "write", "extra"}, + {"alice", "data1", "read"}, + }) + + testSyncEnforceCache(t, e, "alice", "data1", "read", false) +} + +// TestSyncRemovePoliciesCacheRaggedRulesLongerRule verifies that a rule longer +// than the first rule of the batch does not panic. The key buffer used to be +// sized from the first rule and reused, so writing a longer rule panicked with +// index out of range. +func TestSyncRemovePoliciesCacheRaggedRulesLongerRule(t *testing.T) { + e, _ := NewSyncedCachedEnforcer("examples/rbac_model.conf", "examples/rbac_policy.csv") + + testSyncEnforceCache(t, e, "alice", "data1", "read", true) + + _, _ = e.RemovePolicies([][]string{ + {"alice", "data1", "read"}, + {"bob", "data2", "write", "extra"}, + }) +} diff --git a/enforcer_cached_test.go b/enforcer_cached_test.go index b404b651..a11725dc 100644 --- a/enforcer_cached_test.go +++ b/enforcer_cached_test.go @@ -74,3 +74,37 @@ func TestCache(t *testing.T) { testEnforceCache(t, e, "alice", "data2", "read", false) testEnforceCache(t, e, "alice", "data2", "write", false) } + +// TestRemovePoliciesCacheRaggedRulesStaleEntry verifies that RemovePolicies +// invalidates the cached decision of every rule in the batch even when the +// rules have different lengths. The key buffer used to be sized from the first +// rule and reused, so a later shorter rule produced a key with a stale tail and +// the cached decision of a removed rule survived the removal. +func TestRemovePoliciesCacheRaggedRulesStaleEntry(t *testing.T) { + e, _ := NewCachedEnforcer("examples/rbac_model.conf", "examples/rbac_policy.csv") + + _, _ = e.AddPolicies([][]string{{"bob", "data2", "write", "extra"}}) + testEnforceCache(t, e, "alice", "data1", "read", true) + + _, _ = e.RemovePolicies([][]string{ + {"bob", "data2", "write", "extra"}, + {"alice", "data1", "read"}, + }) + + testEnforceCache(t, e, "alice", "data1", "read", false) +} + +// TestRemovePoliciesCacheRaggedRulesLongerRule verifies that a rule longer +// than the first rule of the batch does not panic. The key buffer used to be +// sized from the first rule and reused, so writing a longer rule panicked with +// index out of range. +func TestRemovePoliciesCacheRaggedRulesLongerRule(t *testing.T) { + e, _ := NewCachedEnforcer("examples/rbac_model.conf", "examples/rbac_policy.csv") + + testEnforceCache(t, e, "alice", "data1", "read", true) + + _, _ = e.RemovePolicies([][]string{ + {"alice", "data1", "read"}, + {"bob", "data2", "write", "extra"}, + }) +}