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.

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

Restrict resources returned by user permission #10172

Conversation

RobOatesHistoricEngland
Copy link
Contributor

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Apply permission checks for Related Resources, otherwise it was possible to see resources for which the user should not.

Updated the second reference of resource.to_json() in api.py's ResourceReport.get() method to include the user and perm parameters.

Updated resource.py's Resource.get_related_resources() method to remove related resources for which the user can't read the resource for both the related from and related to resource instance.

Issues Solved

#10131

Checklist

  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Ticket Background

Further comments

AB# 59588

@aj-he aj-he requested a review from chiatt October 23, 2023 07:19
@aj-he
Copy link
Contributor

aj-he commented Oct 23, 2023

@chiatt please reassign as required.

@RobOatesHistoricEngland RobOatesHistoricEngland marked this pull request as ready for review October 23, 2023 08:24
@chiatt chiatt assigned chrabyrd and unassigned chiatt Oct 24, 2023
@chiatt chiatt requested a review from chrabyrd October 24, 2023 12:13
Copy link
Contributor

@chrabyrd chrabyrd left a comment

Choose a reason for hiding this comment

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

👋 @RobOatesHistoricEngland

Overall this looks good but there's a single issue preventing me from merging this in.

arches/app/models/resource.py Show resolved Hide resolved
@chrabyrd
Copy link
Contributor

chrabyrd commented Oct 24, 2023

👋 @RobOatesHistoricEngland

So this PR kicked off a small internal discussion on how to solve the problem of adding a permissions check to related_resources while maintaining performance and pagination logic. We ended up going a bit of a different direction than what you suggested, and you can see the changes in #10191 .

Since we decided to go a different direction, I'm going to close this PR for now. But I wanted to let you know how appreciated this contribution is and that it has had a measurable impact on the codebase. If you have the bandwidth please review #10191 and let me know if there's any changes that need to be made. Thanks again!

@chrabyrd chrabyrd closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants