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

Fix auto-discovery of OIDC details to be too eager #17389

Merged

Conversation

NobodysNightmare
Copy link
Contributor

@NobodysNightmare NobodysNightmare commented Dec 6, 2024

For custom providers it was not possible to overwrite the discovered default settings, because the discovery would be re-triggered every time that any update was performed, thereby overwriting the custom settings just entered.

The only way out so far was to completely remove the metadata_url from the provider.

For custom providers the discovery now only takes place on the form where the metadata_url is being entered, not on other forms. For Google and Microsoft discovery remains active on all forms. Due to further implementation details this means that on creation they will first fetch their metadata when saving the client details form, while during edit they will also refetch on the name form (where Entra allows changing the tenant, which influences the URLs).

Ticket

OP#59928

What approach did you choose and why?

I made the fetching of settings from the OIDC Discovery endpoint an optional behaviour of the UpdateService. This behaviour is now controllable through a query parameter to the update endpoint.

This solution provided the smallest delta to what we already have. It's discussable whether the whole behaviour should be extracted to somewhere else. For example it surprised me at first, that during creation of an Entra provider, we do not fetch the endpoint details. However, we never did that, since the CreateService doesn't have the corresponding code.

This leads to the somewhat inconsistent behaviour that custom providers will only refetch their details on one form, while Google and Entra will refetch on all forms.

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...) in a single browser

@NobodysNightmare NobodysNightmare added this to the 15.1.x milestone Dec 6, 2024
@NobodysNightmare NobodysNightmare requested a review from a team December 6, 2024 14:19
Copy link
Contributor

@mereghost mereghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems great! :D

For custom providers it was not possible to overwrite
the discovered default settings, because the discovery would be
re-triggered every time that any update was performed,
thereby overwriting the custom settings just entered.

The only way out so far was to completely remove the metadata_url
from the provider.

For custom providers the discovery now only takes place on the
form where the metadata_url is being entered, not on other forms.
For Google and Microsoft discovery remains active on all forms.
Due to further implementation details this means that on creation
they will first fetch their metadata when saving the client details
form, while during edit they will also refetch on the name form
(where Entra allows changing the tenant, which influences the URLs).
@oliverguenther
Copy link
Member

This is indeed weird practice. In SAML, we solved it in a way that this is a deliberate action (and service) to call the metadata discovery and update the model. The reasoning for this is that you might want to call this periodically in the background without actually updating the model.

https://github.com/opf/openproject/blob/release/15.1/modules/auth_saml/app/controllers/saml/providers_controller.rb#L49-L68

@NobodysNightmare NobodysNightmare merged commit c5ad07b into opf:release/15.1 Dec 9, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants