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

Initial contract for the authorizations endpoints #92

Merged
merged 9 commits into from Dec 5, 2019

Conversation

@abollini
Copy link
Member

abollini commented Nov 14, 2019

Contract for an authorizations endpoint to provide access to the org.dspace.app.util.AuthorizeUtil methods and other shortcut useful to the UI to quickly verify if a specific feature/button/action should be presented to the user.

See also https://wiki.duraspace.org/display/DSPACE/REST+Authorization

Copy link
Member

tdonohue left a comment

@abollini : Gave this an early review today. It's honestly looking good overall, but some questions inline below about the term scope and how it is being used here. Just a reminder that we also use that term heavily in search endpoints as more of a way to filter a search to a community or collection --- but here, scope seems to represent an individual object, rather than a search filter? We might want to consider a different term, as it confused me initially.

authorizations.md Outdated Show resolved Hide resolved
authorizations.md Outdated Show resolved Hide resolved
authorizations.md Outdated Show resolved Hide resolved
authorizations.md Outdated Show resolved Hide resolved
authorizations.md Outdated Show resolved Hide resolved
Copy link
Member

benbosman left a comment

Thanks @abollini for the contract. I've performed an initial review and included some small feedback inline.
I will review it in more detail once the feedback from Tim and me is addressed.

authorizations.md Outdated Show resolved Hide resolved
authorizations.md Outdated Show resolved Hide resolved
The supported parameters are:
* page, size [see pagination](README.md#Pagination)
* uri: mandatory, the scope to use for the authorization check. The full URI of the rest resource must be specified, i.e. https://{dspace.url}/api/core/community/{uuid}
* eperson: optional, the uuid of the eperson to evaluate for authorization. If not specified authorization of anonymous users will be returned

This comment has been minimized.

Copy link
@benbosman

benbosman Nov 19, 2019

Member

Why is this required. I would expect the current user to be used for verifying the user's authorizations

This comment has been minimized.

Copy link
@abollini

abollini Nov 21, 2019

Author Member

this would allow an administrator to inspect witch permissions has a different user. If we found difficult to implement we can always reduce the first version to "only the current user" but, also if we do that, keeping this as parameter will allow us to evolve without changing the contract.
Anyway, we can and should revisit that after some concrete experiment in the implementation

features.md Outdated Show resolved Hide resolved
@abollini

This comment has been minimized.

Copy link
Member Author

abollini commented Nov 21, 2019

@tdonohue @benbosman thanks for your feedback, can you take another look? I hope to have addressed all of them

@paulo-graca

This comment has been minimized.

Copy link
Contributor

paulo-graca commented Nov 21, 2019

Reading the RestContract I'm getting the feeling that we can change what whatever it was defined in terms of authorization, like the the usage of terms like "provides". I didn't got the idea that was possible from the rest side on our last meeting. I'm still reviewing it and I will provide more comments as soon as possible. Perhaps the text should be more clear about this aspect. Is this Read only or can we change the defined authorizations from the rest side?

@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Nov 21, 2019

Per @paulo-graca 's note above...just summarizing what we discussed in today's DSpace 7 meeting. We agreed that some of the terminology in the Contract PR may need improving, as it is defining READ-ONLY endpoints. Paulo noted he'd try to call out the specific areas of confusion so that we can clean up the text.

I gave this another quick review today. It's honestly looking good to me (no further comments at this time), but I want to see updates to this PR to clarify that all endpoints are read-only (per Paulo's suggestion).

Copy link
Contributor

paulo-graca left a comment

Sorry for delaying my comment!

authorizations.md Outdated Show resolved Hide resolved
authorizations.md Outdated Show resolved Hide resolved
authorizations.md Outdated Show resolved Hide resolved
Copy link
Contributor

paulo-graca left a comment

Thank you again @abollini for this PR, as I said via slack, this PR seems very thoroughly thought but I'm still having some difficulty in envision what it will be the outcome in some parts, so I post some questions in my comments bellow.

"id": "withdrawItem",
"description": "The feature allows to withdrawn an item from the repository without deleting it. The restoreItem feature allow to undo the process",
"resourcetypes": [
"item"

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Nov 27, 2019

Contributor

I think having features locked to resource types might be limiting. I understand that we need to have somehow controlled values for them, but I may have a feature that doesn't relate with any of them, for instance "edit news" or "edit registry" are specific features for the ADMIN user and I think they doesn't apply for any of these resource types. Or perhaps I'm not seeing this properly.

This comment has been minimized.

Copy link
@abollini

abollini Nov 28, 2019

Author Member

hi @paulo-graca ,
for such cases I expect that we would have a "administer" authorization or more fine-grains editNews, editRegistry that will apply to the "site" resourcetype

This comment has been minimized.

Copy link
@mwoodiupui

mwoodiupui Dec 2, 2019

Member

Separately from this PR we need to hold a discussion of the wider application of access control and of finer-grained control.

This comment has been minimized.

Copy link
@tdonohue

tdonohue Dec 2, 2019

Member

I'd recommend we start with something simple for now. Tying a "feature" to a resource type seems OK for now -- as @abollini notes the Site resource type can be used for global features that apply at the site level.

In the future (v8 or later) we may wish to revisit this (as @mwoodiupui notes) as we may want to eventually adopt an enhanced authorization / access control system. However, that is out of scope for v7.


```json
{
"id": [eperson-uuid_]feature-id_object-uuid,

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Nov 27, 2019

Contributor

Will it have any semantic associated with? Meaning should I parse the ID to get each part? I'm in favor of not having semantics here.

This comment has been minimized.

Copy link
@abollini

abollini Nov 28, 2019

Author Member

The client doesn't really need to guess the authorization ID using the above algorithm. Search methods are provided to allow the client to check authorization depending on the object and the requested feature. Anyway, I have preferred to make the ID "syntax" explicit as we don't want to introduce new tables/persistence mechanism for the authorization but instead, as said at the top of the contract and in the wiki authorization are the result of processing resourcepolicies, configurations and business rules

This comment has been minimized.

Copy link
@tdonohue

tdonohue Dec 2, 2019

Member

I'm OK with having the id of an authorization be a combined set of UUIDs. I agree that the only way to more easily "assign" an ID here would be to store this info in a new database table or similar (which I'd rather not do for now -- that could always come later in v8 if we want to enhance the authorization system).

We should ensure the client doesn't need to parse/split this id into its separate UUIDs...to the client this should just be another identifier (which just happens to consist of two other identifiers appended together)

This comment has been minimized.

Copy link
@benbosman

benbosman Dec 5, 2019

Member

I can see a conflict between #92 (comment) and this id.

If you have a need for supporting integer IDs, they will not be unique. You can have two authorizations related to a different type of object, with the same id using this syntax. I think to be unique, the type will need to be included in the id as well

This comment has been minimized.

Copy link
@abollini

abollini Dec 5, 2019

Author Member

you are right @benbosman I will add the object type in the ID. It is not strictly needed when the object id is an uuid but it will help in all the case to speedup the lookup

Please note that these endpoints are all READ-ONLY, authorizations are granted and withdrawn as result of business processes, configurations and resource policies.

## Main Endpoint
**/api/authz/authorizations**

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Nov 27, 2019

Contributor

I'm not sure exactly what this endpoint might retrieve. I think it would be useful to have an example here.
Also, if this retrieves a list of "my" authorizations, should I assume a nonexistent authorization is a DENIED one?

This comment has been minimized.

Copy link
@abollini

abollini Nov 28, 2019

Author Member

well nothing :)
the /api/authz/authorizations is not implemented and will return a 405 code as stated in the contract.

Keep in mind the general approach that we have adopted for our REST api. Each endpoints represent a collection of resources. The GET method over the collection should return, where it makes sense, all the resources in the collection. Don't be confused by the term collection here, it is referred to the REST side, see
https://github.com/DSpace/Rest7Contract/blob/master/README.md#use-of-the-http-verbs-and-http-response-code

About the "denied" authorizations: yes, we keep the general dspace principle here. If you don't have an explicit authorization then it is denied. So, assuming that you want to know if an user is authorized or not to withdraw an Item you can use
/api/authz/authorizations/search/object?uri=https://dspace.url/api/core/items/<item-uuid>&eperson=<eperson-uuid>&feature=withdrawItem
if you get a not empty answer, i.e. you have a matching authorization, the user is allowed otherwise he is denied

This comment has been minimized.

Copy link
@tdonohue

tdonohue Dec 2, 2019

Member

Throwing a 405 Method Not Allowed to all requests on this endpoint seems appropriate for now.

@abollini

This comment has been minimized.

Copy link
Member Author

abollini commented Nov 28, 2019

Hi @tdonohue @benbosman @paulo-graca,
thanks for your reviews.
The PR should be in a good status now, your feedback should have been addressed

Copy link
Member

tdonohue left a comment

👍 This looks like a good start to me. If we discover issues or enhancements during implementation this contract can always be updated or enhanced in future PRs.

Copy link
Contributor

paulo-graca left a comment

This PR it's also ok to me. Thank you @abollini for all the enlightenment!
Just sharing the information about special groups for others that have the same doubt:

Hi Paulo, it should work as the idea is that the authorizations endpoint will reuse the existing logic from the AuthorizeUtil class. This mean that special groups will be take in consideration. Keep in mind that the authorizations are not persisted on the database, there is no plan to make database queries to discover them. It is just a bunch of java code that will be put in a named spring bean and it the java code will return true the eperson has such authorization otherwise the authorization objects doesn't exist

@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Dec 4, 2019

@abollini : This has a minor code conflict. Could you resolve so this can be merged?

@benbosman : It looks to me like your prior suggestions were all taken into account. If you have the chance, could you give this a quick look again? It's currently at +2, so I think it's ready to merge.

@benbosman

This comment has been minimized.

Copy link
Member

benbosman commented Dec 5, 2019

I can see one more problem: #92 (comment)
This can create authorizations with overlapping ids, implying Angular won't be aware they're different, and REST can't respond to the GET based on the ID

@abollini

This comment has been minimized.

Copy link
Member Author

abollini commented Dec 5, 2019

@benbosman added the note about the object type and solved the conflict. It should be ready to merge now. Thanks!

Copy link
Member

benbosman left a comment

Thanks @abollini, this solves the problem with the IDs

@tdonohue tdonohue merged commit 5dc518a into DSpace:master Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.