From 704f1b8a672603a63013ca00dce640b7770e8bca Mon Sep 17 00:00:00 2001
From: Scott Battaglia <scott.battaglia@gmail.com>
Date: Mon, 20 Sep 2010 03:42:51 +0000
Subject: [PATCH] CASC-33

fixed test that was failing (my guess is its due to the different ways the JVMs return arrays).  Also enhanced code to use "Best practice" of never returning null and instead returning an empty list simplifying upstream code.
---
 .../tomcat/AssertionCasRealmDelegate.java     | 61 ++++++++++---------
 .../PropertiesCasRealmDelegateTests.java      | 15 +++--
 2 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/cas-client-integration-tomcat-common/src/main/java/org/jasig/cas/client/tomcat/AssertionCasRealmDelegate.java b/cas-client-integration-tomcat-common/src/main/java/org/jasig/cas/client/tomcat/AssertionCasRealmDelegate.java
index 2a0ac37..613a989 100644
--- a/cas-client-integration-tomcat-common/src/main/java/org/jasig/cas/client/tomcat/AssertionCasRealmDelegate.java
+++ b/cas-client-integration-tomcat-common/src/main/java/org/jasig/cas/client/tomcat/AssertionCasRealmDelegate.java
@@ -6,14 +6,17 @@
 package org.jasig.cas.client.tomcat;
 
 import java.security.Principal;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.jasig.cas.client.authentication.AttributePrincipal;
+import org.jasig.cas.client.util.CommonUtils;
 
 /**
- * {@link CasRealm} implementation with prinicpal and role data backed by the {@link Assertion}.
+ * {@link CasRealm} implementation with prinicpal and role data backed by the {@link org.jasig.cas.client.validation.Assertion}.
  * In particular, an {@link AttributePrincipal} is expected from which the values of
  * the role attribute are retrieved.  The default role attribute name is "role",
  * but this can be customized via {@link #setRoleAttributeName(String)}.
@@ -49,40 +52,40 @@ public class AssertionCasRealmDelegate implements CasRealm {
 
     /** {@inheritDoc} */
     public String[] getRoles(final Principal p) {
-        if (p instanceof AttributePrincipal) {
-            final Collection roles = getRoleCollection(p);
-            if (roles != null) {
-                final String[] array = new String[roles.size()];
-                roles.toArray(array);
-                return array;
-            } else {
-                return new String[0];
-            }
-        } else {
-            throw new IllegalArgumentException("Expected instance of AttributePrincipal but got " + p);
-        }
+        CommonUtils.assertTrue(p instanceof AttributePrincipal, "Expected instance of AttributePrincipal but got " + p.getClass());
+
+        final Collection roles = getRoleCollection(p);
+        final String[] array = new String[roles.size()];
+        roles.toArray(array);
+        return array;
     }
 
     /** {@inheritDoc} */
     public boolean hasRole(final Principal principal, final String role) {
-        final Collection roles = getRoleCollection(principal);
-        if (roles != null) {
-            return roles.contains(role);
-        } else {
-            return false;
-        }
+        return getRoleCollection(principal).contains(role);
     }
-    
+
+    /**
+     * Retrieves the attributes for a Principal.  To make life easy this should NEVER return null.
+     *
+     * @param p the principal to check.
+     * @return the list of attribute values that matched this role, or an empty collection if they don't.
+     */
     private Collection getRoleCollection(final Principal p) {
-        if (p instanceof AttributePrincipal) {
-            final Collection attributes =
-                (Collection) ((AttributePrincipal) p).getAttributes().get(roleAttributeName);
-            if (attributes == null) {
-                log.debug(p + " has no attribute named " + roleAttributeName);
-            }
-            return attributes;
-        } else {
-            return null;
+        if (!(p instanceof AttributePrincipal)) {
+            return Collections.emptyList();
         }
+
+        final Object attributes = ((AttributePrincipal) p).getAttributes().get(this.roleAttributeName);
+
+        if (attributes == null) {
+            return Collections.emptyList();
+        }
+
+        if (attributes instanceof Collection) {
+            return (Collection) attributes;
+        }
+
+        return Arrays.asList(new Object[] {attributes});
     }
 }
diff --git a/cas-client-integration-tomcat-common/src/test/java/org/jasig/cas/client/tomcat/PropertiesCasRealmDelegateTests.java b/cas-client-integration-tomcat-common/src/test/java/org/jasig/cas/client/tomcat/PropertiesCasRealmDelegateTests.java
index 6aad839..e7f272d 100644
--- a/cas-client-integration-tomcat-common/src/test/java/org/jasig/cas/client/tomcat/PropertiesCasRealmDelegateTests.java
+++ b/cas-client-integration-tomcat-common/src/test/java/org/jasig/cas/client/tomcat/PropertiesCasRealmDelegateTests.java
@@ -6,6 +6,9 @@
 package org.jasig.cas.client.tomcat;
 
 import java.security.Principal;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
 
 import junit.framework.TestCase;
 
@@ -35,11 +38,13 @@ public class PropertiesCasRealmDelegateTests extends TestCase {
     
     public void testGetRoles() {
         final Principal p = new AttributePrincipalImpl("rosencrantz");
-        final String[] expected = new String[] {"admins", "users"};
-        final String[] actual = realm.getRoles(p);
-        assertEquals(expected.length, actual.length);
-        for (int i = 0; i < expected.length; i++) {
-            assertEquals(expected[i], actual[i]);
+        final List expected = Arrays.asList(new String[] {"admins", "users"});
+        final List actual = Arrays.asList(realm.getRoles(p));
+        assertEquals(expected.size(), actual.size());
+
+        for (final Iterator iter = expected.iterator(); iter.hasNext();) {
+            final Object value = iter.next();
+            assertTrue(actual.contains(value));
         }
     }