Skip to content

Commit 53da062

Browse files
Copilothsluoyz
andcommitted
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>
1 parent 25a03a3 commit 53da062

File tree

2 files changed

+71
-0
lines changed

2 files changed

+71
-0
lines changed

src/main/java/org/casbin/jcasbin/rbac/DefaultRoleManager.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,12 @@ public void clear() {
171171
public void addLink(String name1, String name2, String... domain) {
172172
Role user = getRole(name1);
173173
Role role = getRole(name2);
174+
175+
// Check if the link already exists to avoid breaking idempotency on rollback
176+
if (user.roles.containsKey(name2)) {
177+
return;
178+
}
179+
174180
user.addRole(role);
175181

176182
// If detector is set, check for cycles after adding the link

src/test/java/org/casbin/jcasbin/main/DetectorTest.java

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,4 +323,69 @@ public void testDiamondStructure() {
323323
e.getMessage().contains("Cycle detected"));
324324
}
325325
}
326+
327+
@Test
328+
public void testIdempotencyWithExistingLink() {
329+
// Test that adding an existing link multiple times is idempotent
330+
// and doesn't break when detector is enabled
331+
DefaultRoleManager rm = new DefaultRoleManager(10);
332+
Detector detector = new DefaultDetector();
333+
rm.setDetector(detector);
334+
335+
// Add initial valid links
336+
rm.addLink("A", "B");
337+
rm.addLink("B", "C");
338+
339+
// Verify initial state
340+
assertTrue("A should have link to B", rm.hasLink("A", "B"));
341+
assertTrue("A should have link to C", rm.hasLink("A", "C"));
342+
343+
// Add the same link again (should be idempotent)
344+
rm.addLink("A", "B");
345+
346+
// Verify the link still exists
347+
assertTrue("A should still have link to B after re-adding", rm.hasLink("A", "B"));
348+
assertTrue("A should still have link to C after re-adding", rm.hasLink("A", "C"));
349+
350+
// Try to create a cycle with an existing link in the path
351+
rm.addLink("C", "D");
352+
assertTrue("C should have link to D", rm.hasLink("C", "D"));
353+
354+
// Re-add an existing link (A->B) should still be idempotent
355+
rm.addLink("A", "B");
356+
357+
// Verify all links still exist
358+
assertTrue("A should still have link to B", rm.hasLink("A", "B"));
359+
assertTrue("B should still have link to C", rm.hasLink("B", "C"));
360+
assertTrue("C should still have link to D", rm.hasLink("C", "D"));
361+
}
362+
363+
@Test
364+
public void testIdempotencyDoesNotPreventCycleDetection() {
365+
// Test that the idempotency check doesn't interfere with cycle detection
366+
// when trying to add a new link that would create a cycle
367+
DefaultRoleManager rm = new DefaultRoleManager(10);
368+
Detector detector = new DefaultDetector();
369+
rm.setDetector(detector);
370+
371+
// Create a valid chain
372+
rm.addLink("A", "B");
373+
rm.addLink("B", "C");
374+
375+
// Try to create a cycle (new link, not existing)
376+
try {
377+
rm.addLink("C", "A");
378+
fail("Expected IllegalArgumentException due to cycle");
379+
} catch (IllegalArgumentException e) {
380+
assertTrue("Exception message should contain 'Cycle detected'",
381+
e.getMessage().contains("Cycle detected"));
382+
}
383+
384+
// Verify the cycle was not added
385+
assertFalse("C should not have link to A", rm.hasLink("C", "A"));
386+
387+
// Verify existing links are intact
388+
assertTrue("A should still have link to B", rm.hasLink("A", "B"));
389+
assertTrue("B should still have link to C", rm.hasLink("B", "C"));
390+
}
326391
}

0 commit comments

Comments
 (0)