can?(:download, file_set) always returns false (display_media_download_link? in Hyrax::FileSetHelper)

[cudevmaxwell]

hyrax/app/helpers/hyrax/file_set_helper.rb

Line 12 in 97f4560

can?(:download, file_set) &&

This call to can?(:download, file_set) always returned 'false' in our testing, which was preventing the download link from appearing below thumbnails on work pages.

I believe the call should be rewritten to can?(:download, file_set.id), like in app/views/hyrax/file_sets/_actions.html.erb.

[elrayle]

@cudevmaxwell Thanks for writing up the issue and including the place in the code that seems likely to be part of the problem. I wonder if you can add a bit more information. Specifically, it would be helpful to have

• the steps you follow that produce the problem
• a screenshot of the page where you don't see the download link
• the value of Hyrax.config.display_media_download_link

It seems like that config should be related to this problem, but some exploration would be required to do some digging to be sure.

[cudevmaxwell]

In a vanilla Hyrax 3.1.0 based app, the download link below the thumbnail in a work display isn't displayed for public users. More information here; https://groups.google.com/g/samvera-tech/c/TBwHy7XbuKE

If I add some logging to display_media_download_link?:

def display_media_download_link?(file_set:)
    Rails.logger.debug "Issue#5241 - Hyrax.config.display_media_download_link?: %s" % Hyrax.config.display_media_download_link?
    Rails.logger.debug "Issue#5241 - can?(:download, file_set): %s" % can?(:download, file_set)
    Rails.logger.debug "Issue#5241 - !workflow_restriction?(file_set.try(:parent)): %s" % !workflow_restriction?(file_set.try(:parent))
    Hyrax.config.display_media_download_link? &&
      can?(:download, file_set) &&
      !workflow_restriction?(file_set.try(:parent))
end

On the left, a logged in user. On the right, a public user, not logged in. The representative media is a public PDF on a public work. display_media_download_link?(file_set:) returns false for the public user, when it should return true, because the call to can?(:download, file_set) returns false.

The thumbnail and link are generated by app/views/hyrax/file_sets/media_display/_pdf.html.erb:

<% if display_media_download_link?(file_set: file_set) %>
  <div>
      <h2 class="sr-only"><%= t('hyrax.file_set.show.downloadable_content.heading') %></h2>
      <%= image_tag thumbnail_url(file_set),
                    class: "representative-media",
                    alt: "",
                    role: "presentation" %>
      <%= link_to t('hyrax.file_set.show.downloadable_content.pdf_link'),
                  hyrax.download_path(file_set),
                  target: :_blank,
                  id: "file_download",
                  data: { label: file_set.id } %>
    </div>
<% else %>
    <div>
      <%= image_tag thumbnail_url(file_set),
                    class: "representative-media",
                    alt: "",
                    role: "presentation" %>
    </div>
<% end %>

The link isn't added if display_media_download_link?(file_set: file_set) returns false.

Also, in production.log, I can see that cancan isn't logging anything when file_set is used. But when file_set.id is used, cancan is logging the check:

Issue#5241 - Hyrax.config.display_media_download_link?: true
Issue#5241 - can?(:download, file_set): false

vs

Issue#5241 - Hyrax.config.display_media_download_link?: true
[CANCAN] Checking download permissions for user:  with groups: ["public"]
[CANCAN] download_groups: ["public", "admin", "library_staff"]
Issue#5241 - can?(:download, file_set.id): true

[cudevmaxwell]

I've submitted a PR for this issue, which uses can?(:download, file_set.id), like in app/views/hyrax/file_sets/_actions.html.erb. I'm not a RoR dev though, so I'm not sure if that's the best thing to do.

The ability is defined in app/models/concerns/hyrax/ability.rb.

 def download_permissions
  can :download, String do |id|
    test_download(id)
  end

  can :download, SolrDocument do |obj|
    cache.put(obj.id, obj)
    test_download(obj.id)
  end
end

Maybe the file_set isn't a SolrDocument, so you have to use file_set.id to use the String match?

[cudevmaxwell]

Closed PR due to failing tests. I'll wait to see if others think this is a legitimate bug, and if my fix is appropriate. If so, I'll rewrite the tests and submit a new PR.

[no-reply]

thanks again for this @cudevmaxwell.

it does seem to me all of can? :download, String, can :download, SolrDocument, AND can :download, FileSet should be equivalent, when these classes are standing in for a given file set. that they aren't in your case definitely points to some problem, but i think it might be deeper rooted than the id substitution suggests.

i'm noting that a can :download, FileSet statement IS missing from Hyrax, and that seems wrong to me. i wonder if changing Ability#download_permissions to the following fixes your issue?

 def download_permissions
  can :download, String do |id|
    test_download(id)
  end

  can :download, [SolrDocument, FileSet] do |obj|
    cache.put(obj.id, obj)
    test_download(obj.id)
  end
end

if so, i'd think that would be a good fix.

even in that case though, i'm still left wondering why this issue is emerging for you now, but not before this, and i think some further digging is called for if you have the capacity.

could you possibly add a stacktrace in a comment here (i'm curious what the call chain leading us here looks like)?

additionally, it might be helpful to pass along some further debugging info; something like:

Rails.logger.debug "Issue#5241 - file_set class: #{file_set.class}"
Rails.logger.debug "Issue#5241 - file_set id: #{file_set.id}"

in the meanwhile, i can dig in on my own app and on nurax and see if i spot anything that might have changed in recent Hyrax versions that might be responsible for this.

[no-reply]

ah, so i took the liberty of looking at (what i think is) your hyrax app, and i'm noticing that the caller is your own custom view.

i don't see exactly where things are going differently on your app's side (how is your file_set ending up being a FileSet instead of a SolrDocument?). but whatever the case, what your app is doing is right, and

hyrax/spec/helpers/hyrax/file_set_helper_spec.rb

Lines 17 to 21 in 3de688a

	it 'allows download when permissions allow it ' do
	allow(ability).to receive(:can?).with(:download, file_set).and_return(true)
	expect(helper.display_media_download_link?(file_set: file_set)).to eq true
	end

tries to test it explicitly but stubs the actual ability check. it seems clear to me that it's Hyrax::Ability that's got it wrong.

would you like to work together on a fix?

[cudevmaxwell]

@no-reply Thanks for looking into this!

• Nurax-stable is now using 3.1.0 (was that you 😃?) and the download link below the thumbnail is missing from this public work with a public representative media file: https://nurax-stable.curationexperts.com/concern/generic_works/2801pg52v?locale=en
• Good idea on checking the class of the file_set object: it's a Hyrax::FileSetPresenter. That makes sense, since (in this case, PDF files) the call to display_media_download_link is happening in app/views/hyrax/file_sets/media_display/_pdf.html.erb.
• can :download, [SolrDocument, FileSet] doesn't fix the problem, since the class doesn't match.
• Logging the output of caller just showed that vendor/bundle/ruby/2.7.0/gems/actionview-5.2.6/lib/action_view/template.rb was calling app/views/hyrax/file_sets/media_display/_pdf.html.erb.
• The call stack is, I think:
  • app/views/hyrax/base/show.html.erb

  <%= render 'representative_media', presenter: @presenter, viewer: false unless @presenter.iiif_viewer? %>

  • app/views/hyrax/base/_representative_media.html.erb

  <%= render media_display_partial(presenter.representative_presenter), file_set: presenter.representative_presenter  %>

  • media_display_partial(file_set) in app/helpers/hyrax/file_set_helper.rb returns hyrax/file_sets/media_display/pdf in this case. So app/views/hyrax/file_sets/media_display/_pdf.html.erb is used. The file_set is presenter.representative_presenter.
  • representative_presenter in main/app/presenters/hyrax/work_show_presenter.rb returns a Hyrax::FileSetPresenter.

[cudevmaxwell]

Where I'm getting stuck is: how should can? be used when we only have the presenter, not the model? I just cribbed from app/views/hyrax/file_sets/_actions.html.erb and used the id:

<% if can?(:download, file_set.id) && !(can?(:edit, file_set.id) || can?(:destroy, file_set.id)) %>

but in app/views/hyrax/dashboard/collections/_show_actions.html.erb,

if can? :edit, presenter.solr_document

is used.

[cudevmaxwell]

When can?(:download, file_set.id) in app/views/hyrax/file_sets/_actions.html.erb is called, I'm pretty sure the file_set object is a Hyrax::FileSetPresenter, So using file_set.id in file_set_helpers's display_media_download_link? would be consistent with that. But maybe we want to do something differently in a helper function vs a template.

[no-reply]

Nurax-stable is now using 3.1.0 (was that you smiley?)

wasn't me! it's supposed to redeploy from the latest hyrax main at least daily. if it wasn't that explains a lot.

When can?(:download, file_set.id) in app/views/hyrax/file_sets/_actions.html.erb is called, I'm pretty sure the file_set object is a Hyrax::FileSetPresenter, So using file_set.id in file_set_helpers's display_media_download_link? would be consistent with that. But maybe we want to do something differently in a helper function vs a template.

it still seems to me like the display_media_download_link? helper should just handle this case. i don't think callers should need to change their behavior.

[no-reply]

it does seem like this should already be handling this case: https://github.com/samvera/hyrax/blob/main/app/models/concerns/hyrax/ability.rb#L420-L421

i'm looking into why it's not.

[no-reply]

confirming that this is a bug introduced in 3.1.0.

working on a fix now.

[no-reply]

@cudevmaxwell i wonder if this one will take care of it for you? #5250

[cudevmaxwell]

@no-reply #5250 seems to have fixed it, great work!

[elrayle]

FYI... nurax-stable deploys the latest stable release (currently 3.1.0). nurax-dev is supposed to deploy main and it auto deploys twice a day. It wasn't doing that and was recently updated to start doing that again.

[jlhardes]

@MPLSFedResearchTZ tested on nurax-dev and verified that the download link is available and working for PDF files.