java-saml core depends on servlet-api, making it less reusable

[luuuis]

In v1 java-saml takes care of generating SSO URLs and validating SAMLResponses and leaves it up to the caller to perform the necessary redirects. This makes the library simple and reusable because it has a single responsibility (how to build SSO requests and understand the responses) and does not bring along many other dependencies.

Currently in the v2 branch most classes are now dependent on accessing the HttpServletRequest and HttpServletResponse directly, which means they now have an additional responsibilities like performing redirects, invalidating sessions, etc. The Java ecosystem is much more than just Servlet containers these days but adding this dependency excludes anyone that is not running in a traditional Servlet environment.

To give an example, I have used v1 in a Scala application that does not use a Servlet container and I chose to use the OneLogin toolkit precisely because it did not have unnecessary dependencies (otherwise I would've used Spring Security SAML or OpenSAML directly). I can not use v2 at all currently because I don't have a HttpServletRequest available to pass to the toolkit.

I propose to remove the servlet dependency from the core module, which should only know how to build SSO/SLO requests (independently of the tech stack in use) and understand the responses. At the same time I would add a java-saml-servlet module with a set of "controller" classes that use the core module and call the servlet APIs to perform redirect, etc. By separating the responsibilities of the two modules, the unit tests for each one will become simpler as well.

Thoughts?

[pitbulk]

The v1 had the AuthRequest and Response
that are available on the v2 as well, so if you want to use the toolkit without using the login/logout methods of Auth class you stil can do it.

On the python-saml toolkit we had a similar issue, because each web framework (django, flask, ..) has it own request object, so we created a custom request object, with the server data that the toolkit needs
and each framework had to provide this data using its methods, and we delegate the redirections to the framework. But some customers expected the toolkit taking care of redirections and wanted a custom Auth for its framework. We thought that it was no necessary since demos were provided and it was no much work to make it work.

I thought that HttpServletRequest and HttpServletResponse were quite standard, but if there are more scenarios I see 3 alternatives:

• Remove HttpServletRequest/HttpServletResponse requirement at Auth.
  (Cons: Common Java users of HttpServletRequest/HttpServletResponse will complain.)
• Create an alternative AuthServletLess class method to cover the scenarios where no HttpServletRequest / HttpServletResponse where user will need to handle redirections and provide server info (maintaining both Auth and AuthServletLess) .
• Provide at the Auth class a way to convert any request /response on a HttpServletRequest/HttpServletResponse using for example HttpServletRequestWrapper and HttpServletResponseWrapper

@miszobi what do you think on that issue?

[luuuis]

The v1 had the AuthRequest and Response
that are available on the v2 as well, so if you want to use the toolkit without using the login/logout methods of Auth class you stil can do it.

Thanks, I hadn't looked closely enough when I raised this issue. I did try to upgrade to v2 yesterday and while I can use AuthnRequest, SamlResponse has a dependency on HttpServletRequest so is not usable in my project.

I've just pushed a proposal in PR #71 to how I think this can work without changing the design of the library too much. This is basically the same as option (2) above except I have simply removed the servlet dependency from the low-level AuthRequest and SamlResponse, leaving Auth as-is.

Let me know what you think @pitbulk . I'd like to continue down this track if you're happy with this change.

[miszobi]

The approach in #71 looks good to me.

It separates out the dependency on HttpServletRequest, without changes to the api. "Servlet" users will still depend on java-saml, and interact with the Auth class, while users that don't want it can use java-saml-core directly.

[pitbulk]

I'm working on pending classes (LogoutRequest and LogoutResponse).

[pitbulk]

@miszobi @luuuis

Now LogoutRequest and LogoutResponse no longer depend on javax.servlet.
I think the servletless branch is ready to be merged with 2.0.0 branch.

My unique doubt is about the addParameter method. I think it should modify the HttpRequest object instead of returning a new one.

[luuuis]

Nice.

The reasoning behind addParameter returning a copy is to allow HttpRequest to be immutable. This makes it easier to reason about since you don't have to worry about the state mutating over time, can safely share these without taking copies, etc. I think most developers will be comfortable with the concept as it's widely used in JDK classes such as java.lang.String (e.g. String.substring, String.concat) but perhaps I should've made it clearer in the Javadoc.

At the end of the day this is a preference as I've found from experience that systems built on immutable object are easier to understand and more maintainable and understandable than those using mutable object. So personally I would recommend moving in the other direction (making other classes immutable where possible) instead of making HttpRequest mutable, though I am not going to fall on my sword for this. :)

[pitbulk]

Ok, I added a removeParameter that I think is not against that immutable philosophy at all.

Please review my PR. I will merge it at the end of the week and plan also to add the pending docunentation.

[luuuis]

Sorry I only had time to look at the PR now, looks good!

Thanks for this—I will integrate the new v2 into our app, hopefully this will yield further contributions :)