From d532829e736c27f155ba6253fe32a49b13ca808c Mon Sep 17 00:00:00 2001 From: Luca Giamminonni Date: Mon, 16 May 2022 15:30:17 +0200 Subject: [PATCH] [CST-5306] Added security check in EPersonAuthority --- .../authorize/AuthorizeServiceImpl.java | 13 +++++ .../authorize/service/AuthorizeService.java | 8 +++ .../content/authority/EPersonAuthority.java | 50 +++++++++++++++---- .../EPersonRestAuthenticationProvider.java | 8 +-- .../dspace/app/rest/EPersonAuthorityIT.java | 39 +++++++++++++-- 5 files changed, 98 insertions(+), 20 deletions(-) diff --git a/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java b/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java index 919e82f14f..a9874afda6 100644 --- a/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/authorize/AuthorizeServiceImpl.java @@ -7,6 +7,9 @@ */ package org.dspace.authorize; +import static org.dspace.app.util.AuthorizeUtil.canCollectionAdminManageAccounts; +import static org.dspace.app.util.AuthorizeUtil.canCommunityAdminManageAccounts; + import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; @@ -900,6 +903,16 @@ public class AuthorizeServiceImpl implements AuthorizeService { return discoverResult.getTotalSearchResults(); } + @Override + public boolean isAccountManager(Context context) { + try { + return (canCommunityAdminManageAccounts() && isCommunityAdmin(context) + || canCollectionAdminManageAccounts() && isCollectionAdmin(context)); + } catch (SQLException e) { + throw new RuntimeException(e); + } + } + private boolean performCheck(Context context, String query) throws SQLException { if (context.getCurrentUser() == null) { return false; diff --git a/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java b/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java index 9f6171a220..6b097cdd73 100644 --- a/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java +++ b/dspace-api/src/main/java/org/dspace/authorize/service/AuthorizeService.java @@ -592,4 +592,12 @@ public interface AuthorizeService { */ long countAdminAuthorizedCollection(Context context, String query) throws SearchServiceException, SQLException; + + /** + * Returns true if the current user can manage accounts. + * + * @param context context with the current user + * @return true if the current user can manage accounts + */ + boolean isAccountManager(Context context); } diff --git a/dspace-api/src/main/java/org/dspace/content/authority/EPersonAuthority.java b/dspace-api/src/main/java/org/dspace/content/authority/EPersonAuthority.java index 4aa7283a34..8d929a8d3b 100644 --- a/dspace-api/src/main/java/org/dspace/content/authority/EPersonAuthority.java +++ b/dspace-api/src/main/java/org/dspace/content/authority/EPersonAuthority.java @@ -9,16 +9,20 @@ package org.dspace.content.authority; import java.sql.SQLException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.UUID; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.dspace.authorize.factory.AuthorizeServiceFactory; +import org.dspace.authorize.service.AuthorizeService; 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.util.UUIDUtils; +import org.dspace.web.ContextUtil; /** * Implementation of {@link ChoiceAuthority} based on EPerson. Allows you to set @@ -38,6 +42,8 @@ public class EPersonAuthority implements ChoiceAuthority { private EPersonService ePersonService = EPersonServiceFactory.getInstance().getEPersonService(); + private AuthorizeService authorizeService = AuthorizeServiceFactory.getInstance().getAuthorizeService(); + @Override public Choices getBestMatch(String text, String locale) { return getMatches(text, 0, 2, locale); @@ -45,18 +51,14 @@ public class EPersonAuthority implements ChoiceAuthority { @Override public Choices getMatches(String text, int start, int limit, String locale) { - Context context = null; if (limit <= 0) { limit = 20; } - context = new Context(); - List ePersons = null; - try { - ePersons = ePersonService.search(context, text, start, limit); - } catch (SQLException e) { - log.error(e.getMessage(), e); - throw new RuntimeException(e.getMessage(), e); - } + + Context context = getContext(); + + List ePersons = searchEPersons(context, text, start, limit); + List choiceList = new ArrayList(); for (EPerson eperson : ePersons) { choiceList.add(new Choice(eperson.getID().toString(), eperson.getFullName(), eperson.getFullName())); @@ -74,7 +76,7 @@ public class EPersonAuthority implements ChoiceAuthority { return null; } - Context context = new Context(); + Context context = getContext(); try { EPerson ePerson = ePersonService.find(context, uuid); return ePerson != null ? ePerson.getFullName() : null; @@ -85,6 +87,34 @@ public class EPersonAuthority implements ChoiceAuthority { } + private List searchEPersons(Context context, String text, int start, int limit) { + + if (!isCurrentUserAdminOrAccessGroupManager(context)) { + return Collections.emptyList(); + } + + try { + return ePersonService.search(context, text, start, limit); + } catch (SQLException e) { + log.error(e.getMessage(), e); + throw new RuntimeException(e.getMessage(), e); + } + + } + + private Context getContext() { + Context context = ContextUtil.obtainCurrentRequestContext(); + return context != null ? context : new Context(); + } + + private boolean isCurrentUserAdminOrAccessGroupManager(Context context) { + try { + return authorizeService.isAdmin(context) || authorizeService.isAccountManager(context); + } catch (SQLException e) { + throw new RuntimeException(e); + } + } + @Override public String getPluginInstanceName() { return authorityName; diff --git a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/EPersonRestAuthenticationProvider.java b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/EPersonRestAuthenticationProvider.java index 5141d7e9b5..e55734e513 100644 --- a/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/EPersonRestAuthenticationProvider.java +++ b/dspace-server-webapp/src/main/java/org/dspace/app/rest/security/EPersonRestAuthenticationProvider.java @@ -21,7 +21,6 @@ import javax.servlet.http.HttpServletRequest; import org.apache.commons.lang3.StringUtils; import org.dspace.app.rest.login.PostLoggedInAction; import org.dspace.app.rest.utils.ContextUtil; -import org.dspace.app.util.AuthorizeUtil; import org.dspace.authenticate.AuthenticationMethod; import org.dspace.authenticate.service.AuthenticationService; import org.dspace.authorize.service.AuthorizeService; @@ -198,20 +197,15 @@ public class EPersonRestAuthenticationProvider implements AuthenticationProvider EPerson eperson = context.getCurrentUser(); if (eperson != null) { boolean isAdmin = false; - boolean isCommunityAdmin = false; - boolean isCollectionAdmin = false; try { isAdmin = authorizeService.isAdmin(context, eperson); - isCommunityAdmin = authorizeService.isCommunityAdmin(context); - isCollectionAdmin = authorizeService.isCollectionAdmin(context); } catch (SQLException e) { log.error("SQL error while checking for admin rights", e); } if (isAdmin) { authorities.add(new SimpleGrantedAuthority(ADMIN_GRANT)); - } else if ((isCommunityAdmin && AuthorizeUtil.canCommunityAdminManageAccounts()) - || (isCollectionAdmin && AuthorizeUtil.canCollectionAdminManageAccounts())) { + } else if (authorizeService.isAccountManager(context)) { authorities.add(new SimpleGrantedAuthority(MANAGE_ACCESS_GROUP)); } diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonAuthorityIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonAuthorityIT.java index 4626144fd9..3b785bbfa0 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonAuthorityIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonAuthorityIT.java @@ -9,6 +9,7 @@ package org.dspace.app.rest; import static org.dspace.app.rest.matcher.VocabularyMatcher.matchVocabularyEntry; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.empty; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -38,7 +39,7 @@ public class EPersonAuthorityIT extends AbstractControllerIntegrationTest { String thirdEPersonId = createEPerson("Luca", "Bollini"); context.restoreAuthSystemState(); - String token = getAuthToken(eperson.getEmail(), password); + String token = getAuthToken(admin.getEmail(), password); getClient(token).perform(get("/api/submission/vocabularies/EPersonAuthority/entries") .param("filter", "Luca")) .andExpect(status().isOk()) @@ -65,7 +66,7 @@ public class EPersonAuthorityIT extends AbstractControllerIntegrationTest { String thirdEPersonId = createEPerson("Luca", "Bollini"); context.restoreAuthSystemState(); - String token = getAuthToken(eperson.getEmail(), password); + String token = getAuthToken(admin.getEmail(), password); getClient(token).perform(get("/api/submission/vocabularies/EPersonAuthority/entries") .param("filter", "Giamminonni")) .andExpect(status().isOk()) @@ -91,7 +92,7 @@ public class EPersonAuthorityIT extends AbstractControllerIntegrationTest { String secondEPersonId = createEPerson("Andrea", "Bollini"); context.restoreAuthSystemState(); - String token = getAuthToken(eperson.getEmail(), password); + String token = getAuthToken(admin.getEmail(), password); getClient(token).perform(get("/api/submission/vocabularies/EPersonAuthority/entries") .param("filter", firstEPersonId)) .andExpect(status().isOk()) @@ -108,6 +109,38 @@ public class EPersonAuthorityIT extends AbstractControllerIntegrationTest { } + @Test + public void testEPersonAuthorityWithAnonymousUser() throws Exception { + + context.turnOffAuthorisationSystem(); + createEPerson("Luca", "Giamminonni"); + createEPerson("Andrea", "Bollini"); + context.restoreAuthSystemState(); + + getClient().perform(get("/api/submission/vocabularies/EPersonAuthority/entries") + .param("filter", "Luca")) + .andExpect(status().isUnauthorized()); + + } + + @Test + public void testEPersonAuthorityWithNotAdminUser() throws Exception { + + context.turnOffAuthorisationSystem(); + createEPerson("Luca", "Giamminonni"); + createEPerson("Andrea", "Bollini"); + createEPerson("Luca", "Bollini"); + context.restoreAuthSystemState(); + + String token = getAuthToken(eperson.getEmail(), password); + getClient(token).perform(get("/api/submission/vocabularies/EPersonAuthority/entries") + .param("filter", "Luca")) + .andExpect(status().isOk()) + .andExpect(jsonPath("$._embedded.entries", empty())) + .andExpect(jsonPath("$.page.totalElements", Matchers.is(0))); + + } + private String createEPerson(String firstName, String lastName) throws SQLException { return EPersonBuilder.createEPerson(context) .withNameInMetadata(firstName, lastName)