Fixes #467 - Disable file downloading for localDisplay videos/audios and metadataDisplay objects. #473

lsitu · 2018-06-27T20:10:51Z

Fixes #467

Local Checklist

What does this PR do?

Disable file downloading for localDisplay videos/audios and metadataDisplay objects, while allowing campus users to download localDisplay images.

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

References #466, #467

@ucsdlib/developers - please review

metadataDisplay objects.

mcritchlow

This is really great, and a complex set of use cases to keep track of. I have only a few questions/comments to get your thoughts on. Overall this is 💯

mcritchlow · 2018-06-27T20:37:25Z

app/controllers/file_controller.rb

@@ -44,6 +44,9 @@ def show
      authorize! :edit, @solr_doc
    elsif !@solr_doc['read_access_group_ssim'].include?("public")
      authorize! :show, @solr_doc
+      unless can? :edit, @solr_doc


I'm a little torn on how I feel about the nested unless checks here. After a second read through it makes sense though. But part of me wonders if this is easier to read if we flip the checks and use || instead. Something like:

unless can_download?(@solr_doc) || can?(:edit, @solr_doc) CanCan::AccessDenied, "User is not allowed to download file: #{field}"

Would that logic work? I may be missing an example where this wouldn't be acceptable. It also might have its own readability issues..

mcritchlow · 2018-06-27T20:56:09Z

lib/dams/controller_helper.rb

-        local_license = document['license_tesim'] && document['license_tesim'].first.to_s.include?('localDisplay') ? true : false
+      license = document['license_tesim']
+      other_rights = document['otherRights_tesim']
+      return false if streaming_media_type?(document['resource_type_tesim']) && (local_display?(license) || local_display?(other_rights))


There's a lot going on here with these conditionals. I know there are many cases to test, but i wonder if we could shrink them doing at all by breaking the returns apart? Like off the top, if the file is not a streaming type, we could presumably return true immediately? And/or maybe the local_display and metadata_only_display checks could be individual conditionals?

The individual supporting methods are great, it's just that the conditional combination here is long.

Maybe some comments are necessary here to provide the narrative for what we're doing here, given the complexity? Curious what you think. Basically I'm thinking this might be easy to forget or understand in a few months if/when we have to port this to DAMS5.

Sound good. I'll refactor it for the conditional combination, and I see it more complicated with those UCSD IP/metadata-only collections that won't allow local download but need VPN/campus view.

mcritchlow · 2018-06-27T21:07:00Z

lib/dams/controller_helper.rb

+    end
+
+    def streaming_media_type?(resource_types)
+      resource_types.each do |t|


For these support methods you might be able to use the .any? Enumerable method to avoid the explicit returns. Something like:

def streaming_media_type?(resource_types) resource_types.any? { |t| t.include?('video') || t.include?('sound') } end

mcritchlow · 2018-06-27T21:08:40Z

spec/controllers/file_controller_spec.rb

@@ -0,0 +1,516 @@
+# frozen_string_literal: true
+
+require 'spec_helper'


This is a great test setup 👏 I'll have to tweak this once we fold in the send_file code, but I already have a pattern in place for that. And these are an excellent set of comprehensive tests.

mcritchlow · 2018-06-27T22:27:43Z

@lsitu - Here's another idea, just to put out there. What if we made can_download? an actual cancan method?

So instead of doing the edit check and then our custom method, we ideally do something like this in the file controller, or even use authorize!:

unless can? :download_file, @solr_doc
  <raise 403 error>
end

We could possibly borrow some ideas from Hydra::Ability and this custom actions page?

I think this would mostly involve moving code into ability.rb. The nice part, if this works, is it would let us use CanCan, which it kinda feels like we ideally should be for something like this. Although the context of the @solr_doc might be a bit confusing, since one isn't trying to download that, but rather a file the doc references. But I suppose we could make the same argument about the other actions to some extent..

That said, if you think it's too much of a change to make, or a bad idea for other reasons, then feel free to ignore :)

lsitu · 2018-06-27T23:59:10Z

@mcritchlow Good point! I was thinking about it but it seem like we may need to add the file_id parameter to the can_download? method to support the specs in #466 that need those UCSD IP/metadata collections to be viewable from campus or through VPN (UCSD IP restriction).
In this case, we may need to disallow the downloading of files like _2.jpg, as well as other _1.* service files. So it seem like we should keep it as a method. What do you think? Do you think if we make a simple check with the filename will be enough, or need to look up and parse the data for all the files o check the file use property, which is not so efficient for a match?

mcritchlow · 2018-06-28T15:09:53Z

@lsitu - Yeah i suspect it might be best to just leave your existing implementation as proposed in this PR as-is, because there's enough complexity in this that it's probably not worth it. it was just a thought that occurred to me :) hopefully next time we do this, in dams5, we'll have more rational metadata for making this decision. we'll see..

lsitu · 2018-06-28T15:23:16Z

@mcritchlow Yep. It sounds good. I was thinking whether we should at least filter those _2.jpg from downloading or not. Thanks.

houndci-bot · 2018-06-28T18:23:09Z

app/controllers/file_controller.rb

@@ -44,6 +44,9 @@ def show
      authorize! :edit, @solr_doc
    elsif !@solr_doc['read_access_group_ssim'].include?("public")
      authorize! :show, @solr_doc
+      unless can?(:edit, @solr_doc) || can_download?(@solr_doc, fileid)


Style/IfUnlessModifier: Favor modifier unless usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

coveralls · 2018-06-28T19:44:09Z

Coverage increased (+0.06%) to 65.98% when pulling af720b8 on feature/file_downloading into 1eb8d3c on develop.

no download but viewable from campus.

VivianChu · 2018-06-29T15:48:01Z

👍

mcritchlow · 2018-06-29T15:55:29Z

lib/dams/controller_helper.rb

-      end
-      !local_other_rights && !local_license
+    def restricted_file?(fileid)
+      fileid && fileid.include?('_2.jpg')


What is specific to an _2.jpg being a restricted file? I don't think I understand the meaning behind this one.

mcritchlow · 2018-06-29T16:06:19Z

lib/dams/controller_helper.rb

-    return result unless data
-    data.each do |datum|
-      result = true if datum.include?('localDisplay') || datum.include?('metadataDisplay')
+    def metadata_display?(data)


Since we already have this metadata_display? method, and it appears to be used in the catalog_helper and dams_resource_controller, do we need the local_display? and metadata_only_display? methods? Could we use this instead? This one seems to just check both (though it could be rewritten to use any? also like the others. Just a thought.

@mcritchlow I think we may need to a separate method for metadata_only_display? to determine the different behavior with localDisplay and metadataDisplay objects. For localDisplay objects, they allow downloading of files (video/audio excluded) while metadataDisplay won't allow file download for any files.

@lsitu - Ok, thanks for clarifying. Perhaps I misunderstood what Greg said. I thought he said if either were filled in then one could not download derivatives. But yeah if there are distinctions then I guess that's another issue. Poorly named properties it turns out, because they don't actually describe what they do :)

@mcritchlow Yes, it's confusing to me too. So I asked Greg to clarify the localDisplay thing earlier since the access control issue is always sensitive to us. Do you want to rename it?

@lsitu - I'm not sure. Honestly, all your tests pass and I think it's a pretty comprehensive test suite. I don't want to slow this down more than necessary. I just think the data model implications and application to these access control scenarios is unsustainable, very fragile at the least. But that's hopefully an issue we can address in the next system. Perhaps unless @hweng or @VivianChu have any other concerns, I'll just merge this and we can move on? (Thanks for all your awesome work on this complicated ticket)

@mcritchlow Could you hold on? I'll refactor it a little bit. Thanks.

lsitu · 2018-06-29T16:15:54Z

@mcritchlow Good question!
To support UCSD IP/metadata-only collections with camps/VPN access behavior, we need to disable the download button and the downloading of the files, but we also need the objects to be viewable, which requires file download for the object viewers. Those _2.jpeg full screen resolution images are linking to the download button so I just restrict them from downloading at this time. I think we can add any other restricted file types from downloading when necessary. Just let me know what we want to start with and I think we can adjust it now or later. Thanks.

mcritchlow · 2018-06-29T16:26:21Z

@lsitu - Ok, I think I understand. This is just really messy. Not your code, just the very custom conditions we're allowing, or not allowing here. I wonder if this method needs to be named more specific to the context. Because on its own, a 2.jpg is usually not restricted, as you know.

