Initial implementation of the authorizations endpoints #2663

abollini · 2020-02-09T15:14:41Z

This is the implementation of the authorization features and authorization endpoints according to the REST Contract in
https://github.com/DSpace/Rest7Contract/blob/master/features.md
https://github.com/DSpace/Rest7Contract/blob/master/authorizations.md

DONE:

general infrastructure to support pluggable AuthorizationFeature
Sample Features withdrawn, reinstate, cclicense
complete features endpoint and integration tests
single authorization endpoint and integration tests
merge with changes from the master (the switch from int to string for the type create logical conflict)
implement the search method in the authorization endpoint and related ITs
add ITs for the first sample features
Contract update PR: DS-4413 split the object search method depending on the return type RestContract#103

abollini · 2020-02-09T15:34:22Z

The reason for the change done in IndexableObject is unclear to me
https://github.com/DSpace/DSpace/blame/31b960d4e109b1220c14563137e3473c80513b0f/dspace-api/src/main/java/org/dspace/discovery/IndexableObject.java#L30

it seems to duplicate the behaviour assigned to the getTextType() method
https://github.com/DSpace/DSpace/blame/31b960d4e109b1220c14563137e3473c80513b0f/dspace-api/src/main/java/org/dspace/discovery/IndexableObject.java#L62

but in the same refactoring the default implementation was dropped
4c9a30b#diff-cfc1cee1748a5c0e0deb076482e6d69bL43
and moved in the single IndexableXXXX interface making it harder to maintain and easy to put out of sync (for instance here 4c9a30b#diff-632df1a30eeb34813b0048edba1e7db0R48 it is now hardcoded instead than be retrieved from the Constants class).

@KevinVdV can you summarize the reason for this change to me or link me to the relevant previous conversations? why we need to introduce a new way to name our classes other than the Constants that were already in place in Constants.typeText? does it make sense to reintroduce a getIntType() method?

KevinVdV

@abollini The changes to the indexableObject were documented on the following places:

getTextType() method

The getTypeText() does indeed reuse the Constants.typeText[integer] for DSpace Objects, it doesn't do this for non indexable objects "IndexableClaimedTask", "IndexableWorkflowItem", ... as these aren't DSpaceObjects & the Constants.typeText array was limited to DSpaceObjects up until DSpace 6 & I think we should keep it like this.

FindableObject

I also took a quick look at your code & I see no need for the FindableObject interface, every instance of the "FindableObject" can be removed & replaced with DSpaceObject in the code. I did notice that the AuthorizationRestUtil returns non DSpace objects like WorkspaceItem & WorkflowItem but these objects aren't supported using the "AuthorizeUtil" method, so their inclusion here is unneeded.

I'm in favor of a more extensive system that leaves room for other objects to be supported but then we need to move away from the Constants integers & move more towards using the actual class names as types instead of integers. We don't want to add integers for example of we just want to add support for the MetadataSchema, MetadataField, ... objects.

abollini · 2020-02-14T16:41:40Z

thanks @KevinVdV for point me to the previous discussions. I don't completely agree with the change done, especially with the use of the SimpleName to identify the object but ok, that's it and we can always discuss that again later if needed and time allowed.
The authorizations endpoint was already planned to deal with any sort of objects, this is already stated in the contract and I'm still convinced that we need it.
Unfortunately the AuthorizeUtil only implement a limited number of "features" check, most of the checks where done in the Servlet/Controller in the previous version with duplication between JSPUI and XMLUI, in any case we will need now for everything. Is the user authorized to delete a workspaceitem? is the user authorized to edit the workspaceitem? we need this information on the angular side to show or not a button without hardcoding in the client the raw logic around configurations and resource policies. Make sure that the authorization endpoint works with all type of objects (and different primary key types) has requested lot of (written) tests so I really prefer to save that instead than have to recover them later.

So I'm working on solving the conflict retaining the FindableObject but removing the need to add additional ID to the Constants class and instead using the class name aka the new getType()

…license feature

…efactoring

benbosman

I did expect the structure of the implementation to be different based on what was previously discussed.
One of the things which was discussed was to make sure it's a spring configuration of the list of "features" which can be offered. I noticed it is now completely autowired, making it harder to e.g. disable a feature
I would also have expected that type of Rest Object would be determined using a Generic type at the interface level, and e.g. CCLicenseFeature would use ItemRest public class CCLicenseFeature implements AuthorizationFeature<ItemRest>. This would allow the supported type to be identified automatically and would avoid the instanceof and casting in each implementation

abollini · 2020-02-19T18:45:34Z

hi @benbosman thanks for your initial feedback. I don't have an use case in mind right now that require to completele remove "core" feature maybe what is needed is that some feature will return always false if a dspace.cfg property is set (for instance if an institution doesn't want to use the request a copy). Anyway, if we want to allow an autowired component to be disabled the SpringBoot Conditional annotation should help us
see for instance this article https://reflectoring.io/spring-boot-conditionals/

about the GenericType suggestion the current contract says that a feature can support multiple object types so it is not necessary tied to a single object but we can of course revisit this decision if needed (here or in a future PR)

tdonohue

@abollini : I've given this an initial code review today. Some inline comments below. I don't think anything here is major, but a lot of minor suggestions / requests. I haven't had a chance to test this yet.

As a sidenote regarding @benbosman's comments about configuring which "features" that can be offered...I agree with the approach taken in this PR. Since "features" only exist in the REST API layer, they should be configured via Spring Annotations (and as necessary @Conditional or @ConditionalOnProperty annotations). For example, we already have code that uses @ConditionalOnProperty annotations to only enable OAI-PMH, RDF & SWORD when corresponding configs are set to true. The main reason I prefer this approach as it's the best practice for Spring Boot webapps. So, my preference is to keep Spring XML based configurations limited to dspace-api as much as possible, and keep dspace-server-webapp aligned with Spring Boot best practices. (That said, I'm noticing we have had a few Spring XMLs added to the dspace-server-webapp without my realizing it. But, I'd rather avoid that in the future, and in fact look to remove the XMLs that exist.)

dspace-api/src/main/java/org/dspace/authorize/AuthorizeConfiguration.java

dspace-server-webapp/src/main/java/org/dspace/app/rest/authorization/Authorization.java

dspace-server-webapp/src/main/java/org/dspace/app/rest/authorization/AuthorizationFeature.java

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

dspace-api/src/main/java/org/dspace/discovery/IndexableObject.java

…load

…avadoc

tdonohue · 2020-02-24T17:31:00Z

@abollini : Thanks for the quick updates. I only have one minor code review feedback remaining here #2663 (comment)

I'll also run some tests on this to make sure it's working for me, but the code looks good!

UPDATE: After testing, I've found a few minor issues relating to how the implementation works when compared to the contract.

When accessing /api/authz/features/cclicense (or any feature), the resourcetypes array returned says core.item, when it's documented to say just item. When doing a search across features, I also have to specify core.item instead of item. For example: /api/authz/features/search/resourcetype?type=core.item
When accessing /api/authz/features/cclicense (or any feature), the description is always empty. I expected it to have a description. However, I see in the implementation that we never override the default description() method.

The /api/authz/authorizations endpoints all appear to work as described in the contract, and based on the updated descriptions in DSpace/RestContract#103

benbosman

I have performed an initial review of the code excluding the ITs. I also still have to test the functionality.

I included some feedback inline, but most things are most likely small changes.

Regarding @tdonohue 's feedback: I do prefer the syntax core.item since it better guarantees uniqueness (although I don't think the same name is used in 2 categories so far), perhaps the contract should be updated?

dspace-api/src/test/java/org/dspace/AbstractIntegrationTest.java

dspace-server-webapp/src/main/java/org/dspace/app/rest/authorization/Authorization.java

dspace-server-webapp/src/main/java/org/dspace/app/rest/authorization/AuthorizationFeature.java

dspace-server-webapp/src/main/java/org/dspace/app/rest/authorization/impl/CCLicenseFeature.java

...-server-webapp/src/main/java/org/dspace/app/rest/repository/AuthorizationRestRepository.java

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/FindableObjectRepository.java

...p/src/main/java/org/dspace/app/rest/security/ReadAuthorizationPermissionEvaluatorPlugin.java

...-server-webapp/src/main/java/org/dspace/app/rest/repository/AuthorizationRestRepository.java

benbosman

While reviewing the ITs, I noticed e.g. AuthorizationFeatureRestRepositoryIT assumes the AuthorizationFeatureService works correctly, but there are no JUnit tests for AuthorizationFeatureService despite the fact that it's a new Service

I've included some inline feedback on the ITs as well.

Next I will test the functionality, but since I did report quite some feedback, I'll wait until that's resolved

...ce-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationFeatureRestRepositoryIT.java

benbosman · 2020-02-27T09:18:31Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

+                    Matchers.anyOf(
+                            JsonPathMatchers.hasJsonPath("$.type", is("authorization")),
+                            JsonPathMatchers.hasJsonPath("$._embedded.feature",
+                                    Matchers.not(Matchers.anyOf(


This tests which features are not embedded, but doesn't verify which features are embedded. The same applies for tests below
Also, I don't expect anything to be embedded since no projection is requested

the reason why projection was not requested is because it was introduced after than these test were written. I have updated them now.
I cannot test that the expected features are included because we don't know all of them in advance. We will add more and more features soon and I prefer to avoid the need to update such test for each new feature. Instead, it would be sufficient to verify that what is returned match the request.

I noticed the $._embedded.feature is currently still not verifying which data is present (only which data is not present). The test would be more reliable if it can verify the expected features are present

@abollini where has this been fixed?

sorry, you are right the check were still missing. Added here 59b7716

Thanks for the updates, that's really helpful.
I think you missed one though:

DSpace/dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

Line 536 in 59b7716

JsonPathMatchers.hasJsonPath("$._embedded.feature",

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

...ce-server-webapp/src/test/java/org/dspace/app/rest/authorization/CCLicenseFeatureRestIT.java

KevinVdV

Made a small code review & found some very easy to fix things.

KevinVdV · 2020-03-06T13:33:42Z

dspace-server-webapp/src/main/java/org/dspace/app/rest/authorization/Authorization.java

+/**
+ * An authorization is the right for a specific {@link EPerson}, eventually null to indicate unauthenticated users, to
+ * use a specific {@link AuthorizationFeature} on a defined object. The target object must implement the
+ * {@link IndexableObject} interface so to have an unique ID and type


The IndexableObject is mentioned here, but it isn't used in this class, I assume it should be the "BaseObjectRest" ?

fixed thanks

KevinVdV · 2020-03-06T13:34:06Z

dspace-server-webapp/src/main/java/org/dspace/app/rest/authorization/impl/ReinstateFeature.java

+            return false;
+        }
+        Item item = (Item) utils.getDSpaceAPIObjectFromRest(context, object);
+        if (!item.isWithdrawn()) {


What if the item is Null ?

the AuthorizeFeatureService protects against NPE, it is stated in the interface contract (javadoc). I have added a test in the service test to demonstrate that as well see commit 4192b09

KevinVdV · 2020-03-06T13:34:26Z

...-server-webapp/src/main/java/org/dspace/app/rest/repository/AuthorizationRestRepository.java

+        try {
+            featureName = authorizationRestUtil.getFeatureName(id);
+        } catch (IllegalArgumentException e) {
+            log.warn(e.getMessage(), e);


Perhaps add a small message here for the log ?

it is provided by the getFeatureName inside the IllegalArgumentException. For such reason the exception is stated in the method javadoc despite to be a runtime exception

KevinVdV · 2020-03-06T13:35:18Z

...-server-webapp/src/main/java/org/dspace/app/rest/repository/AuthorizationRestRepository.java

+     * user. Only administrators and the user identified by the epersonUuid parameter can access this method
+     *
+     * 
+     * @param context


This param isn't used in this method.

fixed thanks

KevinVdV · 2020-03-06T13:35:25Z

...-server-webapp/src/main/java/org/dspace/app/rest/repository/AuthorizationRestRepository.java

+     *            the uri of the object to check the authorization against
+     * @param epersonUuid
+     *            the eperson uuid to use in the authorization evaluation
+     * @param featureName


This param isn't used in this method.

fixed thanks

KevinVdV · 2020-03-06T13:35:42Z

...-server-webapp/src/main/java/org/dspace/app/rest/repository/AuthorizationRestRepository.java

+     * anonymous user. Only administrators and the user identified by the epersonUuid parameter can access this method
+     *
+     * 
+     * @param context


This param isn't used in this method.

fixed thanks

KevinVdV · 2020-03-06T13:36:18Z

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/FindableObjectRepository.java

+ * 
+ * @author Andrea Bollini (andrea.bollini at 4science.it)
+ *
+ * @param <F> the ReloadableEntity type


This , should be

This is now the ReloadableEntityObjectRepository, I guess you noted that F was used instead than T, now fixed

KevinVdV · 2020-03-06T13:36:50Z

...p/src/main/java/org/dspace/app/rest/security/ReadAuthorizationPermissionEvaluatorPlugin.java

+                }
+            }
+        } catch (SQLException e) {
+            log.error(e.getMessage(), e);


Perhaps add a small message here for the log ?

I don't know exactly what useful information can be added, all the other PermissionEvalutorPlugin do the same. This don't mean that we cannot improve the logging here but I really like to get suggestion :)

tdonohue · 2020-03-09T20:36:38Z

@abollini : it looks like Travis CI is throwing a compilation error on this PR where the same method exists twice. Here's the error:

[ERROR] /home/travis/build/DSpace/DSpace/dspace-server-webapp/src/test/java/org/dspace/app/rest/builder/CommunityBuilder.java:[104,29] method withAdminGroup(org.dspace.eperson.EPerson...) is already defined in class org.dspace.app.rest.builder.CommunityBuilder

abollini · 2020-03-10T20:58:40Z

hi @tdonohue yes the compilation error was trivial, the oddity here is that it was due to a wrong automatic merge of git that Travis due to build the PR. Indeed, the branch itself have had not failure. Anyway, I have now pulled from the master and solved the error manually

dspace-api/src/main/java/org/dspace/core/Context.java

benbosman · 2020-03-11T13:49:04Z

@abollini thanks for the updates, most of my feedback was resolved. I've updated a few things but marked most as resolved.

I didn't see the JUnit tests yet, and there's some feedback from Kevin and Tim including the [category].[type] related feedback.
Once that's processed, I will also test the branch again

abollini · 2020-03-11T17:34:46Z

hi @benbosman thanks for reviewing that, I will check in more details tomorrow. The test for the new service has been introduced here with this commit: 616323a

benbosman · 2020-03-12T10:02:45Z

Thanks @abollini I missed that class but it looks good

abollini · 2020-03-17T08:17:28Z

@benbosman @tdonohue @KevinVdV all the feedback above have been processed as far as I know, the only missing one could be #2663 (comment) but I really don't know how to process it

benbosman · 2020-03-17T12:36:00Z

@abollini I can see only one comment from me for which I can't see the code: #2663 (comment)
And did you create the JIRA ticket for #2663 (comment) already?

I've tested this branch locally, and didn't see any problems, I think this is almost ready now (but I haven't reviewed whether the feedback from Tim or Kevin has been resolved, only what I had discovered)

tdonohue

Code changes/updates look good to me, and I see DS-4460 has been created to ensure we fix the duplication between RestAddressableModel.getUniqueType() and RestModel.getType() in a future PR (as this will be necessary for consistency in the Angular UI).

However, after manually tested this, I'm having difficulty getting any results to return from the /api/authz/authorizations/search/object endpoint. Previously, I thought this was working correctly. But, now if I authenticate as an Administrator, and use an Item URI, I'm getting zero results for every Item URI that I try. Here's an example request: http://localhost:8080/server/api/authz/authorizations/search/object?uri=http://localhost:8080/server/api/core/items/d1fb08bd-a514-492c-83c2-09eae0bec044

It's always possible there is something in my test environment causing this, but I'd recommend retesting this endpoint to ensure it is returning results as expected.

abollini · 2020-03-20T18:08:48Z

@tdonohue everything have been addressed now, Travis has had a temporary hangoff now it should work. The reason why you don't see result in your search is because your are searching for the anonymous user as no eperson param is specified in your request. It should look like that
http://localhost:8080/server/api/authz/authorizations/search/object?uri=http://localhost:8080/server/api/core/items/d1fb08bd-a514-492c-83c2-09eae0bec044&eperson=335647b6-8a52-4ecb-a8c1-7ebabb199bda

tdonohue

👍 @abollini : thanks for the updates & note on my prior testing. You are correct, I was not including the "eperson" param in my tests. After doing so, it all works great.

This looks good to me now. However, it is failing Travis CI builds cause of minor Checkstyle issues (related to the Checkstyle update it looks like). Once the Checkstyle issues are fixed & this passes Travis CI, then it looks good to me.

benbosman

Thanks @abollini , that solves my open feedback

Initial implementation of the authorizations endpoints

0201ef1

abollini added interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) merge conflict PR has a merge conflict that needs resolution work in progress PR is still being worked on & is not currently ready for review labels Feb 9, 2020

KevinVdV requested changes Feb 12, 2020

View reviewed changes

abollini added 4 commits February 16, 2020 00:38

DS-4413 complete implementation of the authz infrastructure and ITs

214d7df

DS-4413 fix checkstyle and add ITs for the withdraw, reinstate and cc…

4f20954

…license feature

Merge branch 'master' of https://github.com/DSpace/DSpace into authz

9a7f9e7

DS-4413 fix checkstyle and ITs after projection and IndexableObject r…

3524f10

…efactoring

abollini mentioned this pull request Feb 17, 2020

DS-4413 split the object search method depending on the return type DSpace/RestContract#103

Merged

abollini removed merge conflict PR has a merge conflict that needs resolution work in progress PR is still being worked on & is not currently ready for review labels Feb 17, 2020

DS-4413 exclude uniqueType from the json output

1d64d89

benbosman reviewed Feb 18, 2020

View reviewed changes

tdonohue reviewed Feb 21, 2020

View reviewed changes

abollini added 7 commits February 22, 2020 15:39

Add IT to proof that the AuthorizeConfiguration support the config re…

de8e3ec

…load

Code cleanup and improved documentation

6acfd30

track the reset of the local.cfg file

d797ab0

Merge branch 'master' of https://github.com/DSpace/DSpace into authz

2726bc6

Enable additional test for bad request

81b9a18

remove unused field

61cd471

Move the getBaseObjectRestFromUri method to the Utils class and add j…

adfe2b6

…avadoc

benbosman requested changes Feb 26, 2020

View reviewed changes

benbosman requested changes Feb 27, 2020

View reviewed changes

abollini added 2 commits February 27, 2020 15:19

improve comments about all the managed scenarios

be410e2

Clarify the reason of the sleep time

cb83c4c

KevinVdV requested changes Mar 6, 2020

View reviewed changes

abollini added 5 commits March 8, 2020 16:42

Javadoc cleanup, force projection in test that need it

d634939

Add test related to the specialGroups

db2257e

Fix special groups scenario

ace118a

Add test for the AuthorizationFeatureService

616323a

Add test for null object in isAuthorized

4192b09

abollini added 2 commits March 10, 2020 09:58

Merge branch 'master' of https://github.com/DSpace/DSpace into authz

82d4908

Fix unnoted merge conflict by git

2e7a12e

tdonohue reviewed Mar 10, 2020

View reviewed changes

dspace-api/src/main/java/org/dspace/core/Context.java Show resolved Hide resolved

encapsulate the logic to switch current user in the context class

f0210c5

tdonohue reviewed Mar 17, 2020

View reviewed changes

abollini added 3 commits March 20, 2020 18:28

Add expected embedded in the search test

59b7716

Merge branch 'master' of https://github.com/DSpace/DSpace into authz

8cced80

Code cleanup - removed unused special group lists

5e2fc68

tdonohue approved these changes Mar 20, 2020

View reviewed changes

abollini added 2 commits March 20, 2020 21:46

Fix checkstyle issues

95504b4

Add extra check to verify that the returned feature is the expected one

c9bca71

KevinVdV approved these changes Mar 23, 2020

View reviewed changes

benbosman approved these changes Mar 23, 2020

View reviewed changes

tdonohue merged commit 0039b30 into DSpace:master Mar 23, 2020

abollini deleted the DS-4413_authz branch November 6, 2020 08:48

tdonohue added this to the 7.0beta2 milestone Jan 26, 2021

Initial implementation of the authorizations endpoints #2663

Initial implementation of the authorizations endpoints #2663

Conversation

abollini commented Feb 9, 2020 • edited

abollini commented Feb 9, 2020

KevinVdV left a comment

Choose a reason for hiding this comment

abollini commented Feb 14, 2020

benbosman left a comment

Choose a reason for hiding this comment

abollini commented Feb 19, 2020

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue commented Feb 24, 2020 • edited

benbosman left a comment

Choose a reason for hiding this comment

benbosman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinVdV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue commented Mar 9, 2020

abollini commented Mar 10, 2020

benbosman commented Mar 11, 2020

abollini commented Mar 11, 2020

benbosman commented Mar 12, 2020

abollini commented Mar 17, 2020

benbosman commented Mar 17, 2020

tdonohue left a comment

Choose a reason for hiding this comment

abollini commented Mar 20, 2020

tdonohue left a comment

Choose a reason for hiding this comment

benbosman left a comment

Choose a reason for hiding this comment

abollini commented Feb 9, 2020 •

edited

tdonohue commented Feb 24, 2020 •

edited