Log a warning when AuthorizationGrantType does not exactly match a pre-defined constant
#12087
Conversation
|
@msosa Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@msosa Thank you for signing the Contributor License Agreement! |
spring-projects-issues
added
the
status: waiting-for-triage
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/security/oauth2/client/registration/ClientRegistration.java
Outdated
Show resolved
Hide resolved
sjohnr
added
status: duplicate
There was a problem hiding this comment.
Hi @msosa! Apologies for the delay reviewing this PR again. We've been heads down working on the 6.0 release.
There are a couple of additional items to address below. Also, please update the copyright year in the header. Please ensure the branch is rebased on 5.8.x and commits are squashed. Lastly, please add Closes gh-11905 on a separate line in the commit message.
Would you like to address yourself? Or would you like me to address them in a polish commit to get this in time for 5.8.0?
| @@ -46,6 +50,12 @@ | |||
| public final class ClientRegistration implements Serializable { | |||
|
|
|||
| private static final long serialVersionUID = SpringSecurityCoreVersion.SERIAL_VERSION_UID; | |||
| private static final Log logger = LogFactory.getLog(ClientRegistration.class); | |||
|
|
|||
| private static final List<AuthorizationGrantType> authorizationGrantTypes = | |||
There was a problem hiding this comment.
Can this be adjusted to be a constant name (all caps, e.g. AUTHORIZATION_GRANT_TYPES)?
There was a problem hiding this comment.
I would also suggest moving this down to the Builder.
There was a problem hiding this comment.
(Clarification: I would suggest moving both lines down into the Builder and the logger would reference ClientRegistration.Builder.class instead.)
| for (AuthorizationGrantType authorizationGrantType : authorizationGrantTypes) { | ||
| if (authorizationGrantType.getValue().equalsIgnoreCase(this.authorizationGrantType.getValue()) && | ||
| !authorizationGrantType.equals(this.authorizationGrantType)) { | ||
| logger.warn(LogMessage.format("AuthorizationGrantType: %s does not match the pre-defined" + |
There was a problem hiding this comment.
You can run ./gradlew format to format these changes per the contributing guidelines.
There was a problem hiding this comment.
Done, also made me realize I was missing a space.
msosa
force-pushed
the
client-registration-warnings
branch
from
117228c to
1b96295
Compare
…e-defined constant Closes spring-projectsgh-11905
msosa
force-pushed
the
client-registration-warnings
branch
from
1b96295 to
053d944
Compare
|
@sjohnr I have updated the review, if there is anything I missed or something else you noticed, please feel free to do a polish commit to get this into 5.8.0 |
This adds the warning when
AuthorizationGrantTypedoes not exactly match what is expected as discussed on gh-11905