Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 OIDC authentication #6534
Add OIDC authentication #6534
Changes from 21 commits
94bf748
48a9daa
5c8df51
93c15a5
1357814
2701c8e
d987909
6401562
efca4e3
d6326af
676a0df
b0f0f8d
504a9c4
c625086
f56cd5b
49d3a8f
4873fba
31b6998
4676667
0025553
0edb066
f67770c
5109ea7
edef763
07277c1
47361cc
1e447cf
b767c8d
e7ac9af
fd8cbea
a889f7a
395b133
633fab1
308ff63
f7ad5e6
f6e7c5b
221fd64
4ace8f1
5e46a3d
13dc1ff
390647c
90de845
d94f43f
caae3ce
d200266
0694935
d851569
c2d6f37
f119cf0
0bbfd99
3a5f1d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should be WkConf.Http.uri instead of hard-coded localhost:9000
And I think it would be fair to move this to the OpenIdConnectClient class, as it is only used there
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'm not sure about moving this to the oidcc class, because the client should exist independently of the controller and the callback url is very specific controller knowledge
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.
(if I understand correctly that OpenIdConnect uses OpenConnectIds? Seems confusing to me, but may be correct if that’s what the protocol states)
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.
This is not something that is directly part of the protocol. I renamed it to OpenIdConnectClaimSet since it describes it more accurately
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’d add the
get
becauseredirect
also reads a bit like a verb.I’d say it is not necessary to pass the parameters in from the caller, but rather have lazy vals for the client itself, I thinkt it already has everything it needs to construct them (mostly the injected WkConf).
Maybe the OpenIdConnectConfig case class is not needed at all, but the fields can be read from the passed WkConf where needed?
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.
Redirect URL as stated above should not be known to the oidcclient IMO. The oidcconfig info can also be gathered at the client, true. However, the case class can be useful IMO because it allows for checking if the configuration is valid (and thus determine if the routes are activated)