1792 superuser can search all

inveniosoftware / invenio-records-permissions

Permissions for Invenio's records REST API.

https://invenio-records-permissions.readthedocs.io

MIT License

0 stars 25 forks source link

1792 superuser can search all #75

Closed fenekku closed 1 year ago

fenekku commented 2 years ago

permission: inject superuser-access logic in query_filter [+] superuser-access logic was already injected in Permission.needs/excludes, but was missing from query_filters. This caused searches by superusers to not return all records, even though all records can be seen by superusers. closes https://github.com/inveniosoftware/invenio-app-rdm/issues/1792
permission_filter: fix empty query_filters bug, test and refactor There was a problem (a form of Liskov violation) with permission_filter where None would be returned if query_filters returned []. This would break the search. Now, that issue is solved, permission_filter is tested, and refactored a little.

A better refactoring IMO would place the permission_filter code in Permission.query_filters directly and remove the need for search to call this dangling function. If you agree I can create a ticket for this.

fenekku commented 2 years ago

Dropping that in the back office project board given @ntarocco is saying you are having a hard time tracking open pull requests https://discord.com/channels/692989811736182844/704625518552547329/1009003421023617074

sharpattack commented 1 year ago

Hi, can I get a status update on this PR? Our librarians (admins) can't effectively review restricted records without it.

ntarocco commented 1 year ago

@fenekku is this needed for RDM v9? Could you please rebase it?

fenekku commented 1 year ago

Rebase done.

Yes we want it for v9 too.

ntarocco commented 1 year ago

@fenekku can you please review my last commit in this PR? Then we can merge and cherry-pick to backport.

ntarocco commented 1 year ago

@fenekku I am merging this so we can continue with OpenSearch v2 and I am preparing the new commit to backport. Feel free to add any comment, we can add an extra commit later, or contact me directly.

fenekku commented 1 year ago

For this PR per se:

The removal of SuperUser should have been accompanied by its removal in the docstring of __init__.py https://github.com/inveniosoftware/invenio-records-permissions/blob/master/invenio_records_permissions/__init__.py#L125
The name superusers is very confusing. Role is a singular, superusers is a plural. Role is the name of a group, superusers is the name of its members. That naming convention conflates the 2. Just superuser would have been much clearer given we are talking about the role here.

For the rest, I will contact you directly.

ntarocco commented 1 year ago

Thank you for your feedback:

I will remove it for the docstring, not a big deal.
it has been done on purpose. There is a lot of confusion on roles and actions. The role has an extra s to highlight that is different from the action or anything. A role called superusers makes much more sense.