-
Notifications
You must be signed in to change notification settings - Fork 187
OIDC Enhancements #5637
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
OIDC Enhancements #5637
Conversation
fc3352a
to
00324ea
Compare
Thank you! Let me check for some weeks |
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
This PR was closed because it has been stalled for 7 days with no activity. Feel free to reopen if still applicable. |
6d65f31
to
e510d4e
Compare
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.
LGreatTM
Tested
It worked well. With the following control-plane-config.yaml and AWS Cognito configs, I could successfully use given_name
instead of username.
spec:
sharedSSOConfigs:
- name: cognito
provider: OIDC
oidc:
clientId: xxx
clientSecret: xxx
issuer: https://cognito-idp.xxx
redirect_uri: xxx
scopes:
- openid
- profile
usernameClaimKey: given_name #HERE
avatarUrlClaimKey: picture #HERE
Result: (the name is from given_name
!)
For reviewers
Let me explain this issue & PR in short:
- Needs: Users want to assign custom claims for username/role/avatarImage on PipeCD UI
- Before this PR: It was unable
- The reasons:
[1] There was no mapping between custom claims and username/role/avatarImage
[2] IDToken does not have custom claims in some IdPs such as Okta
- To get them, pipecd needs to access UserInfoEndpoint - The solution:
[1] Add mapping for custom claims byusernameClaimKey
etc. anddefaultUsernameClaimKeys
etc.
[2] Fetch custom claims from UserInfoEndpoint (here)
// then pass user-provided URLs to override the existing URLs in the providerConfig struct. | ||
// https://pkg.go.dev/github.com/coreos/go-oidc/[email protected]/oidc#NewProvider | ||
// https://pkg.go.dev/github.com/coreos/go-oidc/[email protected]/oidc#ProviderConfig | ||
func createCustomOIDCProvider(ctx context.Context, sso *model.ProjectSSOConfig_Oidc) (*oidc.Provider, error) { |
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.
I got it.
This func is based on https://github.com/coreos/go-oidc/blob/0fe98873951208147e6d412602432038c91cda54/oidc/oidc.go#L208 and basically the changed points are only overriding AuthorizationEndpoint etc. in providerConfig := oidc.ProviderConfig{
.
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.
Because this function is based on the code written by other authors, we should include copyright notice in some places.
The go-oidc's notice is here, so please include the copyright notice around this function and the modification summary.
https://github.com/coreos/go-oidc/blob/a7c457eacb849c163a496b29274242474a8f44ab/NOTICE
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.
@Warashi Hi, I’ve added the NOTICE. Could you please review it and let me know if it looks okay?
For example,
|
@kumo-rn5s verifier := c.Verifier(&oidc.Config{ClientID: c.sharedSSOConfig.ClientId}) |
Head branch was pushed to by a user without write access
Signed-off-by: ishikawa_kumo <[email protected]>
Signed-off-by: ishikawa_kumo <[email protected]>
Signed-off-by: ishikawa_kumo <[email protected]>
Signed-off-by: ishikawa_kumo <[email protected]>
Signed-off-by: ishikawa_kumo <[email protected]>
9ff9fb7
to
f324d67
Compare
Signed-off-by: Kumo Ishikawa <[email protected]>
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.
The codes are good. I commented on the copyright notice of the codes taken from the go-oidc package.
// then pass user-provided URLs to override the existing URLs in the providerConfig struct. | ||
// https://pkg.go.dev/github.com/coreos/go-oidc/[email protected]/oidc#NewProvider | ||
// https://pkg.go.dev/github.com/coreos/go-oidc/[email protected]/oidc#ProviderConfig | ||
func createCustomOIDCProvider(ctx context.Context, sso *model.ProjectSSOConfig_Oidc) (*oidc.Provider, error) { |
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.
Because this function is based on the code written by other authors, we should include copyright notice in some places.
The go-oidc's notice is here, so please include the copyright notice around this function and the modification summary.
https://github.com/coreos/go-oidc/blob/a7c457eacb849c163a496b29274242474a8f44ab/NOTICE
…ster/NOTICE Signed-off-by: ishikawa_kumo <[email protected]>
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.
Looks Great! Thank you!
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.
Thank you so much for amazing feature!!!! 👍 🙏
What this PR does:
Why we need it:
Which issue(s) this PR fixes:
Fixes #5330
Does this PR introduce a user-facing change?:
Yes
How are users affected by this change:
Something to note:
Is this a breaking change:
No
How to migrate (if breaking change):
Not applicable