Skip to content
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

Add bean for CancerStudyPermissionEvaluator #10825

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Add bean for CancerStudyPermissionEvaluator #10825

merged 1 commit into from
Jun 24, 2024

Conversation

Floris-Hyve
Copy link
Contributor

Add a bean for CancerStudyPermissionEvaluator instead of instantiating directly within createExpressionHandler

@TJMKuijpers TJMKuijpers added the bug label Jun 7, 2024
dippindots
dippindots approved these changes Jun 17, 2024
View reviewed changes
Copy link
Member

@dippindots dippindots left a comment

Choose a reason for hiding this comment

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

LGTM

haynescd
haynescd approved these changes Jun 19, 2024
View reviewed changes
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.

LGTM

inodb
inodb reviewed Jun 19, 2024
View reviewed changes
Copy link
Member

@inodb inodb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@Floris-Hyve Thanks for trying to solve this! It's unclear why changing how the bean is instantiated would solve an authorization issue. I don't see any issue with merging this but would be good to first update title+description to explain the reasoning

inodb
inodb requested changes Jun 19, 2024
View reviewed changes
Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

see comment re title/desciption

@Floris-Hyve
Copy link
Contributor Author

Floris-Hyve commented Jun 20, 2024 •
edited
Loading

I think there are issues with spring recognizing the CancerStudyPermissionEvaluator when it is not instantiated as a bean, which causes the expressionHandler to be set incorrectly. I think its related to how dependency injection works in spring. See below screenshots:

  • Without instantiating as bean, study is accessible to a user which shouldnt have permission (can explore it too)
    image

  • As bean, study not accessible
    image

@Floris-Hyve Floris-Hyve changed the title Fix for unauthorized study access Add bean for CancerStudyPermissionEvaluator Jun 20, 2024
@Floris-Hyve Floris-Hyve requested a review from inodb June 20, 2024 13:53
@inodb inodb added cleanup and removed bug labels Jun 24, 2024
@inodb inodb merged commit adf38ab into cBioPortal:master Jun 24, 2024
12 of 15 checks passed
@inodb inodb added bug and removed cleanup labels Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haynescd haynescd haynescd approved these changes

@dippindots dippindots dippindots approved these changes

@inodb inodb Awaiting requested review from inodb

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Floris-Hyve @inodb @dippindots @haynescd @TJMKuijpers