Skip to content

Commit

Permalink
Merge pull request #61 from groldan/fix/dont_cache_accessinfo_with_no…
Browse files Browse the repository at this point in the history 
…_matching_rules

Evict all authorizations on rule events and do not cache authroizations with no matching rules
  • Loading branch information
@groldan
groldan authored Apr 3, 2024
2 parents d9c11d8 + aae26e9 commit 2715614
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 110 deletions.
102 changes: 36 additions & 66 deletions ...ache/src/main/java/org/geoserver/acl/authorization/cache/CachingAuthorizationService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>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 {

Expand All @@ -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<String> 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<String> 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> A logLoaded(Object request, A accessInfo) {
log.debug("loaded and cached {} -> {}", request, accessInfo);
return accessInfo;
}

private boolean matches(AccessInfo cached, final Set<String> affectedRuleIds) {
List<String> 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<String> 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);
}
}
125 changes: 82 additions & 43 deletions .../src/test/java/org/geoserver/acl/authorization/cache/CachingAuthorizationServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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");
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion ...ugin/web/src/main/java/org/geoserver/acl/plugin/web/components/Select2SetMultiChoice.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public Select2SetMultiChoice(String id, IModel<Set<T>> model, ChoiceProvider<T>
new ArrayList<T>(model.getObject() == null ? Set.of() : model.getObject());
IModel<Collection<T>> selectModel = Model.of(initalValue);

add(select2 = new Select2MultiChoice<T>("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);
Expand Down

0 comments on commit 2715614

Please sign in to comment.