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

Lock unauthorized permissions from update #3479

Closed

Conversation

laritakr
Copy link
Contributor

Fixes #3166

Allowed permission maintenance:

  • 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.

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.

@samvera/hyrax-code-reviewers

@laritakr laritakr changed the title Lock unauthorized permissions from update [WIP] Lock unauthorized permissions from update Jan 24, 2019
@stale stale bot added the stale label Feb 23, 2019
@laritakr laritakr force-pushed the #3166-lock_unauthorized_permissions_from_update branch from 399381d to e46a946 Compare February 26, 2019 21:48
@stale stale bot removed the stale label Feb 26, 2019
@laritakr laritakr force-pushed the #3166-lock_unauthorized_permissions_from_update branch 5 times, most recently from 5da223f to 72647a3 Compare February 27, 2019 20:34
@samvera samvera deleted a comment from stale bot Feb 28, 2019
@laritakr laritakr force-pushed the #3166-lock_unauthorized_permissions_from_update branch 3 times, most recently from 2d013ee to 523466b Compare March 1, 2019 17:29
@laritakr laritakr force-pushed the #3166-lock_unauthorized_permissions_from_update branch from ae81bf8 to 6f5b246 Compare March 26, 2019 15:41
@laritakr laritakr changed the title [WIP] Lock unauthorized permissions from update Lock unauthorized permissions from update Mar 26, 2019
@laritakr laritakr force-pushed the #3166-lock_unauthorized_permissions_from_update branch from 0a6e770 to 4d4d9c4 Compare March 26, 2019 19:04
@laritakr laritakr requested a review from no-reply March 26, 2019 19:23
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
@laritakr laritakr force-pushed the #3166-lock_unauthorized_permissions_from_update branch from 4d4d9c4 to b5e2c70 Compare April 2, 2019 19:33
@laritakr laritakr removed the request for review from no-reply April 8, 2019 16:01
@elrayle elrayle self-requested a review April 17, 2019 16:41
@stale
Copy link

stale bot commented May 17, 2019

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

@stale stale bot added the stale label May 17, 2019
@stale stale bot closed this May 31, 2019
@cjcolvar
Copy link
Member

@laritakr Is this ready for review?

@cjcolvar cjcolvar reopened this Jul 10, 2019
@stale stale bot removed the stale label Jul 10, 2019
@laritakr
Copy link
Contributor Author

It was complete, but there was some question whether the behavior I put in was actually wanted after all. I assumed the PR was dead.

Before merging this, be sure to discuss it with @no-reply

@laritakr laritakr removed the request for review from elrayle July 10, 2019 16:43
@cjcolvar
Copy link
Member

Thanks for the background. I know you spent a lot of effort on this and no one had reviewed it before stale-bot closed it so I brought it up during the weekly PR review time and we couldn't remember if it was considered done or not. I'll try to get a review in soon but I'll talk with @no-reply about timeline if it should be a part of 3.0 or post-3.0.

@laritakr
Copy link
Contributor Author

I think the problem is that there wasn't consensus whether the change in behavior is desired. The assigning of permissions via admin sets and collections does not apply permissions beyond initial creation of the collection, and there isn't consensus on whether this should be changed.

It is more than just an issue of timing, so I gave up on trying to get it reviewed.

@cjcolvar cjcolvar requested a review from no-reply July 24, 2019 16:22
@stale
Copy link

stale bot commented Aug 23, 2019

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

@stale stale bot added the stale label Aug 23, 2019
@no-reply
Copy link
Contributor

no-reply commented Sep 4, 2019

bumping this since it's assigned to me.

@stale stale bot removed the stale label Sep 4, 2019
@stale
Copy link

stale bot commented Oct 4, 2019

This issue has been automatically marked as stale because it has not had activity for 30 days. It will be closed if no further activity occurs within 14 days. Thank you for your contributions.

@stale stale bot added the stale label Oct 4, 2019
@stale stale bot closed this Oct 18, 2019
@jeremyf jeremyf deleted the #3166-lock_unauthorized_permissions_from_update branch January 22, 2020 21:11
@jeremyf jeremyf restored the #3166-lock_unauthorized_permissions_from_update branch September 3, 2020 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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