Code cleanup and improved documentation

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

@@ -103,7 +103,12 @@ public class Authorization {
     /**
      * 
      * @return an unique business identifier generated by concatenation of eperson uuid (if any), feature name and
-     *         object unique identifier
+     *         object unique identifier.
+     *         Some examples
+     *         alwaystrue_core.site_94274020-e617-43b8-90e6-277d04f6be13
+     *         8c7b9132-eadc-4199-af6d-3260a678e96f_alwaystrueadmins_core.site_94274020-e617-43b8-90e6-277d04f6be13
+     *         8c7b9132-eadc-4199-af6d-3260a678e96f_withdrawItem_core.item_c8924526-67ef-479a-8e37-dd8d19e625e9
+     *         8c7b9132-eadc-4199-af6d-3260a678e96f_alwaystrue_submission.workspaceitem_1
      */
     public String getID() {
         StringBuilder sb = new StringBuilder();

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

@@ -11,7 +11,7 @@ import java.sql.SQLException;
 
 import org.dspace.app.rest.model.BaseObjectRest;
 import org.dspace.app.rest.model.RestModel;
-import org.dspace.content.Site;
+import org.dspace.app.rest.model.SiteRest;
 import org.dspace.core.Context;
 import org.springframework.core.annotation.AnnotationUtils;
 
@@ -29,8 +29,8 @@ public interface AuthorizationFeature {
      * @param context
      *            the DSpace Context
      * @param object
-     *            the object target by the feature (MUST be NOT null). Use the {@link Site} object for repository wide
+     *            the object target by the feature (MUST be NOT null). Use the {@link SiteRest} object for repository
-     *            feature
+     *            wide feature
      * @return true if the user associated with the context has access to the feature for the specified object
      */
     boolean isAuthorized(Context context, BaseObjectRest object) throws SQLException;

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

@@ -11,7 +11,7 @@ import java.sql.SQLException;
 import java.util.List;
 
 import org.dspace.app.rest.model.BaseObjectRest;
-import org.dspace.content.Site;
+import org.dspace.app.rest.model.SiteRest;
 import org.dspace.core.Context;
 
 /**
@@ -31,13 +31,13 @@ public interface AuthorizationFeatureService {
      *            the Authorization Feature to check
      * @param object
      *            the object target by the feature. Passing a null object always return false. To check repository wide
-     *            feature pass the {@link Site} object
+     *            feature pass the {@link SiteRest} object
      * @return true if the user associated with the context has access to the feature
      */
     boolean isAuthorized(Context context, AuthorizationFeature feature, BaseObjectRest object) throws SQLException;
 
     /**
-     * Get all the authorization features defined in the syste
+     * Get all the authorization features defined in the system
      *
      * @return a list of all the authorization features
      */
@@ -52,5 +52,12 @@ public interface AuthorizationFeatureService {
      */
     public AuthorizationFeature find(String name);
 
+    /**
+     * Return all the feature that apply to the rest resources identified by the
+     * uniqueType string category.model
+     * 
+     * @param categoryDotModel
+     * @return
+     */
     List<AuthorizationFeature> findByResourceType(String categoryDotModel);
 }

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

@@ -68,17 +68,11 @@ public class AuthorizationFeatureServiceImpl implements AuthorizationFeatureServ
     }
 
     @Override
-    public List<AuthorizationFeature> findByResourceType(String typeID) {
+    public List<AuthorizationFeature> findByResourceType(String categoryDotType) {
+        // Loops through all features, returning any that match the given categoryDotType
         return features
                 .stream()
-                .filter(f -> ArrayUtils.contains(f.getSupportedTypes(), typeID))
+                .filter(f -> ArrayUtils.contains(f.getSupportedTypes(), categoryDotType))
                 .collect(Collectors.toList());
-//        List<AuthorizationFeature> foundFeatures = new ArrayList<AuthorizationFeature>();
-//        for (AuthorizationFeature f : features) {
-//            if (ArrayUtils.contains(f.getSupportedTypes(), typeID)) {
-//                foundFeatures.add(f);
-//            }
-//        }
-//        return foundFeatures;
     }
 }

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

@@ -99,7 +99,7 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
             if (authorizationFeature == null) {
                 return null;
             }
-
+            // get the user specified identified by the id
             EPerson user;
             try {
                 user = authorizationRestUtil.getEperson(context, id);
@@ -108,6 +108,8 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
                 return null;
             }
             EPerson currUser = context.getCurrentUser();
+            // Temporarily change the Context's current user in order to retrieve
+            // authorizations based on that user
             context.setCurrentUser(user);
 
             if (authorizationFeatureService.isAuthorized(context, authorizationFeature, object)) {
@@ -117,6 +119,7 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
                 authz.setObject(object);
                 authorizationRest = converter.toRest(authz, utils.obtainProjection());
             }
+            // restore the real current user
             context.setCurrentUser(currUser);
         } catch (SQLException e) {
             throw new RuntimeException(e.getMessage(), e);
@@ -158,22 +161,10 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
         }
 
         EPerson currUser = context.getCurrentUser();
-        EPerson user = currUser;
+        // get the user specified in the requested parameters
-
+        EPerson user = getUserFromRequestParameter(context, epersonUuid);
-        if (epersonUuid != null) {
+        // Temporarily change the Context's current user in order to retrieve
-            if (context.getCurrentUser() == null) {
+        // authorizations based on that user
-                throw new AuthorizeException("attempt to anonymously access the authorization of the eperson "
-                        + epersonUuid);
-            } else {
-                if (!authorizeService.isAdmin(context) && !epersonUuid.equals(currUser.getID())) {
-                    throw new AuthorizeException("attempt to access the authorization of the eperson " + epersonUuid
-                            + " only system administrators can see the authorization of other users");
-                }
-                user = epersonService.find(context, epersonUuid);
-            }
-        } else {
-            user = null;
-        }
         context.setCurrentUser(user);
         List<AuthorizationFeature> features = authorizationFeatureService.findByResourceType(obj.getUniqueType());
         List<Authorization> authorizations = new ArrayList<Authorization>();
@@ -182,6 +173,7 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
                 authorizations.add(new Authorization(user, f, obj));
             }
         }
+        // restore the real current user
         context.setCurrentUser(currUser);
         return converter.toRestPage(utils.getPage(authorizations, pageable), utils.obtainProjection());
     }
@@ -219,21 +211,10 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
         }
 
         EPerson currUser = context.getCurrentUser();
-        EPerson user = currUser;
+        // get the user specified in the requested parameters
-        if (epersonUuid != null) {
+        EPerson user = getUserFromRequestParameter(context, epersonUuid);
-            if (context.getCurrentUser() == null) {
+        // Temporarily change the Context's current user in order to retrieve
-                throw new AuthorizeException("attempt to anonymously access the authorization of the eperson "
+        // authorizations based on that user
-                        + epersonUuid);
-            } else {
-                if (!authorizeService.isAdmin(context) && !epersonUuid.equals(currUser.getID())) {
-                    throw new AuthorizeException("attempt to access the authorization of the eperson " + epersonUuid
-                            + " only system administrators can see the authorization of other users");
-                }
-                user = epersonService.find(context, epersonUuid);
-            }
-        } else {
-            user = null;
-        }
         context.setCurrentUser(user);
         AuthorizationFeature feature = authorizationFeatureService.find(featureName);
         AuthorizationRest authorizationRest = null;
@@ -244,6 +225,7 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
             authz.setObject(obj);
             authorizationRest = converter.toRest(authz, utils.obtainProjection());
         }
+        // restore the real current user
         context.setCurrentUser(currUser);
         return authorizationRest;
     }
@@ -284,6 +266,38 @@ public class AuthorizationRestRepository extends DSpaceRestRepository<Authorizat
         }
     }
 
