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

Auth with RolesAllowedDynamicFeature #952

Merged
merged 1 commit into from Apr 7, 2015

Conversation

Projects
None yet
6 participants
@carlo-rtr
Member

carlo-rtr commented Mar 24, 2015

This allows evaluating auth before processing resource method params

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 24, 2015

Member

@carlo-rtr do we need to add that annotation dependency or are they available for use outside of the test scope? Otherwise I think this looks great. Thanks for all of your work on it!

Member

jplock commented Mar 24, 2015

@carlo-rtr do we need to add that annotation dependency or are they available for use outside of the test scope? Otherwise I think this looks great. Thanks for all of your work on it!

@glennmcallister

This comment has been minimized.

Show comment
Hide comment
@glennmcallister

glennmcallister Mar 25, 2015

Contributor

Agreed. This is awesome.

We do still need to decide if this warrants branching for 0.8.1 for all the
other changes made up to this point, and including this in 0.9.x since it
is a MAJOR change in how we do authentication. And now authorization as a
feature, since we never really supported that previously.

On Tue, Mar 24, 2015 at 5:42 PM Justin Plock notifications@github.com
wrote:

@carlo-rtr https://github.com/carlo-rtr do we need to add that
annotation dependency or are they available for use outside of the test
scope? Otherwise I think this looks great. Thanks for all of your work on
it!


Reply to this email directly or view it on GitHub
#952 (comment).

Contributor

glennmcallister commented Mar 25, 2015

Agreed. This is awesome.

We do still need to decide if this warrants branching for 0.8.1 for all the
other changes made up to this point, and including this in 0.9.x since it
is a MAJOR change in how we do authentication. And now authorization as a
feature, since we never really supported that previously.

On Tue, Mar 24, 2015 at 5:42 PM Justin Plock notifications@github.com
wrote:

@carlo-rtr https://github.com/carlo-rtr do we need to add that
annotation dependency or are they available for use outside of the test
scope? Otherwise I think this looks great. Thanks for all of your work on
it!


Reply to this email directly or view it on GitHub
#952 (comment).

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 25, 2015

Member

My vote would be for 0.9.0 and that we should accelerate releasing 0.8.1 to address the issues that have been fixed there. We could create a 0.8 branch to keep a place for creating additional 0.8.x fixes.

Member

jplock commented Mar 25, 2015

My vote would be for 0.9.0 and that we should accelerate releasing 0.8.1 to address the issues that have been fixed there. We could create a 0.8 branch to keep a place for creating additional 0.8.x fixes.

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 25, 2015

Member

I agree with Justin on this one.
I think we should release 0.8.1 in mid-april and then move to 0.9.0 with back reports to 0.8.x

Member

arteam commented Mar 25, 2015

I agree with Justin on this one.
I think we should release 0.8.1 in mid-april and then move to 0.9.0 with back reports to 0.8.x

@jplock jplock modified the milestone: 0.9.0 Mar 25, 2015

@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@carlo-rtr

carlo-rtr Mar 26, 2015

Member

@jplock The example works fine. You get the dependency for free with jersey-server, which is non optional for DW. Do you think I should add it explicitly?

[INFO] | +- io.dropwizard:dropwizard-jersey:jar:0.8.1-SNAPSHOT:compile
[INFO] | | +- org.glassfish.jersey.core:jersey-server:jar:2.17:compile
[INFO] | | | +- org.glassfish.jersey.core:jersey-common:jar:2.17:compile
[INFO] | | | | +- org.glassfish.jersey.bundles.repackaged:jersey-guava:jar:2.17:compile
[INFO] | | | | - org.glassfish.hk2:osgi-resource-locator:jar:1.0.1:compile
[INFO] | | | +- javax.ws.rs:javax.ws.rs-api:jar:2.0.1:compile
[INFO] | | | +- org.glassfish.jersey.media:jersey-media-jaxb:jar:2.17:compile
[INFO] | | | +- javax.annotation:javax.annotation-api:jar:1.2:compile

Member

carlo-rtr commented Mar 26, 2015

@jplock The example works fine. You get the dependency for free with jersey-server, which is non optional for DW. Do you think I should add it explicitly?

