From fb9774f2c58840edf904eb8c675754adbc3bd637 Mon Sep 17 00:00:00 2001 From: Gabriel Roldan Date: Mon, 1 Apr 2024 18:32:19 -0300 Subject: [PATCH] Evict all authorizations on rule events and do not cache authroizations with no matching rules Caching authrization responses with no matching rules results in cached entries not being evicted when the request starts matching a rule. In any case, evict all authorizations upon rule events because the affected rule ids held in AccessInfo is not enough to cherry pick which ones to evict. A rule change may change the affected rules. --- .../cache/CachingAuthorizationService.java | 102 +++++--------- .../CachingAuthorizationServiceTest.java | 126 ++++++++++++------ .../web/components/Select2SetMultiChoice.java | 2 +- 3 files changed, 120 insertions(+), 110 deletions(-) diff --git a/src/integration/spring/cache/src/main/java/org/geoserver/acl/authorization/cache/CachingAuthorizationService.java b/src/integration/spring/cache/src/main/java/org/geoserver/acl/authorization/cache/CachingAuthorizationService.java index 45eff20..908b30f 100644 --- a/src/integration/spring/cache/src/main/java/org/geoserver/acl/authorization/cache/CachingAuthorizationService.java +++ b/src/integration/spring/cache/src/main/java/org/geoserver/acl/authorization/cache/CachingAuthorizationService.java @@ -17,10 +17,17 @@ import org.geoserver.acl.domain.rules.RuleEvent; import org.springframework.context.event.EventListener; -import java.util.List; -import java.util.Set; +import java.util.Map; import java.util.concurrent.ConcurrentMap; +/** + * {@link AuthorizationService} decorator that caches requests and responses, listens to events for + * eviction. + * + *

All {@link RuleEvent rule events} result in a full eviction of the {@link AccessInfo} cache, + * and all {@link AdminRuleEvent adminrule events} in the eviction of the {@link AdminAccessInfo} + * cache. + */ @Slf4j(topic = "org.geoserver.acl.authorization.cache") public class CachingAuthorizationService extends ForwardingAuthorizationService { @@ -40,89 +47,52 @@ public CachingAuthorizationService( @Override public AccessInfo getAccessInfo(@NonNull AccessRequest request) { AccessInfo grant = ruleAccessCache.computeIfAbsent(request, this::load); - if (grant.getMatchingRules().isEmpty()) {} + if (grant.getMatchingRules().isEmpty()) { + // do not cache results with no matching rules. It'll make it impossible to evict them + this.ruleAccessCache.remove(request); + } return grant; } private AccessInfo load(AccessRequest request) { - return super.getAccessInfo(request); + return logLoaded(request, super.getAccessInfo(request)); } @Override public AdminAccessInfo getAdminAuthorization(@NonNull AdminAccessRequest request) { - return adminRuleAccessCache.computeIfAbsent(request, this::load); - } - - private AdminAccessInfo load(AdminAccessRequest request) { - return super.getAdminAuthorization(request); - } - - @EventListener(RuleEvent.class) - public void onRuleEvent(RuleEvent event) { - switch (event.getEventType()) { - case DELETED, UPDATED: - evictRuleAccessCache(event); - break; - case CREATED: - default: - break; - } - } - - @EventListener(AdminRuleEvent.class) - public void onAdminRuleEvent(AdminRuleEvent event) { - switch (event.getEventType()) { - case DELETED, UPDATED: - evictAdminAccessCache(event); - break; - case CREATED: - default: - break; + AdminAccessInfo grant = adminRuleAccessCache.computeIfAbsent(request, this::load); + if (grant.getMatchingAdminRule() == null) { + // do not cache results with no matching rules. It'll make it impossible to evict them + this.adminRuleAccessCache.remove(request); } + return grant; } - private void evictRuleAccessCache(RuleEvent event) { - final Set affectedRuleIds = event.getRuleIds(); - ruleAccessCache.entrySet().stream() - .parallel() - .filter(e -> matches(e.getValue(), affectedRuleIds)) - .forEach( - e -> { - AccessRequest req = e.getKey(); - AccessInfo grant = e.getValue(); - ruleAccessCache.remove(req); - logEvicted(event, req, grant); - }); + private AdminAccessInfo load(AdminAccessRequest request) { + return logLoaded(request, super.getAdminAuthorization(request)); } - private void evictAdminAccessCache(AdminRuleEvent event) { - final Set affectedRuleIds = event.getRuleIds(); - adminRuleAccessCache.entrySet().stream() - .parallel() - .filter(e -> matches(e.getValue(), affectedRuleIds)) - .forEach( - e -> { - AdminAccessRequest req = e.getKey(); - AdminAccessInfo grant = e.getValue(); - adminRuleAccessCache.remove(req); - logEvicted(event, req, grant); - }); + private A logLoaded(Object request, A accessInfo) { + log.debug("loaded and cached {} -> {}", request, accessInfo); + return accessInfo; } - private boolean matches(AccessInfo cached, final Set affectedRuleIds) { - List matchingRules = cached.getMatchingRules(); - return matchingRules.stream().anyMatch(affectedRuleIds::contains); + @EventListener(RuleEvent.class) + public void onRuleEvent(RuleEvent event) { + int evictCount = evictAll(ruleAccessCache); + log.debug("evicted all {} authorizations upon event {}", evictCount, event); } - private boolean matches(AdminAccessInfo cached, final Set affectedRuleIds) { - String matchingRuleId = cached.getMatchingAdminRule(); - return affectedRuleIds.contains(matchingRuleId); + private int evictAll(Map cache) { + int size = cache.size(); + cache.clear(); + return size; } - private void logEvicted(Object event, Object req, Object grant) { - if (log.isDebugEnabled()) { - log.debug("event: {}, evicted {} -> {}", event, req, grant); - } + @EventListener(AdminRuleEvent.class) + public void onAdminRuleEvent(AdminRuleEvent event) { + int evictCount = evictAll(adminRuleAccessCache); + log.debug("evicted all {} admin authorizations upon event {}", evictCount, event); } } diff --git a/src/integration/spring/cache/src/test/java/org/geoserver/acl/authorization/cache/CachingAuthorizationServiceTest.java b/src/integration/spring/cache/src/test/java/org/geoserver/acl/authorization/cache/CachingAuthorizationServiceTest.java index f83f47a..21b67c5 100644 --- a/src/integration/spring/cache/src/test/java/org/geoserver/acl/authorization/cache/CachingAuthorizationServiceTest.java +++ b/src/integration/spring/cache/src/test/java/org/geoserver/acl/authorization/cache/CachingAuthorizationServiceTest.java @@ -56,7 +56,7 @@ void testCachingAuthorizationService() { @Test void testGetAccessInfo() { AccessRequest req = AccessRequest.builder().roles("ROLE_AUTHENTICATED").build(); - AccessInfo expected = AccessInfo.DENY_ALL; + AccessInfo expected = AccessInfo.DENY_ALL.withMatchingRules(List.of("1", "2")); when(delegate.getAccessInfo(req)).thenReturn(expected); AccessInfo r1 = caching.getAccessInfo(req); @@ -69,11 +69,29 @@ void testGetAccessInfo() { verify(delegate, times(1)).getAccessInfo(req); } + @Test + void testGetAccessInfoNotCachedIfNoMatchingrules() { + AccessRequest req = AccessRequest.builder().roles("ROLE_UNMATCHED").build(); + AccessInfo expected = AccessInfo.DENY_ALL.withMatchingRules(List.of()); + when(delegate.getAccessInfo(req)).thenReturn(expected); + + AccessInfo r1 = caching.getAccessInfo(req); + assertSame(expected, r1); + assertThat(this.dataAccessCache) + .as("AccessInfo with no matching rules should not be cached") + .doesNotContainKey(req); + } + @Test void testGetAdminAuthorization() { AdminAccessRequest req = AdminAccessRequest.builder().roles("ROLE_AUTHENTICATED").workspace("test").build(); - AdminAccessInfo expected = AdminAccessInfo.builder().admin(false).workspace("test").build(); + AdminAccessInfo expected = + AdminAccessInfo.builder() + .admin(false) + .workspace("test") + .matchingAdminRule("1") + .build(); when(delegate.getAdminAuthorization(req)).thenReturn(expected); AdminAccessInfo r1 = caching.getAdminAuthorization(req); @@ -87,35 +105,63 @@ void testGetAdminAuthorization() { } @Test - void testOnRuleEvent() { + void testGetAdminAuthorizationNotCachedIfNoMatchingrule() { + AdminAccessRequest req = + AdminAccessRequest.builder().roles("ROLE_UNMATCHED").workspace("test").build(); + AdminAccessInfo expected = + AdminAccessInfo.builder() + .admin(false) + .workspace("test") + .matchingAdminRule(null) // no + // matching + // rule + .build(); + when(delegate.getAdminAuthorization(req)).thenReturn(expected); + + AdminAccessInfo r1 = caching.getAdminAuthorization(req); + assertSame(expected, r1); + assertThat(this.adminAccessCache).doesNotContainKey(req); + verify(delegate, times(1)).getAdminAuthorization(req); + } + + @Test + void testOnRuleEventEvictsAll() { Rule rule1 = Rule.allow().withId("r1").withWorkspace("ws1").withLayer("l1"); Rule rule2 = rule1.withId("r2").withLayer("l2"); Rule rule3 = rule1.withId("r3").withLayer("l3"); - Rule rule4 = rule1.withId("r4").withLayer("l4"); - Rule rule5 = rule1.withId("r5").withLayer("l5"); AccessRequest req1 = req(rule1); AccessRequest req2 = req(rule2); AccessRequest req3 = req(rule3); - AccessRequest req4 = req(rule4); - AccessRequest req5 = req(rule5); + grantAll(rule1, rule2, rule3, req1, req2, req3); + caching.onRuleEvent(RuleEvent.updated(rule1)); + assertThat(dataAccessCache).isEmpty(); + + grantAll(rule1, rule2, rule3, req1, req2, req3); + caching.onRuleEvent(RuleEvent.deleted(rule2.getId())); + assertThat(dataAccessCache).isEmpty(); + + grantAll(rule1, rule2, rule3, req1, req2, req3); + caching.onRuleEvent(RuleEvent.created(rule3)); + assertThat(dataAccessCache).isEmpty(); + } + + private void grantAll( + Rule rule1, + Rule rule2, + Rule rule3, + AccessRequest req1, + AccessRequest req2, + AccessRequest req3) { grant(req1, rule1); grant(req2, rule1, rule2); grant(req3, rule1, rule2, rule3); - grant(req4, rule4, rule5); - grant(req5, rule5); - - // change to rule5 evicts req5 and req4, both have rule5.id int their matching rules - testOnRuleEvent(rule5, req5, req4); - - // change to rule1 evicts req1, req2, and req3, all of them have rule1.id in their matching - // rules - testOnRuleEvent(rule1, req1, req2, req3); + assertThat(dataAccessCache).containsKeys(req1, req2, req3); } @Test - void testOnAdminRuleEvent() { + void testOnAdminRuleEventEvictsAll() { var rule1 = AdminRule.admin().withId("r1").withWorkspace("ws1"); var rule2 = rule1.withId("r2"); var rule3 = rule1.withId("r3"); @@ -124,36 +170,30 @@ void testOnAdminRuleEvent() { var req2 = req1.withUser("user2"); var req3 = req1.withUser("user3"); - grant(req1, rule1); - grant(req2, rule2); - grant(req3, rule3); + grantAll(rule1, rule2, rule3, req1, req2, req3); + caching.onAdminRuleEvent(AdminRuleEvent.created(rule1)); + assertThat(adminAccessCache).isEmpty(); - testOnAdminRuleEvent(rule1, req1); - testOnAdminRuleEvent(rule2, req2); - testOnAdminRuleEvent(rule3, req3); - } + grantAll(rule1, rule2, rule3, req1, req2, req3); + caching.onAdminRuleEvent(AdminRuleEvent.updated(rule2)); + assertThat(adminAccessCache).isEmpty(); - private void testOnAdminRuleEvent(AdminRule modified, AdminAccessRequest expectedEviction) { - assertThat(adminAccessCache.get(expectedEviction)).isNotNull(); - var event = AdminRuleEvent.updated(modified); - caching.onAdminRuleEvent(event); - assertThat(adminAccessCache.get(expectedEviction)).isNull(); + grantAll(rule1, rule2, rule3, req1, req2, req3); + caching.onAdminRuleEvent(AdminRuleEvent.deleted(rule3.getId())); + assertThat(adminAccessCache).isEmpty(); } - private void testOnRuleEvent(Rule modified, AccessRequest... expectedEvictions) { - // pre-flight - for (AccessRequest req : expectedEvictions) { - AccessInfo grant = dataAccessCache.get(req); - assertThat(grant).isNotNull(); - assertThat(grant.getMatchingRules()).contains(modified.getId()); - } - - RuleEvent event = RuleEvent.updated(modified); - caching.onRuleEvent(event); - - for (AccessRequest req : expectedEvictions) { - assertThat(dataAccessCache).doesNotContainKey(req); - } + private void grantAll( + AdminRule rule1, + AdminRule rule2, + AdminRule rule3, + AdminAccessRequest req1, + AdminAccessRequest req2, + AdminAccessRequest req3) { + grant(req1, rule1); + grant(req2, rule2); + grant(req3, rule3); + assertThat(adminAccessCache).hasSize(3); } private AccessInfo grant(AccessRequest req, Rule... matching) { diff --git a/src/plugin/web/src/main/java/org/geoserver/acl/plugin/web/components/Select2SetMultiChoice.java b/src/plugin/web/src/main/java/org/geoserver/acl/plugin/web/components/Select2SetMultiChoice.java index a6d73f9..e4a3a2f 100644 --- a/src/plugin/web/src/main/java/org/geoserver/acl/plugin/web/components/Select2SetMultiChoice.java +++ b/src/plugin/web/src/main/java/org/geoserver/acl/plugin/web/components/Select2SetMultiChoice.java @@ -33,7 +33,7 @@ public Select2SetMultiChoice(String id, IModel> model, ChoiceProvider new ArrayList(model.getObject() == null ? Set.of() : model.getObject()); IModel> selectModel = Model.of(initalValue); - add(select2 = new Select2MultiChoice("select", selectModel, provider)); + add(select2 = new Select2MultiChoice<>("select", selectModel, provider)); select2.getSettings().setQueryParam("qm"); select2.getSettings().setPlaceholder("select"); // required for allowClear select2.getSettings().setAllowClear(true);