Skip to content

Commit 61d6d1a

Browse files
swickenswicken
authored
fix(permissions): propagate role revocation through short-term permission cache (#35458)
## Summary - PermissionCacheImpl.clearCache() now flushes both primaryGroup **and** shortLivedGroup. Previously only primaryGroup was flushed, so cached doesUserHavePermission boolean decisions survived calls to this method (including the admin-UI "Flush Permission Cache" button, which delegates here via PermissionBitAPIImpl.clearCache). - RoleFactoryImpl.addRoleToUser / removeRoleFromUser now queue a post-commit flushShortTermCache via HibernateUtil.addCommitListener. The listener is tagged so bulk operations (e.g. delete-role cascading to N users, removeAllRolesFromUser) collapse to a single flush per transaction. Post-commit placement means a rolled-back transaction does not spuriously wipe the cache. Fixes dotCMS/private-issues#567. ## Coverage audit Every user-facing users_cms_roles mutation routes through RoleFactoryImpl.addRoleToUser / removeRoleFromUser (directly or transitively via RoleAPIImpl.delete -- per-user removeRoleFromUser, removeAllRolesFromUser, addRoleToUserWithoutEditUserValidation, and the SAML / user-resource helpers). The bulk SQL inside RoleFactoryImpl.delete is a safety-net that runs against an already-empty set after RoleAPIImpl.delete has iterated per-user removals. RoleFactoryImpl.addUserRole at user-creation time does not need invalidation (no cached decisions can exist for a brand-new user). RoleIntegrityChecker also rewrites role_id on users_cms_roles without any cache invalidation today (not just permission cache -- role cache is untouched there too). Out of scope for this PR; worth a separate ticket. ## Test plan - [x] Unit test PermissionCacheImplTest -- verifies clearCache flushes both groups and flushShortTermCache leaves primaryGroup alone. 3/3 passing locally. - [x] Integration test RoleRevocationPermissionCacheTest -- covers grant, revoke, and admin-UI-style clearCache scenarios against real RoleAPI / PermissionAPI with the short-term cache enabled. - [x] Manual end-to-end validation against a running stack. - [ ] Reviewer: ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=RoleRevocationPermissionCacheTest - [ ] Reviewer: regression check against PermissionAPITest, PermissionAPIIntegrationTest, RoleAPITest
1 parent 9ab3b3f commit 61d6d1a

4 files changed

Lines changed: 300 additions & 3 deletions

File tree

‎dotCMS/src/main/java/com/dotmarketing/business/PermissionCacheImpl.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,19 @@ public class PermissionCacheImpl extends PermissionCache {
3636
static final Lazy<Integer> EMPTY_PERMISSIONS_TTL =
3737
Lazy.of(() -> Config.getIntProperty("EMPTY_PERMISSIONS_TTL", 300)); // 5 minutes default
3838

39-
private DotCacheAdministrator cache;
39+
private final DotCacheAdministrator cache;
4040

4141
private final String primaryGroup = "PermissionCache";
4242
private final String shortLivedGroup = "PermissionShortLived";
4343
// region's name for the cache
4444
private final String[] groupNames = {primaryGroup,shortLivedGroup};
4545

4646
protected PermissionCacheImpl() {
47-
cache = CacheLocator.getCacheAdministrator();
47+
this(CacheLocator.getCacheAdministrator());
48+
}
49+
50+
PermissionCacheImpl(final DotCacheAdministrator cacheAdministrator) {
51+
this.cache = cacheAdministrator;
4852
}
4953

5054
/* (non-Javadoc)
@@ -92,8 +96,11 @@ protected List<Permission> getPermissionsFromCache(String key) {
9296
* @see com.dotmarketing.business.PermissionCache#clearCache()
9397
*/
9498
public void clearCache() {
95-
// clear the cache
99+
// Both groups must be flushed. shortLivedGroup caches boolean
100+
// doesUserHavePermission() decisions; leaving it here means role
101+
// revocations do not propagate until a full Flush All.
96102
cache.flushGroup(primaryGroup);
103+
cache.flushGroup(shortLivedGroup);
97104
}
98105

99106
/* (non-Javadoc)

‎dotCMS/src/main/java/com/dotmarketing/business/RoleFactoryImpl.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,7 @@ protected void addRoleToUser(Role role, User user) throws DotDataException {
283283
ur.setUserId(user.getUserId());
284284
HibernateUtil.save(ur);
285285
rc.remove(user.getUserId());
286+
flushPermissionShortTermCacheOnCommit();
286287

287288
}
288289

@@ -294,6 +295,19 @@ protected void removeRoleFromUser(Role role, User user) throws DotDataException
294295
dc.addParam(role.getId());
295296
dc.loadResult();
296297
rc.remove(user.getUserId());
298+
flushPermissionShortTermCacheOnCommit();
299+
}
300+
301+
/**
302+
* Queues a post-commit flush of PermissionCache.shortLivedGroup so that
303+
* cached doesUserHavePermission() decisions are re-evaluated after a
304+
* role<->user mutation. The tag collapses bulk operations (e.g. delete
305+
* role that touches N users) to a single flush per transaction.
306+
*/
307+
private void flushPermissionShortTermCacheOnCommit() {
308+
HibernateUtil.addCommitListener(
309+
"role-mutation-flush-permission-shortterm",
310+
() -> CacheLocator.getPermissionCache().flushShortTermCache());
297311
}
298312

299313
@Override

‎dotCMS/src/test/java/com/dotmarketing/business/PermissionCacheImplTest.java‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
package com.dotmarketing.business;
2+
3+
import static org.junit.Assert.assertNotNull;
4+
import static org.junit.Assert.assertNull;
5+
6+
import com.dotmarketing.beans.Permission;
7+
import com.dotmarketing.business.cache.provider.MockCacheAdministrator;
8+
import java.util.List;
9+
import org.junit.Before;
10+
import org.junit.Test;
11+
12+
/**
13+
* Unit tests for {@link PermissionCacheImpl}.
14+
*
15+
* Guards against regressions of https://github.com/dotCMS/private-issues/issues/567
16+
* where {@code clearCache()} did not flush {@code shortLivedGroup}, so the
17+
* admin-UI "Flush Permission Cache" button left cached permission decisions
18+
* in place and role revocation did not propagate.
19+
*
20+
* Exercises the cache administrator directly rather than going through
21+
* {@code doesUserHavePermission}/{@code putUserHavePermission}, because the
22+
* latter call {@code DbConnectionFactory.inTransaction()} which requires a
23+
* configured data source — out of scope for a pure unit test.
24+
*/
25+
public class PermissionCacheImplTest {
26+
27+
private static final String PRIMARY_GROUP = "PermissionCache";
28+
private static final String SHORT_LIVED_GROUP = "PermissionShortLived";
29+
30+
private MockCacheAdministrator mockCache;
31+
private PermissionCacheImpl permissionCache;
32+
33+
@Before
34+
public void setUp() {
35+
mockCache = new MockCacheAdministrator();
36+
permissionCache = new PermissionCacheImpl(mockCache);
37+
}
38+
39+
@Test
40+
public void clearCache_flushesPrimaryGroup() throws Exception {
41+
permissionCache.addToPermissionCache("k1", List.of(new Permission()));
42+
assertNotNull("Primary entry should be present before clearCache",
43+
mockCache.get(PRIMARY_GROUP + "k1", PRIMARY_GROUP));
44+
45+
permissionCache.clearCache();
46+
47+
assertNull("Primary entry should be gone after clearCache",
48+
mockCache.get(PRIMARY_GROUP + "k1", PRIMARY_GROUP));
49+
}
50+
51+
@Test
52+
public void clearCache_flushesShortLivedGroup() throws Exception {
53+
// Seed shortLivedGroup directly to avoid DbConnectionFactory init via
54+
// putUserHavePermission -> shortLivedKey.
55+
mockCache.put("short-k1", Boolean.TRUE, SHORT_LIVED_GROUP);
56+
assertNotNull("Short-lived entry should be present before clearCache",
57+
mockCache.get("short-k1", SHORT_LIVED_GROUP));
58+
59+
permissionCache.clearCache();
60+
61+
assertNull("Short-lived entry must be gone after clearCache — this is the"
62+
+ " fix for private-issues#567",
63+
mockCache.get("short-k1", SHORT_LIVED_GROUP));
64+
}
65+
66+
@Test
67+
public void flushShortTermCache_leavesPrimaryGroupUntouched() throws Exception {
68+
permissionCache.addToPermissionCache("primary-key", List.of(new Permission()));
69+
mockCache.put("short-k2", Boolean.TRUE, SHORT_LIVED_GROUP);
70+
71+
permissionCache.flushShortTermCache();
72+
73+
assertNotNull("primaryGroup must be untouched by flushShortTermCache()",
74+
mockCache.get(PRIMARY_GROUP + "primary-key", PRIMARY_GROUP));
75+
assertNull("shortLivedGroup must be empty after flushShortTermCache()",
76+
mockCache.get("short-k2", SHORT_LIVED_GROUP));
77+
}
78+
}

‎dotcms-integration/src/test/java/com/dotmarketing/business/RoleRevocationPermissionCacheTest.java‎

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
package com.dotmarketing.business;
2+
3+
import static org.junit.Assert.assertFalse;
4+
import static org.junit.Assert.assertTrue;
5+
6+
import com.dotcms.IntegrationTestBase;
7+
import com.dotcms.datagen.RoleDataGen;
8+
import com.dotcms.datagen.SiteDataGen;
9+
import com.dotcms.datagen.UserDataGen;
10+
import com.dotcms.util.IntegrationTestInitService;
11+
import com.dotmarketing.beans.Host;
12+
import com.dotmarketing.beans.Permission;
13+
import com.dotmarketing.common.db.DotConnect;
14+
import com.dotmarketing.db.HibernateUtil;
15+
import com.dotmarketing.exception.DotDataException;
16+
import com.dotmarketing.exception.DotSecurityException;
17+
import com.dotmarketing.util.Config;
18+
import com.liferay.portal.model.User;
19+
import org.junit.AfterClass;
20+
import org.junit.BeforeClass;
21+
import org.junit.Test;
22+
23+
/**
24+
* Regression tests for role-revocation propagation through
25+
* {@link PermissionCache#flushShortTermCache()}.
26+
*
27+
* Scenario: dotCMS/private-issues#567 — a user's cached
28+
* {@code doesUserHavePermission()} decision survives role revocation until a
29+
* full {@code Flush All}. Root cause was two gaps:
30+
* <ol>
31+
* <li>{@link PermissionCacheImpl#clearCache()} did not flush
32+
* {@code shortLivedGroup}.</li>
33+
* <li>{@link RoleFactoryImpl#addRoleToUser}/{@code removeRoleFromUser}
34+
* invalidated only the role cache, not cached permission decisions.</li>
35+
* </ol>
36+
*
37+
* These tests verify that after both fixes, cache invalidation happens without
38+
* any manual flush call.
39+
*/
40+
public class RoleRevocationPermissionCacheTest extends IntegrationTestBase {
41+
42+
private static PermissionAPI permissionAPI;
43+
private static RoleAPI roleAPI;
44+
private static User sysuser;
45+
private static Host site;
46+
private static int originalShortLivedSize;
47+
48+
@BeforeClass
49+
public static void prepare() throws Exception {
50+
IntegrationTestInitService.getInstance().init();
51+
permissionAPI = APILocator.getPermissionAPI();
52+
roleAPI = APILocator.getRoleAPI();
53+
sysuser = APILocator.getUserAPI().getSystemUser();
54+
55+
// The short-term permission cache must be enabled for this regression
56+
// to be meaningful; PermissionAPITest sets it to 0 to neutralize cache
57+
// flakes, but here we are specifically validating cache behavior.
58+
originalShortLivedSize = Config.getIntProperty("cache.permissionshortlived.size", 0);
59+
Config.setProperty("cache.permissionshortlived.size", 1000);
60+
61+
site = new SiteDataGen().nextPersisted();
62+
permissionAPI.permissionIndividually(site.getParentPermissionable(), site, sysuser);
63+
64+
CacheLocator.getPermissionCache().clearCache();
65+
}
66+
67+
@AfterClass
68+
public static void cleanup() {
69+
try {
70+
HibernateUtil.startTransaction();
71+
APILocator.getHostAPI().archive(site, sysuser, false);
72+
APILocator.getHostAPI().delete(site, sysuser, false);
73+
HibernateUtil.closeAndCommitTransaction();
74+
} catch (Exception e) {
75+
try {
76+
HibernateUtil.rollbackTransaction();
77+
} catch (Exception ignored) {
78+
// best-effort cleanup
79+
}
80+
} finally {
81+
HibernateUtil.closeSessionSilently();
82+
}
83+
84+
Config.setProperty("cache.permissionshortlived.size", originalShortLivedSize);
85+
CacheLocator.getPermissionCache().clearCache();
86+
}
87+
88+
/**
89+
* Role revocation must invalidate cached permission decisions without
90+
* requiring a manual cache flush.
91+
*/
92+
@Test
93+
public void removeRoleFromUser_invalidatesShortTermPermissionCache() throws Exception {
94+
final Role role = new RoleDataGen().nextPersisted();
95+
final User user = new UserDataGen().nextPersisted();
96+
97+
roleAPI.addRoleToUser(role, user);
98+
99+
final Permission p = new Permission();
100+
p.setPermission(PermissionAPI.PERMISSION_READ);
101+
p.setRoleId(role.getId());
102+
p.setInode(site.getIdentifier());
103+
permissionAPI.save(p, site, sysuser, false);
104+
105+
// Prime the short-term cache with a TRUE decision for this user.
106+
assertTrue("Granted role should grant READ on test host",
107+
permissionAPI.doesUserHavePermission(site, PermissionAPI.PERMISSION_READ, user, false));
108+
109+
// Revoke the role. No manual cache flush.
110+
roleAPI.removeRoleFromUser(role, user);
111+
112+
assertFalse("Cached TRUE decision must be invalidated by removeRoleFromUser;"
113+
+ " user no longer has any role granting READ on host",
114+
permissionAPI.doesUserHavePermission(site, PermissionAPI.PERMISSION_READ, user, false));
115+
}
116+
117+
/**
118+
* Role grant must also invalidate cached FALSE decisions so newly granted
119+
* permissions take effect immediately.
120+
*/
121+
@Test
122+
public void addRoleToUser_invalidatesShortTermPermissionCache() throws Exception {
123+
final Role role = new RoleDataGen().nextPersisted();
124+
final User user = new UserDataGen().nextPersisted();
125+
126+
final Permission p = new Permission();
127+
p.setPermission(PermissionAPI.PERMISSION_READ);
128+
p.setRoleId(role.getId());
129+
p.setInode(site.getIdentifier());
130+
permissionAPI.save(p, site, sysuser, false);
131+
132+
// Prime the short-term cache with a FALSE decision (user has no role yet).
133+
assertFalse("User without any granting role should be denied READ",
134+
permissionAPI.doesUserHavePermission(site, PermissionAPI.PERMISSION_READ, user, false));
135+
136+
// Grant the role. No manual cache flush.
137+
roleAPI.addRoleToUser(role, user);
138+
139+
assertTrue("Cached FALSE decision must be invalidated by addRoleToUser;"
140+
+ " user now has a role granting READ on host",
141+
permissionAPI.doesUserHavePermission(site, PermissionAPI.PERMISSION_READ, user, false));
142+
}
143+
144+
/**
145+
* {@link PermissionCache#clearCache()} must flush both primaryGroup and
146+
* shortLivedGroup. This is what the admin-UI "Flush Permission Cache"
147+
* button ultimately calls (via {@link PermissionAPI#clearCache()}).
148+
*/
149+
@Test
150+
public void clearCache_flushesShortLivedGroup() throws DotDataException, DotSecurityException {
151+
final Role role = new RoleDataGen().nextPersisted();
152+
final User user = new UserDataGen().nextPersisted();
153+
154+
roleAPI.addRoleToUser(role, user);
155+
156+
final Permission p = new Permission();
157+
p.setPermission(PermissionAPI.PERMISSION_READ);
158+
p.setRoleId(role.getId());
159+
p.setInode(site.getIdentifier());
160+
permissionAPI.save(p, site, sysuser, false);
161+
162+
// Prime the short-lived cache.
163+
assertTrue(permissionAPI.doesUserHavePermission(site, PermissionAPI.PERMISSION_READ, user, false));
164+
165+
// Simulate the "Flush Permission Cache" admin-UI button by calling the
166+
// API-level clearCache(), which delegates to PermissionCacheImpl.
167+
permissionAPI.clearCache();
168+
169+
// With the underlying DB state unchanged, the next check should still
170+
// return TRUE — but the cached boolean must be dropped and recomputed,
171+
// not served stale. We assert the permission decision remains correct
172+
// post-clear (the cache was wiped and re-populated from DB truth).
173+
assertTrue("Post-clear lookup should recompute from DB and still return TRUE",
174+
permissionAPI.doesUserHavePermission(site, PermissionAPI.PERMISSION_READ, user, false));
175+
176+
// Now revoke the role without going through RoleAPI/RoleFactory, because
177+
// that path has its own permission-cache invalidation. This leaves a
178+
// cached TRUE permission decision in place so clearCache() is the only
179+
// operation under test.
180+
revokeRoleMembershipInDbOnly(role, user);
181+
permissionAPI.clearCache();
182+
183+
assertFalse("After role revocation + clearCache, decision must be FALSE",
184+
permissionAPI.doesUserHavePermission(site, PermissionAPI.PERMISSION_READ, user, false));
185+
}
186+
187+
private void revokeRoleMembershipInDbOnly(final Role role, final User user) throws DotDataException {
188+
final DotConnect dc = new DotConnect();
189+
dc.setSQL("delete from users_cms_roles where user_id = ? and role_id = ?");
190+
dc.addParam(user.getUserId());
191+
dc.addParam(role.getId());
192+
dc.loadResult();
193+
194+
// Keep role lookups honest after bypassing RoleFactory; do not touch
195+
// PermissionCache here, because clearCache() is the behavior under test.
196+
CacheLocator.getCmsRoleCache().remove(user.getUserId());
197+
}
198+
}

0 commit comments

Comments
 (0)