Skip to content
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

#79 Added wrappers for HttpAuthenticationMechanism and IdentityStore #80

Merged
merged 1 commit into from
Jun 14, 2018
Merged

#79 Added wrappers for HttpAuthenticationMechanism and IdentityStore #80

merged 1 commit into from
Jun 14, 2018

Conversation

arjantijms
Copy link
Contributor

Signed-off-by: arjantijms [email protected]

@ggam
Copy link
Contributor

ggam commented Jun 5, 2018

Change is good but could you add some example? This makes total sense for the CDI case you mentioned on the related issue, but it's not obvious at first glance.

@arjantijms
Copy link
Contributor Author

The easiest example is perhaps this one (from the blog and from EE 8 samples):

 // Note that we provide an extra wrapper since unfortunately createInterceptedInstance
        // says: 
        // "If the provided instance is an internal container construct (such as client proxy), non-portable behavior results."
        return interceptionFactory.createInterceptedInstance(
            new HttpAuthenticationMechanismWrapper(mechanism));

An other example is sub-classing the wrapper, the providing additional behaviour for one method:

public class MyMechanism extends HttpAuthenticationMechanismWrapper {

    public MyMechanism(HttpAuthenticationMechanism mechanism) {
        super(mechanism);
    }

    public AuthenticationStatus validateRequest(HttpServletRequest request, HttpServletResponse response, HttpMessageContext httpMessageContext) throws AuthenticationException {
        System.out.println("ValidateRequest for ...");
        
        return getWrapped().validateRequest(request, response, httpMessageContext);
    }
}

The sky is the limit really on what an individual developer may want to do. Note that Servlet and JSF have very similar wrappers for many of their artefacts.

See e.g. https://arjan-tijms.omnifaces.org/p/jsf-22.html#001

@ggam
Copy link
Contributor

ggam commented Jun 6, 2018

The idea of the wrapper seems right to me, and your blog shows a very clever and interesting trick. I just wonder if users reading the API will figure out the usecases.

I mean, the HttpServletRequestWrapper from Servlet has an obvious case for wrapping in filters. Wrappers in JSF are to be registered through faces-config.xml. As a user, I clearly see how to use it.

In this case, I can't see a direct use case just by looking at the API, which doesn't mean it isn't useful (you already proved it is). But as a user, if I needed to customize a built-in HttpAuthenticationMechanism, I would probably define a decorator (which is btw what JSF and Servlet wrappers facilitate), or I would create a new one from scratch, which I probably won't need to wrap. A simple sentence in the JavaDoc...

This can be used, for example, to add custom interceptors to a built-in HttpAuthenticationMechanism via CDI InterceptorFactory.

...already provides me enough information to search for.

Hope my point is clearer now. I'm fine with the proposed changes anyway. Hopefully others will have an opinion on this too.

@arjantijms
Copy link
Contributor Author

Hope my point is clearer now. I'm fine with the proposed changes anyway. Hopefully others will have an opinion on this too.

Ah, ok, I get it. Probably the best thing would be to add the explanation about dynamically adding something like @RememberMe to the JavaDoc then.

@keilw keilw merged commit 017f88c into jakartaee:master Jun 14, 2018
@arjantijms arjantijms deleted the 79-wrappers branch July 4, 2018 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants