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.

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

Authorizer Headers are not carried through to Response #34

Closed
jmmk opened this issue Oct 2, 2017 · 10 comments · Fixed by pac4j/jax-rs-pac4j#29
Closed

Authorizer Headers are not carried through to Response #34

jmmk opened this issue Oct 2, 2017 · 10 comments · Fixed by pac4j/jax-rs-pac4j#29
Milestone

Comments

@jmmk
Copy link

jmmk commented Oct 2, 2017

I am not sure if this is an issue with dropwizard-pac4j or jax-rs-pac4j, or something I am doing incorrectly

Authorizers such as xssprotection, noframe, csrf, and nosniff work by setting a header on the WebContext. I have enabled these Authorizers, and I can see them being called and setting the header on the ServletJaxRsContext (I can see this by looking at the abortResponse). However, I do not see the headers in the server's response.

It seems that the headers from ServletJaxRsContext are lost and do not actually make it into the response.

Example code:

Pac4j Configuration:

bootstrap.addBundle(object : Pac4jBundle<MyConfiguration>() {
  override fun getPac4jFactory(config: MyConfiguration): Pac4jFactory {
    val encoder = SpringSecurityPasswordEncoder(BCryptPasswordEncoder())
    val dbAuthenticator = CustomDbAuthenticator(dbi, encoder)
    val formClient = FormClient("/auth/login", dbAuthenticator)

    val authMatcher = PathMatcher().apply {
      excludeBranch("/auth")
    }
    val globalAuthorizers = Pac4jFactory.JaxRsSecurityFilterConfiguration().apply {
      matchers = "auth"
      authorizers = "isFullyAuthenticated,csrf,nosniff,noframe,xssprotection"
    }

    return Pac4jFactory().apply {
      callbackUrl = "/auth/callback"
      globalFilters = listOf(globalAuthorizers)
      clients = listOf(formClient)
      matchers = mapOf("auth" to authMatcher)
    }
  }
}

Add Resources

override fun run(config: MyConfiguration, environment: Environment) {
  environment.jersey().register(AuthResource())
  environment.jersey().register(NormalResource())
}

AuthResource:

@Path("/auth")
class AuthResource() {

  @GET
  @Path("/login")
  @Produces(MediaType.TEXT_HTML)
  fun login(): View {
    return object : View("/login.ftl") {
      val callbackUrl = config.clients.findClient(FormClient::class.java).callbackUrl
    }
  }

  @POST
  @Path("/callback")
  @Pac4JCallback(renewSession = booleanArrayOf(false), defaultUrl = arrayOf("/normal/home/"))
  fun loginFormPost() {
    // Handled by pac4j
  }

  @GET
  @Path("/logout")
  @Pac4JLogout(defaultUrl = arrayOf("/auth/login"))
  fun logout() {
    // Handled by pac4j
  }
}

NormalResource:

This is where I would expect to have the headers set in the response, but they are not there.

@Path("/normal")
class NormalResource {

  @GET
  @Path("/home")
  @Produces(MediaType.TEXT_HTML)
  fun home(): View {
    return object : View("/home.ftl") {}
  }

  @GET
  @Path("/test")
  fun test(): Response {
    return Response.ok().build()
  }
}
@victornoel
Copy link
Member

Hm, this looks like a (big!) bug in jax-rs-pac4j :)

I suppose the problem comes from the fact that I originally assumed that the headers added to the response would be only in case there was a refusal of the request by pac4j (hence the headers set in the abortResponse), but in these cases if I understand well, the headers should simply be set to the normal response!

My bad, I will try to quickly find a way to reproduce in a unit test and fix it!

Thanks for the report :)

victornoel added a commit to victornoel/jax-rs-pac4j that referenced this issue Oct 4, 2017
All the properties set by pac4j (e.g., headers from authorizers) were
only set on the response in case of failure, but not when the jax-rs
runtime was answering.

This is fixed, but the `skipResponse` option disables this behaviour.

Closes pac4j/dropwizard-pac4j#34
@victornoel
Copy link
Member

@jmmk this should be fixed on master of jax-rs-pac4j, could you try with it? (simply use the latest stable version of dropwizard-pac4j and 2.0.2-SNAPSHOT of jax-rs-pac4j, there are instructions in the README for the repository to use to get them).

If everything is good, then we will quickly release a new version of jax-rs-pac4j and dropwizard-pac4j!

@jmmk
Copy link
Author

jmmk commented Oct 4, 2017

@victornoel I've tested jax-rs-pac4j 2.0.2-SNAPSHOT locally and I see the headers being set

Thanks for the quick turnaround

@victornoel
Copy link
Member

@leleuj could we get a release for jax-rs-pac4j and dropwizard-pac4j? This bug was a big one :) and after I will start migrating to pac4j 3 I think…

@leleuj
Copy link
Member

leleuj commented Oct 6, 2017

Sure. I will do that this week-end...

@leleuj
Copy link
Member

leleuj commented Oct 7, 2017

I released jax-rs-pac4j v2.0.2 and dropwizard-pac4j v2.0.1.

@victornoel
Copy link
Member

excellent, thanks. @jmmk you can go on with the latest release! Thanks for the report and don't hesitate if you see more problems :)

@victornoel
Copy link
Member

@leleuj I'm really sorry, I forgot to push my commits in dropwizard-pac4j where I updated some of the dependencies before asking you to release… could you release dropwizard-pac4j again? It would now be v2.0.2. Sorry again :)

@leleuj
Copy link
Member

leleuj commented Oct 13, 2017

I updated the jax-rs-pac4j dependency, but not the others in fact. Starting the release...

@leleuj
Copy link
Member

leleuj commented Oct 13, 2017

I'm done. The artifacts will show up in the Maven central repository. I let you handle the rest...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants