From e30b8c55456b3e0499fd084aa2b07472116d5573 Mon Sep 17 00:00:00 2001
From: Brian Major <major.brian@gmail.com>
Date: Tue, 12 Apr 2016 15:42:40 -0700
Subject: [PATCH] s1885 - users no longer sorted in groups
---
.../web/groups/AbstractGroupAction.java | 4 +-
.../web/groups/AddUserMemberAction.java | 7 +-
.../web/groups/RemoveUserMemberAction.java | 12 +-
.../cadc/ac/server/ldap/LdapGroupDAOTest.java | 16 +-
.../src/ca/nrc/cadc/ac/Group.java | 14 +-
.../src/ca/nrc/cadc/ac/User.java | 137 +++++-------------
.../ac/json/JsonGroupReaderWriterTest.java | 7 +-
.../cadc/ac/xml/GroupReaderWriterTest.java | 7 +-
8 files changed, 66 insertions(+), 138 deletions(-)
diff --git a/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AbstractGroupAction.java b/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AbstractGroupAction.java
index e7466c9c..b71d3262 100755
--- a/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AbstractGroupAction.java
+++ b/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AbstractGroupAction.java
@@ -199,11 +199,11 @@ public abstract class AbstractGroupAction implements PrivilegedExceptionAction<O
}
catch (Throwable t)
{
+ log.error("Internal Error", t);
String message = "Internal Error: " + t.getMessage();
this.logInfo.setSuccess(false);
- this.logInfo.setMessage(message);
- log.error(message, t);
sendError(500, message);
+ this.logInfo.setMessage(message);
}
return null;
}
diff --git a/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberAction.java b/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberAction.java
index 5f43eadb..e0281883 100755
--- a/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberAction.java
+++ b/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberAction.java
@@ -72,6 +72,8 @@ import java.security.Principal;
import java.util.ArrayList;
import java.util.List;
+import org.apache.log4j.Logger;
+
import ca.nrc.cadc.ac.Group;
import ca.nrc.cadc.ac.MemberAlreadyExistsException;
import ca.nrc.cadc.ac.User;
@@ -79,6 +81,8 @@ import ca.nrc.cadc.auth.AuthenticationUtil;
public class AddUserMemberAction extends AbstractGroupAction
{
+ private static final Logger log = Logger.getLogger(AddUserMemberAction.class);
+
private final String groupName;
private final String userID;
private final String userIDType;
@@ -92,12 +96,13 @@ public class AddUserMemberAction extends AbstractGroupAction
this.userIDType = userIDType;
}
- @SuppressWarnings("unchecked")
+ @Override
public void doAction() throws Exception
{
Group group = groupPersistence.getGroup(this.groupName);
Principal userPrincipal = AuthenticationUtil.createPrincipal(this.userID, this.userIDType);
User toAdd = new User();
+
toAdd.getIdentities().add(userPrincipal);
if (!group.getUserMembers().add(toAdd))
{
diff --git a/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberAction.java b/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberAction.java
index 8abb5a7a..b5b6960f 100755
--- a/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberAction.java
+++ b/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberAction.java
@@ -78,7 +78,6 @@ import ca.nrc.cadc.ac.User;
import ca.nrc.cadc.ac.server.PluginFactory;
import ca.nrc.cadc.ac.server.UserPersistence;
import ca.nrc.cadc.auth.AuthenticationUtil;
-import ca.nrc.cadc.util.ObjectUtil;
public class RemoveUserMemberAction extends AbstractGroupAction
{
@@ -86,7 +85,7 @@ public class RemoveUserMemberAction extends AbstractGroupAction
private final String userID;
private final String userIDType;
- RemoveUserMemberAction(String groupName, String userID, String userIDType)
+ public RemoveUserMemberAction(String groupName, String userID, String userIDType)
{
super();
this.groupName = groupName;
@@ -94,7 +93,7 @@ public class RemoveUserMemberAction extends AbstractGroupAction
this.userIDType = userIDType;
}
- @SuppressWarnings("unchecked")
+ @Override
public void doAction() throws Exception
{
Group group = groupPersistence.getGroup(this.groupName);
@@ -102,18 +101,15 @@ public class RemoveUserMemberAction extends AbstractGroupAction
Principal userPrincipal = AuthenticationUtil.createPrincipal(this.userID, this.userIDType);
User user = getUserPersistence().getAugmentedUser(userPrincipal);
- User toRemove = new User();
- ObjectUtil.setField(toRemove, user.getID(), "id");
- toRemove.getIdentities().addAll(user.getIdentities());
- if (!group.getUserMembers().remove(toRemove))
+ if (!group.getUserMembers().remove(user))
{
throw new MemberNotFoundException();
}
groupPersistence.modifyGroup(group);
List<String> deletedMembers = new ArrayList<String>();
- deletedMembers.add(getUseridForLogging(toRemove));
+ deletedMembers.add(getUseridForLogging(user));
logGroupInfo(group.getID(), deletedMembers, null);
}
diff --git a/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java b/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java
index 4b475fdf..62ac7580 100644
--- a/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java
+++ b/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java
@@ -72,7 +72,6 @@ import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
-import java.security.Principal;
import java.security.PrivilegedExceptionAction;
import java.util.Collection;
@@ -118,14 +117,6 @@ public class LdapGroupDAOTest extends AbstractLdapDAOTest
getGroupDAO().addGroup(expectGroup);
Group actualGroup = getGroupDAO().getGroup(expectGroup.getID(), true);
log.info("addGroup: " + expectGroup.getID());
- for (Principal p : expectGroup.getOwner().getIdentities())
- {
- log.info("ep: " + p);
- }
- for (Principal p : actualGroup.getOwner().getIdentities())
- {
- log.info("ap: " + p);
- }
assertGroupsEqual(expectGroup, actualGroup);
Group otherGroup = new Group(getGroupID());
@@ -446,7 +437,8 @@ public class LdapGroupDAOTest extends AbstractLdapDAOTest
}
assertEquals(gr1.getUserMembers().size(), gr2.getUserMembers().size());
- assertEquals(gr1.getUserMembers(), gr2.getUserMembers());
+ assertTrue(gr1.getUserMembers().containsAll(gr2.getUserMembers()));
+ assertTrue(gr2.getUserMembers().containsAll(gr1.getUserMembers()));
for (User user : gr1.getUserMembers())
{
assertTrue(gr2.getUserMembers().contains(user));
@@ -459,9 +451,7 @@ public class LdapGroupDAOTest extends AbstractLdapDAOTest
assertTrue(gr2.getGroupAdmins().contains(gr));
}
- assertEquals(gr1.getUserAdmins(), gr2.getUserAdmins());
- assertEquals(gr1.getUserAdmins().size(), gr2.getUserAdmins()
- .size());
+ assertEquals(gr1.getUserAdmins().size(), gr2.getUserAdmins().size());
for (User user : gr1.getUserAdmins())
{
assertTrue(gr2.getUserAdmins().contains(user));
diff --git a/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java b/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java
index a830db23..c775729a 100644
--- a/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java
+++ b/cadcAccessControl/src/ca/nrc/cadc/ac/Group.java
@@ -71,7 +71,6 @@ package ca.nrc.cadc.ac;
import java.util.Date;
import java.util.HashSet;
import java.util.Set;
-import java.util.TreeSet;
public class Group
{
@@ -83,13 +82,13 @@ public class Group
protected Set<GroupProperty> properties = new HashSet<GroupProperty>();
// group's user members
- private Set<User> userMembers = new TreeSet<User>();
+ private UserSet userMembers = new UserSet();
// group's group members
private Set<Group> groupMembers = new HashSet<Group>();
// group's user admins
- private Set<User> userAdmins = new TreeSet<User>();
+ private UserSet userAdmins = new UserSet();
// group's group admins
private Set<Group> groupAdmins = new HashSet<Group>();
@@ -97,7 +96,9 @@ public class Group
public String description;
public Date lastModified;
- public Group() {}
+ public Group()
+ {
+ }
/**
* Ctor.
@@ -153,7 +154,7 @@ public class Group
*
* @return individual user members of this group
*/
- public Set<User> getUserMembers()
+ public UserSet getUserMembers()
{
return userMembers;
}
@@ -171,7 +172,7 @@ public class Group
*
* @return individual user admins of this group
*/
- public Set<User> getUserAdmins()
+ public UserSet getUserAdmins()
{
return userAdmins;
}
@@ -225,4 +226,5 @@ public class Group
{
return getClass().getSimpleName() + "[" + groupID + "]";
}
+
}
diff --git a/cadcAccessControl/src/ca/nrc/cadc/ac/User.java b/cadcAccessControl/src/ca/nrc/cadc/ac/User.java
index 126e3b53..607af355 100644
--- a/cadcAccessControl/src/ca/nrc/cadc/ac/User.java
+++ b/cadcAccessControl/src/ca/nrc/cadc/ac/User.java
@@ -77,14 +77,14 @@ import java.util.TreeSet;
import javax.security.auth.x500.X500Principal;
-import ca.nrc.cadc.auth.AuthenticationUtil;
import ca.nrc.cadc.auth.HttpPrincipal;
+import ca.nrc.cadc.auth.PrincipalComparator;
-public class User implements Comparable<User>
+public class User
{
private InternalID id;
- private SortedSet<Principal> identities = new TreeSet<Principal>(new PrincipalComparator());
+ private SortedSet<Principal> identities;
public PersonalDetails personalDetails;
public PosixDetails posixDetails;
@@ -96,7 +96,12 @@ public class User implements Comparable<User>
*/
public Object appData;
- public User() {}
+ public User()
+ {
+ PrincipalComparator p = new PrincipalComparator();
+ UserPrincipalComparator u = new UserPrincipalComparator(p);
+ this.identities = new TreeSet<Principal>(u);
+ }
public InternalID getID()
{
@@ -118,13 +123,15 @@ public class User implements Comparable<User>
*/
public <S extends Principal> Set<S> getIdentities(final Class<S> identityClass)
{
- final Set<S> matchedIdentities = new TreeSet<S>(new PrincipalComparator());
+ PrincipalComparator p = new PrincipalComparator();
+ UserPrincipalComparator u = new UserPrincipalComparator(p);
+ final Set<S> matchedIdentities = new TreeSet<S>(u);
- for (final Principal p : identities)
+ for (final Principal principal : identities)
{
- if (identityClass.isAssignableFrom(p.getClass()))
+ if (identityClass.isAssignableFrom(principal.getClass()))
{
- matchedIdentities.add((S) p);
+ matchedIdentities.add((S) principal);
}
}
@@ -165,61 +172,27 @@ public class User implements Comparable<User>
*/
public boolean isConsistent(final User superset)
{
+
if (superset == null)
{
return false;
}
- if (this.identities.isEmpty() || superset.identities.isEmpty())
+ if (this.identities.size() == 0 || superset.identities.size() == 0)
{
return false;
}
- return superset.getIdentities().containsAll(this.getIdentities());
-
-// // could be improved because both sets are ordered
-// for (Principal identity: getIdentities())
-// {
-// boolean found = false;
-// for (Principal op: superset.getIdentities())
-// {
-// if (AuthenticationUtil.equals(op, identity))
-// {
-// found = true;
-// break;
-// }
-// }
-// if (!found)
-// {
-// return false;
-// }
-// }
-// return true;
+ PrincipalComparator p = new PrincipalComparator();
+ TreeSet<Principal> set1 = new TreeSet<Principal>(p);
+ TreeSet<Principal> set2 = new TreeSet<Principal>(p);
+ set1.addAll(superset.getIdentities());
+ set2.addAll(this.getIdentities());
+ return set1.containsAll(set2);
}
-// /* (non-Javadoc)
-// * @see java.lang.Object#hashCode()
-// */
-// @Override
-// public int hashCode()
-// {
-// int prime = 31;
-// int result = 1;
-// if (id != null)
-// {
-// result = prime * result + id.hashCode();
-// }
-// else
-// {
-// for (Principal principal : getIdentities())
-// {
-// result = prime * result + principal.hashCode();
-// }
-// }
-// return result;
-// }
- /* (non-Javadoc)
+ /*
* @see java.lang.Object#equals(java.lang.Object)
*/
@Override
@@ -231,33 +204,6 @@ public class User implements Comparable<User>
return (this.isConsistent(user) || user.isConsistent(this));
}
return false;
-// if (this == obj)
-// {
-// return true;
-// }
-// if (obj == null)
-// {
-// return false;
-// }
-// if (!(obj instanceof User))
-// {
-// return false;
-// }
-// User other = (User) obj;
-// if (this.id == null && other.id == null)
-// {
-// return isConsistent(other);
-// }
-// if ((this.id == null && other.id != null) ||
-// (this.id != null && other.id == null))
-// {
-// return false;
-// }
-// if (id.equals(other.id))
-// {
-// return true;
-// }
-// return false;
}
@Override
@@ -274,8 +220,15 @@ public class User implements Comparable<User>
return sb.toString();
}
- private class PrincipalComparator implements Comparator<Principal>
+ private class UserPrincipalComparator implements Comparator<Principal>
{
+ private PrincipalComparator p;
+
+ UserPrincipalComparator(PrincipalComparator p)
+ {
+ this.p = p;
+ }
+
@Override
public int compare(Principal o1, Principal o2)
{
@@ -289,32 +242,8 @@ public class User implements Comparable<User>
return 0;
}
- return AuthenticationUtil.compare(o1, o2);
+ return p.compare(o1, o2);
}
}
- @Override
- public int compareTo(User other)
- {
- if (other == null)
- {
- throw new IllegalArgumentException("Cannot compare null objects");
- }
-
- if (this.getIdentities().isEmpty() || other.getIdentities().isEmpty())
- {
- throw new IllegalArgumentException("Users need identities for comparison.");
- }
-
- if (this.isConsistent(other) || other.isConsistent(this))
- {
- return 0;
- }
-
- // compare the first pricipals in the order set
- Principal p1 = this.getIdentities().iterator().next();
- Principal p2 = other.getIdentities().iterator().next();
- return AuthenticationUtil.compare(p1, p2);
- }
-
}
diff --git a/cadcAccessControl/test/src/ca/nrc/cadc/ac/json/JsonGroupReaderWriterTest.java b/cadcAccessControl/test/src/ca/nrc/cadc/ac/json/JsonGroupReaderWriterTest.java
index 704f8273..e2079a71 100644
--- a/cadcAccessControl/test/src/ca/nrc/cadc/ac/json/JsonGroupReaderWriterTest.java
+++ b/cadcAccessControl/test/src/ca/nrc/cadc/ac/json/JsonGroupReaderWriterTest.java
@@ -71,6 +71,7 @@ package ca.nrc.cadc.ac.json;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import java.io.IOException;
@@ -258,8 +259,10 @@ public class JsonGroupReaderWriterTest
assertEquals(expected.lastModified, actual.lastModified);
assertEquals("Properties don't match.", sortedExpectedProperties,
sortedActualProperties);
- assertEquals(expected.getGroupMembers(), actual.getGroupMembers());
- assertEquals(expected.getUserMembers(), actual.getUserMembers());
+ assertTrue(expected.getGroupMembers().containsAll(actual.getGroupMembers()));
+ assertTrue(actual.getGroupMembers().containsAll(expected.getGroupMembers()));
+ assertTrue(expected.getUserMembers().containsAll(actual.getUserMembers()));
+ assertTrue(actual.getUserMembers().containsAll(expected.getUserMembers()));
}
class GroupPropertyComparator implements Comparator<GroupProperty>
diff --git a/cadcAccessControl/test/src/ca/nrc/cadc/ac/xml/GroupReaderWriterTest.java b/cadcAccessControl/test/src/ca/nrc/cadc/ac/xml/GroupReaderWriterTest.java
index 2fc278f9..ee219c9c 100644
--- a/cadcAccessControl/test/src/ca/nrc/cadc/ac/xml/GroupReaderWriterTest.java
+++ b/cadcAccessControl/test/src/ca/nrc/cadc/ac/xml/GroupReaderWriterTest.java
@@ -71,6 +71,7 @@ package ca.nrc.cadc.ac.xml;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import java.io.IOException;
@@ -217,8 +218,10 @@ public class GroupReaderWriterTest
assertEquals(expected.description, actual.description);
assertEquals(expected.lastModified, actual.lastModified);
assertEquals(expected.getProperties(), actual.getProperties());
- assertEquals(expected.getGroupMembers(), actual.getGroupMembers());
- assertEquals(expected.getUserMembers(), actual.getUserMembers());
+ assertTrue(expected.getGroupMembers().containsAll(actual.getGroupMembers()));
+ assertTrue(actual.getGroupMembers().containsAll(expected.getGroupMembers()));
+ assertTrue(expected.getUserMembers().containsAll(actual.getUserMembers()));
+ assertTrue(actual.getUserMembers().containsAll(expected.getUserMembers()));
}
private void setGroupOwner(Group group, User owner)
--
GitLab