Skip to content

Commit

Permalink
Evict all authorizations on rule events and do not cache authroizatio…
Browse files Browse the repository at this point in the history 
…ns 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.
  • Loading branch information
@groldan
groldan committed Apr 3, 2024
1 parent d9c11d8 commit fb9774f
Show file tree
Hide file tree
Showing 3 changed files with 120 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);
}
}
126 changes: 83 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,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");
Expand All @@ -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) {
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 fb9774f

Please sign in to comment.