-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add acr_values support for OIDC #2418
Conversation
Created a PR for the website too -> dexidp/website#110 looking for your feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello there! First of all, thank you for the PR. I have a couple of suggestions. Please look through the, when you have time.
-
Because RFC states that
act_values is a space-separated string that specifies the acr values
Let's use the same structure for the
acrValues
config option for scopes - a slice of strings. -
In the
Open
method of the connector, compose the values of the slice into a single string. -
What do you think about validating the
acr_values_supported
received from the discovery OIDC enpoint before sending a request or event on connector creation? Does it seem to be a good feature, or is it just a complication without benefits?
Signed-off-by: Engin Diri <[email protected]>
Hi @nabokihms, thanks for the feedback! I changed your suggestions in the code.
I would create for this, if needed in the future a seperate issue. Our company Looking for your feedback. |
I updated the website too, as we have now a list instead a string |
Overview
What this PR does / why we need it
This PR adds acr_values support for the OIDC connection
Fixes: #2417
Special notes for your reviewer
Does this PR introduce a user-facing change?
yes, a new non-mandatory field is introduced called
acrValues
in the config of the OIDC connection