1792 superuser can search all

[fenekku]

• 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 Super user REST API search doesn't return restricted records/communities invenio-app-rdm#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]

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

[fenekku]

This keeps the same functionality as before while fixing the bug.

It is a little strange that our default is MatchAll() but it has worked so far. I was looking for an no-op Query() but no luck.

[ppanero]

question (making sure I got it). The case that was breaking is when permission is not None, but permission.query_filters = [] then it was returning None instead of [Q()]?

[ntarocco]

It looks like the case, given the tests

[fenekku]

This assumes that the way to set a superuser is via the setup of ActionRole or ActionUser... This is how needs and excludes get it, so the same mechanism is used, but it's surprising that it's not just a check for the superuser_access Need directly. I remember some vague performance considerations, but I don't understand how checking for this one Need isn't performant enough... 🤔

To be candid, permissions are hard and it would be worth revisiting some of our mechanics. See thread starting here: https://discord.com/channels/692989811736182844/724974365451747329/1007312461831094404

[fenekku]

Not pleasant, but it works...

[sharpattack]

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

[ntarocco]

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

[fenekku]

Rebase done.

Yes we want it for v9 too.

[ntarocco]

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

[ntarocco]

@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]

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]

Thank you for your feedback:

1. I will remove it for the docstring, not a big deal.
2. 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.