Skip to content

getClaimAsBoolean() should not be falsy #10151

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

getClaimAsBoolean() should not be falsy #10151

wants to merge 1 commit into from

Conversation

ahmedmq
Copy link

@ahmedmq ahmedmq commented Aug 1, 2021

Closes #10148

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 1, 2021
@eleftherias eleftherias added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 10, 2021
sjohnr
sjohnr reviewed Aug 12, 2021
View reviewed changes
Copy link
Member

@sjohnr sjohnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the submission @ahmedmq! One minor comment is below, and you can squash your changes and force push to the same branch.

@ahmedmq
Copy link
Author

ahmedmq commented Aug 19, 2021

@sjohnr - Not sure what has gone wrong, it ran all tests successfully before pushing

@sjohnr
Copy link
Member

sjohnr commented Aug 19, 2021

@ahmedmq, you'll need to rebase on main and check OAuth2TokenIntrospectionClaimAccessorTests.

OAuth2TokenIntrospectionClaimAccessorTests > isActiveWhenActiveClaimValueIsNullThenReturnFalse() FAILED
    java.lang.NullPointerException at OAuth2TokenIntrospectionClaimAccessorTests.java:56

Quite a bit has changed related to gh-9647 and gh-10135.

@sjohnr
Copy link
Member

sjohnr commented Sep 3, 2021

@ahmedmq, are you able to rebase on main? Let me know if you have trouble with this step.

getClaimAsBoolean() should not be falsy
af8c72f 
Closes spring-projectsgh-10148

Remove comment as per PR review
@ahmedmq
Copy link
Author

ahmedmq commented Sep 5, 2021

@sjohnr - I am not sure of the expected behaviour when the claimValue is null. For e.g in the test case isActiveWhenActiveClaimValueIsNullThenReturnFalse() under OAuth2TokenIntrospectionClaimAccessorTests we have

this.claims.put(OAuth2TokenIntrospectionClaimNames.ACTIVE, null);

Since in the above the claimValue is null, what should the boolean converter return? Should it return false by default

sjohnr
sjohnr suggested changes Sep 9, 2021
View reviewed changes
Copy link
Member

@sjohnr sjohnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmedmq, thanks for updating the PR. The implementation of the converter looks correct with your changes applied based on #10148. Meaning, it would now return an IllegalArgumentException, so I would expect the test you mentioned to need to change. This change will break passivity, and so will go in the 5.6 release.

Also, please see below feedback inline.

...2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/ClaimAccessorTests.java
@@ -135,4 +136,13 @@ public void getClaimWhenValueIsNotConvertedThenThrowClassCastException() {
assertThatObject(this.claimAccessor.getClaim(claimName)).isNotInstanceOf(Boolean.class);
}

// gh-10148
@Test
public void getClaimAsBooleanThrowsIllegalArgumentForNonBooleanType() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to include When for the condition set up by the test, e.g. getClaimAsBooleanWhenNonBooleanTypeThenThrowsIllegalArgumentException.

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java
}
Object claimValue = getClaims().get(claim);
Boolean convertedValue = ClaimConversionService.getSharedInstance().convert(claimValue, Boolean.class);
Assert.isTrue(convertedValue != null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use Assert.isNotNull here? Looks like other checks in this class don't do that, however, which is fine for now.

@sjohnr sjohnr added the type: breaks-passivity A change that breaks passivity with the previous release label Sep 9, 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
@sjohnr
Copy link
Member

sjohnr commented Oct 8, 2021

@ahmedmq did you have a chance to review the above feedback?

@qavid
Copy link
Contributor

qavid commented Oct 8, 2021 •
edited
Loading

Hi @sjohnr, I can work on this PR? Actually, I was just about to finish it.

@sjohnr
Copy link
Member

sjohnr commented Oct 14, 2021

Closing in favor of #10356.

@sjohnr sjohnr closed this Oct 14, 2021
@sjohnr sjohnr removed this from the 5.6.0-RC1 milestone Oct 14, 2021
@sjohnr sjohnr added status: declined A suggestion or change that we don't feel we should currently apply and removed status: duplicate A duplicate of another issue type: bug A general bug type: breaks-passivity A change that breaks passivity with the previous release labels Oct 14, 2021
@sjohnr sjohnr added the type: bug A general bug label Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjohnr sjohnr sjohnr requested changes

Assignees

@sjohnr sjohnr

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

getClaimAsBoolean should not be falsy
5 participants
@ahmedmq @sjohnr @qavid @eleftherias @spring-projects-issues