Skip to content
GitLab
Commit e30b8c55 authored by Brian Major's avatar Brian Major
Browse files

s1885 - users no longer sorted in groups

parent d1ebc41c
Hide whitespace changes
Inline Side-by-side
cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AbstractGroupAction.java
View file @ e30b8c55
......@@ -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;
}
......
cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberAction.java
View file @ e30b8c55
......@@ -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))
{
......
cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberAction.java
View file @ e30b8c55
......@@ -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);
}
......
cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/ldap/LdapGroupDAOTest.java
View file @ e30b8c55
......@@ -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));
......
cadcAccessControl/src/ca/nrc/cadc/ac/Group.java
View file @ e30b8c55
......@@ -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 + "]";
}
}
cadcAccessControl/src/ca/nrc/cadc/ac/User.java
View file @ e30b8c55
......@@ -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);
}
}
cadcAccessControl/test/src/ca/nrc/cadc/ac/json/JsonGroupReaderWriterTest.java
View file @ e30b8c55
......@@ -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>
......
cadcAccessControl/test/src/ca/nrc/cadc/ac/xml/GroupReaderWriterTest.java
View file @ e30b8c55
......@@ -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)
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment