Add wrapper class for loading permission info (Fix #12474) by joshmoore · Pull Request #3910 · ome/openmicroscopy

joshmoore · 2015-06-26T07:36:35Z

Since no solution for properly loading a Permissions object
when the target of a project query (select d.details.permissions),
this workaround is being added which will allow wrapping a
object in HQL (select new ome.util.PermDetails(d)) to produce
the desired effect.

Testing

Primary effect of this PR is that the test_permissions.py tests which were marked xfail should start passing:

components/tools/OmeroPy$ ./setup.py test -t test/integration/test_permissions.py -k Projection -svx
...
test/integration/test_permissions.py::TestPermissionProjections::testProjectionPermissions[rw-----admin-admin] PASSED
test/integration/test_permissions.py::TestPermissionProjections::testProjectionPermissions[rw-----admin-owner] PASSED
test/integration/test_permissions.py::TestPermissionProjections::testProjectionPermissionsWorkaround[rwrw---member-owner] PASSED
test/integration/test_permissions.py::TestPermissionProjections::testProjectionPermissionsWorkaround[rwrw---member-member] PASSED
test/integration/test_permissions.py::TestPermissionProjections::testExtendedRestrictions[PlateI] SKIPPED
test/integration/test_permissions.py::TestPermissionProjections::testExtendedRestrictions[WellI] SKIPPED
test/integration/test_permissions.py::TestPermissionProjections::testExtendedRestrictions[ImageI] SKIPPED
test/integration/test_permissions.py::TestPermissionProjections::testExtendedRestrictions[FileAnnotationI] SKIPPED

================================================================== 30 tests deselected by '-kProjection' ===================================================================
========================================================== 72 passed, 4 skipped, 30 deselected in 141.34 seconds ===========================================================

For there to be further effects, queries will need to be changed from select d.details.permissions to new map(d as d_details_permissions).

joshmoore · 2015-06-26T07:40:10Z

cc: @will-moore @dpwrussell @jburel @chris-allan @knabar

Looks like the wrapper is adding worst case approx. 10ms overhead for a single object return:

 5.964994 v  9.224892
 6.411076 v  9.506226
24.404049 v 39.561033
 6.895065 v 17.786980
 6.025076 v 11.584997
21.947861 v 36.419868
29.384136 v 35.341978
21.653891 v 34.821987
23.499012 v 35.854816
23.951054 v 48.586130
25.995970 v 37.803888
25.675058 v 36.134005
24.055958 v 37.276983
6.199121  v  8.799076
6.303072  v  8.976936
6.533861  v  9.315014
28.892994 v 41.266918
24.214029 v 35.775185
19.517899 v 35.866976
21.919012 v 31.142950
18.978119 v 40.296078
28.111935 v 34.930944
24.958134 v 34.609079
27.335167 v 33.847094
24.810791 v 40.127993
29.417992 v 44.022083
23.461819 v 35.982132
22.911787 v 29.793978

(That's min(details.permissions) v max(PermDetails)). We'd have to look into the larger calls that the jstree branch is making.

Since no solution for properly loading a Permissions object when the target of a project query (`select d.details.permissions`), this workaround is being added which will allow wrapping a object in HQL (`select new ome.util.PermDetails(d)`) to produce the desired effect.

will-moore · 2015-07-23T12:45:35Z

@joshmoore What is the correct way of doing something like this:

        select project.id,
               project.name,
               project.details.owner.id,
               new ome.util.PermDetails(project),
               (select count(id) from ProjectDatasetLink pdl
                where pdl.parent = project.id)
        from Project project

since this gives me a Query Exception:

unexpected token: , near line 4, column 40 [

joshmoore · 2015-07-23T13:11:24Z

I fear we may have just run into https://hibernate.atlassian.net/browse/HHH-2460

will-moore · 2015-07-23T14:03:05Z

Hmmm - bummer. Doesn't look like that is going to be fixed. Any ideas for a workaround?

Due to HHH-2460, once the `new PermDetails()` object is selected, other objects can not be selected at the same time. Instead, we now make use of a single top layer `new map` and detect objects based on the key: `new map(p as p_details_permissions)`. Rather than getting a map of perms as `rv[0][0]`, now a map with the map is returned: `rv[0][0]["p_details_permissions"]`.

joshmoore · 2015-07-23T18:47:57Z

@will-moore : can you give joshmoore@7785939 a try? (We're now deeper into the HHH rabbit hole...)

will-moore · 2015-07-23T22:49:56Z

@joshmoore It's looking good... thanks!

joshmoore · 2015-07-24T12:17:32Z

NB: #3919 should be updated before merging.

joshmoore · 2015-07-27T19:18:55Z

Adding breaking to match gh-3995.

joshmoore · 2015-08-25T18:43:04Z

Removed breaking as per #3995 (comment)

sbesson · 2015-08-26T06:53:51Z

This link generates a Javadoc warning

Constructing Javadoc information... Standard Doclet version 1.8.0_51 Building tree for all the packages and classes... /opt/hudson/workspace/OMERO-DEV-merge-build/src/components/model/src/ome/util/PermDetails.java:54: warning - Tag @link: reference not found: Permissions Building index for all the packages and classes...

joshmoore · 2015-08-27T08:01:25Z

Pushed

sbesson · 2015-08-28T10:46:07Z

https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-merge-integration-python/64/testReport/OmeroPy.test.integration.test_permissions/TestPermissionProjections/ certainly looks good as well as the wrapper class as a workaround. Deferring to the jstree testing team to know whether this has caused some performance issues /cc @pwalczysko?

pwalczysko · 2015-09-01T12:39:26Z

@joshmoore @sbesson : The last jstree testing was performed on 28 August . No performance issues were detected. Is this date including this PR ?

sbesson · 2015-09-01T12:40:26Z

@pwalczysko definitely, I think this PR was always included in the jstree testing because there is a dependency.

joshmoore · 2015-09-01T12:40:41Z

Definitely. This has been out of breaking for over a week (the entire time jstree has been out of breaking).

pwalczysko · 2015-09-01T12:41:05Z

In that case no objections.

Add wrapper class for loading permission info (Fix #12474)

joshmoore · 2015-09-03T07:06:31Z

--no-rebase

PermDetails objects are being passed to the BasicACLVoter.postProcess method when `new map(x_details_permissions)` is used in the HQL query. Not having access to the original object confuses the security system leading to incorrect can* values. This permits unwrapping of internal PermDetails instances. See: * ome#3910 * https://trello.com/c/QbdB9M8k/115-canchgrp-canchown-bug

Add wrapper class for loading permission info (Fix #12474)

PermDetails objects are being passed to the BasicACLVoter.postProcess method when `new map(x_details_permissions)` is used in the HQL query. Not having access to the original object confuses the security system leading to incorrect can* values. This permits unwrapping of internal PermDetails instances. See: * ome#3910 * https://trello.com/c/QbdB9M8k/115-canchgrp-canchown-bug

PermDetails objects are being passed to the BasicACLVoter.postProcess method when `new map(x_details_permissions)` is used in the HQL query. Not having access to the original object confuses the security system leading to incorrect can* values. This permits unwrapping of internal PermDetails instances. See: * ome/openmicroscopy#3910 * https://trello.com/c/QbdB9M8k/115-canchgrp-canchown-bug

joshmoore force-pushed the 12474-perm-wrapper branch from 9d2e487 to fe086fd Compare June 26, 2015 07:55

jburel added the develop label Jun 29, 2015

joshmoore added the ON_HOLD label Jul 20, 2015

Handle null return objects

7708c89

will-moore mentioned this pull request Jul 27, 2015

Jstree 2015 #3995

Merged

joshmoore added breaking and removed ON_HOLD labels Jul 27, 2015

joshmoore removed the breaking label Aug 25, 2015

sbesson reviewed Aug 26, 2015
View reviewed changes

Fix javadoc issues

400584f

snoopycrimecop mentioned this pull request Sep 1, 2015

improve Server and Blitz Javadoc #4107

Merged

sbesson added a commit that referenced this pull request Sep 1, 2015

Merge pull request #3910 from joshmoore/12474-perm-wrapper

00a886e

Add wrapper class for loading permission info (Fix #12474)

sbesson merged commit 00a886e into ome:develop Sep 1, 2015

mtbc added a commit to mtbc/openmicroscopy that referenced this pull request Sep 1, 2015

restore change since ome#3910 was merged

7c98cdf

sbesson added this to the 5.2.0 milestone Oct 2, 2015

joshmoore deleted the 12474-perm-wrapper branch December 4, 2015 09:22

joshmoore mentioned this pull request Aug 14, 2017

Unwrap PermDetails for canChgrp/canChown detection #5437

Merged

rgozim pushed a commit to rgozim/openmicroscopy that referenced this pull request Oct 8, 2018

Merge pull request ome#3910 from joshmoore/12474-perm-wrapper

cdb139f

Add wrapper class for loading permission info (Fix #12474)

Conversation

joshmoore commented Jun 26, 2015

Testing

Uh oh!

joshmoore commented Jun 26, 2015

Uh oh!

will-moore commented Jul 23, 2015

Uh oh!

joshmoore commented Jul 23, 2015

Uh oh!

will-moore commented Jul 23, 2015

Uh oh!

joshmoore commented Jul 23, 2015

Uh oh!

will-moore commented Jul 23, 2015

Uh oh!

joshmoore commented Jul 24, 2015

Uh oh!

joshmoore commented Jul 27, 2015

Uh oh!

joshmoore commented Aug 25, 2015

Uh oh!

sbesson Aug 26, 2015

Choose a reason for hiding this comment

Uh oh!

joshmoore commented Aug 27, 2015

Uh oh!

sbesson commented Aug 28, 2015

Uh oh!

pwalczysko commented Sep 1, 2015

Uh oh!

sbesson commented Sep 1, 2015

Uh oh!

joshmoore commented Sep 1, 2015

Uh oh!

pwalczysko commented Sep 1, 2015

Uh oh!

joshmoore commented Sep 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants