Extract groups from OAuth JWT token

[pnartowski]

Extract array of groups from string

Fix #11429

Describe changes proposed in this pull request:

• Register Java Time Module in Object Mapper to prevent exception on serializing JWT claims ( ID token contains few dates )
• for group properties if string instead of array is found deserialize this string to extract array from it

Checks

• The commit log is comprehensible. It follows 7 rules of great commit messages. We can fix this during merge by using a squash+merge if necessary
• Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
• Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!
• Make sure your PR has one of the labels defined in https://github.com/cBioPortal/cbioportal/blob/master/.github/release-drafter.yml

[pnartowski]

@haynescd may I ask you to help progress with this PR ?

[pnartowski]

@inodb could you help progress with this PR ?

[Gluks54]

@inodb please could you help progress with this PR ?

[kozaka4]

@inodb Is there any chance we can proceed with this PR?

[kozaka4]

Hello cbio team,
Could someone help us with this PR? It's blocked because ci/circleci: end_to_end_tests_localdb is failed?

[KingAlex1985]

@pnartowski and @kozaka4

I have created a Unit-Test regarding your change / check "rolesCursor.isTextual()" at class "ClaimRoleExtractorUtil.java":
Maybe you can check this. If this is correct so far you can maybe als add it. So that others can better understand your change / new check.

package org.cbioportal.application.security.util;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ObjectNode;
import org.junit.jupiter.api.Test;
import org.junit.runner.RunWith;
import org.mockito.junit.MockitoJUnitRunner;
import static org.junit.jupiter.api.Assertions.*;

@RunWith(MockitoJUnitRunner.class)
class ClaimRoleExtractorUtilTest {
    
    @Test
    void extractClientRolesTest01() {
        String claims = """
        {
            "roles": ["role1", "role2"]
        }
        """;
        String jwtRolesPath = "roles";
        var result = ClaimRoleExtractorUtil.extractClientRoles(claims, jwtRolesPath);
        assertNotNull(result);
        assertEquals(2, result.size());
        assertTrue(result.contains("role1"));
        assertTrue(result.contains("role2"));
    }
    
    @Test
    void extractClientRolesTest01b() {
        ObjectMapper objectMapper = new ObjectMapper();
        String roleString = """
            ["role1","role2","role3"]
        """;
        ObjectNode root = objectMapper.createObjectNode();
        root.put("roles", roleString);
        String jwtRolesPath = "roles";
        
        var result = ClaimRoleExtractorUtil.extractClientRoles(root, jwtRolesPath);
        
        assertNotNull(result);
        assertEquals(3, result.size());
        assertTrue(result.contains("role1"));
        assertTrue(result.contains("role2"));
        assertTrue(result.contains("role3"));
    }

    @Test
    void extractClientRolesTest02() {
        String claims = """
        {
            "groups": ["GROUP_A", "GROUP_B", "GROUP_C"]
        }
        """;
        String jwtRolesPath = "groups";
        var result = ClaimRoleExtractorUtil.extractClientRoles(claims, jwtRolesPath);
        assertNotNull(result);
        assertEquals(3, result.size());
        assertTrue(result.contains("GROUP_A"));
        assertTrue(result.contains("GROUP_B"));
        assertTrue(result.contains("GROUP_C"));
    }
    
    @Test
    void extractClientRolesTest02b() {
        ObjectMapper objectMapper = new ObjectMapper();
        String groupString = """
            ["GROUP_A","GROUP_B","GROUP_C"]
        """;
        ObjectNode root = objectMapper.createObjectNode();
        root.put("groups", groupString);
        String jwtRolesPath = "groups";

        var result = ClaimRoleExtractorUtil.extractClientRoles(root, jwtRolesPath);

        assertNotNull(result);
        assertEquals(3, result.size());
        assertTrue(result.contains("GROUP_A"));
        assertTrue(result.contains("GROUP_B"));
        assertTrue(result.contains("GROUP_C"));
    }

    @Test
    void extractClientRolesTest03() {
        String claims = """
            {
                "iss": "https://auth.example.com/",
                "sub": "user123",
                "aud": "my-app-client-id",
                "exp": 1712570400,
                "iat": 1712566800,
                "roles": ["admin", "editor"]
            }
            """;

        JsonNode claimsNode;
        
        try {
            claimsNode = new ObjectMapper().readTree(claims);
        } catch (JsonProcessingException e) {
            throw new RuntimeException(e);
        }

        if (claimsNode instanceof ObjectNode objectNode) {
            String groupString = """
                ["GROUP_A","GROUP_B","GROUP_C"]
            """;
            objectNode.put("groups", groupString);
        } else {
            throw new IllegalStateException("claimsNode is not an ObjectNode");
        }
        
        String jwtRolesPath = "groups";
        var result = ClaimRoleExtractorUtil.extractClientRoles(claimsNode, jwtRolesPath);
        assertNotNull(result);
        assertEquals(3, result.size());
        assertTrue(result.contains("GROUP_A"));
        assertTrue(result.contains("GROUP_B"));
        assertTrue(result.contains("GROUP_C"));
    }
}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[kozaka4]

I’d like to kindly ask for one more review and if everything looks good a merge. Thanks in advance!
@zainasir @dippindots @alisman 🙏

[KingAlex1985]

I will also mention here @inodb
See the previous comment from @kozaka4

[Copilot]

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/main/java/org/cbioportal/application/security/util/ClaimRoleExtractorUtil.java:22

• Instead of using a new ObjectMapper in the subsequent readTree call, reuse the configured 'objectMapper' to ensure that the JavaTimeModule remains registered and to avoid potential inconsistencies.

String jsonString = objectMapper.writeValueAsString(claims);

src/main/java/org/cbioportal/application/security/util/ClaimRoleExtractorUtil.java:56

• Consider reusing the previously configured ObjectMapper for parsing the textual JSON instead of creating a new instance, ensuring consistent module registration and behavior.

if (rolesCursor.isTextual()) { rolesCursor = new ObjectMapper().readTree(rolesCursor.asText()); }

[pnartowski]

We might consider moving objectMapper to class field. As methods which use it are static the field needs to be static to.
Module registration is such case should be done in static block.
It isn't brilliant solution but this way we will avoid recreation each time of ObjectMapper.

private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();

static { OBJECT_MAPPER.registerModule(new JavaTimeModule()); }

What do you think ?

[kozaka4]

@sheridancbio and @hweej we need your help, pls

[kozaka4]

@haynescd merge:)?