Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User can remove (repo admin or any user) permission in Sharing tab on work #3166

Closed
4 tasks done
julesies opened this issue Jul 25, 2018 · 14 comments · Fixed by #4497
Closed
4 tasks done

User can remove (repo admin or any user) permission in Sharing tab on work #3166

julesies opened this issue Jul 25, 2018 · 14 comments · Fixed by #4497
Assignees
Milestone

Comments

@julesies
Copy link

julesies commented Jul 25, 2018

Descriptive summary

Hyrax 2.1. We are testing permissions for our go live of hyrax and came across this bug which I confirmed on nurax today.

A user/depositor can remove any person/role via the Sharing tab in the UI including the repo admin. We tested adding a work and having the depositor remove access to the repo admin, which was allowed. The repo admin could still access the work and edit it, but on visiting the sharing tab, the role was not visible.

Expected behavior

depositor can only see, remove roles/permissions that they added. permissions that are removed, are actually removed.

Steps to reproduce the behavior

  1. Create a work as a regular user
  2. Go to sharing tab
  3. Remove admin access, hit save
  4. log in as admin, find and edit work, hit save

Done looks like

  • Admins cannot have their edit access revoked by any user for any work
  • Users cannot override edit access permissions created by the work's admin set or collection
  • User view: the "currently shared with" table hides rows for admin and any groups that exist as participants in the work's admin set or collection
  • Admin/ Manager/ Editor view: the "currently shared with" table shows admin and all users and groups that have edit access to the work (for example, if User A is listed as a Manager of a collection, User A can go to any work in that collect, click Edit, view the Sharing tab, and see a table that lists all users and groups that have edit access to that work and act according to the settings of the admin set or collection containing that work).
@julesies
Copy link
Author

@vantuyls @chrisdaaz @no-reply ping!

@julesies julesies added the bug label Jul 25, 2018
@julesies
Copy link
Author

Also, I just tested adding an editor to the work, then revoking the editor and that worked as expected.

@julesies julesies changed the title User can remove any user's permission in Sharing tab User can remove (repo admin or any user) permission in Sharing tab on work Jul 25, 2018
@chrisdaaz
Copy link

@julesies would removing this row from the "Currently Shared With" table satisfy the issue?

image

i believe admins should always have edit access to all works in the repository. it doesn't seem useful to show the user that admins have edit access to the work they deposited, let alone showing them options to edit or revoke that access.

@julesies
Copy link
Author

julesies commented Jul 25, 2018

yeah agree it seems odd to display admin access on the work view but it does go beyond admins. We have 3 groups from using role mapper, for example and the user can "remove" any user or group who has access granted to them another way. Since currently the only way to remove an individual users listed on the admin set is by visiting each work, I'm thinking you would have to show that to managers, admins, (or what ever group has the right to edit permissions on works).

here is an example: (tab bug reported here #3175)
In this case, the work was granted this access to three groups and one individual from the admin set participants settings (not added to the individual work)

screen shot 2018-07-25 at 1 11 19 pm

@chrisdaaz
Copy link

chrisdaaz commented Jul 25, 2018

could we add this to the issue? @julesies

Done looks like

  • Users cannot override edit access permissions created by the work's admin set
  • The "currently shared with" table hides admin and any groups that exist as participants in the work's admin set

@julesies
Copy link
Author

yes, but add something like only show those groups to admins, managers, editors? and change to groups and individuals from admin sets or collections. (this seems right, what do you think)

@chrisdaaz
Copy link

@julesies okay, i added that stuff to the issue to make it more actionable, but i hope it's not too complicated. take a look and add any clarifications if necessary.

@laritakr laritakr self-assigned this Sep 4, 2018
@laritakr
Copy link
Contributor

laritakr commented Sep 7, 2018

After talking with @julesies my understanding is

  1. show permissions inherited from a collection or admin set only to participants of the collection or admin set
  2. don't show admin at all, since it can't be removed anyway (and we also don't show public or registered access at the work level.)

@no-reply no-reply added this to the 2.x series milestone Sep 8, 2018
@laritakr
Copy link
Contributor

laritakr commented Sep 11, 2018

While working on this issue, it became apparent that this change as requested could be very confusing and misleading to a work's editor.

A work can be in more than one collection, and if so, we wouldn't know which collection affected the permissions, because we don't track that. And permission changes on a collection don't carry through to a work, at least not at this point. So the tie between the collection and the work's permissions is tenuous at best.

Admin set permissions always carry through to the work when it is created. Collection permissions only do if you create a new work from the button off of the collection's show view. Creating a work and adding it to a collection while you create it does not change permissions, nor does adding an existing work to a collection.

So a work could have been created, and the depositor/editor could have given it permissions that they can now not change because it just happens to be the same permissions as the collection had. They also could add or attempt to add a permission which would appear to not be working because that permission is also on the collection or admin set.

I would recommend showing all permissions but limiting the edit and delete access to them, and optionally showing something to identify the collection which is limiting the ability to change those permissions.

So the changes I intend to implement to the work's permissions view are:

  • No one can view admin permissions on the work, because they should always exist and we don't want to allow them to be edited
  • no one can edit a work's permissions which inherit from the manager role of a collection/admin set unless they are also a manager of that collection/admin set. these permissions will show on the work but not be editable, and the collection which causes them to be frozen from change will be identified
  • other permissions may be edited on the work by anyone with edit rights

(note: An editor could still change these rights by removing the work from the collection, removing the rights, and adding it back to the collection, because the inheritance of permissions only happens during the work's creation.)

@julesies Does this sound like what we discussed as the best option?
@vantuyls @chrisdaaz Do you agree with this solution?

laritakr pushed a commit that referenced this issue Sep 13, 2018
#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 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. The
shared partial, `currently_shared.html.erb` 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.
laritakr pushed a commit that referenced this issue Sep 13, 2018
#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 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. The
shared partial, `currently_shared.html.erb` 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.
jeremyf pushed a commit that referenced this issue Sep 3, 2020
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
jeremyf added a commit that referenced this issue Oct 7, 2020
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
@jeremyf
Copy link
Contributor

jeremyf commented Oct 7, 2020

@jlhardes now that you're product owner, I'm wondering about this issue. What are your thoughts? There is now aging work that @laritakr did, and I can help carry this to completion OR close this out.

@jlhardes
Copy link
Contributor

jlhardes commented Oct 8, 2020

@jeremyf It looks like this is still happening and it seems like good changes to make to clarify that Sharing view and prevent that action of removing permission when you can’t actually remove that permission.

How aged is @laritakr's code changes? Is this something that is feasible to include in Hyrax 3.0 release? I don't know that it is worth adding to the list of things for Hyrax 3.0 if it extends the timeline for the release. Would it work better as a Hyrax 3.1 change to incorporate?

@jeremyf
Copy link
Contributor

jeremyf commented Oct 8, 2020

@jlhardes I've rebased the code to the current Hyrax state. It's something that I'm prepared to move forward. I believe that it wouldn't take too much to get the code into 3.0 release.

@laritakr spent considerable time working through the possible scenarios and ensuring proper behavior.

jeremyf pushed a commit that referenced this issue Oct 12, 2020
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
jeremyf added a commit that referenced this issue Oct 12, 2020
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
jeremyf pushed a commit that referenced this issue Jan 25, 2021
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
jeremyf added a commit that referenced this issue Jan 25, 2021
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
@rjkati rjkati self-assigned this Jan 28, 2021
jeremyf pushed a commit that referenced this issue Feb 2, 2021
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
jeremyf added a commit that referenced this issue Feb 2, 2021
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
jeremyf pushed a commit that referenced this issue Feb 3, 2021
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
jeremyf added a commit that referenced this issue Feb 3, 2021
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
jeremyf pushed a commit that referenced this issue Feb 5, 2021
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
jeremyf added a commit that referenced this issue Feb 5, 2021
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
jeremyf pushed a commit that referenced this issue Feb 23, 2021
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
jeremyf added a commit that referenced this issue Feb 23, 2021
Given we are expecting a specific hash structure, favor `fetch` over
`[]` access.  This prevents a null-pointer.

Also, re-arrange the operands for the boolean test.  The
`permission_hash.fetch(:access)` is cheaper/quicker than the other
include test.  This is a small tweak that helps conserve a bit of
computation.

I believe it is important to capture some of @laritakr's intentions.
While this may not be the correct commit to associate, it's close to the
work.  Larita had the [following comment][1]:

> While working on this issue, it became apparent that this change as
> requested could be very confusing and misleading to a work's editor.
>
> A work can be in more than one collection, and if so, we wouldn't know
> which collection affected the permissions, because we don't track
> that. And permission changes on a collection don't carry through to a
> work, at least not at this point. So the tie between the collection and
> the work's permissions is tenuous at best.
>
> Admin set permissions always carry through to the work when it is
> created. Collection permissions only do if you create a new work from
> the button off of the collection's show view. Creating a work and adding
> it to a collection while you create it does not change permissions, nor
> does adding an existing work to a collection.
>
> So a work could have been created, and the depositor/editor could have
> given it permissions that they can now not change because it just
> happens to be the same permissions as the collection had. They also
> could add or attempt to add a permission which would appear to not be
> working because that permission is also on the collection or admin set.
>
> I would recommend showing all permissions but limiting the edit and
> delete access to them, and optionally showing something to identify the
> collection which is limiting the ability to change those permissions.
>
> So the changes I intend to implement to the work's permissions view are:
>
> - No one can view admin permissions on the work, because they should
>   always exist and we don't want to allow them to be edited
> - no one can edit a work's permissions which inherit from the manager
>   role of a collection/admin set unless they are also a manager of that
>   collection/admin set. these permissions will show on the work but not be
>   editable, and the collection which causes them to be frozen from change
>   will be identified
> - other permissions may be edited on the work by anyone with edit rights
>
> (note: An editor could still change these rights by removing the work
> from the collection, removing the rights, and adding it back to the
> collection, because the inheritance of permissions only happens during
> the work's creation.)

[1]:#3166 (comment)
dlpierce added a commit that referenced this issue Mar 1, 2021
…ions_from_update

Lock unauthorized permissions from update
@jlhardes
Copy link
Contributor

jlhardes commented Mar 5, 2021

Tested locally and regular user can no longer remove (or see) admin access and repo-admin user can still see the regular user's work and make edits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment