From 63ede81450709d9ba505e9cbb32f08dd98c9cf5d Mon Sep 17 00:00:00 2001
From: Brian Major <brian.major@nrc-cnrc.gc.ca>
Date: Mon, 15 Sep 2014 12:23:20 -0700
Subject: [PATCH] s1651 - Added caching to GMSClient

---
 .../nrc/cadc/ac/server/ldap/LdapGroupDAO.java |  40 ++--
 .../cadc/ac/server/ldap/LdapGroupDAOTest.java |  28 +--
 projects/cadcAccessControl/build.xml          |  31 +---
 .../src/ca/nrc/cadc/ac/AC.java                |   6 +-
 .../src/ca/nrc/cadc/ac/Group.java             |   6 -
 .../src/ca/nrc/cadc/ac/GroupReader.java       |  16 +-
 .../src/ca/nrc/cadc/ac/GroupWriter.java       |  12 +-
 .../src/ca/nrc/cadc/ac/client/GMSClient.java  |  98 ++++++++--
 .../ca/nrc/cadc/ac/GroupReaderWriterTest.java |  17 +-
 .../test/src/ca/nrc/cadc/ac/GroupTest.java    |  10 +-
 .../ca/nrc/cadc/ac/client/GMSClientTest.java  | 174 ++++++++++++++++++
 11 files changed, 303 insertions(+), 135 deletions(-)
 create mode 100644 projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/client/GMSClientTest.java

diff --git a/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java b/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java
index a55ffe84..ac540a0a 100755
--- a/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java
+++ b/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java
@@ -68,6 +68,17 @@
  */
 package ca.nrc.cadc.ac.server.ldap;
 
+import java.security.AccessControlException;
+import java.security.Principal;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Date;
+import java.util.List;
+
+import javax.security.auth.x500.X500Principal;
+
+import org.apache.log4j.Logger;
+
 import ca.nrc.cadc.ac.ActivatedGroup;
 import ca.nrc.cadc.ac.Group;
 import ca.nrc.cadc.ac.GroupAlreadyExistsException;
@@ -76,6 +87,7 @@ import ca.nrc.cadc.ac.Role;
 import ca.nrc.cadc.ac.User;
 import ca.nrc.cadc.ac.UserNotFoundException;
 import ca.nrc.cadc.net.TransientException;
+
 import com.unboundid.ldap.sdk.AddRequest;
 import com.unboundid.ldap.sdk.Attribute;
 import com.unboundid.ldap.sdk.DN;
@@ -90,15 +102,6 @@ import com.unboundid.ldap.sdk.SearchResult;
 import com.unboundid.ldap.sdk.SearchResultEntry;
 import com.unboundid.ldap.sdk.SearchScope;
 import com.unboundid.ldap.sdk.controls.ProxiedAuthorizationV2RequestControl;
-import java.security.AccessControlException;
-import java.security.Principal;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Date;
-import java.util.List;
-import java.util.logging.Level;
-import javax.security.auth.x500.X500Principal;
-import org.apache.log4j.Logger;
 
 public class LdapGroupDAO<T extends Principal> extends LdapDAO
 {
@@ -112,9 +115,6 @@ public class LdapGroupDAO<T extends Principal> extends LdapDAO
             "(version 3.0;acl \"Group Write\";allow " + 
             "(read,compare,search,selfwrite,write,add)" + 
             "(groupdn = \"ldap:///<ACTUAL_GROUP>\");)";
-    private static final String PUB_GROUP_ACI = "(targetattr = \"*\") " + 
-            "(version 3.0;acl \"Group Public\";" + 
-            "allow (read,compare,search)userdn=\"ldap:///all\";)";
     
     private LdapUserDAO<T> userPersist;
 
@@ -185,7 +185,6 @@ public class LdapGroupDAO<T extends Principal> extends LdapDAO
                             new ActivatedGroup(modifiedGroup.getID(),
                                                modifiedGroup.getOwner());
                     activatedGroup.description = modifiedGroup.description;
-                    activatedGroup.publicRead = modifiedGroup.publicRead;
                     activatedGroup.groupRead = modifiedGroup.groupRead;
                     activatedGroup.groupWrite = modifiedGroup.groupWrite;
                     activatedGroup.getProperties()
@@ -240,10 +239,6 @@ public class LdapGroupDAO<T extends Principal> extends LdapDAO
 
                 // acis
                 List<String> acis = new ArrayList<String>();
-                if (group.publicRead)
-                {
-                    acis.add(PUB_GROUP_ACI);
-                }
                 if (groupWriteAci != null)
                 {
                     acis.add(groupWriteAci);
@@ -428,10 +423,6 @@ public class LdapGroupDAO<T extends Principal> extends LdapDAO
                             Group groupWrite = getGroup(grWrite.trim());
                             ldapGroup.groupWrite = groupWrite;
                         }
-                        else if (aci.equals(PUB_GROUP_ACI))
-                        {
-                            ldapGroup.publicRead = true;
-                        }
                     }
                 }
             }
@@ -529,10 +520,6 @@ public class LdapGroupDAO<T extends Principal> extends LdapDAO
                                              writeGrDN.toNormalizedString()));
         }
 
-        if (newGroup.publicRead)
-        {
-            acis.add(PUB_GROUP_ACI);
-        }
         modifs.add(new Modification(ModificationType.REPLACE, "aci", (String[]) 
                                     acis.toArray(new String[acis.size()])));
 
@@ -645,8 +632,7 @@ public class LdapGroupDAO<T extends Principal> extends LdapDAO
         }
         
         if (group.groupRead != null || 
-            group.groupWrite != null || 
-            group.publicRead)
+            group.groupWrite != null)
         {
             modifs.add(new Modification(ModificationType.DELETE, "aci"));
         }
diff --git a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java
index 48de04ef..94081f73 100644
--- a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java
+++ b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java
@@ -34,17 +34,24 @@
 
 package ca.nrc.cadc.ac.server.ldap;
 
-import ca.nrc.cadc.ac.ActivatedGroup;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
+import java.security.AccessControlException;
 import java.security.PrivilegedExceptionAction;
+import java.util.Collection;
+import java.util.Set;
 
 import javax.security.auth.Subject;
 import javax.security.auth.x500.X500Principal;
 
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.junit.BeforeClass;
 import org.junit.Test;
 
+import ca.nrc.cadc.ac.ActivatedGroup;
 import ca.nrc.cadc.ac.Group;
 import ca.nrc.cadc.ac.GroupAlreadyExistsException;
 import ca.nrc.cadc.ac.GroupNotFoundException;
@@ -53,14 +60,6 @@ import ca.nrc.cadc.ac.Role;
 import ca.nrc.cadc.ac.User;
 import ca.nrc.cadc.ac.UserNotFoundException;
 import ca.nrc.cadc.util.Log4jInit;
