-
Notifications
You must be signed in to change notification settings - Fork 15
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: access control #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite like the example. The implementation will help refine things over time.
Co-Authored-By: fenekku <fenekku@fenekku.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made comments because we will review it soon with other architects.
Aren't we going in the direction of an evolution of RBAC -> ABAC? (e.g. https://github.com/kolotaev/vakt)
In general I would say that if I am a user reading this document, it is not clear to me what is the change compared to the current implementation. A brief introduction of the current situation, explaining the need of a change, is in my opinion needed.
Another thing is that I understood how I can define permissions, but the part of the search is not very clear to me. I can do a mistake and set needs only for read
and completely forget about the list
(query_filter
).
If I am a developer that needs to develop this, I am not sure I understand what I should do: should I create a new module? Should I change the existing code? Should I keep backward compatibility or not needed? It is not extremely clear how I integrate this in Invenio.
|
||
## Motivation | ||
|
||
- As an administrator, I want to have both public and restricted records, so that not all the records are publicly visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we change the have
with another verb? I am not sure I understand that I want to be possible in my system
or I want to have access
or both :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rephrased 👍
- As an administrator, I want to have a flexible and easy way of defining access control policies, so that I do not need to code them. | ||
- As an administrator, I want to have IP-based access-control, so that I can comply with publisher rules. | ||
- As a user, I want to be able to obtain a shareable link, so that my colleagues can access the files of my restricted record. | ||
- As an administrator, I want to easily define roles like *academic staff* based on my IdP (Identity Provider), so that I can implement business rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I fully understand this one, but it might be that I lack of knowledge in this domain. Can it be a little more explicit or descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ntarocco you mean for IdP based roles? We have the scenario where users will submit to certain communities based on parameters delivered by the IdP (like deanery) as well as the scenario where users are given access to records based on the identity federation (like eduGAIN) they used for signing in.
return Query(owner=user.id) | ||
``` | ||
|
||
As a result, it can be seen that the *query filter* inverts the paradigm with respect to the *needs*/*excludes*. Instead of receiving a record and returning its owners, it takes the id of the user performing the action and returns a query object. This is due to the nature of the *list* (search) operation. The check must not be *Can user A access record R, for each one of the records in the system*. This would not be scalable. Therefore, the check must be *which documents can be accessed by the user A*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would re-phrase the second part This is due to...
to explain it a little bit better.
|
||
Invenio RDM comes with a set of predefined generators: | ||
- Any user: Any user can perform the action. | ||
- Any user if public: Any user can perform the action if the record is public. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How a record is defined public
?
|
||
## How we teach this | ||
|
||
This is a hard to teach concept because it involves a breaking change on the paradigm as from what it is today. However, it makes it much more user friendly and involves less work to implement custom access control rules. The best way to teach this would be to edit the corresponding section in the Invenio Tutorials (assuming the model is adopted by the whole Invenio Framework). In addition, create tutorial in the general RDM documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should maybe elaborate a little bit more this section.
The module documentation itself should have an extensive documentation since this is a complex topic, with many examples.
I agree about a specific tutorial with many use cases and the main Invenio doc.
- Deposit Edit vs Record update? | ||
- Is *query filters* a good name? | ||
- Mention Elasticsearch Query object. Which is a implementation detail. | ||
- Is the example with Team A being blacklisted clear? I was thinking for example of ATLAS vs. CMS not allowing each other to access their research (for mission purposes.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is clear, but I would mention that it is an example! Given the following example use case...
|
||
- Update and Delete actions over files, are iherited from the associated record. Or are per file? | ||
- Deposit Edit vs Record update? | ||
- Is *query filters* a good name? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it might be too specific to searching/ES, while I would go for something closer to access/REST. But I have no ideas right now! :)
As an admin I want to grant read access to the owners of a document, except if the owner is part of team A. | ||
``` | ||
|
||
The following policy applies the `RecordOwner` and `TeamMembership` generators to the record read action. The `RecordOwner` generator, allows the owners of the document (defined right above it, with ids 1, 2 and 3) to access the document. On the other hand, `TeamMembership` blacklists users belonging to Team A. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename TeamMembership
to ExcludeTeam('A')
or ExceptTeam('A')
. This makes reading it easier:
Who can read? record owners except for team A
|
||
### Why is this needed | ||
|
||
The current implementation (November 2019) requires the re-implementation of full permission factories in order to customize access control. Moreover, it is not clear across implementations if the factories should return a set of needs or simply over-write the `can` function. In the latter and most common does not leverage the whole power of *Flask-Principal*. Therefore, a change of paradigm is needed in order to make access control customizations simple and consistent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation (November 2019) requires the re-implementation of full permission factories in order to customize access control. Moreover, it is not clear across implementations if the factories should return a set of needs or simply over-write the `can` function. In the latter and most common does not leverage the whole power of *Flask-Principal*. Therefore, a change of paradigm is needed in order to make access control customizations simple and consistent. | |
The current implementation (November 2019) requires the re-implementation of full permission factories in order to customize access control. Moreover, it is not clear across implementations if the factories should return a set of needs or simply over-write the `can` function. The latter (and most common case) does not leverage the whole power of *Flask-Principal*. Therefore, a change of paradigm is needed in order to make access control customizations simple and consistent. |
No description provided.