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

getClaimAsBoolean should not be falsy #10148

Closed
jzheaux opened this issue Jul 28, 2021 · 3 comments · Fixed by #10356
Closed

getClaimAsBoolean should not be falsy #10148

jzheaux opened this issue Jul 28, 2021 · 3 comments · Fixed by #10356
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Jul 28, 2021

Related to #10117 (comment)

ClaimAccessor#getClaimAsBoolean currently coerces any type into a Boolean, which is somewhat surprising when it is a map or list.

For example, the following snippet will somewhat surprisingly pass:

ClaimAccessor claims = () -> Collections.singletonMap("claim", new HashMap<>());
assertThat(claims.getClaimAsBoolean("claim")).isFalse();

In reality, the above is a usage error. It would be better for the application to complain so that the developer can adjust their system.

getClaimAsBoolean should match core Java behavior more closely. It should only coerce booleans and Strings (like "true" and "FALSE") into Booleans.

The logic to change is in ObjectToBooleanConverter where it does:

return Boolean.valueOf(source.toString());

This should instead do something like the following:

if (source instanceof String) {
    return Boolean.valueOf((String) source);
}

return null;

Then, getClaimAsBoolean should introduce an assertion, similar to the assertions in the other getClaimAsXXX methods:

Object claimValue = getClaims().get(claim);
Boolean convertedValue = ClaimConversionService.getSharedInstance().convert(claimValue, Boolean.class);
Assert.isNotNull(convertedValue, () -> "Unable to convert claim '" + claim + "' of type '" + claimValue.getClass() + "' to Boolean.");
return convertedValue;

Thereafter, if an application really needs the old behavior, it can register a custom converter like so:

ClaimConversionService.getInstance().addConverter(new MyObjectToFalsyBooleanConverter());

during application startup.

@jzheaux jzheaux added type: bug A general bug in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: first-timers-only An issue that can only be worked on by brand new contributors labels Jul 28, 2021
@ahmedmq
Copy link

ahmedmq commented Jul 29, 2021

@jzheaux - Could I work on this issue please?

@sjohnr
Copy link
Member

sjohnr commented Jul 30, 2021

It's yours, @ahmedmq!

ahmedmq added a commit to ahmedmq/spring-security that referenced this issue Sep 5, 2021
@sjohnr sjohnr modified the milestones: 5.6.0, 5.6.0-M3 Sep 9, 2021
@sjohnr sjohnr modified the milestones: 5.6.0-M3, 5.6.0-RC1 Sep 20, 2021
@qavid
Copy link
Contributor

qavid commented Oct 9, 2021

@jzheaux, @sjohnr I have created PR #10356. Please take look.

qavid added a commit to qavid/spring-security that referenced this issue Oct 14, 2021
sjohnr pushed a commit that referenced this issue Oct 14, 2021
@sjohnr sjohnr assigned qavid and unassigned ahmedmq Oct 14, 2021
@sjohnr sjohnr added the type: breaks-passivity A change that breaks passivity with the previous release label Oct 14, 2021
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: first-timers-only An issue that can only be worked on by brand new contributors type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug
Projects
None yet
4 participants