spring-security icon indicating copy to clipboard operation
spring-security copied to clipboard

Log a warning when `AuthorizationGrantType` does not exactly match a pre-defined constant

Open sjohnr opened this issue 3 years ago • 3 comments

Expected Behavior

When building a ClientRegistration and passing a string to the AuthorizationGrantType constructor, invalid grant types that match case insensitively with a pre-defined constant could log a warning informing users that it won't match a valid OAuth2AuthorizedClientProvider.

Current Behavior

The AuthorizationGrantType constructor accepts any string (including a capitalized grant type string, e.g. CLIENT_CREDENTIALS), assuming it is a custom grant type. This allows an application to start up and load a ClientRegistration without warnings, but does not work as expected because no OAuth2AuthorizedClientProvider is matched.

Context

Issue gh-11897

sjohnr avatar Sep 27 '22 03:09 sjohnr

@msosa (see comment):

Would the warning show if there are any capitals in the value, or would it only show if a value you input is similar to one of the spring-provided defaults?

I think it would make sense to log a warning if it does not strictly equal, but does equal case-insensitively (e.g. equalsIgnoreCase()).

And would this warning be inside the constructor?

I think it would go in ClientRegistration.Builder.build().

sjohnr avatar Sep 27 '22 03:09 sjohnr

@sjohnr I came up with this approach inside ClientRegistration.Builder, let me know if the implementation is ok or if you have any suggestions.

		private final Log logger = LogFactory.getLog(getClass());

		private void validateAuthorizationGrantType() {
			Stream.of(AuthorizationGrantType.AUTHORIZATION_CODE, AuthorizationGrantType.IMPLICIT,
							AuthorizationGrantType.CLIENT_CREDENTIALS, AuthorizationGrantType.REFRESH_TOKEN,
							AuthorizationGrantType.PASSWORD)
					.filter(defaultGrantType -> !defaultGrantType.equals(this.authorizationGrantType)
							&& defaultGrantType.getValue().equalsIgnoreCase(this.authorizationGrantType.getValue()))
					.forEach(defaultGrantType ->
							logger.warn(LogMessage.format("AuthorizationGrantType: %s does not match the pre-defined" +
									"constant %s and won't match a valid OAuth2AuthorizedClientProvider",
									this.authorizationGrantType, defaultGrantType)));
		}

will make a PR to follow if this is an ok approach

msosa avatar Sep 27 '22 05:09 msosa

@msosa while this is for initialization only, we generally avoid the use of streams for internal framework usage. But otherwise, this would be a good start, yes!

sjohnr avatar Sep 28 '22 18:09 sjohnr