On Thu, Apr 21, 2016 at 8:51 PM arjan tijms <arjan.tijms_at_gmail.com> wrote:
> 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.
>
Yes, SUCCESS and FAILURE is pretty clear. I have no opinion on
SEND_CONTINUE vs IN_PROGRESS other than that IN_PROGRESS maybe sounds a bit
more general than SEND_CONTINUE.
>
>
>
>>
>>
>>>
>>> 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.
>
I think the doXXX naming pattern is ok for template methods, such as
Filter's doFilter (called from filter())
If I have understood correctly this isn't the case here, so authenticate
would be a better name than doAuthenticate.
>
>
>
>>
>>>
>>> 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.
>
If most implementations will have use for the context, which I suspect, it
makes more sense to actually have it as a method parameter. Do we need the
request/response to be available from the context, or just leave it to the
implementer to inject it if needed?
@ApplicationScoped
public class TokenAuthenticationMechanism implements
HttpAuthenticationMechanism {
private final static Pattern tokenPattern =
compile("OmniLogin\\s+auth\\s*=\\s*(.*)");
@Inject
private IdentityStore identityStore;
@Inject
private HttpServletRequest request;
@Override
public AuthStatus validateRequest(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();
}
}
One case we may need to provide it through the context is if we manipulate
it, or wrap it in some way earlier.
>
>
>
>>
>>
>>> 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.
>
Thanks, to me it makes sense to keep the exception then.
>
>
>
>> 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 ;)
>
Super clear :)
I was not thinking specifically of the servlet filter mechanism, but more
in the direction of something like the ContainerResponseFilter in JAX-RS,
but it is probably the same issue there.
>
> Thanks again for the feedback, it's really appreciated!
>
> Kind regards,
> Arjan Tijms
>
>
>
>
>