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

Changes to project structure and code cleanup #155

Open
wants to merge 15 commits into
base: remove_mcrypt
Choose a base branch
from
Open

Changes to project structure and code cleanup #155

wants to merge 15 commits into from

Conversation

tjallingt
Copy link

This sort of depends on/overwrites #151

I modernised the project structure to follow psr-4 (instead of psr-0) and removed a bunch of IMO strange code. Also updated the tests, refactored the service provider and added more tests. Some of my changes might have been too aggressive (composer.json php version and changes to config for example) but we can discuss what will and won't work based around the contents of this PR.

There are a couple of other changes I'm considering:

  • I'd like to remove the OneLogin singleton registration from the service provider as it isn't adding anything. This library's Auth class should probably receive the Laravel config, massage it into the right format and create a OneLogin instance.
  • OneLogin seems to already strip the leading and ending tags from openssl certificates and private keys so maybe the ExtractOpenssl class can be removed entirely at some point. (see https://github.com/onelogin/php-saml/blob/3.0.0/src/Saml2/Utils.php#L189-L272)
  • be stricter about how the certificates and private key are configured; I dislike pasting the entire contents in the .env file and the use of file:// is also not my favourite so maybe we can just specify a folder and only configure filenames?
  • Use Laravel Redirects instead of relying on OneLogin to redirect us.

Even larger changes I'd like to see eventually:

  • Don't use an event to signal a login attempt
  • Instead of having users configure this manually supply a trait like Laravel's default AuthorizesUsers on the login controller

Thoughts and input are very welcome 😄

@aacotroneo
Copy link
Owner

the cleanup is nice, but I'd like to keep the library flexible, just a thin wrapper of one-login. The more logic we put in, the easier to use, but the least cases it supports. I don't mean to make this library easier to use than OneLogin, just do all the connections it needs with Laravel.

For example, why hardcode $config['sp']['assertionConsumerService']['url'] = URL::route('saml2.acs'); ? That forces all users to use that implementation. If you want custom behavior extend the controller or create your own, and use your routes... you could remove the events if you wanted that way.
Events were a way I found to hand control over to the user code in a decoupled way... as long as you don't enforce an implementation or rely on session, we can evaluate other options.

@tjallingt
Copy link
Author

Well in the end you should be able to cherry pick the commits you like and leave out the ones you don't from this PR, the ideas I listed are plans that I have for continuing my own work on this library, that doesn't mean I expect you to merge it. Two versions of a library with different ideas about UX/DX should be able to coexist 😄.

I hardcoded specific behaviour because I feel this library should give users less options and be more opinionated so that this library becomes smaller and easier to use.
Additionally the way I made the routes work now is the exact way it works in Laravel Horizon (see https://github.com/laravel/horizon/blob/1.0/src/HorizonServiceProvider.php#L44-L58. I believe Laravel Nova uses a system like this as well) so it feels very Laravel-y.

For this same reason I want to be more opinionated about how users should supply the crt/key files, less options means less code and less breakage.

As I mentioned my end goal would be to implement a trait Aacotroneo\Saml2\AuthenticatesUsersWithSaml which should be (as close as possible to) a drop-in replacement for Laravel's Illuminate\Foundation\Auth\AuthenticatesUsers used in the default login controller.
This would get rid of the controller in this library entirely and allow users to override whatever functionality they want while allowing this library to offer something that "just-works" out of the box without having to define an event handler (which as you say is decoupled which IMO is a bad thing when it's such a crucial part of logging a user in to the application).

Hope this makes sense. If you are not up for merging any of what I'm describing, that's fine. Like I mentioned before I think I'm still going to experiment with these changes are see if it works like I think it might. Regardless I'd love to know what you think.

@aacotroneo
Copy link
Owner

I get your point.. I see the value on it, but we can't drop support for our current users, and both approaches have some deep differences in design. We'll need to keep updating this library a little bit to add support to newer PHP versions and keep up to date with oneLogin - I don't think we can make both compatible.

May be you can do a fork tjallingt/laravel-simple-saml2? (or the name you like) and we can update the REDME to explain how both libraries relate. I'd be happy to help users migrate towards your library to take more advantage of Laravel. I'm not using Laravel right now, so I can't give more support than maintaining what we have.

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.

2 participants