diff --git a/components/server/src/ome/security/basic/BasicACLVoter.java b/components/server/src/ome/security/basic/BasicACLVoter.java index def4c1046a7..3fae952f857 100644 --- a/components/server/src/ome/security/basic/BasicACLVoter.java +++ b/components/server/src/ome/security/basic/BasicACLVoter.java @@ -115,25 +115,43 @@ 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) || - sysTypes.isInSystemGroup(d) || - sysTypes.isInUserGroup(d)) { - return true; + 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 = securityFilter.passesFilter(session, d, currentUser.current()); + boolean rv = false; + if (sysTypes.isInSystemGroup(d) || + sysTypes.isInUserGroup(d)) { + rv = true; + } + else { + 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. 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); + } } } @@ -375,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 } } 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): 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 # ==============================================