[ELY-2797] check for null Boolean and return boolean #2187

rsearls · 2024-08-26T15:19:19Z

https://issues.redhat.com/browse/ELY-2797 NullPointerException in
OidcClientConfiguration.resolveUrls if parameter "request_parameter_supported"
is not present in openid-configuration

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

fjuma · 2024-08-26T16:30:22Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+ * Test OIDC json config class properly returns boolean values.
+ */
+public class OidcProviderMetadataTest {
+    private OidcProviderMetadata oidcSecRealm;


Just minor, but since this represents the provider metadata, I think oidcProviderMetadata would be better than oidcSecRealm.

fjuma · 2024-08-26T16:40:00Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+
+    @Test
+    public void testClaimsParameterSupported() throws Exception {
+        assertFalse(oidcSecRealm.getClaimsParameterSupported());


The OIDC provider metadata gets created by sending a request to the OIDC provider's /.well-known endpoint and then using JsonSerialization#readValue (as seen here https://github.com/wildfly-security/wildfly-elytron/blob/2.x/http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfiguration.java#L273) to parse what we get back from the well-known endpoint into the OidcProviderMetadata object.

I'm wondering if we can update these tests to make use of JsonSerialization#readValue to more closely mimic what happens in the code.

To do that, I'm wondering if we can call JsonSerialization#readValue directly with a string value for the metadata and that way we can control what options are specified/not specified in our string.

To get a sample string value to work with, you could try setting up a Keycloak OpenID provider locally (following the steps from one of the OIDC blog posts), try to hit the well-known endpoint, e.g., http://localhost:<KEYCLOAK_PORT>/realms/myrealm/.well-known/openid-configuration to get sample OIDC provider metadata.

We can then create hardcoded provider metadata strings to be used for these tests to test the case where these boolean options are present and not present.

Does that make sense?

fjuma · 2024-08-27T13:19:57Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

@@ -0,0 +1,330 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2020 Red Hat, Inc., and individual contributors


fjuma · 2024-08-27T13:20:50Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcProviderMetadata.java

@@ -330,7 +330,7 @@ public void setClaimTypesSupported(List<String> claimTypesSupported) {
    }

    public Boolean getClaimsParameterSupported() {


Since OidcProviderMetadata is currently private API, let's go ahead and change the return values from Boolean to boolean as well.

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

fjuma · 2024-08-27T18:04:15Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+    @Test
+    public void testIssuer() throws Exception {
+        assertTrue("http://localhost:8080/realms/myrealm".equals(oidcProviderMetadata.getIssuer()));
+        assertNull(emptyOidcProviderMetadata.getIssuer());


Would it make sense to add an assertion here for withoutBooleansProviderMetaData? That way, we are testing the expected values for all three cases (control data, empty, control data modified to leave out some options) in each test case.

fjuma · 2024-08-27T18:10:45Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+            ",\"id_token_encryption_alg_values_supported\":[\"RSA-OAEP\",\"RSA-OAEP-256\",\"RSA1_5\"]\n" +
+            ",\"id_token_encryption_enc_values_supported\":[\"A256GCM\"]\n" +
+            ",\"userinfo_signing_alg_values_supported\":[\"PS384\",\"none\"]\n" +
+            ",\"request_object_signing_alg_values_supported\":[\"PS384\",\"none\"]\n" +


Let's also remove the other options that were introduced in ELY-2584 since it's also optional for the provider to include these in the metadata:

request_object_signing_alg_values_supported

request_object_encryption_enc_values_supported

request_object_encryption_alg_values_supported

pushed_authorization_request_endpoint

fjuma · 2024-08-27T18:51:57Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+    public void testRequestObjectEncryptionAlgValuesSupported() throws Exception {
+        List<String> l = oidcProviderMetadata.getRequestObjectEncryptionAlgValuesSupported();
+        assertTrue(l.contains("RSA1_5"));
+        assertNull(emptyOidcProviderMetadata.getRequestObjectEncryptionAlgValuesSupported());


I think this needs an assertNull for withoutBooleansProviderMetaData.

fjuma · 2024-08-27T18:52:06Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+    public void testRequestObjectEncryptionEncValuesSupported() throws Exception {
+        List<String> l = oidcProviderMetadata.getRequestObjectEncryptionEncValuesSupported();
+        assertTrue(l.contains("A192GCM"));
+        assertNull(emptyOidcProviderMetadata.getRequestObjectEncryptionEncValuesSupported());


I think this needs an assertNull for withoutBooleansProviderMetaData.

fjuma · 2024-08-27T18:52:57Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+    @Test
+    public void testPushedAuthorizationRequestEndpoint() throws Exception {
+        assertTrue("http://localhost:8080/realms/myrealm/protocol/openid-connect/ext/par/request".equals(oidcProviderMetadata.getPushedAuthorizationRequestEndpoint()));
+        assertNull(emptyOidcProviderMetadata.getPushedAuthorizationRequestEndpoint());


I think this needs an assertNull for withoutBooleansProviderMetaData.

fjuma · 2024-08-27T18:55:25Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+    public void testRequestObjectSigningAlgValuesSupported() throws Exception {
+        List<String> l = oidcProviderMetadata.getRequestObjectSigningAlgValuesSupported();
+        assertTrue(l.contains("RS384"));
+        assertNull(emptyOidcProviderMetadata.getRequestObjectSigningAlgValuesSupported());


I think this needs an assertNull for withoutBooleansProviderMetaData.

fjuma · 2024-08-27T18:57:48Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java

+public class OidcProviderMetadataTest {
+    private static OidcProviderMetadata oidcProviderMetadata;
+    private static OidcProviderMetadata emptyOidcProviderMetadata;
+    private static OidcProviderMetadata withoutBooleansOidcProviderMetadata;


Maybe just to make it more clear, we could rename withoutBooleansOidcProviderMetatdata withoutOptionalsOidcProviderMetadata (since some of the values that are optional aren't booleans).

fjuma

Thanks very much @rsearls!

rsearls requested review from fjuma and Skyllarr as code owners August 26, 2024 15:19

fjuma reviewed Aug 26, 2024

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java Show resolved Hide resolved

fjuma reviewed Aug 26, 2024

View reviewed changes

rsearls force-pushed the ELY-2797-NullPointerException-request_parameter_supported branch from 3c56fcc to fbbdf77 Compare August 26, 2024 21:33

fjuma reviewed Aug 27, 2024

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcProviderMetadataTest.java Show resolved Hide resolved

rsearls force-pushed the ELY-2797-NullPointerException-request_parameter_supported branch from fbbdf77 to 8362c43 Compare August 27, 2024 17:56

fjuma reviewed Aug 27, 2024

View reviewed changes

rsearls force-pushed the ELY-2797-NullPointerException-request_parameter_supported branch from 8362c43 to b4f6712 Compare August 27, 2024 18:47

fjuma reviewed Aug 27, 2024

View reviewed changes

[ELY-2797] check for null Boolean and return boolean

882fae7

rsearls force-pushed the ELY-2797-NullPointerException-request_parameter_supported branch from b4f6712 to 882fae7 Compare August 27, 2024 19:34

fjuma approved these changes Aug 27, 2024

View reviewed changes

fjuma requested a review from darranl August 27, 2024 19:45

ivassile approved these changes Aug 28, 2024

View reviewed changes

fjuma merged commit d78b28d into wildfly-security:2.x Aug 28, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ELY-2797] check for null Boolean and return boolean #2187

[ELY-2797] check for null Boolean and return boolean #2187

rsearls commented Aug 26, 2024

fjuma Aug 26, 2024

rsearls Aug 26, 2024

fjuma Aug 26, 2024

rsearls Aug 26, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma Aug 27, 2024 •

edited

Loading

rsearls Aug 27, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma Aug 27, 2024

rsearls Aug 27, 2024

fjuma left a comment

		@@ -330,7 +330,7 @@ public void setClaimTypesSupported(List<String> claimTypesSupported) {
		}

		public Boolean getClaimsParameterSupported() {

[ELY-2797] check for null Boolean and return boolean #2187

[ELY-2797] check for null Boolean and return boolean #2187

Conversation

rsearls commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma left a comment

Choose a reason for hiding this comment

fjuma Aug 27, 2024 •

edited

Loading