Merge branch 'master' into 15558-alternate-email-addresses
authorPeter Amstutz <pamstutz@veritasgenetics.com>
Wed, 4 Sep 2019 20:10:34 +0000 (16:10 -0400)
committerPeter Amstutz <pamstutz@veritasgenetics.com>
Wed, 4 Sep 2019 20:10:34 +0000 (16:10 -0400)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

services/api/app/controllers/user_sessions_controller.rb
services/api/app/models/user.rb
services/api/test/integration/remote_user_test.rb
services/api/test/integration/users_test.rb
services/api/test/unit/user_test.rb

index ef0f8868666dfb3bb786dab263270c8911df45e6..ca7ba4a40aaec0f8381512252ada75b622f61d39 100644 (file)
@@ -15,68 +15,13 @@ class UserSessionsController < ApplicationController
   def create
     omniauth = request.env['omniauth.auth']
 
-    identity_url_ok = (omniauth['info']['identity_url'].length > 0) rescue false
-    unless identity_url_ok
-      # Whoa. This should never happen.
-      logger.error "UserSessionsController.create: omniauth object missing/invalid"
-      logger.error "omniauth: "+omniauth.pretty_inspect
-
+    begin
+      user = User.register omniauth['info']
+    rescue => e
+      Rails.logger.warn e
       return redirect_to login_failure_url
     end
 
-    # Only local users can create sessions, hence uuid_like_pattern
-    # here.
-    user = User.unscoped.where('identity_url = ? and uuid like ?',
-                               omniauth['info']['identity_url'],
-                               User.uuid_like_pattern).first
-    if not user
-      # Check for permission to log in to an existing User record with
-      # a different identity_url
-      Link.where("link_class = ? and name = ? and tail_uuid = ? and head_uuid like ?",
-                 'permission',
-                 'can_login',
-                 omniauth['info']['email'],
-                 User.uuid_like_pattern).each do |link|
-        if prefix = link.properties['identity_url_prefix']
-          if prefix == omniauth['info']['identity_url'][0..prefix.size-1]
-            user = User.find_by_uuid(link.head_uuid)
-            break if user
-          end
-        end
-      end
-    end
-
-    if not user
-      # New user registration
-      user = User.new(:email => omniauth['info']['email'],
-                      :first_name => omniauth['info']['first_name'],
-                      :last_name => omniauth['info']['last_name'],
-                      :identity_url => omniauth['info']['identity_url'],
-                      :is_active => Rails.configuration.Users.NewUsersAreActive,
-                      :owner_uuid => system_user_uuid)
-      if omniauth['info']['username']
-        user.set_initial_username(requested: omniauth['info']['username'])
-      end
-      act_as_system_user do
-        user.save or raise Exception.new(user.errors.messages)
-      end
-    else
-      user.email = omniauth['info']['email']
-      user.first_name = omniauth['info']['first_name']
-      user.last_name = omniauth['info']['last_name']
-      if user.identity_url.nil?
-        # First login to a pre-activated account
-        user.identity_url = omniauth['info']['identity_url']
-      end
-
-      while (uuid = user.redirect_to_user_uuid)
-        user = User.unscoped.where(uuid: uuid).first
-        if !user
-          raise Exception.new("identity_url #{omniauth['info']['identity_url']} redirects to nonexistent uuid #{uuid}")
-        end
-      end
-    end
-
     # For the benefit of functional and integration tests:
     @user = user
 
index ee44812e0075aaa1eb0b71a66a8c60ec73907930..9526f6593308da0e3700d0be2c417e248dfa361f 100644 (file)
@@ -327,6 +327,90 @@ class User < ArvadosModel
     end
   end
 
+  def redirects_to
+    user = self
+    redirects = 0
+    while (uuid = user.redirect_to_user_uuid)
+      user = User.unscoped.find_by_uuid(uuid)
+      if !user
+        raise Exception.new("user uuid #{user.uuid} redirects to nonexistent uuid #{uuid}")
+      end
+      redirects += 1
+      if redirects > 15
+        raise "Starting from #{self.uuid} redirect_to_user_uuid exceeded maximum number of redirects"
+      end
+    end
+    user
+  end
+
+  def self.register info
+    # login info expected fields, all can be optional but at minimum
+    # must supply either 'identity_url' or 'email'
+    #
+    #   email
+    #   first_name
+    #   last_name
+    #   username
+    #   alternate_emails
+    #   identity_url
+
+    info = info.with_indifferent_access
+
+    primary_user = nil
+
+    # local database
+    identity_url = info['identity_url']
+
+    if identity_url && identity_url.length > 0
+      # Only local users can create sessions, hence uuid_like_pattern
+      # here.
+      user = User.unscoped.where('identity_url = ? and uuid like ?',
+                                 identity_url,
+                                 User.uuid_like_pattern).first
+      primary_user = user.redirects_to if user
+    end
+
+    if !primary_user
+      # identity url is unset or didn't find matching record.
+      emails = [info['email']] + (info['alternate_emails'] || [])
+      emails.select! {|em| !em.nil? && !em.empty?}
+
+      User.unscoped.where('email in (?) and uuid like ?',
+                          emails,
+                          User.uuid_like_pattern).each do |user|
+        if !primary_user
+          primary_user = user.redirects_to
+        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
+
+    if !primary_user
+      # New user registration
+      primary_user = User.new(:owner_uuid => system_user_uuid,
+                              :is_admin => false,
+                              :is_active => Rails.configuration.Users.NewUsersAreActive)
+
+      primary_user.set_initial_username(requested: info['username']) if info['username']
+    end
+
+    primary_user.email = info['email'] if info['email']
+    primary_user.identity_url = info['identity_url'] if identity_url
+    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.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
+
+    act_as_system_user do
+      primary_user.save!
+    end
+
+    primary_user
+  end
+
   protected
 
   def change_all_uuid_refs(old_uuid:, new_uuid:)
@@ -345,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
index 90a55865397cefb6ce7e0829eca626a21734c2fc..81ea9623e886ba9434162a4202b150a78b62441f 100644 (file)
@@ -33,39 +33,50 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
 
     @controller = Arvados::V1::UsersController.new
     ready = Thread::Queue.new
-    srv = WEBrick::HTTPServer.new(
-      Port: 0,
-      Logger: WEBrick::Log.new(
-        Rails.root.join("log", "webrick.log").to_s,
-        WEBrick::Log::INFO),
-      AccessLog: [[File.open(Rails.root.join(
-                              "log", "webrick_access.log").to_s, 'a+'),
-                   WEBrick::AccessLog::COMBINED_LOG_FORMAT]],
-      SSLEnable: true,
-      SSLVerifyClient: OpenSSL::SSL::VERIFY_NONE,
-      SSLPrivateKey: OpenSSL::PKey::RSA.new(
-        File.open(Rails.root.join("tmp", "self-signed.key")).read),
-      SSLCertificate: OpenSSL::X509::Certificate.new(
-        File.open(Rails.root.join("tmp", "self-signed.pem")).read),
-      SSLCertName: [["CN", WEBrick::Utils::getservername]],
-      StartCallback: lambda { ready.push(true) })
-    srv.mount_proc '/discovery/v1/apis/arvados/v1/rest' do |req, res|
-      Rails.cache.delete 'arvados_v1_rest_discovery'
-      res.body = Arvados::V1::SchemaController.new.send(:discovery_doc).to_json
-    end
-    srv.mount_proc '/arvados/v1/users/current' do |req, res|
-      res.status = @stub_status
-      res.body = @stub_content.is_a?(String) ? @stub_content : @stub_content.to_json
-    end
-    Thread.new do
-      srv.start
+
+    @remote_server = []
+    @remote_host = []
+
+    ['zbbbb', 'zbork'].each do |clusterid|
+      srv = WEBrick::HTTPServer.new(
+        Port: 0,
+        Logger: WEBrick::Log.new(
+          Rails.root.join("log", "webrick.log").to_s,
+          WEBrick::Log::INFO),
+        AccessLog: [[File.open(Rails.root.join(
+                                 "log", "webrick_access.log").to_s, 'a+'),
+                     WEBrick::AccessLog::COMBINED_LOG_FORMAT]],
+        SSLEnable: true,
+        SSLVerifyClient: OpenSSL::SSL::VERIFY_NONE,
+        SSLPrivateKey: OpenSSL::PKey::RSA.new(
+          File.open(Rails.root.join("tmp", "self-signed.key")).read),
+        SSLCertificate: OpenSSL::X509::Certificate.new(
+          File.open(Rails.root.join("tmp", "self-signed.pem")).read),
+        SSLCertName: [["CN", WEBrick::Utils::getservername]],
+        StartCallback: lambda { ready.push(true) })
+      srv.mount_proc '/discovery/v1/apis/arvados/v1/rest' do |req, res|
+        Rails.cache.delete 'arvados_v1_rest_discovery'
+        res.body = Arvados::V1::SchemaController.new.send(:discovery_doc).to_json
+      end
+      srv.mount_proc '/arvados/v1/users/current' do |req, res|
+        if clusterid == 'zbbbb' and req.header['authorization'][0][10..14] == 'zbork'
+          # asking zbbbb about zbork should yield an error, zbbbb doesn't trust zbork
+          res.status = 401
+          return
+        end
+        res.status = @stub_status
+        res.body = @stub_content.is_a?(String) ? @stub_content : @stub_content.to_json
+      end
+      Thread.new do
+        srv.start
+      end
+      ready.pop
+      @remote_server << srv
+      @remote_host << "127.0.0.1:#{srv.config[:Port]}"
     end
