diff --git a/components/server/src/ome/logic/AdminImpl.java b/components/server/src/ome/logic/AdminImpl.java index e4f085863eb..919bef65e94 100644 --- a/components/server/src/ome/logic/AdminImpl.java +++ b/components/server/src/ome/logic/AdminImpl.java @@ -10,6 +10,7 @@ import java.sql.SQLException; import java.util.Arrays; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -460,8 +461,7 @@ public long createUser(final Experimenter newUser, String defaultGroup) { // logged via createExperimenter final ExperimenterGroup proxy = groupProxy(defaultGroup); - adminOrPiOfGroup(proxy); - + // logged & secured via createExperimenter return createExperimenter(newUser, proxy, groupProxy(sec .getSecurityRoles().getUserGroupName())); @@ -469,7 +469,7 @@ public long createUser(final Experimenter newUser, String defaultGroup) { @RolesAllowed("system") public long createSystemUser(Experimenter newSystemUser) { - // logged via createExperimenter + // logged & secured via createExperimenter return createExperimenter(newSystemUser, groupProxy(sec.getSecurityRoles().getSystemGroupName()), groupProxy(sec.getSecurityRoles().getUserGroupName())); @@ -480,7 +480,14 @@ public long createSystemUser(Experimenter newSystemUser) { public long createExperimenter(final Experimenter experimenter, ExperimenterGroup defaultGroup, ExperimenterGroup... otherGroups) { - adminOrPiOfGroups(defaultGroup, otherGroups); + Set nonUserGroupGroups = new HashSet(); + for (ExperimenterGroup eg : otherGroups) { + if (!eg.getId().equals(getSecurityRoles().getUserGroupId())) { + nonUserGroupGroups.add(eg); + } + } + adminOrPiOfGroups(defaultGroup, + nonUserGroupGroups.toArray(new ExperimenterGroup[0])); long uid = roleProvider.createExperimenter(experimenter, defaultGroup, otherGroups); // If this method passes, then the Experimenter is valid. @@ -593,14 +600,14 @@ public void setGroupOwner(final ExperimenterGroup group, final Experimenter owne @RolesAllowed("user") public void unsetGroupOwner(final ExperimenterGroup group, final Experimenter owner) { adminOrPiOfGroup(group); - toggleGroupOwner(group, owner, Boolean.TRUE); + toggleGroupOwner(group, owner, Boolean.FALSE); } @RolesAllowed("user") public void addGroupOwners(final ExperimenterGroup group, final Experimenter... owner) { adminOrPiOfGroup(group); for (Experimenter o : owner) { - toggleGroupOwner(group, o, true); + toggleGroupOwner(group, o, Boolean.TRUE); } } @@ -608,7 +615,7 @@ public void addGroupOwners(final ExperimenterGroup group, final Experimenter... public void removeGroupOwners(final ExperimenterGroup group, final Experimenter... owner) { adminOrPiOfGroup(group); for (Experimenter o : owner) { - toggleGroupOwner(group, o, false); + toggleGroupOwner(group, o, Boolean.FALSE); } } @@ -1287,6 +1294,7 @@ private boolean isPiOf(ExperimenterGroup group) { if (group == null) { return true; } + EventContext ec = getEventContext(); List piOf = ec.getLeaderOfGroupsList(); return piOf.contains(group.getId()); diff --git a/components/server/test/ome/server/itests/perms42/AdminPermsTest.java b/components/server/test/ome/server/itests/perms42/AdminPermsTest.java index 72e4b7b67b0..e956e26f5e6 100644 --- a/components/server/test/ome/server/itests/perms42/AdminPermsTest.java +++ b/components/server/test/ome/server/itests/perms42/AdminPermsTest.java @@ -34,14 +34,116 @@ public class AdminPermsTest extends PermissionsTest { // updateExperimenter // updateExperimenterWithPassword // updateGroup - // createUser - // createExperimenter - // createExperimenterWithPassword - // removeGroups - // setGroupOwner - // unsetGroupOwner - // addGroupOwners - // removeGroupOwners + + @Test + public void testUser() { + + // Non-member group to be used as dummy + loginRoot(); + ExperimenterGroup g2 = newGroup(); + + setup(Permissions.PRIVATE); + + Experimenter e = uuidUser(); + try { + iAdmin.createUser(e, g2.getName()); + fail("not in my group"); + } catch (SecurityViolation sv) { + // good; + } + try { + iAdmin.createUser(e, fixture.groupName); + fail("my group, i'm not leader"); + } catch (SecurityViolation sv) { + // good; + } + + fixture.make_leader(); + + try { + iAdmin.createUser(e, g2.getName()); + fail("still not in my group even thought i'm leader"); + } catch (SecurityViolation sv) { + // good; + } + + iAdmin.createUser(e, fixture.groupName); + + } + + + + @Test + public void testCreateExperimenterWithPassword() { + + // Non-member group to be used as dummy + loginRoot(); + ExperimenterGroup g2 = newGroup(); + + setup(Permissions.PRIVATE); + + Experimenter e = uuidUser(); + try { + iAdmin.createExperimenterWithPassword(e, "pass", g2); + fail("not in my group"); + } catch (SecurityViolation sv) { + // good; + } + try { + iAdmin.createExperimenterWithPassword(e, "pass", fixture.group()); + fail("my group, i'm not leader"); + } catch (SecurityViolation sv) { + // good; + } + + fixture.make_leader(); + + try { + iAdmin.createExperimenterWithPassword(e, "pass", g2); + fail("still not in my group even thought i'm leader"); + } catch (SecurityViolation sv) { + // good; + } + + iAdmin.createExperimenterWithPassword(e, "pass", fixture.group()); // Yes. + + } + + @Test + public void testCreateExperimenter() { + + // Non-member group to be used as dummy + loginRoot(); + ExperimenterGroup g2 = newGroup(); + + setup(Permissions.PRIVATE); + + Experimenter e = uuidUser(); + try { + iAdmin.createExperimenter(e, g2); + fail("not in my group"); + } catch (SecurityViolation sv) { + // good; + } + try { + iAdmin.createExperimenter(e, fixture.group()); + fail("my group, i'm not leader"); + } catch (SecurityViolation sv) { + // good; + } + + fixture.make_leader(); + + try { + iAdmin.createExperimenter(e, g2); + fail("still not in my group even thought i'm leader"); + } catch (SecurityViolation sv) { + // good; + } + + iAdmin.createExperimenter(e, fixture.group()); // Yes. + + } @Test @@ -113,21 +215,23 @@ public void testAddRemoveGroupOwners() throws Exception { } @Test - public void testCreateUserMakeOwnerAndRemoveAsOwner() throws Exception { + public void testCreateUserMakeOwnerAndRemoveAsOwnerWithAddRemove() throws Exception { setup(Permissions.PRIVATE); assertMembers(fixture.group(), fixture.user.getId()); assertLeaders(fixture.group()); + try { // Try to add self + iAdmin.addGroupOwners(fixture.group(), fixture.user); + fail("sec-vio"); + } catch (SecurityViolation sv) { + // good + } loginRoot(); iAdmin.addGroupOwners(fixture.group(), fixture.user); assertMembers(fixture.group(), fixture.user.getId()); assertLeaders(fixture.group(), fixture.user.getId()); - // Now add another user - Experimenter e2 = new Experimenter(); - e2.setOmeName(uuid()); - e2.setFirstName(uuid()); - e2.setLastName(uuid()); + Experimenter e2 = uuidUser(); fixture.log_in(); long uid = iAdmin.createExperimenter(e2, fixture.group()); e2.setId(uid); @@ -136,12 +240,49 @@ public void testCreateUserMakeOwnerAndRemoveAsOwner() throws Exception { iAdmin.addGroupOwners(fixture.group(), e2); assertLeaders(fixture.group(), fixture.user.getId(), e2.getId()); - // Now remove that new user + // 2. Now remove that new user iAdmin.removeGroupOwners(fixture.group(), e2); assertMembers(fixture.group(), fixture.user.getId(), e2.getId()); assertLeaders(fixture.group(), fixture.user.getId()); + + // Finally, the one owner removes his/herself (valid) + iAdmin.removeGroupOwners(fixture.group(), fixture.user); } + @Test + public void testCreateUserMakeOwnerAndRemoveAsOwnerWithSetUnset() throws Exception { + setup(Permissions.PRIVATE); + + assertMembers(fixture.group(), fixture.user.getId()); + assertLeaders(fixture.group()); + try { // Try to add self + iAdmin.setGroupOwner(fixture.group(), fixture.user); + fail("sec-vio"); + } catch (SecurityViolation sv) { + // good + } + loginRoot(); + iAdmin.setGroupOwner(fixture.group(), fixture.user); + assertMembers(fixture.group(), fixture.user.getId()); + assertLeaders(fixture.group(), fixture.user.getId()); + + Experimenter e2 = uuidUser(); + fixture.log_in(); + long uid = iAdmin.createExperimenter(e2, fixture.group()); + e2.setId(uid); + + assertMembers(fixture.group(), fixture.user.getId(), e2.getId()); + iAdmin.setGroupOwner(fixture.group(), e2); + assertLeaders(fixture.group(), fixture.user.getId(), e2.getId()); + + // 2. Now remove that new user + iAdmin.unsetGroupOwner(fixture.group(), e2); + assertMembers(fixture.group(), fixture.user.getId(), e2.getId()); + assertLeaders(fixture.group(), fixture.user.getId()); + + // Finally, the one owner removes his/herself (valid) + iAdmin.unsetGroupOwner(fixture.group(), fixture.user); + } // Helpers // ========================================================================= @@ -178,4 +319,19 @@ protected void assertEqualSets(Set thatHas, Set toCheck) { missing.size() == 0 && extra.size() == 0); } + + private Experimenter uuidUser() { + Experimenter e = new Experimenter(); + e.setOmeName(uuid()); + e.setFirstName(uuid()); + e.setLastName(uuid()); + return e; + } + + private ExperimenterGroup newGroup() { + ExperimenterGroup g2 = new ExperimenterGroup(); + g2.setName(uuid()); + g2 = iAdmin.getGroup(iAdmin.createGroup(g2)); + return g2; + } } diff --git a/sql/psql/OMERO4.2-DEV__0/data.sql b/sql/psql/OMERO4.2-DEV__0/data.sql index 5fe223f8d59..96d5e0ffe75 100644 --- a/sql/psql/OMERO4.2-DEV__0/data.sql +++ b/sql/psql/OMERO4.2-DEV__0/data.sql @@ -79,13 +79,13 @@ insert into session select ome_nextval('seq_session'),-35, 0,0,now(),now(),'rw----','PREVIOUSITEMS','1111',0,0; insert into event (id,permissions,time,status,experimenter,session) values (0,0,now(),'BOOTSTRAP',0,0); insert into experimentergroup (id,permissions,version,name) - values (0,-35,0,'system'); + values (0,-103,0,'system'); insert into experimentergroup (id,permissions,version,name) - values (ome_nextval('seq_experimentergroup'),-35,0,'user'); + values (ome_nextval('seq_experimentergroup'),-103,0,'user'); insert into experimentergroup (id,permissions,version,name) - values (ome_nextval('seq_experimentergroup'),-35,0,'default'); + values (ome_nextval('seq_experimentergroup'),-103,0,'default'); insert into experimentergroup (id,permissions,version,name) - values (ome_nextval('seq_experimentergroup'),-35,0,'guest'); + values (ome_nextval('seq_experimentergroup'),-103,0,'guest'); insert into eventtype (id,permissions,value) values (0,-35,'Bootstrap'); insert into groupexperimentermap