Skip to content

Commit

Permalink
Merge pull request #1595 from mtbc/11465-admin-checks
Browse files Browse the repository at this point in the history 
fix #11465: add validation checks for admin service (rebased)
  • Loading branch information
@joshmoore
joshmoore committed Oct 8, 2013
2 parents d5f3101 + 311a187 commit 69367ab
Show file tree
Hide file tree
Showing 7 changed files with 495 additions and 119 deletions.
5 changes: 5 additions & 0 deletions components/blitz/resources/omero/System.ice
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,12 @@ module omero {
long userGroupId;
string userGroupName;

// the guest user
long guestId;
string guestName;

// "guest" group. Can log in and use some methods.
long guestGroupId;
string guestGroupName;
};

Expand Down
12 changes: 11 additions & 1 deletion components/common/src/ome/api/IAdmin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ void updateSelf(@NotNull
/**
* Updates an experimenter if admin or owner of group. Only string fields on
* the object are taken into account.
* The root and guest experimenters may not be renamed.
*
* Before a SecurityViolation would be thrown, however, this method will
* pass to {@link #updateSelf(Experimenter)} <em>if</em> the current user
Expand All @@ -258,6 +259,7 @@ void updateExperimenter(@NotNull
/**
* Updates an experimenter if admin or owner of group.
* Only string fields on the object are taken into account.
* The root and guest experimenters may not be renamed.
*
* @param experimenter
* the Experimenter to update.
Expand All @@ -271,6 +273,7 @@ void updateExperimenterWithPassword(@NotNull
/**
* Updates an experimenter group if admin or owner of group.
* Only string fields on the object are taken into account.
* The root, system and guest groups may not be renamed.
*
* @param group
* the ExperimenterGroup to update.
Expand Down Expand Up @@ -369,7 +372,14 @@ void addGroups(@NotNull
ExperimenterGroup... groups);

/**
* removes a user from the given groups.
* Removes an experimenter from the given groups.
* <ul>
* <li>The root experimenter is required to be in both the user and system groups.</li>
* <li>An experimenter may not remove themself from the user or system group.</li>
* <li>An experimenter may not be a member of only the user group,
* some other group is also required as the default group.</li>
* <li>An experimenter must remain a member of some group.</li>
* </ul>
*
* @param user
* A currently managed entity. Not null.
Expand Down
91 changes: 74 additions & 17 deletions components/common/src/ome/system/Roles.java
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/*
* ome.system.Roles
*
* Copyright 2006 University of Dundee. All rights reserved.
* Copyright 2006-2013 University of Dundee. All rights reserved.
* Use is subject to license terms supplied in LICENSE.txt
*/

Expand All @@ -10,8 +10,8 @@
// Java imports
import java.io.Serializable;

import ome.annotations.RevisionDate;
import ome.annotations.RevisionNumber;
import com.google.common.base.Predicate;

import ome.model.meta.Experimenter;
import ome.model.meta.ExperimenterGroup;

Expand All @@ -21,16 +21,34 @@
* These values are also used during install to initialize the database.
*
* @author Josh Moore, josh.moore at gmx.de
* @version $Revision$, $Date$
* @see ome.model.meta.Experimenter
* @see ome.model.meta.ExperimenterGroup
* @since 3.0-M3
*/
@RevisionDate("$Date$")
@RevisionNumber("$Revision$")
public final class Roles implements Serializable {

private static final long serialVersionUID = -7130017567693194759L;
private static final long serialVersionUID = -2488864989534638213L;

public final Predicate<Experimenter> IS_ROOT_USER = new Predicate<Experimenter>() {
@Override
public boolean apply(Experimenter experimenter) {
return isRootUser(experimenter);
}
};

public final Predicate<ExperimenterGroup> IS_USER_GROUP = new Predicate<ExperimenterGroup>() {
@Override
public boolean apply(ExperimenterGroup group) {
return isUserGroup(group);
}
};

public final Predicate<ExperimenterGroup> IS_SYSTEM_GROUP = new Predicate<ExperimenterGroup>() {
@Override
public boolean apply(ExperimenterGroup group) {
return isSystemGroup(group);
}
};

private final long rId;

Expand All @@ -44,29 +62,45 @@ public final class Roles implements Serializable {

private final String ugName;

private final long guestId;

private final String guestName;

private final long ggId;

private final String ggName;

/** default constructor which assigns hard-coded values to all roles */
public Roles() {
this.rId = 0L;
long nextUserId = 0;
long nextGroupId = 0;
/* these must be defined in the same order as in psql-footer.vm */
this.rId = nextUserId++;
this.rName = "root";
this.sgId = 0L;
this.sgId = nextGroupId++;
this.sgName = "system";
this.ugId = 1L;
this.ugId = nextGroupId++;
this.ugName = "user";
this.guestId = nextUserId++;
this.guestName = "guest";
this.ggId = nextGroupId++;
this.ggName = "guest";
}

/** constructor which allows full specification of all roles */
public Roles(long rootId, String rootName, long systemGroupId,
String systemGroupName, long userGroupId, String userGroupName) {
this.rId = rootId;
this.rName = rootName;
public Roles(long rootUserId, String rootUserName,
long systemGroupId, String systemGroupName, long userGroupId, String userGroupName,
long guestUserId, String guestUserName, long guestGroupId, String guestGroupName) {
this.rId = rootUserId;
this.rName = rootUserName;
this.sgId = systemGroupId;
this.sgName = systemGroupName;
this.ugId = userGroupId;
this.ugName = userGroupName;
this.guestName = "guest";
this.guestId = guestUserId;
this.guestName = guestUserName;
this.ggId = guestGroupId;
this.ggName = guestGroupName;
}

// ~ Checks
Expand Down Expand Up @@ -104,6 +138,20 @@ public String getRootName() {
return rName;
}

/**
* @return the id of the guest user
*/
public long getGuestId() {
return guestId;
}

/**
* @return the {@link Experimenter#getOmeName()} of the guest user
*/
public String getGuestName() {
return guestName;
}

/**
* @return the id of the system group
*/
Expand Down Expand Up @@ -132,8 +180,17 @@ public String getUserGroupName() {
return ugName;
}

public String getGuestGroupName() {
return guestName;
/**
* @return the id of the guest group
*/
public long getGuestGroupId() {
return ggId;
}

/**
* @return the {@link ExperimenterGroup#getName()} of the guest group
*/
public String getGuestGroupName() {
return ggName;
}
}
83 changes: 63 additions & 20 deletions components/server/src/ome/logic/AdminImpl.java
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
/*
* $Id$
*
* Copyright 2006 University of Dundee. All rights reserved.
* Copyright 2006-2013 University of Dundee. All rights reserved.
* Use is subject to license terms supplied in LICENSE.txt
*/

package ome.logic;

import java.sql.SQLException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
Expand All @@ -19,11 +19,15 @@
import javax.persistence.PrimaryKeyJoinColumn;
import javax.persistence.Table;

import org.slf4j.Logger;
import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterators;

import org.hibernate.Criteria;
import org.hibernate.HibernateException;
import org.hibernate.Session;
import org.hibernate.criterion.Restrictions;
import org.slf4j.Logger;
import org.springframework.beans.BeansException;
import org.springframework.context.ApplicationContext;
import org.springframework.context.ApplicationContextAware;
Expand All @@ -35,17 +39,13 @@

import ome.annotations.NotNull;
import ome.annotations.PermitAll;
import ome.annotations.RevisionDate;
import ome.annotations.RevisionNumber;
import ome.annotations.RolesAllowed;
import ome.api.IAdmin;
import ome.api.RawFileStore;
import ome.api.ServiceInterface;
import ome.api.local.LocalAdmin;
import ome.api.local.LocalUpdate;
import ome.conditions.ApiUsageException;
import ome.conditions.AuthenticationException;
import ome.conditions.GroupSecurityViolation;
import ome.conditions.InternalException;
import ome.conditions.SecurityViolation;
import ome.conditions.ValidationException;
Expand All @@ -58,8 +58,6 @@
import ome.model.core.Pixels;
import ome.model.enums.ChecksumAlgorithm;
import ome.model.internal.Permissions;
import ome.model.internal.Permissions.Right;
import ome.model.internal.Permissions.Role;
import ome.model.meta.Experimenter;
import ome.model.meta.ExperimenterGroup;
import ome.model.meta.GroupExperimenterMap;
Expand Down Expand Up @@ -99,14 +97,11 @@
* while developing services. Misuse could circumvent security or auditing.
*
* @author Josh Moore, josh.moore at gmx.de
* @version $Revision:1754 $, $Date:2007-08-20 10:36:07 +0100 (Mon, 20 Aug 2007) $
* @see SecuritySystem
* @see Permissions
* @since 3.0-M3
*/
@Transactional(readOnly = true)
@RevisionDate("$Date:2007-08-20 10:36:07 +0100 (Mon, 20 Aug 2007) $")
@RevisionNumber("$Revision:1754 $")
public class AdminImpl extends AbstractLevel2Service implements LocalAdmin,
ApplicationContextAware {

Expand Down Expand Up @@ -230,11 +225,11 @@ public List<Long> getLeaderOfGroupIds(final Experimenter e) {
qb.and("m.child.id = :id");
qb.param("id", e.getId());

List<Long> groupIds = iQuery.execute(new HibernateCallback() {
public Object doInHibernate(Session session)
List<Long> groupIds = iQuery.execute(new HibernateCallback<List<Long>>() {
public List<Long> doInHibernate(Session session)
throws HibernateException, SQLException {
org.hibernate.Query q = qb.query(session);
return q.list();
return (List<Long>) q.list();
}
});
return groupIds;
Expand Down Expand Up @@ -559,7 +554,19 @@ public void updateExperimenterWithPassword(@NotNull final
*/
private void copyAndSaveExperimenter(final Experimenter experimenter) {
final Experimenter orig = userProxy(experimenter.getId());
orig.setOmeName(experimenter.getOmeName());
final String origOmeName = orig.getOmeName();
final String newOmeName = experimenter.getOmeName();
if (!origOmeName.equals(newOmeName)) {
final Roles roles = getSecurityRoles();
final Set<String> fixedExperimenterNames =
ImmutableSet.of(roles.getRootName(), roles.getGuestName());
if (fixedExperimenterNames.contains(origOmeName)) {
throw new ValidationException("cannot change name of special experimenter '" + origOmeName + "'");
} else if (fixedExperimenterNames.contains(newOmeName)) {
throw new ValidationException("cannot change name to special experimenter '" + newOmeName + "'");
}
}
orig.setOmeName(newOmeName);
orig.setEmail(experimenter.getEmail());
orig.setFirstName(experimenter.getFirstName());
orig.setMiddleName(experimenter.getMiddleName());
Expand All @@ -581,7 +588,19 @@ public void updateGroup(@NotNull final
changePermissions(group, p); // ticket:1776 WORKAROUND
}
final ExperimenterGroup orig = getGroup(group.getId());
orig.setName(group.getName());
final String origName = orig.getName();
final String newName = group.getName();
if (!origName.equals(newName)) {
final Roles roles = getSecurityRoles();
final Set<String> fixedGroupNames =
ImmutableSet.of(roles.getGuestGroupName(), roles.getSystemGroupName(), roles.getUserGroupName());
if (fixedGroupNames.contains(origName)) {
throw new ValidationException("cannot change name of special group '" + origName + "'");
} else if (fixedGroupNames.contains(newName)) {
throw new ValidationException("cannot change name to special group '" + newName + "'");
}
}
orig.setName(newName);
orig.setDescription(group.getDescription());

reallySafeSave(orig);
Expand Down Expand Up @@ -611,7 +630,6 @@ public long createSystemUser(Experimenter newSystemUser) {

@RolesAllowed("user")
@Transactional(readOnly = false)
@SuppressWarnings("unchecked")
public long createExperimenter(final Experimenter experimenter,
ExperimenterGroup defaultGroup, ExperimenterGroup... otherGroups) {

Expand Down Expand Up @@ -684,6 +702,33 @@ public void removeGroups(final Experimenter user, final ExperimenterGroup... gro
}

adminOrPiOfGroups(null, groups);

final Roles roles = getSecurityRoles();
final boolean removeSystemOrUser =
Iterators.any(Iterators.forArray(groups),Predicates.or(roles.IS_SYSTEM_GROUP, roles.IS_USER_GROUP));
if (removeSystemOrUser && roles.isRootUser(user)) {
throw new ValidationException("experimenter '" + roles.getRootName() + "' may not be removed from the '" +
roles.getSystemGroupName() + "' or '" + roles.getUserGroupName() + "' group");
}
final EventContext eventContext = getEventContext();
final boolean userOperatingOnThemself = eventContext.getCurrentUserId().equals(user.getId());
if (removeSystemOrUser && userOperatingOnThemself) {
throw new ValidationException("experimenters may not remove themselves from the '" +
roles.getSystemGroupName() + "' or '" + roles.getUserGroupName() + "' group");
}
final Set<Long> resultingGroupIds = new HashSet<Long>();
for (final GroupExperimenterMap map : user.<GroupExperimenterMap>collectGroupExperimenterMap(null)) {
resultingGroupIds.add(map.parent().getId());
}
for (final ExperimenterGroup group : groups) {
resultingGroupIds.remove(group.getId());
}
if (resultingGroupIds.isEmpty()) {
throw new ValidationException("experimenter must remain a member of some group");
} else if (resultingGroupIds.equals(Collections.singleton(roles.getUserGroupId()))) {
throw new ValidationException("experimenter cannot be a member of only the '" +
roles.getUserGroupName() + "' group, a different default group is also required");
}
roleProvider.removeGroups(user, groups);

getBeanHelper().getLogger().info(
Expand Down Expand Up @@ -878,7 +923,6 @@ public void changeOwner(IObject iObject, String omeName) {
@RolesAllowed("user")
@Transactional(readOnly = false)
public void changeGroup(IObject iObject, String groupName) {
final LocalUpdate update = iUpdate;
// should take a group
final IObject copy = iQuery.get(iObject.getClass(), iObject.getId());
final ExperimenterGroup group = groupProxy(groupName);
Expand Down Expand Up @@ -1274,7 +1318,6 @@ else if (id != null) {
// ~ group permissions
// =========================================================================

@SuppressWarnings("unchecked")
private Set<String> classes() {
return getExtendedMetadata().getClasses();
}
Expand Down

0 comments on commit 69367ab

Please sign in to comment.