+    /**
+     * Return the user specified in the request parameter if valid
+     * 
+     * @param context
+     * @param epersonUuid
+     * @return
+     * @throws AuthorizeException if the user specified in the request parameter is
+     *                            not valid
+     * @throws SQLException       if a database error occurs
+     */
+    private EPerson getUserFromRequestParameter(Context context, UUID epersonUuid)
+            throws AuthorizeException, SQLException {
+        EPerson currUser = context.getCurrentUser();
+        EPerson user = currUser;
+        if (epersonUuid != null) {
+            if (currUser == null) {
+                throw new AuthorizeException("attempt to anonymously access the authorization of the eperson "
+                        + epersonUuid);
+            } else {
+                // an user is specified in the request parameters
+                if (!authorizeService.isAdmin(context) && !epersonUuid.equals(currUser.getID())) {
+                    throw new AuthorizeException("attempt to access the authorization of the eperson " + epersonUuid
+                            + " only system administrators can see the authorization of other users");
+                }
+                user = epersonService.find(context, epersonUuid);
+            }
+        } else {
+            // the request asks to check the permission for the anonymous user
+            user = null;
+        }
+        return user;
+    }
 
     @Override
     public Class<AuthorizationRest> getDomainClass() {

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

@@ -14,20 +14,30 @@ import org.dspace.core.Context;
 import org.dspace.core.ReloadableEntity;
 
 /**
- * This interface must be implemented by all the rest repository that deal with resources that can be make
+ * This interface must be implemented by all the rest repository that need to
- * {@link FindableObject}
+ * provide access to the DSpace API model objects corresponding to the REST
+ * resources that it manages
  * 
  * @author Andrea Bollini (andrea.bollini at 4science.it)
  *
- * @param <F>
+ * @param <F> the ReloadableEntity type
- *            the FindableObject type
+ * @param <PK> the primary key type
- * @param <PK>
- *            the primary key type
  */
 public interface FindableObjectRepository<T extends ReloadableEntity<PK>,
     PK extends Serializable> {
 
+    /**
+     * 
+     * @param context the DSpace context
+     * @param id      the primary key shared between the rest and dspace api object
+     * @return the dspace api model object related to the specified id
+     * @throws SQLException if a database error occurs
+     */
     T findDomainObjectByPk(Context context, PK id) throws SQLException;
 
+    /**
+     * 
+     * @return the class of the primary key
+     */
     Class<PK> getPKClass();
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/security/ReadAuthorizationPermissionEvaluatorPlugin.java

@@ -32,9 +32,9 @@ import org.springframework.stereotype.Component;
  * @author Andrea Bollini (andrea.bollini at 4science.it)
  */
 @Component
-public class AuthorizationPermissionEvaluatorPlugin extends RestObjectPermissionEvaluatorPlugin {
+public class ReadAuthorizationPermissionEvaluatorPlugin extends RestObjectPermissionEvaluatorPlugin {
 
-    private static final Logger log = LoggerFactory.getLogger(AuthorizationPermissionEvaluatorPlugin.class);
+    private static final Logger log = LoggerFactory.getLogger(ReadAuthorizationPermissionEvaluatorPlugin.class);
 
     @Autowired
     AuthorizeService authorizeService;

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

@@ -17,6 +17,7 @@ import java.io.Serializable;
 import java.util.UUID;
 
 import com.jayway.jsonpath.matchers.JsonPathMatchers;
+import org.apache.logging.log4j.Logger;
 import org.dspace.app.rest.authorization.AlwaysFalseFeature;
 import org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature;
 import org.dspace.app.rest.authorization.AlwaysTrueFeature;
@@ -58,6 +59,9 @@ import org.springframework.beans.factory.annotation.Autowired;
  */
 public class AuthorizationRestRepositoryIT extends AbstractControllerIntegrationTest {
 
+    private static final Logger log = org.apache.logging.log4j.LogManager
+            .getLogger(AuthorizationRestRepositoryIT.class);
+
     @Autowired
     private AuthorizationFeatureService authorizationFeatureService;
 
@@ -75,16 +79,34 @@ public class AuthorizationRestRepositoryIT extends AbstractControllerIntegration
 
     private SiteService siteService;
 
+    /** 
+     * this hold a reference to the test feature {@link AlwaysTrueFeature}
+     */
     private AuthorizationFeature alwaysTrue;
 
+    /** 
+     * this hold a reference to the test feature {@link AlwaysFalseFeature}
+     */
     private AuthorizationFeature alwaysFalse;
 
+    /** 
+     * this hold a reference to the test feature {@link AlwaysThrowExceptionFeature}
+     */
     private AuthorizationFeature alwaysException;
 
+    /** 
+     * this hold a reference to the test feature {@link TrueForAdminsFeature}
+     */
     private AuthorizationFeature trueForAdmins;
 
+    /** 
+     * this hold a reference to the test feature {@link TrueForLoggedUsersFeature}
+     */
     private AuthorizationFeature trueForLoggedUsers;
 
+    /** 
+     * this hold a reference to the test feature {@link TrueForTestFeature}
+     */
     private AuthorizationFeature trueForTestUsers;
 
     @Override
@@ -590,7 +612,7 @@ public class AuthorizationRestRepositoryIT extends AbstractControllerIntegration
         configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true);
 
         for (String invalidUri : invalidUris) {
-            System.out.println("Testing the URI: " + invalidUri);
+            log.debug("findByObjectBadRequestTest - Testing the URI: " + invalidUri);
             // verify that it works for administrators with an invalid or missing uri
             String adminToken = getAuthToken(admin.getEmail(), password);
             getClient(adminToken).perform(get("/api/authz/authorizations/search/object")
@@ -958,7 +980,7 @@ public class AuthorizationRestRepositoryIT extends AbstractControllerIntegration
         configurationService.setProperty("org.dspace.app.rest.authorization.AlwaysThrowExceptionFeature.turnoff", true);
 
         for (String invalidUri : invalidUris) {
-            System.out.println("Testing the URI: " + invalidUri);
+            System.out.println("findByObjectAndFeatureBadRequestTest - Testing the URI: " + invalidUri);
             // verify that it works for administrators with an invalid or missing uri
             String adminToken = getAuthToken(admin.getEmail(), password);
             getClient(adminToken).perform(get("/api/authz/authorizations/search/objectAndFeature")