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

Add new maven profile for IBM Liberty Profile #263

Open johnament opened 9 years ago

paulben commented 8 years ago

@arjantijms

Yep, that's a bug, which surprised me as we have a test for programmatic authenticate, but obviously the test has a hole in it. We'll try to have a fix in the next beta.

I saw your comment in the servlet re GlassFish and JBoss authenticate. I'll keep that in mind for this fix.

Paul

arjantijms commented 8 years ago

@paulben

Thanks, looking forward to the next beta ;)

arjantijms commented 8 years ago

@notatibm @paulben

I managed to make CDI work inside a SAM by activating the request scope early on Liberty beta 2016.8.

In fact, currently CDI actually already does work from within a SAM, but only for the application scope and other scopes that do not depend on the current request.

The code I used is this one:

From within the validateRequest method of a SAM:

Object weldInitialListener = request.getServletContext().getAttribute("org.jboss.weld.servlet.WeldInitialListener");
ServletRequestEvent event = new ServletRequestEvent(request.getServletContext(), request);

ELProcessor elProcessor = new ELProcessor();
elProcessor.defineBean("weldInitialListener", weldInitialListener);
elProcessor.defineBean("event", event);
elProcessor.eval("weldInitialListener.requestInitialized(event)");

Weld already has support for nested invocations to the weldInitialListener, so when Liberty calls the same method later on it's most likely silently ignored. Now clearly this is a hacky solution that I know you can't possibly support officially.

Maybe the best solution until the (EE) spec clearly defines this situation is to provide a switch that Liberty users can use in ibm-application-bnd.xml,

e.g. <servlet throwRequestInitialized="early" /> or something like that?

That would mean the request initialised event would be thrown as early as a request is available, which I guess is approximately just before you call a JACC provider for the first time in a request (in com.ibm.ws.security.authorization.jacc.web.impl.WebSecurityValidatorImpl#checkDataConstraints)

What do you think?

paulben commented 8 years ago

@arjantijms @notatibm

Arjan, Sure we can look into doing something along those lines. ATM thinking we'd like to keep it JASPIC specific rather than a general servlet / ibm-application-bnd thing.

Also I have the authenticate tests working, should be in the next beta.

Paul

arjantijms commented 8 years ago

@paulben @notatibm

JASPIC specific should work I guess. If people need CDI in their security modules they can use JASPIC then, and if they don't need it they can use either JASPIC or the IBM specific artefacts at their choice.

Only extra decision point would be whether to activate or not before the first JACC check (which is typically for the WebUserDataPermission permission). Depending on how Liberty's request processing pipeline is setup internally this may already happen automatically (like what happens in JBoss or GlassFish/Payara).

Also I have the authenticate tests working, should be in the next beta.

Great, I'll be curious to see if you went with the "Jboss behavior" or the "GlassFish behavior" wrt the return value. Looking forward anyway to it. Thanks!

arjantijms commented 8 years ago

@paulben @notatibm

We've also included Liberty as a CI test target in the JSR 375 RI (Soteria). It does work against 16.0.2 and not the latest beta, since those latest betas do not seem to be available in Maven central (I asked on Twitter as well).

The tests can be started as follows:

git clone https://github.com/javaee-security-spec/soteria.git cd soteria mvn clean install -P liberty -P bundled

A lot of them pass already, but for the BASIC authentication mechanism there's an exception that I need to look into still:

java.lang.NullPointerException: 
   at javax.xml.bind.DatatypeConverter.parseBase64Binary(DatatypeConverter.java:97)
   at org.glassfish.soteria.mechanisms.BasicAuthenticationMechanism.getCredentials(BasicAuthenticationMechanism.java:106)
   at org.glassfish.soteria.mechanisms.BasicAuthenticationMechanism.validateRequest(BasicAuthenticationMechanism.java:80)
   at org.glassfish.soteria.mechanisms.BasicAuthenticationMechanism$Proxy$_$$_WeldClientProxy.validateRequest(Unknown Source)
   at org.glassfish.soteria.mechanisms.jaspic.HttpBridgeServerAuthModule.validateRequest(HttpBridgeServerAuthModule.java:106)
   at org.glassfish.soteria.mechanisms.jaspic.DefaultServerAuthContext.validateRequest(DefaultServerAuthContext.java:75)
   at com.ibm.ws.security.jaspi.JaspiServiceImpl.authenticate(JaspiServiceImpl.java:399)
   at [internal classes]

Looks like the JDK javax.xml.bind.DatatypeConverter.parseBase64Binary(String) for some reason doesn't work in Liberty, but works on at least JBoss and GlassFish.

NottyCode commented 8 years ago

@arjantijms we don't put the betas in maven central, we would want to be able to remove them and maven doesn't really allow for removal.

arjantijms commented 8 years ago

@notatibm That makes sense. Wouldn't there be another maven repo (e.g. IBM specific one) where it could be downloaded from? Or the sonatype snapshot repo?

NottyCode commented 8 years ago

@arjantijms not that contains them. We used to release our maven artifacts into an IBM registry, but it was basically just an http website, rather than an actual repository, so didn't really work very well for people. I wouldn't want to reuse it since peoples expectations are that things from there do not go away. We could replicate, but we haven't had a lot of demand. Although you are the second person to ask recently so perhaps we should consider it.

arjantijms commented 8 years ago

Although you are the second person to ask recently so perhaps we should consider it.

Okay, thanks for at least considering it then ;)

