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

Make authorization optional #1162

Merged
merged 2 commits into from Jul 12, 2015

Conversation

Projects
None yet
3 participants
@arteam
Member

arteam commented Jul 12, 2015

This change consists from two parts: fixing behavior of the@PermitAll annotation and making
providing an authorizer in AuthFilterBuilder optional.

The @PermitAll annotation is actually intended for marking that a resource doesn't require authorization. Currently we treat it like the resource doesn't require authentication.

The solution is to register an auth filter for the methods marked with the @PermitAll annotation. The methods will require authentication, but anyone, who was given access, can use it. This simplifies the work of users who don't care about roles and authorization. They don't need create a fiction role and register RolesAllowedDynamicFeature.

Because users have an option to not authorize their resource methods, it makes sense to not require a custom authorizer in AuthFilterBuilder. By default the filter will permit access to any principal in any role.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Jul 12, 2015

Member

There is a lot of duplication in the authentication provider tests. I wish we could generalize them and not copy-past tests between the different providers...

Member

arteam commented Jul 12, 2015

There is a lot of duplication in the authentication provider tests. I wish we could generalize them and not copy-past tests between the different providers...

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Jul 12, 2015

Member

This looks great. One question, if a user wants to use an Authenticator but has no intention of doing anything related to authorization, is @PermitAll still required on the resources?

Member

jplock commented Jul 12, 2015

This looks great. One question, if a user wants to use an Authenticator but has no intention of doing anything related to authorization, is @PermitAll still required on the resources?

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Jul 12, 2015

Member

Maybe a better question is, if you don't register an authorizer, is there anyway we can assume that all of the resource methods have @PermitAll? That would provide a clear upgrade path for anyone pre-0.9 who is just using an authenticator.

Member

jplock commented Jul 12, 2015

Maybe a better question is, if you don't register an authorizer, is there anyway we can assume that all of the resource methods have @PermitAll? That would provide a clear upgrade path for anyone pre-0.9 who is just using an authenticator.

@jplock jplock added this to the 0.9.0 milestone Jul 12, 2015

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Jul 12, 2015

Member

Maybe a better question is, if you don't register an authorizer, is there anyway we can assume that all of the resource methods have @PermitAll? That would provide a clear upgrade path for anyone pre-0.9 who is just using an authenticator.

No, we can't. To register the filter one need to annotate the method with the @PermitAll annotatiion. In this case we will register the auth filter and the autentiication process will start.

To help migration from 0.8.x we could register the filter if the @Auth annotation is present on resources method parameters. But I'm not completely sure that's a good idea. From one point it makes things simpler if authorization is not a concern, from the other point it will increase confusion in the process of protecting resources.

Currently we clearly point in the documentation that to protect a method one should annotate it with the javax.security.* annotations. And that @Auth is used only for injecting the principal to method parameters. If we add additional logic to @Auth, there could be a question, why don't register the filter if the @Context annotation is present on a method? So far, I'm against it.

Member

arteam commented Jul 12, 2015

Maybe a better question is, if you don't register an authorizer, is there anyway we can assume that all of the resource methods have @PermitAll? That would provide a clear upgrade path for anyone pre-0.9 who is just using an authenticator.

No, we can't. To register the filter one need to annotate the method with the @PermitAll annotatiion. In this case we will register the auth filter and the autentiication process will start.

To help migration from 0.8.x we could register the filter if the @Auth annotation is present on resources method parameters. But I'm not completely sure that's a good idea. From one point it makes things simpler if authorization is not a concern, from the other point it will increase confusion in the process of protecting resources.

Currently we clearly point in the documentation that to protect a method one should annotate it with the javax.security.* annotations. And that @Auth is used only for injecting the principal to method parameters. If we add additional logic to @Auth, there could be a question, why don't register the filter if the @Context annotation is present on a method? So far, I'm against it.

arteam added some commits Jul 12, 2015

Correct behaviour of the @PermitAll annotation
The `@PermitAll` annotation is actually intended  for marking that a
resource doesn't require authorization. Currently we treat it like the
resource doesn't require authentication.

The solution is to register an auth filter for the methods marked with
the `@PermitAll` annotation. The methods will require authentication,
but anyone, who was given access, can use it.

This simplifies the work of users who don't care about roles and
authorization. They don't need create a fiction role and register
`RolesAllowedDynamicFeature`.
Make providing an authorizer optional in AuthFilterBuilder
Users have an option to not authorize their resource methods by
marking them with the `@PermitAll` annotation. So it makes sense
to not require a custom authorizer in `AuthFilterBuilder`. By default
the filter will permit access to any principal in any role.
@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@carlo-rtr

carlo-rtr Jul 12, 2015

Member

Great work, definitely makes the feature more user friendly. This looks good to me, I'll merge.

Previous versions always required the @Auth for authentication. So while the argument about @Context makes sense, I think most people will probably (and should) use @Auth. So I'm a fan of registering @PermitAll if the method has a @Auth parameter. I think if we call it out on the docs, it won't be terribly confusing. I think aiding the migration out weighs the downside. wdyt?

Member

carlo-rtr commented Jul 12, 2015

Great work, definitely makes the feature more user friendly. This looks good to me, I'll merge.

Previous versions always required the @Auth for authentication. So while the argument about @Context makes sense, I think most people will probably (and should) use @Auth. So I'm a fan of registering @PermitAll if the method has a @Auth parameter. I think if we call it out on the docs, it won't be terribly confusing. I think aiding the migration out weighs the downside. wdyt?

carlo-rtr added a commit that referenced this pull request Jul 12, 2015

@carlo-rtr carlo-rtr merged commit 650faf0 into dropwizard:master Jul 12, 2015

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Jul 12, 2015

Member

Weighing the fact that we have many users who use just @Auth and they expect smooth transition to 0.9.0, we could take the step. If we document the reasons why we do that, the process of protecting resources will look transparent enough.

Member

arteam commented Jul 12, 2015

Weighing the fact that we have many users who use just @Auth and they expect smooth transition to 0.9.0, we could take the step. If we document the reasons why we do that, the process of protecting resources will look transparent enough.

@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@carlo-rtr

carlo-rtr Jul 13, 2015

Member

This is a start on making that change: https://github.com/dropwizard/dropwizard/compare/master...carlo-rtr:auth_registers_filter?expand=1

I'm planning to try to consolidate what I can for the tests, and then submit a PR

Member

carlo-rtr commented Jul 13, 2015

This is a start on making that change: https://github.com/dropwizard/dropwizard/compare/master...carlo-rtr:auth_registers_filter?expand=1

I'm planning to try to consolidate what I can for the tests, and then submit a PR

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