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..0fc4f22 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,62 @@ 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") + // no matching rule + .matchingAdminRule(null) + .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 +169,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);