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: Add support for optional resource protection #1931

Merged
merged 3 commits into from Feb 18, 2017

Conversation

Projects
None yet
5 participants
@evnm
Member

evnm commented Feb 17, 2017

The current support for optionally-protected resources isn't great. We require users to implement a custom auth filter to inject a dummy SecurityContext for unauth'd requests.

I had occasion to write such an endpoint at work and decided to take a stab at supporting Optional principals with the @Auth annotation. With this patch, I'm able to do things like this:

@GET
public String getGreeting(@Auth Optional<User> userOpt) {
  if (userOpt.isPresent()) {
    return "Hello, " + userOpt.get().getName() + "!";
  } else {
    return "Greetings, anonymous visitor!"
  }
}

This change breaks down into two steps:

  1. A new ContainerRequestFilter decorator is used to silence exceptions thrown by failed AuthFilter invocations. This results in every request succeeding and principals being injected into the SecurityContext of auth'd requests.
  2. An OptionalPrincipalContainerRequestValueFactory class is used to return the optional principal rather than throwing on its absence.

These two new classes are wired into AuthDynamicFeature and AuthValueFactoryProvider and their polymorphic equivalents. Tests are added to verify the behavior of each combination and the auth section of the used manual is updated to reflect this new capability.

@evnm evnm added the feature label Feb 17, 2017

@evnm evnm added this to the 1.2.0 milestone Feb 17, 2017

@jplock

jplock approved these changes Feb 17, 2017

