javaee7-samples icon indicating copy to clipboard operation
javaee7-samples copied to clipboard

Add new maven profile for IBM Liberty Profile

Open johnament opened this issue 11 years ago • 69 comments

johnament avatar Oct 06 '14 01:10 johnament

The Liberty profile beta supports many Java EE 7 technologies with Servlet 3.1 and Websockets 1.0 being the most complete. At this time neither are 100% CTS compliant although we are making progress with each of our beta updates. The beta is updated roughly every 4 weeks. To get these features both need to be configured into the server. The default server template configures for servlet-3.0 and jsp-2.2 only.

The Java Servlet 3.1, Concurrency Utilities for Java EE 1.0, Java Websocket API 1.0, and JSON Parsing 1.0 will be released on December 9th 2014. To get these capabilities the server.xml needs to be updated to add:

<featureManager>
    <feature>concurrent-1.0</feature>
    <feature>jsonp-1.0</feature>
    <feature>servlet-3.1</feature>
    <feature>websocket-1.0</feature>
</featureManager>

We have run these samples against Liberty (although not using the maven plugin hence no contribution). @arun-gupta tried to run some of the samples on an earlier beta where the Liberty profile did not process the servlet 3.1 deployment descriptor schema. Liberty profile currently supports the servlet 3.1 deployment descriptor schema provided the servlet 3.1 feature is configured.

At the time this was written based on the Liberty profile beta the following samples can run:

Java Batch 1.0 batch.batch-listeners, batch.batchlet-simple, batch.chunk-checkpoint, batch.chuck-csv-database, batch.chunk-exception, batch.chunk-optional-processor, batch.chunk-simple-nobeans, batch.chunk-simple, batch.decision, batch.flow, batch.multiple-steps, batch.split

Concurrency Utilities for Java EE 1.0 concurrency.dynamicproxy, concurrency.managedscheduledexecutor, concurrency.managedthreadfactory

Enterprise JavaBeans 3.2 ejb.async-ejb, ejb.singelton

Java API for XML RESTful Services 2.0 jaxrs.async-client, jaxrs.beanparam, jaxrs.beanvalidation, jaxrs.dynamicfilter, jaxrs.fileupload, jaxrs.filter-itnerceptor, jaxrs.interceptor, jaxrs.invocation, jaxrs.invocation-async, jaxrs.jaxrs-endpoint, jaxrs.mapping-exceptions, jaxrs.moxy, jaxrs.readwriter, jaxrs.readwriter-json, jaxrs.request-binding, jaxrs.singleton

Java Persistence Architecture 2.1 jpa.criteria, jpa.datasourcedefinition, jpa.datasourcedefinition-annotation-pu, jpa.datasourcedefinition-webxml-pu, jpa.dynamic-named-query, jpa.entitygraph, jpa.extended-pc, jpa.jndi-context, jpa.jpa-converter, jpa.listeners, jpa.listeners-injection, jpa.locking-optimistic, jpa.locking-pessimistic, jpa.native-sql, jpa.native-sql-resultset-mapping, jpa.pu-typesafe, jpa.schema-gen-metadata, jpa.schema-gen-scripts, jpa.schema-gen-scripts-external, jpa.schema-gen-scripts-generate

JSON Parsing 1.0 json.object-builder, json.object-reader, json.streaming-generate, json.streaming-parser

Java Servlet 3.1 servlet.async-servlet, servlet.cookies, servlet.error-mapping, servlet.event-listeners, servlet.nonblocking, servlet.web-fragment

Java Websocket 1.0 websocket.endpoint-config, websocket.endpoint-javatypes, websocket.endpoint-partial, websocket.endpoint-programmatic-async, websocket.endpoint-programmatic-config, websocket.endpoint-programmatic-injection, websocket.endpoint-programmatic-partial, websocket.endpoint-singleton, websocket.httpsession, websocket.messagesize, websocket.parameters, websocket.properties, websocket.subprotocol

Anything not listed has either not been run by us, or it ran with errors.

