TRUNK-6466: Enhance Proxy Privilege methods (#5473)

Author: ibacher

api/src/main/java/org/openmrs/api/context/Context.java

@@ -11,7 +11,6 @@
 
 import org.aopalliance.aop.Advice;
 import org.apache.commons.lang3.StringUtils;
-import org.hibernate.SessionFactory;
 import org.openmrs.Allergen;
 import org.openmrs.GlobalProperty;
 import org.openmrs.OpenmrsObject;
@@ -726,9 +725,7 @@ public static Set<Role> getAllRoles(User user) throws Exception {
 	}
 
 	/**
-	 * Convenience method. Passes through to userContext.hasPrivilege(String)
-	 *
-	 * <strong>Should</strong> give daemon user full privileges
+	 * Tests whether the currently authenticated user has a particular privilege
 	 */
 	public static boolean hasPrivilege(String privilege) {
 		// the daemon threads have access to all things
@@ -760,19 +757,49 @@ public static void requirePrivilege(String privilege) throws ContextAuthenticati
 			throw new ContextAuthenticationException(errorMessage);
 		}
 	}
-
+	
 	/**
-	 * Convenience method. Passes through to {@link UserContext#addProxyPrivilege(String)}
+	 * Adds one or more privileges to the list of privileges that that {@link #hasPrivilege(String)} will
+	 * regard as available regardless of whether the user would otherwise have the privilege.
+	 * <p/>
+	 * This is useful for situations where a system process may need access to some piece of data that the
+	 * user would not otherwise have access to, like a GlobalProperty. <strong>This facility should not be
+	 * used to return data to the user that they otherwise would be unable to see.</strong>
+	 * <p/>
+	 * The expected usage is:
+	 * <p/>
+	 * <pre>{@code
+	 * try {
+	 *   Context.addProxyPrivilege(&quot;AAA&quot;);
+	 *   Context.get*Service().methodRequiringAAAPrivilege();
+	 * }
+	 * finally {
+	 *   Context.removeProxyPrivilege(&quot;AAA&quot;);
+	 * }}
+	 * </pre>
+	 * <p/>
+	 *
+	 * @param privileges privileges to add in string form
+	 * @see #hasPrivilege(String)
+	 * @see #removeProxyPrivilege(String...)
 	 */
-	public static void addProxyPrivilege(String privilege) {
-		getUserContext().addProxyPrivilege(privilege);
+	public static void addProxyPrivilege(String... privileges) {
+		getUserContext().addProxyPrivilege(privileges);
 	}
-
+	
 	/**
-	 * Convenience method. Passes through to {@link UserContext#removeProxyPrivilege(String)}
+	 * Removes one or more privileges to the list of privileges that that {@link #hasPrivilege(String)} will
+	 * regard as available regardless of whether the user would otherwise have the privilege.
+	 * <p/>
+	 * This is the compliment for {@link #addProxyPrivilege(String...)} to clean-up the context.
+	 * <p/>
+	 *
+	 * @param privileges privileges to remove in string form
+	 * @see #hasPrivilege(String)
+	 * @see #addProxyPrivilege(String...)
 	 */
-	public static void removeProxyPrivilege(String privilege) {
-		getUserContext().removeProxyPrivilege(privilege);
+	public static void removeProxyPrivilege(String... privileges) {
+		getUserContext().removeProxyPrivilege(privileges);
 	}
 
 	/**

api/src/main/java/org/openmrs/api/context/UserContext.java

@@ -11,10 +11,12 @@
 
 import java.io.Serializable;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.List;
+import java.util.LinkedHashSet;
 import java.util.Locale;
+import java.util.Objects;
 import java.util.Set;
 
 import org.apache.commons.lang3.StringUtils;
@@ -59,7 +61,7 @@ public class UserContext implements Serializable {
 	/**
 	 * User's permission proxies
 	 */
-	private List<String> proxies = Collections.synchronizedList(new ArrayList<>());
+	private final Set<String> proxies = Collections.synchronizedSet(new LinkedHashSet<>());
 
 	/**
 	 * User's locale
@@ -223,40 +225,63 @@ public void logout() {
 	}
 
 	/**
-	 * Gives the given privilege to all calls to hasPrivilege. This method was visualized as being
-	 * used as follows (try/finally is important):
-	 *
-	 * <pre>
+	 * Adds one or more privileges to the list of privileges that that {@link #hasPrivilege(String)} will
+	 * regard as available regardless of whether the user would otherwise have the privilege.
+	 * <p/>
+	 * This is useful for situations where a system process may need access to some piece of data that the
+	 * user would not otherwise have access to, like a GlobalProperty. <strong>This facility should not be
+	 * used to return data to the user that they otherwise would be unable to see.</strong>
+	 * <p/>
+	 * The expected usage is:
+	 * <p/>
+	 * <pre>{@code
 	 * try {
 	 *   Context.addProxyPrivilege(&quot;AAA&quot;);
 	 *   Context.get*Service().methodRequiringAAAPrivilege();
 	 * }
 	 * finally {
 	 *   Context.removeProxyPrivilege(&quot;AAA&quot;);
-	 * }
+	 * }}
 	 * </pre>
+	 * <p/>
 	 *
-	 * @param privilege to give to users
+	 * @param privileges privileges to add in string form
+	 * @see #hasPrivilege(String)
+	 * @see #removeProxyPrivilege(String...)
 	 */
-	public void addProxyPrivilege(String privilege) {
-		
-		if (privilege == null) {
+	public void addProxyPrivilege(String... privileges) {
+		if (privileges == null || Arrays.stream(privileges).anyMatch(Objects::isNull)) {
 			throw new IllegalArgumentException("UserContext.addProxyPrivilege does not accept null privileges");
 		}
 
-		log.debug("Adding proxy privilege: {}", privilege);
-		
-		proxies.add(privilege);
+		for (String privilege : privileges) {
+			log.debug("Adding proxy privilege: {}", privilege);
+			proxies.add(privilege);
+		}
 	}
 
 	/**
-	 * Will remove one instance of privilege from the privileges that are currently proxied
+	 * Removes one or more privileges to the list of privileges that that {@link #hasPrivilege(String)} will
+	 * regard as available regardless of whether the user would otherwise have the privilege.
+	 * <p/>
+	 * This is the compliment for {@link #addProxyPrivilege(String...)} to clean-up the context.
+	 * <p/>
 	 *
-	 * @param privilege Privilege to remove in string form
+	 * @param privileges privileges to remove in string form
+	 * @see #hasPrivilege(String)
+	 * @see #addProxyPrivilege(String...)
 	 */
-	public void removeProxyPrivilege(String privilege) {
-		log.debug("Removing proxy privilege: {}", privilege);
-		proxies.remove(privilege);
+	public void removeProxyPrivilege(String... privileges) {
+		if (privileges == null || Arrays.stream(privileges).allMatch(Objects::isNull)) {
+			return;
+		}
+		
+		for (String privilege : privileges) {
+			if (privilege != null) {
+				log.debug("Removing privilege: {}", privilege);
+				proxies.remove(privilege);
+			}
+		}
 	}
 
 	/**
@@ -315,9 +340,9 @@ public Set<Role> getAllRoles(User user) throws Exception {
 	}
 
 	/**
-	 * Tests whether currently authenticated user has a particular privilege
+	 * Tests whether the currently authenticated user has a particular privilege
 	 *
-	 * @param privilege
+	 * @param privilege The privilege to check if it's available
 	 * @return true if authenticated user has given privilege
 	 * <strong>Should</strong> authorize if authenticated user has specified privilege
 	 * <strong>Should</strong> authorize if authenticated role has specified privilege

api/src/test/java/org/openmrs/api/context/ContextTest.java

@@ -368,4 +368,80 @@ public void clearEntireCache_shouldClearEntireCache() {
 		Context.getPatientService().getPatient(PERSON_NAME_ID_2);
 		assertThat(sf.getStatistics().getSecondLevelCacheHitCount(), is(hitCount));
 	}
+
+	/**
+	 * @see Context#addProxyPrivilege(String...)
+	 */
+	@Test
+	public void addProxyPrivilege_shouldAddMultiplePrivileges() {
+		Context.logout(); // logout to ensure user doesn't have privileges through roles
+		try {
+			// arrange - verify user doesn't have these privileges
+			assertFalse(Context.hasPrivilege("Some Privilege"));
+			assertFalse(Context.hasPrivilege("Another Privilege"));
+
+			// act
+			Context.addProxyPrivilege("Some Privilege", "Another Privilege");
+
+			// assert
+			assertTrue(Context.hasPrivilege("Some Privilege"));
+			assertTrue(Context.hasPrivilege("Another Privilege"));
+		} finally {
+			Context.removeProxyPrivilege("Some Privilege", "Another Privilege");
+			authenticate(); // restore authenticated state
+		}
+	}
+
+	/**
+	 * @see Context#addProxyPrivilege(String...)
+	 */
+	@Test
+	public void addProxyPrivilege_shouldThrowExceptionForNullArray() {
+		// act & assert
+		assertThrows(IllegalArgumentException.class, () -> Context.addProxyPrivilege((String[]) null));
+	}
+
+	/**
+	 * @see Context#removeProxyPrivilege(String...)
+	 */
+	@Test
+	public void removeProxyPrivilege_shouldRemoveMultiplePrivileges() {
+		Context.logout();
+		try {
+			// arrange
+			Context.addProxyPrivilege("Privilege A", "Privilege B", "Privilege C");
+
+			// act
+			Context.removeProxyPrivilege("Privilege A", "Privilege C");
+
+			// assert
+			assertFalse(Context.hasPrivilege("Privilege A"));
+			assertTrue(Context.hasPrivilege("Privilege B"));
+			assertFalse(Context.hasPrivilege("Privilege C"));
+		} finally {
+			Context.removeProxyPrivilege("Privilege B");
+			authenticate();
+		}
+	}
+
+	/**
+	 * @see Context#removeProxyPrivilege(String...)
+	 */
+	@Test
+	public void removeProxyPrivilege_shouldHandleNullArrayGracefully() {
+		Context.logout();
+		try {
+			// arrange
+			Context.addProxyPrivilege("Some Test Privilege");
+
+			// act
+			Context.removeProxyPrivilege((String[]) null);
+
+			// assert - should still have the privilege since null was passed
+			assertTrue(Context.hasPrivilege("Some Test Privilege"));
+		} finally {
+			Context.removeProxyPrivilege("Some Test Privilege");
+			authenticate();
+		}
+	}
 }

api/src/test/java/org/openmrs/api/context/UserContextTest.java

@@ -22,8 +22,12 @@
 import org.springframework.beans.factory.annotation.Autowired;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class UserContextTest extends BaseContextSensitiveTest {
 
@@ -104,11 +108,64 @@ void getDefaultLocationId_shouldReturnNullForInvalidUuid() {
 		Context.getUserContext().setLocationId(null);
 		testUser.setUserProperty(OpenmrsConstants.USER_PROPERTY_DEFAULT_LOCATION, "0e32f474-eca5-4cc2-a64d-53b086f27e52");
 		userService.saveUser(testUser);
-		
+
 		// act
 		Integer locationId = Context.getUserContext().getDefaultLocationId(testUser);
 
 		// assert
 		assertThat(locationId, nullValue());
 	}
+
+	@Test
+	void addProxyPrivilege_shouldAddMultiplePrivileges() {
+		// arrange
+		UserContext userContext = new UserContext(new TestUsernameAuthenticationScheme());
+
+		// act
+		userContext.addProxyPrivilege("Privilege1", "Privilege2", "Privilege3");
+
+		// assert
+		assertThat(userContext.hasPrivilege("Privilege1"), is(true));
+		assertThat(userContext.hasPrivilege("Privilege2"), is(true));
+		assertThat(userContext.hasPrivilege("Privilege3"), is(true));
+	}
+
+	@Test
+	void addProxyPrivilege_shouldThrowExceptionForNullArray() {
+		// arrange
+		UserContext userContext = new UserContext(new TestUsernameAuthenticationScheme());
+
+		// act & assert
+		assertThrows(IllegalArgumentException.class, () -> userContext.addProxyPrivilege((String[]) null));
+	}
+
+	@Test
+	void removeProxyPrivilege_shouldRemoveMultiplePrivileges() {
+		// arrange
+		UserContext userContext = new UserContext(new TestUsernameAuthenticationScheme());
+		userContext.addProxyPrivilege("Privilege1");
+		userContext.addProxyPrivilege("Privilege2");
+		userContext.addProxyPrivilege("Privilege3");
+
+		// act
+		userContext.removeProxyPrivilege("Privilege1", "Privilege3");
+
+		// assert
+		assertThat(userContext.hasPrivilege("Privilege1"), is(false));
+		assertThat(userContext.hasPrivilege("Privilege2"), is(true));
+		assertThat(userContext.hasPrivilege("Privilege3"), is(false));
+	}
+
+	@Test
+	void removeProxyPrivilege_shouldHandleNullArrayGracefully() {
+		// arrange
+		UserContext userContext = new UserContext(new TestUsernameAuthenticationScheme());
+		userContext.addProxyPrivilege("Privilege1");
+
+		// act
+		userContext.removeProxyPrivilege((String[]) null);
+
+		// assert - should still have the privilege since null was passed
+		assertThat(userContext.hasPrivilege("Privilege1"), is(true));
+	}
 }