Hi Arjan,
See my comments inlined.
Ivar
On Wed, Apr 20, 2016 at 8:57 PM arjan tijms <arjan.tijms_at_gmail.com> wrote:
> Hi,
>
> A while ago I drafted an initial interface for the authentication
> mechanism. There hasn't been that much direct feedback on this yet,
> specifically concerning the method signatures.
>
> Darran had a great proposal concerning the multi-phased mechanisms, which
> I really like to investigate next, but for now some feedback on the single
> phase interface would be great.
>
> To reiterate, this is the current interface:
>
> public interface HttpAuthenticationMechanism {
>
> /**
> * Authenticate an HTTP request.
> *
> * <p>
> * This method is called in response to an HTTP client request for a
> resource, and is always invoked
> * <strong>before</strong> any {_at_link Filter} or {_at_link HttpServlet}.
> Additionally this method is called
> * in response to {_at_link
> HttpServletRequest#authenticate(HttpServletResponse)}
> *
> * <p>
> * Note that by default this method is <strong>always</strong> called
> for every request, independent of whether
> * the request is to a protected or non-protected resource, or whether
> a caller was successfully authenticated
> * before within the same HTTP session or not.
> *
> * <p>
> * A CDI/Interceptor spec interceptor can be used to prevent calls to
> this method if needed.
> * See {_at_link AutoApplySession} and {_at_link RememberMe} for two
> examples.
> *
> * @param request contains the request the client has made
> * @param response contains the response that will be send to the
> client
> * @param httpMessageContext context for interacting with the container
> * @return the completion status of the processing performed by this
> method
> * @throws AuthException when the processing failed
> */
> AuthStatus validateRequest(HttpServletRequest request,
> HttpServletResponse response, HttpMessageContext httpMessageContext) throws
> AuthException;
>
> /**
> * Secure the response, optionally.
> *
> * <p>
> * This method is called to allow for any post processing to be done
> on the request, and is always invoked
> * <strong>after</strong> any {_at_link Filter} or {_at_link HttpServlet}.
> *
> * <p>
> * Note that this method is only called when a (Servlet) resource has
> indeed been invoked, i.e. if a previous call
> * to <code>validateRequest</code> that was invoked before any {_at_link
> Filter} or {_at_link HttpServlet} returned SUCCESS.
> *
> * @param request contains the request the client has made
> * @param response contains the response that will be send to the
> client
> * @param httpMessageContext context for interacting with the container
> * @return the completion status of the processing performed by this
> method
> * @throws AuthException when the processing failed
> */
> default AuthStatus secureResponse(HttpServletRequest request,
> HttpServletResponse response, HttpMessageContext httpMessageContext) throws
> AuthException {
> return SEND_SUCCESS;
> }
>
> /**
> * Remove mechanism specific principals and credentials from the
> subject and any other state the mechanism
> * might have used.
> *
> * <p>
> * This method is called in response to {_at_link
> HttpServletRequest#logout()} and gives the authentication mechanism
> * the option to remove any state associated with an earlier
> established authenticated identity. For example, an
> * authentication mechanism that stores state within a cookie can send
> remove that cookie here.
> *
> * @param request contains the request the client has made
> * @param response contains the response that will be send to the
> client
> * @param httpMessageContext context for interacting with the container
> */
> default void cleanSubject(HttpServletRequest request,
> HttpServletResponse response, HttpMessageContext httpMessageContext) {
> httpMessageContext.cleanClientSubject();
> }
>
> }
>
> See:
> https://github.com/javaee-security-spec/soteria/blob/master/api/src/main/java/javax/security/authentication/mechanism/http/HttpAuthenticationMechanism.java
>
> There are some points to consider here:
>
> 1. AuthStatus return value
>
> I quickly took the existing JASPIC status here, but only for the quick
> prototype. I'd rather see an enum here, with eg the following values:
>
> SUCCESS, FAILURE, IN_PROGRESS
>
> I got feedback off-list that a boolean return (true = success, false =
> failure) could work as well. I investigated this, and indeed all existing
> servers that I investigated only look at "outcome == success". But, for the
> declarative interceptors (such as the login to continue one) it does appear
> to be needed to have IN_PROGESS (now called SEND_CONTINUE).
>
Enums give us more flexibility, so +1 for that.
>
> 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.
>
>
> 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.
> 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? It feels a little hard to force app
developers to catch the exception if it does not have an explicit purpose?
>
> 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?
I think it is better to leave the method out and add it later if it really
is needed, even if it is a default method.
> Kind regards,
> Arjan Tijms
>
>