Feature #6254

[Workbench] Support specifying group memberships for shell accounts

Added by Ward Vandewege over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
06/10/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Description

Original requirements

We need a way to list the groups a user should be part of on the node they can log in to.

Typical use cases:

a) docker group
b) admin group

Solution to implement

The simplest thing that could possibly work:

  • can_login links grow a groups property, an array of group name strings.
  • Workbench's user setup form has a text entry field for the administrator to enter a comma-separated list of group names. After setting up the user, Workbench updates the can_login link to include the list of groups as a property.
  • The script that actually creates shell accounts from can_login links creates group memberships from the new groups property. (This is a separate story. This story is just about Workbench development.)

We know that this method is susceptible to typos—it will silently fail to set the desired group memberships if the admin makes a mistake entering the group memberships in the setup form. We consider that an acceptable risk/limitation for now.


Subtasks

Task #6297: Review branch: 6254-groups-during-setupResolvedRadhika Chippada


Related issues

Blocks Arvados - Story #6320: [OPS] Shell account creation script supports `groups` properties in can_login linksResolved06/16/2015

Associated revisions

Revision 832486e3
Added by Radhika Chippada over 6 years ago

closes #6254
Merge branch '6254-groups-during-setup'

History

#1 Updated by Ward Vandewege over 6 years ago

  • Description updated (diff)

#2 Updated by Ward Vandewege over 6 years ago

  • Subject changed from [API + Workbench] Make it possible to mark a user on a shell node as a root-equivalent user to [API + Workbench] Make it specify groups for shell users

#3 Updated by Brett Smith over 6 years ago

  • Subject changed from [API + Workbench] Make it specify groups for shell users to [API + Workbench] Support specifying group memberships for shell accounts
  • Target version set to 2015-07-08 sprint

#4 Updated by Brett Smith over 6 years ago

  • Description updated (diff)

#5 Updated by Brett Smith over 6 years ago

  • Story points set to 1.0

#6 Updated by Brett Smith over 6 years ago

  • Subject changed from [API + Workbench] Support specifying group memberships for shell accounts to [ Workbench] Support specifying group memberships for shell accounts
  • Description updated (diff)
  • Category set to Workbench
  • Assigned To set to Radhika Chippada

#7 Updated by Radhika Chippada over 6 years ago

  • Status changed from New to In Progress

#8 Updated by Bryan Cosca over 6 years ago

  • Subject changed from [ Workbench] Support specifying group memberships for shell accounts to [Workbench] Support specifying group memberships for shell accounts

#9 Updated by Radhika Chippada over 6 years ago

  • Updated the setup popup to also include "groups"
    • Store these, after a successful setup step, in the can_login link as groups property
    • Populate the groups text field in the popup with any stored groups
  • Did not do any removal of groups during unsetup process
    • If an admin want to delete groups for a user, just need to execute setup method with the groups text field cleared.

#10 Updated by Brett Smith over 6 years ago

Reviewing 6254-groups-during-setup at cdec4d7

I'm sorry this wasn't spelled out better in the story description, but unfortunately, you're manipulating the wrong kind of can_login permission link. There are two kinds. The first kind has a head_uuid of the user UUID and a tail_uuid of an e-mail address. This is an "OID login" permission. This is related to API server logins somehow, and might be obsolete—I don't really know what it does. (Tom would probably know what it is and whether we still use it, if you're curious.)

The second kind is a virtual machine (VM, a.k.a. shell) login permission. This has a head_uuid of the Arvados VirtualMachine's UUID and a tail_uuid of the user UUID. This is the permission that we need to extend with group information, since the groups are used on virtual machines and not for OID logins.

When you make the change to update those links, you won't need the user_email hidden input anymore, so removing that input from the form would be a nice cleanup.

The user setup method returns all the links that it finds or creates in a list. Instead of re-finding the can_login permission link on the API server, can we save the result of calling the user setup method, find the can_login permission link in the response, and send an update with new properties for that link? That would save a round-trip with the API server and keep Workbench a little more responsive.

You should also be able to use this returned link object to compare the list of requested groups against the list of actual groups. This way, you can avoid propagating the previous list of groups through the HTML form, and doing all the associated string manipulation.

The user setup popup instructs the user to enter a comma-separated list of names in the input title. I believe many users will only see this if they hover a mouse pointer over the input—that's what my Firefox does, at least. I think it would be helpful to include this note as plain text on the form to reduce the risk that users might enter groups in a different format (in particular, they might try just using space-separated names).

Hidden inputs are being added to the user setup popup that have a maxlength attribute set. This attribute is not defined and has no meaning for hidden inputs, so it would be good to remove to avoid potential confusion later.

Then one last small style note, this doesn't need to be addressed before merge: Ruby's string#split method always returns an array of strings. You don't have to worry about the possibility that there are non-string objects, including nil, in the response. This means that the line:

params[:groups].split(',').map(&:strip).compact.select{|i| !i.to_s.empty?}

can be simplified to:

params[:groups].split(',').map(&:strip).select{|i| !i.empty?}

Thanks.

#11 Updated by Radhika Chippada over 6 years ago

  • Brett said:

This has a head_uuid of the Arvados VirtualMachine's UUID and a tail_uuid of the user UUID. This is the permission that we need to extend with group information ...

Corrected this. I should have suspected and checked, because I initially was going to work on this :)

  • Brett said:

user setup method returns all the links that it finds or creates in a list. Instead of re-finding the can_login permission link on the API server, can we ...

Unfortunately, setup response is a HashList, not resource list. Hence the links information is coming back as map, not as a link object. So, I needed to get it first to update it. However, this was still a good idea and the updated revision optimized on getting just the uuid specific link from setup response.

  • Implemented other updates as suggested.
  • Updated Groups text box label. While doing more testing, I noticed that if I do not select the VM, but enter the groups, they will not take effect; no vm link to add these links to. Rather than complicate implementation to disable the text box until a VM is selected, I updated the label to be helpful. Given that it is admin UI, I think this would be sufficient.

Thanks.

#12 Updated by Brett Smith over 6 years ago

d3fc80c is good to merge. One last follow-up:

Radhika Chippada wrote:

  • Brett said:

user setup method returns all the links that it finds or creates in a list. Instead of re-finding the can_login permission link on the API server, can we ...

Unfortunately, setup response is a HashList, not resource list. Hence the links information is coming back as map, not as a link object. So, I needed to get it first to update it. However, this was still a good idea and the updated revision optimized on getting just the uuid specific link from setup response.

I think you can say vm_login_link = Link.new(vm_link) to instantiate a proper Link object from the attributes hash. Then you can just modify its properties and save! it as if you got it normally. I'm not 100% sure, but if you want to try it, it might simplify things a little more. But it's not critical; you can merge either way. Thanks.

#13 Updated by Radhika Chippada over 6 years ago

Brett said:

I think you can say vm_login_link = Link.new(vm_link) to instantiate a proper Link object from the attributes hash ...

This does not work. I had actually already tried with pure find also because I have the link uuid. I could not save the object and get API error. Leaving it as is. Thanks.

#14 Updated by Radhika Chippada over 6 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:832486e363889d47adecf8ad898ab9d21384dca3.

Also available in: Atom PDF