-
Notifications
You must be signed in to change notification settings - Fork 44
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 OidcAuthenticationCustomizer service provider interface #755
base: master
Are you sure you want to change the base?
Add OidcAuthenticationCustomizer service provider interface #755
Conversation
0c89533
to
15f49cb
Compare
@nscuro Suggestions from DependencyTrack/hyades#1632 were implemented. Let me know if that looks a little closer to what you would expect, and if the |
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.
Just a minor remark, otherwise looks good.
I guess the most important question is, does it help you in achieving what you want?
customizer.onAuthenticationSuccess(profile, idToken, accessToken); | ||
return qm.updateOidcUser(user); |
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.
Just to be safe, since updateOidcUser
could still fail:
customizer.onAuthenticationSuccess(profile, idToken, accessToken); | |
return qm.updateOidcUser(user); | |
user = qm.updateOidcUser(user); | |
customizer.onAuthenticationSuccess(profile, idToken, accessToken); | |
return user; |
Same for other invocations. You want to make sure onAuthenticationSuccess
is only called when you can be sure that the authentication succeeded.
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.
Do you think onAuthenticationSuccess
might benefit from accepting and/or returning an OidcUser
? So this would become something like
return customizer.onAuthenticationSuccess(qm.updateOidcUser(user), profile, idToken, accessToken);
That way, implementers would have access to user.setTeams()
, user.setPermissions()
, etc. inside onAuthenticateSuccess
, if you think that would be useful and if it seems like an acceptable design pattern
Ok thanks, I'll implement that suggestion for all calls to
What I think is needed:
Do you think this is currently achievable with these changes? Should the |
Signed-off-by: Jonathan Howard <[email protected]>
- Implement suggestions Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
adf94a7
to
398223f
Compare
This PR adds the capability to customize OIDC authentication with extended functionality if desired.