-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add ability to store SAML Service Providers in database #153
Add ability to store SAML Service Providers in database #153
Conversation
@upwebdesign do you feel this is inline with the package? It's not ready to merge, but want to check you would be happy with this continuing? |
@JamesMowatt, I love this idea. I have never liked defining the service providers in the |
@upwebdesign I will continue down this route, will test and tag you when it's ready. Thanks |
@upwebdesign this is ready for review, I have tested it on my local SAML workbench. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the table name, I think this looks good! Let me know your thoughts on renaming the table in the database.
*/ | ||
public function up(): void | ||
{ | ||
Schema::create('saml_service_providers', function (Blueprint $table) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should prefix the package name with the table name.
laravel_samlidp_service_providers
This naming convention will clarify what the table is for and reduce the likelihood of a collision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely happy with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@upwebdesign that is ready to look at again.
database/migrations/2025_01_29_095333_create_saml_service_providers_table.php
Outdated
Show resolved
Hide resolved
database/migrations/2025_01_29_095333_create_saml_service_providers_table.php
Outdated
Show resolved
Hide resolved
@upwebdesign this is ready for review, hope everything is sorted. |
@JamesMowatt, it appears there is a compatibility issue with a method in the migration. |
It appears this code is the fix. The path may need to be updated. $this->publishes([
__DIR__ . '/../database/migrations' => database_path('migrations'),
], 'migrations'); |
This PR adds the functionality allowing you to declare service providers within the database rather than a fixed config file.
To use the database instead, set the
samlidp.sp
to be\CodeGreenCreative\SamlIdp\Models\SamlServiceProvider::class
instead of the config array.