Hi,
Thanks for the feedback, comments also inlined.
On Thu, Apr 21, 2016 at 7:44 PM, Ivar Grimstad <ivar.grimstad_at_gmail.com>
wrote:
>
>
> Enums give us more flexibility, so +1 for that.
>
Okay, I too think this is the better option. If anyone has an opinion on
the naming, that would be helpful too. SUCCESS and FAILURE seem clear
enough, but SEND_CONTINUE vs IN_PROGRESS vs something else may need some
thinking.
>
>
>>
>> 2. Method names
>>
>> I took validateRequest, secureResponse and cleanSubject, because those
>> are the names uses by the ServerAuthModule (SAM) interface.
>>
>> I got feedback here that better names may the ones that reflect the
>> corresponding HttpServletRequest and soon SecurityContext methods. E.g.:
>>
>> validateRequest() -> authenticate() or authenticateRequest()
>> cleanSubject() -> logout()
>>
>
> +1
> Makes them more understandable.
>
Okay, here too if anyone else has name suggestion that would be helpful. An
additional name suggestion for validateRequest would be "doAuthenticate"
(in analogy to a Filter's doFilter() method).
One thing I did just realize is that the IdentityStore main method is
called "validate". I'm not sure if it was intended by Alex (since the
IdentityStore was proposed first),
but HttpAuthenticationMechanism#validateRequest calling
IdentityStore#validate does have some consistency as well.
cleanSubject() is almost certainly a bit unclear and logout() seems to be
the best option.
>
>>
>> 3. Method parameters
>>
>> The methods name have request, response and the http message context as
>> parameters. The request and response are for convenience, and for
>> similarity to Servlet Filters. They don't necessarily need to be passed in,
>> as they can also be obtained from the context.
>>
>> Alternatively, the methods could all have 1 parameter (the context) or
>> even zero (depend on the context being injected).
>>
>> I personally feel 3 convenience parameters is likely the easiest for the
>> user, but I'd like to have some feedback here.
>>
>
> I think it is a better option to keep the method parameters to a minimum
> and depend on injections.
>
So then something like:
@ApplicationScoped
public class TokenAuthenticationMechanism implements
HttpAuthenticationMechanism {
@Inject
private IdentityStore identityStore;
@Override
public AuthStatus validateRequest(HttpServletRequest request,
HttpServletResponse response, HttpMessageContext httpMessageContext) throws
AuthException {
String token = getToken(request);
if (!isEmpty(token)) {
CredentialValidationResult result = identityStore.validate(new
TokenCredential(true, null, token));
if (result.getStatus() == VALID) {
return httpMessageContext.notifyContainerAboutLogin(
result.getCallerPrincipal(),
result.getCallerGroups());
}
}
if (httpMessageContext.isProtected()) {
return httpMessageContext.responseNotFound();
}
return httpMessageContext.doNothing();
}
}
Would then become:
@ApplicationScoped
public class TokenAuthenticationMechanism implements
HttpAuthenticationMechanism {
private final static Pattern tokenPattern =
compile("OmniLogin\\s+auth\\s*=\\s*(.*)");
@Inject
private IdentityStore identityStore;
@Inject
private HttpMessageContext httpMessageContext;
@Override
public AuthStatus validateRequest() throws AuthException {
String token = getToken(httpMessageContext.getRequest());
if (!isEmpty(token)) {
CredentialValidationResult result = identityStore.validate(new
TokenCredential(true, null, token));
if (result.getStatus() == VALID) {
return httpMessageContext.notifyContainerAboutLogin(
result.getCallerPrincipal(),
result.getCallerGroups());
}
}
if (httpMessageContext.isProtected()) {
return httpMessageContext.responseNotFound();
}
return httpMessageContext.doNothing();
}
}
So the advantage is that the method signature is shorter and maybe
that HttpMessageContext
can be decorated, but the user now also has to read and learn what to do
in validateRequest(), and then discover that the getRequest() on the
context gives the request.
>
>
>> 4. Checked exception
>>
>> Some methods now declare the checked AuthException. This is again a type
>> I quickly took over from JASPIC. Do we really need it?
>>
>
> In which situations will it be thrown?
>
Basically when something went wrong with authentication, and the mechanism
does not want or can't provide a response. Loosely interpreted, I *think*
when the mechanism wants to depend on the servers native error/forbidden
page.
This is description in the JSR 196 spec:
"throw an AuthException – If the request validation failed, and when the
client should not retry the request, *and when the module has not defined a
response to be sent by the runtime*. If the runtime chooses to send a
response, it must define the HTTP status code and descriptive content (of
the response).
The HTTP status code of the response must indicate to the client (for
example, HTTP 403 forbidden, HTTP 404 not found, or HTTP 500 internal
server error) that the request failed and that it should NOT be retried.
The descriptive content set in the response may be obtained from the
AuthException."
(** emphasis mine)
Of course, it could just as well be another status, or we could forgo this
feature altogether.
> It feels a little hard to force app developers to catch the exception if
> it does not have an explicit purpose?
>
That should not really be a problem, as the authentication mechanism will
never be directly called by an app developer, but only by the server (just
like e.g. an app developer does not call a Servlet or a Filter instance).
>
> 5. The secureResponse method
>>
>> The secureResponse method gives a mechanism the option to do something
>> after the response. As its name indicates, securing (maybe encrypting) the
>> response. Of course this can only work when the response was wrapped before
>> by a buffering one. I.e. this roughly works like a GZIP filter would do its
>> work.
>>
>> In practice I've seen very few authentication mechanisms use this, so
>> that may justify removing the method.
>>
>> On the other hand, being a default method in the interface it has a very
>> low impact on complexity to support and almost zero impact for the user.
>>
>
> Is this achievable in some other way? Some response filter or something?
>
A response filter would work in some cases indeed, but it's not "balanced"
with the authentication mechanism. With that I mean that validateRequest()
is called before any Filter is called, and secureResponse() after. With a
Filter the application developer should make sure that the Filter is
absolutely the last one, and have them work together. As illustration, what
happens now is:
t=1 Mechanism#validateRequest
t=2 Filter 1 #doFilter
t=3 Filter 2 #doFilter
t=4 Filter 2 after chain
t=5 Filter 1 after chain
t=6 Mechanism#secureResponse
Hope the little ascii drawing is clear ;)
Thanks again for the feedback, it's really appreciated!
Kind regards,
Arjan Tijms