privileges canChgrp by will-moore · Pull Request #5352 · ome/openmicroscopy

will-moore · 2017-06-29T14:28:09Z

What this PR does

Fixes bug of Light Admin without canChgrp privilege still being able to do chgrp in webcilent.
See "crash on Move in not-your-group" checkbox in https://trello.com/c/LUzfIhaU/46-crashes-in-webclient

Testing this PR

Create a Light Admin user without Chgrp privilege
User shouldn't be able to chgrp other users' data (right-click menu item "Move to Group" should be disabled.
Full Admin or Light Admin with Chgrp privilege should still be able to move others' data.
Users or Light Admins without Chgrp should still be able to move their own data.

cc @pwalczysko @mtbc

mtbc · 2017-06-29T14:32:49Z

It's probably outwith this PR but FYI when one has the actual object's details in hand we now have a couple more can* methods that should always work: canChgrp() and canChown() at http://www.openmicroscopy.org/site/support/omero5.3-staging/developers/Server/Permissions.html#permissions-object.

will-moore · 2017-06-29T14:53:43Z

@mtbc - It would be nice to user server logic for canChgrp but I don't see that flag on that permissions object.
This permissions object is returned from the "magic" mapping of project_details_permissions in this query:

        select new map(project.id as id,
               project.name as name,
               project.details.owner.id as ownerId,
               project as project_details_permissions,
               (select count(id) from ProjectDatasetLink pdl
                where pdl.parent = project.id) as childCount)
        from Project project

When I print it, it's just a dict:

{'canDelete': True, 'canAnnotate': True, 'canEdit': True, 'perm': 'rwr---', 'canLink': True}

mtbc · 2017-06-29T15:08:32Z

Huh, that's interesting. Your omero_model_Permissions_ice.py has canChgrp? I wonder if there's some Pythonic thing somewhere on the way to the web client that I didn't know about. canChgrp() is working fine from the Java integration tests.

pwalczysko · 2017-06-29T16:34:32Z

travis failing.

will-moore · 2017-06-30T08:43:04Z

Ah, I didn't realise that @mtbc's PR wasn't getting included in the merged build yet.
So, currently this PR is causing a crash when you try to load another user's data in the tree.
I need to exclude this or open against roles. Currently impossible to test my other PRs.

mtbc · 2017-06-30T08:44:11Z

My PR's in breaking: feel free to add --breaking and use https://ci.openmicroscopy.org/job/OMERO-DEV-breaking-trigger/.

pwalczysko · 2017-07-03T11:55:57Z

All four tests from the header of this PR passed. All works as expected.
Although the canChgrp missing question is still to be solved, I suggest we merge this PR as it fixes the crashes in appropriate manner.

jburel · 2017-07-04T09:02:14Z

not included in today's build
Removing breaking label so it goes along @mtbc "migration" PR

mtbc · 2017-07-04T11:24:22Z

With #5358 that kind of query now gives me results like {'canDelete': True, 'canAnnotate': False, 'perm': 'rw----', 'canChown': False, 'canChgrp': False, 'canLink': False, 'canEdit': True}. (Not a blocker for this PR though.)

sbesson

One stylistic request, then this is good to merge.

sbesson · 2017-07-14T10:48:48Z

        permissionsCss.append("isOwned")
-    if ownerid == conn.getUserId() or conn.isAdmin():
+    if (ownerid == conn.getUserId() or
+            "Chgrp" in conn.getEventContext().adminPrivileges):


Looks good. For unification of our codebase, can this be adjusted to use the enums as in https://github.com/openmicroscopy/openmicroscopy/pull/5376/files#diff-4c592a4ec58d6f4830c88b81f8aa77a3R66

Don't need this now since we're not using getEventContext in unit test This reverts commit cf8620e.

will-moore · 2017-07-31T23:02:28Z

Use canChgrp from #5358 now.
Needs retest.

pwalczysko · 2017-08-02T17:19:26Z

The UI retest indicated in #5352 (comment) and started according to the header of this PR fails on:

full admin has no "Move" menu item enabled on any objects of another user (user-4, who is a member of at least 2 groups)
the same for light admin with Chgrp privilege

will-moore · 2017-08-03T08:29:45Z

For some reason, the functionality added in #5358 has stopped working and the project_details_permissions map queries return a permissions dict that is missing the canChgrp and canChown flags.
This is why @pwalczysko's retest failed, but we also see Robot test failures etc.

mtbc · 2017-08-10T14:08:03Z

I haven't yet managed to reproduce the missing flags locally (via bin/omero hql --all ...) but I'll keep investigating.

mtbc · 2017-08-11T12:47:23Z

So, while I am always getting those map entries, they are wrongly false even for root so they may not be of much help anyway. This fell through the cracks because I had no idea that _details_permissions was possible. I created https://trello.com/c/QbdB9M8k/115-canchgrp-canchown-bug and am not yet easily seeing a fix.

will-moore · 2017-08-11T14:54:25Z

@mtbc Before this PR, there was no check that the permissions loaded like this:

perms2 = unwrap(reader.projection((
                "select new map(p.id as id, p as p_details_permissions) "
                "from Project p where p.id = :id"),
                ParametersI().addId(project.id.val)))[0][0]

were correct, partly because the ProjectionFixtures at https://github.com/openmicroscopy/openmicroscopy/blob/develop/components/tools/OmeroPy/src/omero/testlib/__init__.py#L1132 didn't have the canChgrp and canChown added to them, and because the def assertPerms(perms, fixture) used in the test didn't check for canChgrp and canChown. Adding these checks on the 1st August caused the tests to start failing.

will-moore · 2017-08-14T09:08:06Z

@mtbc I'll close this until we have canChgrp flag working in _details_permissions dict, so we don't have so many tests failing... cc @joshmoore

joshmoore · 2017-08-15T05:30:12Z

Ooops. Forgot to point out that this can likely be re-opened with #5437 though at this point we can await the status of this mornings integration tests: https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration/748/

will-moore · 2017-08-15T11:55:28Z

Reopening this to test against #5437.

will-moore · 2017-08-16T05:34:54Z

Test permissions now passing with #5437 https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-Python27/630/testReport/OmeroPy.test.integration.test_permissions/

joshmoore · 2017-08-16T09:21:46Z

            # isOwned and canChgrp is True
            expected.append('isOwned')
-            expected.append('canChgrp')
+            # expected.append('canChgrp')


Can this be removed?

joshmoore · 2017-08-16T09:29:08Z

@sbesson : I assume you're happy with this?

mtbc

Great to see all the tests.

Use eventContext adminPrivileges 'Chgrp' for 'canChgrp' flag in jsTree

13a5f08

Fix unit tests, adding getEventContext() to MockConnection

cf8620e

jburel added the develop label Jul 3, 2017

ome deleted a comment from will-moore Jul 4, 2017

sbesson requested changes Jul 14, 2017

View reviewed changes

will-moore added 2 commits July 31, 2017 23:56

Add canChgrp and canChown from, PR ome#5358 to tree.py JSON

7d947c6

Revert "Fix unit tests, adding getEventContext() to MockConnection"

57eae3b

Don't need this now since we're not using getEventContext in unit test This reverts commit cf8620e.

will-moore added 6 commits August 1, 2017 11:13

Fix unit tests with canChgrp in test_tree.py

6523212

Update get_perms() in test_tree.py with canChgrp and canChown

0e262d7

Merge remote-tracking branch 'origin/develop' into privileges_canChgrp

42f4b2e

Add canChgrp and canChown to permissions fixture in omero/testlib

268cfe0

Add canChgrp and canChown to test in test_permissions.py

ce302e4

Test that project_details_permissions map query works as expected

33342da

Merge remote-tracking branch 'origin/develop' into privileges_canChgrp

197cee4

Remove duplicate testing and rename testProjectionPermissionsWorkaround

f2e3314

will-moore closed this Aug 14, 2017

mtbc mentioned this pull request Aug 15, 2017

Unwrap PermDetails for canChgrp/canChown detection #5437

Merged

will-moore reopened this Aug 15, 2017

joshmoore reviewed Aug 16, 2017

View reviewed changes

sbesson approved these changes Aug 16, 2017

View reviewed changes

Remove commented-out line

d662633

mtbc approved these changes Aug 21, 2017

View reviewed changes

jburel merged commit f69b7dd into ome:develop Aug 21, 2017

sbesson added this to the 5.4.0 milestone Oct 6, 2017

will-moore deleted the privileges_canChgrp branch February 18, 2019 04:13

Conversation

will-moore commented Jun 29, 2017

What this PR does

Testing this PR

Uh oh!

mtbc commented Jun 29, 2017

Uh oh!

will-moore commented Jun 29, 2017

Uh oh!

mtbc commented Jun 29, 2017

Uh oh!

pwalczysko commented Jun 29, 2017

Uh oh!

will-moore commented Jun 30, 2017

Uh oh!

mtbc commented Jun 30, 2017

Uh oh!

pwalczysko commented Jul 3, 2017

Uh oh!

jburel commented Jul 4, 2017

Uh oh!

mtbc commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbesson left a comment

Choose a reason for hiding this comment

Uh oh!

sbesson Jul 14, 2017

Choose a reason for hiding this comment

Uh oh!

will-moore commented Jul 31, 2017

Uh oh!

pwalczysko commented Aug 2, 2017

Uh oh!

will-moore commented Aug 3, 2017

Uh oh!

mtbc commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtbc commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

will-moore commented Aug 11, 2017

Uh oh!

will-moore commented Aug 14, 2017

Uh oh!

joshmoore commented Aug 15, 2017

Uh oh!

will-moore commented Aug 15, 2017

Uh oh!

will-moore commented Aug 16, 2017

Uh oh!

joshmoore Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

joshmoore commented Aug 16, 2017

Uh oh!

mtbc left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mtbc commented Jul 4, 2017 •

edited

Loading

mtbc commented Aug 10, 2017 •

edited

Loading

mtbc commented Aug 11, 2017 •

edited

Loading