-import java.security.AccessControlException;
-import java.security.Principal;
-import java.util.Collection;
-import java.util.Set;
-import org.apache.log4j.Level;
-import org.apache.log4j.Logger;
-import static org.junit.Assert.fail;
-import org.junit.BeforeClass;
 
 public class LdapGroupDAOTest
 {
@@ -177,15 +176,6 @@ public class LdapGroupDAOTest
                     actualGroup = getGroupDAO().modifyGroup(expectGroup);
                     assertGroupsEqual(expectGroup, actualGroup);
 
-                    // publicRead
-                    expectGroup.publicRead = true;
-                    actualGroup = getGroupDAO().modifyGroup(expectGroup);
-                    assertGroupsEqual(expectGroup, actualGroup);
-                    
-                    expectGroup.publicRead = false;
-                    actualGroup = getGroupDAO().modifyGroup(expectGroup);
-                    assertGroupsEqual(expectGroup, actualGroup);
-
                     // userMembers
                     expectGroup.getUserMembers().add(daoTestUser2);
                     actualGroup = getGroupDAO().modifyGroup(expectGroup);
@@ -208,7 +198,6 @@ public class LdapGroupDAOTest
                     expectGroup.description = "Happy testing";
                     expectGroup.groupRead = otherGroup;
                     expectGroup.groupWrite = otherGroup;
-                    expectGroup.publicRead = true;
                     expectGroup.getUserMembers().add(daoTestUser2);
                     expectGroup.getGroupMembers().add(otherGroup);
                     
@@ -662,7 +651,6 @@ public class LdapGroupDAOTest
         {
             assertTrue(gr2.getUserMembers().contains(user));
         }
-        assertEquals(gr1.publicRead, gr2.publicRead);
         assertEquals(gr1.groupRead, gr2.groupRead);
         assertEquals(gr1.groupWrite, gr2.groupWrite);
         assertEquals(gr1.groupWrite, gr2.groupWrite);
diff --git a/projects/cadcAccessControl/build.xml b/projects/cadcAccessControl/build.xml
index 9d71805d..7b1f362a 100644
--- a/projects/cadcAccessControl/build.xml
+++ b/projects/cadcAccessControl/build.xml
@@ -87,14 +87,15 @@
 
     <property name="project"    value="cadcAccessControl" />
 
-    <property name="cadcUtil"   value="${lib}/cadcUtil.jar" />
+    <property name="cadcUtil"           value="${lib}/cadcUtil.jar" />
+	<property name="cadcRegistryClient" value="${lib}/cadcRegistryClient.jar" />
     
     <property name="jdom2"      value="${ext.lib}/jdom2.jar" />
     <property name="log4j"      value="${ext.lib}/log4j.jar" />
     <property name="unboundid"  value="${ext.lib}/unboundid-ldapsdk-se.jar" />
 
 
-    <property name="jars" value="${cadcUtil}:${jdom2}:${log4j}:${unboundid}" />
+    <property name="jars" value="${cadcUtil}:${cadcRegistryClient}:${jdom2}:${log4j}:${unboundid}" />
     
     <target name="build" depends="compile">
         <jar jarfile="${build}/lib/${project}.jar"
@@ -114,30 +115,4 @@
     
     <property name="testingJars" value="${build}/class:${jars}:${xerces}:${asm}:${cglib}:${easymock}:${junit}:${objenesis}" />
 
-    <target name="test" depends="compile-test">
-        <echo message="Running test" />
-
-        <!-- Run the junit test suite -->
-        <echo message="Running test suite..." />
-        <junit printsummary="yes" haltonfailure="yes" fork="yes">
-            <classpath>
-                <pathelement path="${build}/class"/>
-                <pathelement path="${build}/test/class"/>
-                <pathelement path="${testingJars}"/>
-            </classpath>
-            <test name="ca.nrc.cadc.ac.GroupTest" />
-            <test name="ca.nrc.cadc.ac.GroupPropertyTest" />
-            <test name="ca.nrc.cadc.ac.GroupPropertyReaderWriterTest" />
-            <test name="ca.nrc.cadc.ac.GroupReaderWriterTest" />
-            <test name="ca.nrc.cadc.ac.IdentityReaderWriterTest" />
-            <test name="ca.nrc.cadc.ac.PersonalDetailsTest" />
-            <test name="ca.nrc.cadc.ac.PosixDetailsTest" />
-            <test name="ca.nrc.cadc.ac.UserDetailsReaderWriterTest" />
-            <test name="ca.nrc.cadc.ac.UserReaderWriterTest" />
-            <test name="ca.nrc.cadc.ac.UserTest" />
-            
-            <formatter type="plain" usefile="false" />
-        </junit>
-    </target>
-
 </project>
diff --git a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/AC.java b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/AC.java
index 44c83bb7..afbf97f6 100755
--- a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/AC.java
+++ b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/AC.java
@@ -85,13 +85,9 @@ public class AC
     // Denotes a group readable by public
     public static final String PROPERTY_PUBLIC = "ivo://ivoa.net/gms#public";
     
-    public static final String GMS_SERVICE_URI = "ivo://cadc.nrc.ca/ac";
+    public static final String GMS_SERVICE_URI = "ivo://cadc.nrc.ca/gms";
     
     // Group URI attribute once the group name is appended
     public static final String GROUP_URI = "ivo://cadc.nrc.ca/gms#";
     
-//    public static final String ID_TYPE_X500 = "X500";
-//    public static final String ID_TYPE_OPENID = "OpenID";
-//    public static final String ID_TYPE_USERNAME = "HTTP";
-//    public static final String ID_TYPE_UID = "UID";
 }
