Project

General

Profile

Actions

Story #6320

closed

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

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

Status:
Resolved
Priority:
Normal
Assigned To:
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 2 (0 open2 closed)

Task #6325: Review 6320-api-logins-include-groups-wipResolvedBrett Smith06/16/2015

Actions
Task #6326: Review puppet repository branch 6320-create-shell-users-with-groups-wipResolvedBrett Smith06/16/2015

Actions

Related issues

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

Actions
Actions #1

Updated by Brett Smith over 7 years ago

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

Updated by Brett Smith over 7 years ago

  • Status changed from New to In Progress
Actions #3

Updated by Tom Clegg over 7 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...)

Actions #4

Updated by Brett Smith over 7 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.

Actions #5

Updated by Nico César over 7 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!

Actions #6

Updated by Nico César over 7 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 }
Actions #7

Updated by Brett Smith over 7 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.

Actions #8

Updated by Nico César over 7 years ago

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

Actions #9

Updated by Brett Smith over 7 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF