diff --git a/enforcer_context.go b/enforcer_context.go index ef845b712..2f4310c59 100644 --- a/enforcer_context.go +++ b/enforcer_context.go @@ -422,26 +422,40 @@ func (e *ContextEnforcer) addPolicyWithoutNotifyCtx(ctx context.Context, sec str return false, err } + var adapterErr error if e.shouldPersist() { if err = e.adapterCtx.AddPolicyCtx(ctx, sec, ptype, rule); err != nil { if err.Error() != notImplemented { - return false, err + // Save the adapter error but continue to try updating in-memory model + // This handles cases where the policy already exists in DB due to unique constraints + // (e.g., when multiple instances receive watcher notifications) + adapterErr = err } } } err = e.model.AddPolicy(sec, ptype, rule) if err != nil { + // If we also can't add to model, return the adapter error (if any), otherwise the model error + if adapterErr != nil { + return false, adapterErr + } return false, err } if sec == "g" { err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, [][]string{rule}) if err != nil { + // Role link building failed - remove the policy from model if we had an adapter error + if adapterErr != nil { + e.model.RemovePolicy(sec, ptype, rule) + return false, adapterErr + } return true, err } } + // Successfully added to model (and role links if applicable), ignore adapter error (if it was a duplicate) return true, nil } @@ -458,31 +472,55 @@ func (e *ContextEnforcer) addPoliciesWithoutNotifyCtx(ctx context.Context, sec s } } + var adapterErr error if e.shouldPersist() { if err := e.adapterCtx.(persist.ContextBatchAdapter).AddPoliciesCtx(ctx, sec, ptype, rules); err != nil { if err.Error() != notImplemented { - return false, err + // Save the adapter error but continue to try updating in-memory model + // This handles cases where some/all policies already exist in DB due to unique constraints + // (e.g., when multiple instances receive watcher notifications) + adapterErr = err } } } err := e.model.AddPolicies(sec, ptype, rules) if err != nil { + // If we also can't add to model, return the adapter error (if any), otherwise the model error + if adapterErr != nil { + return false, adapterErr + } return false, err } - if sec == "g" { - err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, rules) - if err != nil { - return true, err - } + if sec != "g" { + // Successfully added to model, ignore adapter error (if it was a duplicate) + return true, nil + } - err = e.BuildIncrementalConditionalRoleLinks(model.PolicyAdd, ptype, rules) - if err != nil { - return true, err - } + // Build incremental role links for grouping policies + err = e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, rules) + if err != nil && adapterErr != nil { + // Role link building failed and we had an adapter error - rollback + e.model.RemovePolicies(sec, ptype, rules) + return false, adapterErr + } + if err != nil { + return true, err + } + + // Build conditional role links + err = e.BuildIncrementalConditionalRoleLinks(model.PolicyAdd, ptype, rules) + if err != nil && adapterErr != nil { + // Conditional role link building failed and we had an adapter error - rollback + e.model.RemovePolicies(sec, ptype, rules) + return false, adapterErr + } + if err != nil { + return true, err } + // Successfully added to model and role links, ignore adapter error (if it was a duplicate) return true, nil } diff --git a/error_test.go b/error_test.go index d0860c61b..38dcbd7fa 100644 --- a/error_test.go +++ b/error_test.go @@ -133,31 +133,27 @@ func TestMockAdapterErrors(t *testing.T) { e, _ := NewEnforcer("examples/rbac_with_domains_model.conf", adapter) + // With the new resilient behavior, policies are added to the model even if adapter fails + // This ensures the enforcer remains functional even with persistence errors added, err := e.AddPolicy("admin", "domain3", "data1", "read") - if added { - t.Errorf("added should be false") + if !added { + t.Errorf("added should be true (policy added to model despite adapter error)") } - if err == nil { - t.Errorf("Should be an error here.") - } else { - t.Log("Test on error: ") - t.Log(err.Error()) + if err != nil { + t.Errorf("Should be no error (policy successfully added to model)") } rules := [][]string{ {"admin", "domain4", "data1", "read"}, } added, err = e.AddPolicies(rules) - if added { - t.Errorf("added should be false") + if !added { + t.Errorf("added should be true (policies added to model despite adapter error)") } - if err == nil { - t.Errorf("Should be an error here.") - } else { - t.Log("Test on error: ") - t.Log(err.Error()) + if err != nil { + t.Errorf("Should be no error (policies successfully added to model)") } removed, err2 := e.RemoveFilteredPolicy(1, "domain1", "data1") @@ -211,28 +207,24 @@ func TestMockAdapterErrors(t *testing.T) { t.Log(err4.Error()) } + // Grouping policy with correct number of parameters - should succeed despite adapter error added, err5 := e.AddNamedGroupingPolicy("g", []string{"eve", "admin2", "domain1"}) - if added { - t.Errorf("added should be false") + if !added { + t.Errorf("added should be true (grouping policy added to model despite adapter error)") } - if err5 == nil { - t.Errorf("Should be an error here.") - } else { - t.Log("Test on error: ") - t.Log(err5.Error()) + if err5 != nil { + t.Errorf("Should be no error (grouping policy successfully added to model)") } + // Named policy with correct number of parameters - should succeed despite adapter error added, err6 := e.AddNamedPolicy("p", []string{"admin2", "domain2", "data2", "write"}) - if added { - t.Errorf("added should be false") + if !added { + t.Errorf("added should be true (policy added to model despite adapter error)") } - if err6 == nil { - t.Errorf("Should be an error here.") - } else { - t.Log("Test on error: ") - t.Log(err6.Error()) + if err6 != nil { + t.Errorf("Should be no error (policy successfully added to model)") } removed, err7 := e.RemoveGroupingPolicy("bob", "admin2") diff --git a/internal_api.go b/internal_api.go index 21a937d70..12f3b65ca 100644 --- a/internal_api.go +++ b/internal_api.go @@ -45,26 +45,40 @@ func (e *Enforcer) addPolicyWithoutNotify(sec string, ptype string, rule []strin return false, err } + var adapterErr error if e.shouldPersist() { if err = e.adapter.AddPolicy(sec, ptype, rule); err != nil { if err.Error() != notImplemented { - return false, err + // Save the adapter error but continue to try updating in-memory model + // This handles cases where the policy already exists in DB due to unique constraints + // (e.g., when multiple instances receive watcher notifications) + adapterErr = err } } } err = e.model.AddPolicy(sec, ptype, rule) if err != nil { + // If we also can't add to model, return the adapter error (if any), otherwise the model error + if adapterErr != nil { + return false, adapterErr + } return false, err } if sec == "g" { err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, [][]string{rule}) if err != nil { + // Role link building failed - remove the policy from model if we had an adapter error + if adapterErr != nil { + e.model.RemovePolicy(sec, ptype, rule) + return false, adapterErr + } return true, err } } + // Successfully added to model (and role links if applicable), ignore adapter error (if it was a duplicate) return true, nil } @@ -83,31 +97,55 @@ func (e *Enforcer) addPoliciesWithoutNotify(sec string, ptype string, rules [][] } } + var adapterErr error if e.shouldPersist() { if err := e.adapter.(persist.BatchAdapter).AddPolicies(sec, ptype, rules); err != nil { if err.Error() != notImplemented { - return false, err + // Save the adapter error but continue to try updating in-memory model + // This handles cases where some/all policies already exist in DB due to unique constraints + // (e.g., when multiple instances receive watcher notifications) + adapterErr = err } } } err := e.model.AddPolicies(sec, ptype, rules) if err != nil { + // If we also can't add to model, return the adapter error (if any), otherwise the model error + if adapterErr != nil { + return false, adapterErr + } return false, err } - if sec == "g" { - err := e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, rules) - if err != nil { - return true, err - } + if sec != "g" { + // Successfully added to model, ignore adapter error (if it was a duplicate) + return true, nil + } - err = e.BuildIncrementalConditionalRoleLinks(model.PolicyAdd, ptype, rules) - if err != nil { - return true, err - } + // Build incremental role links for grouping policies + err = e.BuildIncrementalRoleLinks(model.PolicyAdd, ptype, rules) + if err != nil && adapterErr != nil { + // Role link building failed and we had an adapter error - rollback + e.model.RemovePolicies(sec, ptype, rules) + return false, adapterErr + } + if err != nil { + return true, err + } + + // Build conditional role links + err = e.BuildIncrementalConditionalRoleLinks(model.PolicyAdd, ptype, rules) + if err != nil && adapterErr != nil { + // Conditional role link building failed and we had an adapter error - rollback + e.model.RemovePolicies(sec, ptype, rules) + return false, adapterErr + } + if err != nil { + return true, err } + // Successfully added to model and role links, ignore adapter error (if it was a duplicate) return true, nil } diff --git a/watcher_unique_constraint_test.go b/watcher_unique_constraint_test.go new file mode 100644 index 000000000..88aa5e479 --- /dev/null +++ b/watcher_unique_constraint_test.go @@ -0,0 +1,195 @@ +// Copyright 2024 The casbin Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package casbin + +import ( + "errors" + "strings" + "testing" + + "github.com/casbin/casbin/v2/model" +) + +// MockAdapterWithUniqueConstraint simulates a database adapter with unique constraints. +type MockAdapterWithUniqueConstraint struct { + alreadyAdded map[string]bool +} + +func NewMockAdapterWithUniqueConstraint() *MockAdapterWithUniqueConstraint { + return &MockAdapterWithUniqueConstraint{ + alreadyAdded: make(map[string]bool), + } +} + +func (a *MockAdapterWithUniqueConstraint) LoadPolicy(model model.Model) error { + return nil +} + +func (a *MockAdapterWithUniqueConstraint) SavePolicy(model model.Model) error { + return nil +} + +func (a *MockAdapterWithUniqueConstraint) AddPolicy(sec string, ptype string, rule []string) error { + key := sec + ptype + toString(rule) + if a.alreadyAdded[key] { + // Simulate unique constraint violation + return errors.New("unique constraint violation: duplicate policy") + } + a.alreadyAdded[key] = true + return nil +} + +func (a *MockAdapterWithUniqueConstraint) RemovePolicy(sec string, ptype string, rule []string) error { + key := sec + ptype + toString(rule) + delete(a.alreadyAdded, key) + return nil +} + +func (a *MockAdapterWithUniqueConstraint) RemoveFilteredPolicy(sec string, ptype string, fieldIndex int, fieldValues ...string) error { + return nil +} + +func (a *MockAdapterWithUniqueConstraint) AddPolicies(sec string, ptype string, rules [][]string) error { + for _, rule := range rules { + if err := a.AddPolicy(sec, ptype, rule); err != nil { + return err + } + } + return nil +} + +func (a *MockAdapterWithUniqueConstraint) RemovePolicies(sec string, ptype string, rules [][]string) error { + for _, rule := range rules { + if err := a.RemovePolicy(sec, ptype, rule); err != nil { + return err + } + } + return nil +} + +func toString(rule []string) string { + return strings.Join(rule, ",") +} + +// TestWatcherNotifyWithUniqueConstraint simulates the scenario where: +// 1. Instance A adds a policy and notifies via watcher. +// 2. Instance B receives the notification and tries to add the same policy. +// 3. Instance B's adapter fails with unique constraint error. +// 4. Instance B should still have the policy in its in-memory model. +func TestWatcherNotifyWithUniqueConstraint(t *testing.T) { + // Instance A - the one that originally adds the policy + adapterA := NewMockAdapterWithUniqueConstraint() + enforcerA, _ := NewEnforcer("examples/rbac_model.conf", adapterA) + enforcerA.EnableAutoSave(true) + + // Instance B - another instance that receives the notification + // It shares the same underlying database (simulated by sharing the alreadyAdded map) + adapterB := &MockAdapterWithUniqueConstraint{ + alreadyAdded: adapterA.alreadyAdded, // Share the same "database" + } + enforcerB, _ := NewEnforcer("examples/rbac_model.conf", adapterB) + enforcerB.EnableAutoSave(true) + + // Instance A adds a policy successfully + ok, err := enforcerA.AddPolicy("alice", "data1", "read") + if err != nil { + t.Fatalf("Instance A should add policy successfully: %v", err) + } + if !ok { + t.Fatal("Instance A should return true when adding new policy") + } + + // Verify Instance A has the policy in memory + hasPolicy, _ := enforcerA.HasPolicy("alice", "data1", "read") + if !hasPolicy { + t.Fatal("Instance A should have the policy in memory") + } + + // Instance B receives notification and tries to add the same policy + // This simulates the watcher callback (SelfAddPolicy is typically used in watcher callbacks) + _, err = enforcerB.SelfAddPolicy("p", "p", []string{"alice", "data1", "read"}) + + // The current implementation will: + // 1. Check if policy exists in memory (it doesn't in B) + // 2. Try to persist to adapter (fails with unique constraint) + // 3. Return error without updating memory + // This is the BUG - Instance B should still have the policy in memory + + if err != nil { + t.Logf("Expected: Instance B got error from adapter: %v", err) + } + + // Instance B should have the policy in its in-memory model even if adapter failed + // because the policy already exists in the database (added by Instance A) + hasPolicy, _ = enforcerB.HasPolicy("alice", "data1", "read") + if !hasPolicy { + t.Fatal("BUG: Instance B should have the policy in memory even if adapter fails with unique constraint") + } +} + +// TestWatcherNotifyBatchWithUniqueConstraint tests the batch version. +func TestWatcherNotifyBatchWithUniqueConstraint(t *testing.T) { + // Instance A - the one that originally adds the policies + adapterA := NewMockAdapterWithUniqueConstraint() + enforcerA, _ := NewEnforcer("examples/rbac_model.conf", adapterA) + enforcerA.EnableAutoSave(true) + + // Instance B - another instance that receives the notification + adapterB := &MockAdapterWithUniqueConstraint{ + alreadyAdded: adapterA.alreadyAdded, // Share the same "database" + } + enforcerB, _ := NewEnforcer("examples/rbac_model.conf", adapterB) + enforcerB.EnableAutoSave(true) + + // Instance A adds policies successfully + rules := [][]string{ + {"alice", "data1", "read"}, + {"bob", "data2", "write"}, + } + ok, err := enforcerA.AddPolicies(rules) + if err != nil { + t.Fatalf("Instance A should add policies successfully: %v", err) + } + if !ok { + t.Fatal("Instance A should return true when adding new policies") + } + + // Verify Instance A has the policies in memory + hasPolicy, _ := enforcerA.HasPolicy("alice", "data1", "read") + if !hasPolicy { + t.Fatal("Instance A should have first policy in memory") + } + hasPolicy, _ = enforcerA.HasPolicy("bob", "data2", "write") + if !hasPolicy { + t.Fatal("Instance A should have second policy in memory") + } + + // Instance B receives notification and tries to add the same policies + _, err = enforcerB.SelfAddPolicies("p", "p", rules) + + if err != nil { + t.Logf("Expected: Instance B got error from adapter: %v", err) + } + + // Instance B should have the policies in its in-memory model even if adapter failed + hasPolicy, _ = enforcerB.HasPolicy("alice", "data1", "read") + if !hasPolicy { + t.Fatal("Instance B should have first policy in memory even if adapter fails") + } + hasPolicy, _ = enforcerB.HasPolicy("bob", "data2", "write") + if !hasPolicy { + t.Fatal("Instance B should have second policy in memory even if adapter fails") + } +}