search

codegreencreative / laravel-samlidp

Laravel SAML IdP
MIT License
232 stars 84 forks source link

allow loading a sp certificate from path #91

Closed qwertz182 closed 1 year ago

qwertz182 commented 2 years ago

Pull request #89

It seems my IDE messed up the whitespaces at some points. It's the default setting for the projects in our company. Hope this is not an issue.

upwebdesign commented 2 years ago

@as-dialoggroup this looks very simple and effective. You have tested this, correct? If so, I would like to get this merged in.

upwebdesign commented 2 years ago

@as-dialoggroup also, it seems the README should also be updated to describe how this should be used, would you please update the appropriate section of the README with some examples perhaps? Thank you for your contribution!

chindit commented 2 years ago

Why not using the same as for metadata ?

$cert = Storage::disk('samlidp')->get(config('samlidp.certname', 'cert.pem')); (MetadataController.php:22)

JaZo commented 2 years ago

Why not using the same as for metadata ?

That would be awesome! We use this package in one of our projects and are migrating to Kubernetes hosting. In our scenario it's easier to load secrets from ENV rather than files. I'll get a PR ready for that!

qwertz182 commented 2 years ago

Why not using the same as for metadata ? $cert = Storage::disk('samlidp')->get(config('samlidp.certname', 'cert.pem')); (MetadataController.php:22)

Wouldn't that be a breaking change? That way, setting the certificate as a string is not possible anymore.

JaZo commented 2 years ago

@as-dialoggroup, I've implemented that in a non breaking way: #95

Treggats commented 1 year ago

@as-dialoggroup @JaZo we're interested in the environment settings approach. Since Lambda makes using a file pretty hard. For context: we are using Lavavel Vapor for our application.

Is there any progress on this?

JaZo commented 1 year ago

@Treggats, I also like to get these changes through! Maybe you can help test my PR and leave a review? We're currently using my fork/PR in production and it's running smooth for months now.

Treggats commented 1 year ago

@JaZo I just deployed it to a dev environment. With one addition, Laravel Vapor/Lambda doesn't seem to like the xml opening tag. With that removed it works like a charm.

Edit; thought this was #95 🙈

Treggats commented 1 year ago

While the code looks good, I can't test it as it still requires a file. Which isn't possible on Laravel Vapor/Lambda.

upwebdesign commented 1 year ago

@Treggats @JaZo @as-dialoggroup ,

This is some nice dialog, great job. Sorry I have not contributed to the conversation up to this point. I want to get this released to a new version. However, I am unclear whether or not this is a breaking change. I am also not currently active in using this package, so time would be an issue to get this adequately tested. I would rely on you to determine if this can be merged and released. If it is clear to release, please confirm. Thanks!

Treggats commented 1 year ago

@upwebdesign as mentioned before, testing this PR is a bit hard because lambda doesn't allow files. But I found a way around that, so will try to test it and come back to you on that

Treggats commented 1 year ago

@upwebdesign Sidenote regarding #95 @JaZo mentioned that he has that running in production for quite some time now. I've tested it on my side as well and seems to work well

upwebdesign commented 1 year ago

@Treggats, so if I'm understanding correctly, both PR's could be merged in and this would provide you with the functionality you are looking for.

Treggats commented 1 year ago

@upwebdesign looking good. Works locally with our testsuite 👌

JaZo commented 1 year ago

We don't need this particular fix for our app, but I've also tested it and can confirm it's backwards compatible. Would be nice if we can get this and #95 merged and released! 🚀