From 8ffceeb0bddd457cee62586d405afd8e082e1d6f Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curoverse.com>
Date: Thu, 20 Oct 2016 04:58:27 -0400
Subject: [PATCH] 10287: Perform blacklist and duplicate checks on usernames
 received from SSO.

---
 .../controllers/user_sessions_controller.rb   |  4 ++-
 services/api/app/models/user.rb               | 36 ++++++++++---------
 .../test/integration/user_sessions_test.rb    | 21 ++++++++---
 3 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/services/api/app/controllers/user_sessions_controller.rb b/services/api/app/controllers/user_sessions_controller.rb
index e25b4a408d..8bb27a705e 100644
--- a/services/api/app/controllers/user_sessions_controller.rb
+++ b/services/api/app/controllers/user_sessions_controller.rb
@@ -44,8 +44,10 @@ class UserSessionsController < ApplicationController
                       :last_name => omniauth['info']['last_name'],
                       :identity_url => omniauth['info']['identity_url'],
                       :is_active => Rails.configuration.new_users_are_active,
-                      :username => omniauth['info']['username'],
                       :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
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 18d33a6b0b..9363cc4f02 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -261,6 +261,25 @@ class User < ArvadosModel
     self.save!
   end
 
+  def set_initial_username(requested: false)
+    if !requested.is_a?(String) || requested.empty?
+      email_parts = email.partition("@")
+      local_parts = email_parts.first.partition("+")
+      if email_parts.any?(&:empty?)
+        return
+      elsif not local_parts.first.empty?
+        requested = local_parts.first
+      else
+        requested = email_parts.first
+      end
+    end
+    requested.sub!(/^[^A-Za-z]+/, "")
+    requested.gsub!(/[^A-Za-z0-9]/, "")
+    unless requested.empty?
+      self.username = find_usable_username_from(requested)
+    end
+  end
+
   protected
 
   def ensure_ownership_path_leads_to_user
@@ -326,23 +345,6 @@ class User < ArvadosModel
     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/test/integration/user_sessions_test.rb b/services/api/test/integration/user_sessions_test.rb
index 814e6eb670..7a9f9176d3 100644
--- a/services/api/test/integration/user_sessions_test.rb
+++ b/services/api/test/integration/user_sessions_test.rb
@@ -5,7 +5,7 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
     'https://wb.example.com'
   end
 
-  def mock_auth_with_email email
+  def mock_auth_with(email: nil, username: nil)
     mock = {
       'provider' => 'josh_id',
       'uid' => 'https://edward.example.com',
@@ -14,17 +14,30 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
         'name' => 'Edward Example',
         'first_name' => 'Edward',
         'last_name' => 'Example',
-        'email' => email,
       },
     }
+    mock['info']['email'] = email unless email.nil?
+    mock['info']['username'] = username unless username.nil?
     post('/auth/josh_id/callback',
          {return_to: client_url},
          {'omniauth.auth' => mock})
     assert_response :redirect, 'Did not redirect to client with token'
   end
 
+  test 'assign username from sso' do
+    mock_auth_with(email: 'foo@example.com', username: 'bar')
+    u = assigns(:user)
+    assert_equal 'bar', u.username
+  end
+
+  test 'no assign username from sso' do
+    mock_auth_with(email: 'foo@example.com')
+    u = assigns(:user)
+    assert_equal 'foo', u.username
+  end
+
   test 'create new user during omniauth callback' do
-    mock_auth_with_email 'edward@example.com'
+    mock_auth_with(email: 'edward@example.com')
     assert_equal(0, @response.redirect_url.index(client_url),
                  'Redirected to wrong address after succesful login: was ' +
                  @response.redirect_url + ', expected ' + client_url + '[...]')
@@ -61,7 +74,7 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest
       Rails.configuration.auto_setup_new_users_with_repository =
         testcase[:cfg][:repo]
 
-      mock_auth_with_email testcase[:email]
+      mock_auth_with(email: testcase[:email])
       u = assigns(:user)
       vm_links = Link.where('link_class=? and tail_uuid=? and head_uuid like ?',
                             'permission', u.uuid,
-- 
2.30.2