Story #6320

[OPS] Shell account creation script supports `groups` properties in can_login links

Added by Brett Smith over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
-
Target version:
Start date:
06/16/2015
Due date:
% Done:

100%

Estimated time:
(Total: 2.00 h)
Story points:
0.5

Description

Update the appropriate script in Puppet to respect the groups property specified after #6254.


Subtasks

Task #6325: Review 6320-api-logins-include-groups-wipResolvedBrett Smith

Task #6326: Review puppet repository branch 6320-create-shell-users-with-groups-wipResolvedBrett Smith


Related issues

Blocked by Arvados - Feature #6254: [Workbench] Support specifying group memberships for shell accountsResolved06/10/2015

Associated revisions

Revision 5386f665
Added by Brett Smith over 6 years ago

Merge branch '6320-api-logins-include-groups-wip'

Refs #6320. Closes #6325.

Revision d7e0ab96 (diff)
Added by Brett Smith over 6 years ago

6320: Update Workbench test for active user VM setup.

Now that the test fixtures give the active user access to testvm2,
this test needs to be updated to account for it, just like the API
server tests were. Refs #6320.

History

#1 Updated by Brett Smith over 6 years ago

  • Description updated (diff)
  • Assigned To set to Brett Smith

#2 Updated by Brett Smith over 6 years ago

  • Status changed from New to In Progress

#3 Updated by Tom Clegg over 6 years ago

Noticed virtual_machines_controller.rb now has

          username = perm.properties.andand['username']
...
              groups: (perm.properties["groups"].to_a rescue []),

If it's safe to assume properties is always a hash, never nil (which is assured by serialize :properties, Hash in source:services/api/app/models/link.rb and the serialize validation in source:services/api/app/model/arvados_mode.rb), then this might be a good time to get rid of the old .andand and make it more clear that the only purpose of the new "rescue" is to silently ignore cases where properties["groups"] is a Hash, String, Fixnum, etc. (Assuming that was your intent...)

#4 Updated by Brett Smith over 6 years ago

Tom Clegg wrote:

If it's safe to assume properties is always a hash, never nil (which is assured by serialize :properties, Hash in source:services/api/app/models/link.rb and the serialize validation in source:services/api/app/model/arvados_mode.rb), then this might be a good time to get rid of the old .andand and make it more clear that the only purpose of the new "rescue" is to silently ignore cases where properties["groups"] is a Hash, String, Fixnum, etc. (Assuming that was your intent...)

You understood correctly. Implemented your suggestion in a9dcceaa. Thanks.

#5 Updated by Nico César over 6 years ago

checked a9dcceaa2d3dcc3ff8984ac013022fda5a1a31e9

"run-tests.sh --only services/api" tested it successfully:

Arvados::V1::VirtualMachinesControllerTest#test_groups_is_an_empty_list_by_default = 0.04 s = .
Arvados::V1::VirtualMachinesControllerTest#test_groups_propagated_from_permission = 0.04 s = .
Arvados::V1::VirtualMachinesControllerTest#test_logins_without_usernames_not_listed = 0.05 s = .
Arvados::V1::VirtualMachinesControllerTest#test_username_propagated_from_permission = 0.04 s = .

I also went thru the code and "LGTM". merge it!

#6 Updated by Nico César over 6 years ago

I'm reviewing 0a367d7f69abc3897a1ab557e8c14b71f2ec456e from puppet repo

I see that

modules/arvados-shell-server/templates/usr-local-arvados-update-shell-accounts.rb.erb

has hardcoded 1000 as first user in line 60. that's a variable. should be taken from:

grep ^UID_MIN /etc/login.defs

and This is having the desired effect:

  1. No system users
    - logins = logins.reject { |l|
    - id = `id u #{l[:username]} 2>/dev/null`.to_i
    $?.to_i == 0 and id < 1000
    - }
    + logins.reject! { |name| (uids[name] || 65535) < 1000 }

#7 Updated by Brett Smith over 6 years ago

Nico Cesar wrote:

I'm reviewing 0a367d7f69abc3897a1ab557e8c14b71f2ec456e from puppet repo

I've updated the branch to address both your comments. Thanks especially for catching that bug around rejecting system logins. Now at 5140a12. Thanks.

#8 Updated by Nico César over 6 years ago

revision 5140a12c784d1c2372d6c9d5b533794e3515e38a LGTM tested on shell.4xphq and DIDN'T wipped out my .ssh/authorized key. I'm happy now.

#9 Updated by Brett Smith over 6 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF