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 editing and auto-discovering supported grant types of OIDC providers #17376

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

NobodysNightmare
Copy link
Contributor

@NobodysNightmare NobodysNightmare commented Dec 5, 2024

This is in preparation to enable authentication flows based on token exchange in the future. Using the grant_types_supported we can determine whether the IDP is capable of exchanging tokens and then use this information to indicate to users whether they are able to set-up token exchange based authentication for their storage provider.

Ticket

OP#58862

What are you trying to accomplish?

Allow to decide for a given IDP, whether it's able to perform token exchange or not.

What approach did you choose and why?

We store the list of grant types available on the IDP, since this is a feature agnostic way to store information about the provider. This information is then used to implement the Provider#token_exchange_capable? method, which is specific to the token exchange feature.

For editing the discovered information, I kept the UI simple by using a text input that accepts the grants in a space-delimited fashion. This solution was merely chosen to keep implementation complexity low, but is probably not the best UX we can offer. Assuming that most users will fill this information through a discovery endpoint, the solution might be sufficient, though.

Screenshots

image

Merge checklist

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

@NobodysNightmare NobodysNightmare requested a review from a team December 5, 2024 16:36
@NobodysNightmare NobodysNightmare force-pushed the select-keycloak-provider branch from 37b42bc to 54307f0 Compare December 6, 2024 09:57
@NobodysNightmare NobodysNightmare marked this pull request as draft December 6, 2024 09:58
@NobodysNightmare NobodysNightmare changed the title Allow to choose Keycloak as separate provider Allow editing and auto-discovering supported grant types of OIDC providers Dec 6, 2024
@NobodysNightmare NobodysNightmare force-pushed the select-keycloak-provider branch from 54307f0 to 4b5dee8 Compare December 6, 2024 15:23
@NobodysNightmare NobodysNightmare marked this pull request as ready for review December 6, 2024 15:28
@NobodysNightmare
Copy link
Contributor Author

Re: Rubocop/Reviewdog

Spec path should end with openid_connect/provider*_spec.rb.

Is this intended or wrongly configured? I see that the spec right next to this one ignores this rule as well (it's not placed inside an openid_connect folder. Considering that all our specs are inside modules/openid_connect, this cop should probably be adapted.

Though, I am surprised that it's not already adapted correctly, so I assume there's a reason for that.

@NobodysNightmare NobodysNightmare force-pushed the select-keycloak-provider branch 4 times, most recently from 8cf08c3 to 7df9935 Compare December 9, 2024 10:07
This value is also auto-discovered from a discovery endpoint.
@NobodysNightmare NobodysNightmare force-pushed the select-keycloak-provider branch from 7df9935 to 630712e Compare December 9, 2024 11:26
Copy link
Contributor

@brunopagno brunopagno left a comment

Choose a reason for hiding this comment

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

This looks pretty cool, although I have limited understanding of the OIDC topic 😬

Would be nice to have some visual information on the PR to understand better the UI changes, but from the code I think it's okay as is.

Left some ignorable comments

✔️

@NobodysNightmare
Copy link
Contributor Author

@brunopagno I added a screenshot to the PR description. I think the UI is an important limitation in this PR, because I put simplicity over the best possible UX here.

I still think that's fair, because I expect least people to have to manually change this stuff, because auto-discovery is how this is usually set up.

@brunopagno
Copy link
Contributor

@NobodysNightmare nice! I can think of two possible alternatives if we want to make this more defensive/explicit to the user.

  1. Put a hint text below (or somewhere where it makes sense) saying something like "you should only change this if you know what you're doing".
  2. Make the field disabled by default and have a button to explicit enable editting it.

Ultimately this falls into the same discussion we were having the other day about hand holding users. I can see someone accidentally break things by editting this field. But I would also be okay with pushing the current implementation and keeping track of improvements for the future so the ✔️ stays 😅.

...

All the above said, I think this should be a product decision. I don't have all the OIDC context to be able to say confidently who is going to use this and how so I might be guessing the wrong direction 😬

@NobodysNightmare
Copy link
Contributor Author

Cool. I am going to merge this MR as-is for now. The "only change if you know what you are doing" comment is actually part of the existing UI already (kind of):

image

@NobodysNightmare NobodysNightmare merged commit 81b7579 into opf:dev Dec 16, 2024
10 checks passed
@NobodysNightmare NobodysNightmare deleted the select-keycloak-provider branch December 16, 2024 09:34
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.

2 participants