-    ready.pop
-    @remote_server = srv
-    @remote_host = "127.0.0.1:#{srv.config[:Port]}"
-    Rails.configuration.RemoteClusters = Rails.configuration.RemoteClusters.merge({zbbbb: ActiveSupport::InheritableOptions.new({Host: @remote_host}),
-                                                                                   zbork: ActiveSupport::InheritableOptions.new({Host: @remote_host})})
-    Arvados::V1::SchemaController.any_instance.stubs(:root_url).returns "https://#{@remote_host}"
+    Rails.configuration.RemoteClusters = Rails.configuration.RemoteClusters.merge({zbbbb: ActiveSupport::InheritableOptions.new({Host: @remote_host[0]}),
+                                                                                   zbork: ActiveSupport::InheritableOptions.new({Host: @remote_host[1]})})
+    Arvados::V1::SchemaController.any_instance.stubs(:root_url).returns "https://#{@remote_host[0]}"
     @stub_status = 200
     @stub_content = {
       uuid: 'zbbbb-tpzed-000000000000000',
@@ -77,7 +88,9 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
   end
 
   teardown do
-    @remote_server.andand.stop
+    @remote_server.each do |srv|
+      srv.stop
+    end
   end
 
   test 'authenticate with remote token' do
@@ -148,7 +161,7 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
     assert_equal 'foo', json_response['username']
   end
 
-  test 'authenticate with remote token from misbhehaving remote cluster' do
+  test 'authenticate with remote token from misbehaving remote cluster' do
     get '/arvados/v1/users/current',
       params: {format: 'json'},
       headers: auth(remote: 'zbork')
@@ -256,13 +269,22 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
   end
 
   test 'pre-activate remote user' do
+    @stub_content = {
+      uuid: 'zbbbb-tpzed-000000000001234',
+      email: 'foo@example.com',
+      username: 'barney',
+      is_admin: true,
+      is_active: true,
+    }
+
     post '/arvados/v1/users',
       params: {
         "user" => {
-          "uuid" => "zbbbb-tpzed-000000000000000",
+          "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)}"}
@@ -272,13 +294,35 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
       params: {format: 'json'},
       headers: auth(remote: 'zbbbb')
     assert_response :success
-    assert_equal 'zbbbb-tpzed-000000000000000', json_response['uuid']
-    assert_equal nil, json_response['is_admin']
+    puts json_response
+    assert_equal 'zbbbb-tpzed-000000000001234', json_response['uuid']
+    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']
   end
 
+
+  test 'remote user inactive without pre-activation' do
+    @stub_content = {
+      uuid: 'zbbbb-tpzed-000000000001234',
+      email: 'foo@example.com',
+      username: 'barney',
+      is_admin: true,
+      is_active: true,
+    }
+
+    get '/arvados/v1/users/current',
+      params: {format: 'json'},
+      headers: auth(remote: 'zbbbb')
+    assert_response :success
+    assert_equal 'zbbbb-tpzed-000000000001234', json_response['uuid']
+    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']
+  end
+
   test "validate unsalted v2 token for remote cluster zbbbb" do
     auth = api_client_authorizations(:active)
     token = "v2/#{auth.uuid}/#{auth.api_token}"
index 5886fb2d08965ee494898a4bf1ca06cfc70a18f2..6b74154073d5edce800efaeeb7c666b1180af4b5 100644 (file)
@@ -275,4 +275,32 @@ class UsersTest < ActionDispatch::IntegrationTest
     assert_response(:success)
     assert_equal(project_uuid, json_response['owner_uuid'])
   end
+
+  test 'pre-activate user' do
+    post '/arvados/v1/users',
+      params: {
+        "user" => {
+          "email" => 'foo@example.com',
+          "is_active" => true,
+          "username" => "barney"
+        }
+      },
+      headers: {'HTTP_AUTHORIZATION' => "OAuth2 #{api_token(:admin)}"}
+    assert_response :success
+    rp = json_response
+    assert_not_nil rp["uuid"]
+    assert_not_nil rp["is_active"]
+    assert_nil rp["is_admin"]
+
+    get "/arvados/v1/users/#{rp['uuid']}",
+      params: {format: 'json'},
+      headers: auth(:admin)
+    assert_response :success
+    assert_equal rp["uuid"], json_response['uuid']
+    assert_nil 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']
+  end
+
 end
index 6d2157b144d689b54534b6cd71a7de37e30b3791..9fafeb0b23e338d86c21e5aa2e49078201d92ee8 100644 (file)
@@ -800,4 +800,86 @@ class UserTest < ActiveSupport::TestCase
       end
     end
   end
+
+  test "lookup user by email" do
+    u = User.register({"email" => "active-user@arvados.local", "identity_url" => "different-identity-url"})
+    active = User.find_by_uuid(users(:active).uuid)
+    assert_equal active.uuid, u.uuid
+    assert_equal "different-identity-url", active.identity_url
+    assert_equal "active-user@arvados.local", active.email
+  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
+
+  test "register new user" do
+    u = User.register({"email" => "never-before-seen-user@arvados.local",
+                       "identity_url" => "different-identity-url",
+                       "first_name" => "Robert",
+                       "last_name" => "Baratheon",
+                       "username" => "bobby"})
+    nbs = User.find_by_uuid(u.uuid)
+    assert_equal nbs.uuid, u.uuid
+    assert_equal "different-identity-url", nbs.identity_url
+    assert_equal "never-before-seen-user@arvados.local", nbs.email
+    assert_equal false, nbs.is_admin
+    assert_equal false , nbs.is_active
+    assert_equal "bobby", nbs.username
+    assert_equal "Robert", nbs.first_name
+    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