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

feat(settings): optional config option for sp entityId #932

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sniegel-mind4bytes
Copy link

Right now the service provider entityId is always generated based on the nextcloud instance URL. At least Keycloak relies on that entityId to resolve the corresponding SAML2 client. This PR introduces a new optional config option to allow the admin to set a different service provider entityId manually. If no manual configuration is done, the current behavior remains as the default.

@blizzz
Copy link
Member

blizzz commented Feb 10, 2025

@sniegel-mind4bytes hey and thanks for your contribution!

Could you elaborate where the current mechanism does not meet your needs?

@sniegel-mind4bytes
Copy link
Author

@sniegel-mind4bytes hey and thanks for your contribution!

Could you elaborate where the current mechanism does not meet your needs?

For me to have the SAML authentication work with Keycloak I have to use https://example.com/apps/user_saml/saml/metadata as the clientId for the SAML client on Keycloak. Instead of using the URL as clientId I would like to use something like "nextcloud". The issue extends a bit further as I have to use a single Keycloak for multiple staging environments. Hence the clientId should be something like, "test-nextcloud" or "prod-nextcloud". Indeed that would work with the current implementation of user_saml as the URLs differ for each stage. The thing is, nextcloud is the only SAML client requiring the URL as clientId. All the others give me the flexibility to use whatever clientId I like. Therefore I thought it would be great to give admins the same flexibility with user_saml as well. For those with no need, everything stays the same, as it is no requirement to set this config option, making this change completely backward compatible.

@sniegel-mind4bytes
Copy link
Author

@blizzz: Hi, any update? Let me know in case I can do something to push this pr further.

Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@blizzz
Copy link
Member

blizzz commented Feb 26, 2025

Hey @sniegel-mind4bytes, thanks for your patience (been off before and had a lot going on then) and for your explanation. Makes sense, I concur!

@blizzz
Copy link
Member

blizzz commented Feb 26, 2025

@sniegel-mind4bytes when i set the entityid it works, but when i unset it again, I run into an internal server error with Invalid array settings: sp_entityId_not_found. I suppose because the key is still present, alas empty. Could you double check that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants