Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce Chmod2 request #3773

Merged
merged 30 commits into from May 12, 2015
Merged

introduce Chmod2 request #3773

merged 30 commits into from May 12, 2015

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Apr 29, 2015

Uses of Chmod requests are now actioned by the Chmod2 implementation. Chmod-related integration tests should pass. Also try changing group permissions from the client to make sure it works correctly. (The admin service's changePermissions still uses the old Chmod but the clients don't much use that.)

This PR should be suitable for 5.1 so that a 5.1.1 (or trout-latest) client can work with a 5.1.2 (or trout-merge) server and vice versa: no commits with this PR switch clients to use Chmod2 directly.

--no-rebase

@mtbc mtbc force-pushed the Chmod2 branch 2 times, most recently from 62d8c2a to 3a6b21c Compare April 29, 2015 14:25
@mtbc
Copy link
Member Author

mtbc commented Apr 30, 2015

OMERO-5.1-merge-integration-java now has 48 more passing tests. 😃

@mtbc
Copy link
Member Author

mtbc commented Apr 30, 2015

Probably hold off extensive manual testing until the failing Python test is fixed.

@pwalczysko
Copy link
Member

Just did a quick check connecting the latest client to merge server. Downgraded and upgraded a test group permissions couple of times. Then went into the group as a non-admin user. The group permissions behaved as expected.

@pwalczysko
Copy link
Member

After the fixes of @mtbc to be tested with the Step 2 of https://www.openmicroscopy.org/private/ome-internal/testing_scenarios/AdminGroupUserEdit.html

@joshmoore
Copy link
Member

Done. Currently "waiting" I'll leave you to re-list as appropriate.

@mtbc
Copy link
Member Author

mtbc commented May 7, 2015

Thank you! I'll leave those gateway test changes for another time so as not to cause any test failures that may delay #3739.

@mtbc
Copy link
Member Author

mtbc commented May 7, 2015

The last two commits bring to Chmod2 the fixes from #3794.

@joshmoore
Copy link
Member

@mtbc : do I understand that now the test here is:

  • user-3 imports an image into their own dataset in read-annotate-1
  • user-3 creates a tag and uses it to tag the image
  • user-5, a group owner, uses OMERO.web to find user-3's dataset
  • user-5 chown chmod the dataset...

@mtbc
Copy link
Member Author

mtbc commented May 8, 2015

That's a really good question. Yes, I would guess, the last step is that user-5 chmods the group to be private. Of course, user-3's tag ought not be deleted.

@mtbc
Copy link
Member Author

mtbc commented May 8, 2015

Also worth testing:

  • create some third user who also tags user-3's image
  • make sure that tag is unlinked but not deleted after user-5 chmods the group to private.

I should have thought about that before.

@pwalczysko
Copy link
Member

Sorry, getting a bit confused here. Why @joshmoore mentions chown ? (change ownership, why ? the PR does not deal with this ?).
I am redeploying trout merge now so that we have user-1 through user-12 there, which will hopefully help me to get the test right.

Please relist for tomorrow with my name.

@pwalczysko
Copy link
Member

(sorry, did not get earlier to this due to LDAP)

@mtbc
Copy link
Member Author

mtbc commented May 11, 2015

No problem, the LDAP ones can be tricky to test. It is easy to mix up chown with chmod! 😃

@joshmoore
Copy link
Member

Indeed! Corrected my typo.

@pwalczysko
Copy link
Member

The test of #3773 (comment) passed as expected. Both a tag of user-3 on image as well as on the dataset itself were preserved when user-5 downgraded the group to private.

@pwalczysko
Copy link
Member

Interestingly, the case #3773 (comment) (the downgrade of r-a to p when non-owner of the images tags an image) is not allowed in web client. Discussing with @will-moore , all what web client is doing in such cases is catching the OMERO security violation -> it results in a message as shown below.

screen shot 2015-05-12 at 10 59 36

But all this together makes not sense - the client-server binding in this workflow seems to be lacking - are we missing some pieces here ? @mtbc @joshmoore

@mtbc
Copy link
Member Author

mtbc commented May 12, 2015

So the chmod worked fine from the CLI but not web?

@pwalczysko
Copy link
Member

@mtbc : No, the CLI failed as well with

Traceback (most recent call last):
  File "bin/omero", line 125, in <module>
    rv = omero.cli.argv()
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/cli.py", line 1431, in argv
    cli.invoke(args[1:])
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/cli.py", line 945, in invoke
    stop = self.onecmd(line, previous_args)
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/cli.py", line 1022, in onecmd
    self.execute(line, previous_args)
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/cli.py", line 1104, in execute
    args.func(args)
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/plugins/group.py", line 190, in perms
    c.submit(chmod)
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/clients.py", line 887, in submit
    closehandle=True)
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/clients.py", line 906, in waitOnCmd
    callback.loop(loops, ms)  # Throw LockTimeout
  File "/opt/hudson/workspace/OMERO-5.1-merge-deploy/src/dist/lib/python/omero/callbacks.py", line 251, in loop
    5000L, int(waited))
omero.LockTimeout: exception ::omero::LockTimeout
{
    serverStackTrace = None
    serverExceptionClass = None
    message = Command unfinished after 5.0 seconds
    backOff = 5000
    seconds = 5
}

Trying Insight now.

@pwalczysko
Copy link
Member

Insight succeeded where Web and CLI failed. The group is now Private.

Edit The group is now private.

@pwalczysko
Copy link
Member

Tag was preserved but unlinked as expected in the case of #3773 (comment). What is surprising is the failure of Web and CLI to allow such downgrade,

@joshmoore
Copy link
Member

NB: likely should consider the lack of --wait setting to the CLI a BUG or strong RFE. cc: @ximenesuk

@mtbc
Copy link
Member Author

mtbc commented May 12, 2015

Insight does seem to use Chmod at least in the gateway so that should have been implemented by Chmod2 via the facade.

@mtbc
Copy link
Member Author

mtbc commented May 12, 2015

At least in today's merge, web still uses the admin service's changePermissions rather than this PR's chmod work. Sorry I didn't know and warn of that in advance.

@mtbc
Copy link
Member Author

mtbc commented May 12, 2015

@pwalczysko: I think then good to merge?

@pwalczysko
Copy link
Member

Good to merge

joshmoore added a commit that referenced this pull request May 12, 2015
introduce Chmod2 request
@joshmoore joshmoore merged commit 464ecbb into ome:develop May 12, 2015
@mtbc mtbc deleted the Chmod2 branch May 12, 2015 12:43
@joshmoore joshmoore added this to the 5.1.2 milestone May 27, 2015
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.

None yet

6 participants