From 1b153d497b6948932b0de2f0088fe7192eb0994a Mon Sep 17 00:00:00 2001
From: William George <code@williamgeorge.co.uk>
Date: Thu, 18 Oct 2018 09:06:44 +0000
Subject: [PATCH] Make getting a user by the username case insensitive
---
app/controllers/autocomplete_controller.rb | 2 +-
app/controllers/profiles/keys_controller.rb | 2 +-
app/controllers/snippets_controller.rb | 6 +-
app/finders/issuable_finder.rb | 4 +-
app/finders/user_finder.rb | 52 ++++--
app/finders/users_finder.rb | 4 +-
app/models/user.rb | 2 +-
bin/profile-url | 2 +-
...38304-username-API-call-case-sensitive.yml | 5 +
doc/api/README.md | 5 +-
doc/api/users.md | 3 +
lib/api/features.rb | 2 +-
lib/api/helpers.rb | 8 +-
lib/api/internal.rb | 4 +-
lib/api/users.rb | 14 +-
lib/gitlab/google_code_import/importer.rb | 2 +-
lib/gitlab/metrics/influx_db.rb | 2 +-
lib/tasks/import.rake | 2 +-
spec/finders/user_finder_spec.rb | 154 +++++++++++++++++-
spec/finders/users_finder_spec.rb | 6 +
spec/requests/api/helpers_spec.rb | 16 ++
spec/requests/api/users_spec.rb | 43 +++++
22 files changed, 284 insertions(+), 56 deletions(-)
create mode 100644 changelogs/unreleased/38304-username-API-call-case-sensitive.yml
diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb
index 3766b64a0915..0d5c8657c9eb 100644
--- a/app/controllers/autocomplete_controller.rb
+++ b/app/controllers/autocomplete_controller.rb
@@ -20,7 +20,7 @@ def users
end
def user
- user = UserFinder.new(params).execute!
+ user = UserFinder.new(params[:id]).find_by_id!
render json: UserSerializer.new.represent(user)
end
diff --git a/app/controllers/profiles/keys_controller.rb b/app/controllers/profiles/keys_controller.rb
index 01801c313276..3c3dc03a4eef 100644
--- a/app/controllers/profiles/keys_controller.rb
+++ b/app/controllers/profiles/keys_controller.rb
@@ -38,7 +38,7 @@ def destroy
def get_keys
if params[:username].present?
begin
- user = User.find_by_username(params[:username])
+ user = UserFinder.new(params[:username]).find_by_username
if user.present?
render text: user.all_ssh_keys.join("\n"), content_type: "text/plain"
else
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index 694c3a59e2b7..dd9bf17cf0c7 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -26,12 +26,9 @@ class SnippetsController < ApplicationController
layout 'snippets'
respond_to :html
- # rubocop: disable CodeReuse/ActiveRecord
def index
if params[:username].present?
- @user = User.find_by(username: params[:username])
-
- return render_404 unless @user
+ @user = UserFinder.new(params[:username]).find_by_username!
@snippets = SnippetsFinder.new(current_user, author: @user, scope: params[:scope])
.execute.page(params[:page])
@@ -41,7 +38,6 @@ def index
redirect_to(current_user ? dashboard_snippets_path : explore_snippets_path)
end
end
- # rubocop: enable CodeReuse/ActiveRecord
def new
@snippet = PersonalSnippet.new
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 1f98ecf95ca0..8abfe0c4c176 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -256,7 +256,7 @@ def assignee
if assignee_id?
User.find_by(id: params[:assignee_id])
elsif assignee_username?
- User.find_by(username: params[:assignee_username])
+ User.find_by_username(params[:assignee_username])
else
nil
end
@@ -284,7 +284,7 @@ def author
if author_id?
User.find_by(id: params[:author_id])
elsif author_username?
- User.find_by(username: params[:author_username])
+ User.find_by_username(params[:author_username])
else
nil
end
diff --git a/app/finders/user_finder.rb b/app/finders/user_finder.rb
index 815388c894ef..556be4c4338c 100644
--- a/app/finders/user_finder.rb
+++ b/app/finders/user_finder.rb
@@ -7,22 +7,52 @@
# times we may want to exclude blocked user. By using this finder (and extending
# it whenever necessary) we can keep this logic in one place.
class UserFinder
- attr_reader :params
+ def initialize(username_or_id)
+ @username_or_id = username_or_id
+ end
+
+ # Tries to find a User by id, returning nil if none could be found.
+ def find_by_id
+ User.find_by_id(@username_or_id)
+ end
- def initialize(params)
- @params = params
+ # Tries to find a User by id, raising a `ActiveRecord::RecordNotFound` if it could
+ # not be found.
+ def find_by_id!
+ User.find(@username_or_id)
end
- # Tries to find a User, returning nil if none could be found.
- # rubocop: disable CodeReuse/ActiveRecord
- def execute
- User.find_by(id: params[:id])
+ # Tries to find a User by username, returning nil if none could be found.
+ def find_by_username
+ User.find_by_username(@username_or_id)
end
- # rubocop: enable CodeReuse/ActiveRecord
- # Tries to find a User, raising a `ActiveRecord::RecordNotFound` if it could
+ # Tries to find a User by username, raising a `ActiveRecord::RecordNotFound` if it could
# not be found.
- def execute!
- User.find(params[:id])
+ def find_by_username!
+ User.find_by_username!(@username_or_id)
+ end
+
+ # Tries to find a User by username or id, returning nil if none could be found.
+ def find_by_id_or_username
+ if input_is_id?
+ find_by_id
+ else
+ find_by_username
+ end
+ end
+
+ # Tries to find a User by username or id, raising a `ActiveRecord::RecordNotFound` if it could
+ # not be found.
+ def find_by_id_or_username!
+ if input_is_id?
+ find_by_id!
+ else
+ find_by_username!
+ end
+ end
+
+ def input_is_id?
+ @username_or_id.is_a?(Numeric) || @username_or_id =~ /^\d+$/
end
end
diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb
index f2ad9b4bda5e..81ae50c0bd1f 100644
--- a/app/finders/users_finder.rb
+++ b/app/finders/users_finder.rb
@@ -43,13 +43,11 @@ def execute
private
- # rubocop: disable CodeReuse/ActiveRecord
def by_username(users)
return users unless params[:username]
- users.where(username: params[:username])
+ users.by_username(params[:username])
end
- # rubocop: enable CodeReuse/ActiveRecord
def by_search(users)
return users unless params[:search].present?
diff --git a/app/models/user.rb b/app/models/user.rb
index a0665518cf5b..34efb22b3598 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -264,7 +264,7 @@ def inactive_message
scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) }
scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) }
scope :confirmed, -> { where.not(confirmed_at: nil) }
- scope :by_username, -> (usernames) { iwhere(username: usernames) }
+ scope :by_username, -> (usernames) { iwhere(username: Array(usernames).map(&:to_s)) }
scope :for_todos, -> (todos) { where(id: todos.select(:user_id)) }
# Limits the users to those that have TODOs, optionally in the given state.
diff --git a/bin/profile-url b/bin/profile-url
index d8d096416249..9e8585aabba8 100755
--- a/bin/profile-url
+++ b/bin/profile-url
@@ -48,7 +48,7 @@ require File.expand_path('../config/environment', File.dirname(__FILE__))
result = Gitlab::Profiler.profile(options[:url],
logger: Logger.new(options[:sql_output]),
post_data: options[:post_data],
- user: User.find_by_username(options[:username]),
+ user: UserFinder.new(options[:username]).find_by_username,
private_token: ENV['PRIVATE_TOKEN'])
printer = RubyProf::CallStackPrinter.new(result)
diff --git a/changelogs/unreleased/38304-username-API-call-case-sensitive.yml b/changelogs/unreleased/38304-username-API-call-case-sensitive.yml
new file mode 100644
index 000000000000..b89778b6c239
--- /dev/null
+++ b/changelogs/unreleased/38304-username-API-call-case-sensitive.yml
@@ -0,0 +1,5 @@
+---
+title: "Use case insensitve username lookups"
+merge_request: 21728
+author: William George
+type: fixed
\ No newline at end of file
diff --git a/doc/api/README.md b/doc/api/README.md
index a351db99bbda..ec539e2d0444 100644
--- a/doc/api/README.md
+++ b/doc/api/README.md
@@ -233,7 +233,10 @@ provided you are authenticated as an administrator with an OAuth or Personal Acc
You need to pass the `sudo` parameter either via query string or a header with an ID/username of
the user you want to perform the operation as. If passed as a header, the
-header name must be `Sudo`.
+header name must be `Sudo`.
+
+NOTE: **Note:**
+Usernames are case insensitive.
If a non administrative access token is provided, an error message will
be returned with status code `403`:
diff --git a/doc/api/users.md b/doc/api/users.md
index 07f03f9c827f..ee24aa091566 100644
--- a/doc/api/users.md
+++ b/doc/api/users.md
@@ -59,6 +59,9 @@ GET /users?active=true
GET /users?blocked=true
```
+NOTE: **Note:**
+Username search is case insensitive.
+
### For admins
```
diff --git a/lib/api/features.rb b/lib/api/features.rb
index 6f2422af13ae..1331248699fd 100644
--- a/lib/api/features.rb
+++ b/lib/api/features.rb
@@ -20,7 +20,7 @@ def gate_value(params)
def gate_targets(params)
targets = []
targets << Feature.group(params[:feature_group]) if params[:feature_group]
- targets << User.find_by_username(params[:user]) if params[:user]
+ targets << UserFinder.new(params[:user]).find_by_username if params[:user]
targets
end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index a7ba80662339..60bf977f0e4d 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -96,15 +96,9 @@ def available_labels_for(label_parent)
LabelsFinder.new(current_user, search_params).execute
end
- # rubocop: disable CodeReuse/ActiveRecord
def find_user(id)
- if id =~ /^\d+$/
- User.find_by(id: id)
- else
- User.find_by(username: id)
- end
+ UserFinder.new(id).find_by_id_or_username
end
- # rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
def find_project(id)
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 6a264c4cc6d4..4dd6b19e3536 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -40,7 +40,7 @@ def response_with_status(code: 200, success: true, message: nil, **extra_options
elsif params[:user_id]
User.find_by(id: params[:user_id])
elsif params[:username]
- User.find_by_username(params[:username])
+ UserFinder.new(params[:username]).find_by_username
end
protocol = params[:protocol]
@@ -154,7 +154,7 @@ def response_with_status(code: 200, success: true, message: nil, **extra_options
elsif params[:user_id]
user = User.find_by(id: params[:user_id])
elsif params[:username]
- user = User.find_by(username: params[:username])
+ user = UserFinder.new(params[:username]).find_by_username
end
present user, with: Entities::UserSafe
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 501c5cf1df33..47382b092074 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -155,7 +155,6 @@ def reorder_users(users)
requires :username, type: String, desc: 'The username of the user'
use :optional_attributes
end
- # rubocop: disable CodeReuse/ActiveRecord
post do
authenticated_as_admin!
@@ -166,17 +165,16 @@ def reorder_users(users)
present user, with: Entities::UserPublic, current_user: current_user
else
conflict!('Email has already been taken') if User
- .where(email: user.email)
- .count > 0
+ .by_any_email(user.email.downcase)
+ .any?
conflict!('Username has already been taken') if User
- .where(username: user.username)
- .count > 0
+ .by_username(user.username)
+ .any?
render_validation_error!(user)
end
end
- # rubocop: enable CodeReuse/ActiveRecord
desc 'Update a user. Available only for admins.' do
success Entities::UserPublic
@@ -198,11 +196,11 @@ def reorder_users(users)
not_found!('User') unless user
conflict!('Email has already been taken') if params[:email] &&
- User.where(email: params[:email])
+ User.by_any_email(params[:email].downcase)
.where.not(id: user.id).count > 0
conflict!('Username has already been taken') if params[:username] &&
- User.where(username: params[:username])
+ User.by_username(params[:username])
.where.not(id: user.id).count > 0
user_params = declared_params(include_missing: false)
diff --git a/lib/gitlab/google_code_import/importer.rb b/lib/gitlab/google_code_import/importer.rb
index 94c157392316..0c08c0fedaa7 100644
--- a/lib/gitlab/google_code_import/importer.rb
+++ b/lib/gitlab/google_code_import/importer.rb
@@ -102,7 +102,7 @@ def import_issues
if username.start_with?("@")
username = username[1..-1]
- if user = User.find_by(username: username)
+ if user = UserFinder.new(username).find_by_username
assignee_id = user.id
end
end
diff --git a/lib/gitlab/metrics/influx_db.rb b/lib/gitlab/metrics/influx_db.rb
index 04135dac4ff5..ce9d3ec3de48 100644
--- a/lib/gitlab/metrics/influx_db.rb
+++ b/lib/gitlab/metrics/influx_db.rb
@@ -86,7 +86,7 @@ def escape_value(value)
# Example:
#
# Gitlab::Metrics.measure(:find_by_username_duration) do
- # User.find_by_username(some_username)
+ # UserFinder.new(some_username).find_by_username
# end
#
# name - The name of the field to store the execution time in.
diff --git a/lib/tasks/import.rake b/lib/tasks/import.rake
index fc59b3f937d5..a16d4c47273f 100644
--- a/lib/tasks/import.rake
+++ b/lib/tasks/import.rake
@@ -9,7 +9,7 @@ class GithubImport
def initialize(token, gitlab_username, project_path, extras)
@options = { token: token }
@project_path = project_path
- @current_user = User.find_by(username: gitlab_username)
+ @current_user = UserFinder.new(gitlab_username).find_by_username
raise "GitLab user #{gitlab_username} not found. Please specify a valid username." unless @current_user
diff --git a/spec/finders/user_finder_spec.rb b/spec/finders/user_finder_spec.rb
index e53aa50dd33d..4771b878b8e5 100644
--- a/spec/finders/user_finder_spec.rb
+++ b/spec/finders/user_finder_spec.rb
@@ -3,40 +3,176 @@
require 'spec_helper'
describe UserFinder do
- describe '#execute' do
+ set(:user) { create(:user) }
+
+ describe '#find_by_id' do
+ context 'when the user exists' do
+ it 'returns the user' do
+ found = described_class.new(user.id).find_by_id
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user exists (id as string)' do
+ it 'returns the user' do
+ found = described_class.new(user.id.to_s).find_by_id
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user does not exist' do
+ it 'returns nil' do
+ found = described_class.new(1).find_by_id
+
+ expect(found).to be_nil
+ end
+ end
+ end
+
+ describe '#find_by_username' do
context 'when the user exists' do
it 'returns the user' do
- user = create(:user)
- found = described_class.new(id: user.id).execute
+ found = described_class.new(user.username).find_by_username
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user does not exist' do
+ it 'returns nil' do
+ found = described_class.new("non_existent_username").find_by_username
+
+ expect(found).to be_nil
+ end
+ end
+ end
+
+ describe '#find_by_id_or_username' do
+ context 'when the user exists (id)' do
+ it 'returns the user' do
+ found = described_class.new(user.id).find_by_id_or_username
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user exists (id as string)' do
+ it 'returns the user' do
+ found = described_class.new(user.id.to_s).find_by_id_or_username
expect(found).to eq(user)
end
end
+ context 'when the user exists (username)' do
+ it 'returns the user' do
+ found = described_class.new(user.username).find_by_id_or_username
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user does not exist (username)' do
+ it 'returns nil' do
+ found = described_class.new("non_existent_username").find_by_id_or_username
+
+ expect(found).to be_nil
+ end
+ end
+
context 'when the user does not exist' do
it 'returns nil' do
- found = described_class.new(id: 1).execute
+ found = described_class.new(1).find_by_id_or_username
expect(found).to be_nil
end
end
end
- describe '#execute!' do
+ describe '#find_by_id!' do
+ context 'when the user exists' do
+ it 'returns the user' do
+ found = described_class.new(user.id).find_by_id!
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user exists (id as string)' do
+ it 'returns the user' do
+ found = described_class.new(user.id.to_s).find_by_id!
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user does not exist' do
+ it 'raises ActiveRecord::RecordNotFound' do
+ finder = described_class.new(1)
+
+ expect { finder.find_by_id! }.to raise_error(ActiveRecord::RecordNotFound)
+ end
+ end
+ end
+
+ describe '#find_by_username!' do
context 'when the user exists' do
it 'returns the user' do
- user = create(:user)
- found = described_class.new(id: user.id).execute!
+ found = described_class.new(user.username).find_by_username!
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user does not exist' do
+ it 'raises ActiveRecord::RecordNotFound' do
+ finder = described_class.new("non_existent_username")
+
+ expect { finder.find_by_username! }.to raise_error(ActiveRecord::RecordNotFound)
+ end
+ end
+ end
+
+ describe '#find_by_id_or_username!' do
+ context 'when the user exists (id)' do
+ it 'returns the user' do
+ found = described_class.new(user.id).find_by_id_or_username!
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user exists (id as string)' do
+ it 'returns the user' do
+ found = described_class.new(user.id.to_s).find_by_id_or_username!
expect(found).to eq(user)
end
end
+ context 'when the user exists (username)' do
+ it 'returns the user' do
+ found = described_class.new(user.username).find_by_id_or_username!
+
+ expect(found).to eq(user)
+ end
+ end
+
+ context 'when the user does not exist (username)' do
+ it 'raises ActiveRecord::RecordNotFound' do
+ finder = described_class.new("non_existent_username")
+
+ expect { finder.find_by_id_or_username! }.to raise_error(ActiveRecord::RecordNotFound)
+ end
+ end
+
context 'when the user does not exist' do
it 'raises ActiveRecord::RecordNotFound' do
- finder = described_class.new(id: 1)
+ finder = described_class.new(1)
- expect { finder.execute! }.to raise_error(ActiveRecord::RecordNotFound)
+ expect { finder.find_by_id_or_username! }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
diff --git a/spec/finders/users_finder_spec.rb b/spec/finders/users_finder_spec.rb
index 4249c52c481e..fecf97dc6411 100644
--- a/spec/finders/users_finder_spec.rb
+++ b/spec/finders/users_finder_spec.rb
@@ -22,6 +22,12 @@
expect(users).to contain_exactly(user1)
end
+ it 'filters by username (case insensitive)' do
+ users = described_class.new(user, username: 'joHNdoE').execute
+
+ expect(users).to contain_exactly(user1)
+ end
+
it 'filters by search' do
users = described_class.new(user, search: 'orando').execute
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 0a789d58fd81..cca449e9e56e 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -368,6 +368,14 @@ def set_param(key, value)
it_behaves_like 'successful sudo'
end
+ context 'when providing username (case insensitive)' do
+ before do
+ env[API::Helpers::SUDO_HEADER] = user.username.upcase
+ end
+
+ it_behaves_like 'successful sudo'
+ end
+
context 'when providing user ID' do
before do
env[API::Helpers::SUDO_HEADER] = user.id.to_s
@@ -386,6 +394,14 @@ def set_param(key, value)
it_behaves_like 'successful sudo'
end
+ context 'when providing username (case insensitive)' do
+ before do
+ set_param(API::Helpers::SUDO_PARAM, user.username.upcase)
+ end
+
+ it_behaves_like 'successful sudo'
+ end
+
context 'when providing user ID' do
before do
set_param(API::Helpers::SUDO_PARAM, user.id.to_s)
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 09c1d0160815..e6d01c9689f0 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -51,6 +51,15 @@
expect(json_response[0]['username']).to eq(user.username)
end
+ it "returns the user when a valid `username` parameter is passed (case insensitive)" do
+ get api("/users"), username: user.username.upcase
+
+ expect(response).to match_response_schema('public_api/v4/user/basics')
+ expect(json_response.size).to eq(1)
+ expect(json_response[0]['id']).to eq(user.id)
+ expect(json_response[0]['username']).to eq(user.username)
+ end
+
it "returns an empty response when an invalid `username` parameter is passed" do
get api("/users"), username: 'invalid'
@@ -132,6 +141,14 @@
expect(json_response.first['username']).to eq(omniauth_user.username)
end
+ it "returns one user (case insensitive)" do
+ get api("/users?username=#{omniauth_user.username.upcase}", user)
+
+ expect(response).to match_response_schema('public_api/v4/user/basics')
+ expect(response).to include_pagination_headers
+ expect(json_response.first['username']).to eq(omniauth_user.username)
+ end
+
it "returns a 403 when non-admin user searches by external UID" do
get api("/users?extern_uid=#{omniauth_user.identities.first.extern_uid}&provider=#{omniauth_user.identities.first.provider}", user)
@@ -343,6 +360,12 @@
let(:path) { "/users/#{user.username}/status" }
end
end
+
+ context 'when finding the user by username (case insensitive)' do
+ it_behaves_like 'rendering user status' do
+ let(:path) { "/users/#{user.username.upcase}/status" }
+ end
+ end
end
describe "POST /users" do
@@ -528,6 +551,18 @@
expect(json_response['message']).to eq('Username has already been taken')
end
+ it 'returns 409 conflict error if same username exists (case insensitive)' do
+ expect do
+ post api('/users', admin),
+ name: 'foo',
+ email: 'foo@example.com',
+ password: 'password',
+ username: 'TEST'
+ end.to change { User.count }.by(0)
+ expect(response).to have_gitlab_http_status(409)
+ expect(json_response['message']).to eq('Username has already been taken')
+ end
+
it 'creates user with new identity' do
post api("/users", admin), attributes_for(:user, provider: 'github', extern_uid: '67890')
@@ -749,6 +784,14 @@
expect(response).to have_gitlab_http_status(409)
expect(@user.reload.username).to eq(@user.username)
end
+
+ it 'returns 409 conflict error if username taken (case insensitive)' do
+ @user_id = User.all.last.id
+ put api("/users/#{@user.id}", admin), username: 'TEST'
+
+ expect(response).to have_gitlab_http_status(409)
+ expect(@user.reload.username).to eq(@user.username)
+ end
end
end
--
GitLab