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

MappedJwtClaimSetConverter#withDefaults doesn't remove claims from JWT as documented #10135

Closed
fguenci opened this issue Jul 21, 2021 · 2 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@fguenci
Copy link

fguenci commented Jul 21, 2021

Describe the bug
As stated in Spring Security Documentation, to remove a claim from a JWT just pass a converter for the claim in MappedJwtClaimSetConverter.withDefaults() that return null.
Actually this setting doesn't remove the claim from JWT.

I think the problem is in class org.springframework.security.oauth2.core.converter.ClaimTypeConverter, method convert.

	public Map<String, Object> convert(Map<String, Object> claims) {
		if (CollectionUtils.isEmpty(claims)) {
			return claims;
		}

		Map<String, Object> result = new HashMap<>(claims);
		this.claimTypeConverters.forEach((claimName, typeConverter) -> {
			if (claims.containsKey(claimName)) {
				Object claim = claims.get(claimName);
				Object mappedClaim = typeConverter.convert(claim);
				if (mappedClaim != null) {
					result.put(claimName, mappedClaim);
				}
			}
		});

		return result;
	}

I think that the result map should contain all the mapped claims even if its value is null because null value claims are removed later.

To Reproduce
Steps to reproduce:
I want to remove the NBF claim from jwt.
To do that I set in my jwtdecoder a converter that return null for this claim:

		var jwtDecoder = NimbusJwtDecoder.withJwkSetUri(jwkSetUri).build();
		var converter = MappedJwtClaimSetConverter.withDefaults(Collections.singletonMap(JwtClaimNames.NBF, nbfClaimValue -> null));
		jwtDecoder.setClaimSetConverter(converter);

Expected behavior
The decoded JWT doen't contains the NBF claim.

@fguenci fguenci added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jul 21, 2021
@marcusdacoregio marcusdacoregio added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 21, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Jul 26, 2021

I agree, @fguenci. Are you able to submit a PR with the change, including a test in MappedJwtClaimSetConverterTests that matches this use case?

@fguenci
Copy link
Author

fguenci commented Jul 27, 2021

ok, i'll try

@jzheaux jzheaux added this to the 5.6.0-M2 milestone Jul 27, 2021
fguenci pushed a commit to fguenci/spring-security that referenced this issue Jul 27, 2021
to null.

This commit allows ClaimTypeConverter to overwrite and return claim with
value converted to null.

Closes spring-projects#10135
fguenci added a commit to fguenci/spring-security that referenced this issue Jul 28, 2021
Prior to this commit ClaimTypeConverter returned the claims with the
original value for all the claims with a null converted value.
The changes allows ClaimTypeConverter to overwrite and return claims
with converted value of null.

Closes spring-projectsgh-10135
jzheaux added a commit that referenced this issue Aug 12, 2021
Preserves the original behavior of ClaimTypeConverter so that its
converters can maintain their default behavior of null meaning that
conversion failed.

Issue gh-10135
jzheaux pushed a commit that referenced this issue Aug 16, 2021
Prior to this commit ClaimTypeConverter returned the claims with the
original value for all the claims with a null converted value.
The changes allows ClaimTypeConverter to overwrite and return claims
with converted value of null.

Closes gh-10135
@spring-projects-issues spring-projects-issues added the status: backported An issue that has been backported to maintenance branches label Aug 16, 2021
jzheaux pushed a commit that referenced this issue Aug 16, 2021
Prior to this commit ClaimTypeConverter returned the claims with the
original value for all the claims with a null converted value.
The changes allows ClaimTypeConverter to overwrite and return claims
with converted value of null.

Closes gh-10135
jzheaux pushed a commit that referenced this issue Aug 16, 2021
Prior to this commit ClaimTypeConverter returned the claims with the
original value for all the claims with a null converted value.
The changes allows ClaimTypeConverter to overwrite and return claims
with converted value of null.

Closes gh-10135
jzheaux pushed a commit that referenced this issue Aug 17, 2021
Prior to this commit ClaimTypeConverter returned the claims with the
original value for all the claims with a null converted value.
The changes allows ClaimTypeConverter to overwrite and return claims
with converted value of null.

Closes gh-10135
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
Prior to this commit ClaimTypeConverter returned the claims with the
original value for all the claims with a null converted value.
The changes allows ClaimTypeConverter to overwrite and return claims
with converted value of null.

Closes spring-projectsgh-10135
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
Preserves the original behavior of ClaimTypeConverter so that its
converters can maintain their default behavior of null meaning that
conversion failed.

Issue spring-projectsgh-10135
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) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
4 participants