Story #3153

[API] Configuration option to automatically set up users (VM, repository, invite)

Added by Ward Vandewege over 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
API
Target version:
Start date:
08/20/2014
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
3.0

Description

Currently, after logging in for the first time (and assuming the user has not been pre-activated), a new user is in the "not invited" state: "wait for us to activate your account". An admin gets notified, and clicks the "Setup user" button on the users>show>admin tab and assigns a username / repository name.

New behavior: If apiserver is configured to auto-invite by setting auto_setup_new_users, in a before_filter that runs before send_admin_notifications,
  • Pick a username and repository name automatically (see below)
  • Do the same stuff the "setup new user" admin button does now:
    • Add a repository
    • Give the user write access to the repository
    • Give the user login access to an existing shell VM (auto_setup_new_users_with_vm_uuid config variable -- if false, skip this step)
    • Add the user to the existing "all users" group
  • Do not send the "your account is now ready" email to the user during auto-invite. (It will be obvious!)
  • Send the notification email to the administrator as before, but add a phrase to new_user.text.erb indicating whether the new user has been setup.
Pick username / repository name:
  • use ascii part of the e-mail address before the @ sign if unique.
  • If not unique, add a number. Use this regular expression to sanitize the username:
    • str.match /^[_.A-Za-z0-9][-\@_.A-Za-z0-9]*\$?$/
  • If it is not already there, this should go into the repository model validation.
  • If email address is nil or empty or has nothing before the @ sign, skip auto-setup. (Admin will get an email notification saying the new user is not setup -- this is handled by existing code.)
Sanity-checking login/repo names (arvados, git, gitolite, gitolite-admin, root, syslog)
  • System login names are automatically excluded by the script that creates the shell users (it strips any logins that belong to users with ids < 1000).
  • Rails.configuration.auto_setup_name_blacklist
    • Default: arvados, git, gitolite, gitolite-admin, root, syslog
  • Check blacklist from the User auto-setup method while choosing a repository/login name.
Defaults for application.default.yml:
  • auto_setup_new_users: false
  • auto_setup_new_users_with_vm_uuid: false
  • auto_setup_new_users_with_repository: false
    • Note in application.default.yml that auto_setup_new_users_with_* don't work until you turn on auto_setup_new_users.
Notes:
  • Don't automatically re-setup users who have been unsetup by administrator (implementing as an after_create filter should take care of this)
  • Don't re-use existing repositories or login usernames.
    • Look up permission links (name=can_login, head_uuid=vm_uuid) and check properties['username']. If any match, this is not an acceptable name and we need to try again to make a unique name.
    • But do re-use existing VM!
  • Document existing new_users_are_active flag in the default config file (this skips the user agreement step)

Subtasks

Task #3643: Review branch: 3153-auto-setup-userResolvedTom Clegg

Associated revisions

Revision d36525cf
Added by Radhika Chippada about 5 years ago

closes #3153
Merge branch '3153-auto-setup-user'

Revision 1ee96f5e
Added by Radhika Chippada about 5 years ago

closes #3153
Merge branch '3153-auto-setup-user'

History

#1 Updated by Radhika Chippada over 5 years ago

  • Subject changed from auto-activate beta users after they fill in a non-optional form with contact (and other) info to [API] auto-activate beta users after they fill in a non-optional form with contact (and other) info
  • Category set to API

#2 Updated by Peter Amstutz over 5 years ago

  • Assigned To set to Peter Amstutz

#3 Updated by Tom Clegg over 5 years ago

  • Target version changed from 2014-08-06 Sprint to Arvados Future Sprints

#4 Updated by Tom Clegg about 5 years ago

  • Target version changed from Arvados Future Sprints to 2014-08-06 Sprint

#5 Updated by Tom Clegg about 5 years ago

  • Story points changed from 1.0 to 2.0

#6 Updated by Tom Clegg about 5 years ago

  • Tracker changed from Feature to Story
  • Subject changed from [API] auto-activate beta users after they fill in a non-optional form with contact (and other) info to [API] Configuration option to automatically set up users (VM, repository, invite)

#7 Updated by Tom Clegg about 5 years ago

  • Subject changed from [API] Configuration option to automatically set up users (VM, repository, invite) to [API] [Draft] Configuration option to automatically set up users (VM, repository, invite)
  • Description updated (diff)
  • Assigned To deleted (Peter Amstutz)

#8 Updated by Ward Vandewege about 5 years ago

  • Description updated (diff)

#9 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#10 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#11 Updated by Tom Clegg about 5 years ago

  • Target version changed from 2014-08-06 Sprint to 2014-08-27 Sprint

#12 Updated by Tom Clegg about 5 years ago

  • Subject changed from [API] [Draft] Configuration option to automatically set up users (VM, repository, invite) to [API] Configuration option to automatically set up users (VM, repository, invite)
  • Assigned To set to Radhika Chippada
  • Story points changed from 2.0 to 3.0

#13 Updated by Radhika Chippada about 5 years ago

  • Status changed from New to In Progress

#14 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#15 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#16 Updated by Tom Clegg about 5 years ago

  • Description updated (diff)

#17 Updated by Tom Clegg about 5 years ago

At 347d974...

services/api/app/models/user.rb
  • In auto_setup_new_user, check Rails.configuration.auto_setup_new_users right at the top and return true if not set. (If it's turned off, there's no point doing that other preparatory stuff.)
  • In auto_setup_new_user, looks like username will be nil if self.email is nil -- maybe check this right after config, and give up (return true) right away if it's nil
  • I see you faithfully copied the regexp from the description above but I propose we change it to something more restrictive.
    • Safer suggestion: /^[a-zA-Z][-._a-zA-Z0-9]{0,30}[a-zA-Z0-9]$/
    • Failing examples: "4ad@example.com", ".foo@example.com", "bar.@example.com", "..@example.com"
    • Passing examples: "ice9@example.com", "o_o@example.com", "r00t@example.com"
  • Comment above derive_unique_username suggests this method expects an email address as input. Propose: "Find a username that starts with the given string and does not collide with any existing repository name or VM login name"?
  • I think it would be better to separate the configuration logic from the "find a unique username" function: i.e., derive_unique_username always finds a unique username regardless of whether the caller was justified in asking for one. In particular, we should avoid colliding with an existing VM login name even if we're not planning to give this user access to a VM during automatic setup. (And same goes for repo names.) In cases where we aren't going to set up a repo or a VM, we won't need a username, so we shouldn't even bother calling derive_unique_username at all.
  • Rather than append 6 digits per loop iteration, I suggest appending 1 random digit per loop. (This way, the resulting usernames will usually be short enough to remember.)
  • login_props is unused
  • Could simplify the loop logic by using "break" or "return" when a unique username is found:
    • while true
        if Repository.where(name: username).empty?
          login_collisions = Link.where(...).select do |perm|
            perm.properties['username'] == username
          end
          if login_collisions.empty?
            return username
          end
        end
        username = username + SecureRandom.random_number(10).to_s
      end
      
  • Only say " and setup" in the notification email if @user.is_invited
  • If not @user.is_invited and Rails.configuration.auto_setup_new_users then it would be nice to add a sentence like "However, automatic setup did not work out."
  • If auto_setup_new_users is true but VM and repo parts are both disabled, don't bother inventing a username or checking the blacklist. Just pass nil to setup_repo_vm_links() -- it is not expected to look at username if it's not being asked to do VM or repo.
Configuration
  • auto_setup_name_blacklist should be an array (string.split is fragile: if someone makes the typo "root,syslog" then both root and syslog will be allowed through). Probably makes sense to use yaml array format like this in application.default.yml:
    • auto_setup_name_blacklist: [foo, bar, baz]
Tests
  • Either something confusing (to me) is happening here, or the second username assignment here is superfluous:
    • user.email = email
      ...
      username = email.partition('@')[0] if email
      ...
      username = user.email.partition('@')[0]
      
  • Use user.save! instead of user.save in create_user_and_verify_setup_and_notifications
  • When auto_setup_new_users==true, auto_setup_new_users_with_vm_uid==false, auto_setup_new_users_with_repository==false a new user with email address *!*@example.com should have is_invited==true (i.e., can read "All users" group) because that doesn't involve picking a username.
  • I don't see where we test that the "foo" activations (with repo or VM) result in foo5 etc. rather than conflicting with the existing foo repository. (Same goes for conflicting VM logins -- we'll need a different login username in the test fixture to trigger the "append digits" code if we start stripping "_" from the username in our sanitizer as suggested above.)

Thanks!

#18 Updated by Radhika Chippada about 5 years ago

Tom, all suggestions are applied. Please take another look.

I have one concern about deriving unique username. Assuming a name is used again and again and we run out of all possible "random numbers" within the range we are using (I used 100 as of now), the while(true) loop would never terminate.

I think we should address this concern:
- Use 1000 instead of 100 (up to three digits in unique name) and still easy to remember.
- Add a loop counter and if it reaches 10,000 iterations or so, break out of the loop. In that case, return nil for unique username and hence not perform setup.

#19 Updated by Radhika Chippada about 5 years ago

Went ahead and added the loop counter with 10000 iterations. Please do let me know if you have any suggestions about this. Thanks.

#20 Updated by Radhika Chippada about 5 years ago

Tom, I see your logic for generating unique username now. Reverted my while loop with count. Reassigning the review task to you. Thanks.

#21 Updated by Tom Clegg about 5 years ago

In services/api/app/models/user.rb would it be better to set username=nil after the third line of auto_setup_new_user, and delete the following block? That way we would use the existing setup_repo_vm_links method instead of replicating its functionality. (As far as I can see, it already does the right thing if you give it a nil username.)
  •     # setup user
        if !Rails.configuration.auto_setup_new_users_with_vm_uuid &&
           !Rails.configuration.auto_setup_new_users_with_repository
          oid_login_perm = create_oid_login_perm Rails.configuration.default_openid_prefix
          group_perm = create_user_group_link
        else
    
Add space here in services/api/app/mailers/admin_notifier.rb, I think:
  • -        add_to_subject = @user.is_invited ? 'and setup' : ', but not setup'
    +        add_to_subject = @user.is_invited ? ' and setup' : ', but not setup'
    

In the tests, the name valid_username seems to be a misnomer: it really indicates something like ok_to_auto_setup.

I admit (or complain?) that I find this big test case bewildering. The first element of each array is named "active", which we use to set the is_active flag of the new user, and then use again to decide which kind of notification email to test for. The notify@ email addresses seem to be a channel for disabling the tests for different types of email that might get sent, but we also use them to change the application configuration before running the test case, so it's not clear whether the observed email behavior is a result of correct test case handling, or the configuration tweaking.

I noticed, too, that we don't have a functional or integration test. I added a 3153-auto-setup-tests branch with a set of integration tests, 195c340 (it also adds a "restore Rails.configuration after each test case" piece, still a hack but possibly nicer than special-casing it each time?).

Auto-setup worked as expected, which is great. (I did change two existing tests to make them pass, though.)

Minor things

In user.rb -- spurious semicolon at end of line "return true;"

In services/api/test/unit/user_test.rb -- blaklisted→blacklisted

#22 Updated by Radhika Chippada about 5 years ago

Tom:

1. "use the existing setup_repo_vm_links method instead of replicating its functionality" : Yes, you are right. I reverted back to just using the setup method directly. I had a different though process earlier, which was if a username is blacklisted, go ahead and create the all_groups and oid links but skip the repo and vm links. This resulted in my over-engineering!!

2. "Add space here in services/api/app/mailers/admin_notifier.rb" : this did end up being problematic when auto_setup is not set -- ended up two space characters in that email. Fixed it.

3. Changed to "ok_to_auto_setup" and addressed other comments about typo and extra semicolon character.

4. Thanks for adding the integration tests. I remembered to run all tests and look into adding integration test(s) this morning!! Thanks.

5. Regarding the "big test case bewildering" : It appears that only one email is available by the time the test verifies the email and hence Ward had that logic to test the received email. I wanted to make sure the auto_setup tests auto setup as well as delivered email for newly created user case. So, I stuck to that format. Basically, the test is only able to test the new user created mail only when the user is active due to this. I did verify by looking into logs that both are delivered if the user is inactive though!

#23 Updated by Radhika Chippada about 5 years ago

I also went back and tested Ward's theory that only one message is available during the test run. This is not true. When a new and inactive user is created, two mails are sent: one for new user creation notification, and another about a new inactive user creation.

Updated the test(s) to look for both when an inactive user is being created.

#24 Updated by Tom Clegg about 5 years ago

Looks good, please merge.

Thanks!

#25 Updated by Radhika Chippada about 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:d36525cf0f67b35e9f59a3481a3739f9ebffae29.

Also available in: Atom PDF