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.

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

dropwizard-auth: Introduce CachingAuthorizer #1639

Merged
merged 1 commit into from Jul 27, 2016

Conversation

Projects
None yet
4 participants
@evnm
Member

evnm commented Jul 16, 2016

Addresses #1638

The boolean authorization state of principal+role pairs are cached individually using ImmutablePairs internally as cache keys. This unfortunately leads to more allocation-prone invalidate methods than those of CachingAuthenticator.

I assumed that CachingAuthorizer wouldn't be subject to the negative-result-caching issue that was dealt with in CachingAuthenticator. If this is incorrect, then we could instead cache an ImmutableSet<String> of role associations for each principal and always pass negative cache results through to the underlying authorizer.

@coveralls

This comment has been minimized.

coveralls commented Jul 16, 2016

Coverage Status

Coverage increased (+0.1%) to 82.269% when pulling 7a54a34 on evnm:dropwizard-auth/caching-authorizer into 38879e6 on dropwizard:master.

*/
public class CachingAuthorizer<P extends Principal> implements Authorizer<P> {
private final Authorizer<P> underlying;
private final Cache<ImmutablePair<P, String>, Boolean> cache;

This comment has been minimized.

@Jimexist

Jimexist Jul 17, 2016

what about the value being an enum:

private enum AuthzResult {
  Allow, Deny
}

so it's rather clearer that the tri-state of true, false, null in the boolean case.

This comment has been minimized.

@evnm

evnm Jul 17, 2016

Member

I'm all for improving clarity, but because Java enum types are defined as classes, references to them can also be null. For example, the following is valid code:

enum Foo { Allow, Deny }    
final Foo foo = null;

If it seems ambiguous to use boolean values within the context of authorization (i.e. if it's not obvious that true=authorized, false=unauthorized), then an Allow/Deny enum would be more explicit. However, given that all of this is private code internal to the class, I think it's a fair tradeoff to use booleans to avoid introducing a new class. I think an inline comment on the cache type semantics is definitely warranted.

This comment has been minimized.

@Jimexist

Jimexist Jul 17, 2016

that's a valid point, a comment should suffice since it's private

@coveralls

This comment has been minimized.

coveralls commented Jul 17, 2016

Coverage Status

Coverage increased (+0.1%) to 82.269% when pulling e81a50a on evnm:dropwizard-auth/caching-authorizer into 38879e6 on dropwizard:master.

@jplock jplock added the improvement label Jul 19, 2016

@jplock jplock added this to the 1.1.0 milestone Jul 19, 2016

@jplock jplock added feature and removed improvement labels Jul 19, 2016

@jplock jplock merged commit ec3f606 into dropwizard:master Jul 27, 2016

@evnm evnm deleted the evnm:dropwizard-auth/caching-authorizer branch Jul 27, 2016

@evnm evnm referenced this pull request Jul 27, 2016

Closed

CachingAuthorizer? #1638

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment