Commit 2451bae5 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'master' into 'master'

Make getting a user by the username case insensitive

Closes #27229 and #38304

See merge request gitlab-org/gitlab-ce!21728
parents c5d8e7fc 1b153d49
...@@ -20,7 +20,7 @@ class AutocompleteController < ApplicationController ...@@ -20,7 +20,7 @@ class AutocompleteController < ApplicationController
end end
def user def user
user = UserFinder.new(params).execute! user = UserFinder.new(params[:id]).find_by_id!
render json: UserSerializer.new.represent(user) render json: UserSerializer.new.represent(user)
end end
......
...@@ -38,7 +38,7 @@ class Profiles::KeysController < Profiles::ApplicationController ...@@ -38,7 +38,7 @@ class Profiles::KeysController < Profiles::ApplicationController
def get_keys def get_keys
if params[:username].present? if params[:username].present?
begin begin
user = User.find_by_username(params[:username]) user = UserFinder.new(params[:username]).find_by_username
if user.present? if user.present?
render text: user.all_ssh_keys.join("\n"), content_type: "text/plain" render text: user.all_ssh_keys.join("\n"), content_type: "text/plain"
else else
......
...@@ -26,12 +26,9 @@ class SnippetsController < ApplicationController ...@@ -26,12 +26,9 @@ class SnippetsController < ApplicationController
layout 'snippets' layout 'snippets'
respond_to :html respond_to :html
# rubocop: disable CodeReuse/ActiveRecord
def index def index
if params[:username].present? if params[:username].present?
@user = User.find_by(username: params[:username]) @user = UserFinder.new(params[:username]).find_by_username!
return render_404 unless @user
@snippets = SnippetsFinder.new(current_user, author: @user, scope: params[:scope]) @snippets = SnippetsFinder.new(current_user, author: @user, scope: params[:scope])
.execute.page(params[:page]) .execute.page(params[:page])
...@@ -41,7 +38,6 @@ class SnippetsController < ApplicationController ...@@ -41,7 +38,6 @@ class SnippetsController < ApplicationController
redirect_to(current_user ? dashboard_snippets_path : explore_snippets_path) redirect_to(current_user ? dashboard_snippets_path : explore_snippets_path)
end end
end end
# rubocop: enable CodeReuse/ActiveRecord
def new def new
@snippet = PersonalSnippet.new @snippet = PersonalSnippet.new
......
...@@ -256,7 +256,7 @@ class IssuableFinder ...@@ -256,7 +256,7 @@ class IssuableFinder
if assignee_id? if assignee_id?
User.find_by(id: params[:assignee_id]) User.find_by(id: params[:assignee_id])
elsif assignee_username? elsif assignee_username?
User.find_by(username: params[:assignee_username]) User.find_by_username(params[:assignee_username])
else else
nil nil
end end
...@@ -284,7 +284,7 @@ class IssuableFinder ...@@ -284,7 +284,7 @@ class IssuableFinder
if author_id? if author_id?
User.find_by(id: params[:author_id]) User.find_by(id: params[:author_id])
elsif author_username? elsif author_username?
User.find_by(username: params[:author_username]) User.find_by_username(params[:author_username])
else else
nil nil
end end
......
...@@ -7,22 +7,52 @@ ...@@ -7,22 +7,52 @@
# times we may want to exclude blocked user. By using this finder (and extending # 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. # it whenever necessary) we can keep this logic in one place.
class UserFinder 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) # Tries to find a User by id, raising a `ActiveRecord::RecordNotFound` if it could
@params = params # not be found.
def find_by_id!
User.find(@username_or_id)
end end
# Tries to find a User, returning nil if none could be found. # Tries to find a User by username, returning nil if none could be found.
# rubocop: disable CodeReuse/ActiveRecord def find_by_username
def execute User.find_by_username(@username_or_id)
User.find_by(id: params[:id])
end 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. # not be found.
def execute! def find_by_username!
User.find(params[:id]) 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
end end
...@@ -43,13 +43,11 @@ class UsersFinder ...@@ -43,13 +43,11 @@ class UsersFinder
private private
# rubocop: disable CodeReuse/ActiveRecord
def by_username(users) def by_username(users)
return users unless params[:username] return users unless params[:username]
users.where(username: params[:username]) users.by_username(params[:username])
end end
# rubocop: enable CodeReuse/ActiveRecord
def by_search(users) def by_search(users)
return users unless params[:search].present? return users unless params[:search].present?
......
...@@ -264,7 +264,7 @@ class User < ActiveRecord::Base ...@@ -264,7 +264,7 @@ class User < ActiveRecord::Base
scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) } 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 :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) }
scope :confirmed, -> { where.not(confirmed_at: nil) } 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)) } scope :for_todos, -> (todos) { where(id: todos.select(:user_id)) }
# Limits the users to those that have TODOs, optionally in the given state. # Limits the users to those that have TODOs, optionally in the given state.
......
...@@ -48,7 +48,7 @@ require File.expand_path('../config/environment', File.dirname(__FILE__)) ...@@ -48,7 +48,7 @@ require File.expand_path('../config/environment', File.dirname(__FILE__))
result = Gitlab::Profiler.profile(options[:url], result = Gitlab::Profiler.profile(options[:url],
logger: Logger.new(options[:sql_output]), logger: Logger.new(options[:sql_output]),
post_data: options[:post_data], 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']) private_token: ENV['PRIVATE_TOKEN'])
printer = RubyProf::CallStackPrinter.new(result) printer = RubyProf::CallStackPrinter.new(result)
......
---
title: "Use case insensitve username lookups"
merge_request: 21728
author: William George
type: fixed
\ No newline at end of file
...@@ -235,6 +235,9 @@ You need to pass the `sudo` parameter either via query string or a header with a ...@@ -235,6 +235,9 @@ You need to pass the `sudo` parameter either via query string or a header with a
the user you want to perform the operation as. If passed as a header, the 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 If a non administrative access token is provided, an error message will
be returned with status code `403`: be returned with status code `403`:
......
...@@ -59,6 +59,9 @@ GET /users?active=true ...@@ -59,6 +59,9 @@ GET /users?active=true
GET /users?blocked=true GET /users?blocked=true
``` ```
NOTE: **Note:**
Username search is case insensitive.
### For admins ### For admins
``` ```
......
...@@ -20,7 +20,7 @@ module API ...@@ -20,7 +20,7 @@ module API
def gate_targets(params) def gate_targets(params)
targets = [] targets = []
targets << Feature.group(params[:feature_group]) if params[:feature_group] 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 targets
end end
......
...@@ -96,15 +96,9 @@ module API ...@@ -96,15 +96,9 @@ module API
LabelsFinder.new(current_user, search_params).execute LabelsFinder.new(current_user, search_params).execute
end end
# rubocop: disable CodeReuse/ActiveRecord
def find_user(id) def find_user(id)
if id =~ /^\d+$/ UserFinder.new(id).find_by_id_or_username
User.find_by(id: id)
else
User.find_by(username: id)
end end
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def find_project(id) def find_project(id)
......
...@@ -40,7 +40,7 @@ module API ...@@ -40,7 +40,7 @@ module API
elsif params[:user_id] elsif params[:user_id]
User.find_by(id: params[:user_id]) User.find_by(id: params[:user_id])
elsif params[:username] elsif params[:username]
User.find_by_username(params[:username]) UserFinder.new(params[:username]).find_by_username
end end
protocol = params[:protocol] protocol = params[:protocol]
...@@ -154,7 +154,7 @@ module API ...@@ -154,7 +154,7 @@ module API
elsif params[:user_id] elsif params[:user_id]
user = User.find_by(id: params[:user_id]) user = User.find_by(id: params[:user_id])
elsif params[:username] elsif params[:username]
user = User.find_by(username: params[:username]) user = UserFinder.new(params[:username]).find_by_username
end end
present user, with: Entities::UserSafe present user, with: Entities::UserSafe
......
...@@ -155,7 +155,6 @@ module API ...@@ -155,7 +155,6 @@ module API
requires :username, type: String, desc: 'The username of the user' requires :username, type: String, desc: 'The username of the user'
use :optional_attributes use :optional_attributes
end end
# rubocop: disable CodeReuse/ActiveRecord
post do post do
authenticated_as_admin! authenticated_as_admin!
...@@ -166,17 +165,16 @@ module API ...@@ -166,17 +165,16 @@ module API
present user, with: Entities::UserPublic, current_user: current_user present user, with: Entities::UserPublic, current_user: current_user
else else
conflict!('Email has already been taken') if User conflict!('Email has already been taken') if User
.where(email: user.email) .by_any_email(user.email.downcase)
.count > 0 .any?
conflict!('Username has already been taken') if User conflict!('Username has already been taken') if User
.where(username: user.username) .by_username(user.username)
.count > 0 .any?
render_validation_error!(user) render_validation_error!(user)
end end
end end
# rubocop: enable CodeReuse/ActiveRecord
desc 'Update a user. Available only for admins.' do desc 'Update a user. Available only for admins.' do
success Entities::UserPublic success Entities::UserPublic
...@@ -198,11 +196,11 @@ module API ...@@ -198,11 +196,11 @@ module API
not_found!('User') unless user not_found!('User') unless user
conflict!('Email has already been taken') if params[:email] && 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 .where.not(id: user.id).count > 0
conflict!('Username has already been taken') if params[:username] && 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 .where.not(id: user.id).count > 0
user_params = declared_params(include_missing: false) user_params = declared_params(include_missing: false)
......
...@@ -102,7 +102,7 @@ module Gitlab ...@@ -102,7 +102,7 @@ module Gitlab
if username.start_with?("@") if username.start_with?("@")
username = username[1..-1] username = username[1..-1]
if user = User.find_by(username: username) if user = UserFinder.new(username).find_by_username
assignee_id = user.id assignee_id = user.id
end end
end end
......
...@@ -86,7 +86,7 @@ module Gitlab ...@@ -86,7 +86,7 @@ module Gitlab
# Example: # Example:
# #
# Gitlab::Metrics.measure(:find_by_username_duration) do # Gitlab::Metrics.measure(:find_by_username_duration) do
# User.find_by_username(some_username) # UserFinder.new(some_username).find_by_username
# end # end
# #
# name - The name of the field to store the execution time in. # name - The name of the field to store the execution time in.
......
...@@ -9,7 +9,7 @@ class GithubImport ...@@ -9,7 +9,7 @@ class GithubImport
def initialize(token, gitlab_username, project_path, extras) def initialize(token, gitlab_username, project_path, extras)
@options = { token: token } @options = { token: token }
@project_path = project_path @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 raise "GitLab user #{gitlab_username} not found. Please specify a valid username." unless @current_user
......
...@@ -3,40 +3,176 @@ ...@@ -3,40 +3,176 @@
require 'spec_helper' require 'spec_helper'
describe UserFinder do 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 context 'when the user exists' do
it 'returns the user' do it 'returns the user' do
user = create(:user) found = described_class.new(user.username).find_by_username
found = described_class.new(id: user.id).execute
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) expect(found).to eq(user)
end end
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 context 'when the user does not exist' do
it 'returns nil' 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 expect(found).to be_nil
end end
end 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 context 'when the user exists' do
it 'returns the user' do it 'returns the user' do
user = create(:user) found = described_class.new(user.username).find_by_username!
found = described_class.new(id: user.id).execute!
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) expect(found).to eq(user)
end end
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 context 'when the user does not exist' do
it 'raises ActiveRecord::RecordNotFound' 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 end
end end
......
...@@ -22,6 +22,12 @@ describe UsersFinder do ...@@ -22,6 +22,12 @@ describe UsersFinder do
expect(users).to contain_exactly(user1) expect(users).to contain_exactly(user1)
end 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 it 'filters by search' do
users = described_class.new(user, search: 'orando').execute users = described_class.new(user, search: 'orando').execute
......
...@@ -368,6 +368,14 @@ describe API::Helpers do ...@@ -368,6 +368,14 @@ describe API::Helpers do
it_behaves_like 'successful sudo' it_behaves_like 'successful sudo'
end 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 context 'when providing user ID' do
before do before do
env[API::Helpers::SUDO_HEADER] = user.id.to_s env[API::Helpers::SUDO_HEADER] = user.id.to_s
...@@ -386,6 +394,14 @@ describe API::Helpers do ...@@ -386,6 +394,14 @@ describe API::Helpers do
it_behaves_like 'successful sudo' it_behaves_like 'successful sudo'
end 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 context 'when providing user ID' do
before do before do
set_param(API::Helpers::SUDO_PARAM, user.id.to_s) set_param(API::Helpers::SUDO_PARAM, user.id.to_s)
......
...@@ -51,6 +51,15 @@ describe API::Users do ...@@ -51,6 +51,15 @@ describe API::Users do
expect(json_response[0]['username']).to eq(user.username) expect(json_response[0]['username']).to eq(user.username)
end 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 it "returns an empty response when an invalid `username` parameter is passed" do
get api("/users"), username: 'invalid' get api("/users"), username: 'invalid'
...@@ -132,6 +141,14 @@ describe API::Users do ...@@ -132,6 +141,14 @@ describe API::Users do
expect(json_response.first['username']).to eq(omniauth_user.username) expect(json_response.first['username']).to eq(omniauth_user.username)
end 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 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) get api("/users?extern_uid=#{omniauth_user.identities.first.extern_uid}&provider=#{omniauth_user.identities.first.provider}", user)
...@@ -343,6 +360,12 @@ describe API::Users do ...@@ -343,6 +360,12 @@ describe API::Users do
let(:path) { "/users/#{user.username}/status" } let(:path) { "/users/#{user.username}/status" }
end end
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 end
describe "POST /users" do describe "POST /users" do
...@@ -528,6 +551,18 @@ describe API::Users do ...@@ -528,6 +551,18 @@ describe API::Users do
expect(json_response['message']).to eq('Username has already been taken') expect(json_response['message']).to eq('Username has already been taken')
end 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 it 'creates user with new identity' do
post api("/users", admin), attributes_for(:user, provider: 'github', extern_uid: '67890') post api("/users", admin), attributes_for(:user, provider: 'github', extern_uid: '67890')
...@@ -749,6 +784,14 @@ describe API::Users do ...@@ -749,6 +784,14 @@ describe API::Users do
expect(response).to have_gitlab_http_status(409) expect(response).to have_gitlab_http_status(409)
expect(@user.reload.username).to eq(@user.username) expect(@user.reload.username).to eq(@user.username)
end 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
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment