search

aacotroneo / laravel-saml2

A Laravel 5 package for Saml2 integration as a SP (service provider) based on the simple OneLogin toolkit
MIT License
567 stars 238 forks source link

Merge multi-idp fork (nirajp) into laravel-saml2 #187

Closed darynmitchell closed 5 years ago

darynmitchell commented 5 years ago

Proposal

Merge nirajp's fork into aacotroneo/laravel-saml2, bringing multi-IdP support into the library

Background

Discussion

I'm working with a laravel stack, and needed to be able to support multiple IdPs. I can see from activity on this project's issue #117, as well as questions in various online forums, that there are others who have the same need.

Because nirajp's fork was "minimal changes" to support multi IdP, I think it is a candidate to merge back into the upstream project rather than living as a fork. If this project supported multiple IdPs it would "simplify the landscape" for laravel devs by keeping the main library sufficient for most dev's needs.

Right now is a great time to merge it back in because the sync work is done.

This PR contains

  1. nirajp's fork work
  2. The sync, which updated nirajp/laravel-saml2 to incorporate all the changes in aacotroneo in the years since it the fork
  3. small additions on this branch to prepare for merge back into aacotroneo:
    • Version number bumped to 2.0 since this is a "breaking change" on the config side
    • Readme updates
    • Saml2 facade removed since it's no longer static, as must be invoked with the IdP name to load the IdP-specific config

For the most part, this PR does not attempt to slip in "improvements", but rather be a straight "make the changes necessary to sync the fork, and prepare it to merge back".

I offer it for your consideration, and hope that the end result will be multi-IdP support made available for all users of aacotroneo/laravel-saml2

darynmitchell commented 5 years ago

Not sure what's causing Travis failure for PHP 7.2 - I see that the most recent master build had the same failure too. I thought cc49ee8 might have found the cause but didn't fix. Anyone know the cause for the complaint about Declaration of Saml2AuthServiceProviderTest::tearDown() vs TestCase::tearDown()?

darynmitchell commented 5 years ago

re travis failure of phpunit in php 7.2, travis log shows that phpunit running is: /home/travis/.phpenv/versions/7.2.16/bin/phpunit

So one possible change would be to make sure .travis.yml runs the phpunit installed by composer: change script: phpunit to script: vendor/bin/phpunit

Thoughts?

darynmitchell commented 5 years ago

The latter change about phpunit path worked,

but I would have thought should NOT be needed, based on Travis doc about phpunit, which states that vendor/bin/phpunit will be checked before phpunit found on $PATH:

Travis CI looks for phpunit in the same order as Composer does and uses the first one found. ...

  1. vendor/bin/phpunit
  2. phpunit, which is found on $PATH (typically one that is pre-packaged with the PHP runtime)

However, it's possible that because this project's travis config specified a script rather than using the default (not specifying, and let travis run phpunit), perhaps for manual scripts travis just executes and the command is resolved like a standard command, meaning the travis order quoted above does not apply?

In any case, the build is fixed now, and the composer-installed phpunit is in fact that one that we want to run.

aacotroneo commented 5 years ago

amazing! thanks so much!! I'll take a look later today, and give it (only) a few days in case other users have comments. But as it's a new version, we'll have a chance to update if anything pops up in the future.

You beat me with the tests - I was getting tired of that false error!

breart commented 5 years ago

I have a bit of feedback in here.

1) Do you think using IDP keys represented as strings is safe? I think the approach with UUID tokens would be safer. 2) Imagine you have more than 5 IdPs — you have to manage them manually and it will become annoying. It's also a problem if you want to have UI to configure IdPs. 3) If you resolve IdP based on the URL, it would make more sense to do using middlewares 4) Some parts of code look pretty.....hardcoded:

I thought this library was abandoned so I had to implement those features by myself. The issues I've described above already implemented there, so be welcome to grab something useful :)

darynmitchell commented 5 years ago
  1. Do you think using IDP keys represented as strings is safe

I agree, internally for my project config I am representing them as random strings of a certain length. nirajp's design doesn't stop one from using a UUID as the string.

  1. [manual configuration]

Programmatic configuration is out of the scope of this PR, but could be a future improvement to the library

  1. Some parts of code look pretty.....hardcoded: https://github.com/aacotroneo/laravel-saml2/pull/187/files#diff-8bac3a9a1f0897afd3f94379d72db579R27 ... https://github.com/aacotroneo/laravel-saml2/pull/187/files#diff-afa1b594e46cb5864abbeb10131850efR3

The 1st and 3rd link you linked to, I think could be improved by: instead of iterating on the routes, using a route group prefix with parameter, and fail the request if the parameter doesn't match one of the configured IdPs. And in fact I see in your repo (not fork?) you did use route parameter rather than the way this one "parses the request", and in middleware use that parameter to load IdP config. In keeping close to nirajp's design we could also do it without middleware, accessing the Route::parameter('idpName') (I think) in the controller constructor in place of the current parsing of the request path.

For what I viewed as my mandate, this falls into "improvements" which I intentionally didn't try to make. For the PR such improvements are welcome suggestions. One of the appeals of the nirajp fork was that the scope of design changes were minimal were pretty minimal compared to aacotroneo, except the move from singleton to dynamically instantiated OneLogin Auth. But the route parameter suggestion seems to me to keep the same design and just be better. I'm curious about other's thoughts on how how to approach these suggestions as well as other potential changes.

aacotroneo commented 5 years ago

I like it! I'd agree with brezzhnev in general terms, but the PR is 'good enough' to be merged.

This library's philosophy is to keep it short and simple (and a little hacky sometimes) and featureless, and as least dependable on Laravel as possible as well (*). OneLogin does most of the work, this library just has to give it control, and allows users to do their craziness in hooks, it doesn't try to solve anything else. That's the main reason why the library has been roughly 'unmantained', but working after 5 years (second reason is the contributors)!

(*) I regret we're using logger and session now - I'm not sure when I missed that, but it'd have been ideal to fire some event at the most, and let the user do whatever they want.

I'll give it some more time, I vote +1

aacotroneo commented 5 years ago

released!! https://github.com/aacotroneo/laravel-saml2/releases/tag/2.0.0 thank you very much!

darynmitchell commented 5 years ago

BTW @brezzhnev your saml2 rewrite looks solid, thanks for sharing it. 👍