Toggle group ownership on projects

[Bubballoo3]

Fixes #4780 by adding a form item to edit forms allowing you to set the group owner. This has a few parts to it:

1. We detect the possible group by taking the intersection of {groups the currentuser is a member of} and {groups that have execute access on the project root's ancestors}. We only show the form option if this intersection is nonempty
2. We add methods to set and get the group owner
3. We populate the form item with the current group owner ONLY IF members of the current owning group can (generically) access the project root's parent.

Eventually I would like to show the group owner in the show page, but this is a proof of concept that gets the hard part out of the way. I also think we may want to move the permissions settings to their own modal on the show page (which would more elegantly handle the fact that these can only be set after project creation), but since there is a good deal of frontend work and usage planning that would have to go into that, placing it in the form is a quicker way to get the same functionality.

The one piece this is currently missing is the ability to remove group ownership once it is given. The only way I can think to do this currently is to find a group inside the intersection mentioned above so that only the owner can access, but I don't know if that is guaranteed to exist. That being said, we are eventually going to add options to change facl permissions, so maybe "setting the group owner to 'none'" is actually best implemented by removing group access altogether, though interpreting that correctly (like in get_group_owner could get complicated.

The last special case here is that you can't use this to target intersections of groups. So if I (member of group1 and group2) create a project in a directory only accessible to group2, I can't change the project owner to group1 knowing that a certain member of group2 is also a member of group1 (effectively sharing it just with them). In a specialized permissions pane, it would be nice to have a toggle that would ignore the "who can access the parent" filter, but I don't know if this is possible inside a form without getting too complicated.

[Bubballoo3]

Even though this could be condensed by combining the 'None' case and the unset case, I left them separate so that we can replace true in the ternary with the ownership removal later. Otherwise we could simplify it to just

new_group = attributes.fetch(:group_owner, 'None')
(new_group == 'None') ? true : set_group_owner(new_group)

[johrstrom]

I don't see where this is being called from. Also - None value is likely dangerous if it's ever used. It should default to the users Primary group as that will always be a valid fallback.

[Bubballoo3]

I already removed the 'None' option from the select, as that was only necessary when we were detecting the 'possible groups', which could have been an empty set. Now that we are detecting the user's groups directly, there is no way to submit 'None'. However I see how this could create issues, maybe just sticking with nil as the default is a better idea. It should still cause set_group_owner to rescue, but ideally without any unintended effects

[Bubballoo3]

Although this instance variable is not explicitly used, it is responsible for setting the initial value in the form

[johrstrom]

I don't think I'm a fan of modifying the directory after it's been created. I'd think this is needed in the project creation - but editing or changing it after the fact, I'm not so sure.

I don't think changing the group/account/charge-back code after the fact is something that'll ever need to happen. This is because funds ($) are tied to this group/account/charge-back code. A new/different group/account/charge-back code is an entirely new project.

Specifying it during project creation? Sure, I think that's needed.

[Bubballoo3]

If we specify it during creation, we don't get to do the automagic detection of which groups it is 'shareable' with, since that depends on the location of the project root (unknown before initial creation). Of course this is all an alternative to specifying in the group level directory the proper default group ownership, but that moves the responsibility from the project owner to the center admin.

I don't think changing the group/account/charge-back code after the fact is something that'll ever need to happen.

Isn't that the only way we can toggle projects from private to shared and vice versa? If charge schemes prevent us from modifying group ownership then we could not allow for mistakes to be made during project creation (which seems user-error prone)

[Bubballoo3]

I am also trying to account for a universal shared directory where you want to limit your project to a specific group (but multiple groups have access to the parent). Is that something we should be accounting for or not?

The use case I had in mind for this was the following interaction when working with non-primary groups
user-A: Contacts admin to create a new group for new collaboration
admin: Creates new group AB with user-A and user-B and new group directory project-AB
user-A: Creates new project in the project-AB directory, but it accidentally uses user-A's primary group
user-B: Contacts user-A that they cannot see project
user-A: Edits project to have the correct (non-primary) group

This seems like a common enough occurrence that we should find a good solution to it, or putting this responsibility on the admin who creates the project-AB directory. That would also mean we would instruct people to contact their admin instead of the project owner when things aren't right (which I was trying to avoid). Hopefully this gives a more concrete sequence to discuss further

[johrstrom]

I am also trying to account for a universal shared directory where you want to limit your project to a specific group (but multiple groups have access to the parent). Is that something we should be accounting for or not?

I don't think there is such a thing as a "universal shared directory" outside of maybe /tmp.

user-A: Creates new project in the project-AB directory, but it accidentally uses user-A's primary group

If we're accounting for a mistake - then maybe - but project#new doesn't allow us to set the group ownership. This is what I'd want to see first, the ability to create the project with the right group ownership on the outset.

Changing the ownership after the fact I think just sets us up for a lot of edge case failures. You'd want to chown -R right? You'd want to change every child directory's group ownership. But then we'd have to account for partial success if you can change some (because you're the owner) but not others (because a collaborator could be the owner). Or we'd have to enforce a 770 permission so that you can, but everyone else could too.

This seems like a common enough occurrence that we should find a good solution to it, or putting this responsibility on the admin who creates the project-AB directory

Not sure where you're getting the admin from here. If a user creates /fs/ess/project-AB/foo the directory in question is foo which is owned by the user, not project-AB. But even so, if that same user is the PI (the person who requested the project and supplies the funds for the budget), then they likely own project-AB. I own /fs/ess/PZS0714 for example. If you look through /fs/ess you'll see most are owned by a regular user who is the principle investigator (PI) for that project.

[Bubballoo3]

Ok the group selection option has now been enabled for the new page only, and the group is only shown on projects outside the user's home directory (potentially shared projects). We can return to group owner modification later, when we have a way to account for edge cases or have a clearer idea of what that should look like.

[johrstrom]

CurrentUser already has this api.

ondemand/apps/dashboard/lib/current_user.rb

Lines 39 to 41 in c4d8536

	def group_names
	@group_names ||= groups.map(&:name)
	end

[johrstrom]

This should likely go into the save method. I don't think we should chmod during object creation as it may never actually get saved.

[Bubballoo3]

Yeah that seems like a good idea

[johrstrom]

I'm getting tripped up on project_dataroot. This was supposed to be the root directory for all the default projects - i.e., ~/ondemand/data/sys/dashboard/projects

But it appears to be used throughout this file as if it's @directory which seems erroneous to me. Not sure how this got so corrupted, but I think it's wrong to be using this all over the place as we are.

[johrstrom]

Yes looking through the git blame this is a mistake to use. We're confusing Project.dataroot as a static directory for all projects to default to and @directory the specific directory that this project lives in.

[Bubballoo3]

Yeah I had noticed that as well but was sticking to the (confusing) precedent within this class. I think the reason that we don't access @directory directly is that this is a string instead of a pathname, but I would be in favor of the idea of renaming project_dataroot to something like directory_path

[johrstrom]

Yes all of this works a little bit by coincidence.

Joining two paths that have a top level root (i.e., starts with /) - as this method does -

ondemand/apps/dashboard/app/models/project.rb

Lines 220 to 222 in c4d8536

	def project_dataroot
	Project.dataroot.join(directory.to_s)
	end

Actually just chooses the operand given in join.

Consider this simple example:

Pathname.new('/a').join('/b').to_s

The outtput is not /a/b or /b/a - but is in fact just simply /b.

[johrstrom]

Yeah I had noticed that as well but was sticking to the (confusing) precedent within this class. I think the reason that we don't access @Directory directly is that this is a string instead of a pathname, but I would be in favor of the idea of renaming project_dataroot to something like directory_path

project_dataroot should not even exist. If we want to cast directory to a Pathname, then fine we can do that in an initializer and/or a getter/setter method. We can use Project.dataroot as a default, but this should never be bleed into a member variable.

Let's have 1 single variable and use it.

[Bubballoo3]

It seems like the advantage over casting is it handles a relative directory path, so that project_dataroot is guaranteed to be absolute. (If @directory is relative we assume it is relative to Project.dataroot). So it seems like a good guardrail to have, just should be a better name IMO.

[Bubballoo3]

Obviously a relative directory should never happen, and the path selector should discourage it. To make it more confusing the variable is set straight from textbox value with

File.expand_path(@directory)

Which actually interprets relative paths relative to the working directory of the code, which seems like a very unpredictable default

[Bubballoo3]

🤦‍♂️Glad we discussed this, because I realized that all these quirks made me accidentally change the ownership of my personal project root to my non-primary account. Since before a project directory exists project_dataroot == Project.dataroot. I've added some protection against that, both by ensuring that we never set the group ownership of private? projects, and that get_group_owner never gets the owner of Project.dataroot by mistake. I mean that could be a way to ascertain the user's primary group, but certainly not the ideal way.

The flow now should be

1. set the @group_owner variable every time we initialize, ensuring that forms are populated correctly.
2. on save, run set_group_owner to make sure the actual owner matches @group_owner (which comes from a parameter). We run it directly after we make the project root directory to ensure that the subsequent files are created with the correct group (the setgid method will go here as well)
3. If the project directory hasn't been initialized, @group_owner should be nil, and set_group_owner has no effect (no risk of repeating the mistake above)