At this point in the conditional checking in can_download?, do we need to limit the file to 2.jpg? Basically, if we're in a metadata-only situation, I'm curious what files a user is allowed to download? Thumbnail? Can they even see that? I guess if I think "metadata only", that makes me think "no files can be downloaded". But perhaps that's not true?

lsitu · 2018-06-29T16:46:40Z

Yep, good questions again. I think "metadata only" is only for public users, things become complicated/confusing for campus /VPN (UCSD IP restricted) users. The issue is that why the download button need to be removed from the viewer for those UCSD IP/metadata-only collections. And as you noted, does those images link to the download button a matter? If the answer is no, then removing of the download button seems to be a question.

IP/metadata-only collections.

lsitu · 2018-06-29T17:50:33Z

@mcritchlow I've refactored the metadata_display? codes to use it to include audio/video download controlling for the UCSD IP/metadata-only collections with the additional tests added.

mcritchlow · 2018-06-29T18:12:09Z

LGTM

hweng · 2018-06-29T21:01:40Z

👍

Fixes #467 - Disable file downloading for localDisplay videos/audios and

e50500a

metadataDisplay objects.

mcritchlow reviewed Jun 27, 2018

View reviewed changes

houndci-bot reviewed Jun 28, 2018

View reviewed changes

lsitu force-pushed the feature/file_downloading branch from 253fd81 to f2b80f9 Compare June 28, 2018 22:57

Refoctor codes and added support for UCSD IP/metadata-only objects with

ec61352

no download but viewable from campus.

lsitu force-pushed the feature/file_downloading branch from f2b80f9 to ec61352 Compare June 28, 2018 23:31

mcritchlow reviewed Jun 29, 2018

View reviewed changes

Refactor codes to add video/audio download restriction for UCSD

af720b8

IP/metadata-only collections.

VivianChu merged commit fdb14d0 into develop Jun 29, 2018

VivianChu deleted the feature/file_downloading branch June 29, 2018 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #467 - Disable file downloading for localDisplay videos/audios and metadataDisplay objects. #473

Fixes #467 - Disable file downloading for localDisplay videos/audios and metadataDisplay objects. #473

lsitu commented Jun 27, 2018

mcritchlow left a comment

mcritchlow Jun 27, 2018

mcritchlow Jun 27, 2018

lsitu Jun 27, 2018

mcritchlow Jun 27, 2018

mcritchlow Jun 27, 2018

mcritchlow commented Jun 27, 2018 •

edited

Loading

lsitu commented Jun 27, 2018

mcritchlow commented Jun 28, 2018

lsitu commented Jun 28, 2018

houndci-bot Jun 28, 2018

coveralls commented Jun 28, 2018 •

edited

Loading

VivianChu commented Jun 29, 2018

mcritchlow Jun 29, 2018

mcritchlow Jun 29, 2018

lsitu Jun 29, 2018

mcritchlow Jun 29, 2018

lsitu Jun 29, 2018

mcritchlow Jun 29, 2018

lsitu Jun 29, 2018

lsitu commented Jun 29, 2018

mcritchlow commented Jun 29, 2018 •

edited

Loading

lsitu commented Jun 29, 2018

lsitu commented Jun 29, 2018

mcritchlow commented Jun 29, 2018

hweng commented Jun 29, 2018

		@@ -0,0 +1,516 @@
		# frozen_string_literal: true

		require 'spec_helper'

Fixes #467 - Disable file downloading for localDisplay videos/audios and metadataDisplay objects. #473

Fixes #467 - Disable file downloading for localDisplay videos/audios and metadataDisplay objects. #473

Conversation

lsitu commented Jun 27, 2018

Local Checklist

What does this PR do?

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

mcritchlow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcritchlow commented Jun 27, 2018 • edited Loading

lsitu commented Jun 27, 2018

mcritchlow commented Jun 28, 2018

lsitu commented Jun 28, 2018

Choose a reason for hiding this comment

coveralls commented Jun 28, 2018 • edited Loading

VivianChu commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsitu commented Jun 29, 2018

mcritchlow commented Jun 29, 2018 • edited Loading

lsitu commented Jun 29, 2018

lsitu commented Jun 29, 2018

mcritchlow commented Jun 29, 2018

hweng commented Jun 29, 2018

mcritchlow commented Jun 27, 2018 •

edited

Loading

coveralls commented Jun 28, 2018 •

edited

Loading

mcritchlow commented Jun 29, 2018 •

edited

Loading