Skip to content

Conversation

akulyakhtin
Copy link

@akulyakhtin akulyakhtin commented Jul 3, 2025

Closes #11609

This request fixes the following issue:
if we have set authorities=true in app properties
and if we have a group value in cancer_study table sample_group
and if we have authority sample_group (or cbioportal:sample_group, depending on the filtering option) in authorities table for a user.

Then the access to that cancer study is not granted to the user even though the authroity matches the cancer study group.

This happens because authorities are convereted to upper case while cancer group names are not.

To fix this the pull request changes Collections.disjoint (which is case-sensitive) to a case-insensitive utility method caseInsensitiveDisjoint.

@inodb inodb requested a review from Copilot July 25, 2025 13:21
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a case-sensitivity issue in group-based cancer study access control where authorities are converted to uppercase but cancer study group names are not, preventing proper access validation. The fix implements a case-insensitive comparison method to ensure group matching works correctly regardless of case differences.

  • Replace case-sensitive Collections.disjoint with a case-insensitive utility method
  • Add caseInsensitiveDisjoint helper method that converts both collections to uppercase before comparison

return toReturn;
}

private static boolean caseInsensitiveDisjoint(Collection<String> c1, Collection<String> c2) {
Copy link

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

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

The method should include null safety checks for the input collections to prevent NullPointerException if either c1 or c2 is null.

Suggested change
private static boolean caseInsensitiveDisjoint(Collection<String> c1, Collection<String> c2) {
private static boolean caseInsensitiveDisjoint(Collection<String> c1, Collection<String> c2) {
if (c1 == null || c2 == null) {
return true; // If either collection is null, they are considered disjoint.
}

Copilot uses AI. Check for mistakes.

Comment on lines +397 to +398
Set<String> upperC1 = c1.stream().map(String::toUpperCase).collect(Collectors.toSet());
Set<String> upperC2 = c2.stream().map(String::toUpperCase).collect(Collectors.toSet());
Copy link

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

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

The stream operations should handle null elements within the collections to prevent NullPointerException when calling toUpperCase() on null strings.

Suggested change
Set<String> upperC1 = c1.stream().map(String::toUpperCase).collect(Collectors.toSet());
Set<String> upperC2 = c2.stream().map(String::toUpperCase).collect(Collectors.toSet());
Set<String> upperC1 = c1.stream()
.filter(Objects::nonNull)
.map(String::toUpperCase)
.collect(Collectors.toSet());
Set<String> upperC2 = c2.stream()
.filter(Objects::nonNull)
.map(String::toUpperCase)
.collect(Collectors.toSet());

Copilot uses AI. Check for mistakes.

Comment on lines +397 to +398
Set<String> upperC1 = c1.stream().map(String::toUpperCase).collect(Collectors.toSet());
Set<String> upperC2 = c2.stream().map(String::toUpperCase).collect(Collectors.toSet());
Copy link

Copilot AI Jul 25, 2025

Choose a reason for hiding this comment

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

The stream operations should handle null elements within the collections to prevent NullPointerException when calling toUpperCase() on null strings.

Suggested change
Set<String> upperC1 = c1.stream().map(String::toUpperCase).collect(Collectors.toSet());
Set<String> upperC2 = c2.stream().map(String::toUpperCase).collect(Collectors.toSet());
Set<String> upperC1 = c1.stream()
.filter(Objects::nonNull)
.map(String::toUpperCase)
.collect(Collectors.toSet());
Set<String> upperC2 = c2.stream()
.filter(Objects::nonNull)
.map(String::toUpperCase)
.collect(Collectors.toSet());

Copilot uses AI. Check for mistakes.

@inodb inodb changed the title Fix for issue #11609: group comparison should be case-insensitive Make group auth check case-insensitive Jul 25, 2025
@inodb inodb added the security label Jul 25, 2025
@inodb inodb requested a review from haynescd July 25, 2025 13:24
@KingAlex1985
Copy link
Contributor

Hi @akulyakhtin .
Take a look at Copilot's recommendations regarding null pointer checks in the pervious comment.
Perhaps you could include these changes as well.

Copy link
Collaborator

@haynescd haynescd left a comment

Choose a reason for hiding this comment

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

Yea, looks good. I would just address the suggested null checks.

Interesting how this is just now coming up... but the docs do say its not case sensitive.

@KingAlex1985
Copy link
Contributor

Hi @akulyakhtin
Take please a look at Copilot's recommendations regarding null pointer checks in the pervious comment.
Can you include these changes as well.
I guess after these changes this PR will be accepted and merged into master branch.

@KingAlex1985
Copy link
Contributor

@haynescd
Are you able to take over the Copilot's recommendations by yourself without waiting for @akulyakhtin.
Is there a function at github like a "one click take over Copilot's recommendations"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Authorization is broken when cancer study groups are not in uppercase

5 participants