public Optional<Principal> provide() {
final Principal principal = getContainerRequest().getSecurityContext().getUserPrincipal();
if (principal == null) {

This comment has been minimized.

@jplock

jplock Feb 17, 2017

Member

could this be simplified to return Optional.ofNullable(principal);?

This comment has been minimized.

@evnm

evnm Feb 17, 2017

Member

Derp, yes.

@coveralls

This comment has been minimized.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.1%) to 84.765% when pulling 98d15de on evnm:dropwizard-auth/optional-auth into 9295413 on dropwizard:master.

@arteam

arteam approved these changes Feb 17, 2017

for (int i = 0; i < parameterAnnotations.length; i++) {
for (final Annotation annotation : parameterAnnotations[i]) {
// Optional auth requires that a concrete AuthFilter be provided.
if (annotation instanceof Auth && parameterTypes[i].equals(Optional.class) && authFilter != null) {

This comment has been minimized.

@arteam

arteam Feb 17, 2017

Member

I guess we could check the annotation's type only one time:

if (annotation instanceof Auth) {
    // Optional auth requires that a concrete AuthFilter be provided.
    if (parameterTypes[i].equals(Optional.class) && authFilter != null) {
        context.register(new WebApplicationExceptionCatchingFilter(authFilter));
        return;
    } else {
        registerAuthFilter(context);
        return;
    }
}

This comment has been minimized.

@evnm

evnm Feb 17, 2017

Member

I typically prefer flatter conditional tree, but in this case it seems reasonable. 👍

@@ -52,8 +54,15 @@ public AuthValueFactoryProvider(MultivaluedParameterExtractorProvider mpep,
*/
@Override
public AbstractContainerRequestValueFactory<?> createValueFactory(Parameter parameter) {
if (!parameter.isAnnotationPresent(Auth.class) || !principalClass.equals(parameter.getRawType())) {
final boolean isOptionalPrincipal = parameter.getRawType().equals(Optional.class)

This comment has been minimized.

@arteam

arteam Feb 17, 2017

Member

I guess we could make it more concise as:

if (!parameter.isAnnotationPresent(Auth.class)) {
    return null;
} else if (principalClass == parameter.getRawType()) {
    return new PrincipalContainerRequestValueFactory();
} else {
    final boolean isOptionalPrincipal = parameter.getRawType() == Optional.class
        && ParameterizedType.class.isAssignableFrom(parameter.getType().getClass())
        && principalClass == ((ParameterizedType) parameter.getType()).getActualTypeArguments()[0];
    return isOptionalPrincipal ? new OptionalPrincipalContainerRequestValueFactory() : null;
}
return;
// If the parameter type is an Optional, extract its type
// parameter. Otherwise, use the parameter type itself.
final Type paramType = (parameterTypes[i].equals(Optional.class))

This comment has been minimized.

@arteam

arteam Feb 17, 2017

Member

I think we could extract this block to the outer loop and also I believe we can compare classes by == instead of equals.

for (int i = 0; i < parameterAnnotations.length; i++) {
    final Class<?> type = parameterTypes[i];
    // If the parameter type is an Optional, extract its type
    // parameter. Otherwise, use the parameter type itself.
    final Type paramType = type == Optional.class
    ? ((ParameterizedType) parameterGenericTypes[i]).getActualTypeArguments()[0]
    : type;
: parameterTypes[i];
if (annotation instanceof Auth && authFilterMap.containsKey(paramType)) {
if (parameterTypes[i].equals(Optional.class)) {

This comment has been minimized.

@arteam

arteam Feb 17, 2017

Member

I believe we could just use == here

This comment has been minimized.

@evnm

evnm Feb 17, 2017

Member

Not sure why I opted for .equals all over the place 😑

@@ -57,8 +59,15 @@ public PolymorphicAuthValueFactoryProvider(
*/
@Override
public AbstractContainerRequestValueFactory<?> createValueFactory(Parameter parameter) {
if (!parameter.isAnnotationPresent(Auth.class) || !principalClassSet.contains(parameter.getRawType())) {
final boolean isOptionalPrincipal = parameter.getRawType().equals(Optional.class)

This comment has been minimized.

@arteam

arteam Feb 17, 2017

Member

I think we could implement this in a more concise form as well:

if (!parameter.isAnnotationPresent(Auth.class)) {
    return null;
} else if (principalClassSet.contains(parameter.getRawType())) {
    return new PrincipalContainerRequestValueFactory();
} else {
    final boolean isOptionalPrincipal = parameter.getRawType() == Optional.class
        && ParameterizedType.class.isAssignableFrom(parameter.getType().getClass())
        && principalClassSet.contains(((ParameterizedType) parameter.getType()).getActualTypeArguments()[0]);
    return isOptionalPrincipal ? new OptionalPrincipalContainerRequestValueFactory() : null;
}
private final ContainerRequestFilter underlying;
public WebApplicationExceptionCatchingFilter(ContainerRequestFilter underlying) {
this.underlying = underlying;

This comment has been minimized.

@arteam

arteam Feb 17, 2017

Member

I think we could add a precondition check that underlying should be not null

@GET
@Path("optional")
public String checkOptionalAuth(@Auth Optional<Principal> principalOpt) {
return "principal was " + ((principalOpt.isPresent()) ? "" : "not ") + "present";

This comment has been minimized.

@arteam

arteam Feb 17, 2017

Member

I guess we could omit the parentheses around principalOpt.isPresent

@coveralls

This comment has been minimized.

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.1%) to 84.761% when pulling a4c24f1 on evnm:dropwizard-auth/optional-auth into 9295413 on dropwizard:master.

@coveralls

This comment has been minimized.

coveralls commented Feb 18, 2017

Coverage Status

Coverage increased (+0.1%) to 84.761% when pulling f701585 on evnm:dropwizard-auth/optional-auth into 9295413 on dropwizard:master.

@arteam arteam merged commit c44e14e into dropwizard:master Feb 18, 2017

@eonwhite

This comment has been minimized.

Contributor

eonwhite commented Feb 21, 2017

@evnm Is there any chance that this feature could make it into an earlier milestone, perhaps the 1.1.x release?

I really need optional resource protection for something I'm working on now, and was actually about to go and write a PR for the exact same approach myself, so I was really delighted to see this written and merged. I was slightly less delighted to see it's two whole minor versions away.

I can rely on 1.2.0-SNAPSHOT for a bit while developing, but I don't want to roll something into prod with a snapshot dependency.

Since Dropwizard made a breaking change to remove the optional flag from @Auth several releases back, addressing this is somewhat of a bugfix as well as a feature. Understand if 1.1.x is feature-locked, but figured I'd ask!

@evnm

This comment has been minimized.

Member

evnm commented Feb 21, 2017

@ibwhite I also would like to be able to make use of this change in production, so I share your sense of haste.

I'm going to defer to other committers, though. It seems to me that it's somewhat debatable whether this change represents an introduction of new functionality or, as you say, a bugfix-esque improvement. @jplock, @arteam, what do you guys think?

@jplock

This comment has been minimized.

Member

jplock commented Feb 21, 2017

I wouldn't mind seeing this in 1.1.0 as well. +1 from me.

@evnm evnm modified the milestones: 1.1.0, 1.2.0 Feb 21, 2017

@evnm

This comment has been minimized.

Member

evnm commented Feb 21, 2017

Set the milestone to 1.1.0. Perhaps this can be cherry-picked into a second 1.1.0 release candidate.

@jplock

This comment has been minimized.

Member

jplock commented Feb 21, 2017

I can cherry-pick it into release/1.1.x later today

jplock added a commit that referenced this pull request Feb 21, 2017

dropwizard-auth: Add support for optional resource protection (#1931)
* dropwizard-auth: Add support for optional resource protection

* Heed jplock and arteam's feedback

* Fix checkstyle issues
@jplock

This comment has been minimized.

Member

jplock commented Feb 21, 2017

Cherry picked into release/1.1.x in 9c90c51

@eonwhite

This comment has been minimized.

Contributor

eonwhite commented Feb 21, 2017

👍 Thank you so much @evnm and @jplock! I really appreciate moving this up. My production codebases thank you!

@evnm evnm added improvement and removed feature labels Feb 22, 2017

nickbabcock added a commit that referenced this pull request Mar 8, 2017

nickbabcock added a commit that referenced this pull request Mar 8, 2017

reneploetz added a commit to reneploetz/dropwizard that referenced this pull request Mar 10, 2017

aaanders added a commit to aaanders/dropwizard that referenced this pull request Apr 26, 2017

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