diff --git a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java
index f4e11a97..5172ddb9 100644
--- a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java
+++ b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java
@@ -104,12 +104,6 @@ public class Group
      */
     public Group groupWrite;
     
-    /**
-     * flag that show whether the details of this group are publicly readable
-     * Note: this class does not enforce any access control rules
-     */
-    public boolean publicRead = false;
-    
     /**
      * Ctor.
      * 
diff --git a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupReader.java b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupReader.java
index 7e51c1dd..4b666bf5 100755
--- a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupReader.java
+++ b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupReader.java
@@ -68,8 +68,6 @@
  */
 package ca.nrc.cadc.ac;
 
-import ca.nrc.cadc.date.DateUtil;
-import ca.nrc.cadc.xml.XmlUtil;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
@@ -81,11 +79,14 @@ import java.security.Principal;
 import java.text.DateFormat;
 import java.text.ParseException;
 import java.util.List;
-import java.util.Set;
+
 import org.jdom2.Document;
 import org.jdom2.Element;
 import org.jdom2.JDOMException;
 
+import ca.nrc.cadc.date.DateUtil;
+import ca.nrc.cadc.xml.XmlUtil;
+
 public class GroupReader
 {
 
@@ -237,14 +238,7 @@ public class GroupReader
             }
 
         }
-
-        // publicRead
-        Element publicReadElement = groupElement.getChild("publicRead");
-        if (publicReadElement != null)
-        {
-            group.publicRead = Boolean.valueOf(publicReadElement.getText());
-        }
-
+        
         // properties
         Element propertiesElement = groupElement.getChild("properties");
         if (propertiesElement != null)
diff --git a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupWriter.java b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupWriter.java
index ff3ae502..3c4e5447 100755
--- a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupWriter.java
+++ b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/GroupWriter.java
@@ -68,8 +68,6 @@
  */
 package ca.nrc.cadc.ac;
 
-import ca.nrc.cadc.date.DateUtil;
-import ca.nrc.cadc.util.StringBuilderWriter;
 import java.io.BufferedWriter;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -78,13 +76,16 @@ import java.io.UnsupportedEncodingException;
 import java.io.Writer;
 import java.security.Principal;
 import java.text.DateFormat;
-import java.util.Set;
+
 import org.jdom2.Attribute;
 import org.jdom2.Document;
 import org.jdom2.Element;
 import org.jdom2.output.Format;
 import org.jdom2.output.XMLOutputter;
 
+import ca.nrc.cadc.date.DateUtil;
+import ca.nrc.cadc.util.StringBuilderWriter;
+
 public class GroupWriter
 {
     /**
@@ -181,11 +182,6 @@ public class GroupWriter
                 groupElement.addContent(descriptionElement);
             }
 
-            // Group publicRead
-            Element publicReadElement = new Element("publicRead");
-            publicReadElement.setText(String.valueOf(group.publicRead));
-            groupElement.addContent(publicReadElement);
-
             // lastModified
             if (group.lastModified != null)
             {
diff --git a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/GMSClient.java b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/GMSClient.java
index 7bbb8e42..1ddecf95 100755
--- a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/GMSClient.java
+++ b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/GMSClient.java
@@ -79,10 +79,10 @@ import java.security.AccessControlContext;
 import java.security.AccessControlException;
 import java.security.AccessController;
 import java.security.Principal;
-import java.util.ArrayList;
-import java.util.Collection;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import javax.net.ssl.HttpsURLConnection;
@@ -108,8 +108,6 @@ import ca.nrc.cadc.net.NetUtil;
 
 /**
  * Client class for communicating with the access control web service.
- * 
- * TODO: Cache the group memberships using getCachedGroups(), setCachedGroups()
  */
 public class GMSClient
 {
@@ -652,6 +650,12 @@ public class GMSClient
             throw new IllegalArgumentException("userID and role are required.");
         }
         
+        List<Group> cachedGroups = getCachedGroups(userID, role);
+        if (cachedGroups != null)
+        {
+            return cachedGroups;
+        }
+        
         String idType = AuthenticationUtil.getPrincipalType(userID);
         String id = userID.getName();
         String roleString = role.getValue();
@@ -697,7 +701,9 @@ public class GMSClient
         {
             String groupsXML = new String(out.toByteArray(), "UTF-8");
             log.debug("getMemberships returned: " + groupsXML);
-            return GroupsReader.read(groupsXML);
+            List<Group> groups = GroupsReader.read(groupsXML);
+            setCachedGroups(userID, groups, role);
+            return groups;
         }
         catch (Exception bug)
         {
@@ -720,6 +726,20 @@ public class GMSClient
             throw new IllegalArgumentException("userID and role are required.");
         }
         
+        List<Group> cachedGroups = getCachedGroups(userID, role);
+        if (cachedGroups != null)
+        {
+            int index = cachedGroups.indexOf(groupName);
+            if (index != -1)
+            {
+                return cachedGroups.get(index);
+            }
+            else
+            {
+                return null;
+            }
+        }
+        
         String idType = AuthenticationUtil.getPrincipalType(userID);
         String id = userID.getName();
         String roleString = role.getValue();
@@ -730,7 +750,7 @@ public class GMSClient
         searchGroupURL.append("ID=" + URLEncoder.encode(id, "UTF-8"));
         searchGroupURL.append("&TYPE=" + URLEncoder.encode(idType, "UTF-8"));
         searchGroupURL.append("&ROLE=" + URLEncoder.encode(roleString, "UTF-8"));
-        searchGroupURL.append("&GURI=" + URLEncoder.encode(groupName, "UTF-8"));
+        searchGroupURL.append("&GROUPID=" + URLEncoder.encode(groupName, "UTF-8"));
         
         log.debug("getMembership request to " + searchGroupURL.toString());
         ByteArrayOutputStream out = new ByteArrayOutputStream();
@@ -773,6 +793,8 @@ public class GMSClient
             }
             if (groups.size() == 1)
             {
+                // don't cache these results as it is not a complete
+                // list of memberships--it only applies to one group.
                 return groups.get(0);
             }
             throw new IllegalStateException(
@@ -821,45 +843,83 @@ public class GMSClient
         return this.sslSocketFactory;
     }
 
-    protected Collection<Group> getCachedGroups()
+    protected List<Group> getCachedGroups(Principal userID, Role role)
     {
         AccessControlContext acContext = AccessController.getContext();
         Subject subject = Subject.getSubject(acContext);
-        if (subject != null)
+        
+        // only consult cache if the userID is of the calling subject
+        if (userIsSubject(userID, subject))
         {
-            Set groupCredentialSet = 
-                    subject.getPrivateCredentials(GroupCredentials.class);
+            
+            Set groupCredentialSet = subject.getPrivateCredentials(GroupMemberships.class);
             if ((groupCredentialSet != null) && 
                 (groupCredentialSet.size() == 1))
             {
                 Iterator i = groupCredentialSet.iterator();
-                return ((GroupCredentials) i.next()).groupMemberships;
+                GroupMemberships groupMemberships = ((GroupMemberships) i.next());
+                return groupMemberships.memberships.get(role);
             }
         }
         return null;
     }
 
