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

rdm: define additional record permissions schema and logic #12

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

fenekku
Copy link
Contributor

@fenekku fenekku commented Oct 24, 2019

This is an extension to RFC #7 . To be verified by implementation work (inveniosoftware/invenio-records-permissions#16).

@fenekku fenekku force-pushed the permissions_metadata branch from 9089664 to d4cf887 Compare November 5, 2019 16:31
@fenekku fenekku changed the title rdm: define record permissions schema and logic rdm: define additional record permissions schema and logic Nov 5, 2019
Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be very careful (we implemented the same in ILS) and I would like to have a quick brainstorming around this (we will discuss with the other architects), because as you said we are introducing a mix of implicit/explicit permission.

Another small drawback is that when adding permissions in the record metadata, they are indexed in Elasticsearch and we are having some issues with that.
E.g. I have a record with:

permissions:
  can_read: [{
    2, 'person'
  }]
  ...
  all other fields of the record, the word `person` is not appearing in any field
  ...

And then the user searches for the text person, the record above will appear in the results, but the there is no record field having the text person.

To be discussed...

@ppanero
Copy link
Member

ppanero commented Nov 18, 2019

@ntarocco I had the same issue in Citadel Search (rebrand of CERN Search). At the moment (8 months ago) there was no way of removing a field form the search in ES (v6.2), maybe now there is one.

The way I solved it was to replicate the behaviour of _all field (which was deprecated in ES, maybe we should also check why). So now in my schemas I have something like:

_access: explicit permissions here
_ data: all fields with content to be searchable

Afterwards by using the ES python DSL, you can specify the field in which the query will be performed (https://github.com/elastic/elasticsearch-dsl-py/blob/master/elasticsearch_dsl/query.py#L12 through params to https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-query-string-query.html see "default_field").

However, I recall issues with booleans and dates, because they were not strings so I had to move them out of this default fields.

From my perspective the tasks would be:

  • check is fields can be excluded from search queries
  • check why _all was deprecated
  • implement something like the _data metioned above.
    • Evaluate copy_to vs having a field containing everything e.g. _data (copy_to this would have implications when doing queries to specific fields, which leads to the next point)
    • Do we need a query parser?

@ntarocco ntarocco added the RDM Invenio RDM label Apr 22, 2020
@lnielsen lnielsen merged commit 3803751 into inveniosoftware:master Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RDM Invenio RDM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants