Skip to content

Add EffectConflictDetector for detecting policy effect conflicts#1668

Open
Copilot wants to merge 4 commits intomasterfrom
copilot/add-effect-conflict-detector
Open

Add EffectConflictDetector for detecting policy effect conflicts#1668
Copilot wants to merge 4 commits intomasterfrom
copilot/add-effect-conflict-detector

Conversation

Copy link
Contributor

Copilot AI commented Jan 8, 2026

Implements a new detector that identifies conflicts where a user has one effect (allow/deny) for an action while their role has the opposite effect for the same action.

Implementation

  • New ModelDetector interface: Extends detector framework to support detectors requiring both Model and RoleManager access
  • EffectConflictDetector: Detects explicit allow/deny conflicts between user-level and role-level policies
  • Updated RunDetections(): Handles both Detector and ModelDetector interfaces via type assertion

Design

The detector is opt-in rather than default. In Casbin, explicit user policies intentionally override role policies, so effect conflicts are often legitimate (e.g., denying a user an action their role allows). Users wanting strict validation can enable it:

e.SetDetectors([]detector.Detector{
    detector.NewDefaultDetector(),
    detector.NewEffectConflictDetector(),
})
err := e.RunDetections()  // Returns error on conflict

Example Conflict

p, alice, data2, write, deny
p, admin, data2, write, allow
g, alice, admin

The detector flags this as: user 'alice' has 'deny' effect for (data2, write), but role 'admin' has 'allow' effect for the same action

Testing

  • 11 test cases covering conflicts, non-conflicts, edge cases (multiple roles, default effects, nil validation)
  • Integration test verifying enforcer usage
  • No regressions in existing test suite
Original prompt

This section details on the original issue you should resolve

<issue_title>[feature] add second default detector: EffectConflictDetector</issue_title>
<issue_description>this detector needs to detect effect conflict. like alice is allowed to do something, but the role of alice is denied to do the same thing. so one allow and one deny. they are conflict.

note the deny here should be explicitly deny effect</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@hsluoyz hsluoyz marked this pull request as ready for review January 8, 2026 17:25
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI and others added 3 commits January 8, 2026 17:31
Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
- Extract policy key generation to helper function
- Extract conflict checking logic to reduce duplication
- Improve error message for Check() method
- Add constant for policy key separator

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Copilot AI changed the title [WIP] Add second default detector for effect conflict detection Add EffectConflictDetector for detecting policy effect conflicts Jan 8, 2026
Copilot AI requested a review from hsluoyz January 8, 2026 17:39
@hsluoyz
Copy link
Member

hsluoyz commented Jan 8, 2026

fix:

[golangci](https://github.com/casbin/casbin/actions/runs/20825958603/job/59828063024#step:4:31)
issues found
[golangci: detector/effect_conflict_detector.go#L82](https://github.com/casbin/casbin/pull/1668/files#annotation_43866264688)
error is not nil (line 79) but it returns nil (nilerr)
[golangci: detector/effect_conflict_detector.go#L128](https://github.com/casbin/casbin/pull/1668/files#annotation_43866264693)
`if subject == user` has complex nested blocks (complexity: 7) (nestif)
[golangci: enforcer_test.go#L808](https://github.com/casbin/casbin/pull/1668/files#annotation_43866264700)
File is not `goimports`-ed (goimports)
[golangci: detector/effect_conflict_detector_test.go#L28](https://github.com/casbin/casbin/pull/1668/files#annotation_43866264705)
File is not `goimports`-ed (goimports)
[golangci: detector/effect_conflict_detector.go#L43](https://github.com/casbin/casbin/pull/1668/files#annotation_43866264709)
File is not `goimports`-ed (goimports)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature] add second default detector: EffectConflictDetector

3 participants