-    protected void setCachedGroups(Collection<Group> groups)
+    protected void setCachedGroups(Principal userID, List<Group> groups, Role role)
     {
         AccessControlContext acContext = AccessController.getContext();
         Subject subject = Subject.getSubject(acContext);
-        if (subject != null)
+        
+        // only save to cache if the userID is of the calling subject
+        if (userIsSubject(userID, subject))
         {
-            GroupCredentials groupCredentials = new GroupCredentials();
-            groupCredentials.groupMemberships.addAll(groups);
-            subject.getPrivateCredentials().add(groupCredentials);
+            GroupMemberships groupCredentials = null;
+            Set groupCredentialSet = subject.getPrivateCredentials(GroupMemberships.class);
+            if ((groupCredentialSet != null) && 
+                (groupCredentialSet.size() == 1))
+            {
+                Iterator i = groupCredentialSet.iterator();
+                groupCredentials = ((GroupMemberships) i.next());
+            }
+            else
+            {
+                groupCredentials = new GroupMemberships();
+                subject.getPrivateCredentials().add(groupCredentials);
+            }
+            
+            groupCredentials.memberships.put(role,  groups);
+        }
+    }
+    
+    protected boolean userIsSubject(Principal userID, Subject subject)
+    {
+        if (userID == null || subject == null)
+        {
+            return false;
+        }
+        
+        Set<Principal> subjectPrincipals = subject.getPrincipals();
+        Iterator<Principal> i = subjectPrincipals.iterator();
+        Principal next = null;
+        while (i.hasNext())
+        {
+            next = i.next();
+            if (next.equals(userID))
+            {
+                return true;
+            }
         }
+        return false;
     }
 
     /**
      * Class used to hold list of groups in which
      * a user is a member.
      */
-    protected class GroupCredentials
+    protected class GroupMemberships
     {
-        Collection<Group> groupMemberships = new ArrayList<Group>();
+        Map<Role, List<Group>> memberships = new HashMap<Role, List<Group>>();
 
-        protected GroupCredentials()
+        protected GroupMemberships()
         {
         }
 
diff --git a/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupReaderWriterTest.java b/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupReaderWriterTest.java
index 2ea98693..83675c64 100644
--- a/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupReaderWriterTest.java
+++ b/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupReaderWriterTest.java
@@ -68,18 +68,25 @@
  */
 package ca.nrc.cadc.ac;
 
-import ca.nrc.cadc.auth.HttpPrincipal;
-import ca.nrc.cadc.auth.NumericPrincipal;
-import ca.nrc.cadc.auth.OpenIdPrincipal;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.Reader;
 import java.security.Principal;
 import java.util.Date;
+
 import javax.security.auth.x500.X500Principal;
+
 import org.apache.log4j.Logger;
 import org.junit.Test;
-import static org.junit.Assert.*;
+
+import ca.nrc.cadc.auth.HttpPrincipal;
+import ca.nrc.cadc.auth.NumericPrincipal;
+import ca.nrc.cadc.auth.OpenIdPrincipal;
 
 /**
  *
@@ -152,7 +159,6 @@ public class GroupReaderWriterTest
         Group expected = new Group("groupID", new User<Principal>(new HttpPrincipal("foo")));
         expected.description = "description";
         expected.lastModified = new Date();
-        expected.publicRead = true;
         expected.properties.add(new GroupProperty("key", "value", true));
         
         Group readGroup = new Group("read", new User<Principal>(new X500Principal("cn=foo,o=ca")));
@@ -174,7 +180,6 @@ public class GroupReaderWriterTest
         assertEquals(expected, actual);
         assertEquals(expected.description, actual.description);
         assertEquals(expected.lastModified, actual.lastModified);
-        assertEquals(expected.publicRead, actual.publicRead);
         assertEquals(expected.getProperties(), actual.getProperties());
         assertEquals(expected.groupRead, actual.groupRead);
         assertEquals(expected.groupWrite, actual.groupWrite);
diff --git a/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupTest.java b/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupTest.java
index 0cb97835..af09cf89 100644
--- a/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupTest.java
+++ b/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/GroupTest.java
@@ -68,9 +68,13 @@
  */
 package ca.nrc.cadc.ac;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
 import org.apache.log4j.Logger;
 import org.junit.Test;
-import static org.junit.Assert.*;
 
 import ca.nrc.cadc.auth.HttpPrincipal;
 
@@ -124,10 +128,6 @@ public class GroupTest
         group3.groupWrite = group4;
         assertEquals(group3.hashCode(), group4.hashCode());
         assertEquals(group3,group4);
-
-        group3.publicRead = true;
-        assertEquals(group3.hashCode(), group4.hashCode());
-        assertEquals(group3,group4);
         
         group4 = new Group("NewTestGroup-._~.", owner);
         assertFalse(group3.hashCode() == group4.hashCode());
diff --git a/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/client/GMSClientTest.java b/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/client/GMSClientTest.java
new file mode 100644
index 00000000..3fcd9e18
--- /dev/null
+++ b/projects/cadcAccessControl/test/src/ca/nrc/cadc/ac/client/GMSClientTest.java
@@ -0,0 +1,174 @@
+/*
+ ************************************************************************
+ *******************  CANADIAN ASTRONOMY DATA CENTRE  *******************
+ **************  CENTRE CANADIEN DE DONNÉES ASTRONOMIQUES  **************
+ *
+ *  (c) 2014.                            (c) 2014.
+ *  Government of Canada                 Gouvernement du Canada
+ *  National Research Council            Conseil national de recherches
+ *  Ottawa, Canada, K1A 0R6              Ottawa, Canada, K1A 0R6
+ *  All rights reserved                  Tous droits réservés
+ *
+ *  NRC disclaims any warranties,        Le CNRC dénie toute garantie
+ *  expressed, implied, or               énoncée, implicite ou légale,
+ *  statutory, of any kind with          de quelque nature que ce
+ *  respect to the software,             soit, concernant le logiciel,
+ *  including without limitation         y compris sans restriction
+ *  any warranty of merchantability      toute garantie de valeur
+ *  or fitness for a particular          marchande ou de pertinence
+ *  purpose. NRC shall not be            pour un usage particulier.
+ *  liable in any event for any          Le CNRC ne pourra en aucun cas
+ *  damages, whether direct or           être tenu responsable de tout
+ *  indirect, special or general,        dommage, direct ou indirect,
+ *  consequential or incidental,         particulier ou général,
+ *  arising from the use of the          accessoire ou fortuit, résultant
+ *  software.  Neither the name          de l'utilisation du logiciel. Ni
+ *  of the National Research             le nom du Conseil National de
+ *  Council of Canada nor the            Recherches du Canada ni les noms
+ *  names of its contributors may        de ses  participants ne peuvent
+ *  be used to endorse or promote        être utilisés pour approuver ou
+ *  products derived from this           promouvoir les produits dérivés
+ *  software without specific prior      de ce logiciel sans autorisation
+ *  written permission.                  préalable et particulière
+ *                                       par écrit.
+ *
+ *  This file is part of the             Ce fichier fait partie du projet
+ *  OpenCADC project.                    OpenCADC.
+ *
+ *  OpenCADC is free software:           OpenCADC est un logiciel libre ;
+ *  you can redistribute it and/or       vous pouvez le redistribuer ou le
+ *  modify it under the terms of         modifier suivant les termes de
+ *  the GNU Affero General Public        la “GNU Affero General Public
+ *  License as published by the          License” telle que publiée
+ *  Free Software Foundation,            par la Free Software Foundation
+ *  either version 3 of the              : soit la version 3 de cette
+ *  License, or (at your option)         licence, soit (à votre gré)
+ *  any later version.                   toute version ultérieure.
+ *
+ *  OpenCADC is distributed in the       OpenCADC est distribué
+ *  hope that it will be useful,         dans l’espoir qu’il vous
+ *  but WITHOUT ANY WARRANTY;            sera utile, mais SANS AUCUNE
+ *  without even the implied             GARANTIE : sans même la garantie
+ *  warranty of MERCHANTABILITY          implicite de COMMERCIALISABILITÉ
+ *  or FITNESS FOR A PARTICULAR          ni d’ADÉQUATION À UN OBJECTIF
+ *  PURPOSE.  See the GNU Affero         PARTICULIER. Consultez la Licence
+ *  General Public License for           Générale Publique GNU Affero
+ *  more details.                        pour plus de détails.
+ *
+ *  You should have received             Vous devriez avoir reçu une
+ *  a copy of the GNU Affero             copie de la Licence Générale
+ *  General Public License along         Publique GNU Affero avec
+ *  with OpenCADC.  If not, see          OpenCADC ; si ce n’est
+ *  <http://www.gnu.org/licenses/>.      pas le cas, consultez :
+ *                                       <http://www.gnu.org/licenses/>.
+ *
+ *  $Revision: 4 $
+ *
+ ************************************************************************
+ */
+
+package ca.nrc.cadc.ac.client;
+
+import java.net.URI;
+import java.net.URL;
+import java.security.PrivilegedExceptionAction;
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.security.auth.Subject;
+
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.junit.Assert;
+import org.junit.Test;
+
+import ca.nrc.cadc.ac.AC;
+import ca.nrc.cadc.ac.Group;
+import ca.nrc.cadc.ac.Role;
+import ca.nrc.cadc.auth.HttpPrincipal;
+import ca.nrc.cadc.reg.client.RegistryClient;
+import ca.nrc.cadc.util.Log4jInit;
+
+public class GMSClientTest
+{
+    
+    private static final Logger log = Logger.getLogger(GMSClientTest.class);
+    
+    public GMSClientTest()
+    {
+        Log4jInit.setLevel("ca.nrc.cadc.ac", Level.INFO);
+    }
+    
+    @Test
+    public void testGroupCaching()
+    {
+        try
+        {
+            Subject subject = new Subject();
+            final HttpPrincipal userID = new HttpPrincipal("test");
+            subject.getPrincipals().add(userID);
+
+            Subject.doAs(subject, new PrivilegedExceptionAction<Object>()
+                {
+                    @Override
+                    public Object run() throws Exception
+                    {
+                        RegistryClient regClient = new RegistryClient();
+                        URL baseURL = regClient.getServiceURL(new URI(AC.GMS_SERVICE_URI));
+                        GMSClient client = new GMSClient(baseURL.toString());
+
+                        List<Group> initial = client.getCachedGroups(userID, Role.MEMBER);
+                        Assert.assertNull("Cache should be null", initial);
+
+                        List<Group> expected = new ArrayList<Group>();
+                        Group group1 = new Group("1");
+                        Group group2 = new Group("2");
+                        expected.add(group1);
+                        expected.add(group2);
+
+                        client.setCachedGroups(userID, expected, Role.MEMBER);
+
+                        List<Group> actual = client.getCachedGroups(userID, Role.MEMBER);
+                        Assert.assertEquals("Wrong cached groups", expected, actual);
+                        
+                        // check against another role
+                        actual = client.getCachedGroups(userID, Role.OWNER);
+                        Assert.assertNull("Cache should be null", actual);
+                        
+                        // check against another userid
+                        final HttpPrincipal userID2 = new HttpPrincipal("test2");
+                        actual = client.getCachedGroups(userID2, Role.MEMBER);
+                        Assert.assertNull("Cache should be null", actual);
+
+                        return null;
+                    }
+                });
+
+            // do the same without a subject
+            RegistryClient regClient = new RegistryClient();
+            URL baseURL = regClient.getServiceURL(new URI(AC.GMS_SERVICE_URI));
+            GMSClient client = new GMSClient(baseURL.toString());
+
+            List<Group> initial = client.getCachedGroups(userID, Role.MEMBER);
+            Assert.assertNull("Cache should be null", initial);
+
+            List<Group> newgroups = new ArrayList<Group>();
+            Group group1 = new Group("1");
+            Group group2 = new Group("2");
+            newgroups.add(group1);
+            newgroups.add(group2);
+
+            client.setCachedGroups(userID, newgroups, Role.MEMBER);
+
+            List<Group> actual = client.getCachedGroups(userID, Role.MEMBER);
+            Assert.assertNull("Cache should still be null", actual);
+        }
+
+        catch (Throwable t)
+        {
+            log.error("Unexpected exception", t);
+            Assert.fail("Unexpected exception: " + t.getMessage());
+        }
+    }
+
+}
-- 
GitLab