Skip to content

more carefully prevent update/delete for user group#5284

Merged
mtbc merged 4 commits intoome:rolesfrom
mtbc:check-for-user-group
May 12, 2017
Merged

more carefully prevent update/delete for user group#5284
mtbc merged 4 commits intoome:rolesfrom
mtbc:check-for-user-group

Conversation

@mtbc
Copy link
Copy Markdown
Member

@mtbc mtbc commented May 9, 2017

What this PR does

Makes the ACL checks for update/delete wrt the user group clearer and safer.

Testing this PR

https://10.0.51.154:8443/job/OMERO-test-integration/testngreports/ should have no skips or failures: @pwalczysko, this PR should fix all cases of testOfficialScriptDeleteNoSudo if you want to try again removing the skip. (Can be tested locally if need be.)

The test in the description of https://github.com/openmicroscopy/omero_private/issues/13 should fail. (Source may need a bit of tweaking for current branch, but not much: PYTHON_MIMETYPE == "text/x-python" and roles is now already a field in the superclass.)

@mtbc mtbc closed this May 9, 2017
@mtbc mtbc reopened this May 9, 2017
@mtbc mtbc mentioned this pull request May 9, 2017
@jburel jburel added the roles label May 10, 2017
@joshmoore
Copy link
Copy Markdown
Member

In general, this code block has long caused issues. I'm slightly nervous about having something as specific as "Directory" encoded here but don't have any immediate suggestions on how to do it differently.

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented May 11, 2017

It might be that I should create a DevX card for simpllifying this block, seeing which tests break, and justifying to ourselves that those tests aren't at fault. As things are, as with the graphs work, a facet of my initial approach to implementing light admin restrictions is to try to avoid unnecessary changes in behavior, so I am trying not to tweak existing tests.

Speculating wildly: it is also possible that there is actually some canAnnotate of related objects or something that this directory-checking is a poor emulation of, cf. RepositoryDaoImpl.canWriteParentDirectory, i.e. that the check in the ACL voter should ultimately be approaching the asked question quite differently.

@pwalczysko
Copy link
Copy Markdown
Member

pwalczysko commented May 11, 2017

tested locally with this PR in (scc merge roles).

Testing the test in header of https://github.com/openmicroscopy/omero_private/issues/13 ended with failure as expected.

Adjusting the test according to https://github.com/openmicroscopy/omero_private/pull/15#issuecomment-283307652 fixed the failing test into a passing test.

The removal of the Skp lines in https://github.com/pwalczysko/openmicroscopy/blob/new-role-cleaned/components/tools/OmeroJava/test/integration/LightAdminRolesTest.java#L1577 . The skipped tests passed as well as expected.

Ready to merge RMPOV

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented May 12, 2017

Created https://trello.com/c/PAg0H0wB/756-review-group-permissions-in-server to note ongoing uncertainty.

@mtbc mtbc merged commit 025e76b into ome:roles May 12, 2017
@mtbc mtbc deleted the check-for-user-group branch May 12, 2017 12:46
@sbesson sbesson added this to the 5.4.0 milestone Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants