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

fix #12160: root-owned template paths #2448

Merged
merged 28 commits into from May 16, 2014

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented May 5, 2014

Fixes http://trac.openmicroscopy.org.uk/ome/ticket/12160 to allow managed repository template paths to start with some root-owned directories.

In etc/omero.properties the omero.fs.repo.path now has a // for one of the path separators. Imports should still work fine, the images remaining usable, with those earlier directories before the // owned by root instead of by the user.

One can use bin/omero hql to check directory ownership with queries like,

SELECT path || name, details.owner.omeName, details.group.name FROM OriginalFile WHERE mimetype = 'Directory'

--rebased-to #2514

@jburel jburel added the dev_5_0 label May 5, 2014
@joshmoore
Copy link
Member

Currently testing with https://gist.github.com/joshmoore/ea7d99dd7cb5548cab12

I haven't pinned down just which scenarios are the most painful, but the ApiUsageExceptions and ValidationExceptions which are thrown if changing, for example, from //%userId% to %userId%// are leading to several failures.

@mtbc
Copy link
Member Author

mtbc commented May 8, 2014

Certainly some of those paths should fail through not having both root- and user-owned parts or through not being sufficiently unique per fileset.

@joshmoore
Copy link
Member

Yup, eventually I'd like to have repo_test.py be explicit about "this is supported; this isn't". At the moment, the difficulty with moving the // is obscuring that testing, so need to figure out the rules there first. Btw, the clients are seeing, "ApiUsageException: null" as the output of these messages. I haven't looked into why yet.

@mtbc
Copy link
Member Author

mtbc commented May 8, 2014

Basically,

  1. the creation of the last directory of the template path must cause a new directory to be created (the uniqueness may arise from higher in the path)
  2. there must be at least one directory on each side of the //
  3. there must be at least one //; only the last one "counts".

@mtbc
Copy link
Member Author

mtbc commented May 8, 2014

I wonder if I was meant to catch and wrap/map the exception somewhere.

@mtbc
Copy link
Member Author

mtbc commented May 8, 2014

Should I look into why the exception message is disappearing -- you have no idea? Is there anything else I should be doing on this PR?

@joshmoore
Copy link
Member

If you could look into filling the error messages, that would be great. Possibly something in the error printing code of ImportLibrary, though I'm not sure.

I'm still testing behavior. Your 3-point list above is very helpful. I am wondering though if we will need to allow root to take over a user directory if the // is pushed from right to left.

@mtbc
Copy link
Member Author

mtbc commented May 9, 2014

Thank you!

If we do need that, I'll need to chat to you about how one might change the ownership without resorting to SQL.

@mtbc
Copy link
Member Author

mtbc commented May 9, 2014

I haven't yet managed to figure out what's going wrong to eat the exception message. I wanted to check if the https://github.com/openmicroscopy/openmicroscopy/blob/dev_5_0/components/blitz/src/omero/util/IceMapper.java#L870 FIXME is irrelevant?

for ServerError the .toString() and .getMessage() do not
include the message!
@joshmoore
Copy link
Member

With the template gid%groupId%/uid%userId%//%time%, I'm getting

2014-05-12 15:29:05,951 2912       [      main] ERROR        ome.formats.importer.ImportLibrary - Error on import: cannot create directory "gid5253" with template path's root-owned "gid1/uid5352"

as a new user in a new (rw----) group, which looks like the context changes for placing something in the user context are affecting the template expansion.

See https://gist.github.com/joshmoore/ea7d99dd7cb5548cab12/b03c09c5038d5fcde366d35e31cdb811731a82cb#file-repo_test-py-L92 for the test I'm running with py.test ea7d99dd7cb5548cab12/repo_test.py -k giduid -v -s

final Current sudoCurrent = makeAdjustedCurrent(current);
sudoCurrent.ctx = new HashMap<String, String>(current.ctx);
sudoCurrent.ctx.put(SUDO_REAL_SESSIONUUID, current.ctx.get(omero.constants.SESSIONUUID.value));
sudoCurrent.ctx.put(SUDO_REAL_GROUP, current.ctx.get(omero.constants.GROUP.value));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The group doesn't actually seem to be in the context here, just the session UUID and client UUID.

@mtbc
Copy link
Member Author

mtbc commented May 13, 2014

Note: this probably needs a corresponding docs PR.

@joshmoore
Copy link
Member

In that case, /cc @hflynn to force us to do it!

@mtbc
Copy link
Member Author

mtbc commented May 13, 2014

I'll sort one out once we actually decide to merge this! As it is I am still trying to work out how to get a group name for SUDO_REAL_GROUP when current.ctx.get(omero.constants.GROUP.value) == null.

@mtbc
Copy link
Member Author

mtbc commented May 13, 2014

(I had tried plugging the admin service into PublicRepositoryI but in that situation there doesn't seem to be an event context for getEventContextQuiet() to return. Still, I do have a session UUID, maybe there's some way to get the appropriate group name based on that.)

@hflynn
Copy link
Member

hflynn commented May 13, 2014

Docs PR open by Friday if at all possible please @mtbc

public Object doWork(Session session, ServiceFactory sf) {
return ((LocalAdmin) sf.getAdminService()).getEventContextQuiet();
}
});
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in trying to import as user-2 into private-1, realSessionUuid is set but realGroup == null. Also, effectiveEventContext does have user-2 but group user.

@mtbc
Copy link
Member Author

mtbc commented May 14, 2014

Documentation PR opened: ome/omero-documentation#778

@joshmoore
Copy link
Member

Just writing more tests now, but the only conditions I'm seeing with the current state of ea7d99dd7cb5548cab12 is no root-owned directories in managed repository template path - If we can allow root to take over a user's dir, is this still needed?

@joshmoore
Copy link
Member

Previous commit made previous tests all pass:

ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[//-False] PASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[//%time%-True] PASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[//uid%userId%/%time%-True] PASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[uid%userId%//%time%-True] PASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[drive-1//uid%userId%/%time%-TruPASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[drive-1/uid%userId%//%time%-True] PASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[drive-1//uid%userId%/%time%-True] PASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[gid%groupId%/uid%userId%//%time%-True] PASSED
ea7d99dd7cb5548cab12/repo_test.py:68: test_repo[gid%groupId%//uid%userId%/%time%-True] PASSED
ea7d99dd7cb5548cab12/repo_test.py:95: test_simple_giduid PASSED
ea7d99dd7cb5548cab12/repo_test.py:99: test_swap_user_root PASSED
ea7d99dd7cb5548cab12/repo_test.py:107: test_change_to_root_with_mount PASSED
ea7d99dd7cb5548cab12/repo_test.py:114: test_change_to_user_with_mount PASSED

=== 13 passed in 1299.85 seconds ===

@joshmoore
Copy link
Member

Haven't found any other issues; merging. More testing will be good over the course of the RCs. Thanks, @mtbc!

joshmoore added a commit that referenced this pull request May 16, 2014
fix #12160: root-owned template paths
@joshmoore joshmoore merged commit 44374c0 into ome:dev_5_0 May 16, 2014
@mtbc mtbc deleted the trac-12160-template-paths branch May 19, 2014 08:41
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

4 participants