From 1b0fef7d98348bd29773a5cb9b7fab296883e069 Mon Sep 17 00:00:00 2001 From: Chris Allan Date: Tue, 18 Sep 2012 12:27:25 +0100 Subject: [PATCH 1/6] Test case for admin querying with "omero.group = -1". (See #9632) --- components/tools/OmeroPy/test/integration/permissions.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/components/tools/OmeroPy/test/integration/permissions.py b/components/tools/OmeroPy/test/integration/permissions.py index 27c4700ed5b..8c55b60e308 100644 --- a/components/tools/OmeroPy/test/integration/permissions.py +++ b/components/tools/OmeroPy/test/integration/permissions.py @@ -652,6 +652,15 @@ def testPrivateGroupCallContext(self): qb.get("TagAnnotation", tid, specific) # Not currently in gid qb.get("TagAnnotation", tid, negone) + # Reading with an admin user + # ============================================== + + def testAdminCanQueryWithGroupMinusOneTicket9632(self): + q = self.root.sf.getQueryService() + ctx = self.root.ic.getImplicitContext().getContext() + ctx["omero.group"] = "-1" + q.findAllByQuery('select p from Project as p', None, ctx) + # Use of omero.user # ============================================== From 1cbf3b9c5a3b8433de2b02896ea9190be63ca6a6 Mon Sep 17 00:00:00 2001 From: jmoore Date: Wed, 19 Sep 2012 09:03:50 +0200 Subject: [PATCH 2/6] Fix bad comparison of getCurrentGroupId (See #9632) --- components/server/src/ome/security/basic/BasicACLVoter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/server/src/ome/security/basic/BasicACLVoter.java b/components/server/src/ome/security/basic/BasicACLVoter.java index def4c1046a7..4e04f9a566a 100644 --- a/components/server/src/ome/security/basic/BasicACLVoter.java +++ b/components/server/src/ome/security/basic/BasicACLVoter.java @@ -125,7 +125,7 @@ public boolean allowLoad(Session session, Class klass, Detail boolean rv = securityFilter.passesFilter(session, d, currentUser.current()); // Misusing this location to store the loaded objects perms for later. - if (this.currentUser.getCurrentEventContext().getCurrentGroupId() < 1) { + if (this.currentUser.getCurrentEventContext().getCurrentGroupId() < 0) { // For every object that gets loaded when omero.group = -1, we // cache it's permissions in the session context so that when the // session is over we can re-apply all the permissions. From e88ec6c878bf977c71f8528aaa679f45e1c7cc19 Mon Sep 17 00:00:00 2001 From: jmoore Date: Wed, 19 Sep 2012 10:15:13 +0200 Subject: [PATCH 3/6] Load perms for system groups as well (Fix #9632) Data that was 1) in the system group and 2) queried via omero.group=-1 was causing an internal exception. The short-circuit block at the top of allowLoad was failing to cache permissions for core groups. --- .../src/ome/security/basic/BasicACLVoter.java | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/components/server/src/ome/security/basic/BasicACLVoter.java b/components/server/src/ome/security/basic/BasicACLVoter.java index 4e04f9a566a..e266fe5b787 100644 --- a/components/server/src/ome/security/basic/BasicACLVoter.java +++ b/components/server/src/ome/security/basic/BasicACLVoter.java @@ -115,14 +115,16 @@ public boolean allowChmod(IObject iObject) { public boolean allowLoad(Session session, Class klass, Details d, long id) { Assert.notNull(klass); + boolean rv = false; if (d == null || sysTypes.isSystemType(klass) || sysTypes.isInSystemGroup(d) || sysTypes.isInUserGroup(d)) { - return true; + rv = true; + } + else { + rv = securityFilter.passesFilter(session, d, currentUser.current()); } - - boolean rv = securityFilter.passesFilter(session, d, currentUser.current()); // Misusing this location to store the loaded objects perms for later. if (this.currentUser.getCurrentEventContext().getCurrentGroupId() < 0) { @@ -130,10 +132,19 @@ public boolean allowLoad(Session session, Class klass, Detail // cache it's permissions in the session context so that when the // session is over we can re-apply all the permissions. ExperimenterGroup g = d.getGroup(); + if (g == null) { + log.warn(String.format("Group null while loading %s:%s", + klass.getName(), id)); + } if (g != null) { // Null for system types Long gid = g.getId(); Permissions p = g.getDetails().getPermissions(); - this.currentUser.current().setPermissionsForGroup(gid, p); + if (p == null) { + log.warn(String.format("Permissions null for group %s " + + "while loading %s:%s", gid, klass.getName(), id)); + } else { + this.currentUser.current().setPermissionsForGroup(gid, p); + } } } From 4ce9217a38e610aa10e266253c4bc8eb812591d0 Mon Sep 17 00:00:00 2001 From: jmoore Date: Wed, 19 Sep 2012 11:58:17 +0200 Subject: [PATCH 4/6] Handle systypes and null details separately (See #9632) Neither of these types will have a group so short-circuiting the if/else will prevent a WARN statement. --- .../src/ome/security/basic/BasicACLVoter.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/components/server/src/ome/security/basic/BasicACLVoter.java b/components/server/src/ome/security/basic/BasicACLVoter.java index e266fe5b787..5cf3a524736 100644 --- a/components/server/src/ome/security/basic/BasicACLVoter.java +++ b/components/server/src/ome/security/basic/BasicACLVoter.java @@ -115,10 +115,17 @@ public boolean allowChmod(IObject iObject) { public boolean allowLoad(Session session, Class klass, Details d, long id) { Assert.notNull(klass); + if (d == null || sysTypes.isSystemType(klass)) { + // Here we're returning true because there + // will be no group value that we can use + // to store any permissions and don't want + // WARNS in the log. + + return true; // EARLY EXIT! + } + boolean rv = false; - if (d == null || - sysTypes.isSystemType(klass) || - sysTypes.isInSystemGroup(d) || + if (sysTypes.isInSystemGroup(d) || sysTypes.isInUserGroup(d)) { rv = true; } From 2482a7f7f5005ca5daacbf0686e814880be2374b Mon Sep 17 00:00:00 2001 From: jmoore Date: Wed, 19 Sep 2012 17:19:25 +0200 Subject: [PATCH 5/6] Fix canEdit corruption (See #9635) --- .../server/src/ome/security/basic/BasicACLVoter.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/components/server/src/ome/security/basic/BasicACLVoter.java b/components/server/src/ome/security/basic/BasicACLVoter.java index 5cf3a524736..3fae952f857 100644 --- a/components/server/src/ome/security/basic/BasicACLVoter.java +++ b/components/server/src/ome/security/basic/BasicACLVoter.java @@ -393,7 +393,14 @@ public void postProcess(IObject object) { // This order must match the ordered of restrictions[] // expected by p.copyRestrictions Scope.LINK, Scope.EDIT, Scope.DELETE, Scope.ANNOTATE); - p.copyRestrictions(allow); + + // #9635 - This is not the most efficient solution + // But since it's unclear why Permission objects + // are currently being shared, the safest solution + // is to always produce a copy. + Permissions copy = new Permissions(p); + copy.copyRestrictions(allow); + details.setPermissions(copy); // #9635 } } From 3cc848b05371a767d47a413a575b8a14e618d3b3 Mon Sep 17 00:00:00 2001 From: jmoore Date: Wed, 19 Sep 2012 17:19:40 +0200 Subject: [PATCH 6/6] Minor fixes to group.py --- components/tools/OmeroPy/src/omero/plugins/group.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/tools/OmeroPy/src/omero/plugins/group.py b/components/tools/OmeroPy/src/omero/plugins/group.py index 85778b2a97e..6884dd681b4 100755 --- a/components/tools/OmeroPy/src/omero/plugins/group.py +++ b/components/tools/OmeroPy/src/omero/plugins/group.py @@ -129,7 +129,7 @@ def find_group(self, admin, id_or_name): gid = g.id.val return gid, g except omero.ApiUsageException: - self.ctx.die(503, "Unknown group: %s" % gid) + self.ctx.die(503, "Unknown group: %s" % id_or_name) def find_user(self, admin, id_or_name): import omero @@ -142,7 +142,7 @@ def find_user(self, admin, id_or_name): uid = u.id.val return uid, u except omero.ApiUsageException: - self.ctx.die(503, "Unknown experimenter: %s" % uid) + self.ctx.die(503, "Unknown experimenter: %s" % id_or_name) def perms(self, args):