Skip to content

Commit

Permalink
Lock unauthorized permissions from update
Browse files Browse the repository at this point in the history
Fixes #3166

- user is permitted to update any work permissions coming from
collections they manage
- user is permitted to update non-manager permissions from any
Collections
- user is permitted to update any non-collection permissions

Process used:
- find all of the work's collections a user can manage
- find all of the work's collections a user cannot manage
- find all of the other managers of collections a user can manage
- find all of the other managers of collections a user cannot manage who
are not also managers in collections that the user CAN manage.

This gives us the permissions the user is not authorized to update.

EditPermissionsService embeds all of the above logic and uses it to
display all of the work or file set's permissions in either a fixed or
editable format.

The new shared partial results in a slight reformat of the display of
these permissions, as there were previously minor differences between
the two displays.

Additionally, a new partial was created for batch edit, as the above
rules cannot be enforced at a batch level. Dealing with batch editing
sharing issues is beyond the scope of this issue.

Restrict user from changing managers which are in both authorized &
unauthorized collections
  • Loading branch information
LaRita Robinson authored and jeremyf committed Sep 3, 2020
1 parent 50e542b commit fde0590
Show file tree
Hide file tree
Showing 14 changed files with 414 additions and 103 deletions.
113 changes: 113 additions & 0 deletions app/services/hyrax/edit_permissions_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# frozen_string_literal: true
module Hyrax
class EditPermissionsService
# Encapsulates the logic to determine which object permissions may be edited by a given user
# - user is permitted to update any work permissions coming ONLY from collections they manage
# - user is not permitted to update a work permission if it comes from a collection they do not manage, even if also from a managed collection
# - user is permitted to update only non-manager permissions from any Collections
# - user is permitted to update any non-collection permissions
attr_reader :depositor, :unauthorized_collection_managers

# @param [Object] GenericWorkForm (if called for object) or GenericWork (if called for file set)
# @param [Ability] user's current_ability
def initialize(object:, ability:)
@object = object
@ability = ability
@depositor = object.depositor
unauthorized = manager_permissions_to_block
@unauthorized_managers = unauthorized.unauthorized_managers
@unauthorized_collection_managers = unauthorized.unauthorized_collection_managers
end

# @param [Hash] one set of permission fields for object {:name, :access}
# @return [Boolean] true if user cannot edit the given permissions
def cannot_edit_permissions?(permission_hash)
@unauthorized_managers.include?(permission_hash[:name]) && permission_hash[:access] == "edit"
end

# @param [Hash] one set of permission fields for object {:name, :access}
# @return [Boolean] true if given permissions are one of fixed exclusions
def excluded_permission?(permission_hash)
exclude_from_display.include? permission_hash[:name].downcase
end

private

# Fixed set of users & groups to exclude from "editable" section of display
def exclude_from_display
[::Ability.public_group_name, ::Ability.registered_group_name, ::Ability.admin_group_name, @depositor]
end

BlockedPermissions = Struct.new(:unauthorized_managers, :unauthorized_collection_managers)

# find all of the other managers of collections which a user cannot manage
#
# Process used:
# - find all of the work's collections which a user can manage
# - find all of the work's collections (of a type which shares permissions) that a user cannot manage
# - find all of the managers of these collections the user cannot manage
# This gives us the manager permissions the user is not authorized to update.
#
# @return [Struct] BlockedPermissions
# - unauthorized_managers [Array] ids of managers of all collections
# - unauthorized_collection_managers [Array hashes] manager ids & collection_ids [{:name, :id}]
def manager_permissions_to_block
unauthorized_managers = []
unauthorized_collection_managers = []
if object_unauthorized_collection_ids.any?
object_unauthorized_collection_ids.each do |id|
Hyrax::PermissionTemplate.find_by(source_id: id).access_grants.each do |grant|
if grant.access == "manage"
unauthorized_managers << grant.agent_id
unauthorized_collection_managers += Array.wrap({ name: grant.agent_id }.merge(id: id))
end
end
end
end
BlockedPermissions.new(unauthorized_managers, unauthorized_collection_managers)
end

# find all of the work's collections a user can manage
# @return [Array] of collection ids
def object_managed_collection_ids
@object_managed_collection_ids ||= object_member_of & managed_collection_ids
end

# find all of the work's collections a user cannot manage
# note: if the collection type doesn't include "sharing_applies_to_new_works", we don't limit access
# @return [Array] of collection ids with limited access
def object_unauthorized_collection_ids
@object_unauthorized_collection_ids ||= begin
limited_access = []
unauthorized_collection_ids = object_member_of - object_managed_collection_ids
if unauthorized_collection_ids.any?
unauthorized_collection_ids.each do |id|
collection = ActiveFedora::Base.find(id)
limited_access << id if (collection.instance_of? AdminSet) || collection.share_applies_to_new_works?
end
end
limited_access
end
end

# find all of the collection ids an object is a member of
# @return [Array] array of collection ids
def object_member_of
@object_member_of ||= begin
belongs_to = []
# get all of work's collection ids from the form
@object.member_of_collections.each do |collection|
belongs_to << collection.id
end
belongs_to << @object.admin_set_id unless @object.admin_set_id.empty?
belongs_to
end
end

# The list of all collections this user has manage rights on
# @return [Array] array of all collection ids that user can manage
def managed_collection_ids
Hyrax::Collections::PermissionsService.source_ids_for_manage(ability: @ability)
end
end
end
66 changes: 66 additions & 0 deletions app/views/hyrax/base/_currently_shared.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<%#
# form object.class = SimpleForm::FormBuilder
# For works (i.e. GenericWork):
# - form object.object = Hyrax::GenericWorkForm
# - form object.object.model = GenericWork
# - use the work itself
# For file_sets:
# - form object.object.class = FileSet
# - use work the file_set is in
# No other object types are supported by this view. %>
<% if f.object.respond_to?(:model) && f.object.model.work?
object_acting_upon = f.object
elsif f.object.file_set?
object_acting_upon = f.object.in_works.first
end %>
<% permission_service = Hyrax::EditPermissionsService.new(object: object_acting_upon, ability: current_ability) %>
<% depositor = permission_service.depositor %>

<h2><%= t('.currently_sharing') %></h2>

<table class="table table-bordered">
<tr>
<th><%= t('.table_title_user') %></th>
<th><div class="col-sm-10"><%= t('.table_title_access') %></div></th>
</tr>
<tr id="file_permissions">
<td>
<%= label_tag :owner_access, class: "control-label" do %>
Depositor (<span id="file_owner" data-depositor="<%= depositor %>"><%= link_to_profile depositor %></span>)
<% end %>
</td>
<td>
<div class="col-sm-10">
<%= Hyrax.config.owner_permission_levels.keys[0] %>
</div>
</td>
</tr>
<%= f.fields_for :permissions do |permission_fields| %>
<% perm_hash = permission_fields.object.to_hash %>
<% next if permission_service.excluded_permission?(perm_hash) %>
<% cannot_edit_perms = permission_service.cannot_edit_permissions?(perm_hash) %>
<tr>
<td>
<%= permission_fields.label :agent_name, class: "control-label" do %>
<%= user_display_name_and_key(perm_hash[:name]) %>
<% permission_service.unauthorized_collection_managers.select {|mgrs| mgrs[:name] == perm_hash[:name] }.each do |coll| %>
<br />Access granted via collection <%= coll[:id] %>
<% end %>
<% end %>
</td>
<td>
<div class="col-sm-10">
<% if cannot_edit_perms %>
<%= Hyrax.config.permission_levels.key(perm_hash[:access]) %>
<% else %>
<%= permission_fields.select :access, Hyrax.config.permission_levels, {}, class: 'form-control select_perm' %>
<% end %>
</div>
<% if !cannot_edit_perms %>
<button class="btn close remove_perm" data-index="<%= permission_fields.index %>">&times;</button>
<% end %>
</td>
</tr>
<% end %>
</table>
38 changes: 5 additions & 33 deletions app/views/hyrax/base/_form_share.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -49,39 +49,11 @@
</div>
</fieldset>

<h2><%= t('.currently_sharing') %></h2>

<table class="table">
<tr id="file_permissions">
<td width="20%">
<%= configured_owner_permission_levels.keys.first %>
</td>
<td width="60%">
<%= label_tag :owner_access, class: "control-label" do %>
<%= t('.depositor') %>(<span id="file_owner" data-depositor="<%= depositor %>"><%= link_to_profile depositor %></span>)
<% end %>
</td>
</tr>
<%= f.fields_for :permissions do |permission_fields| %>
<%# skip the public, registered, and depositor perms as they are displayed first at the top %>
<% next if ['public', 'registered', depositor].include? permission_fields.object.agent_name.downcase %>
<tr>
<td>
<%= permission_fields.select :access, configured_permission_levels, {}, class: 'form-control select_perm' %>
</td>
<td>
<%= permission_fields.label :agent_name, class: "control-label" do %>
<%= user_display_name_and_key(permission_fields.object.agent_name) %>
<% end %>
<button class="btn close remove_perm" data-index="<%= permission_fields.index %>">&times;</button>
</td>
</tr>
<% end %>
</table>
<%= render 'currently_shared', f: f %>

<script type="text/x-tmpl" id="tmpl-work-grant">
<tr>
<td>{%= o.accessLabel %}</td>
<td><label class="control-label">{%= o.name %}</label> <button class="btn close">&times;</button></td>
</tr>
<tr>
<td><label class="control-label">{%= o.name %}</label></td>
<td><div class="col-sm-10">{%= o.accessLabel %}<button class="btn close">&times;</button></div></td>
</tr>
</script>
8 changes: 8 additions & 0 deletions app/views/hyrax/batch_edits/_currently_shared.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<h2><%= t('.share_batch_with') %></h2>

<table class="table table-bordered">
<tr id="file_permissions">
<th width="65%"><%= t('.table_title_user') %></th>
<th><div class="col-sm-10"><%= t('.table_title_access') %></div></th>
</tr>
</table>
33 changes: 2 additions & 31 deletions app/views/hyrax/file_sets/_permission_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -65,37 +65,8 @@
</div>
</div>

<table class="table table-bordered">
<tr>
<th width="60%"><%= t('.table_title_user') %></th>
<th width="40%"><%= t('.table_title_access') %></th>
</tr>
<tr id="file_permissions">
<td>
<%= label_tag :owner_access, class: "control-label" do %>
<%= t('.depositor') %> (<span id="file_owner" data-depositor="<%= depositor %>"><%= link_to_profile depositor %></span>)
<% end %>
</td>
<td>
<%= configured_owner_permission_levels.keys.first %>
</td>
</tr>
<%= f.fields_for :permissions do |permission_fields| %>
<%# skip the public, registered, and depositor perms as they are displayed first at the top %>
<% next if ['public', 'registered', depositor].include? permission_fields.object.agent_name.downcase %>
<tr>
<td><%= permission_fields.label :agent_name, class: "control-label" do %>
<%= user_display_name_and_key(permission_fields.object.agent_name) %>
<% end %></td>
<td>
<div class="col-sm-8">
<%= permission_fields.select :access, Hyrax.config.permission_levels, {}, class: 'form-control select_perm' %>
</div>
<button class="btn close remove_perm" data-index="<%= permission_fields.index %>">X</button>
</td>
</tr>
<% end %>
</table>
<%= render 'currently_shared', f: f %>

<script type="text/x-tmpl" id="tmpl-file-set-grant">
<tr>
<td><label class="control-label">{%= o.name %}</label></td>
Expand Down
10 changes: 8 additions & 2 deletions config/locales/hyrax.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,10 @@ en:
header: 'Citations:'
mendeley: Mendeley
zotero: Zotero
currently_shared:
currently_sharing: Currently Shared With
table_title_access: Access Level
table_title_user: Person/Group
file_manager:
back_to: Back to
toolbar: Toolbar
Expand Down Expand Up @@ -534,6 +538,10 @@ en:
batch_edits:
check_all:
select_to_access_selection_options: Select to access selection option
currently_shared:
share_batch_with: Share Batch With
table_title_access: Access Level
table_title_user: Person/Group
delete_selected:
button_label: Delete Selected
deleting_file_from: Deleting a file from %{application_name} is permanent. Click OK to delete this file from %{application_name}, or Cancel to cancel this operation
Expand Down Expand Up @@ -1088,8 +1096,6 @@ en:
save_note_html: Permissions are <strong>not</strong> saved until the &quot;Save&quot; button is pressed at the bottom of the page.
select_group: Select a group
share_with: Share With
table_title_access: Access Level
table_title_user: Person/Group
user_search: Search for a user
proxy:
message: "%{sending_user} has deposited the file %{title} on your behalf."
Expand Down
2 changes: 1 addition & 1 deletion spec/features/batch_edit_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
let(:permission_template) { create(:permission_template, source_id: admin_set.id) }
let!(:workflow) { create(:workflow, allows_access_grant: true, active: true, permission_template_id: permission_template.id) }

let!(:work1) { create(:public_work, admin_set_id: admin_set.id, user: current_user, members: [file_set]) }
let!(:work1) { create(:public_work, admin_set_id: admin_set.id, user: current_user, ordered_members: [file_set]) }
let!(:work2) { create(:public_work, admin_set_id: admin_set.id, user: current_user) }
let!(:file_set) { create(:file_set) }

Expand Down
2 changes: 1 addition & 1 deletion spec/features/collection_multi_membership_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
end

describe 'when both collections require single membership' do
let(:old_collection) { build(:collection_lw, user: admin_user, collection_type_gid: single_membership_type_1.gid, title: ['OldCollectionTitle']) }
let(:old_collection) { build(:collection_lw, user: admin_user, collection_type_gid: single_membership_type_1.gid, title: ['OldCollectionTitle'], with_permission_template: true) }
let!(:work) do
create(:generic_work,
user: admin_user,
Expand Down
5 changes: 4 additions & 1 deletion spec/features/edit_file_spec.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
# frozen_string_literal: true
RSpec.describe "Editing a file:", type: :feature do
let(:user) { create(:user) }
let(:admin_set) { create(:admin_set) }
let(:permission_template) { create(:permission_template, source_id: admin_set.id) }
let!(:workflow) { create(:workflow, allows_access_grant: true, active: true, permission_template_id: permission_template.id) }
let(:file_title) { 'Some kind of title' }
let(:work) { build(:work, user: user) }
let(:work) { build(:work, user: user, admin_set_id: admin_set.id) }
let(:file_set) { create(:file_set, user: user, title: [file_title]) }
let(:file) { File.open(fixture_path + '/world.png') }

Expand Down
24 changes: 11 additions & 13 deletions spec/features/edit_work_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@
work.ordered_members << create(:file_set, user: user, title: ['ABC123xyz'])
work.read_groups = []
work.save!

create(:permission_template_access,
:deposit,
permission_template: create(:permission_template, source_id: default_admin_set.id, with_admin_set: true, with_active_workflow: true),
agent_type: 'user',
agent_id: user.user_key)
create(:permission_template_access,
:deposit,
permission_template: create(:permission_template, source_id: another_admin_set.id, with_admin_set: true, with_active_workflow: true),
agent_type: 'user',
agent_id: user.user_key)
end

context 'when the user changes permissions' do
Expand All @@ -41,19 +52,6 @@
end

context 'when form loads' do
before do
create(:permission_template_access,
:deposit,
permission_template: create(:permission_template, source_id: default_admin_set.id, with_admin_set: true, with_active_workflow: true),
agent_type: 'user',
agent_id: user.user_key)
create(:permission_template_access,
:deposit,
permission_template: create(:permission_template, source_id: another_admin_set.id, with_admin_set: true, with_active_workflow: true),
agent_type: 'user',
agent_id: user.user_key)
end

it 'selects admin set already assigned' do
visit edit_hyrax_generic_work_path(work)
click_link "Relationships" # switch tab
Expand Down
Loading

0 comments on commit fde0590

Please sign in to comment.