Issue #12644 - use builder API to construct OpenIdConfiguration #12682
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
...ations/jetty-openid/src/main/java/org/eclipse/jetty/security/openid/OpenIdConfiguration.java
Show resolved
Hide resolved
| { | ||
| issuer(issuer); | ||
| clientId(clientId); | ||
| clientSecret(clientSecret); | ||
| } |
There was a problem hiding this comment.
I like this alternative best I think
| { | |
| issuer(issuer); | |
| clientId(clientId); | |
| clientSecret(clientSecret); | |
| } | |
| { | |
| this(); | |
| issuer(issuer); | |
| clientId(clientId); | |
| clientSecret(clientSecret); | |
| } |
| private OpenIdConfiguration(@Name("issuer") String issuer, | ||
| @Name("clientId") String clientId, | ||
| @Name("clientSecret") String clientSecret, | ||
| @Name("authorizationEndpoint") String authorizationEndpoint, | ||
| @Name("tokenEndpoint") String tokenEndpoint, | ||
| @Name("endSessionEndpoint") String endSessionEndpoint, | ||
| @Name("authenticationMethod") String authenticationMethod, | ||
| @Name("httpClient") HttpClient httpClient, | ||
| @Name("authenticateNewUsers") boolean authenticateNewUsers, | ||
| @Name("logoutWhenIdTokenIsExpired") boolean logoutWhenIdTokenIsExpired, | ||
| @Name("scopes") List<String> scopes) |
There was a problem hiding this comment.
You don't need the @name annotation on a private constructor as it will never be used in XML
| .authorizationEndpoint("") | ||
| .tokenEndpoint("") |
There was a problem hiding this comment.
If there is a semantic difference between "", null and not set at all, then you need to say so in the javadoc
There was a problem hiding this comment.
A value of null means it hasn't been set. And it already says in the javadoc if this has not been set it will search for the OpenID metadata at .well-known/openid-configuration.
The authorizationEndpoint/tokenEndpoint value of "" has no special meaning, and will not actually work. In this test I just need a non-null value.
So there is no point mentioning "" specifically in the javadoc because it means nothing and is just an incorrect value.
There was a problem hiding this comment.
Could you set it to "test" instead? That avoids the confusion if "" is unsetting it or not
Signed-off-by: Lachlan Roberts <[email protected]>
| .authorizationEndpoint("") | ||
| .tokenEndpoint("") |
There was a problem hiding this comment.
Could you set it to "test" instead? That avoids the confusion if "" is unsetting it or not
closes #12644