[INFO] | +- io.dropwizard:dropwizard-jersey:jar:0.8.1-SNAPSHOT:compile
[INFO] | | +- org.glassfish.jersey.core:jersey-server:jar:2.17:compile
[INFO] | | | +- org.glassfish.jersey.core:jersey-common:jar:2.17:compile
[INFO] | | | | +- org.glassfish.jersey.bundles.repackaged:jersey-guava:jar:2.17:compile
[INFO] | | | | - org.glassfish.hk2:osgi-resource-locator:jar:1.0.1:compile
[INFO] | | | +- javax.ws.rs:javax.ws.rs-api:jar:2.0.1:compile
[INFO] | | | +- org.glassfish.jersey.media:jersey-media-jaxb:jar:2.17:compile
[INFO] | | | +- javax.annotation:javax.annotation-api:jar:1.2:compile

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 26, 2015

Member

It's probably fine then, thanks for checking. So do we want to merge this into master and add it to a 0.9.0 milestone? Any other opinions?

Member

jplock commented Mar 26, 2015

It's probably fine then, thanks for checking. So do we want to merge this into master and add it to a 0.9.0 milestone? Any other opinions?

@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@carlo-rtr

carlo-rtr Mar 26, 2015

Member

I kicked off a discussion on the forums for the plan for 0.8.1
https://groups.google.com/forum/#!topic/dropwizard-dev/QxAz9PZBA-E

Member

carlo-rtr commented Mar 26, 2015

I kicked off a discussion on the forums for the plan for 0.8.1
https://groups.google.com/forum/#!topic/dropwizard-dev/QxAz9PZBA-E

@jplock jplock added this to the 0.9.0 milestone Mar 27, 2015

Auth with RolesAllowedDynamicFeature
This allows evaluating auth before processing resource method params

carlo-rtr added a commit that referenced this pull request Apr 7, 2015

Merge pull request #952 from carlo-rtr/auth_rewrite2
Auth with RolesAllowedDynamicFeature

@carlo-rtr carlo-rtr merged commit d5410fb into dropwizard:master Apr 7, 2015

@jondavidnd1

This comment has been minimized.

Show comment
Hide comment
@jondavidnd1

jondavidnd1 Apr 17, 2015

I was looking over the changes to the documentation for this issue, and I think the code examples may be missing some info.

It seems like this line:

final Function<AuthFilter.Tuple, SecurityContext> securityContextFunction = return new SecurityContext() {

should be more like this:

final Function<AuthFilter.Tuple, SecurityContext> securityContextFunction = new Function<AuthFilter.Tuple, SecurityContext>() {
            @Override
            public SecurityContext apply(final AuthFilter.Tuple input) {
                return new SecurityContext() {
                     ...
                };
            }
       };

jondavidnd1 commented Apr 17, 2015

I was looking over the changes to the documentation for this issue, and I think the code examples may be missing some info.

It seems like this line:

final Function<AuthFilter.Tuple, SecurityContext> securityContextFunction = return new SecurityContext() {

should be more like this:

final Function<AuthFilter.Tuple, SecurityContext> securityContextFunction = new Function<AuthFilter.Tuple, SecurityContext>() {
            @Override
            public SecurityContext apply(final AuthFilter.Tuple input) {
                return new SecurityContext() {
                     ...
                };
            }
       };
@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@carlo-rtr

carlo-rtr Apr 17, 2015

Member

This should fix that. Thanks.

#999

Member

carlo-rtr commented Apr 17, 2015

This should fix that. Thanks.

#999

@arteam arteam added the feature label Apr 26, 2015

@@ -1,12 +1,15 @@
package io.dropwizard.auth;

This comment has been minimized.

@yunspace

yunspace May 6, 2015

Contributor

Should this test be moved to package io.dropwizard.auth.chained to be consistent with the location of ChainedAuthFilter?

@yunspace

yunspace May 6, 2015

Contributor

Should this test be moved to package io.dropwizard.auth.chained to be consistent with the location of ChainedAuthFilter?

This comment has been minimized.

@carlo-rtr

carlo-rtr May 7, 2015

Member

I think that makes sense.

@carlo-rtr

carlo-rtr May 7, 2015

Member

I think that makes sense.

@nickbabcock nickbabcock referenced this pull request Jul 1, 2015

Closed

Upgrade to Jersey 2.18 #1138

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