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

Better support for setting WWW-Authenticate header for basic autentification #406

Closed
allanbrohansen opened this Issue Jul 14, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@allanbrohansen

allanbrohansen commented Jul 14, 2016

Issue: The http-response for a failed basic autentification should include a WWW-Authenticate header.
But setting such a header is not supported as part of throwing a ca.uhn.fhir.rest.server.exceptions.AuthenticationException.
My current solution is also to implement handleException for this scenario in the autentification interceptor. The code looking something like this (as we only have basic autentification).
@Override public boolean handleException(RequestDetails requestDetails, BaseServerResponseException e,HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { if(e instanceof AuthenticationException) { if(!response.containsHeader("WWW-Authenticate")) { response.addHeader("WWW-Authenticate", "Basic realm=\"Some realm\""); } } return true; }
Not super nice...

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Jul 15, 2016

Owner

Definitely agreed this isn't great.

We don't actually use WWW-Authenticate ourselves, so I don't have much insight into how this would be used by HAPI developers. Any insights into what would be useful behaviour/API?

Owner

jamesagnew commented Jul 15, 2016

Definitely agreed this isn't great.

We don't actually use WWW-Authenticate ourselves, so I don't have much insight into how this would be used by HAPI developers. Any insights into what would be useful behaviour/API?

@allanbrohansen

This comment has been minimized.

Show comment
Hide comment
@allanbrohansen

allanbrohansen Aug 1, 2016

(sorry for the late reply - holiday)
One possible implementation would be to have a specific interceptor class for the basic http autentification (much like I was forced to do). This class should then have an overrideable implementation of String getRealm which the interceptor class would use.
Another way would be to extend the AuthenticationException to support setting any kind of response headers or more specifically a WWW-Authenticate header.
I myself prefer to the latter solution: extend the AuthenticationException with the possiblity to set/add a (WWW-Authenticate) response header(s).

allanbrohansen commented Aug 1, 2016

(sorry for the late reply - holiday)
One possible implementation would be to have a specific interceptor class for the basic http autentification (much like I was forced to do). This class should then have an overrideable implementation of String getRealm which the interceptor class would use.
Another way would be to extend the AuthenticationException to support setting any kind of response headers or more specifically a WWW-Authenticate header.
I myself prefer to the latter solution: extend the AuthenticationException with the possiblity to set/add a (WWW-Authenticate) response header(s).

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Aug 2, 2016

Owner

Ah, I like the second idea a lot. I am going to add some code to allow any BaseServerResponseException to have arbitrary headers attached, and then add a method to AuthenticationException that allows you to specify the realm.

Owner

jamesagnew commented Aug 2, 2016

Ah, I like the second idea a lot. I am going to add some code to allow any BaseServerResponseException to have arbitrary headers attached, and then add a method to AuthenticationException that allows you to specify the realm.

@jamesagnew jamesagnew closed this in 545b359 Aug 2, 2016

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Aug 2, 2016

Owner

Thanks for the suggestion!

I've added a method to AuthenticationException that lets you add a custom realm, and you can now add any other arbitrary headers to the response you want as well.

Owner

jamesagnew commented Aug 2, 2016

Thanks for the suggestion!

I've added a method to AuthenticationException that lets you add a custom realm, and you can now add any other arbitrary headers to the response you want as well.

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