Skip to content

Fix policy synchronization failure with database unique constraints in distributed watcher scenarios #1593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 5 commits into
base: master
Choose a base branch
from
+313 −50
Open

Fix policy synchronization failure with database unique constraints in distributed watcher scenarios #1593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e76ea0
Initial plan
Copilot Dec 7, 2025
e930335
Add test to reproduce unique constraint bug with Redis WatcherEX
Copilot Dec 7, 2025
3cef448
Fix Redis WatcherEX update failure with database unique constraints
Copilot Dec 7, 2025
d4d537e
Address code review feedback and fix linter issues
Copilot Dec 7, 2025
3a31759
Refactor to fix nestif linter warnings
Copilot Dec 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 49 additions & 11 deletions enforcer_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
48 changes: 20 additions & 28 deletions error_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
60 changes: 49 additions & 11 deletions internal_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down
Loading
Loading