NIHVIVO-2211 Clean up the logic in ServletPolicyList and the classes that call it.

webapp/config/default.log4j.properties

@@ -33,7 +33,6 @@ log4j.appender.AllAppender.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss,SSS}
 
 log4j.rootLogger=INFO, AllAppender 
 
-log4j.logger.edu.cornell.mannlib.vitro.webapp.auth.policy.ServletPolicyList=WARN
 log4j.logger.edu.cornell.mannlib.vitro.webapp.controller.freemarker.BrowseController=WARN
 log4j.logger.edu.cornell.mannlib.vitro.webapp.dao.jena.pellet.PelletListener=WARN
 log4j.logger.edu.cornell.mannlib.vitro.webapp.dao.jena.RDBGraphGenerator=WARN

webapp/src/edu/cornell/mannlib/vitro/webapp/auth/AuthTestController.java

@@ -14,6 +14,7 @@ import javax.servlet.http.HttpServletResponse;
 
 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.PolicyList;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ServletPolicyList;
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.PolicyDecision;
 import edu.cornell.mannlib.vitro.webapp.auth.requestedAction.ifaces.RequestedAction;
@@ -59,11 +60,11 @@ public class AuthTestController extends VitroHttpServlet {
 
     private void checkAuths(ServletOutputStream out, IdentifierBundle ids, ServletContext servletContext)
     throws IOException{
-        ServletPolicyList policy = ServletPolicyList.getPolicies(servletContext);
+        PolicyList policy = ServletPolicyList.getPolicies(servletContext);
         out.println("<h1>Authorization tests:</h1>");
 
-        if( policy == null ) { out.println("No Policy objects found in ServletContext. ");
+		if (policy.isEmpty()) {
-
+			out.println("No Policy objects found in ServletContext. ");
 		}
         out.println("<table>");
         for(RequestedAction action: actions){

webapp/src/edu/cornell/mannlib/vitro/webapp/auth/AuthorizationHelper.java

@@ -51,11 +51,6 @@ public class AuthorizationHelper {
         PolicyIface policy = RequestPolicyList.getPolicies(vreq);
         if (isEmptyPolicy(policy)) {
             policy = ServletPolicyList.getPolicies(servletContext);
-            if (isEmptyPolicy(policy)) {
-                log.error("No policy found in request at "
-                        + RequestPolicyList.POLICY_LIST);
-                policy = new PolicyList();
-            }
         }
 
         return policy;

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

@@ -375,9 +375,7 @@ public class JenaNetidPolicy extends DefaultInconclusivePolicy implements Visiti
                     log.error("could not get jenaOntModel from JenaBaseDao, JenaNetidPolicy will not work");
                 }
 
-                JenaNetidPolicy jnip = new JenaNetidPolicy(model);
+                ServletPolicyList.addPolicy(sce.getServletContext(), new JenaNetidPolicy(model));
-                ServletPolicyList spl = ServletPolicyList.getPolicies(sce.getServletContext());
-                spl.add(jnip);
 
                 ActiveIdentifierBundleFactories.addFactory(sce, new SelfEditingIdentifierFactory());
             }catch(Exception e){

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

@@ -3,6 +3,7 @@
 package edu.cornell.mannlib.vitro.webapp.auth.policy;
 
 import java.util.ArrayList;
+import java.util.Collection;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -22,9 +23,7 @@ import edu.cornell.mannlib.vitro.webapp.auth.requestedAction.ifaces.RequestedAct
  *  or null decisions will be ignored and the next policy on the list will
  *  be queried. 
  *   
- *   
  * @author bdc34
- *
  */
 public class PolicyList extends ArrayList<PolicyIface> implements PolicyIface{
     private static final Log log = LogFactory.getLog(PolicyList.class.getName());
@@ -33,6 +32,11 @@ public class PolicyList extends ArrayList<PolicyIface> implements PolicyIface{
         super();
     }
 
+	public PolicyList(Collection<PolicyIface> policies) {
+		super(policies);
+	}
+
+	@Override
 	public PolicyDecision isAuthorized(IdentifierBundle whoToAuth, RequestedAction whatToAuth) {
         PolicyDecision pd = null;
         for(PolicyIface policy : this){ 
@@ -48,7 +52,6 @@ public class PolicyList extends ArrayList<PolicyIface> implements PolicyIface{
                 } else{
                     log.debug("policy " + policy.toString() + " returned a null PolicyDecision");
                 }
-
             }catch(Throwable th){
                 log.error("ignoring exception in policy " + policy.toString(), th );
             }

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

@@ -11,94 +11,105 @@ import org.apache.commons.logging.LogFactory;
 
 import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.PolicyIface;
 
+/**
+ * This maintains a PolicyList in the ServletContext. As a rule, however, this
+ * is only used as the basis for the RequestPolicyList. Client code that wants
+ * to access the current list of policies should look there.
+ */
+public class ServletPolicyList {
+	private static final String ATTRIBUTE_POLICY_LIST = ServletPolicyList.class.getName();
+	private static final Log log = LogFactory.getLog(ServletPolicyList.class);
 
 	/**
- * This is a PolicyList that can do isAuthorized and stashes a singleton
+	 * Get a copy of the current list of policies. This method may return an
- * in the ServletContext.
+	 * empty list, but it never returns null.
- * 
- * The intent of this class is to allow a single point for policies
- * in a ServletContext.  example:
- * <code>
- * Authorization canIDoIt = ServletPolicyList.getPolicies( getServletContext() ).isAuthorized( IdBundle, action );
- * </code>
- * 
- * @author bdc34
- *
 	 */
-public class ServletPolicyList extends PolicyList {
+	public static PolicyList getPolicies(ServletContext sc) {
-    protected static String POLICY_LIST = "policy_list";
+		return new PolicyList(getPolicyList(sc));
-    private static final Log log = LogFactory.getLog(ServletPolicyList.class.getName());
+	}
 
 	/**
-     * This is for general public use to get a list of policies for the ServletContext.
+	 * Add the policy to the end of the list.
-     * @param sc
-     * @return
 	 */
-    @SuppressWarnings("unchecked")
-    public static ServletPolicyList getPolicies(ServletContext sc){
-        ServletPolicyList list  = null;
-        try{
-            list = (ServletPolicyList)sc.getAttribute(POLICY_LIST);
-        }catch(ClassCastException cce){
-            log.error(POLICY_LIST +" server context attribute was not of type List<PolicyIface>");
-        }
-        if( list == null ){
-            list = new ServletPolicyList();
-            sc.setAttribute(POLICY_LIST, list);
-        }
-        return list;
-    }
-
 	public static void addPolicy(ServletContext sc, PolicyIface policy) {
-        ServletPolicyList policies = getPolicies(sc);
+		if (policy == null) {
+			return;
+		}
+		
+		PolicyList policies = getPolicyList(sc);
 		if (!policies.contains(policy)) {
 			policies.add(policy);
-            log.info("Added policy: " + policy.toString());
+			log.info("Added policy: " + policy.getClass().getSimpleName());
+			log.debug("Added policy: " + policy.toString());
 		} else {
-            log.info("Ignored attempt to add redundent policy.");
+			log.warn("Ignored attempt to add redundant policy.");
 		}
 	}
 
 	/**
-     * This adds the policy to the front of the list but it may be moved further down
+	 * Add the policy to the front of the list. It may be moved further down the
-     * the list by other policies that are later added using this method.
+	 * list by other policies that are later added using this method.
 	 */
 	public static void addPolicyAtFront(ServletContext sc, PolicyIface policy) {
-        ServletPolicyList policies = getPolicies(sc);
+		if (policy == null) {
+			return;
+		}
+		
+		PolicyList policies = getPolicyList(sc);
 		if (!policies.contains(policy)) {
 			policies.add(0, policy);
-            log.info("Added policy at front of ServletPolicyList: " + policy.toString());
+			log.info("Added policy at front: " + policy.getClass().getSimpleName());
+			log.debug("Added policy at front: " + policy.toString());
 		} else {
-            log.info("Ignored attempt to add redundent policy.");
+			log.warn("Ignored attempt to add redundant policy.");
 		}
 	}
 
-    /** import edu.cornell.mannlib.vitro.webapp.auth.policy.ifaces.VisitingPolicyIface;
+	/**
-     * Replace first instance of policy found in policy list.  If no instance
+	 * Replace the first instance of this class of policy in the list. If no
-     * is found in list add at end of the list.
+	 * instance is found, add the policy to the end of the list.
-     * 
-     * @param sc
-     * @param policy
 	 */
 	public static void replacePolicy(ServletContext sc, PolicyIface policy) {
-        if( sc == null ) 
+		if (policy == null) {
-            throw new IllegalArgumentException( "replacePolicy() needs a non-null ServletContext");
-        if( policy == null )
 			return;
-        Class clzz = policy.getClass();
+		}
 
-        ServletPolicyList spl = ServletPolicyList.getPolicies(sc);
+		Class<?> clzz = policy.getClass();
-        ListIterator<PolicyIface> it = spl.listIterator();
+		PolicyList policies = getPolicyList(sc);
-        boolean replaced = false;
+		ListIterator<PolicyIface> it = policies.listIterator();
 		while (it.hasNext()) {
-            PolicyIface p = (PolicyIface)it.next();            
+			if (clzz.isAssignableFrom(it.next().getClass())) {
-            if( clzz.isAssignableFrom(p.getClass()) ){
 				it.set(policy);
-                replaced = true;
+				return;
 			}
 		}
-        if( ! replaced ){
+
-            ServletPolicyList.addPolicy(sc, policy);
+		addPolicy(sc, policy);
 	}
+
+	/**
+	 * Get the current PolicyList from the context, or create one if there is
+	 * none. This method may return an empty list, but it never returns null.
+	 */
+	private static PolicyList getPolicyList(ServletContext ctx) {
+		if (ctx == null) {
+			throw new NullPointerException("ctx may not be null.");
 		}
+
+		Object obj = ctx.getAttribute(ATTRIBUTE_POLICY_LIST);
+		if (obj == null) {
+			obj = new PolicyList();
+			ctx.setAttribute(ATTRIBUTE_POLICY_LIST, obj);
+		}
+
+		if (!(obj instanceof PolicyList)) {
+			throw new IllegalStateException("Expected to find an instance of "
+					+ PolicyList.class.getName()
+					+ " in the context, but found an instance of "
+					+ obj.getClass().getName() + " instead.");
+		}
+
+		return (PolicyList) obj;
+	}
+
 }

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

@@ -93,10 +93,6 @@ public class PropertyEditLinks extends TagSupport{
         PolicyIface policy = RequestPolicyList.getPolicies(pageContext.getRequest());
         if( policy == null || ( policy instanceof PolicyList && ((PolicyList)policy).size() == 0 )){
             policy = ServletPolicyList.getPolicies( pageContext.getServletContext() );
-            if( policy == null || ( policy instanceof PolicyList && ((PolicyList)policy).size() == 0 )){            
-                log.error("No policy found in request at " + RequestPolicyList.POLICY_LIST);
-                return SKIP_BODY;
-            }
         }              
         
 		IdentifierBundle ids = RequestIdentifiers.getIdBundleForRequest(pageContext.getRequest());

webapp/src/edu/cornell/mannlib/vitro/webapp/web/templatemodels/individual/EditingPolicyHelper.java

@@ -38,9 +38,6 @@ public class EditingPolicyHelper {
         policy = RequestPolicyList.getPolicies(vreq);
         if( policy == null || ( policy instanceof PolicyList && ((PolicyList)policy).size() == 0 )){
             policy = ServletPolicyList.getPolicies( servletContext );
-            if( policy == null || ( policy instanceof PolicyList && ((PolicyList)policy).size() == 0 )){            
-                log.error("No policy found in request at " + RequestPolicyList.POLICY_LIST);
-            }
         }           
     }
     

webapp/web/edit/selfeditcheck.jsp

@@ -28,7 +28,7 @@
 
 <h3>Is there a self editing policy in the context?</h3>
 <% 
-ServletPolicyList spl = ServletPolicyList.getPolicies(application);
+PolicyList spl = ServletPolicyList.getPolicies(application);
 SelfEditingPolicy sePolicy = null;
 ListIterator it = spl.listIterator();
 String found = "Could not find a SelfEditingPolicy";