Implemented the file use model with access control for file downloads. #129

lsitu · 2017-07-31T23:09:25Z

Fixes #111 ; refs #111

Implemented the file use model with access control for file downloads.

Changes proposed in this pull request:

Implemented the file use model to allow ingesting files with other uses into the same fileset as the original file.
Added support to index technical metadata as JSON format for all files in a FileSet for UI display.
Implemented access control for to restrict the PreservationMasterFile and the TIFF format files for curator-only access.
Display additional files like PreservationMasterFile in the FileSet viewer and enabled file download for curators.

@ucsdlib/developers - please review

mcritchlow

Added some comments/questions. I think my only overall concern is the number of upstream classes that we seem to be overriding. So my questions are largely targeted around trying to understand the large set of overrides.

Perhaps there isn't an alternative, but I admittedly worry a bit about the sustainability/upgrade path. Hoping @VivianChu and @hweng can take a look as well.

This is not to discount the work on this, really great job @lsitu 👍

mcritchlow · 2017-08-01T16:35:55Z

app/actors/file_set_attach_files_actor.rb

@@ -0,0 +1,33 @@
+# Actions are decoupled from controller logic so that they may be called from a controller or a background job.
+class FileSetAttachFilesActor < Hyrax::Actors::FileSetActor


What change was needed here that required us overriding the (presumably) upstream FileSetAttachFilesActor?

I think we just extend/override the Hyrax::Actors::FileSetActor to accept a relation/file use parameter.

mcritchlow · 2017-08-01T16:36:43Z

app/actors/hyrax/actors/create_with_files_actor.rb

+module Hyrax
+  module Actors
+    # Creates a work and attaches files to the work
+    class CreateWithFilesActor < Hyrax::Actors::AbstractActor


What change was needed here that required us overriding the (presumably) upstream CreateWithFilesActor ?

I think we need to override it to extract file uses attributes from the object metadata and extend method attach_files(files, env, file_uses) to accept the file uses parameter for ingest.

mcritchlow · 2017-08-01T16:38:17Z

app/actors/hyrax/actors/create_with_remote_files_actor.rb

+module Hyrax
+  module Actors
+    # Attaches remote files to the work
+    class CreateWithRemoteFilesActor < Hyrax::Actors::AbstractActor


What change was needed here that required us overriding the (presumably) upstream CreateWithRemoteFilesActor?

The same as above to extract file uses from object metadata and extend it to accept the file uses properties for ingest.

mcritchlow · 2017-08-01T16:41:16Z

app/controllers/hyrax/downloads_controller.rb

+      # return [binary]
+      def relation_content(relation)
+        case relation
+        when 'preservation_master_file'


Could preservation_master_file be a constant instead of a plain string? Perhaps available via the new ExtendedContainedFiles

Done in commit e8696d3. Thanks.

mcritchlow · 2017-08-01T16:41:31Z

app/controllers/hyrax/downloads_controller.rb

+        when 'preservation_master_file'
+          preservation_master_file = asset.attached_files.base.preservation_master_file
+          return preservation_master_file if preservation_master_file
+        when 'transcript'


Could transcript be a constant instead of a plain string? Perhaps available via the new ExtendedContainedFiles

Also done in commit e8696d3. Thanks.

mcritchlow · 2017-08-01T16:44:36Z

app/jobs/attach_files_to_file_set_job.rb

@@ -0,0 +1,75 @@
+# Converts UploadedFiles into FileSets and attaches them to works.
+class AttachFilesToFileSetJob < AttachFilesToWorkJob


What change was needed here that required us overriding the (presumably) upstream AttachFilesToFileSetJob?

I think we need to extend the functionality to accept the file use parameter and perform ingest files with those files use properties provided as well.

mcritchlow · 2017-08-01T16:47:43Z

app/models/ability.rb

+
+    def download_file_permission
+      can :read, ActiveFedora::File do |file|
+        file.mime_type == 'image/tiff' ? false : true


Why is this check specifically targeting image/tiff? What are we trying to prevent here? The download of source/master files?

This is implemented because Ryan's comment for not allowing public users to download the TIFF files, see #111 (comment),.

I see. So is this the primarily place where we're ensuring original_file's aren't downloaded by non-curators?

Because if so, it seems like we have far more file types than tiff which would fall under this check. My reading of Ryan's comment is tiff was just a (common) example. Not the sole use case or instance where we have this use case. And, it also seems like it would be nice if we could check the file_use instead of the mime_type to determine download-ability? Should we clarify with Ryan?

@mcritchlow Yes, this is one and there could be the videos/audios as well. But it will require deeper and tricker overriding in the derivative creative and technical metadata extraction layer if we change the strategic to ingest a primary file with File Use other than the OriginalFile, which will make it harder for us to maintain our codes over time. So it seems like checking for mime_type la cleaner solution for us at the moment.

mcritchlow · 2017-08-01T16:55:19Z

app/indexers/file_set_indexer.rb

@@ -2,4 +2,54 @@

 class FileSetIndexer < Hyrax::FileSetIndexer
  self.thumbnail_path_service = ::ThumbnailPathService
+
+  def generate_solr_document


was there previously no Solr document content generated for FileSets? Or are we overriding what was there and generating our own here?

I think hyrax does have solr document generate for FileSets but unfortunately it won't support ingest/store multiples files in one fileset at this time. And there are no strategic to store technical metadata for other files other than the original file in the same fileset. So we need to extend it to include technical metadata for all other files in Solr document so that we can expose them for UI displaying.

Ok, that makes sense. Thanks for clarifying 👍

mcritchlow · 2017-08-01T16:56:07Z

app/jobs/import_url_with_file_use_job.rb

+# download that file and put it into Fedora
+# Called by AttachFilesToWorkJob (when files are uploaded to s3)
+# and CreateWithRemoteFilesActor when files are located in some other service.
+class ImportUrlWithFileUseJob < ::ImportUrlJob


What change was needed here that required us overriding the (presumably) upstream ImportUrlWithFileUseJob?

I think the main purpose for overriding here is also to extend the functionality to accept the file_use/relation parameter and ingest the file with the file use specified

TIFF with curator only access restriction.

coveralls · 2017-08-01T18:07:11Z

Coverage increased (+0.2%) to 92.58% when pulling e8696d3 on feature/file_uses into 4989221 on develop.

VivianChu · 2017-08-03T20:55:11Z

👍

mcritchlow · 2017-08-08T17:37:40Z

Noting @lsitu 's post on samvera google group which finally got some discussion going. I'm going to attend the tech call tomorrow to get more feedback from folks. Then we can circle back and discuss next steps with this. So let's hold the merge for now, please.

Fixes #111 - Implemented the file use model.

ac1ccec

lsitu mentioned this pull request Jul 31, 2017

Finalize File Use modeling #111

Closed

mcritchlow reviewed Aug 1, 2017

View reviewed changes

lsitu force-pushed the feature/file_uses branch from a7ccba9 to e8696d3 Compare August 1, 2017 17:45

Fixes #111 - Implemented access controll for PreservationMasterFile and

e8696d3

TIFF with curator only access restriction.

mcritchlow mentioned this pull request Sep 5, 2017

Support File-level access control samvera/hyrax#1621

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented the file use model with access control for file downloads. #129

Implemented the file use model with access control for file downloads. #129

lsitu commented Jul 31, 2017

mcritchlow left a comment

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

mcritchlow Aug 1, 2017

mcritchlow Aug 1, 2017

lsitu Aug 1, 2017

coveralls commented Aug 1, 2017

VivianChu commented Aug 3, 2017

mcritchlow commented Aug 8, 2017

		@@ -0,0 +1,33 @@
		# Actions are decoupled from controller logic so that they may be called from a controller or a background job.
		class FileSetAttachFilesActor < Hyrax::Actors::FileSetActor

		@@ -0,0 +1,75 @@
		# Converts UploadedFiles into FileSets and attaches them to works.
		class AttachFilesToFileSetJob < AttachFilesToWorkJob

Implemented the file use model with access control for file downloads. #129

Are you sure you want to change the base?

Implemented the file use model with access control for file downloads. #129

Conversation

lsitu commented Jul 31, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 1, 2017

VivianChu commented Aug 3, 2017

mcritchlow commented Aug 8, 2017