-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix download permissions #2517
Fix download permissions #2517
Conversation
… staff to download all files
0fa7d7e
to
2478d7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say something about why we have to look to parents, where we weren't doing that before?
spec/models/ability_spec.rb
Outdated
@@ -227,6 +228,7 @@ | |||
is_expected.to be_able_to(:manifest, flagged_scanned_resource) | |||
is_expected.to be_able_to(:color_pdf, color_enabled_resource) | |||
is_expected.to be_able_to(:read, :graphql) | |||
is_expected.to be_able_to(:download, other_staff_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use a different resource name here? the reference to staff file implies that maybe it's not public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This resource is used several places in the tests, and the name is to contrast with staff_file
where we test staff interacting with their own files vs. with files created by another staff member.
I'd be happy to use admin_file
instead, or create a new file called open_file
if that seems like a clearer illustration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think open_file
would be clearer in this context!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 i've pushed another commit that switches to open_file
We're looking at parents because the requirement changed from "never let users download master files" to "let users download master files of public objects" — so we need to look at the state/visibility of the parent. I thought it made the most sense to just use |
got it. thanks! |
Fixes #1568; Fixes #2458