Refactored out Servlet dependencies from core and toolkit

[markkolich]

Another proposed solution for https://github.com/SAML-Toolkits/java-saml/issues/349 and https://github.com/SAML-Toolkits/java-saml/pull/115.

• Introduced servlet-jakarta and servlet-javax which provide differing implementations of the core HttpRequest and HttpResponse interfaces depending on the servlet container implementation where the library is consumed
• Teased apart HTTP request and HTTP response objects along a common seam and refactored all core and toolkit code
• Removed all dependencies on the servlet API from core and toolkit
• Bumped version to 3.0.0

With this change, if you're on a container pre-EE9, then you declare a dependency on:

<dependency>
  <groupId>com.onelogin</groupId>
  <artifactId>java-saml</artifactId>
  <version>3.0.0</version>
</dependency>
<dependency>
  <groupId>com.onelogin</groupId>
  <artifactId>java-saml-servlet-javax</artifactId>
  <version>3.0.0</version>
</dependency>

If you're on an EE9 or later container, then you declare a dependency on:

<dependency>
  <groupId>com.onelogin</groupId>
  <artifactId>java-saml</artifactId>
  <version>3.0.0</version>
</dependency>
<dependency>
  <groupId>com.onelogin</groupId>
  <artifactId>java-saml-servlet-jakarta</artifactId>
  <version>3.0.0</version>
</dependency>

Of course, this PR also opens the door to non-servlet container implementations, meaning someone in Akka, Play, etc. could use this library as well as long as they provide their own HttpRequest and HttpResponse implementation. The servlet API dependency in core and toolkit has been completely removed.

Usage on pre-EE9 containers look like this:

import com.onelogin.saml2.servlet.javax.JavaxSamlHttpRequest;
import com.onelogin.saml2.servlet.javax.JavaxSamlHttpResponse;

final HttpRequest request = JavaxSamlHttpRequest.makeHttpRequest(javaxServletRequest);
final HttpResponse response = JavaxSamlHttpResponse.makeHttpResponse(javaxServletResponse);

final Auth auth = new Auth(saml2Settings, request, response);
auth.processResponse();

EE9 and later usage looks like this:

import com.onelogin.saml2.servlet.jakarta.JakartaSamlHttpRequest;
import com.onelogin.saml2.servlet.jakarta.JakartaSamlHttpResponse;

final HttpRequest request = JakartaSamlHttpRequest.makeHttpRequest(jakartaServletRequest);
final HttpResponse response = JakartaSamlHttpResponse.makeHttpResponse(jakartaServletResponse);

final Auth auth = new Auth(saml2Settings, request, response);
auth.processResponse();

[pitbulk]

Thanks @markkolich for this PR. Let's see what rest of the people think about it.

@veselov, @mauromol, @jeroenvs, @lounagen, @krzysztof-osiecki , @dsvensson, @josemgut, @bramhaag, @fanste

[bramhaag]

This works for me. Thank you @markkolich!

[jeroenvs]

This would also work for me. Thank you!

[fanste]

Thats seems to be the best solution instead of using the "transformer hack". Thanks for your work, @markkolich.

[josemgut]

This is also good for me. I'm not able to test it inmediatelly, but in two weeks we will apply it and it will work for sure. Thanks a lot.

[mauromol]

I had a quick look at the commit and I would say this is exactly what I had in mind in https://github.com/SAML-Toolkits/java-saml/issues/349#issuecomment-1453909117. So, good job @markkolich

Just my 2 cents: maybe the invalidateSession() implementations could use delegate.getSession(false) and test the result against null before invalidating? Otherwise if, for whatever reason, no session exists, that method would uselessly create a new one to immediately invalidate it...

[markkolich]

@mauromol fair point on invalidateSession() - I left it as is because the original logic prior to this refactor was:

request.getSession().invalidate();

Happy to change that to:

HttpSession session = delegate.getSession(false);
if (session != null) {
  session.invalidate();   
}

[kush-vi]

will this be merged anytime soon ?

[ardeleana]

@markkolich We encounter a NPE when using the processSLO because "SAMLRequest" param does not exist in request, that makes getParameters of the HttpRequest return null and isEmpty is directly applied on the null values object in getParameter method of HttpRequest. NB1: we are using a sample app deployed in payara 6 app server and the jakarta servlet version. NB2: we have fixed the issue in a particular fork that we'll use temporary, until this pull req will be merged and released Stacktrace: jakarta.enterprise.web|_ThreadID=82;_ThreadName=http-thread-pool::http-listener-1(3);_TimeMillis=1683800617926;_LevelValue=900;|   StandardWrapperValve[jsp]: Servlet.service() for servlet jsp threw exception java.lang.NullPointerException         at com.onelogin.saml2.http.HttpRequest.getParameter(HttpRequest.java:48)         at com.onelogin.saml2.Auth.processSLO(Auth.java:1269)         at com.onelogin.saml2.Auth.processSLO(Auth.java:1371)         at com.onelogin.saml2.Auth.processSLO(Auth.java:1380)         at org.apache.jsp.sls_jsp._jspService(sls_jsp.java:113)

[markkolich]

Nice find @ardeleana - I have fixed the bug in this PR and have updated the unit tests accordingly. Thank you!

@pitbulk there were also a bunch of LogoutRequest's used in tests that were coded to expire (NotOnOrAfter) on 2023-05-10. As I write this it's now 2023-05-12 and so those tests started failing. I have fixed that test data as well in this PR.

[pitbulk]

@markkolich thanks for the PR and for fixing the SAML Messages of the tests, I will review and merge the PR as soon as possible.

[ardeleana]

@pitbulk are you planning to merge this PR and release a 3.0.0 version pretty soon? Thanks!

[wendigo]

Hi, @pitbulk I need it too. Any chance to merge this PR and release it?

[aidando73]

Hi, @pitbulk. Same here - our company needs it too. We want to upgrade from Wildfly 24 to Wildfly 28 but can't.

[npsakthi]

Hi @pitbulk , My project upgrading from javax to jakarta. java-saml not working. Waiting for jakarta supported version.

[spen37]

@pitbulk We're having to migrate to Spring 6 & Jetty 11 due to vulnerabilities and hence need to move to Jakarta. Can we get an update on the status of this PR please?

[nj4x]

@markkolich - thank you for the changes, I was able to use your branch in a project of Java17 and SB311 upgrades. Can we also consider bumping some dependencies, so to pass OWASP checks (https://github.com/markkolich/java-saml/pull/1) ?

[markkolich]

@nj4x LGTM - thanks! I've merged the library version bumps from https://github.com/markkolich/java-saml/pull/1 into this PR ... the build & all tests pass.

We still need a maintainer of this library (@pitbulk 👀) to review this PR and merge in for release.

[danielstravito]

@pitbulk Any update on this, @markkolich seems to have prepared everything for you.

[ak98neon]

upvote, really need this changes

[ak98neon]

@pitbulk review pls

[dsvensson]

@pitbulk Sorry to add another nag here, but could you perhaps clarify if your plans. Is your current recommendation, for the foreseeable future, to just import a copy of the code of this branch into the repo of projects using saml-toolkit today? Or would that be a waste of time with just a bit more patience?

[haavar]

The worst thing that could happen to this project is that we all do private forks of it. Before you do that, let's consider other options.

A service that I own heavily depends on this library, and I think it's the best SAML library for java. Before @pitbulk took over ownership, I offered to be a co-owner of this project. That offer still stands. I can justify spending work hours on this project, because any alternative would take a lot more time. My interest is mainly in java-saml-core.

If we can't work that out, I'm also open to for a shared public fork. I don't want that to happen, but it's better than killing the project by each of us doing private forks.

[pitbulk]

Hi,

Sorry guys, I invested time on the rest of the toolkits and left the java-saml project to the latest on my queue.

I will do my best and get an official release in 2-3 weeks.

Thanks for your patience and collaboration.

[dsvensson]

Managing an open source project is what it is, but one way of reducing the burden is luring in a herd of recurring contributors, and this PR seems to have a few candidates. Was mostly after hearing about any kind of plan, rather than the all too common "is it done yesterday!!11" :)

[dsvensson]

@pitbulk Any specific area the community can offer a helping hand in that allows you to nail this new goal, this PR or otherwise?

[zeletrik]

Are there any news on the new release @pitbulk ? 🙇

[pitbulk]

Working on it, there were a lot of work to be done :)

[pitbulk]

I fixed some test from the master branch and updated dependencies.

I started to review and test this PR, it will be merged soon and an official release finally should see the light. Apologies for the long delay :(

[markkolich]

@pitbulk I have rebased this PR with the latest upstream/master and have resolved all merge conflicts.

[pitbulk]

@markkolich can you adjust the mockito dependency to use

<artifactId>mockito-core</artifactId>
<version>3.12.4</version>

instead

<artifactId>mockit-core</artifactId>
<version>1.10.19</version>

Also notice I removed its dependency from java-saml-core as I only had 1 test using it and implemented it in other way.

Also remove the dependency com.fasterxml.jackson.core

And jacoco-maven-plugin on servlet-jakarta and servlet-javax should be 0.8.10

Doing that, I guess the mvn test from CI should pass.

[markkolich]

@pitbulk dependencies fixed & pushed - thanks for calling out those merge typos in the pom.xml!

[pitbulk]

@markkolich, what could be done to avoid the web example code, which is what many servers gonna have, to stop working

<?
Auth auth = new Auth(request, response);

Due

 Auth(HttpServletRequest request, HttpServletResponse response)

was removed, could we somehow consider a way to keep the backward compatibility? Could work making JavaxSamlHttpRequest somehow implements HttpRequest and javax.servlet.http.HttpServletRequest

Or anychance to somehow define something like

public Auth(Object request, Object response) throws IOException, SettingsException, Error {
        java.lang.Class<?>[] interfaces = request.getClass().getInterfaces();
        if (interfaces != null && interfaces.length >0 && interfaces[0].getName() == "javax.servlet.http.HttpServletRequest") {
            HttpRequest httpRequest = <retrieve_from request>
            HttpResponse httpResponse = <retrieve_from response>
            this(new SettingsBuilder().fromFile("onelogin.saml.properties").build(), httpRequest, httpResponse);
        }       
    }

and then try to inspect the objects and properly handle them?

[markkolich]

@pitbulk I welcome the thoughts of others from the community on this, but my 2-cents ...

This PR is very intentionally not backwards compatible with the previous versions of java-saml. That's precisely why I bumped the major version to 3: it's a new version that will require existing users to do some small amount of work when they upgrade to 3.0.0.

I don't think changing the signature of the Auth constructor to Auth(Object request, Object response) and then using reflection to determine the argument types is the right approach. Such a change feels like it's defeating the purpose of the compiler and arguably makes it more difficult to identify the correct Auth constructor argument types.

That said though, perhaps what's missing in this PR is some documentation changes and example code to explain or clarify the upgrade path?

For instance, in your JSP example, this:

<%@page import="com.onelogin.saml2.Auth"%>

<?
Auth auth = new Auth(request, response);
?>

Would become this (please excuse the wildcard imports):

<%@page import="com.onelogin.saml2.Auth"%>

<%@page import="com.onelogin.saml2.http.*"%>
<%@page import="com.onelogin.saml2.servlet.javax.*"%>

<?
HttpRequest samlRequest = JavaxSamlHttpRequest.makeHttpRequest(request);
HttpResponse samlResponse = JavaxSamlHttpResponse.makeHttpResponse(response);

Auth auth = new Auth(samlRequest, samlResponse);
?>

Everything else remains the same.

I'm happy to help out with correcting or beefing up the documentation, either as a commit in this PR or as a separate fast-follow PR.

Let me know what you all think!

[mauromol]

Alternative idea: what about keeping existing Auth signature as before, introducing a JakartaAuth that uses the new Jakarta interfaces and then making both delegate to a new AuthImpl class that instead uses HttpRequest and HttpResponse and does the real job?

[pitbulk]

@markkolich , what do you think about @mauromol's suggestion?

[markkolich]

@pitbulk @mauromol I think that's a reasonable suggestion!

In the JSP example, you'd still have to change some imports, so the library upgrade wouldn't be a seamless drop in replacement but maybe less invasive of a change?

With this proposal, consider JavaxSamlAuth implements Auth and JakartaSamlAuth implements Auth. Even if we had the Auth implementations handle the servlet specific stuff, you'd still end up with something like this:

<%@page import="com.onelogin.saml2.Auth"%>
<%@page import="com.onelogin.saml2.servlet.jakarka.JakartaSamlAuth"%>

<?
Auth auth = new JakartaSamlAuth(request, response);
?>

In other words, the user would still have to make some code or JSP change to support this new version. It wouldn't be completely free.

I, for one, don't think we should be optimizing the path forward for antiquated JSP use-cases but I am acutely aware of the general concern around backwards compatibility and want to respectful of that when it's reasonable to do so.

If you agree that a JavaxSamlAuth implements Auth and JakartaSamlAuth implements Auth solution is better than the original proposal in this PR I'm totally OK with refactoring the changeset in this PR. LMK!

[mauromol]

What I had in mind was a bit different and (unless I'm missing something - I didn't investigate further) should be 100% API and binary compatible for legacy consuming applications. I mean:

• com.onelogin.saml2.Auth maintains the exact same contract of today, but delegates all its operations to an extracted super class (named BaseAuth, for instance) that has no dependency on javax.servlet, but rather has a constructor that takes HttpRequest and HttpResponse objects
• then, another JakartaAuth class extends BaseAuth but exposes a constructor with jakarta.servlet input parameters

In this way, code that uses the old Auth class (including the above JSP) should not need any code change and binary compatibility should be preserved as well (perhaps some tests would be needed). The only remaining requirement would be to change dependencies, because the Auth class should be moved from the java-saml-toolkit artifact to something like a java-saml-toolkit-javax artifact (while BaseAuth should stay in java-saml-toolkit). Even better, Auth can then be deprecated in favor of another class named JavaxAuth which does the same thing of Auth but makes it explicit that there are now two distinct implementations for the different servlet APIs.

The only thing that would still need a bit of thought is about Java 9 modules, if the toolkit wants to support them: in fact, in the above scenario the package com.onelogin.saml2 would be split into two different artifacts (java-saml-toolkit for the base/API part, java-saml-toolkit-javax for the moved Auth class): however, this is forbidden by Java 9 module system. Perhaps, a full support for Java 9 modules may be delayed to a later version, in which the (now deprecated) Auth class will be removed altogether in favor of JavaxAuth which should be placed from the beginning in a dedicated package of the new java-saml-toolkit-javax artifact.

[dsvensson]

Perfect is the enemy of good, how about tolerating the API breakage, and revisiting this in release+1? Just communicate it in the release notes.

[mauromol]

If you break the API now you can't recover in release+1...

Anyway, mine were just suggestions, I'm not leading this project.

[dsvensson]

Sure you can. Might be confusing with a one-off, but as long as it's communicated I doubt it would cause any real harm. Can just spam out some major version bumps to signal it clearly - they're just numbers, nothing sacred about them. Sure, a bit ugly, but as is the unreleased situation.

[jemonra]

We must do something with this project, I don't know if the correct solution is to accept the breakage or not, but it cannot wait anymore. @pitbulk wouldn't you consider opening this project to other collaborators (like @haavar proposed)? I think that the workload is too heavy for only one person.

[markkolich]

I'll take a look at @mauromol's suggestions and see if it's something I can implement later this week.

[pitbulk]

@markkolich that's great.

Next week I took vacation so I will have time to complete, review and publish.

[markkolich]

@mauromol I've been tinkering with the changes you suggested above. I think I understand what you're suggesting, and now that it's implemented the JavaxSamlAuth extends BaseAuth and JakartaSamlAuth extends BaseAuth approach looks like this...

Before:

<%@page import="com.onelogin.saml2.Auth"%>

<?
Auth auth = new Auth(request, response);
?>

After, with the BaseAuth approach discussed above in the thread:

<%@page import="com.onelogin.saml2.BaseAuth"%>
<%@page import="com.onelogin.saml2.servlet.jakarka.JakartaSamlAuth"%>

<?
BaseAuth auth = new JakartaSamlAuth(request, response);
?>

To gently reiterate, this is not a drop-in and make-no-changes to your JSPs backwards compatible change. Users will have to change imports and rename Auth -> BaseAuth. Is that acceptable?

I've pushed the suggested changes to this PR as a separate commit - I can squash if this looks good.

[markkolich]

@pitbulk @mauromol any update here?

[ak98neon]

@pitbulk @mauromol please review

[mauromol]

I just would like to specify that I'm not a maintainer of this project and I cannot merge. Some years ago I would have needed java-saml to be enhanced to properly support Italian SPID system, I've submitted some PRs, most of which have been merged, a couple of them are still there and one was never opened. Unfortunately, OneLogin lost interest in this project, Sixto changed his job, the new maintainers were absent, I even applied myself after being suggested by one of them, but things went in a different way. Probably this project should have been forked at that time, but I didn't have the necessary experience to do that. Now Sixto has come back as an independent maintainer, but it's clear that it is very low priority for him. I myself have changed job and I'm not into SAML and SPID any more, the latter now being pushed towards a migration to OpenID Connect, by the way (if it doesn't get dismissed before).

Here I just tried to give my suggestions on how to implement this based on what I know about Sixto point of view, and what I would have done myself. I still believe a solution which does not require any code change for old code should be possible, but I also think that small adjustments could probably be made by Sixto himself if he's willing to.

So, if I were you, I would seriously consider to fork this project and push the necessary changes the way you prefer.

[pitbulk]

It is not a matter of low priority, I'm sorry about the big delay I gave to this project.

As people may know, I maintain in my spare time not only java-saml, but php-saml, ruby-saml and python3-saml. And at the end of the day, it is a matter of time.

I had in mind to push this project in December, but sadly I have had not the chance to do it yet. I was and I'm always open to collaboration and appreciate the effort you guys are spending in the PRs and suggestions you are sending.

[dsvensson]

You can probably spot 1-2 people in this thread who you can give merge access to to get the ball rolling. No harm in merging and then iterating, throw out a rc and get heated feedback.