As noted by @aslakknutsen the WebSphere Application Server for Developers license doesn’t include use on a build server, but it does allow people to run these samples and associated tests on a developer desktop so I don’t think that restriction should preclude the creation of a profile for Liberty profile.

NottyCode avatar Oct 06 '14 16:10 NottyCode

@notatibm Thanks a lot for detailed summary!

Will you be adding a profile ?

arun-gupta avatar Oct 06 '14 16:10 arun-gupta

although not using the maven plugin hence no contribution

@notatibm Didn't Arquillian work with Liberty or was there another issue? It looks like there IS an Arquillian container for Liberty available (see http://arquillian.org/modules/arquillian-wlp-managed-8.5-container-adapter)

PS if/when Liberty is adding support for JASPIC and you have issues with the JASPIC tests, please let me know if I can help (you can contact me privately if needed). I executed a Java EE 6 variant of the tests against WebSphere and initially nothing passed. But when I added the user and groups that the JASPIC tests use to some internal repository of WebSphere many tests did pass (see http://arjan-tijms.omnifaces.org/2012/11/implementing-container-authentication.html for more details).

arjantijms avatar Oct 06 '14 21:10 arjantijms

@arun-gupta I'd love to do it, but I can't do it right now so if someone else chooses to pick it up in the meantime that is fine by me.

@arjantijms There is an Arquillian plugin for Liberty profile I think what @arun-gupta is looking for is the maven updates to enable that for these samples. I'll do my best to remember to contact you about JASPIC.

NottyCode avatar Oct 06 '14 21:10 NottyCode

@notatibm @arun-gupta

I just added a PR that adds the mentioned Arquillian container for Liberty to pom.xml. See https://github.com/javaee-samples/javaee7-samples/pull/299

As discussed on SO the JASPIC samples had to be adjusted to wrap the war that's normally used for testing in a dummy EAR to satisfy the somewhat unfortunate requirement that Liberty has that a group to role mapping file can only be stored in an EAR.

In order to run the JASPIC tests a cheat had to be added via a user repository that contains exactly the user and group that's used by the JASPIC tests. In reality Liberty fails 100% of the JASPIC tests, but with this cheat in place at least the tests are actually attempted. For real JASPIC usage a special noop user registry would have to be installed (one that simply validates every user and group). I have in fact created this, but I'm not entire sure how to reference such a feature from Maven (it's an .esa archive) and install it to Liberty from the tests.

Unfortunately quite a few JASPIC tests fail, but of course Liberty is still in beta.

The following tests failed:

  • testPublicPageNotRememberLogin (org.javaee7.jaspic.basicauthentication.BasicAuthenticationPublicTest)
  • testPublicPageLoggedin (org.javaee7.jaspic.basicauthentication.BasicAuthenticationPublicTest)
  • testProtectedAccessIsStateless (org.javaee7.jaspic.basicauthentication.BasicAuthenticationStatelessTest)
  • testPublicServletWithLoginCallingEJB (org.javaee7.jaspic.ejbpropagation.ProtectedEJBPropagationTest) testProtectedServletWithLoginCallingEJB (org.javaee7.jaspic.ejbpropagation.ProtectedEJBPropagationTest)
  • testProtectedServletWithLoginCallingEJB (org.javaee7.jaspic.ejbpropagation.PublicEJBPropagationLogoutTest)
  • testProtectedServletWithLoginCallingEJB(org.javaee7.jaspic.ejbpropagation.PublicEJBPropagationTest)
  • testLogout(org.javaee7.jaspic.lifecycle.AuthModuleMethodInvocationTest) SAM method cleanSubject not called, but should have been
  • testJoinSessionIsOptional (org.javaee7.jaspic.registersession.RegisterSessionTest)
  • testRemembersSession (org.javaee7.jaspic.registersession.RegisterSessionTest)
  • testResponseWrapping (org.javaee7.jaspic.wrapping.WrappingTest) Response wrapped by SAM did not arrive in Servlet.
  • testRequestWrapping(org.javaee7.jaspic.wrapping.WrappingTest) Request wrapped by SAM did not arrive in Servlet.

Specifically the EJB, register session (new JASPIC 1.1 feature) and request/response wrapper tests completely failed (all tests in those sections failed).

arjantijms avatar Apr 19 '15 23:04 arjantijms

One thing to consider, as mentioned by @notatibm

the WebSphere Application Server for Developers license doesn’t include use on a build server

Would it be an idea to ask special permission for this?

it does allow people to run these samples and associated tests on a developer desktop

Alternatively, someone could run the tests for Liberty periodically on a desktop and publish the test results.

What do you think?

arjantijms avatar Apr 21 '15 15:04 arjantijms

@arjantijms since I made that comment we updated the license of the version on WASdev.net that would allow use on a build server, provided the heap is under 2Gb. The actual limitation is no more than 2Gb per organization, so if you ran 2 builds at the same time it would need to be 1Gb each build. Also not sure in this case what the organization would be. I don't know if that helps.

IANAL

NottyCode avatar Apr 21 '15 19:04 NottyCode

@notatibm right, I almost forgot about that. 2GB would be more than enough I guess and I don't think @arun-gupta does concurrent builds on the same server.

IANAL either, but it sounds like it should not be an issue indeed ;)

P.s. what do you think about the results from the JASPIC test?

arjantijms avatar Apr 21 '15 21:04 arjantijms

@arjantijms, We're looking into the JASPIC test failures you highlighted.

kwsutter avatar Apr 22 '15 14:04 kwsutter

@arjantijms have you implemented your own JASPIC provider and configured it as a Liberty product extension feature? We only provide the support to plug your own JASPIC provider and do not ship one.

arkarkala avatar Apr 22 '15 15:04 arkarkala

@arkarkala for the tests nothing was configured as a Liberty product extension feature, although I'm not 100% sure what you mean with a JASPIC provider. It sounds like there may be a confusion with a JACC provider, which is indeed something you don't seem to ship (I'm pretty sure this is a spec violation as well, but that's an entirely different story).

For this case it's only about JASPIC. JACC is not involved.

For the test run, Liberty was downloaded from the Liberty beta page, and server.xml configured as indicated here: https://github.com/javaee-samples/javaee7-samples/blob/master/README.md

In order to by pass a spec violation in Liberty where the callback handler tries to validate users and groups with a user registry, a basicRegistry was configured in server.xml that contains exactly the users and groups used by the test. This itself should not be needed, but without it nothing at all runs.

The exact details about the Server Auth Module (SAM) that was used is in the source code of the tests, but as a summary:

  • An application provided SAM is registered by each test case using the following code: https://github.com/javaee-samples/javaee7-samples/blob/master/jaspic/common/src/main/java/org/javaee7/jaspic/common/JaspicUtils.java#L23
  • Part of the registration process consists of an application provided auth config provider, auth config and auth context
  • Example of an application provided SAM: https://github.com/javaee-samples/javaee7-samples/blob/master/jaspic/basic-authentication/src/main/java/org/javaee7/jaspic/basicauthentication/sam/TestServerAuthModule.java

JASPIC itself basically works on Liberty using this spec defined method of installing a SAM. The installed SAM is called at the right moment before the Servlet/filter chain is invoked, but an amount of JASPIC features such as the ability to wrap a request and response apparently do not work.

arjantijms avatar Apr 22 '15 16:04 arjantijms

@arjantijms Yes, I did mean JASPIC (not JACC). But since you are registering your config provider in your code there should not be a need for an extension feature. Not all changes went into Beta - so you might be hitting these. We will investigate.

Regarding the userRegistry (UR) issue, did you have to specify the same user/group (test/architect) or configure a UR with any (dummy) user only - the latter indicating that you need a UR but is not used for authentication?

arkarkala avatar Apr 22 '15 21:04 arkarkala

@arkarkala

I did mean JASPIC (not JACC).

Okay, it's just that the term "JASPIC provider" did not ring a bell, while the term "JACC provider" is quite common.

But since you are registering your config provider in your code there should not be a need for an extension feature.

True, there should not be a need for this. But to work around the spec violation where usernames/groups are being validated one is unfortunately needed for practical purposes.

For the test however there was no extension feature used.

Regarding the userRegistry (UR) issue, did you have to specify the same user/group (test/architect) or configure a UR with any (dummy) user only

Well, given the following server.xml that was modified once with the following UR:

<basicRegistry id="basic">
    <user name="test" password="not needed"/>
    <group name="architect"/>
</basicRegistry>

If you would change the user name to say "test-does-not-exist", then every test fails immediately when the callback is being handled in the auth module. The log mentions something about an unknown user (I'd have to repeat the test to see what the exact exception is).

When I do not provide a UR at all, the tests also fails, but instead of an unknown user exception a null pointer exception is logged.

So my conclusion was that a UR needs to be present AND it actually needs to contain the user and group that the tests use.

In private tests on my workstation I have executed a parallel test run where I did not add the basicRegistry, but instead installed my own custom user registry as an extension feature. Here I could see that Liberty actually calls a couple of validate methods (isValidUser, isValidGroup). When I return "false" from these methods handling the callback does not work, when I return "true" the callback works.

I'm just about to finish a blog post with some details about this custom user registry.

At any length, using either the basic registry with the test/architect username/group or the noop custom registry yields the same results; in both situations the tests that fail and succeed are the same.

arjantijms avatar Apr 22 '15 22:04 arjantijms

@arjantijms , are you seeing the isValid calls made in the latest Beta (April/May) too?

arkarkala avatar Apr 22 '15 22:04 arkarkala

@arkarkala

are you seeing the isValid calls made in the latest Beta (April/May) too?

Yes. The archive I downloaded was wlp-beta-javaee7-2015.4.0.0.zip and when starting the server reported Launching defaultServer (WebSphere Application Server 2015.4.0.0/wlp-1.0.9.20150407-2322

I just finished the article which contains some more details about the extra test with the custom user registry that I've been doing. See http://arjan-tijms.omnifaces.org/2015/04/testing-jaspic-11-on-ibm-liberty-ee-7.html

arjantijms avatar Apr 22 '15 23:04 arjantijms

@arjantijms Can you provide the following trace - com.ibm.ws.security.=all:com.ibm.ws.webcontainer.=all so we can see when these isValid calls are being made?

Majority of our customers do use UR so we made it mandatory. Yes, there are few situations where it is not required (for eg, when one does not use the default authentication AND authorization) and having some dummy values in it would be sufficient - they should not be used. We might make the UR configuration optional to handle these cases sometime in future - you can also open Request For Feature (RFE) - https://www.ibm.com/developerworks/rfe/execute?use_case=changeRequestLanding

There are also some scenarios where a custom authentication is performed (for eg using JASPIC), WAS can do the authorization without consulting the registry. For eg, when all valid (authenticated) users are assigned to the role(s) inside the application-bnd as shown below.

  <security-role name="architect">
          <special-subject type="ALL_AUTHENTICATED_USERS" />
  </security-role>

arkarkala avatar Apr 23 '15 03:04 arkarkala

@arkarkala

Can you provide the following trace - com.ibm.ws.security.=all:com.ibm.ws.webcontainer.=all so we can see when these isValid calls are being made?

I'm on a different computer now, but I'll provide these later today. It should also be easy for yourself to see this. This test repository is set up such that everything is fully reproducible. Nothing in the tests depends on anything that's specific to my environment.

If you follow the instructions from https://github.com/javaee-samples/javaee7-samples/blob/master/README.md you should be able to easily get the exact same results.

Majority of our customers do use UR so we made it mandatory.

The problem here is that the JASPIC specification doesn't allow this. Liberty is not fully Java EE 7 compatible as long as it mandates this.

The spec lead, Ron Monzillo said the following about this:

The behavior attributed to WebSphere 8.5, WRT the processing of the JASPIC CallerPrincipalCallback is NOT compatible with the JASPIC specification.

The CallerPrincipalCallback(s) must be able to support the case where the user registry is integrated within the SAM, including for the purpose of providing user group memberships.

For the special case of password based validation, A SAM can invoke the container provided CallbackHandler to handle a PasswordValidationCallback; in which case the CallbackHandler will return a failure result if the username and/or password combination does not exist in the user registry integrated with the container's CallbackHandler. In that case, the SAM would return a failed (or continuation) authentication result and would NOT invoke the CallbackHandler to handle the CallerPrincipalCallback.

Ultimately the TCK should have flagged this, but unfortunately it didn't.

arjantijms avatar Apr 23 '15 07:04 arjantijms

@arkarkala @notatibm Just wondering, any luck with the JASPIC issues yet?

arjantijms avatar May 04 '15 07:05 arjantijms

Hi Arjan, We've made fantastic progress with the samples... One of the developers will be replying very soon, but he took a couple days of vacation. But, it's looking very good.

Thanks for your patience. Kevin

On Mon, May 4, 2015 at 2:05 AM, Arjan Tijms [email protected] wrote:

@arkarkala https://github.com/arkarkala @notatibm https://github.com/notatibm Just wondering, any luck with the JASPIC issues yet?

— Reply to this email directly or view it on GitHub https://github.com/javaee-samples/javaee7-samples/issues/263#issuecomment-98603977 .

kwsutter avatar May 04 '15 13:05 kwsutter

Hi Arjan,

I've been working on the JASPIC problems you reported on the WebSphere Liberty profile. I have all of the failures that you reported working in the new May beta. I've been using the github/maven test environment as documented here on github.com/javaee-samples/javaee7-samples, which does work as advertised, no problem.

You will see one failure - async-authentication - that I didn't get fixed in the beta. I have fixed it. If you need that fix let me know.

Here are the changes you'll need to make.

  1. You will now only need the following features in server.xml, specifically you no longer need to include jaspic-1.1:

     <featureManager>
        <feature>javaee-7.0</feature>
        <feature>localConnector-1.0</feature>
    </featureManager>
    
  2. Please add the following to your server.xml:

     <webContainer allowQueryParamWithNoEqual="true"/>
    

    The reason for this is that I found a number of failures were caused by a difference between Liberty and other Java EE app servers in the default behavior of the method HttpServletRequest.getParameter(). This method was returning null in your SAMs even though the parameter was specified. I originally resolved this problem by changing your tests to add an "=true" value to the params. i.e.

          getFromServerPath("protected/servlet?doLogin=true")
    

    But the allowQueryParamWithNoEqual property will make the Liberty behavior return non null, allowing you to leave your test servlet invocations unchanged.

  3. In your tests where you check for a null principal you need to change to check for "UNAUTHENTICATED". i.e.:

        assertTrue(response.contains("web username: UNAUTHENTICATED"));
    

    instead of:

        assertTrue(response.contains("web username: null"));
    

    or in order to keep your tests container independent:

       assertTrue(response.contains("web username: null") || 
                  response.contains("web username: UNAUTHENTICATED"));
    

    This is consistent with the JASPIC spec which says the container may "establish the container’s representation of the unauthenticated caller". WebSphere's unauthenticated representation is a Subject with a Principal with name UNAUTHENTICATED. It looks like the other app servers you test against represent an unauthenticated caller with a null Principal name.

  4. For the user registry configuration, all you need to add is the group information in your server.xml as shown below. This is needed because you use our container authorization for the group.

    <basicRegistry id="basic">
       <group name="architect" />
    </basicRegistry>
    

    As you can see no need for the user "test". You will see isValid checks being made but if the user/group does not exist we will use callback values directly.

    We agree that there are some scenarios (those that do not utilize built-in WAS authentication and authorization) where user registry may not be required. We will look into this.

Thanks for your input. It is our intention to make the Liberty profile as easy for individual developers such as yourself to use as Glassfish, Tomcat, Widlfly, etc., while also scaling to the largest enterprise production workloads. We hear you and we appreciate it.

Paul

paulben avatar May 11 '15 19:05 paulben

I have all of the failures that you reported working in the new May beta.

Sounds great, thanks for looking into this!

You will see one failure - async-authentication - that I didn't get fixed in the beta.

That's a peculiar one, I didn't see that one failing in the previous test run I did (the one from https://github.com/javaee-samples/javaee7-samples/issues/263#issuecomment-94323049)

specifically you no longer need to include jaspic-1.1:

That's a great improvement, I noticed this in the release notes of the latest beta that was released last week.

The reason for this is that I found a number of failures were caused by a difference between Liberty and other Java EE app servers in the default behavior of the method HttpServletRequest.getParameter().

I'm very sorry that I didn't mentioned this before. Indeed, I noticed the same thing right away. I glanced over the Servlet spec and if I'm not mistaken this may a bug in Liberty, but I wanted to double check to make sure first and then forgot about it. In my local test I changed this for Liberty just like you did.

This is consistent with the JASPIC spec which says the container may "establish the container’s representation of the unauthenticated caller"

I think this is not entirely correct. I remember talking with Ron Monzillo about this and it really has to be null. Only in EJB it's allowed (almost required really) to return something container specific (which incidentally is something I hope to be able to address in the security EG, but that's for Java EE 8)

The fragment you mentioned is in the Javadoc of the CallerPrincipalCallback, which for convenience is also printed on page 93 of the JASPIC 1.1 spec:

The CallbackHandler must use the argument Principal to establish the caller principal associated with the invocation being processed by the container. When the argument Principal is null, the handler must establish the container’s representation of the unauthenticated caller principal.

So internally, there can be a server specific representation. Which means a specific Principal implementation (e.g. com.ibm.SomePrincipal) or an actual null. A JACC provider would get to see this, and would need to take this into account.

However, for Servlets the HttpServletRequest#getUserPrincipal() method is very clear that a null must be returned if the user has not been authenticated:

Principal getUserPrincipal() Returns a java.security.Principal object containing the name of the current authenticated user. If the user has not been authenticated, the method returns null.

See http://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletRequest.html#getUserPrincipal()

There's a similar requirement for getRemoteUser(), and the requirement is repeated in the description of methods such as isUserInRole() and login().

I do agree that the wording about "container’s representation of the unauthenticated caller principal" can be confusing.

We will look into this.

Would be really great if something can be done here, thanks in advance!

Thanks for your input.

You're welcome, thanks again for looking into this.

arjantijms avatar May 11 '15 20:05 arjantijms

Correct, the async-authentication failure was not there before. It was introduced by my changes to fix the failures you reported. I didn't catch it in time for the beta, but it is fixed now.

We're investigating the getUserPrincipal issue.

paulben avatar May 11 '15 20:05 paulben

Yep, that's a bug in Liberty getUserPrincipal. I coded up a fix and the tests run clean without the UNAUTHENTICATED change.

paulben avatar May 12 '15 01:05 paulben

I coded up a fix and the tests run clean without the UNAUTHENTICATED change.

That's really great news! :) Thanks for your efforts.

Btw, I'm working on a few more tests but those aren't ready yet.

They concern the ability to include/forward to a resource from the SAM (new requirement in JASPIC 1.1) , setting a specific HTTP status code from a SAM and explicitly testing whether the response can still be written to when a SAM's secureResponse() method is called.

arjantijms avatar May 12 '15 07:05 arjantijms

Hi Arjan, Have you been able to get the forward to work on any container? I'm able to get include working on Liberty. But I found that on return from the forward the response object is committed and closed. The spec requires the MessageInfo object to contain the request and response objects that the runtime uses for the web request being processed, and it also requires that those req/res objects be used for the forward, so we're not seeing an easy way around this problem.

The Servlet spec and the Javadoc for RequestDispatcher are very clear about the response being committed and closed on return from forward. It's looking to us like there is a conflict between the requirements of the JASPIC spec and the Servlet spec on this particular item.

Also it seems to us that this usage - doing a forward while processing a request and then, on return from the forward, continuing processing the original request and ultimately invoking the original request target - is not the intended use for a forward. A forward is intended "replace" the original target, with the results of the forward returned to the user agent, which is consistent with the response being committed and closed.

So...we would be interested to know what your experience is with the forward on other containers.

Thanks, Paul

paulben avatar May 14 '15 04:05 paulben

@paulben

Hi Paul,

Have you been able to get the forward to work on any container?

It's been a while since I tested this, but originally (for JASPIC 1.0) it already worked in GlassFish and Geronimo, but failed on JBoss, WebLogic and WebsSphere.

Also it seems to us that this usage - doing a forward while processing a request and then, on return from the forward, continuing processing the original request and ultimately invoking the original request target - is not the intended use for a forward.

Indeed, that's of course not the intended use for a forward.

The main use case is to be able to implement Servlet's native FORM authentication using a portable JASPIC authentication module. Servlet's FORM asks for a forward to a login page and of course doesn't invoke the original target then.

I personally did the proposal for this feature to be included in JASPIC 1.1 and discussed it with Ron Monzillo. See here: https://java.net/jira/browse/JASPIC_SPEC-7

The exact text that ended up in the spec indeed doesn't quite capture the full intend.

You could argue though that the text "Under the constraints defined by RequestDispatcher" indirectly defines that JASPIC indeed does not ask for the possibility to also invoke the original request target after a forward. The constraints for dispatching via a forward (Servlet 3.1, section 9.4) simply do not allow this.

You could perhaps interpret this in the same way as that a Servlet can not forward to two other Servlets either, even though I think the spec does not explicitly say this. It's implied by the specification that says a dispatcher MUST close the response before returning from a forward.

There's a small (indirect) clarification in the JASPIC 1.1 spec about this. In C.7.3 it says:

In Section 3.8.3.1, “validateRequest Before Service Invocation”, added clarification to description of processing for SEND_CONTINUE, especially to allow for forwards to a login page within an authentication module.

My interpretation would be that after having called RequestDispatcher#forward an authentication module SHOULD NOT return SUCCESS, since this would cause the container to invoke the resource, which would then get to see a closed response object.

It's debatable what should be returned in that case though. The original discussion mentioned "SEND_SUCCESS", but the JASPIC 1.1 spec has been adjusted such that a "SEND_CONTINUE" can be used after having used a forward.

If Liberty's JASPIC implementation behaves the same as WebSphere's 8.5 one, then possibly all that's needed here might be the removal of the constraint where WebSphere enforced that only the "302, 303, 307 and 401" status codes are allowed when SEND_CONTINUE is returned.

Hope this helps.

arjantijms avatar May 14 '15 12:05 arjantijms

Yes that helps thanks. Definitely agree that the intent (and the implied flows) are not at all clear from the spec.

So just to summarize: the behavior on SEND_CONTINUE with status code other than 302, 303, 307 or 401 is to simply return the response object as is. Essentially the provider is not authenticating the request, it's just displaying a login form, and the user agent will then need to invoke the original target again somehow with the results of the login form submission.

Can you let me know if that's correct?

paulben avatar May 14 '15 23:05 paulben

Essentially the provider is not authenticating the request, it's just displaying a login form

That's correct.

and the user agent will then need to invoke the original target again somehow with the results of the login form submission.

It doesn't necessarily need to be the original target. In Servlet FORM for instance the postback is a virtual URL named "j_security_check". There's no resource behind this URL, just a name the SAM is listening too.

There the flow is thus:

  • User accesses /protected/foo
  • SAM sees /protected/foo is protected and user not authenticated
  • SAM remembers original request, forwards to /login
  • /login renders a form with "/test_bar" as the action (postback url)
  • User fills out form, submits to /test_bar
  • SAM checks every incoming URL, if requestUri is /test_bar redirects to original URL
  • SAM checks again, if request details saved, restores original request by wrapping current request and lets original target be invoked

Of course this is just a flow, but the above is roughly how the FORM authentication method is specified.

arjantijms avatar May 15 '15 07:05 arjantijms

Gotcha, totally clear now. Thanks.

We can make that change easily enough. One concern is that it appears to be an incompatible change of behavior from the 1.0 spec - a SEND_CONTINUE that causes us to fail the request today would now return the response as is. So we want to carefully consider how to proceed as we want provider portability between our 1.0 and 1.1 impl's as much as possible.

From your new feature request for forward/include (https://java.net/jira/browse/JASPIC_SPEC-7) Ron said this re our 1.0 impl:

"I think the websphere implementation is probably most correct (in terms of what the spec currently requires, asthey are apparently enforcing the following for SEND_CONTINUE..."

Also thinking what if the forward returns 302, 303, 307, or 401? That could be ambiguous to the container.

I'm wondering if maybe a new AuthStatus value might be useful here? Something like SEND_RETURN which would tell the container to return the response as is, specifically do not try to set anything in the response as it may very well be committed. Then the container's SEND_CONTINUE processing could remain compatible with 1.0.

Paul

paulben avatar May 15 '15 16:05 paulben

@paulben

a SEND_CONTINUE that causes us to fail the request today would now return the response as is.

True

So we want to carefully consider how to proceed as we want provider portability between our 1.0 and 1.1 impl's as much as possible.

I hear you, however it might be debatable how much this is really an issue for this particular point.

JASPIC 1.0 said the following:

return AuthStatus.SEND_CONTINUE – If it has established a response (available to the runtime by calling messageInfo.getResponseMessage) that must be sent by the runtime for the request validation to be effectively continued by the client. The module must have set the HTTP status code of the response to a value (for example, HTTP 401 unauthorized, HTTP 303 see other, or HTTP 307 temporary redirect) that will indicate to the client that it should retry the request.

There are a few points to take into account here:

First is that the spec text mentioned 401, 303, 307 as examples, but doesn't say anything definitive about which status codes should be set, not even which ranges. It also says nothing about which status codes should not be set.

Second it does not say whether the runtime should do anything to validate this behavior. In the spirit of what the spec asked before validating the response code might have been correct from a certain point of view, but this was never explicitly mandated. JASPIC 1.1 then clarified that it's the auth module's responsibility to set the right response code for a particular flow that's specific to a given auth module.

Basically following the second point is that no JASPIC implementation I'm aware of actually does anything with return values. All the implementations out there (including the RI), exclusively check for SUCCESS or not. Whether this is a good thing is another debate, but it does mean that no portable SAM could have depended on the runtime throwing an exception when the module did not set the right response code.

Finally, just in my personal opinion, it's perhaps not so bad if a module that previously would not have worked on WebSphere's JASPIC 1.0 would now work on Liberty's JASPIC 1.1. It's another server and a newer version of the spec after all. There are a few more examples of such things in Java EE where a constraint was lifted. E.g. the EJB 3.0 and earlier spec said that an EJB was not allowed to access the file system. If I'm not mistaken 3.1 lifted this. If a server actively enforced this constraint then an EJB accessing the file system would not have worked on a EJB 3.0 impl of that server, but would have to work on an EJB 3.1 one.

Still, maybe if this is really an issue for IBM than you could set a flag in server.xml? There's already some configuration for JASPIC there. E.g. something like jaspic_validate_send_continue_status_codes and then have the user set it to "302, 303, 307, 401" if they would like to have the old WebSphere specific behavior?

Also thinking what if the forward returns 302, 303, 307, or 401? That could be ambiguous to the container.

I think it probably should not matter.

The SAM can either forward to another resource, or write to the response itself. Just as it can retrieve the username/roles via its own code or delegate that to say a JAAS login module.

The only thing that matters to the container is the outcome, not the way in which this outcome was established. So it probably shouldn't matter if the SAM would use the response object to set a 302 directly, or it passed the response object to another class which sets the 302, or it forwards to a Servlet which sets the 302.

Something like SEND_RETURN which would tell the container to return the response as is, specifically do not try to set anything in the response as it may very well be committed.

Something like this might be discussed for a newer version of JASPIC, but that's likely not going to help much for JASPIC 1.1/Java EE 7. If it was really a show stopper, then maybe a new MR of the spec could be asked for, but that's a big step.

There's also the issue that it probably wouldn't help much, as all existing authentication modules that I checked are already returning SEND_CONTINUE after having forwarded.

arjantijms avatar May 15 '15 20:05 arjantijms