Missing state parameter in Authorization Consent request throws 500

[guo-jun-airwallex]

Describe the bug
If client not pass state when authorization, it throws 500 error when user click cancel button in consent page.

The error was throwed in OAuth2AuthorizationCodeRequestAuthenticationToken

if (this.consent) {
    Assert.hasText(this.state, "state cannot be empty");
}

However click allow it can be redirected to client uri successful.

According to OAuth RFC, state is recommended in authorization endpoint.
So why we need to assert state when require consent?

To Reproduce

1. Client open authorization url without state
2. User login and click cancel button.
3. Display 500 error page
   

Expected behavior
state should be recommended for client, not only when user allow authorization but also reject.

[jgrandja]

@guo-jun-airwallex You are correct, the state parameter is not required in the Authorization Request, however, a different state parameter is required for the Authorization Consent Request.

The assertion check:

if (this.consent) {
    Assert.hasText(this.state, "state cannot be empty");
}

is applied against the Authorization Consent Request NOT the Authorization Request. FYI, the spec does not outline how to implement the Authorization Consent flow as this is left to implementors. The current implementation requires the state parameter in the Authorization Consent Request so it can correlate to the previous Authorization Request. Also note that the state parameter in the Authorization Request is not the same as the state parameter in the Authorization Consent Request.

I'm going to close this as works as designed.

[guo-jun-airwallex]

Hi @jgrandja

Plz check this line again:

String state = authorizationCodeRequestAuthentication.isConsent() && authorizationRequest != null ?
authorizationRequest.getState() : authorizationCodeRequestAuthentication.getState();

When redirectOnError but no redirect uri in authorizationCodeRequestAuthentication
Then in isConsent authorization request, it use the state in authorizationRequest, not the authorizationCodeRequestAuthentication, right?

However the state in authorizationRequest is recommended, then it throws error when build authorizationCodeRequestAuthenticationResult

if (this.consent) {
    Assert.hasText(this.state, "state cannot be empty");
}

Sorry, I still think it is a bug and plz confirm it again, so I want to reopen this issue.

[jgrandja]

@guo-jun-airwallex The Authorization Consent flow you're simulating using Postman is not correct. Please review and run the messages sample to understand the Authorization Request -> Authorization Consent flow.

FYI, this flow has been working for quite some time now. If you still feel there is a bug, then please review the tests in OAuth2AuthorizationCodeRequestAuthenticationProviderTests and provide a test that reproduces the error to confirm that a bug exists. The test should look similar to the tests starting with authenticateWhenConsentRequest*.

[guo-jun-airwallex]

@jgrandja
Sorry to reply later in Monday.

