Skip to content
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

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

Closed
sjohnr opened this issue Sep 27, 2022 · 3 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement

Comments

@sjohnr
Copy link
Member

sjohnr commented Sep 27, 2022

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 sjohnr added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Sep 27, 2022
@sjohnr
Copy link
Member Author

sjohnr commented Sep 27, 2022

@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().

@msosa
Copy link
Contributor

msosa commented Sep 27, 2022

@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

@sjohnr
Copy link
Member Author

sjohnr commented Sep 28, 2022

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants