Admin user has no read ability on files from within download controller.

[randalldfloyd]

Expected behavior

Admin user should be able to view files attached to other users' private works, especially if they are able to view or enumerate the private file_sets they are attached to.

Actual behavior

Admin user can view other users' private works and related file_sets but not the actual files via the download controller. For example, an admin user can enumerate and select any member file in an admin set to be the representative thumbnail even though the admin user itself may not have ability to view it (see #790).

In the example below, a regular depositor has attached a file to a private work and the admin user can search, view, and edit the private work and its file_set. However, when rendering the thumbnail or attempting to download, it fails the ability check to get the file via the download controller and gets the default asset instead:

Started GET "/downloads/gq67jr16q?file=thumbnail" for 127.0.0.1 at 2017-06-07 10:57:48 -0400
Processing by Hyrax::DownloadsController#show as HTML
  Parameters: {"file"=>"thumbnail", "id"=>"gq67jr16q"}
  User Load (0.3ms)  SELECT  "users".* FROM "users" WHERE "users"."id" = ? ORDER BY "users"."id" ASC LIMIT ?  [["id", 3], ["LIMIT", 1]]
Usergroups are ["public", "admin", "registered"]
  Hyrax::Feature Load (0.1ms)  SELECT  "hyrax_features".* FROM "hyrax_features" WHERE "hyrax_features"."key" = ? ORDER BY "hyrax_features"."id" ASC LIMIT ?  [["key", "transfer_works"], ["LIMIT", 1]]
  Hyrax::Feature Load (0.1ms)  SELECT  "hyrax_features".* FROM "hyrax_features" WHERE "hyrax_features"."key" = ? ORDER BY "hyrax_features"."id" ASC LIMIT ?  [["key", "proxy_deposit"], ["LIMIT", 1]]
[CANCAN] Checking read permissions for user: admin@example.com with groups: ["public", "admin", "registered"]
[CANCAN] download_groups: []
[CANCAN] read_groups: []
[CANCAN] edit_groups: []
[CANCAN] read_groups: []
[CANCAN] download_users: []
[CANCAN] read_users: []
[CANCAN] edit_users: ["depositor@example.com"]
[CANCAN] read_users: ["depositor@example.com"]
Redirected to http://localhost:3000/assets/default-f936e9c3ea7a38e2c2092099586a71380b11258697b37fb4df376704495a849a.png
Filter chain halted as :authorize_download! rendered or redirected
Completed 302 Found in 10ms (ActiveRecord: 0.5ms)

Related work

Disparity in ability first described in #167 Affects #790

[randalldfloyd]

The admin user fails the ability check on files attached to file_sets here. Admin users have been given 'manage' ability on curation concerns models here, which explains why they can view private file_sets in the first place. But attached files are in basic LDP containers and are being checked for read access via URI and therefore not covered by any ability declarations for named models.

[randalldfloyd]

@mjgiarlo or @jcoyne I need an opinion on my solution to fix this (that is, the disparity of an admin being able to read a file_set but not its attached files.)

I was hoping to add ability declarations to an admin user and then check for them but files are in basic LDP containers so I couldn't figure out how to do that. The depositor has access and passes a basic read test via the persisted Hydra Access controls but I doubt we want to persist that for admins because we may not want this behavior later as related stories get hashed out. So the only way I could make it work is to just shortcut the authorize method for admins with a guard clause: https://github.com/samvera/hyrax/compare/1163_admin_file_ability#diff-510b39a3044a0d71ca5e901aa58bc72fR44

[jcoyne]

@randalldfloyd How about adding can :authorize_download! to the admin block of the Ability class?

[randalldfloyd]

@jcoyne Ah, I did not know you could declare ability for a named method. That would put it back in Ability where it belongs.

[jcoyne]

@randalldfloyd actually it should be for can :read, String https://github.com/samvera/hyrax/blob/0ef4d81d5e5af1b916decd422362087409cf88e4/app/controllers/hyrax/downloads_controller.rb#L44

I don't understand why that isn't already there.

[randalldfloyd]

@jcoyne So here's a different take on this problem. In the Hyrax downloads controller, the comment in the authorize_download! method says Hydra::Ability#download_permissions can't be used in this case because it assumes that files are in a LDP basic container, and thus, included in the asset's uri. But looking at the definition of download_permissions, I would think the behavior defined is exactly what we want to happen. It walks back and checks read ability for the parent, which would cover both the depositor and the admin because admin users have been given 'manage' ability on curation concerns models. That seems like the desired behavior to me because it prevents having to additionally declare that some other kind of user has ability on files, which is more of a hacky workaround. Anyway, can you help me decipher what that comment means and why we can't just use what's already defined?

[jcoyne]

PCDM files are not in basic containers (that's a relic of the fedora 3 model in fedora4). PCDM files are in a direct container of the FileSet.

[randalldfloyd]

@mjgiarlo @jcoyne I'm stumped on this now that I have a better understanding of what's actually happening. In this line in the downloads_controller, the thing being authorized is the parent file_set and not the file in the direct container as I had assumed. In that line it is authorizing by ID without loading the resource (from Fedora). That's the correct desired behavior, so why does the admin user get authorized by ID in file_set controller, which it clearly does, but not here in the downloads_controller? It should just work and I can't figure it out what's missing. I also can't get the can :authorize_download! suggestion offered earlier working either. I can still fix this with the current_ability.admin? workaround I mentioned.