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

Add +t (sticky bit) to default omero_server_datadir_managedrepo_mode #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manics
Copy link
Member

@manics manics commented May 4, 2018

After a discussion with @pwalczysko over ome/omero-documentation#1873 we realised the current outreach playbook has the +t permission on the managed repo: https://github.com/openmicroscopy/prod-playbooks/blob/51ddfa88c52248e2affa4300eb73df488be99efd/training-server.yml#L218

It sounds like this may be a good idea but could do with some input from @rleigh-codelibre @mtbc

Note this requires Ansible 2.4 so travis will fail with bad symbolic permission for mode: +t, though we can discuss this change as part of https://trello.com/c/SsE1sGqR/184-in-place-import-doc-is-unclear

@sbesson
Copy link
Member

sbesson commented May 4, 2018

Re the Ansible 2.4 minimal requirement, this might be the moment to start planning what it would involve across the board. Step 1 might be to come back to ome/ome-ansible-molecule#10 and review our Ansible ecosystem.

@manics manics changed the title Add +t (sticky bit) to default +omero_server_datadir_managedrepo_mode Add +t (sticky bit) to default omero_server_datadir_managedrepo_mode May 4, 2018
@manics
Copy link
Member Author

manics commented May 4, 2018

The biggest barrier is rewriting the molecule tests for molecule 2.x, all the roles used by the IDR (which should cover most of our galaxy roles) work fine with Ansible 2.4.

@rleigh-codelibre
Copy link

It might be worth for checking if there are any complications when it comes to deleting files, particularly for e.g. in-place imports or any situation where the user performing the deletion is not the owner of the directory with the sticky bit, and/or the owner of the content to be deleted in this directory.

@mtbc
Copy link
Member

mtbc commented May 7, 2018

It's a plausible change but probably also requires opinion from @joshmoore as I'm really not sure.

@pwalczysko
Copy link
Member

any situation where the user performing the deletion is not the owner of the directory with the sticky bit, and/or the owner of the content to be deleted in this directory.

So this is a point of confusion for me. I would actually say that the deletion only matters in the sense whether or not omero can delete things. In wherever I look, both the directories and the files in them are always owned by omero. And yes, from OMERO I can delete those without problem. Maybe there is a question of whether or not the clean scripts would work as well. But how could they not, since again, it is omero who is executing them ?

I understand that we constantly worry about who can do this and that diractly manipulating the ManagedRepo dir, but I think I have proven that this is safe already.

Where am I making a mistake in my musing above ?

@joshmoore
Copy link
Member

So here are my assumptions when I look at this:

  • In the case of "only user omero-server is a member of group omero and the directory ManagedRepository belongs to user omero-user", then this makes no difference.
  • If we add "plus one inplace importer in group omero", then I think this still doesn't have much effect.
  • i.e. this only becomes relevant in the "two or more inplaces importers in group omero" and the only effect is that each inplace importer cannot delete top-level directories created by other inplace importers. That, however, is an unlikely scenario since I assume that the top-level directories will typically be created by the user omero-server

i.e. this is safe but doesn't necessarily enable much. We'd need to see how to have the next few levels of directories created in the same fashion.

I haven't thought through cases where ManagedRepository doesn't belong to omero-server, but likely we don't want to support that.

@pwalczysko
Copy link
Member

pwalczysko commented May 7, 2018

I have started on a testing matrix as encouraged by @manics . Please let me know if you find that testing sufficient.

@joshmoore Seeing the columns L and M of this testing matrix, does it not disprove your assumption about non-propagation of perms into lower level dirs ?

We'd need to see how to have the next few levels of directories created in the same fashion.

@pwalczysko
Copy link
Member

Yes, but now I have something a bit worrying. See the cell P5 in https://docs.google.com/spreadsheets/d/1m27kar8CzCFJEyvQprkvsANUQrmC9b5TYMzacYD9b98/edit#gid=0
It looks like there is something not right.
Is it possible that the flaw highlighted in cell C5 is also there when the other setup of in-place importer is facilitated, as mentioned in the doc ? (@joshmoore )

@joshmoore
Copy link
Member

@pwalczysko : I agree with your spreadsheet that there is at least one other dimension to consider: fresh repo vs. re-used repo.

@pwalczysko
Copy link
Member

Analysing the situation with @manics and @mtbc we concluded that the problem noticed in the cell P5 in https://docs.google.com/spreadsheets/d/1m27kar8CzCFJEyvQprkvsANUQrmC9b5TYMzacYD9b98/edit#gid=0 is an OMERO Bug which is not specific to this setup, but to any in-place import setup.

@manics manics closed this Jan 14, 2021
@manics manics reopened this Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants