Skip to content
This repository has been archived by the owner on Mar 24, 2020. It is now read-only.

Remove download link for simple ucsd-only object #471

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

VivianChu
Copy link
Member

Fixes #461

Local Checklist

  • Tests written and passing locally?
  • Code style checked?
  • QA-ed locally?
  • Rebased with master branch?
  • Configuration updated (if needed)?
  • Documentation updated (if needed)?

What does this PR do?

Remove download function for UCSD-only video objects

Why are we doing this? Any context of related work?

References #461

@ucsdlib/developers - please review

<% end %>
<% if can? :update, @document then %>
<%= link_to adl_glyph, downloadFilePath, :rel => 'nofollow', class:adl_class, title:adl_title %>
<% elsif local_otherRights == 'false' && local_license == 'false' && !downloadDerivativePath.nil?%>
Copy link
Member

Choose a reason for hiding this comment

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

Could we potentially move this code in a helper method, something like can_download? (which takes in a solr document) or something? This would include the code above that checks the various Solr properties to set the values of local_otherRights and local_license

My thinking behind this is because while this code will do a good job preventing links from showing to users who shouldn't see them, I don't think this actually prevents those users from downloading the files directly through the file dowload controller.

So if this line in the file controller could be changed to something like:

authorize!(:show, @solr_doc) && can_download?(@solr_doc)

Then we could enforce the logic in both places with the same code. Or something like that.

Curious what you think.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mcritchlow - Sounds good. I'll move the code in a helper method and update the file controller. I think the issue of downloading the files directly is also mentioned in this issue #467 and Longshou is working on it.

Copy link
Member

Choose a reason for hiding this comment

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

@VivianChu - sounds great. Yeah I think this would hopefully take of both tickets in one PR potentially.

# @return Boolean value
#---

def can_download?(document)

Choose a reason for hiding this comment

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

Metrics/CyclomaticComplexity: Cyclomatic complexity for can_download? is too high. [8/7]

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 65.922% when pulling ff1128e on feature/remove-download into 3fd6a7f on develop.

@VivianChu
Copy link
Member Author

@mcritchlow - The code has been moved to the helper method. However, if I update the file_controller to use authorize! :show, @solr_doc && can_download?(@solr_doc), then one test fails in feature/file_spec - https://github.com/ucsdlib/damspas/blob/feature/remove-download/spec/features/file_spec.rb#L69 I think maybe because of different return type - authorize! :show, @solr_doc returns a solr document while can_download? function returns a boolean. When I use authorize! :show, @solr_doc if can_download?(@solr_doc), then the test passes. I try to test the file download for video object, & I can always be able to download it with no login or login as anonymous with all options in file_controller like with && can_download?(@solr_doc), if can_download?(@solr_doc), or without calling the can_download function. This is the test I try and it always fails-
scenario 'local user should not be able to download file for localDisplay license object' do
visit file_path( @obj, '_1.mp4' )
expect(page).to have_selector('h1', :text => 'Forbidden')
end
Probably there are more things to do about file access control than just changing that line.
Do you think if I should continue trying to fix the file download issue in this branch or let Longshou works on that for the other ticket? Thanks

@lsitu
Copy link
Member

lsitu commented Jun 25, 2018

@mcritchlow /@VivianChu If file download is not straight forward with refactoring to the helper method, just leave it for ticket #467 for me to work on then. Thanks.

@mcritchlow
Copy link
Member

mcritchlow commented Jun 25, 2018

@VivianChu @lsitu - I defer to you both on who gets the file controller working. I suspect if authorize method returns a solr document, then the call to can_download? could potentially be on the next line by itself. Something like:

authorize! :show, @solr_doc
unless can_download?(@solr_doc)
  raise ActionController::RoutingError, "User is not allowed to download requested file: #{field}" 
end

Edit: @VivianChu - reading your described failed test more closely, I think if your test is expecting a 403 error to be shown to the user then the file controller will need to raise/send one if it encounters this situation. So that could be within that block or whatever you think makes sense. Given the way our application controller is setup now, maybe something like:

authorize! :show, @solr_doc
unless can_download?(@solr_doc)
  raise CanCan::AccessDenied, "User is not allowed to download requested file: #{field}" 
end

@VivianChu
Copy link
Member Author

@mcritchlow - I use the above block of code and the test passes for local user. However, it doesn't work correctly because now even if I use sign_in_developer to be a curator user, he/she can't download the file, he/she will see the forbidden text.

@mcritchlow
Copy link
Member

@VivianChu - ah i see, yeah that makes sense. we'd need to use a cancan check as a first conditional or something so that curators are allowed through regardless of the metadata checks in can_download? Or, you could have can_download? eturn true immediately if the user is a curator. Something like that maybe? Whatever you think best.

@lsitu
Copy link
Member

lsitu commented Jun 26, 2018

@VivianChu Please feel free to leave it for ticket #467 if you want, which is targeted the file download things.

Fix failed test

Add tests for complex video object

Remove fixture file

Add can_download function

Update rubocop

Remove file download check
@VivianChu
Copy link
Member Author

@mcritchlow @lsitu - I removed the can_download? check in file_controller so Longshou can work on his ticket about file download. Could you review the changes? Thanks

@mcritchlow
Copy link
Member

@VivianChu - Ok, got it. In that case, for the context of hiding links, as far as I can tell, this LGTM

@lsitu
Copy link
Member

lsitu commented Jun 26, 2018

👍
@mcritchlow / @VivianChu Yeah, I agree that the context seems right with the PR and I see that the file download things may need special check with CANCAN authorization, audio/videos context and thorough rspec testing, which is well under the context of ticket #467.

@lsitu lsitu merged commit 1eb8d3c into develop Jun 26, 2018
@lsitu lsitu deleted the feature/remove-download branch June 26, 2018 19:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants