From d246b7b9e0336c26bc3c2d235a2fac8208db2693 Mon Sep 17 00:00:00 2001 From: j2blake Date: Mon, 18 Apr 2011 17:31:02 +0000 Subject: [PATCH] NIHVIVO-2492 A better implementation to allow RequiresAuthorizationFor this or that. --- .../webapp/auth/policy/PolicyHelper.java | 275 +++++++++++------- .../webapp/controller/VitroHttpServlet.java | 33 +-- .../controller/jena/JenaExportController.java | 9 +- .../web/jsptags/RequiresAuthorizationFor.java | 16 +- .../webapp/auth/policy/PolicyHelperTest.java | 77 ++++- 5 files changed, 259 insertions(+), 151 deletions(-) diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelper.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelper.java index 61d35c43b..090c5053f 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelper.java +++ b/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): + * + * 
+	 * #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)})
+	 *
*/ @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.TYPE) @@ -49,7 +63,14 @@ public class PolicyHelper { /* no fields */ } + @Retention(RetentionPolicy.RUNTIME) + public static @interface Or { + Class[] value() default NoAction.class; + } + Class[] value() default NoAction.class; + + Or[] or() default @Or(); } /** @@ -57,7 +78,11 @@ public class PolicyHelper { */ public static boolean isRestrictedPage(VitroHttpServlet servlet) { Class 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 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 servletClass) { - return areRequiredAuthorizationsSatisfied(req, - getRequiredAuthorizationsForServlet(servletClass)); + try { + return isActionClausesAuthorized(req, + ActionClauses.forServletClass(servletClass)); + } catch (PolicyHelperException e) { + return false; + } } /** @@ -87,132 +115,183 @@ public class PolicyHelper { */ public static boolean areRequiredAuthorizationsSatisfied( HttpServletRequest req, - Class... actionClasses) { - List> classList = Arrays - .asList(actionClasses); - - Set actions = instantiateActions(classList); - if (actions == null) { - log.debug("not authorized: failed to instantiate actions"); + Collection> 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 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 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 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 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 getRequiredAuthorizationsForServlet( - Class clazz) { - Set result = new HashSet(); + private static class ActionClauses { + static ActionClauses forServletClass( + Class servletClass) + throws PolicyHelperException { + return new ActionClauses( + servletClass.getAnnotation(RequiresAuthorizationFor.class)); + } - RequiresAuthorizationFor annotation = clazz - .getAnnotation(RequiresAuthorizationFor.class); + private final List> clauseList; - if (annotation != null) { - for (Class actionClass : annotation - .value()) { - if (NoAction.class != actionClass) { - RequestedAction action = instantiateAction(actionClass); - if (action != null) { - result.add(action); - } + ActionClauses(RequiresAuthorizationFor annotation) + throws PolicyHelperException { + List> list = new ArrayList>(); + if (annotation != null) { + addClause(list, annotation.value()); + for (Or orAnnotation : annotation.or()) { + addClause(list, orAnnotation.value()); } } + this.clauseList = Collections.unmodifiableList(list); } - return result; - } - /** - * Instantiate actions from their classes. If any one of the classes cannot - * be instantiated, return null. - */ - private static Set instantiateActions( - Collection> actionClasses) { - Set actions = new HashSet(); - for (Class actionClass : actionClasses) { - RequestedAction action = instantiateAction(actionClass); - if (action == null) { - return null; - } else { - actions.add(action); + ActionClauses(Collection> actionClasses) + throws PolicyHelperException { + this.clauseList = Collections + .singletonList(buildClause(actionClasses)); + } + + ActionClauses(Class actionClass) + throws PolicyHelperException { + this.clauseList = Collections.singletonList(Collections + .singleton(instantiateAction(actionClass))); + } + + private void addClause(List> list, + Class[] actionClasses) + throws PolicyHelperException { + Set clause = buildClause(Arrays + .asList(actionClasses)); + if (!clause.isEmpty()) { + list.add(clause); } } - return actions; + + private Set buildClause( + Collection> actionClasses) + throws PolicyHelperException { + Set clause = new HashSet(); + for (Class actionClass : actionClasses) { + if (!NoAction.class.equals(actionClass)) { + clause.add(instantiateAction(actionClass)); + } + } + return Collections.unmodifiableSet(clause); + } + + /** + * Get an instance of the RequestedAction, from its class, or throw an + * exception. + */ + private RequestedAction instantiateAction( + Class actionClass) + throws PolicyHelperException { + try { + Constructor constructor = actionClass + .getConstructor(); + RequestedAction instance = constructor.newInstance(); + return instance; + } catch (NoSuchMethodException e) { + log.error("'" + actionClass.getName() + + "' does not have a no-argument constructor."); + throw new PolicyHelperException(); + } catch (IllegalAccessException e) { + log.error("The no-argument constructor for '" + + actionClass.getName() + "' is not public."); + throw new PolicyHelperException(); + } catch (Exception e) { + log.error("Failed to instantiate '" + actionClass.getName() + + "'", e); + throw new PolicyHelperException(); + } + } + + boolean isEmpty() { + return this.clauseList.isEmpty(); + } + + List> getClauseList() { + return this.clauseList; + } } - /** - * Get an instance of the RequestedAction, from its class. If the class - * cannot be instantiated, return null. - */ - private static RequestedAction instantiateAction( - Class actionClass) { - try { - Constructor constructor = actionClass - .getConstructor(); - RequestedAction instance = constructor.newInstance(); - return instance; - } catch (NoSuchMethodException e) { - log.error("'" + actionClass.getName() - + "' does not have a no-argument constructor."); - return null; - } catch (IllegalAccessException e) { - log.error("The no-argument constructor for '" - + actionClass.getName() + "' is not public."); - return null; - } catch (Exception e) { - log.error("Failed to instantiate '" + actionClass.getName() + "'", - e); - return null; - } + /** We failed to instantiate a RequestedAction */ + private static class PolicyHelperException extends Exception { + // no members } /** diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/controller/VitroHttpServlet.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/controller/VitroHttpServlet.java index 76a3ba9a4..22dbd5c8e 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/controller/VitroHttpServlet.java +++ b/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... actionClasses) { - for (Class 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. diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/controller/jena/JenaExportController.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/controller/jena/JenaExportController.java index 3476b63a8..47fa100ff 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/controller/jena/JenaExportController.java +++ b/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; diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/web/jsptags/RequiresAuthorizationFor.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/web/jsptags/RequiresAuthorizationFor.java index 5d780452c..055f40329 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/web/jsptags/RequiresAuthorizationFor.java +++ b/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 actions = instantiateActions(); - if (actions == null) { + Set> 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 instantiateActions() { + private Set> instantiateActionClasses() { Set classNames = parseClassNames(); if (classNames.isEmpty()) { return Collections.emptySet(); } - Set> actionClasses = loadClassesAndCheckTypes(classNames); - if (actionClasses == null) { - return null; - } - - return getInstancesFromClasses(actionClasses); + return loadClassesAndCheckTypes(classNames); } private Set parseClassNames() { diff --git a/webapp/test/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelperTest.java b/webapp/test/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelperTest.java index 3cead99c1..91c3ab376 100644 --- a/webapp/test/edu/cornell/mannlib/vitro/webapp/auth/policy/PolicyHelperTest.java +++ b/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 */ }