From ff8721dd5854279fab712f4a4ff8aed2256c1b87 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Tue, 17 Mar 2015 18:10:14 -0400 Subject: [PATCH] 4253: Add a username attribute to users. * 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 | 1 + services/api/app/models/user.rb | 73 +++++++++- .../20150317132720_add_username_to_users.rb | 127 ++++++++++++++++++ services/api/db/structure.sql | 16 ++- services/api/test/fixtures/users.yml | 12 ++ services/api/test/unit/user_test.rb | 99 ++++++++++++++ 6 files changed, 320 insertions(+), 8 deletions(-) create mode 100644 services/api/db/migrate/20150317132720_add_username_to_users.rb diff --git a/doc/api/schema/User.html.textile.liquid b/doc/api/schema/User.html.textile.liquid index 9a1b0566d4..335b6c1097 100644 --- a/doc/api/schema/User.html.textile.liquid +++ b/doc/api/schema/User.html.textile.liquid @@ -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||| diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index a47a4583cd..3978df741a 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -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 index 0000000000..de2fc9648b --- /dev/null +++ b/services/api/db/migrate/20150317132720_add_username_to_users.rb @@ -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 diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql index 0711f90251..40861f4557 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -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 diff --git a/services/api/test/fixtures/users.yml b/services/api/test/fixtures/users.yml index c04aa47d2f..46fbd0f289 100644 --- a/services/api/test/fixtures/users.yml +++ b/services/api/test/fixtures/users.yml @@ -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 diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb index 9bcb0116fb..9ba5646013 100644 --- a/services/api/test/unit/user_test.rb +++ b/services/api/test/unit/user_test.rb @@ -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], -- 2.30.2