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

[BC brake] Avoid DB for configuration #108

Closed
wants to merge 4 commits into from
Closed

Conversation

RoSk0
Copy link

@RoSk0 RoSk0 commented Nov 29, 2024

Coming from aacotroneo/laravel-saml2 I discovered multiple issues in this package:

I believe storing IDP configuration in the DB complicates a lot of things and most importantly makes it harder to maintain this project.

I've tried to consider reasons behind that move but couldn't find any. So now I'm proposing revert back to the v1 approach and get rid of the DB as the configuration storage.

Targeting master to simplify review, but significance of the change definitely warrant a new major version release. I know there was a start on v3 in #88 , but still believe the project and it's users would benefit from the simplicity and additional features that this change brings:

  • all configuration in one place
  • simplified configuration management utilizing environment
  • support off all features of the SAML PHP Toolkit

@breart
Copy link
Contributor

breart commented Dec 8, 2024

Hey @RoSk0, thank you for this PR. Valid issue observations and some of them definitely need to be addressed.

This forked solution has been developed in 24Slides as a result of the business needs we had, and we decided to open-source it so other people that share the same problem could use it.

The reasons behind the decision to store IdP configurations in the database were the following:

  1. We had over 10 different IdPs we worked with and that number was projected to increase to 50 or even 100. It made us realize keeping all those configurations in some kind of configuration files would result in higher maintenance cost
  2. Strategically, we wanted in the future to make it possible for customers to manage their IdP configuration right from their account settings page. Database would be the obvious choice here.

I hope it gives you needed context. This PR won't be merged due to the arguments stated above, but feel free to create a fork for simpler cases like yours.

@breart breart closed this Dec 8, 2024
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