From 37c3e1f6fb19c174ceae165a2ba1b1f769761579 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 28 Dec 2025 18:06:19 +0000 Subject: [PATCH 1/7] Initial plan From e5dc0128de4ecebb1d9d4096dbe0f664769453e4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 28 Dec 2025 18:11:47 +0000 Subject: [PATCH 2/7] feat(core): integrate Detector into DefaultRoleManager with rollback support - Add optional Detector field to DefaultRoleManager with setter method - Modify addLink to call detector.check() after adding inheritance link - Implement automatic rollback on cycle detection - Throw IllegalArgumentException with error description on cycle detection - Create comprehensive DetectorTest.java with 12 test cases - All tests pass including existing RoleManagerUnitTest Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- .../jcasbin/rbac/DefaultRoleManager.java | 22 ++ .../org/casbin/jcasbin/main/DetectorTest.java | 324 ++++++++++++++++++ 2 files changed, 346 insertions(+) create mode 100644 src/test/java/org/casbin/jcasbin/main/DetectorTest.java diff --git a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java index 29ad56a2..d96b7d71 100644 --- a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java +++ b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java @@ -14,6 +14,7 @@ package org.casbin.jcasbin.rbac; +import org.casbin.jcasbin.detector.Detector; import org.casbin.jcasbin.util.SyncedLRUCache; import org.casbin.jcasbin.util.Util; @@ -27,6 +28,7 @@ public class DefaultRoleManager implements RoleManager { BiPredicate matchingFunc; private SyncedLRUCache matchingFuncCache; + private Detector detector; /** * DefaultRoleManager is the constructor for creating an instance of the default RoleManager @@ -81,6 +83,15 @@ public void addMatchingFunc(String name, BiPredicate matchingFun public void addDomainMatchingFunc(String name, BiPredicate domainMatchingFunc) { } + /** + * setDetector sets the detector for cycle detection in role inheritance. + * + * @param detector the detector instance to use for cycle detection. + */ + public void setDetector(Detector detector) { + this.detector = detector; + } + private void rebuild() { Map roles = new HashMap<>(this.allRoles); this.clear(); @@ -161,6 +172,17 @@ public void addLink(String name1, String name2, String... domain) { Role user = getRole(name1); Role role = getRole(name2); user.addRole(role); + + // If detector is set, check for cycles after adding the link + if (detector != null) { + String errorMsg = detector.check(this); + if (errorMsg != null && !errorMsg.isEmpty()) { + // Rollback the operation + user.removeRole(role); + // Throw exception with the error description + throw new IllegalArgumentException(errorMsg); + } + } } /** diff --git a/src/test/java/org/casbin/jcasbin/main/DetectorTest.java b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java new file mode 100644 index 00000000..bd2e1f30 --- /dev/null +++ b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java @@ -0,0 +1,324 @@ +// Copyright 2025 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 org.casbin.jcasbin.main; + +import org.casbin.jcasbin.detector.DefaultDetector; +import org.casbin.jcasbin.detector.Detector; +import org.casbin.jcasbin.rbac.DefaultRoleManager; +import org.junit.Test; + +import static org.junit.Assert.*; + +/** + * Unit tests for Detector integration with DefaultRoleManager. + * Tests cycle detection without depending on Enforcer. + */ +public class DetectorTest { + + @Test + public void testValidInheritanceChainNoException() { + // Construct a DefaultRoleManager with detector + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Add valid inheritance chains: u1 -> g1 -> g2 -> g3 + rm.addLink("u1", "g1"); + rm.addLink("g1", "g2"); + rm.addLink("g2", "g3"); + + // Verify no exception was thrown and the links exist + assertTrue("u1 should have link to g1", rm.hasLink("u1", "g1")); + assertTrue("u1 should have link to g2", rm.hasLink("u1", "g2")); + assertTrue("u1 should have link to g3", rm.hasLink("u1", "g3")); + } + + @Test + public void testMultipleBranchesNoException() { + // Create a more complex valid hierarchy + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Create tree structure: + // g3 + // / \ + // g1 g2 + // / \ | + // u1 u2 u3 + rm.addLink("u1", "g1"); + rm.addLink("u2", "g1"); + rm.addLink("u3", "g2"); + rm.addLink("g1", "g3"); + rm.addLink("g2", "g3"); + + // Verify all links work correctly + assertTrue("u1 should have link to g3", rm.hasLink("u1", "g3")); + assertTrue("u2 should have link to g3", rm.hasLink("u2", "g3")); + assertTrue("u3 should have link to g3", rm.hasLink("u3", "g3")); + } + + @Test + public void testCycleDetectionThreeNodes() { + // Test cycle A -> B -> C -> A + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Add first two valid links + rm.addLink("A", "B"); + rm.addLink("B", "C"); + + // Adding the third link should create a cycle and throw exception + try { + rm.addLink("C", "A"); + fail("Expected IllegalArgumentException due to cycle detection"); + } catch (IllegalArgumentException e) { + // Expected exception + assertTrue("Exception message should contain 'Cycle detected'", + e.getMessage().contains("Cycle detected")); + } + + // Verify the illegal link was rolled back + assertFalse("C should not have link to A after rollback", + rm.hasLink("C", "A")); + + // Verify the previous valid links still exist + assertTrue("A should still have link to B", rm.hasLink("A", "B")); + assertTrue("B should still have link to C", rm.hasLink("B", "C")); + } + + @Test + public void testSelfLoopDetection() { + // Test self-loop: A -> A + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + try { + rm.addLink("A", "A"); + fail("Expected IllegalArgumentException due to self-loop"); + } catch (IllegalArgumentException e) { + // Expected exception + assertTrue("Exception message should contain 'Cycle detected'", + e.getMessage().contains("Cycle detected")); + } + + // Verify the self-loop was not added by checking internal state + // Since hasLink("A", "A") always returns true by design (reflexivity), + // we verify by checking that A has no roles + assertEquals("A should have no parent roles after rollback", + 0, rm.getRoles("A").size()); + } + + @Test + public void testTwoNodeCycleDetection() { + // Test two-node cycle: A -> B -> A + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + rm.addLink("A", "B"); + + try { + rm.addLink("B", "A"); + fail("Expected IllegalArgumentException due to cycle"); + } catch (IllegalArgumentException e) { + // Expected exception + assertTrue("Exception message should contain 'Cycle detected'", + e.getMessage().contains("Cycle detected")); + } + + // Verify rollback + assertTrue("A should still have link to B", rm.hasLink("A", "B")); + assertFalse("B should not have link to A after rollback", + rm.hasLink("B", "A")); + } + + @Test + public void testComplexGraphCycleDetection() { + // Test complex graph with cycle + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Build a complex structure + rm.addLink("u1", "g1"); + rm.addLink("u2", "g1"); + rm.addLink("g1", "g2"); + rm.addLink("g2", "g3"); + rm.addLink("u3", "g3"); + + // Try to create a cycle: g3 -> g1 + try { + rm.addLink("g3", "g1"); + fail("Expected IllegalArgumentException due to cycle"); + } catch (IllegalArgumentException e) { + // Expected exception + assertTrue("Exception message should contain 'Cycle detected'", + e.getMessage().contains("Cycle detected")); + } + + // Verify the illegal link was rolled back + assertFalse("g3 should not have link to g1 after rollback", + rm.hasLink("g3", "g1")); + + // Verify existing structure is intact + assertTrue("u1 should still reach g3", rm.hasLink("u1", "g3")); + assertTrue("u2 should still reach g3", rm.hasLink("u2", "g3")); + } + + @Test + public void testStateAfterRollback() { + // Test that after rollback, the state is consistent + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Create chain: A -> B -> C + rm.addLink("A", "B"); + rm.addLink("B", "C"); + + // Try to create cycle + try { + rm.addLink("C", "A"); + } catch (IllegalArgumentException e) { + // Expected + } + + // Add a new valid link should work + rm.addLink("D", "A"); + assertTrue("D should have link to A", rm.hasLink("D", "A")); + assertTrue("D should reach C through A->B->C", rm.hasLink("D", "C")); + + // The rolled-back link should still not exist + assertFalse("C should still not have link to A", rm.hasLink("C", "A")); + } + + @Test + public void testDetectorCanBeSetToNull() { + // Test that detector can be set to null (disabled) + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + rm.addLink("A", "B"); + + // Disable detector + rm.setDetector(null); + + // Now cycles should be allowed + rm.addLink("B", "A"); + + // Both links should exist (no cycle detection) + assertTrue("A should have link to B", rm.hasLink("A", "B")); + assertTrue("B should have link to A", rm.hasLink("B", "A")); + } + + @Test + public void testNoDetectorNoCycleCheck() { + // Test that without detector, cycles are not detected + DefaultRoleManager rm = new DefaultRoleManager(10); + // Don't set detector + + // Create a cycle: A -> B -> A + rm.addLink("A", "B"); + rm.addLink("B", "A"); + + // Should not throw exception + assertTrue("A should have link to B", rm.hasLink("A", "B")); + assertTrue("B should have link to A", rm.hasLink("B", "A")); + } + + @Test + public void testMultipleDisconnectedComponents() { + // Test with multiple disconnected valid components + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Component 1: u1 -> g1 -> g2 + rm.addLink("u1", "g1"); + rm.addLink("g1", "g2"); + + // Component 2: u2 -> g3 -> g4 + rm.addLink("u2", "g3"); + rm.addLink("g3", "g4"); + + // Verify all links work + assertTrue("u1 should reach g2", rm.hasLink("u1", "g2")); + assertTrue("u2 should reach g4", rm.hasLink("u2", "g4")); + + // Components should be independent + assertFalse("u1 should not reach g4", rm.hasLink("u1", "g4")); + assertFalse("u2 should not reach g2", rm.hasLink("u2", "g2")); + } + + @Test + public void testLongChainNoCycle() { + // Test a long chain without cycles + DefaultRoleManager rm = new DefaultRoleManager(100); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Create chain: r0 -> r1 -> r2 -> ... -> r9 + for (int i = 0; i < 9; i++) { + rm.addLink("r" + i, "r" + (i + 1)); + } + + // Verify chain works + assertTrue("r0 should reach r9", rm.hasLink("r0", "r9")); + assertTrue("r5 should reach r9", rm.hasLink("r5", "r9")); + + // Adding cycle should fail + try { + rm.addLink("r9", "r0"); + fail("Expected IllegalArgumentException due to cycle"); + } catch (IllegalArgumentException e) { + assertTrue("Exception message should contain 'Cycle detected'", + e.getMessage().contains("Cycle detected")); + } + } + + @Test + public void testDiamondStructure() { + // Test diamond structure (common pattern in role hierarchies) + // admin + // / \ + // editor viewer + // \ / + // user + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + rm.addLink("user", "editor"); + rm.addLink("user", "viewer"); + rm.addLink("editor", "admin"); + rm.addLink("viewer", "admin"); + + // Verify the structure + assertTrue("user should reach admin through editor", rm.hasLink("user", "admin")); + + // Try to create a cycle + try { + rm.addLink("admin", "user"); + fail("Expected IllegalArgumentException due to cycle"); + } catch (IllegalArgumentException e) { + assertTrue("Exception message should contain 'Cycle detected'", + e.getMessage().contains("Cycle detected")); + } + } +} From 25a03a3beeda93cd1dc9f35fe1d5990d9918005a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 28 Dec 2025 18:13:36 +0000 Subject: [PATCH 3/7] refactor(test): replace magic number with named constant in DetectorTest Replace hardcoded value 9 with CHAIN_LENGTH constant for better code clarity Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- .../java/org/casbin/jcasbin/main/DetectorTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/casbin/jcasbin/main/DetectorTest.java b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java index bd2e1f30..940e3002 100644 --- a/src/test/java/org/casbin/jcasbin/main/DetectorTest.java +++ b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java @@ -273,18 +273,20 @@ public void testLongChainNoCycle() { Detector detector = new DefaultDetector(); rm.setDetector(detector); + final int CHAIN_LENGTH = 10; + // Create chain: r0 -> r1 -> r2 -> ... -> r9 - for (int i = 0; i < 9; i++) { + for (int i = 0; i < CHAIN_LENGTH - 1; i++) { rm.addLink("r" + i, "r" + (i + 1)); } // Verify chain works - assertTrue("r0 should reach r9", rm.hasLink("r0", "r9")); - assertTrue("r5 should reach r9", rm.hasLink("r5", "r9")); + assertTrue("r0 should reach r9", rm.hasLink("r0", "r" + (CHAIN_LENGTH - 1))); + assertTrue("r5 should reach r9", rm.hasLink("r5", "r" + (CHAIN_LENGTH - 1))); // Adding cycle should fail try { - rm.addLink("r9", "r0"); + rm.addLink("r" + (CHAIN_LENGTH - 1), "r0"); fail("Expected IllegalArgumentException due to cycle"); } catch (IllegalArgumentException e) { assertTrue("Exception message should contain 'Cycle detected'", From 53da062eb7413de39c216222192956be666a2635 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 29 Dec 2025 13:14:45 +0000 Subject: [PATCH 4/7] fix(core): prevent data loss by checking existing links before rollback Add idempotency check in addLink to prevent deleting pre-existing valid links when detector fails. This ensures addLink is truly idempotent even with cycle detection enabled. - Check if link already exists before adding (user.roles.containsKey(name2)) - Return early if link exists, avoiding unnecessary processing and rollback issues - Add comprehensive tests for idempotency scenarios Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- .../jcasbin/rbac/DefaultRoleManager.java | 6 ++ .../org/casbin/jcasbin/main/DetectorTest.java | 65 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java index d96b7d71..16d4abde 100644 --- a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java +++ b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java @@ -171,6 +171,12 @@ public void clear() { public void addLink(String name1, String name2, String... domain) { Role user = getRole(name1); Role role = getRole(name2); + + // Check if the link already exists to avoid breaking idempotency on rollback + if (user.roles.containsKey(name2)) { + return; + } + user.addRole(role); // If detector is set, check for cycles after adding the link diff --git a/src/test/java/org/casbin/jcasbin/main/DetectorTest.java b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java index 940e3002..6eb4ba5d 100644 --- a/src/test/java/org/casbin/jcasbin/main/DetectorTest.java +++ b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java @@ -323,4 +323,69 @@ public void testDiamondStructure() { e.getMessage().contains("Cycle detected")); } } + + @Test + public void testIdempotencyWithExistingLink() { + // Test that adding an existing link multiple times is idempotent + // and doesn't break when detector is enabled + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Add initial valid links + rm.addLink("A", "B"); + rm.addLink("B", "C"); + + // Verify initial state + assertTrue("A should have link to B", rm.hasLink("A", "B")); + assertTrue("A should have link to C", rm.hasLink("A", "C")); + + // Add the same link again (should be idempotent) + rm.addLink("A", "B"); + + // Verify the link still exists + assertTrue("A should still have link to B after re-adding", rm.hasLink("A", "B")); + assertTrue("A should still have link to C after re-adding", rm.hasLink("A", "C")); + + // Try to create a cycle with an existing link in the path + rm.addLink("C", "D"); + assertTrue("C should have link to D", rm.hasLink("C", "D")); + + // Re-add an existing link (A->B) should still be idempotent + rm.addLink("A", "B"); + + // Verify all links still exist + assertTrue("A should still have link to B", rm.hasLink("A", "B")); + assertTrue("B should still have link to C", rm.hasLink("B", "C")); + assertTrue("C should still have link to D", rm.hasLink("C", "D")); + } + + @Test + public void testIdempotencyDoesNotPreventCycleDetection() { + // Test that the idempotency check doesn't interfere with cycle detection + // when trying to add a new link that would create a cycle + DefaultRoleManager rm = new DefaultRoleManager(10); + Detector detector = new DefaultDetector(); + rm.setDetector(detector); + + // Create a valid chain + rm.addLink("A", "B"); + rm.addLink("B", "C"); + + // Try to create a cycle (new link, not existing) + try { + rm.addLink("C", "A"); + fail("Expected IllegalArgumentException due to cycle"); + } catch (IllegalArgumentException e) { + assertTrue("Exception message should contain 'Cycle detected'", + e.getMessage().contains("Cycle detected")); + } + + // Verify the cycle was not added + assertFalse("C should not have link to A", rm.hasLink("C", "A")); + + // Verify existing links are intact + assertTrue("A should still have link to B", rm.hasLink("A", "B")); + assertTrue("B should still have link to C", rm.hasLink("B", "C")); + } } From 4ecff156fda02fcce3e08f46c68782a069aa9bb6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 16:46:14 +0000 Subject: [PATCH 5/7] fix(core): add synchronization to addLink to prevent race conditions Add synchronized keyword to addLink method to ensure thread safety. The method now performs compound operations (check -> add -> check -> rollback) which could lead to race conditions without proper synchronization. This aligns with the existing synchronized hasLink method. Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java index 16d4abde..3fa8eabb 100644 --- a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java +++ b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java @@ -168,7 +168,7 @@ public void clear() { * inherits role: name2. domain is a prefix to the roles. */ @Override - public void addLink(String name1, String name2, String... domain) { + public synchronized void addLink(String name1, String name2, String... domain) { Role user = getRole(name1); Role role = getRole(name2); From b1680818227c84a21878e142f070fa7c572acaf0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 17:08:28 +0000 Subject: [PATCH 6/7] perf(core): optimize rebuild() to avoid O(N^2) complexity Temporarily disable detector during rebuild loop to prevent repeated cycle checks on each addLink call. Perform a single check at the end. This reduces rebuild() complexity from O(N^2) to O(N) when detector is enabled. - Save original detector before rebuild - Set detector to null during addLink loop - Restore detector and perform single check after loop - Properly restore detector in exception handler Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- .../jcasbin/rbac/DefaultRoleManager.java | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java index 3fa8eabb..bafda120 100644 --- a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java +++ b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java @@ -95,9 +95,29 @@ public void setDetector(Detector detector) { private void rebuild() { Map roles = new HashMap<>(this.allRoles); this.clear(); - roles.values().forEach(user -> { - user.getAllRoles().keySet().forEach(roleName -> addLink(user.getName(), roleName, DEFAULT_DOMAIN)); - }); + + // Temporarily disable detector to avoid O(N^2) complexity during rebuild + Detector originalDetector = this.detector; + this.detector = null; + + try { + roles.values().forEach(user -> { + user.getAllRoles().keySet().forEach(roleName -> addLink(user.getName(), roleName, DEFAULT_DOMAIN)); + }); + + // Restore detector and perform single check at the end + this.detector = originalDetector; + if (this.detector != null) { + String errorMsg = this.detector.check(this); + if (errorMsg != null && !errorMsg.isEmpty()) { + throw new IllegalArgumentException(errorMsg); + } + } + } catch (IllegalArgumentException e) { + // Restore detector before re-throwing + this.detector = originalDetector; + throw e; + } } boolean match(String str, String pattern) { From 25d842a6f1733f9b9226e250ea7d5c18153043b8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 2 Jan 2026 17:48:06 +0000 Subject: [PATCH 7/7] refactor(core): move cycle detection from real-time to post-load Change cycle detection strategy to improve bulk loading performance: - Remove automatic cycle check from addLink() method - Add public checkCycles() method for manual cycle detection - Simplify rebuild() to just call checkCycles() at the end - Override loadPolicy() in ManagementEnforcer to call checkCycles() after loading - Update all DetectorTest cases to call checkCycles() explicitly This changes the detection from O(N^2) real-time checks to O(N) post-load check, significantly improving performance during bulk policy loading while maintaining the same cycle detection capability. Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- .../jcasbin/main/ManagementEnforcer.java | 20 +++ .../jcasbin/rbac/DefaultRoleManager.java | 54 +++---- .../org/casbin/jcasbin/main/DetectorTest.java | 150 +++++++++--------- 3 files changed, 115 insertions(+), 109 deletions(-) diff --git a/src/main/java/org/casbin/jcasbin/main/ManagementEnforcer.java b/src/main/java/org/casbin/jcasbin/main/ManagementEnforcer.java index 1a21dded..b3c6e930 100644 --- a/src/main/java/org/casbin/jcasbin/main/ManagementEnforcer.java +++ b/src/main/java/org/casbin/jcasbin/main/ManagementEnforcer.java @@ -16,6 +16,8 @@ import org.casbin.jcasbin.effect.Effect; import org.casbin.jcasbin.model.Assertion; +import org.casbin.jcasbin.rbac.DefaultRoleManager; +import org.casbin.jcasbin.rbac.RoleManager; import org.casbin.jcasbin.util.Util; import org.casbin.jcasbin.util.function.CustomFunction; @@ -26,6 +28,24 @@ * ManagementEnforcer = InternalEnforcer + Management API. */ public class ManagementEnforcer extends InternalEnforcer { + + /** + * loadPolicy reloads the policy from file/database. + * Overridden to add cycle detection check after policy loading. + */ + @Override + public void loadPolicy() { + super.loadPolicy(); + + // Check for cycles in all role managers after policy is loaded + for (Map.Entry entry : rmMap.entrySet()) { + RoleManager rm = entry.getValue(); + if (rm instanceof DefaultRoleManager) { + ((DefaultRoleManager) rm).checkCycles(); + } + } + } + /** * getAllSubjects gets the list of subjects that show up in the current policy. * diff --git a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java index bafda120..f8114089 100644 --- a/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java +++ b/src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java @@ -92,32 +92,31 @@ public void setDetector(Detector detector) { this.detector = detector; } + /** + * checkCycles performs a one-time cycle detection check on the current role graph. + * This should be called after bulk policy loading to detect any cycles. + * + * @throws IllegalArgumentException if a cycle is detected in the role inheritance graph. + */ + public void checkCycles() { + if (this.detector != null) { + String errorMsg = this.detector.check(this); + if (errorMsg != null && !errorMsg.isEmpty()) { + throw new IllegalArgumentException(errorMsg); + } + } + } + private void rebuild() { Map roles = new HashMap<>(this.allRoles); this.clear(); - // Temporarily disable detector to avoid O(N^2) complexity during rebuild - Detector originalDetector = this.detector; - this.detector = null; + roles.values().forEach(user -> { + user.getAllRoles().keySet().forEach(roleName -> addLink(user.getName(), roleName, DEFAULT_DOMAIN)); + }); - try { - roles.values().forEach(user -> { - user.getAllRoles().keySet().forEach(roleName -> addLink(user.getName(), roleName, DEFAULT_DOMAIN)); - }); - - // Restore detector and perform single check at the end - this.detector = originalDetector; - if (this.detector != null) { - String errorMsg = this.detector.check(this); - if (errorMsg != null && !errorMsg.isEmpty()) { - throw new IllegalArgumentException(errorMsg); - } - } - } catch (IllegalArgumentException e) { - // Restore detector before re-throwing - this.detector = originalDetector; - throw e; - } + // Perform cycle check at the end + checkCycles(); } boolean match(String str, String pattern) { @@ -192,23 +191,12 @@ public synchronized void addLink(String name1, String name2, String... domain) { Role user = getRole(name1); Role role = getRole(name2); - // Check if the link already exists to avoid breaking idempotency on rollback + // Check if the link already exists to maintain idempotency if (user.roles.containsKey(name2)) { return; } user.addRole(role); - - // If detector is set, check for cycles after adding the link - if (detector != null) { - String errorMsg = detector.check(this); - if (errorMsg != null && !errorMsg.isEmpty()) { - // Rollback the operation - user.removeRole(role); - // Throw exception with the error description - throw new IllegalArgumentException(errorMsg); - } - } } /** diff --git a/src/test/java/org/casbin/jcasbin/main/DetectorTest.java b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java index 6eb4ba5d..1f81bc5e 100644 --- a/src/test/java/org/casbin/jcasbin/main/DetectorTest.java +++ b/src/test/java/org/casbin/jcasbin/main/DetectorTest.java @@ -43,6 +43,9 @@ public void testValidInheritanceChainNoException() { assertTrue("u1 should have link to g1", rm.hasLink("u1", "g1")); assertTrue("u1 should have link to g2", rm.hasLink("u1", "g2")); assertTrue("u1 should have link to g3", rm.hasLink("u1", "g3")); + + // checkCycles should not throw exception for valid hierarchy + rm.checkCycles(); } @Test @@ -68,6 +71,9 @@ public void testMultipleBranchesNoException() { assertTrue("u1 should have link to g3", rm.hasLink("u1", "g3")); assertTrue("u2 should have link to g3", rm.hasLink("u2", "g3")); assertTrue("u3 should have link to g3", rm.hasLink("u3", "g3")); + + // checkCycles should not throw exception for valid hierarchy + rm.checkCycles(); } @Test @@ -77,27 +83,23 @@ public void testCycleDetectionThreeNodes() { Detector detector = new DefaultDetector(); rm.setDetector(detector); - // Add first two valid links + // Add all links including the one that creates a cycle rm.addLink("A", "B"); rm.addLink("B", "C"); + rm.addLink("C", "A"); // Creates a cycle, but addLink doesn't throw + + // Verify the link was added + assertTrue("C should have link to A", rm.hasLink("C", "A")); - // Adding the third link should create a cycle and throw exception + // checkCycles should throw exception when called try { - rm.addLink("C", "A"); + rm.checkCycles(); fail("Expected IllegalArgumentException due to cycle detection"); } catch (IllegalArgumentException e) { // Expected exception assertTrue("Exception message should contain 'Cycle detected'", e.getMessage().contains("Cycle detected")); } - - // Verify the illegal link was rolled back - assertFalse("C should not have link to A after rollback", - rm.hasLink("C", "A")); - - // Verify the previous valid links still exist - assertTrue("A should still have link to B", rm.hasLink("A", "B")); - assertTrue("B should still have link to C", rm.hasLink("B", "C")); } @Test @@ -107,20 +109,18 @@ public void testSelfLoopDetection() { Detector detector = new DefaultDetector(); rm.setDetector(detector); + // addLink doesn't throw, just adds the link + rm.addLink("A", "A"); + + // checkCycles should throw exception try { - rm.addLink("A", "A"); + rm.checkCycles(); fail("Expected IllegalArgumentException due to self-loop"); } catch (IllegalArgumentException e) { // Expected exception assertTrue("Exception message should contain 'Cycle detected'", e.getMessage().contains("Cycle detected")); } - - // Verify the self-loop was not added by checking internal state - // Since hasLink("A", "A") always returns true by design (reflexivity), - // we verify by checking that A has no roles - assertEquals("A should have no parent roles after rollback", - 0, rm.getRoles("A").size()); } @Test @@ -131,20 +131,21 @@ public void testTwoNodeCycleDetection() { rm.setDetector(detector); rm.addLink("A", "B"); + rm.addLink("B", "A"); // Creates a cycle, but addLink doesn't throw + + // Verify both links were added + assertTrue("A should have link to B", rm.hasLink("A", "B")); + assertTrue("B should have link to A", rm.hasLink("B", "A")); + // checkCycles should throw exception try { - rm.addLink("B", "A"); + rm.checkCycles(); fail("Expected IllegalArgumentException due to cycle"); } catch (IllegalArgumentException e) { // Expected exception assertTrue("Exception message should contain 'Cycle detected'", e.getMessage().contains("Cycle detected")); } - - // Verify rollback - assertTrue("A should still have link to B", rm.hasLink("A", "B")); - assertFalse("B should not have link to A after rollback", - rm.hasLink("B", "A")); } @Test @@ -154,16 +155,20 @@ public void testComplexGraphCycleDetection() { Detector detector = new DefaultDetector(); rm.setDetector(detector); - // Build a complex structure + // Build a complex structure with a cycle rm.addLink("u1", "g1"); rm.addLink("u2", "g1"); rm.addLink("g1", "g2"); rm.addLink("g2", "g3"); rm.addLink("u3", "g3"); + rm.addLink("g3", "g1"); // Creates a cycle + + // Verify the link was added + assertTrue("g3 should have link to g1", rm.hasLink("g3", "g1")); - // Try to create a cycle: g3 -> g1 + // checkCycles should throw exception try { - rm.addLink("g3", "g1"); + rm.checkCycles(); fail("Expected IllegalArgumentException due to cycle"); } catch (IllegalArgumentException e) { // Expected exception @@ -171,42 +176,11 @@ public void testComplexGraphCycleDetection() { e.getMessage().contains("Cycle detected")); } - // Verify the illegal link was rolled back - assertFalse("g3 should not have link to g1 after rollback", - rm.hasLink("g3", "g1")); - // Verify existing structure is intact assertTrue("u1 should still reach g3", rm.hasLink("u1", "g3")); assertTrue("u2 should still reach g3", rm.hasLink("u2", "g3")); } - @Test - public void testStateAfterRollback() { - // Test that after rollback, the state is consistent - DefaultRoleManager rm = new DefaultRoleManager(10); - Detector detector = new DefaultDetector(); - rm.setDetector(detector); - - // Create chain: A -> B -> C - rm.addLink("A", "B"); - rm.addLink("B", "C"); - - // Try to create cycle - try { - rm.addLink("C", "A"); - } catch (IllegalArgumentException e) { - // Expected - } - - // Add a new valid link should work - rm.addLink("D", "A"); - assertTrue("D should have link to A", rm.hasLink("D", "A")); - assertTrue("D should reach C through A->B->C", rm.hasLink("D", "C")); - - // The rolled-back link should still not exist - assertFalse("C should still not have link to A", rm.hasLink("C", "A")); - } - @Test public void testDetectorCanBeSetToNull() { // Test that detector can be set to null (disabled) @@ -219,12 +193,15 @@ public void testDetectorCanBeSetToNull() { // Disable detector rm.setDetector(null); - // Now cycles should be allowed + // Now cycles can be created and checkCycles won't throw rm.addLink("B", "A"); - // Both links should exist (no cycle detection) + // Both links should exist assertTrue("A should have link to B", rm.hasLink("A", "B")); assertTrue("B should have link to A", rm.hasLink("B", "A")); + + // checkCycles should not throw when detector is null + rm.checkCycles(); // Should not throw } @Test @@ -240,6 +217,9 @@ public void testNoDetectorNoCycleCheck() { // Should not throw exception assertTrue("A should have link to B", rm.hasLink("A", "B")); assertTrue("B should have link to A", rm.hasLink("B", "A")); + + // checkCycles should not throw when detector is not set + rm.checkCycles(); // Should not throw } @Test @@ -264,6 +244,9 @@ public void testMultipleDisconnectedComponents() { // Components should be independent assertFalse("u1 should not reach g4", rm.hasLink("u1", "g4")); assertFalse("u2 should not reach g2", rm.hasLink("u2", "g2")); + + // checkCycles should not throw for valid disconnected components + rm.checkCycles(); } @Test @@ -284,9 +267,15 @@ public void testLongChainNoCycle() { assertTrue("r0 should reach r9", rm.hasLink("r0", "r" + (CHAIN_LENGTH - 1))); assertTrue("r5 should reach r9", rm.hasLink("r5", "r" + (CHAIN_LENGTH - 1))); - // Adding cycle should fail + // checkCycles should not throw for valid chain + rm.checkCycles(); + + // Add a link that creates a cycle + rm.addLink("r" + (CHAIN_LENGTH - 1), "r0"); + + // checkCycles should throw after adding cycle try { - rm.addLink("r" + (CHAIN_LENGTH - 1), "r0"); + rm.checkCycles(); fail("Expected IllegalArgumentException due to cycle"); } catch (IllegalArgumentException e) { assertTrue("Exception message should contain 'Cycle detected'", @@ -314,9 +303,15 @@ public void testDiamondStructure() { // Verify the structure assertTrue("user should reach admin through editor", rm.hasLink("user", "admin")); - // Try to create a cycle + // checkCycles should not throw for valid diamond structure + rm.checkCycles(); + + // Add a link that creates a cycle + rm.addLink("admin", "user"); + + // checkCycles should throw after adding cycle try { - rm.addLink("admin", "user"); + rm.checkCycles(); fail("Expected IllegalArgumentException due to cycle"); } catch (IllegalArgumentException e) { assertTrue("Exception message should contain 'Cycle detected'", @@ -327,7 +322,6 @@ public void testDiamondStructure() { @Test public void testIdempotencyWithExistingLink() { // Test that adding an existing link multiple times is idempotent - // and doesn't break when detector is enabled DefaultRoleManager rm = new DefaultRoleManager(10); Detector detector = new DefaultDetector(); rm.setDetector(detector); @@ -347,7 +341,10 @@ public void testIdempotencyWithExistingLink() { assertTrue("A should still have link to B after re-adding", rm.hasLink("A", "B")); assertTrue("A should still have link to C after re-adding", rm.hasLink("A", "C")); - // Try to create a cycle with an existing link in the path + // checkCycles should not throw for valid chain + rm.checkCycles(); + + // Add another link rm.addLink("C", "D"); assertTrue("C should have link to D", rm.hasLink("C", "D")); @@ -358,12 +355,14 @@ public void testIdempotencyWithExistingLink() { assertTrue("A should still have link to B", rm.hasLink("A", "B")); assertTrue("B should still have link to C", rm.hasLink("B", "C")); assertTrue("C should still have link to D", rm.hasLink("C", "D")); + + // checkCycles should still not throw + rm.checkCycles(); } @Test public void testIdempotencyDoesNotPreventCycleDetection() { // Test that the idempotency check doesn't interfere with cycle detection - // when trying to add a new link that would create a cycle DefaultRoleManager rm = new DefaultRoleManager(10); Detector detector = new DefaultDetector(); rm.setDetector(detector); @@ -372,20 +371,19 @@ public void testIdempotencyDoesNotPreventCycleDetection() { rm.addLink("A", "B"); rm.addLink("B", "C"); - // Try to create a cycle (new link, not existing) + // Add a link that creates a cycle + rm.addLink("C", "A"); + + // Verify the link was added + assertTrue("C should have link to A", rm.hasLink("C", "A")); + + // checkCycles should throw exception try { - rm.addLink("C", "A"); + rm.checkCycles(); fail("Expected IllegalArgumentException due to cycle"); } catch (IllegalArgumentException e) { assertTrue("Exception message should contain 'Cycle detected'", e.getMessage().contains("Cycle detected")); } - - // Verify the cycle was not added - assertFalse("C should not have link to A", rm.hasLink("C", "A")); - - // Verify existing links are intact - assertTrue("A should still have link to B", rm.hasLink("A", "B")); - assertTrue("B should still have link to C", rm.hasLink("B", "C")); } }