arjantijms commented 7 years ago

@paulben

Also I have the authenticate tests working, should be in the next beta.

Just to be sure, that next beta is not 2016.9, or is it?

I tried that one as well as the stable 16.0.0.3, but request.authenticate in both cases brings up the BASIC dialog again instead of going to the SAM.

Just asking in case it should be in there and I'm doing something wrong somehow.

paulben commented 7 years ago

@arjantijms

No, you're not doing anything wrong. Sorry for the confusion, I was thinking next in terms of our internal cutoff schedule. I should have said the beta after next. The fix was too late to make 2016.9, it will be in 2016.10.

Paul

arjantijms commented 7 years ago

@paulben

Ah, ok. No worries, thanks for the clarification ;)

arjantijms commented 7 years ago

@paulben

I think I discovered another bug in Liberty and JASPIC. It concerns the request wrapping. I was investigating erratic behaviour in Liberty with seemingly random null pointer exceptions.

As it turned out, Liberty is storing the request (and response) wrappers set by a SAM in the global application scope as servlet context attributes (using com.ibm.ws.security.jaspi.servlet.request.wrapper and com.ibm.ws.security.jaspi.servlet.response.wrapper as keys).

E.g. in a Servlet you can do the following:

request.getServletContext().getAttribute("com.ibm.ws.security.jaspi.servlet.request.wrapper");

It seems that the wrapper stored here is automatically applied to each request, but this seems incorrect. The wrapper should only be applied to the request in which it was set. The result is that if a delegating wrapper is used (that delegates to the original Liberty request), things seem to be totally corrupted in those other requests. Even worse, since it's stored in the Servlet context it's already potentially seen by other requests while the request in which authentication took place is still running.

paulben commented 7 years ago

@arjantijms

Indeed, I wasn't thinking of different behavior per request. I'll look into request scoping it.

Paul

arjantijms commented 7 years ago

@paulben

Okay, thanks for looking into making it request scoped.

Btw, from the stack in a debugger it can be seen that a filter called JaspiServletFilter is setting that wrapped request and response. However, this Filter seems to be executed as the last Filter, not as the absolute first.

What happens now is that if a user supplied Filter also wraps the request, JaspiServletFilter totally discards that and sets the SAM supplied request.

What should happen I think is that the very first Filter in the chain sees the request that the SAM wrapped, so it gets a chance to wrap that, and other Filters down the chain can continue wrapping it until it arrives at the target Servlet.

A complication here is that using the standard Servlet API it's not really possible AFAIK to absolutely guarantee a given Filter is the very first to be executed. All other servers therefor do something special (container specific) to set the wrapped request and response.

paulben commented 7 years ago

@arjantijms

Got the wrappers request scoped, will have to investigate a bit on the wrapping/filter order.

Paul

paulben commented 7 years ago

@arjantijms

Hi Arjan, Hopefully will have the request scoped wrapper fix in 2016.11 beta. Pre EE8 CDI and filter ordering issues still being worked.

Paul

arjantijms commented 7 years ago

@paulben

Hi Paul, sounds good, thanks for the update!

To help with the filtering issues, I extended the wrapping test here: https://github.com/javaee-samples/javaee7-samples/tree/master/jaspic/wrapping

On all servers I tested this on (Tomcat, JBoss, Payara, ...) the result is:

programmatic filter request isWrapped: true
programmatic filter response isWrapped: true
declared filter request isWrapped: true
declared filter response isWrapped: true
servlet request isWrapped: true
servlet response isWrapped: true

For Liberty however it's:

programmatic filter request isWrapped: null
programmatic filter response isWrapped: null
declared filter request isWrapped: null
declared filter response isWrapped: null
servlet request isWrapped: true
servlet response isWrapped: true

Hope this test helps and thanks again for all your work on this.