Arvados

At 347d974...

• 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.

• 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]

• 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!

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.

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

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

•     # 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_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

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!

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.

Looks good, please merge.

Thanks!