Here is my test:

        @Test
	public void authenticateWhenConsentRequestNotApprovedAndStateIsNullInRequestThenThrowOAuth2AuthorizationCodeRequestAuthenticationException() {
		RegisteredClient registeredClient = TestRegisteredClients.registeredClient()
				.build();
		when(this.registeredClientRepository.findByClientId(eq(registeredClient.getClientId())))
				.thenReturn(registeredClient);
		OAuth2Authorization authorization = authorization(registeredClient, Collections.emptyMap())
				.principalName(this.principal.getName())
				.build();
		OAuth2AuthorizationCodeRequestAuthenticationToken authentication =
				authorizationConsentRequestAuthentication(registeredClient, this.principal)
						.scopes(new HashSet<>())	// No scopes approved
						.build();
		when(this.authorizationService.findByToken(eq(authentication.getState()), eq(STATE_TOKEN_TYPE)))
				.thenReturn(authorization);

		OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute(OAuth2AuthorizationRequest.class.getName());

		assertThatThrownBy(() -> this.authenticationProvider.authenticate(authentication))
				.isInstanceOf(OAuth2AuthorizationCodeRequestAuthenticationException.class)
				.satisfies(ex ->
						assertAuthenticationException((OAuth2AuthorizationCodeRequestAuthenticationException) ex,
								OAuth2ErrorCodes.ACCESS_DENIED, OAuth2ParameterNames.CLIENT_ID, authorizationRequest.getRedirectUri())
				);

		verify(this.authorizationService).remove(eq(authorization));
	}

	private static OAuth2Authorization.Builder authorization(RegisteredClient registeredClient,
			Map<String, Object> authorizationRequestAdditionalParameters) {
		OAuth2AccessToken accessToken = new OAuth2AccessToken(
				OAuth2AccessToken.TokenType.BEARER, "access-token", Instant.now(), Instant.now().plusSeconds(300));
		return authorization(registeredClient, accessToken, Collections.emptyMap(), authorizationRequestAdditionalParameters);
	}

	private static OAuth2Authorization.Builder authorization(RegisteredClient registeredClient,
			OAuth2AccessToken accessToken, Map<String, Object> accessTokenClaims,
			Map<String, Object> authorizationRequestAdditionalParameters) {
		OAuth2AuthorizationCode authorizationCode = new OAuth2AuthorizationCode(
				"code", Instant.now(), Instant.now().plusSeconds(120));
		OAuth2RefreshToken refreshToken = new OAuth2RefreshToken(
				"refresh-token", Instant.now(), Instant.now().plus(1, ChronoUnit.HOURS));
		OAuth2AuthorizationRequest authorizationRequest = OAuth2AuthorizationRequest.authorizationCode()
				.authorizationUri("https://provider.com/oauth2/authorize")
				.clientId(registeredClient.getClientId())
				.redirectUri(registeredClient.getRedirectUris().iterator().next())
				.scopes(registeredClient.getScopes())
				.additionalParameters(authorizationRequestAdditionalParameters)
				// .state("state") state is null in authorization request
				.build();
		return OAuth2Authorization.withRegisteredClient(registeredClient)
				.id("id")
				.principalName("principal")
				.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
				.token(authorizationCode)
				.token(accessToken, (metadata) -> metadata.putAll(tokenMetadata(accessTokenClaims)))
				.refreshToken(refreshToken)
				.attribute(OAuth2ParameterNames.STATE, "state")
				.attribute(OAuth2AuthorizationRequest.class.getName(), authorizationRequest)
				.attribute(Principal.class.getName(),
						new TestingAuthenticationToken("principal", null, "ROLE_A", "ROLE_B"))
				.attribute(OAuth2Authorization.AUTHORIZED_SCOPE_ATTRIBUTE_NAME, authorizationRequest.getScopes());
	}

	private static Map<String, Object> tokenMetadata(Map<String, Object> tokenClaims) {
		Map<String, Object> tokenMetadata = new HashMap<>();
		tokenMetadata.put(OAuth2Authorization.Token.INVALIDATED_METADATA_NAME, false);
		if (CollectionUtils.isEmpty(tokenClaims)) {
			tokenClaims = defaultTokenClaims();
		}
		tokenMetadata.put(OAuth2Authorization.Token.CLAIMS_METADATA_NAME, tokenClaims);
		return tokenMetadata;
	}


	private static Map<String, Object> defaultTokenClaims() {
		Map<String, Object> claims = new HashMap<>();
		claims.put("claim1", "value1");
		claims.put("claim2", "value2");
		claims.put("claim3", "value3");
		return claims;
	}

I only change the state in authorization request to null:

Then the test result is

[guo-jun-airwallex]

Hi @jgrandja
Hope you can check my test when you are free.
The code of my test
authenticateWhenConsentRequestNotApprovedAndStateIsNullInRequestThenThrowOAuth2AuthorizationCodeRequestAuthenticationException
is same with authenticateWhenConsentRequestNotApprovedThenThrowOAuth2AuthorizationCodeRequestAuthenticationException

The only one line I modified is commented

// .state("state") state is null in authorization request

It should throw OAuth2AuthorizationCodeRequestAuthenticationException with access denied error code, not IllegalArgumentException

[guo-jun-airwallex]

Hi @jgrandja
Sorry to bother your, could you check my test and confirm it again?

[jgrandja]

@guo-jun-airwallex Thanks for providing the test as it did reveal a validation check was missing in OAuth2AuthorizationCodeRequestAuthenticationConverter.

I'm about to push the fix just in time for today's release.
Thanks again for reporting this!

[guo-jun-airwallex]

#595

Actually the issue I reported before was the same as above one, thankfully the new version finally fixed it.