Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ashleymercer
Copy link

@ashleymercer ashleymercer commented Jul 8, 2017

Fixes #115

The default Auth and ServletUtils classes were previously tied to the javax.servlet APIs, so couldn't be used in other frameworks (e.g. Play).

Instead, introduce abstract HttpRequest and HttpResponse classes which can be used as wrappers around different request and response objects depending on the framework (plus default javax implementations).

A nice result of this is that Auth and ServletUtils can move to the core module - the toolkit jar now contains only the javax.servlet wrappers.

The default Auth and ServletUtils classes were previously tied to the
javax.servlet APIs, so couldn't be used in other frameworks (e.g. Play)

Instead, introduce abstract HttpRequest and HttpResponse classes which
can be used as wrappers around different request and response objects
depending on the framework (plus default javax implementations).
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 94.845% when pulling 89ce157 on ashleymercer:master into 9274e6b on onelogin:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 95.544% when pulling 7793000 on ashleymercer:master into 9274e6b on onelogin:master.

@pitbulk
Copy link
Contributor

pitbulk commented Jul 22, 2017

That solution breaks environments that currently use HttpServletRequest and HttpServletResponse on Auth.

@markkolich
Copy link

markkolich commented Apr 10, 2018

@pitbulk any update on this PR? I'm finding myself in the same boat as @ashleymercer here. Specifically, I'd like to pass my own version of com.onelogin.saml2.http.HttpRequest into com.onelogin.saml2.Auth, but Auth appears to be strongly tied to javax.servlet.

I'm having to implement my own skinny version of HttpServletRequest and pass that into Auth instead, which is effectively a wrapper on top of my underlying request object. If I could only pass my own HttpRequest implementation into Auth that would get me most of the way there.

Somewhat related, it seems that com.onelogin.saml2.http.HttpRequest should actually be an interface. Your default implementation could be backed by javax.servlet, but for those who want to implement their own version of HttpRequest you could leave that door open as well. E.g., my own version of ServletUtils#makeHttpRequest().

@pitbulk
Copy link
Contributor

pitbulk commented Apr 10, 2018

We need to find a solution compatible with current Auth API...

Since request and response attributes from the Auth class are private, I'm ok replacing then from HttpServletRequest/HttpServletResponse to HttpRequest/HttpResponse, but we need to
keep the way of initializing the Auth class with HttpServletRequest and HttpServletResponse objects, so be able to transform those objects into HttpRequest/HttpResponse objects.

@markkolich
Copy link

@pitbulk I think it would be completely reasonable to add a few additional constructors to Auth that take a com.onelogin.saml2.http.HttpRequest and com.onelogin.saml2.http.HttpResponse instead of a HttpServletRequest and HttpServletResponse.

In other words, leave the existing constructors alone (for backwards compatibility sake), but add a means for consumers to pass their own HttpRequest and HttpResponse instances into a new set of Auth constructors.

I still contend though that definitions like HttpRequest and HttpResponse should be interfaces, not concrete classes. Just looking at how java-saml uses the request and response, the toolkit only uses a small subset of methods defined by HttpServletRequest and HttpServletResponse. I think it would be much cleaner if HttpRequest and HttpResponse were interfaces that defined only what java-saml needs from each entity, instead of requiring the consumer to pass around implementations of HttpServletRequest and HttpServletResponse.

@antonpetkoff
Copy link

@markkolich +1
We can't integrate java-saml with the Play Framework because of the Servlet API dependence...

@pitbulk
Copy link
Contributor

pitbulk commented May 17, 2018

As far as the solution keep compatible with the current Auth class, I'm ok merging it.

@slorber
Copy link

slorber commented Dec 19, 2018

Hey, wondering if this can be used with play now?
I've seen the parameter "stay" which avoid writing to the response and instead just returns the url, is this enough to solve the problem?

#86

@markkolich
Copy link

FWIW, I think #395 now supersedes this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants