4253: Add a username attribute to users.
authorBrett Smith <brett@curoverse.com>
Tue, 17 Mar 2015 22:10:14 +0000 (18:10 -0400)
committerBrett Smith <brett@curoverse.com>
Wed, 25 Mar 2015 15:21:34 +0000 (11:21 -0400)
* Add the column, and propagate it based on available VM logins or
  e-mail address, if possible.
* Add format validation and tests.
* Set new usernames based on e-mail address, with tests.

doc/api/schema/User.html.textile.liquid
services/api/app/models/user.rb
services/api/db/migrate/20150317132720_add_username_to_users.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/test/fixtures/users.yml
services/api/test/unit/user_test.rb

index 9a1b0566d4b0861e340e3d198de81ccb60e67e38..335b6c10977fb40dee050e8fd95c59dbdc7c6522 100644 (file)
@@ -19,6 +19,7 @@ Each User has, in addition to the usual "attributes of Arvados resources":{{site
 table(table table-bordered table-condensed).
 |_. Attribute|_. Type|_. Description|_. Example|
 |email|string|||
+|username|string|The username used for the user's git repositories and virtual machine logins.  Usernames must start with a letter, and contain only alphanumerics.  When a new user is created, a default username is set from their e-mail address.  Only administrators may change the username.||
 |first_name|string|||
 |last_name|string|||
 |identity_url|string|||
index a47a4583cd533e272d9b5f850b4281aef75f26c3..3978df741a94eb73f13d9a22fc4463c111fdf6df 100644 (file)
@@ -8,19 +8,29 @@ class User < ArvadosModel
 
   serialize :prefs, Hash
   has_many :api_client_authorizations
+  validates(:username,
+            format: {
+              with: /^[A-Za-z][A-Za-z0-9]*$/,
+              message: "must begin with a letter and contain only alphanumerics",
+            },
+            uniqueness: true,
+            allow_nil: true)
   before_update :prevent_privilege_escalation
   before_update :prevent_inactive_admin
   before_create :check_auto_admin
+  before_create :set_initial_username, :if => Proc.new { |user|
+    user.username.nil? and user.email
+  }
   after_create :add_system_group_permission_link
   after_create :auto_setup_new_user
   after_create :send_admin_notifications
   after_update :send_profile_created_notification
 
-
   has_many :authorized_keys, :foreign_key => :authorized_user_uuid, :primary_key => :uuid
 
   api_accessible :user, extend: :common do |t|
     t.add :email
+    t.add :username
     t.add :full_name
     t.add :first_name
     t.add :last_name
@@ -222,9 +232,13 @@ class User < ArvadosModel
   end
 
   def permission_to_update
-    # users must be able to update themselves (even if they are
-    # inactive) in order to create sessions
-    self == current_user or super
+    if username_changed?
+      current_user.andand.is_admin
+    else
+      # users must be able to update themselves (even if they are
+      # inactive) in order to create sessions
+      self == current_user or super
+    end
   end
 
   def permission_to_create
@@ -237,13 +251,62 @@ class User < ArvadosModel
     return if self.uuid.end_with?('anonymouspublic')
     if (User.where("email = ?",self.email).where(:is_admin => true).count == 0 and
         Rails.configuration.auto_admin_user and self.email == Rails.configuration.auto_admin_user) or
-       (User.where("uuid not like '%-000000000000000'").where(:is_admin => true).count == 0 and 
+       (User.where("uuid not like '%-000000000000000'").where(:is_admin => true).count == 0 and
         Rails.configuration.auto_admin_first_user)
       self.is_admin = true
       self.is_active = true
     end
   end
 
+  def find_usable_username_from(basename)
+    # If "basename" is a usable username, return that.
+    # Otherwise, find a unique username "basenameN", where N is the
+    # smallest integer greater than 1, and return that.
+    # Return nil if a unique username can't be found after reasonable
+    # searching.
+    quoted_name = self.class.connection.quote_string(basename)
+    next_username = basename
+    next_suffix = 1
+    while Rails.configuration.auto_setup_name_blacklist.include?(next_username)
+      next_suffix += 1
+      next_username = "%s%i" % [basename, next_suffix]
+    end
+    0.upto(6).each do |suffix_len|
+      pattern = "%s%s" % [quoted_name, "_" * suffix_len]
+      self.class.
+          where("username like '#{pattern}'").
+          select(:username).
+          order(username: :asc).
+          find_each do |other_user|
+        if other_user.username > next_username
+          break
+        elsif other_user.username == next_username
+          next_suffix += 1
+          next_username = "%s%i" % [basename, next_suffix]
+        end
+      end
+      return next_username if (next_username.size <= pattern.size)
+    end
+    nil
+  end
+
+  def set_initial_username
+    email_parts = email.partition("@")
+    local_parts = email_parts.first.partition("+")
+    if email_parts.any?(&:empty?)
+      return
+    elsif not local_parts.first.empty?
+      base_username = local_parts.first
+    else
+      base_username = email_parts.first
+    end
+    base_username.sub!(/^[^A-Za-z]+/, "")
+    base_username.gsub!(/[^A-Za-z0-9]/, "")
+    unless base_username.empty?
+      self.username = find_usable_username_from(base_username)
+    end
+  end
+
   def prevent_privilege_escalation
     if current_user.andand.is_admin
       return true
diff --git a/services/api/db/migrate/20150317132720_add_username_to_users.rb b/services/api/db/migrate/20150317132720_add_username_to_users.rb
new file mode 100644 (file)
index 0000000..de2fc96
--- /dev/null
@@ -0,0 +1,127 @@
+require 'has_uuid'
+require 'kind_and_etag'
+
+class AddUsernameToUsers < ActiveRecord::Migration
+  include CurrentApiClient
+
+  SEARCH_INDEX_COLUMNS =
+    ["uuid", "owner_uuid", "modified_by_client_uuid",
+     "modified_by_user_uuid", "email", "first_name", "last_name",
+     "identity_url", "default_owner_uuid"]
+
+  class ArvadosModel < ActiveRecord::Base
+    self.abstract_class = true
+    extend HasUuid::ClassMethods
+    include CurrentApiClient
+    include KindAndEtag
+    before_create do |record|
+      record.uuid ||= record.class.generate_uuid
+      record.owner_uuid ||= system_user_uuid
+    end
+    serialize :properties, Hash
+
+    def self.to_s
+      # Clean up the name of the stub model class so we generate correct UUIDs.
+      super.rpartition("::").last
+    end
+  end
+
+  class Log < ArvadosModel
+    def self.log_for(thing, age="old")
+      { "#{age}_etag" => thing.etag,
+        "#{age}_attributes" => thing.attributes,
+      }
+    end
+
+    def self.log_create(thing)
+      new_log("create", thing, log_for(thing, "new"))
+    end
+
+    def self.log_update(thing, start_state)
+      new_log("update", thing, start_state.merge(log_for(thing, "new")))
+    end
+
+    def self.log_destroy(thing)
+      new_log("destroy", thing, log_for(thing, "old"))
+    end
+
+    private
+
+    def self.new_log(event_type, thing, properties)
+      create!(event_type: event_type,
+              event_at: Time.now,
+              object_uuid: thing.uuid,
+              object_owner_uuid: thing.owner_uuid,
+              properties: properties)
+    end
+  end
+
+  class Link < ArvadosModel
+  end
+
+  class User < ArvadosModel
+  end
+
+  def sanitize_username(username)
+    username.
+      sub(/^[^A-Za-z]+/, "").
+      gsub(/[^A-Za-z0-9]/, "")
+  end
+
+  def usernames_wishlist(user)
+    usernames = Hash.new(0)
+    usernames[user.email.split("@", 2).first] += 1
+    Link.
+       where(tail_uuid: user.uuid, link_class: "permission", name: "can_login").
+       find_each do |login_perm|
+      username = login_perm.properties["username"]
+      usernames[username] += 2 if (username and not username.empty?)
+    end
+    usernames.keys.
+      sort_by { |n| -usernames[n] }.
+      map { |n| sanitize_username(n) }.
+      reject(&:empty?)
+  end
+
+  def increment_username(username)
+    @username_suffixes[username] += 1
+    "%s%i" % [username, @username_suffixes[username]]
+  end
+
+  def each_wanted_username(user)
+    usernames = usernames_wishlist(user)
+    usernames.each { |n| yield n }
+    base_username = usernames.first || "arvadosuser"
+    loop { yield increment_username(base_username) }
+  end
+
+  def recreate_search_index(columns)
+    remove_index :users, name: "users_search_index"
+    add_index :users, columns, name: "users_search_index"
+  end
+
+  def up
+    @username_suffixes = Hash.new(1)
+    add_column :users, :username, :string, null: true
+    add_index :users, :username, unique: true
+    recreate_search_index(SEARCH_INDEX_COLUMNS + ["username"])
+
+    [Link, Log, User].each { |m| m.reset_column_information }
+    User.validates(:username, uniqueness: true, allow_nil: true)
+    User.where(is_active: true).order(created_at: :asc).find_each do |user|
+      start_log = Log.log_for(user)
+      each_wanted_username(user) do |username|
+        user.username = username
+        break if user.valid?
+      end
+      user.save!
+      Log.log_update(user, start_log)
+    end
+  end
+
+  def down
+    remove_index :users, :username
+    recreate_search_index(SEARCH_INDEX_COLUMNS)
+    remove_column :users, :username
+  end
+end
index 0711f9025190bbf2a125d52e343f39b07a984d9d..40861f455716a9d1b6cfb917692f9d11b5d2d0f6 100644 (file)
@@ -889,7 +889,8 @@ CREATE TABLE users (
     prefs text,
     updated_at timestamp without time zone NOT NULL,
     default_owner_uuid character varying(255),
-    is_active boolean DEFAULT false
+    is_active boolean DEFAULT false,
+    username character varying(255)
 );
 
 
@@ -1964,6 +1965,13 @@ CREATE INDEX index_users_on_modified_at ON users USING btree (modified_at);
 CREATE INDEX index_users_on_owner_uuid ON users USING btree (owner_uuid);
 
 
+--
+-- Name: index_users_on_username; Type: INDEX; Schema: public; Owner: -; Tablespace: 
+--
+
+CREATE UNIQUE INDEX index_users_on_username ON users USING btree (username);
+
+
 --
 -- Name: index_users_on_uuid; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
@@ -2122,7 +2130,7 @@ CREATE UNIQUE INDEX unique_schema_migrations ON schema_migrations USING btree (v
 -- Name: users_search_index; Type: INDEX; Schema: public; Owner: -; Tablespace: 
 --
 
-CREATE INDEX users_search_index ON users USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, email, first_name, last_name, identity_url, default_owner_uuid);
+CREATE INDEX users_search_index ON users USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, email, first_name, last_name, identity_url, default_owner_uuid, username);
 
 
 --
@@ -2364,4 +2372,6 @@ INSERT INTO schema_migrations (version) VALUES ('20150216193428');
 
 INSERT INTO schema_migrations (version) VALUES ('20150303210106');
 
-INSERT INTO schema_migrations (version) VALUES ('20150312151136');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20150312151136');
+
+INSERT INTO schema_migrations (version) VALUES ('20150317132720');
\ No newline at end of file
index c04aa47d2f34f779b8cdbaf3d0a196a499b00e27..46fbd0f2896c227d97bc845d9398633f4ed42935 100644 (file)
@@ -25,6 +25,7 @@ admin:
   identity_url: https://admin.openid.local
   is_active: true
   is_admin: true
+  username: admin
   prefs:
     profile:
       organization: example.com
@@ -39,6 +40,7 @@ miniadmin:
   identity_url: https://miniadmin.openid.local
   is_active: true
   is_admin: false
+  username: miniadmin
   prefs:
     profile:
       organization: example.com
@@ -53,6 +55,7 @@ rominiadmin:
   identity_url: https://rominiadmin.openid.local
   is_active: true
   is_admin: false
+  username: rominiadmin
   prefs:
     profile:
       organization: example.com
@@ -67,6 +70,7 @@ active:
   identity_url: https://active-user.openid.local
   is_active: true
   is_admin: false
+  username: active
   prefs:
     profile:
       organization: example.com
@@ -81,6 +85,7 @@ project_viewer:
   identity_url: https://project-viewer.openid.local
   is_active: true
   is_admin: false
+  username: projectviewer
   prefs:
     profile:
       organization: example.com
@@ -96,6 +101,7 @@ future_project_user:
   identity_url: https://future-project-user.openid.local
   is_active: true
   is_admin: false
+  username: futureprojectviewer
   prefs:
     profile:
       organization: example.com
@@ -110,6 +116,7 @@ subproject_admin:
   identity_url: https://subproject-admin.openid.local
   is_active: true
   is_admin: false
+  username: subprojectadmin
   prefs:
     profile:
       organization: example.com
@@ -124,6 +131,7 @@ spectator:
   identity_url: https://spectator.openid.local
   is_active: true
   is_admin: false
+  username: spectator
   prefs:
     profile:
       organization: example.com
@@ -184,6 +192,7 @@ job_reader:
   identity_url: https://spectator.openid.local
   is_active: true
   is_admin: false
+  username: jobber
   prefs:
     profile:
       organization: example.com
@@ -223,6 +232,7 @@ user_foo_in_sharing_group:
   identity_url: https://user_foo_in_sharing_group.openid.local
   is_active: true
   is_admin: false
+  username: fooinsharing
 
 user_bar_in_sharing_group:
   owner_uuid: zzzzz-tpzed-000000000000000
@@ -233,6 +243,7 @@ user_bar_in_sharing_group:
   identity_url: https://user_bar_in_sharing_group.openid.local
   is_active: true
   is_admin: false
+  username: barinsharing
 
 user1_with_load:
   owner_uuid: zzzzz-tpzed-000000000000000
@@ -257,6 +268,7 @@ fuse:
   identity_url: https://fuse.openid.local
   is_active: true
   is_admin: false
+  username: FUSE
   prefs:
     profile:
       organization: example.com
index 9bcb0116fba1628bfda8808a7cfbe408da9dc32e..9ba5646013fc8a17ae25955e607f19653c14b7b8 100644 (file)
@@ -9,6 +9,105 @@ class UserTest < ActiveSupport::TestCase
     system_user
   end
 
+  %w(a aa a0 aA Aa AA A0).each do |username|
+    test "#{username.inspect} is a valid username" do
+      user = User.new(username: username)
+      assert(user.valid?)
+    end
+  end
+
+  test "username is not required" do
+    user = User.new(username: nil)
+    assert(user.valid?)
+  end
+
+  test "username beginning with numeral is invalid" do
+    user = User.new(username: "0a")
+    refute(user.valid?)
+  end
+
+  "\\.-_/!@#$%^&*()[]{}".each_char do |bad_char|
+    test "username containing #{bad_char.inspect} is invalid" do
+      user = User.new(username: "bad#{bad_char}username")
+      refute(user.valid?)
+    end
+  end
+
+  test "username must be unique" do
+    user = User.new(username: users(:active).username)
+    refute(user.valid?)
+  end
+
+  test "non-admin can't update username" do
+    set_user_from_auth :active_trustedclient
+    user = User.find_by_uuid(users(:active).uuid)
+    user.username = "selfupdate"
+    begin
+      refute(user.save)
+    rescue ArvadosModel::PermissionDeniedError
+      # That works too.
+    end
+  end
+
+  def check_admin_username_change(fixture_name)
+    set_user_from_auth :admin_trustedclient
+    user = User.find_by_uuid(users(fixture_name).uuid)
+    user.username = "newnamefromtest"
+    assert(user.save)
+  end
+
+  test "admin can set username" do
+    check_admin_username_change(:active_no_prefs)
+  end
+
+  test "admin can update username" do
+    check_admin_username_change(:active)
+  end
+
+  test "admin can update own username" do
+    check_admin_username_change(:admin)
+  end
+
+  def check_new_username_setting(email_name, expect_name)
+    set_user_from_auth :admin
+    user = User.create!(email: "#{email_name}@example.org")
+    assert_equal(expect_name, user.username)
+  end
+
+  test "new username set from e-mail" do
+    check_new_username_setting("dakota", "dakota")
+  end
+
+  test "new username set from e-mail with leading digits" do
+    check_new_username_setting("1dakota9", "dakota9")
+  end
+
+  test "new username set from e-mail with punctuation" do
+    check_new_username_setting("dakota.9", "dakota9")
+  end
+
+  test "new username set from e-mail with leading digits and punctuation" do
+    check_new_username_setting("1.dakota.z", "dakotaz")
+  end
+
+  test "new username set from e-mail with extra part" do
+    check_new_username_setting("dakota+arvados", "dakota")
+  end
+
+  test "new username set with deduplication" do
+    name = users(:active).username
+    check_new_username_setting(name, "#{name}2")
+  end
+
+  test "new username set avoiding blacklist" do
+    Rails.configuration.auto_setup_name_blacklist = ["root"]
+    check_new_username_setting("root", "root2")
+  end
+
+  test "no username set when no base available" do
+    check_new_username_setting("_", nil)
+  end
+
   [[false, 'foo@example.com', true, nil],
    [false, 'bar@example.com', nil, true],
    [true, 'foo@example.com', true, nil],