ticket:1781 - Fixing issues with exp/grp update ('''API CHANGE!''')

For implementation reasons, we are changing the contract of the IAdmin
"update*" methods to only respect the string changes on the objects, and
not all the links for now.

git-svn-id: file:///home/svn/omero/trunk@6066 05709c45-44f0-0310-885b-81a1db45b4a6

components/common/src/ome/api/IAdmin.java

@@ -214,8 +214,8 @@ void updateSelf(@NotNull
     Experimenter experimenter);
 
     /**
-     * Updates an experimenter as admin. All aspects of the passed object are
-     * taken into account including omeName, groups, and default group.
+     * Updates an experimenter if admin or owner of group.
+     * Only string fields on the object are taken into account.
      * 
      * @param experimenter
      *            the Experimenter to update.
@@ -224,8 +224,8 @@ void updateExperimenter(@NotNull
     Experimenter experimenter);
 
     /**
-     * Updates an experimenter as admin. All aspects of the passed object are
-     * taken into account including omeName, groups, and default group.
+     * Updates an experimenter if admin or owner of group.
+     * Only string fields on the object are taken into account.
      * 
      * @param experimenter
      *            the Experimenter to update.
@@ -237,8 +237,8 @@ void updateExperimenterWithPassword(@NotNull
     String password);
 
     /**
-     * Updates a group. All aspects of the passed object are taken into account
-     * including group name and the included users.
+     * Updates an experimenter group if admin or owner of group.
+     * Only string fields on the object are taken into account.
      * 
      * @param group
      *            the ExperimenterGroup to update.

components/server/src/ome/logic/AdminImpl.java

@@ -65,6 +65,7 @@
 import ome.system.Roles;
 import ome.system.SimpleEventContext;
 import ome.tools.hibernate.QueryBuilder;
+import ome.tools.hibernate.SecureMerge;
 import ome.util.Utils;
 
 import org.apache.commons.logging.Log;
@@ -110,6 +111,8 @@ public class AdminImpl extends AbstractLevel2Service implements LocalAdmin,
 
     protected final SessionFactory sf;
 
+    protected final ome.tools.hibernate.SessionFactory osf;
+
     protected final MailSender mailSender;
 
     protected final SimpleMailMessage templateMessage;
@@ -138,6 +141,7 @@ public AdminImpl(SimpleJdbcOperations jdbc, SessionFactory sf,
         this.aclVoter = aclVoter;
         this.passwordProvider = passwordProvider;
         this.roleProvider = roleProvider;
+        this.osf = new ome.tools.hibernate.SessionFactory(sf);
     }
 
     public Class<? extends ServiceInterface> getServiceInterface() {
@@ -412,47 +416,51 @@ public void updateExperimenter(@NotNull final
     Experimenter experimenter) {
         adminOrPiOfUser(experimenter);
         String name = experimenter.getOmeName();
-        getSecuritySystem().runAsAdmin(new AdminAction() {
-            public void runAsAdmin() {
-                iUpdate.saveObject(experimenter);
-            }
-        });
+        copyAndSaveExperimenter(experimenter);
         getBeanHelper().getLogger().info("Updated user info for " + name);
     }
 
     @RolesAllowed("user")
     public void updateExperimenterWithPassword(@NotNull final
     Experimenter experimenter, final String password) {
         adminOrPiOfUser(experimenter);
-        final String name = experimenter.getOmeName();
-        getSecuritySystem().runAsAdmin(new AdminAction() {
-            public void runAsAdmin() {
-                iUpdate.saveObject(experimenter);
-                changeUserPassword(name, password);
-            }
-        });
+        copyAndSaveExperimenter(experimenter);
+        final Experimenter orig = userProxy(experimenter.getId());
+        String name = orig.getOmeName();
+        changeUserPassword(name, password);
         getBeanHelper().getLogger().info(
                 "Updated user info and password for " + name);
     }
 
+    /**
+     * @param experimenter
+     */
+    private void copyAndSaveExperimenter(final Experimenter experimenter) {
+        final Experimenter orig = userProxy(experimenter.getId());
+        orig.setOmeName(experimenter.getOmeName());
+        orig.setEmail(experimenter.getEmail());
+        orig.setFirstName(experimenter.getFirstName());
+        orig.setLastName(experimenter.getLastName());
+        orig.setInstitution(experimenter.getInstitution());
+        reallySafeSave(orig);
+    }
+
     @RolesAllowed("user")
     public void updateGroup(@NotNull final
     ExperimenterGroup group) {
         adminOrPiOfGroup(group);
-        getSecuritySystem().runAsAdmin(new AdminAction() {
-            public void runAsAdmin() {
-                Permissions p = group.getDetails().getPermissions();
-                if (p != null) {
-                    // Setting permissions is not allowed via IUpdate
-                    // so use the logic in changePermissions and then
-                    // reset permissions to the current value.
-                    changePermissions(group, p); // ticket:1776 WORKAROUND
-                    p = getGroup(group.getId()).getDetails().getPermissions();
-                    group.getDetails().setPermissions(p);
-                }
-                iUpdate.saveObject(group);
-            }
-        });
+        Permissions p = group.getDetails().getPermissions();
+        if (p != null) {
+            // Setting permissions is not allowed via IUpdate
+            // so use the logic in changePermissions and then
+            // reset permissions to the current value.
+            changePermissions(group, p); // ticket:1776 WORKAROUND
+        }
+        final ExperimenterGroup orig = getGroup(group.getId());
+        orig.setName(group.getName());
+        orig.setDescription(group.getDescription());
+
+        reallySafeSave(orig);
         getBeanHelper().getLogger().info("Updated group info for " + group);
     }
 
@@ -788,6 +796,7 @@ public void changePermissions(final IObject iObject, final Permissions perms) {
 
         // ticket:1434
         if (iObject instanceof ExperimenterGroup) {
+            adminOrPiOfGroup((ExperimenterGroup) iObject);
             handleGroupChange(iObject.getId(), perms);
             return; // EARLY EXIT!
         }
@@ -1239,7 +1248,7 @@ private void handleGroupChange(Long id, Permissions newPerms) {
             throw new ApiUsageException("PERMS cannot be null");
         }
 
-        final Session s = new ome.tools.hibernate.SessionFactory(sf).getSession();
+        final Session s = osf.getSession();
         final ExperimenterGroup group = (ExperimenterGroup) s.get(ExperimenterGroup.class, id);
         final Permissions oldPerms = group.getDetails().getPermissions();
 
@@ -1272,6 +1281,22 @@ private void handleGroupChange(Long id, Permissions newPerms) {
     // ticket:1781 - group-owner admin privileges
     // =========================================================================
 
+    /**
+     * Saves an object as admin.
+     *
+     * Due to the disabling of the MergeEventListener, it is necessary to
+     * jump through several hoops to get non-admin saving of system types
+     * to work properly.
+     */
+    private void reallySafeSave(final IObject obj) {
+        final Session session = osf.getSession();
+        sec.doAction(new SecureMerge(session), obj);
+        sec.runAsAdmin(new AdminAction(){
+            public void runAsAdmin() {
+                session.flush();
+            }});
+    }
+
     private boolean isAdmin() {
         return getEventContext().isCurrentUserAdmin();
     }

components/server/src/ome/security/auth/SimpleRoleProvider.java

@@ -13,7 +13,6 @@
 
 import ome.conditions.ApiUsageException;
 import ome.conditions.ValidationException;
-import ome.model.IObject;
 import ome.model.internal.Permissions;
 import ome.model.internal.Permissions.Right;
 import ome.model.internal.Permissions.Role;
@@ -23,6 +22,7 @@
 import ome.security.SecureAction;
 import ome.security.SecuritySystem;
 import ome.tools.hibernate.HibernateUtils;
+import ome.tools.hibernate.SecureMerge;
 import ome.tools.hibernate.SessionFactory;
 
 import org.apache.commons.logging.Log;
@@ -262,16 +262,4 @@ private Experimenter userById(long id, Session s) {
     private ExperimenterGroup groupById(long id, Session s) {
         return (ExperimenterGroup) s.load(ExperimenterGroup.class, id);
     }
-
-    private final class SecureMerge implements SecureAction {
-        private final Session session;
-
-        private SecureMerge(Session session) {
-            this.session = session;
-        }
-
-        public <T extends IObject> T updateObject(T... objs) {
-            return (T) session.merge(objs[0]);
-        }
-    }
 }

components/server/src/ome/tools/hibernate/SecureMerge.java

@@ -0,0 +1,35 @@
+/*
+ *   $Id$
+ *
+ *   Copyright 2009 Glencoe Software, Inc. All rights reserved.
+ *   Use is subject to license terms supplied in LICENSE.txt
+ */
+
+package ome.tools.hibernate;
+
+import ome.model.IObject;
+import ome.security.SecureAction;
+import ome.security.auth.SimpleRoleProvider;
+
+import org.hibernate.Session;
+
+/**
+ * Helper originally from {@link SimpleRoleProvider} for merging admin objects.
+ *
+ * @author Josh Moore, josh at glencoesoftware.com
+ * @since 4.0
+ *
+ */
+public class SecureMerge implements SecureAction {
+
+    private final Session session;
+
+    public SecureMerge(Session session) {
+        this.session = session;
+    }
+
+    public <T extends IObject> T updateObject(T... objs) {
+        return (T) session.merge(objs[0]);
+    }
+
+}

components/server/test/ome/server/itests/perms42/AdminPermsTest.java

@@ -28,15 +28,57 @@
 @Test(groups = "ticket:1434")
 public class AdminPermsTest extends PermissionsTest {
 
+    @Test
+    public void testUpdateSelf() {
+        setup(Permissions.PRIVATE);
+        fixture.user.setEmail(uuid());
+        iAdmin.updateSelf(fixture.user);
+    }
+
+    @Test
+    public void testUpdateExperimenter() {
+        setup(Permissions.PRIVATE);
+        Experimenter other = loginNewUserInOtherUsersGroup(fixture.user);
+        fixture.log_in();
 
-    // METHODS:
-    // --------------------
-    // updateExperimenter
-    // updateExperimenterWithPassword
-    // updateGroup
+        try {
+            other.setEmail(uuid());
+            iAdmin.updateExperimenter(other);
+            fail("sec-vio");
+        } catch (SecurityViolation sv) {
+            // goood
+        }
+
+        try {
+            other.setEmail(uuid());
+            iAdmin.updateExperimenterWithPassword(other, uuid());
+            fail("sec-vio");
+        } catch (SecurityViolation sv) {
+            // goood
+        }
+
+        fixture.make_leader();
+
+        other.setEmail(uuid());
+        iAdmin.updateExperimenter(other);
+
+        other.setEmail(uuid());
+        iAdmin.updateExperimenterWithPassword(other, uuid());
+
+    }
+
+    @Test
+    public void testUpdateGroup() {
+        setup(Permissions.PRIVATE);
+        fixture.make_leader();
+        ExperimenterGroup g = fixture.group();
+        g.setName(uuid());
+        // g.getDetails().setPermissions(Permissions.SHARED);
+        iAdmin.updateGroup(g);
+    }
 
     @Test
-    public void testUser() {
+    public void testCreateUser() {
 
         // Non-member group to be used as dummy
         loginRoot();