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

allow loading a sp certificate from path #91

Merged
merged 4 commits into from
Dec 19, 2022

Conversation

qwertz182
Copy link
Contributor

@qwertz182 qwertz182 commented Apr 19, 2022

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
Copy link
Contributor

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

@upwebdesign
Copy link
Contributor

@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
Copy link

chindit commented Jul 12, 2022

Why not using the same as for metadata ?

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

@JaZo
Copy link
Contributor

JaZo commented Jul 12, 2022

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
Copy link
Contributor Author

  • tested
  • merged
  • updated readme
  • tested again

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
Copy link
Contributor

JaZo commented Sep 2, 2022

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

@Treggats
Copy link

@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
Copy link
Contributor

JaZo commented Dec 12, 2022

@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
Copy link

Treggats commented Dec 12, 2022

@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
Copy link

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
Copy link
Contributor

@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
Copy link

@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
Copy link

@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
Copy link
Contributor

@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
Copy link

@upwebdesign looking good. Works locally with our testsuite 👌

@JaZo
Copy link
Contributor

JaZo commented Dec 14, 2022

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! 🚀

@upwebdesign upwebdesign merged commit 65c70c1 into codegreencreative:master Dec 19, 2022
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.

6 participants