15558: email address must refer to unique user account

Email address must unambigiously refer to one user account, after
following redirects.

Make email address field read-only by non-admins.

Also fix tests and add a few new ones.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index db9eddc066f8d301fca11a58a94d20a758538709..9526f6593308da0e3700d0be2c417e248dfa361f 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -374,16 +374,14 @@ class User < ArvadosModel
       # identity url is unset or didn't find matching record.
       emails = [info['email']] + (info['alternate_emails'] || [])
       emails.select! {|em| !em.nil? && !em.empty?}
-      emails.each do |em|
-        # Go through each email address, try to find a user record
-        # corresponding to one of the addresses supplied.
-
-        user = User.unscoped.where('email = ? and uuid like ?',
-                                   em,
-                                   User.uuid_like_pattern).first
-        if user
+
+      User.unscoped.where('email in (?) and uuid like ?',
+                          emails,
+                          User.uuid_like_pattern).each do |user|
+        if !primary_user
           primary_user = user.redirects_to
-          break
+        elsif primary_user.uuid != user.redirects_to.uuid
+          raise "Ambigious email address, directs to both #{primary_user.uuid} and #{user.redirects_to.uuid}"
         end
       end
     end
@@ -402,7 +400,7 @@ class User < ArvadosModel
     primary_user.first_name = info['first_name'] if info['first_name']
     primary_user.last_name = info['last_name'] if info['last_name']
 
-    if (!primary_user.email or primary_user.identity_url.empty?) and (!primary_user.identity_url or primary_user.identity_url.empty?)
+    if (!primary_user.email or primary_user.email.empty?) and (!primary_user.identity_url or primary_user.identity_url.empty?)
       raise "Must have supply at least one of 'email' or 'identity_url' to User.register"
     end
 
@@ -431,7 +429,7 @@ class User < ArvadosModel
   end
 
   def permission_to_update
-    if username_changed? || redirect_to_user_uuid_changed?
+    if username_changed? || redirect_to_user_uuid_changed? || email_changed?
       current_user.andand.is_admin
     else
       # users must be able to update themselves (even if they are

diff --git a/services/api/test/integration/remote_user_test.rb b/services/api/test/integration/remote_user_test.rb
index f68a271ede02e23f040729720de6ef12dd1a4528..81ea9623e886ba9434162a4202b150a78b62441f 100644 (file)
--- a/services/api/test/integration/remote_user_test.rb
+++ b/services/api/test/integration/remote_user_test.rb
@@ -283,7 +283,8 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
           "uuid" => "zbbbb-tpzed-000000000001234",
           "email" => 'foo@example.com',
           "username" => 'barney',
-          "is_active" => true
+          "is_active" => true,
+          "is_admin" => false
         }
       },
       headers: {'HTTP_AUTHORIZATION' => "OAuth2 #{api_token(:admin)}"}
@@ -293,8 +294,9 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
       params: {format: 'json'},
       headers: auth(remote: 'zbbbb')
     assert_response :success
+    puts json_response
     assert_equal 'zbbbb-tpzed-000000000001234', json_response['uuid']
-    assert_nil json_response['is_admin']
+    assert_equal false, json_response['is_admin']
     assert_equal true, json_response['is_active']
     assert_equal 'foo@example.com', json_response['email']
     assert_equal 'barney', json_response['username']
@@ -315,7 +317,7 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
       headers: auth(remote: 'zbbbb')
     assert_response :success
     assert_equal 'zbbbb-tpzed-000000000001234', json_response['uuid']
-    assert_nil json_response['is_admin']
+    assert_equal false, json_response['is_admin']
     assert_equal false, json_response['is_active']
     assert_equal 'foo@example.com', json_response['email']
     assert_equal 'barney', json_response['username']

diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 21252eeafa44492e705e7194da015c28f0ff0035..9fafeb0b23e338d86c21e5aa2e49078201d92ee8 100644 (file)
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -810,11 +810,15 @@ class UserTest < ActiveSupport::TestCase
   end
 
   test "lookup user by alternate email" do
+    # register method will find that active-user@arvados.local already
+    # exists and return existing 'active' user.
     u = User.register({"email" => "user@parent-company.com",
                        "alternate_emails" => ["active-user@arvados.local"],
                        "identity_url" => "different-identity-url"})
     active = User.find_by_uuid(users(:active).uuid)
     assert_equal active.uuid, u.uuid
+
+    # identity_url and email of 'active' should be updated
     assert_equal "different-identity-url", active.identity_url
     assert_equal "user@parent-company.com", active.email
   end
@@ -836,11 +840,46 @@ class UserTest < ActiveSupport::TestCase
     assert_equal "Baratheon", nbs.last_name
   end
 
+  test "fail when email address is ambigious" do
+    User.register({"email" => "active-user@arvados.local"})
+    u = User.register({"email" => "never-before-seen-user@arvados.local"})
+    u.email = "active-user@arvados.local"
+    act_as_system_user do
+      u.save!
+    end
+    assert_raises do
+      User.register({"email" => "active-user@arvados.local"})
+    end
+  end
+
   test "fail lookup without identifiers" do
     assert_raises do
       User.register({"first_name" => "Robert", "last_name" => "Baratheon"})
     end
+    assert_raises do
+      User.register({"first_name" => "Robert", "last_name" => "Baratheon", "identity_url" => "", "email" => ""})
+    end
   end
 
+  test "user can update name" do
+    set_user_from_auth :active
+    user = users(:active)
+    user.first_name = "MyNewName"
+    assert user.save
+  end
+
+  test "user cannot update email" do
+    set_user_from_auth :active
+    user = users(:active)
+    user.email = "new-name@example.com"
+    assert_not_allowed { user.save }
+  end
+
+  test "admin can update email" do
+    set_user_from_auth :admin
+    user = users(:active)
+    user.email = "new-name@example.com"
+    assert user.save
+  end
 
 end