From 64e9ac995abcc42d15952cdddc0111882ab8502e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1vid=20Kov=C3=A1=C4=8D?= Date: Sat, 9 Oct 2021 00:55:08 +0200 Subject: [PATCH] getClaimAsBoolean() should not be falsy Closes gh-10148 --- .../security/oauth2/core/ClaimAccessor.java | 18 ++++++++++--- .../converter/ObjectToBooleanConverter.java | 7 +++-- .../oauth2/core/ClaimAccessorTests.java | 27 +++++++++++++++++++ ...2TokenIntrospectionClaimAccessorTests.java | 5 ++-- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java index f709bb3a1a..8d79d68288 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java @@ -91,13 +91,23 @@ public interface ClaimAccessor { } /** - * Returns the claim value as a {@code Boolean} or {@code null} if it does not exist. + * Returns the claim value as a {@code Boolean} or {@code null} if the claim does not + * exist. * @param claim the name of the claim - * @return the claim value or {@code null} if it does not exist + * @return the claim value or {@code null} if the claim does not exist + * @throws IllegalArgumentException if the claim value cannot be converted to a + * {@code Boolean} + * @throws NullPointerException if the claim value is {@code null} */ default Boolean getClaimAsBoolean(String claim) { - return !hasClaim(claim) ? null - : ClaimConversionService.getSharedInstance().convert(getClaims().get(claim), Boolean.class); + if (!hasClaim(claim)) { + return null; + } + Object claimValue = getClaims().get(claim); + Boolean convertedValue = ClaimConversionService.getSharedInstance().convert(claimValue, Boolean.class); + Assert.notNull(convertedValue, + () -> "Unable to convert claim '" + claim + "' of type '" + claimValue.getClass() + "' to Boolean."); + return convertedValue; } /** diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToBooleanConverter.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToBooleanConverter.java index 8dd12c1e21..71d36ec86f 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToBooleanConverter.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToBooleanConverter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -41,7 +41,10 @@ final class ObjectToBooleanConverter implements GenericConverter { if (source instanceof Boolean) { return source; } - return Boolean.valueOf(source.toString()); + if (source instanceof String) { + return Boolean.valueOf((String) source); + } + return null; } } diff --git a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/ClaimAccessorTests.java b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/ClaimAccessorTests.java index 456a2a29d6..c211d4639d 100644 --- a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/ClaimAccessorTests.java +++ b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/ClaimAccessorTests.java @@ -105,6 +105,33 @@ public class ClaimAccessorTests { assertThat(this.claimAccessor.getClaimAsString(claimName)).isNull(); } + @Test + public void getClaimAsBooleanWhenBooleanTypeThenReturnBoolean() { + Boolean expectedClaimValue = Boolean.TRUE; + String claimName = "boolean"; + this.claims.put(claimName, expectedClaimValue); + assertThat(this.claimAccessor.getClaimAsBoolean(claimName)).isEqualTo(expectedClaimValue); + } + + @Test + public void getClaimAsBooleanWhenStringTypeThenReturnBoolean() { + Boolean expectedClaimValue = Boolean.TRUE; + String claimName = "boolean"; + this.claims.put(claimName, expectedClaimValue.toString()); + assertThat(this.claimAccessor.getClaimAsBoolean(claimName)).isEqualTo(expectedClaimValue); + } + + // gh-10148 + @Test + public void getClaimAsBooleanWhenNonBooleanTypeThenThrowIllegalArgumentException() { + String claimName = "boolean"; + Map claimValue = new HashMap<>(); + this.claims.put(claimName, claimValue); + assertThatIllegalArgumentException().isThrownBy(() -> this.claimAccessor.getClaimAsBoolean(claimName)) + .withMessage("Unable to convert claim '" + claimName + "' of type '" + claimValue.getClass() + + "' to Boolean."); + } + @Test public void getClaimAsMapWhenNotExistingThenReturnNull() { assertThat(this.claimAccessor.getClaimAsMap("map")).isNull(); diff --git a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/OAuth2TokenIntrospectionClaimAccessorTests.java b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/OAuth2TokenIntrospectionClaimAccessorTests.java index bd0ea50923..986e5ff89b 100644 --- a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/OAuth2TokenIntrospectionClaimAccessorTests.java +++ b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/OAuth2TokenIntrospectionClaimAccessorTests.java @@ -28,6 +28,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatNullPointerException; /** * Tests for {@link OAuth2TokenIntrospectionClaimAccessor}. @@ -51,9 +52,9 @@ public class OAuth2TokenIntrospectionClaimAccessorTests { } @Test - public void isActiveWhenActiveClaimValueIsNullThenReturnFalse() { + public void isActiveWhenActiveClaimValueIsNullThenThrowsNullPointerException() { this.claims.put(OAuth2TokenIntrospectionClaimNames.ACTIVE, null); - assertThat(this.claimAccessor.isActive()).isFalse(); + assertThatNullPointerException().isThrownBy(this.claimAccessor::isActive); } @Test