Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Evict all authorizations on rule events and do not cache authroizations with no matching rules #61

Merged
merged 1 commit into from
Apr 3, 2024
Merged

Evict all authorizations on rule events and do not cache authroizations with no matching rules #61

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 36 additions & 66 deletions ...ache/src/main/java/org/geoserver/acl/authorization/cache/CachingAuthorizationService.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading