[API] Configuration option to automatically set up users (VM, repository, invite)
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
- 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_uuidconfig 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.erbindicating whether the new user has been setup.
- 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:
- 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.)
- 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).
- Default: arvados, git, gitolite, gitolite-admin, root, syslog
- Check blacklist from the User auto-setup method while choosing a repository/login name.
- 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.
- Don't automatically re-setup users who have been unsetup by administrator (implementing as an
after_createfilter 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!
- Look up permission links (name=can_login, head_uuid=vm_uuid) and check
- Document existing
new_users_are_activeflag in the default config file (this skips the user agreement step)
#12 Updated by Tom Clegg over 6 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
#17 Updated by Tom Clegg over 6 years ago
Rails.configuration.auto_setup_new_usersright at the top and return true if not set. (If it's turned off, there's no point doing that other preparatory stuff.)
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:
- Failing examples:
"email@example.com", ".firstname.lastname@example.org", "bar.@example.com", "..@example.com"
- Passing examples:
"email@example.com", "firstname.lastname@example.org", "email@example.com"
- Safer suggestion:
- Comment above
derive_unique_usernamesuggests 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
- 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.)
- 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
not @user.is_invited and Rails.configuration.auto_setup_new_usersthen it would be nice to add a sentence like "However, automatic setup did not work out."
truebut 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.
- 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
auto_setup_name_blacklist: [foo, bar, baz]
- Either something confusing (to me) is happening here, or the second
usernameassignment here is superfluous:
user.email = email ... username = email.partition('@') if email ... username = user.email.partition('@')
auto_setup_new_users==true, auto_setup_new_users_with_vm_uid==false, auto_setup_new_users_with_repository==falsea new user with email address
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.)
#18 Updated by Radhika Chippada over 6 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.
#21 Updated by Tom Clegg over 6 years ago
username=nilafter the third line of
auto_setup_new_user, and delete the following block? That way we would use the existing
setup_repo_vm_linksmethod 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
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
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.)
user.rb -- spurious semicolon at end of line "return true;"
services/api/test/unit/user_test.rb -- blaklisted→blacklisted
#22 Updated by Radhika Chippada over 6 years ago
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 over 6 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.