Refactored out Servlet dependencies from core and toolkit #395

markkolich · 2023-03-05T01:12:13Z

Another proposed solution for #349 and #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();

markkolich · 2023-03-05T01:19:58Z

servlet-jakarta/src/main/java/com/onelogin/saml2/servlet/jakarta/JakartaSamlHttpRequest.java

+        delegate.getSession().invalidate();
+    }
+
+    public static HttpRequest makeHttpRequest(HttpServletRequest delegate) {


I'm not even sure this static method is really needed, I mean is this really any better than new JakartaSamlHttpRequest(delegate)?

markkolich · 2023-03-05T01:20:11Z

servlet-jakarta/src/main/java/com/onelogin/saml2/servlet/jakarta/JakartaSamlHttpResponse.java

+        delegate.sendRedirect(location);
+    }
+
+    public static HttpResponse makeHttpResponse(HttpServletResponse delegate) {


I'm not even sure this static method is really needed, I mean is this really any better than new JakartaSamlHttpResponse(delegate)?

markkolich · 2023-03-05T01:22:21Z

core/src/main/java/com/onelogin/saml2/http/HttpRequest.java

-public final class HttpRequest {
-
-    public static final Map<String, List<String>> EMPTY_PARAMETERS = Collections.<String, List<String>>emptyMap();
+public interface HttpRequest {


I'm OK calling this interface SamlHttpRequest or OneLoginSamlHttpRequest if the existing name is too generic.

Three implementors in this PR:

markkolich · 2023-03-05T01:22:35Z

core/src/main/java/com/onelogin/saml2/http/HttpResponse.java

+ *
+ * @since 3.0.0
+ */
+public interface HttpResponse {


I'm OK calling this interface SamlHttpResponse or OneLoginSamlHttpResponse if the existing name is too generic.

Two implementors in this PR:

pitbulk · 2023-03-14T00:18:26Z

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 · 2023-03-14T01:41:57Z

This works for me. Thank you @markkolich!

jeroenvs · 2023-03-14T06:53:35Z

This would also work for me. Thank you!

fanste · 2023-03-14T10:32:05Z

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

josemgut · 2023-03-14T11:45:52Z

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 · 2023-03-14T13:46:18Z

I had a quick look at the commit and I would say this is exactly what I had in mind in #349 (comment). 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 · 2023-03-14T17:16:05Z

@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 · 2023-04-26T07:38:04Z

will this be merged anytime soon ?

ardeleana · 2023-05-12T05:44:12Z

@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 · 2023-05-12T16:50:13Z

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.

markkolich · 2023-10-13T21:20:18Z

@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 · 2023-10-13T22:07:01Z

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 · 2023-10-15T21:52:54Z

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

markkolich · 2023-10-16T03:31:48Z

@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 · 2023-10-16T19:29:55Z

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 · 2023-10-27T06:35:06Z

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 · 2023-10-27T07:03:26Z

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 · 2023-10-27T08:37:20Z

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 · 2023-11-21T09:54:01Z

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 · 2023-11-21T16:44:44Z

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

pitbulk · 2023-11-21T17:51:25Z

@markkolich that's great.

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

- Adding JakartaSamlAuth and JavaxSamlAuth which both extend BaseAuth

markkolich · 2023-11-22T17:36:59Z

@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 · 2023-12-08T21:19:14Z

@pitbulk @mauromol any update here?

ak98neon · 2023-12-12T14:38:37Z

@pitbulk @mauromol please review

mauromol · 2023-12-12T23:04:39Z

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 · 2023-12-30T23:21:02Z

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 · 2023-12-31T08:45:21Z

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.

dsvensson · 2024-01-12T13:55:15Z

@pitbulk Perhaps a bit to subtle given the lack of reply, but scrolling up here both @haavar (who mentioned being open to maintaining a fork) and @markkolich both sound like potential co-maintainers?

arunkumarun · 2024-02-17T02:36:14Z

@pitbulk Need this dependency for Wildfly Server version >= 27.

Waiting this to be merged.

jacqueskpoty · 2024-03-04T17:16:06Z

@markkolich coul you folllow up with comments so this PR can be merge?

markkolich · 2024-03-04T22:38:15Z

@jacqueskpoty no, I am not a maintainer of this library ... I have no merge permissions, I'm just a contributor who opened this PR. Please followup with @pitbulk.

jacqueskpoty · 2024-03-04T23:17:43Z

@pitbulk Is this PR going to be merged any time soon?

haavar

Can we completely remove the HttpRequest from core? I don't think it's too much to ask that the user extract the parameters and pass it to the library. That seems easier than for the user to understand how to configure the library for their version of the servlet API.

haavar · 2024-03-05T07:56:43Z

core/src/main/java/com/onelogin/saml2/http/HttpRequestUtils.java

+
+import org.apache.commons.lang3.StringUtils;
+
+public class HttpRequestUtils {


I don't see this class used anywhere in core. It should either be moved or removed.

It's used in toolkit but not core.

However, I kept the utils in core so that they stay near the HttpRequest and HttpResponse interfaces given the close relationship between these classes and their usage:

haavar · 2024-03-05T07:56:57Z

core/src/main/java/com/onelogin/saml2/http/HttpResponseUtils.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class HttpResponseUtils {


I don't see this class used anywhere in core. It should either be moved or removed.

jacqueskpoty · 2024-03-06T17:23:17Z

toolkit/src/main/java/com/onelogin/saml2/BaseAuth.java

@@ -625,7 +625,7 @@ public String login(String relayState, AuthnRequestParams authnRequestParams, Bo
 		parameters.put("SAMLRequest", samlRequest);

 		if (relayState == null) {
-			relayState = ServletUtils.getSelfRoutedURLNoQuery(request);
+			relayState = HttpRequestUtils.getSelfRoutedURLNoQuery(request);
 		}


@haavar The HttpRequestUtils class is being used here and is heavily used throughout this BaseAuth class.

markkolich · 2024-03-06T21:00:29Z

I don't think it's too much to ask that the user extract the parameters and pass it to the library. That seems easier than for the user to understand how to configure the library for their version of the servlet API.

@haavar I disagree, but feel free to send a PR to my fork (e.g., markkolich#1) with your proposed changes and we can review+discuss.

markkolich mentioned this pull request Mar 5, 2023

Provide Jakarta compatible build #349

Open

markkolich commented Mar 5, 2023

View reviewed changes

markkolich force-pushed the master branch 8 times, most recently from 22c228f to fbfe69b Compare March 5, 2023 19:11

markkolich mentioned this pull request Mar 8, 2023

Allow java-saml to be used in non-JavaEE containers #115

Open

markkolich force-pushed the master branch from fbfe69b to c6b12ba Compare March 14, 2023 05:54

markkolich force-pushed the master branch 2 times, most recently from d5dbbde to 0c8040f Compare March 14, 2023 17:38

jacek-poreda-sonarsource mentioned this pull request Apr 19, 2023

PLUGINAPI-42 Migrate javax -> jakarta (backward compatibility) SonarSource/sonar-plugin-api#92

Closed

markkolich force-pushed the master branch from 0c8040f to fc69b19 Compare May 12, 2023 16:47

markkolich mentioned this pull request May 13, 2023

Provide compliance with both javax.servlet and jakarta.servlet #379

Open

pietro-saccani mentioned this pull request Nov 16, 2023

Jakarta supported version #401

Open

- Renaming Auth -> BaseAuth

5017e2f

- Adding JakartaSamlAuth and JavaxSamlAuth which both extend BaseAuth

markkolich mentioned this pull request Nov 26, 2023

Testcases seems to be failing #402

Open

mauromol mentioned this pull request Mar 4, 2024

Issues Javax EE to Jakarta EE using java-saml #419

Open

haavar reviewed Mar 5, 2024

View reviewed changes

jacqueskpoty reviewed Mar 6, 2024

View reviewed changes

dsvensson mentioned this pull request Apr 11, 2024

This project is currently not under active development #388

Open


		import org.apache.commons.lang3.StringUtils;

		public class HttpRequestUtils {

Refactored out Servlet dependencies from core and toolkit #395

Are you sure you want to change the base?

Refactored out Servlet dependencies from core and toolkit #395

Conversation

markkolich commented Mar 5, 2023 • edited

markkolich Mar 5, 2023 • edited

Choose a reason for hiding this comment

markkolich Mar 5, 2023 • edited

Choose a reason for hiding this comment

markkolich Mar 5, 2023 • edited

Choose a reason for hiding this comment

markkolich Mar 5, 2023 • edited

Choose a reason for hiding this comment

pitbulk commented Mar 14, 2023

bramhaag commented Mar 14, 2023

jeroenvs commented Mar 14, 2023

fanste commented Mar 14, 2023

josemgut commented Mar 14, 2023

mauromol commented Mar 14, 2023

markkolich commented Mar 14, 2023

kush-vi commented Apr 26, 2023

ardeleana commented May 12, 2023

markkolich commented May 12, 2023 • edited

markkolich commented Oct 13, 2023 • edited

mauromol commented Oct 13, 2023

pitbulk commented Oct 15, 2023

markkolich commented Oct 16, 2023

mauromol commented Oct 16, 2023

dsvensson commented Oct 27, 2023 • edited

mauromol commented Oct 27, 2023

dsvensson commented Oct 27, 2023

jemonra commented Nov 21, 2023 • edited

markkolich commented Nov 21, 2023

pitbulk commented Nov 21, 2023

markkolich commented Nov 22, 2023

markkolich commented Dec 8, 2023

ak98neon commented Dec 12, 2023

mauromol commented Dec 12, 2023

pitbulk commented Dec 30, 2023

dsvensson commented Dec 31, 2023

dsvensson commented Jan 12, 2024

arunkumarun commented Feb 17, 2024

jacqueskpoty commented Mar 4, 2024

markkolich commented Mar 4, 2024

jacqueskpoty commented Mar 4, 2024

haavar left a comment • edited

Choose a reason for hiding this comment

haavar Mar 5, 2024

Choose a reason for hiding this comment

markkolich Mar 6, 2024

Choose a reason for hiding this comment

haavar Mar 5, 2024

Choose a reason for hiding this comment

jacqueskpoty Mar 6, 2024

Choose a reason for hiding this comment

markkolich commented Mar 6, 2024

markkolich commented Mar 5, 2023 •

edited

markkolich Mar 5, 2023 •

edited

markkolich Mar 5, 2023 •

edited

markkolich Mar 5, 2023 •

edited

markkolich Mar 5, 2023 •

edited

markkolich commented May 12, 2023 •

edited

markkolich commented Oct 13, 2023 •

edited

dsvensson commented Oct 27, 2023 •

edited

jemonra commented Nov 21, 2023 •

edited

haavar left a comment •

edited