Open ashleymercer opened 7 years ago
At #71 we removed servlet dependency on the core of java-saml. It separated out the dependency on HttpServletRequest.
"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.
If your solution doesn't break current environments, I'm open to accept a PR.
If the solution breaks backward compatibility, maybe instead of rewirte the toolkit package, make more sense to implement a new package based on it, but independent from javax.servlet so users will be able to use it instead of the toolkit package.
If you want this to be a point release to the existing version 2, then yes we should absolutely ensure back-compatibility.
However, would you consider taking this PR and publishing a new version 3, which does break compatibility? I think there are several benefits (although obviously I'm biased because I wrote the code :P):
with the new wrapper API, it's trivial to add support for a new container - you just need to write implementations for HttpRequest
and HttpResponse
which typically will map directly to methods on the underlying server API (e.g. HttpServletRequest
in the case of JavaEE). There's no need to duplicate any logic out of the Auth
and ServletUtils
classes as there would be currently.
the existing APIs are somewhat mutable (generally a bad idea), and it's not always clear when you need an HttpServletRequest
and when you need an HttpRequest
- and in fact, as I found when I was trying to use the library - because we construct HttpRequest
as effectively a deep copy, it's possible for the two to get out of sync. With wrappers, there's no such possibility since we always delegate to the underlying object.
a lot of methods on the old HttpRequest
class are there only really for testing - such as being able to add extra parameter values. By splitting this out to a mock, we make sure that nobody can depend on this behaviour in production code.
If I publish 3.X, then I will need to maintain 2.X and 3.X branch. (double of work. Something that I really want to avoid). I'm agree that your proposal makes sense and customers/frameworks will be happy with a non servlet dependency
Do you think is too much complicate to provide a back-compatibility solution, or create a new package toolkit-servletless with the changes you introduced, but without modifying the current java-saml-core / java-saml-toolkit?
For me gonna be easier to maintain a new package toolkit-servletless than 2 branches (most of the code changes happens on the core rather than on the toolkit).
@miszobi what is your opinion?
Fair point! Let me have a think - it might be possible to do something like create abstract base classes for Auth
and ServletUtils
so we can share code between different containers (it would be a shame to have to duplicate all the logic).
I already thought on base classes, but then what you gonna do?
Auth and ServletUtils depend heavily on the java servlet API, but not all Java web frameworks use this - for example, the Play framework. The current advice is to copy-paste these classes and tailor them as necessary, but that seems a bit heavyweight.
Since there is already an abstraction over HttpRequest, it should be possible to make Auth and ServletUtils depend only on this interface (might be necessary to add a similar HttpResponse interface) and then provide adapters for the
javax.servlet.http.HttpServletRequest
andjavax.servlet.http.HttpServletResponse
.It would then be trivial to plug java-saml into other web frameworks by only providing new HttpRequest/Response classes.