Devolve authorization to the strategy classes. Add tests. #8304

dspace-api/src/main/java/org/dspace/app/requestitem/RequestItemAuthorExtractor.java

@@ -14,14 +14,14 @@ import org.dspace.core.Context;
 
 /**
  * Interface to abstract the strategy for select the author to contact for
- * request copy
+ * request copy.
  *
  * @author Andrea Bollini
  */
 public interface RequestItemAuthorExtractor {
 
     /**
-     * Retrieve the auhtor to contact for a request copy of the give item.
+     * Retrieve the author to contact for requesting a copy of the given item.
      *
      * @param context DSpace context object
      * @param item item to request
@@ -29,5 +29,15 @@ public interface RequestItemAuthorExtractor {
      *         or null if no valid email address was found.
      * @throws SQLException if database error
      */
-    public RequestItemAuthor getRequestItemAuthor(Context context, Item item) throws SQLException;
+    public RequestItemAuthor getRequestItemAuthor(Context context, Item item)
+            throws SQLException;
+
+    /**
+     *
+     * @param context current DSpace session.
+     * @param item the requested Item.
+     * @return true if {@link eperson} is authorized to respond to requests for
+     *          {@link item}.
+     */
+    public boolean isAuthorized(Context context, Item item);
 }

dspace-api/src/main/java/org/dspace/app/requestitem/RequestItemHelpdeskStrategy.java

@@ -16,33 +16,40 @@ import org.dspace.core.I18nUtil;
 import org.dspace.eperson.EPerson;
 import org.dspace.eperson.service.EPersonService;
 import org.dspace.services.ConfigurationService;
-import org.dspace.services.factory.DSpaceServicesFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 
 /**
- * RequestItem strategy to allow DSpace support team's helpdesk to receive requestItem request
+ * RequestItem strategy to allow DSpace support team's help desk to receive
- * With this enabled, then the Item author/submitter doesn't receive the request, but the helpdesk instead does.
+ * requestItem requests.  With this enabled, the Item author/submitter doesn't
+ * receive the request, but the help desk instead does.
  *
- * Failover to the RequestItemSubmitterStrategy, which means the submitter would get the request if there is no
+ * <p>Fails over to the {@link RequestItemSubmitterStrategy}, which means the
- * specified helpdesk email.
+ * submitter would get the request if there is no specified help desk email.
  *
  * @author Sam Ottenhoff
  * @author Peter Dietz
  */
-public class RequestItemHelpdeskStrategy extends RequestItemSubmitterStrategy {
+public class RequestItemHelpdeskStrategy
+        extends RequestItemSubmitterStrategy {
+    static final String P_HELPDESK_OVERRIDE
+            = "request.item.helpdesk.override";
+    static final String P_MAIL_HELPDESK = "mail.helpdesk";
+
     @Autowired(required = true)
     protected EPersonService ePersonService;
 
+    @Autowired(required = true)
+    protected ConfigurationService configurationService;
+
     public RequestItemHelpdeskStrategy() {
     }
 
     @Override
-    public RequestItemAuthor getRequestItemAuthor(Context context, Item item) throws SQLException {
+    public RequestItemAuthor getRequestItemAuthor(Context context, Item item)
-        ConfigurationService configurationService
+            throws SQLException {
-                = DSpaceServicesFactory.getInstance().getConfigurationService();
         boolean helpdeskOverridesSubmitter = configurationService
-            .getBooleanProperty("request.item.helpdesk.override", false);
+            .getBooleanProperty(P_HELPDESK_OVERRIDE, false);
-        String helpDeskEmail = configurationService.getProperty("mail.helpdesk");
+        String helpDeskEmail = configurationService.getProperty(P_MAIL_HELPDESK);
 
         if (helpdeskOverridesSubmitter && StringUtils.isNotBlank(helpDeskEmail)) {
             return getHelpDeskPerson(context, helpDeskEmail);
@@ -53,16 +60,18 @@ public class RequestItemHelpdeskStrategy extends RequestItemSubmitterStrategy {
     }
 
     /**
-     * Return a RequestItemAuthor object for the specified helpdesk email address.
+     * Return a RequestItemAuthor object for the specified help desk email address.
-     * It makes an attempt to find if there is a matching eperson for the helpdesk address, to use the name,
+     * It makes an attempt to find if there is a matching {@link EPerson} for
-     * Otherwise it falls back to a helpdeskname key in the Messages.props.
+     * the help desk address, to use its name.  Otherwise it falls back to the
+     * {@code helpdeskname} key in {@code Messages.properties}.
      *
      * @param context       context
      * @param helpDeskEmail email
      * @return RequestItemAuthor
      * @throws SQLException if database error
      */
-    public RequestItemAuthor getHelpDeskPerson(Context context, String helpDeskEmail) throws SQLException {
+    public RequestItemAuthor getHelpDeskPerson(Context context, String helpDeskEmail)
+            throws SQLException {
         context.turnOffAuthorisationSystem();
         EPerson helpdeskEPerson = ePersonService.findByEmail(context, helpDeskEmail);
         context.restoreAuthSystemState();
@@ -76,4 +85,9 @@ public class RequestItemHelpdeskStrategy extends RequestItemSubmitterStrategy {
             return new RequestItemAuthor(helpdeskName, helpDeskEmail);
         }
     }
+
+    @Override
+    public boolean isAuthorized(Context context, Item item) {
+        return true;
+    }
 }

dspace-api/src/main/java/org/dspace/app/requestitem/RequestItemSubmitterStrategy.java

@@ -9,6 +9,8 @@ package org.dspace.app.requestitem;
 
 import java.sql.SQLException;
 
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.dspace.content.Item;
 import org.dspace.core.Context;
 import org.dspace.eperson.EPerson;
@@ -19,6 +21,7 @@ import org.dspace.eperson.EPerson;
  * @author Andrea Bollini
  */
 public class RequestItemSubmitterStrategy implements RequestItemAuthorExtractor {
+    private static final Logger LOG = LogManager.getLogger();
 
     public RequestItemSubmitterStrategy() {
     }
@@ -32,7 +35,7 @@ public class RequestItemSubmitterStrategy implements RequestItemAuthorExtractor
      */
     @Override
     public RequestItemAuthor getRequestItemAuthor(Context context, Item item)
-        throws SQLException {
+            throws SQLException {
         EPerson submitter = item.getSubmitter();
         RequestItemAuthor author = null;
         if (null != submitter) {
@@ -41,4 +44,20 @@ public class RequestItemSubmitterStrategy implements RequestItemAuthorExtractor
         }
         return author;
     }
+
+    @Override
+    public boolean isAuthorized(Context context, Item item) {
+        RequestItemAuthor authorizer;
+        try {
+            authorizer = getRequestItemAuthor(context, item);
+        } catch (SQLException ex) {
+            LOG.warn("Failed to find an authorizer:  {}", ex::getMessage);
+            return false;
+        }
+        EPerson user = context.getCurrentUser();
+        if (null == user) {
+            return false;
+        }
+        return authorizer.getEmail().equals(user.getEmail());
+    }
 }

dspace-api/src/test/java/org/dspace/app/requestitem/RequestItemHelpdeskStrategyTest.java

@@ -0,0 +1,128 @@
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+package org.dspace.app.requestitem;
+
+import java.sql.SQLException;
+import static org.junit.Assert.assertTrue;
+
+import org.dspace.AbstractUnitTest;
+import org.dspace.builder.AbstractBuilder;
+import org.dspace.builder.CollectionBuilder;
+import org.dspace.builder.CommunityBuilder;
+import org.dspace.builder.EPersonBuilder;
+import org.dspace.builder.ItemBuilder;
+import org.dspace.content.Collection;
+import org.dspace.content.Community;
+import org.dspace.content.Item;
+import org.dspace.core.Context;
+import org.dspace.eperson.EPerson;
+import org.dspace.eperson.factory.EPersonServiceFactory;
+import org.dspace.eperson.service.EPersonService;
+import org.dspace.services.ConfigurationService;
+import org.dspace.services.factory.DSpaceServicesFactory;
+import org.junit.AfterClass;
+import static org.junit.Assert.assertEquals;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Ignore;
+import org.junit.Test;
+
+/**
+ *
+ * @author mwood
+ */
+public class RequestItemHelpdeskStrategyTest
+        extends AbstractUnitTest {
+    private static final String HELPDESK_ADDRESS = "helpdesk@example.com";
+    private static final String AUTHOR_ADDRESS = "john.doe@example.com";
+
+    private static ConfigurationService configurationService;
+    private static EPersonService epersonService;
+    private static EPerson johnDoe;
+
+    private Item item;
+
+    @BeforeClass
+    public static void setUpClass()
+            throws SQLException {
+        AbstractBuilder.init(); // AbstractUnitTest doesn't do this for us.
+
+        configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
+        epersonService = EPersonServiceFactory.getInstance().getEPersonService();
+
+        Context ctx = new Context();
+        ctx.turnOffAuthorisationSystem();
+        johnDoe = EPersonBuilder.createEPerson(ctx)
+                .withEmail(AUTHOR_ADDRESS)
+                .withNameInMetadata("John", "Doe")
+                .build();
+        ctx.restoreAuthSystemState();
+        ctx.complete();
+    }
+
+    @AfterClass
+    public static void tearDownClass() {
+        AbstractBuilder.destroy(); // AbstractUnitTest doesn't do this for us.
+    }
+
+    @Before
+    public void setUp() {
+        context = new Context();
+        context.setCurrentUser(johnDoe);
+        context.turnOffAuthorisationSystem();
+        Community community = CommunityBuilder.createCommunity(context).build();
+        Collection collection = CollectionBuilder.createCollection(context, community).build();
+        item = ItemBuilder.createItem(context, collection)
+                .build();
+        context.restoreAuthSystemState();
+        context.setCurrentUser(null);
+    }
+
+    /**
+     * Test of getRequestItemAuthor method, of class RequestItemHelpdeskStrategy.
+     * @throws java.lang.Exception passed through.
+     */
+    @Test
+    public void testGetRequestItemAuthor()
+            throws Exception {
+        RequestItemHelpdeskStrategy instance = new RequestItemHelpdeskStrategy();
+        instance.configurationService = configurationService;
+        instance.ePersonService = epersonService;
+
+        // Check with help desk enabled.
+        configurationService.setProperty(RequestItemHelpdeskStrategy.P_HELPDESK_OVERRIDE, "true");
+        configurationService.setProperty(RequestItemHelpdeskStrategy.P_MAIL_HELPDESK, HELPDESK_ADDRESS);
+        RequestItemAuthor author = instance.getRequestItemAuthor(context, item);
+        assertEquals("Wrong author address", HELPDESK_ADDRESS, author.getEmail());
+
+        // Check with help desk disabled.
+        configurationService.setProperty(RequestItemHelpdeskStrategy.P_HELPDESK_OVERRIDE, "false");
+        author = instance.getRequestItemAuthor(context, item);
+        assertEquals("Wrong author address", AUTHOR_ADDRESS, author.getEmail());
+    }
+
+    /**
+     * Test of getHelpDeskPerson method, of class RequestItemHelpdeskStrategy.
+     * @throws java.lang.Exception passed through.
+     */
+    @Ignore
+    @Test
+    public void testGetHelpDeskPerson() throws Exception {
+    }
+
+    /**
+     * Silly test of isAuthorized method.
+     */
+    @Test
+    public void testIsAuthorized() {
+        RequestItemHelpdeskStrategy instance = new RequestItemHelpdeskStrategy();
+        instance.configurationService = configurationService;
+        instance.ePersonService = epersonService;
+        assertTrue(instance.isAuthorized(context, item));
+    }
+}

dspace-api/src/test/java/org/dspace/app/requestitem/RequestItemSubmitterStrategyTest.java

@@ -0,0 +1,115 @@
+/**
+ * The contents of this file are subject to the license and copyright
+ * detailed in the LICENSE and NOTICE files at the root of the source
+ * tree and available online at
+ *
+ * http://www.dspace.org/license/
+ */
+package org.dspace.app.requestitem;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.SQLException;
+
+import org.dspace.AbstractUnitTest;
+import org.dspace.builder.AbstractBuilder;
+import org.dspace.builder.CollectionBuilder;
+import org.dspace.builder.CommunityBuilder;
+import org.dspace.builder.EPersonBuilder;
+import org.dspace.builder.ItemBuilder;
+import org.dspace.content.Collection;
+import org.dspace.content.Community;
+import org.dspace.content.Item;
+import org.dspace.core.Context;
+import org.dspace.eperson.EPerson;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ *
+ * @author mwood
+ */
+public class RequestItemSubmitterStrategyTest
+        extends AbstractUnitTest {
+    private static final String AUTHOR_ADDRESS = "john.doe@example.com";
+    private static final String NON_AUTHOR_ADDRESS = "richard.roe@example.com";
+
+    private static EPerson johnDoe;
+    private static EPerson richardRoe;
+
+    private Item item;
+
+    @BeforeClass
+    public static void setUpClass()
+            throws SQLException {
+        AbstractBuilder.init(); // AbstractUnitTest doesn't do this for us.
+
+        Context ctx = new Context();
+        ctx.turnOffAuthorisationSystem();
+        johnDoe = EPersonBuilder.createEPerson(ctx)
+                .withEmail(AUTHOR_ADDRESS)
+                .withNameInMetadata("John", "Doe")
+                .build();
+        richardRoe = EPersonBuilder.createEPerson(ctx)
+                .withEmail(NON_AUTHOR_ADDRESS)
+                .withNameInMetadata("Richard", "Roe")
+                .build();
+        ctx.restoreAuthSystemState();
+        ctx.complete();
+    }
+
+    @AfterClass
+    public static void tearDownClass() {
+        AbstractBuilder.destroy(); // AbstractUnitTest doesn't do this for us.
+    }
+
+    @Before
+    public void setUp() {
+        context = new Context();
+        context = new Context();
+        context.setCurrentUser(johnDoe);
+        context.turnOffAuthorisationSystem();
+        Community community = CommunityBuilder.createCommunity(context).build();
+        Collection collection = CollectionBuilder.createCollection(context, community).build();
+        item = ItemBuilder.createItem(context, collection)
+                .build();
+        context.restoreAuthSystemState();
+        context.setCurrentUser(null);
+    }
+
+    /**
+     * Test of getRequestItemAuthor method, of class RequestItemSubmitterStrategy.
+     * @throws java.lang.Exception passed through.
+     */
+    @Test
+    public void testGetRequestItemAuthor()
+            throws Exception {
+        RequestItemSubmitterStrategy instance = new RequestItemSubmitterStrategy();
+        RequestItemAuthor author = instance.getRequestItemAuthor(context, item);
+        assertEquals("Wrong author address", AUTHOR_ADDRESS, author.getEmail());
+    }
+
+    /**
+     * Test of isAuthorized method, of class RequestItemSubmitterStrategy.
+     */
+    @Test
+    public void testIsAuthorized() {
+        RequestItemSubmitterStrategy instance = new RequestItemSubmitterStrategy();
+
+        // Test with correct logged-in user.
+        context.setCurrentUser(johnDoe);
+        assertTrue("Should be authorized", instance.isAuthorized(context, item));
+
+        // Test with incorrect logged-in user.
+        context.setCurrentUser(richardRoe);
+        assertFalse("Should not be authorized", instance.isAuthorized(context, item));
+
+        // Test when not logged in.
+        context.setCurrentUser(null);
+        assertFalse("Should not be authorized", instance.isAuthorized(context, item));
+    }
+}

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

@@ -27,7 +27,6 @@ import org.apache.http.client.utils.URIBuilder;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.dspace.app.requestitem.RequestItem;
-import org.dspace.app.requestitem.RequestItemAuthor;
 import org.dspace.app.requestitem.RequestItemAuthorExtractor;
 import org.dspace.app.requestitem.RequestItemEmailNotifier;
 import org.dspace.app.requestitem.service.RequestItemService;
@@ -197,7 +196,7 @@ public class RequestItemRepository
             responseLink = getLinkTokenEmail(ri.getToken());
         } catch (URISyntaxException | MalformedURLException e) {
             LOG.warn("Impossible URL error while composing email:  {}",
-                    e.getMessage());
+                    e::getMessage);
             throw new RuntimeException("Request not sent:  " + e.getMessage());
         }
 
@@ -229,14 +228,7 @@ public class RequestItemRepository
         }
 
         // Check for authorized user
-        RequestItemAuthor authorizer;
+        if (!requestItemAuthorExtractor.isAuthorized(context, ri.getItem())) {
-        try {
-            authorizer = requestItemAuthorExtractor.getRequestItemAuthor(context, ri.getItem());
-        } catch (SQLException ex) {
-            LOG.warn("Failed to find an authorizer:  {}", ex.getMessage());
-            authorizer = new RequestItemAuthor("", "");
-        }
-        if (!authorizer.getEmail().equals(context.getCurrentUser().getEmail())) {
             throw new AuthorizeException("Not authorized to approve this request");
         }
 
@@ -267,7 +259,7 @@ public class RequestItemRepository
         try {
             RequestItemEmailNotifier.sendResponse(context, ri, subject, message);
         } catch (IOException ex) {
-            LOG.warn("Response not sent:  {}", ex.getMessage());
+            LOG.warn("Response not sent:  {}", ex::getMessage);
             throw new RuntimeException("Response not sent", ex);
         }
 
@@ -276,7 +268,7 @@ public class RequestItemRepository
             try {
                 RequestItemEmailNotifier.requestOpenAccess(context, ri);
             } catch (IOException ex) {
-                LOG.warn("Open access request not sent:  {}", ex.getMessage());
+                LOG.warn("Open access request not sent:  {}", ex::getMessage);
                 throw new RuntimeException("Open access request not sent", ex);
             }
         }