NIHVIVO-2492 A better implementation to allow RequiresAuthorizationFor this or that.

webapp/src/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelper.java

@@ -7,8 +7,10 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 import java.lang.reflect.Constructor;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -21,6 +23,7 @@ import org.apache.commons.logging.LogFactory;
 import edu.cornell.mannlib.vitro.webapp.auth.identifier.IdentifierBundle;
 import edu.cornell.mannlib.vitro.webapp.auth.identifier.RequestIdentifiers;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.PolicyHelper.RequiresAuthorizationFor.NoAction;
+import edu.cornell.mannlib.vitro.webapp.auth.policy.PolicyHelper.RequiresAuthorizationFor.Or;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.Authorization;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.PolicyDecision;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.PolicyIface;
@@ -41,6 +44,17 @@ public class PolicyHelper {
 	 * 
 	 * Any RequestedAction can be specified, but the most common use will be to
 	 * specify implementations of UsePagesRequestedAction.
+	 * 
+	 * Note that a combination of AND and OR relationships can be created
+	 * (at-signs converted to #-signs, so Javadoc won't try to actually apply
+	 * the annotations):
+	 * 
+	 * <pre>
+	 * #RequiresAuthorizationFor(This.class)
+	 * #RequiresAuthorizationFor({This.class, That.class})
+	 * #RequiresAuthorizationFor(value=This.class, or=#Or(That.class))
+	 * #RequiresAuthorizationFor(or={#Or(One_A.class, One_B.class), #Or(Two.class)})
+	 * </pre>
 	 */
 	@Retention(RetentionPolicy.RUNTIME)
 	@Target(ElementType.TYPE)
@@ -49,15 +63,26 @@ public class PolicyHelper {
 			/* no fields */
 		}
 
+		@Retention(RetentionPolicy.RUNTIME)
+		public static @interface Or {
 			Class<? extends RequestedAction>[] value() default NoAction.class;
 		}
 
+		Class<? extends RequestedAction>[] value() default NoAction.class;
+
+		Or[] or() default @Or();
+	}
+
 	/**
 	 * Does this servlet require authorization?
 	 */
 	public static boolean isRestrictedPage(VitroHttpServlet servlet) {
 		Class<? extends VitroHttpServlet> servletClass = servlet.getClass();
-		return !getRequiredAuthorizationsForServlet(servletClass).isEmpty();
+		try {
+			return !ActionClauses.forServletClass(servletClass).isEmpty();
+		} catch (PolicyHelperException e) {
+			return true;
+		}
 	}
 
 	/**
@@ -66,8 +91,7 @@ public class PolicyHelper {
 	 */
 	public static boolean areRequiredAuthorizationsSatisfied(
 			HttpServletRequest req, VitroHttpServlet servlet) {
-		Class<? extends VitroHttpServlet> servletClass = servlet.getClass();
-		return areRequiredAuthorizationsSatisfied(req, servletClass);
+		return areRequiredAuthorizationsSatisfied(req, servlet.getClass());
 	}
 
 	/**
@@ -77,8 +101,12 @@ public class PolicyHelper {
 	public static boolean areRequiredAuthorizationsSatisfied(
 			HttpServletRequest req,
 			Class<? extends VitroHttpServlet> servletClass) {
-		return areRequiredAuthorizationsSatisfied(req,
-				getRequiredAuthorizationsForServlet(servletClass));
+		try {
+			return isActionClausesAuthorized(req,
+					ActionClauses.forServletClass(servletClass));
+		} catch (PolicyHelperException e) {
+			return false;
+		}
 	}
 
 	/**
@@ -87,114 +115,151 @@ public class PolicyHelper {
 	 */
 	public static boolean areRequiredAuthorizationsSatisfied(
 			HttpServletRequest req,
-			Class<? extends RequestedAction>... actionClasses) {
-		List<Class<? extends RequestedAction>> classList = Arrays
-				.asList(actionClasses);
-
-		Set<RequestedAction> actions = instantiateActions(classList);
-		if (actions == null) {
-			log.debug("not authorized: failed to instantiate actions");
+			Collection<Class<? extends RequestedAction>> actionClasses) {
+		try {
+			return isActionClausesAuthorized(req, new ActionClauses(
+					actionClasses));
+		} catch (PolicyHelperException e) {
 			return false;
 		}
-
-		return areRequiredAuthorizationsSatisfied(req, actions);
-	}
-
-	/**
-	 * Are these actions authorized for the current user by the current
-	 * policies?
-	 */
-	public static boolean areRequiredAuthorizationsSatisfied(
-			HttpServletRequest req,
-			Collection<? extends RequestedAction> actions) {
-		PolicyIface policy = ServletPolicyList.getPolicies(req);
-		IdentifierBundle ids = RequestIdentifiers.getIdBundleForRequest(req);
-
-		for (RequestedAction action : actions) {
-			if (isAuthorized(policy, ids, action)) {
-				log.debug("not authorized");
-				return false;
-			}
-		}
-
-		log.debug("authorized");
-		return true;
 	}
 
 	/**
 	 * Is this action class authorized for the current user by the current
 	 * policies?
 	 */
-	@SuppressWarnings("unchecked")
-	public static boolean isAuthorized(HttpServletRequest req,
+	public static boolean isActionAuthorized(HttpServletRequest req,
 			Class<? extends RequestedAction> actionClass) {
-		return areRequiredAuthorizationsSatisfied(req, actionClass);
+		try {
+			return isActionClausesAuthorized(req,
+					new ActionClauses(actionClass));
+		} catch (PolicyHelperException e) {
+			return false;
+		}
+	}
+
+	/**
+	 * Actions must be authorized for the current user by the current policies.
+	 * If no actions, no problem.
+	 */
+	private static boolean isActionClausesAuthorized(HttpServletRequest req,
+			ActionClauses actionClauses) {
+		PolicyIface policy = ServletPolicyList.getPolicies(req);
+		IdentifierBundle ids = RequestIdentifiers.getIdBundleForRequest(req);
+
+		return actionClauses.isEmpty()
+				|| isActionClausesAuthorized(policy, ids, actionClauses);
+	}
+
+	/** Any clause in an ActionClauses may be authorized. */
+	private static boolean isActionClausesAuthorized(PolicyIface policy,
+			IdentifierBundle ids, ActionClauses actionClauses) {
+		for (Set<RequestedAction> clause : actionClauses.getClauseList()) {
+			if (isClauseAuthorized(policy, ids, clause)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	/** All actions in a clause must be authorized. */
+	private static boolean isClauseAuthorized(PolicyIface policy,
+			IdentifierBundle ids, Set<RequestedAction> clause) {
+		for (RequestedAction action : clause) {
+			if (!isActionAuthorized(policy, ids, action)) {
+				log.debug("not authorized");
+				return false;
+			}
+		}
+		return true;
 	}
 
 	/**
 	 * Is this action authorized for these IDs by this policy?
 	 */
-	private static boolean isAuthorized(PolicyIface policy,
+	private static boolean isActionAuthorized(PolicyIface policy,
 			IdentifierBundle ids, RequestedAction action) {
 		PolicyDecision decision = policy.isAuthorized(ids, action);
 		log.debug("decision for '" + action.getClass().getName() + "' was: "
 				+ decision);
-		return (decision == null)
-				|| (decision.getAuthorized() != Authorization.AUTHORIZED);
+		return (decision != null)
+				&& (decision.getAuthorized() == Authorization.AUTHORIZED);
 	}
 
 	/**
-	 * What RequestedActions does this servlet require authorization for?
+	 * This helper class embodies the list of OR and AND relationships for the
+	 * required authorizations. A group of AND relationships is a "clause", and
+	 * the list of clauses are in an OR relationship.
 	 * 
-	 * Keep this private, since it reveals how the Annotation is implemented. If
-	 * we change the Annotation to include "or" and "and", then this method
-	 * becomes meaningless with its current return type.
+	 * Authorization is successful if ALL of the actions in ANY of the clauses
+	 * are authorized, or if there are NO clauses.
+	 * 
+	 * If any action can't be instantiated, throw an exception so authorization
+	 * will fail.
 	 */
-	private static Set<RequestedAction> getRequiredAuthorizationsForServlet(
-			Class<? extends VitroHttpServlet> clazz) {
-		Set<RequestedAction> result = new HashSet<RequestedAction>();
+	private static class ActionClauses {
+		static ActionClauses forServletClass(
+				Class<? extends VitroHttpServlet> servletClass)
+				throws PolicyHelperException {
+			return new ActionClauses(
+					servletClass.getAnnotation(RequiresAuthorizationFor.class));
+		}
 
-		RequiresAuthorizationFor annotation = clazz
-				.getAnnotation(RequiresAuthorizationFor.class);
+		private final List<Set<RequestedAction>> clauseList;
 
+		ActionClauses(RequiresAuthorizationFor annotation)
+				throws PolicyHelperException {
+			List<Set<RequestedAction>> list = new ArrayList<Set<RequestedAction>>();
 			if (annotation != null) {
-			for (Class<? extends RequestedAction> actionClass : annotation
-					.value()) {
-				if (NoAction.class != actionClass) {
-					RequestedAction action = instantiateAction(actionClass);
-					if (action != null) {
-						result.add(action);
+				addClause(list, annotation.value());
+				for (Or orAnnotation : annotation.or()) {
+					addClause(list, orAnnotation.value());
 				}
 			}
-			}
-		}
-		return result;
+			this.clauseList = Collections.unmodifiableList(list);
 		}
 
-	/**
-	 * Instantiate actions from their classes. If any one of the classes cannot
-	 * be instantiated, return null.
-	 */
-	private static Set<RequestedAction> instantiateActions(
-			Collection<Class<? extends RequestedAction>> actionClasses) {
-		Set<RequestedAction> actions = new HashSet<RequestedAction>();
+		ActionClauses(Collection<Class<? extends RequestedAction>> actionClasses)
+				throws PolicyHelperException {
+			this.clauseList = Collections
+					.singletonList(buildClause(actionClasses));
+		}
+
+		ActionClauses(Class<? extends RequestedAction> actionClass)
+				throws PolicyHelperException {
+			this.clauseList = Collections.singletonList(Collections
+					.singleton(instantiateAction(actionClass)));
+		}
+
+		private void addClause(List<Set<RequestedAction>> list,
+				Class<? extends RequestedAction>[] actionClasses)
+				throws PolicyHelperException {
+			Set<RequestedAction> clause = buildClause(Arrays
+					.asList(actionClasses));
+			if (!clause.isEmpty()) {
+				list.add(clause);
+			}
+		}
+
+		private Set<RequestedAction> buildClause(
+				Collection<Class<? extends RequestedAction>> actionClasses)
+				throws PolicyHelperException {
+			Set<RequestedAction> clause = new HashSet<RequestedAction>();
 			for (Class<? extends RequestedAction> actionClass : actionClasses) {
-			RequestedAction action = instantiateAction(actionClass);
-			if (action == null) {
-				return null;
-			} else {
-				actions.add(action);
+				if (!NoAction.class.equals(actionClass)) {
+					clause.add(instantiateAction(actionClass));
 				}
 			}
-		return actions;
+			return Collections.unmodifiableSet(clause);
 		}
 
 		/**
-	 * Get an instance of the RequestedAction, from its class. If the class
-	 * cannot be instantiated, return null.
+		 * Get an instance of the RequestedAction, from its class, or throw an
+		 * exception.
 		 */
-	private static RequestedAction instantiateAction(
-			Class<? extends RequestedAction> actionClass) {
+		private RequestedAction instantiateAction(
+				Class<? extends RequestedAction> actionClass)
+				throws PolicyHelperException {
 			try {
 				Constructor<? extends RequestedAction> constructor = actionClass
 						.getConstructor();
@@ -203,18 +268,32 @@ public class PolicyHelper {
 			} catch (NoSuchMethodException e) {
 				log.error("'" + actionClass.getName()
 						+ "' does not have a no-argument constructor.");
-			return null;
+				throw new PolicyHelperException();
 			} catch (IllegalAccessException e) {
 				log.error("The no-argument constructor for '"
 						+ actionClass.getName() + "' is not public.");
-			return null;
+				throw new PolicyHelperException();
 			} catch (Exception e) {
-			log.error("Failed to instantiate '" + actionClass.getName() + "'",
-					e);
-			return null;
+				log.error("Failed to instantiate '" + actionClass.getName()
+						+ "'", e);
+				throw new PolicyHelperException();
 			}
 		}
 
+		boolean isEmpty() {
+			return this.clauseList.isEmpty();
+		}
+
+		List<Set<RequestedAction>> getClauseList() {
+			return this.clauseList;
+		}
+	}
+
+	/** We failed to instantiate a RequestedAction */
+	private static class PolicyHelperException extends Exception {
+		// no members
+	}
+
 	/**
 	 * No need to instantiate this helper class - all methods are static.
 	 */

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/VitroHttpServlet.java

@@ -69,8 +69,10 @@ public class VitroHttpServlet extends HttpServlet {
 			if (!PolicyHelper.areRequiredAuthorizationsSatisfied(hreq, this)) {
 				if (LoginStatusBean.getBean(hreq).isLoggedIn()) {
 					redirectToInsufficientAuthorizationPage(hreq, hresp);
+					return;
 				} else {
 					redirectToLoginPage(hreq, hresp);
+					return;
 				}
 			}
 		}
@@ -164,37 +166,6 @@ public class VitroHttpServlet extends HttpServlet {
 		}
 	}
 
-	/**
-	 * If none of these actions are authorized by the current policy, redirect
-	 * them to the appropriate page.
-	 * 
-	 * Currently the RequiresAuthorizationFor annotation can't handle "or"
-	 * situations, so we need to do an explicit call to this method. You should
-	 * still use the annotation with no actions, so we know this is a restricted
-	 * page when we logout.
-	 */
-	public static boolean checkIfAnyActionsAreAuthorized(
-			HttpServletRequest request, HttpServletResponse response,
-			Class<? extends RequestedAction>... actionClasses) {
-		for (Class<? extends RequestedAction> actionClass : actionClasses) {
-			if (PolicyHelper.isAuthorized(request, actionClass)) {
-				log.trace("Authorized for '" + actionClass.getSimpleName()
-						+ "'");
-				return true;
-			}
-		}
-		LoginStatusBean statusBean = LoginStatusBean.getBean(request);
-		if (statusBean.isLoggedIn()) {
-			log.trace("Authorization is insufficient for requested actions");
-			redirectToInsufficientAuthorizationPage(request, response);
-			return false;
-		} else {
-			log.trace("Not logged in; not sufficient for requested actions");
-			redirectToLoginPage(request, response);
-			return false;
-		}
-	}
-	
 	/**
 	 * Logged in, but with insufficent authorization. Send them to the home page
 	 * with a message. They won't be coming back.

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/jena/JenaExportController.java

@@ -20,6 +20,7 @@ import com.hp.hpl.jena.shared.Lock;
 
 import edu.cornell.mannlib.vedit.controller.BaseEditController;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.PolicyHelper.RequiresAuthorizationFor;
+import edu.cornell.mannlib.vitro.webapp.auth.policy.PolicyHelper.RequiresAuthorizationFor.Or;
 import edu.cornell.mannlib.vitro.webapp.auth.requestedAction.usepages.UseAdvancedDataToolsPages;
 import edu.cornell.mannlib.vitro.webapp.auth.requestedAction.usepages.UseOntologyEditorPages;
 import edu.cornell.mannlib.vitro.webapp.controller.Controllers;
@@ -28,17 +29,13 @@ import edu.cornell.mannlib.vitro.webapp.dao.jena.JenaModelUtils;
 import edu.cornell.mannlib.vitro.webapp.dao.jena.ModelContext;
 import edu.cornell.mannlib.vitro.webapp.servlet.setup.JenaDataSourceSetupBase;
 
-@RequiresAuthorizationFor(/* either-or; see call to checkIfAnyActionsAreAuthorized */)
+@RequiresAuthorizationFor(or={@Or(UseAdvancedDataToolsPages.class), @Or(UseOntologyEditorPages.class)})
 public class JenaExportController extends BaseEditController {
 
+	@Override
 	public void doGet( HttpServletRequest request, HttpServletResponse response ) {
 		VitroRequest vreq = new VitroRequest(request);
 		
-		if (!checkIfAnyActionsAreAuthorized(vreq, response,
-				UseAdvancedDataToolsPages.class, UseOntologyEditorPages.class)) {
-			return;
-		}
-		
 		if ( vreq.getRequestURL().indexOf("/download/") > -1 ) { 
 			outputRDF( vreq, response );
 			return;

webapp/src/edu/cornell/mannlib/vitro/webapp/web/jsptags/RequiresAuthorizationFor.java

@@ -2,7 +2,6 @@
 
 package edu.cornell.mannlib.vitro.webapp.web.jsptags;
 
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
@@ -57,12 +56,12 @@ public class RequiresAuthorizationFor extends BodyTagSupport {
 	 * are authorized for those actions.
 	 */
 	private boolean isAuthorized() {
-		Collection<RequestedAction> actions = instantiateActions();
-		if (actions == null) {
+		Set<Class<? extends RequestedAction>> classes = instantiateActionClasses();
+		if (classes == null) {
 			return false;
 		}
 		return PolicyHelper.areRequiredAuthorizationsSatisfied(getRequest(),
-				actions);
+				classes);
 	}
 
 	/**
@@ -71,18 +70,13 @@ public class RequiresAuthorizationFor extends BodyTagSupport {
 	 * 
 	 * If we can't do all of that, complain and return null.
 	 */
-	private Set<RequestedAction> instantiateActions() {
+	private Set<Class<? extends RequestedAction>> instantiateActionClasses() {
 		Set<String> classNames = parseClassNames();
 		if (classNames.isEmpty()) {
 			return Collections.emptySet();
 		}
 
-		Set<Class<? extends RequestedAction>> actionClasses = loadClassesAndCheckTypes(classNames);
-		if (actionClasses == null) {
-			return null;
-		}
-
-		return getInstancesFromClasses(actionClasses);
+		return loadClassesAndCheckTypes(classNames);
 	}
 
 	private Set<String> parseClassNames() {

webapp/test/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelperTest.java

@@ -2,7 +2,8 @@
 
 package edu.cornell.mannlib.vitro.webapp.auth.policy;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
 
 import java.util.Arrays;
 import java.util.HashSet;
@@ -18,6 +19,7 @@ import stubs.javax.servlet.http.HttpSessionStub;
 import edu.cornell.mannlib.vitro.testing.AbstractTestClass;
 import edu.cornell.mannlib.vitro.webapp.auth.identifier.IdentifierBundle;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.PolicyHelper.RequiresAuthorizationFor;
+import edu.cornell.mannlib.vitro.webapp.auth.policy.PolicyHelper.RequiresAuthorizationFor.Or;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.Authorization;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.PolicyDecision;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.PolicyIface;
@@ -80,21 +82,70 @@ public class PolicyHelperTest extends AbstractTestClass {
 	public void twoRequirementsFailOne() {
 		createPolicy(new Action1());
 		assertExpectedAuthorization("requires Actions 1 and 2",
-				Action1Action2Servlet.class, false);
+				Action1AndAction2Servlet.class, false);
 	}
 
 	@Test
 	public void twoRequirementsFailTwo() {
 		createPolicy(new Action2());
 		assertExpectedAuthorization("requires Actions 1 and 2",
-				Action1Action2Servlet.class, false);
+				Action1AndAction2Servlet.class, false);
 	}
 
 	@Test
 	public void twoRequirementsSucceed() {
 		createPolicy(new Action2(), new Action1());
 		assertExpectedAuthorization("requires Actions 1 and 2",
-				Action1Action2Servlet.class, true);
+				Action1AndAction2Servlet.class, true);
+	}
+
+	@Test
+	public void oneOrTwoFail() {
+		createPolicy();
+		assertExpectedAuthorization("requires Action 1 or 2",
+				Action1OrAction2Servlet.class, false);
+	}
+
+	@Test
+	public void oneOrTwoSucceedOne() {
+		createPolicy(new Action1());
+		assertExpectedAuthorization("requires Action 1 or 2",
+				Action1OrAction2Servlet.class, true);
+	}
+
+	@Test
+	public void oneOrTwoSucceedTwo() {
+		createPolicy(new Action2());
+		assertExpectedAuthorization("requires Action 1 or 2",
+				Action1OrAction2Servlet.class, true);
+	}
+
+	@Test
+	public void oneOrTwoOrThreeFail() {
+		createPolicy();
+		assertExpectedAuthorization("requires Action 1 or 2 or 3",
+				Action1OrAction2OrAction3Servlet.class, false);
+	}
+
+	@Test
+	public void oneOrTwoOrThreeSucceedOne() {
+		createPolicy(new Action1());
+		assertExpectedAuthorization("requires Action 1 or 2 or 3",
+				Action1OrAction2OrAction3Servlet.class, true);
+	}
+
+	@Test
+	public void oneOrTwoOrThreeSucceedTwo() {
+		createPolicy(new Action2());
+		assertExpectedAuthorization("requires Action 1 or 2 or 3",
+				Action1OrAction2OrAction3Servlet.class, true);
+	}
+
+	@Test
+	public void oneOrTwoOrThreeSucceedThree() {
+		createPolicy(new Action3());
+		assertExpectedAuthorization("requires Action 1 or 2 or 3",
+				Action1OrAction2OrAction3Servlet.class, true);
 	}
 
 	// ----------------------------------------------------------------------
@@ -124,6 +175,10 @@ public class PolicyHelperTest extends AbstractTestClass {
 		// actions must be public, with public constructor
 	}
 
+	public static class Action3 extends RequestedAction {
+		// actions must be public, with public constructor
+	}
+
 	// no annotation
 	private static class NoAnnotationServlet extends VitroHttpServlet {
 		/* no body */
@@ -140,7 +195,19 @@ public class PolicyHelperTest extends AbstractTestClass {
 	}
 
 	@RequiresAuthorizationFor({ Action1.class, Action2.class })
-	private static class Action1Action2Servlet extends VitroHttpServlet {
+	private static class Action1AndAction2Servlet extends VitroHttpServlet {
+		/* no body */
+	}
+
+	@RequiresAuthorizationFor(value = Action1.class, or = @Or(Action2.class))
+	private static class Action1OrAction2Servlet extends VitroHttpServlet {
+		/* no body */
+	}
+
+	@RequiresAuthorizationFor(value = Action1.class, or = { @Or(Action2.class),
+			@Or(Action3.class) })
+	private static class Action1OrAction2OrAction3Servlet extends
+			VitroHttpServlet {
 		/* no body */
 	}