Check that JASPIC authentication failure returns 403 instead of empty

javaee-samples / javaee7-samples

Java EE 7 Samples

https://travis-ci.org/javaee-samples/javaee7-samples

Other

2.51k stars 1.66k forks source link

Check that JASPIC authentication failure returns 403 instead of empty #372

Closed stoty closed 7 years ago

stoty commented 7 years ago

200 response

At least Wildfly and Payara return empty 200 reponses when JASPIC returns an unauthenticated user for a protected page. TomEE returns a 403 response.

I believe that an empty 200 response in case of an authorization failure is not correct behaviour, regardless of the authentication mechanism.

stoty commented 7 years ago

Actually SAM in this test is broken, so the correct behaviour of the server is undefined. There is no correct behaviour to test for.

arjantijms commented 7 years ago

@stoty

Sorry, I missed the earlier PR (have been busy and a lot of mail and notifications to catch up to).

What's specifically broken in the SAM?

stoty commented 7 years ago

NP, It was the holidays anyway.

I've found two problems:

The spec says that it is not compliant to return an empty subject with SUCCESS result when the authentication isMandatory (i.e. no pass through when mandatory). A non 200 status code must be set, and the return value should be SEND_FAIL or SEND_CONTINUE.
This is not strictly in the SAM, but the in the AuthContext: According to the spec and your blog entry a few months back, the AuthContext must initialize the SAM's MessagePolicy.isMandatory() correctly. I think this means that you have to initialize two instances (mandantory, optinal) of the SAM, and call the correct one depending on the MessageInfo mandatory map entry.

I plan to submit a PR with these changes this week.

arjantijms commented 7 years ago

@stoty

Thanks again for your comments, yes you are right for both cases.

The first case was initially to somewhat simplify the SAM as I didn't (yet) test for that path at the time, but now it should of course be included.

The second case has the caveat that even if you do implement it that way, it will likely not change anything, as no JASPIC implementation I know of actually checks for the MessagePolicy. Even GlassFish/Payara does not check it (it does set it). The MessagePolicy was perhaps quite useful for the SOAP profile, but for the Servlet Container one not quite so (we already have the MessageInfo map for per request directives).

Still, as the MessagePolicy initialisation is hidden from the SAM it would not hurt to implement it anyway. I think that the JaspicUtils#registerSam method should then be changed to take a SAM Class instead of an instance though, so that you can easily create those two instances.