Delegate read/edit groups and visibility to PermissionManager

Allow a `Hyrax::Resource` to cache a `PermissionManager` (and therefore an ACL
`ChangeSet`). Delegate current `#read_groups=` and similar management to it, and
also use it in `VisibilityReader` and `VisibilityWriter`. This allows each
Resource to maintain its own current permission state across the various access
lenses (visibility, `*_users/*groups`, and permissions) in a single place.

When we convert from an `ActiveFedora` object, we ensure that the current
permissions are copied down to the Resource. Notably we don't do this in the
other direction; our implementation of permissions in Valkyrie treats them as
separate resources, pointing back to repository Objects. Changing an ACL, and
then saving the object it points to has no effect: you need to save the ACL
explictly.

app/models/hyrax/resource.rb

@@ -4,12 +4,19 @@ module Hyrax
   ##
   # The base Valkyrie model for Hyrax.
   class Resource < Valkyrie::Resource
-    include Valkyrie::Resource::AccessControls
-
     attribute :alternate_ids, Valkyrie::Types::Array.of(Valkyrie::Types::ID)
     attribute :embargo,       Hyrax::Embargo.optional
     attribute :lease,         Hyrax::Lease.optional
 
+    delegate :edit_groups, :edit_groups=,
+             :edit_users,  :edit_users=,
+             :read_groups, :read_groups=,
+             :read_users,  :read_users=, to: :permission_manager
+
+    def permission_manager
+      @permission_manager ||= Hyrax::PermissionManager.new(resource: self)
+    end
+
     def visibility=(value)
       visibility_writer.assign_access_for(visibility: value)
     end

app/services/hyrax/access_control_list.rb

@@ -71,6 +71,12 @@ def permissions
       Set.new(change_set.permissions)
     end
 
+    ##
+    # @return [Array<Hyrax::Permission>]
+    def permissions=(new_permissions)
+      change_set.permissions = new_permissions.to_a
+    end
+
     ##
     # @example
     #    user = User.find('user_id')

app/services/hyrax/custom_queries/wings.rb

@@ -42,11 +42,12 @@ def find_many_by_alternate_ids(alternate_ids:, use_valkyrie: true)
       # @raise [Valkyrie::Persistence::ObjectNotFoundError]
       def find_access_control_for(resource:)
         if resource.respond_to?(:access_control_id)
+          raise Valkyrie::Persistence::ObjectNotFoundError if resource.access_control_id.blank?
           result = query_service.find_by(id: resource.access_control_id)
           result.access_to = resource.id # access_to won't be set in wings if there are no permissions
-          return result
+          result
         else
-          raise Valkyrie::Persistence::ObjectNotFoundError
+          raise Valkyrie::Persistence::ObjectNotFoundError,
                 "#{resource.internal_resource} is not a `Hydra::AccessControls::Permissions` implementer"
         end
       end

app/services/hyrax/permission_manager.rb

@@ -35,12 +35,12 @@ class PermissionManager
     ##
     # @!attribute [rw] acl
     #   @return [Hyrax::AccessControlList]
-    attr_reader :acl
+    attr_accessor :acl
 
     ##
     # @param resource [Valkyrie::Resource]
     def initialize(resource:, acl_class: Hyrax::AccessControlList)
-      @acl = acl_class.new(resource: resource)
+      self.acl = acl_class.new(resource: resource)
     end
 
     ##

app/services/hyrax/resource_visibility_propagator.rb

@@ -43,6 +43,8 @@ def propagate
         file_set.visibility = source.visibility
         embargo_manager.copy_embargo_to(target: file_set)
         lease_manager.copy_lease_to(target: file_set)
+
+        file_set.permission_manager.acl.save
         persister.save(resource: file_set)
       end
     end

app/services/hyrax/visibility_reader.rb

@@ -14,22 +14,26 @@ module Hyrax
   #
   class VisibilityReader
     ##
+    # @!attribute [r] permission_manager
+    #   @return [Hyrax::PermissionManager]
     # @!attribute [rw] resource
-    #   @return [Valkyrie::Resource::AccessControls]
+    #   @return [Valkyrie::Resource]
+    attr_reader   :permission_manager
     attr_accessor :resource
 
     ##
     # @param resource [Valkyrie::Resource::AccessControls]
     def initialize(resource:)
       self.resource = resource
+      @permission_manager = resource.permission_manager
     end
 
     ##
     # @return [String]
     def read
-      if resource.read_groups.include? Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_PUBLIC
+      if permission_manager.read_groups.include? Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_PUBLIC
         visibility_map.visibility_for(group: Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_PUBLIC)
-      elsif resource.read_groups.include? Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_AUTHENTICATED
+      elsif permission_manager.read_groups.include? Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_AUTHENTICATED
         visibility_map.visibility_for(group: Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_AUTHENTICATED)
       else
         visibility_map.visibility_for(group: :PRIVATE)

app/services/hyrax/visibility_writer.rb

@@ -17,23 +17,29 @@ module Hyrax
   #
   class VisibilityWriter
     ##
+    # @!attribute [r] permission_manager
+    #   @return [Hyrax::PermissionManager]
     # @!attribute [rw] resource
     #   @return [Valkyrie::Resource]
     attr_accessor :resource
+    attr_reader   :permission_manager
 
     ##
     # @param resource [Valkyrie::Resource]
     def initialize(resource:)
-      self.resource = resource
+      self.resource       = resource
+      @permission_manager = resource.permission_manager
     end
 
     ##
     # @param visibility [String]
     #
     # @return [void]
     def assign_access_for(visibility:)
-      resource.read_groups -= visibility_map.deletions_for(visibility: visibility)
-      resource.read_groups += visibility_map.additions_for(visibility: visibility)
+      permission_manager.read_groups =
+        permission_manager.read_groups.to_a - visibility_map.deletions_for(visibility: visibility)
+
+      permission_manager.read_groups += visibility_map.additions_for(visibility: visibility)
     end
 
     def visibility_map

lib/wings/active_fedora_converter.rb

@@ -158,29 +158,21 @@ def apply_depositor_to(af_object)
         af_object.apply_depositor_metadata(attributes[:depositor]) unless attributes[:depositor].blank?
       end
 
-      # rubocop:disable Metrics/MethodLength planning to remove half this method shortly
       # Add attributes from resource which aren't AF properties into af_object
       def add_access_control_attributes(af_object)
-        if af_object.is_a? Hydra::AccessControls::Permissions
-          af_object.read_users = attributes[:read_users]
-          af_object.edit_users = attributes[:edit_users]
-          af_object.read_groups = attributes[:read_groups]
-          af_object.edit_groups = attributes[:edit_groups]
-        elsif af_object.is_a? Hydra::AccessControl
-          cache = af_object.permissions.to_a
-
-          # if we've saved this before, it has a cache that won't clear
-          # when setting permissions! we need to reset it manually and
-          # rewrite with the values already in there, or saving will fail
-          # to delete cached items
-          af_object.permissions.reset if af_object.persisted?
-
-          af_object.permissions = cache.map do |permission|
-            permission.access_to_id = resource.try(:access_to)&.id
-            permission
-          end
+        return unless af_object.is_a? Hydra::AccessControl
+        cache = af_object.permissions.to_a
+
+        # if we've saved this before, it has a cache that won't clear
+        # when setting permissions! we need to reset it manually and
+        # rewrite with the values already in there, or saving will fail
+        # to delete cached items
+        af_object.permissions.reset if af_object.persisted?
+
+        af_object.permissions = cache.map do |permission|
+          permission.access_to_id = resource.try(:access_to)&.id
+          permission
         end
       end
-    # rubocop:enable Metrics/MethodLength
   end
 end

lib/wings/converter_value_mapper.rb

@@ -25,7 +25,6 @@ def result
         else
           type = 'person'
           name = permission_attrs[:agent].dup
-          name.slice!('person/')
         end
 
         hsh = { type: type, access: permission_attrs[:mode].to_s, name: name }

lib/wings/model_transformer.rb

@@ -73,7 +73,14 @@ def build
       attrs = attributes.tap { |hash| hash[:new_record] = pcdm_object.new_record? }
       attrs[:alternate_ids] = [::Valkyrie::ID.new(pcdm_object.id)] if pcdm_object.id
 
-      klass.new(**attrs)
+      klass.new(**attrs).tap { |resource| ensure_current_permissions(resource) }
+    end
+
+    def ensure_current_permissions(resource)
+      return unless pcdm_object.try(:access_control).present?
+
+      resource.permission_manager.acl.permissions =
+        pcdm_object.access_control.valkyrie_resource.permissions
     end
 
     ##
@@ -230,13 +237,9 @@ def reflection_ids
       end
 
       def additional_attributes
-        { created_at: pcdm_object.try(:create_date),
-          updated_at: pcdm_object.try(:modified_date),
-          read_groups: pcdm_object.try(:read_groups),
-          read_users: pcdm_object.try(:read_users),
-          edit_groups: pcdm_object.try(:edit_groups),
-          edit_users: pcdm_object.try(:edit_users),
-          member_ids: member_ids }
+        { created_at:  pcdm_object.try(:create_date),
+          updated_at:  pcdm_object.try(:modified_date),
+          member_ids:  member_ids }
       end
 
       # Prefer ordered members, but if ordered members don't exist, use non-ordered members.

spec/services/hyrax/resource_visibility_propagator_spec.rb

@@ -25,7 +25,7 @@
     before do
       fs = file_sets.first
       fs.visibility = 'open'
-      Hyrax.persister.save(resource: fs)
+      fs.permission_manager.acl.save
     end
 
     it 'copies visibility' do
@@ -50,7 +50,7 @@
     before do
       fs = file_sets.first
       fs.visibility = 'restricted'
-      Hyrax.persister.save(resource: fs)
+      fs.permission_manager.acl.save
     end
 
     it 'copies visibility' do
@@ -59,7 +59,7 @@
         .to contain_exactly(work.visibility, work.visibility)
     end
 
-    it 'applies a copy of the embargo' do
+    it 'applies a copy of the lease' do
       release_date = work.lease.lease_expiration_date
 
       expect { propagator.propagate }

spec/services/hyrax/visibility_reader_spec.rb

@@ -9,22 +9,22 @@
 
   describe '#read' do
     context 'when public group can read' do
-      before { resource.read_groups = [open] }
+      before { resource.permission_manager.read_groups = [open] }
 
       it 'is open' do
         expect(reader.read).to eq 'open'
       end
     end
 
     context 'when authenticated group can read' do
-      before { resource.read_groups = [auth] }
+      before { resource.permission_manager.read_groups = [auth] }
 
       it 'is authenticated' do
         expect(reader.read).to eq 'authenticated'
       end
 
       it 'and public can also read is open' do
-        resource.read_groups += [open]
+        resource.permission_manager.read_groups += [open]
         expect(reader.read).to eq 'open'
       end
     end

spec/services/hyrax/visibility_writer_spec.rb

@@ -11,7 +11,7 @@
     context 'when setting to public' do
       it 'adds public read group' do
         expect { writer.assign_access_for(visibility: open) }
-          .to change { resource.read_groups }
+          .to change { writer.permission_manager.read_groups.to_a }
           .to contain_exactly(writer.visibility_map[open][:permission])
       end
     end
@@ -21,7 +21,7 @@
         writer.assign_access_for(visibility: open)
 
         expect { writer.assign_access_for(visibility: auth) }
-          .to change { resource.read_groups }
+          .to change { writer.permission_manager.read_groups.to_a }
           .to contain_exactly(writer.visibility_map[auth][:permission])
       end
     end
@@ -31,15 +31,15 @@
         writer.assign_access_for(visibility: open)
 
         expect { writer.assign_access_for(visibility: restricted) }
-          .to change { resource.read_groups }
+          .to change { writer.permission_manager.read_groups.to_a }
           .to be_empty
       end
 
       it 'removes authenticated' do
         writer.assign_access_for(visibility: auth)
 
         expect { writer.assign_access_for(visibility: restricted) }
-          .to change { resource.read_groups }
+          .to change { writer.permission_manager.read_groups.to_a }
           .to be_empty
       end
     end

spec/wings/active_fedora_converter_spec.rb

@@ -69,48 +69,27 @@
       end
     end
 
-    context 'when specifying visibility' do
-      let(:attributes) do
-        FactoryBot.attributes_for(:generic_work)
-      end
-
-      let(:visibility) { Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC }
-
-      before { resource.visibility = visibility }
-
-      it 'sets the visibility' do
-        expect(converter.convert).to have_attributes(visibility: visibility)
-      end
-
-      context 'when restricted' do
-        let(:visibility) { Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED }
-
-        it 'sets the visibility' do
-          expect(converter.convert).to have_attributes(visibility: visibility)
-        end
-      end
-
-      context 'when private' do
-        let(:visibility) { Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PRIVATE }
-
-        it 'sets the visibility' do
-          expect(converter.convert).to have_attributes(visibility: visibility)
-        end
-      end
-    end
-
     context 'when setting ACLs' do
+      let(:resource)    { valkyrie_create(:hyrax_resource) }
+      let(:permissions) { Hyrax::PermissionManager.new(resource: resource) }
+      let(:user_key)    { create(:user).user_key }
+
       it 'converts ACLs' do
-        expect { resource.read_users = ['moomin'] }
+        permissions.read_users = [user_key]
+
+        expect { permissions.acl.save }
           .to change { described_class.new(resource: resource).convert }
-          .to have_attributes(read_users: contain_exactly('moomin'))
+          .to have_attributes(read_users: contain_exactly(user_key))
       end
 
       context 'when ACLs exist' do
-        let(:work) { FactoryBot.create(:public_work) }
+        let(:work)     { FactoryBot.create(:public_work) }
+        let(:resource) { work.valkyrie_resource }
 
         it 'can delete ACLs' do
-          expect { resource.read_groups = [] }
+          permissions.read_groups = []
+
+          expect { permissions.acl.save }
             .to change { described_class.new(resource: resource).convert }
             .from(have_attributes(read_groups: contain_exactly('public')))
             .to have_attributes(read_groups: be_empty)

spec/wings/valkyrie/persister_spec.rb

@@ -27,18 +27,6 @@ class Book < ActiveFedora::Base
 
     # it_behaves_like "a Valkyrie::Persister", :no_deep_nesting, :no_mixed_nesting
 
-    describe 'persists permissions' do
-      let(:depositor) { create(:user) }
-      let(:v_work) { resource_class.new(title: ['test_work'], depositor: depositor.user_key, read_groups: ['registered']) }
-
-      it 'has permissions once saved' do
-        saved = persister.save(resource: v_work)
-        expect(saved.read_groups).to contain_exactly 'registered'
-        expect(saved.depositor).to eq(depositor.user_key)
-        expect(saved.edit_users).to include(depositor.user_key)
-      end
-    end
-
     it { is_expected.to respond_to(:save).with_keywords(:resource) }
     it { is_expected.to respond_to(:save_all).with_keywords(:resources) }
     it { is_expected.to respond_to(:delete).with_keywords(:resource) }