diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 3766b64a09154caecd497c0ef7cecfea51e8b3b4..0d5c8657c9eba40280b83fbd3d76e8df9060f01f 100644 --- a/app/controllers/autocomplete_controller.rb +++ b/app/controllers/autocomplete_controller.rb @@ -20,7 +20,7 @@ class AutocompleteController < ApplicationController 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 01801c313276ee2eabc01d063536e0372d39ebf1..3c3dc03a4eefe4a9e1bc818fa09cbb2e9c046d3d 100644 --- a/app/controllers/profiles/keys_controller.rb +++ b/app/controllers/profiles/keys_controller.rb @@ -38,7 +38,7 @@ class Profiles::KeysController < Profiles::ApplicationController 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 694c3a59e2b77c4ab15a0d748ac64bb65b9e0e5e..dd9bf17cf0c7fc2a49c16a7b6b2de80a07fb1e77 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 @@ class SnippetsController < ApplicationController 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 1f98ecf95ca0304f0707816e7ae9db65ce731a00..8abfe0c4c176887678419681fe1b3bd5955473d9 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -256,7 +256,7 @@ class IssuableFinder 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 @@ class IssuableFinder 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 815388c894ef584108d1299ba86030c72439f8b5..556be4c4338cb9e9bc75c42d725772fd6d50f551 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 f2ad9b4bda5ee2166f02cd617fb9c50989203cdf..81ae50c0bd1fb714a446e883d6e345057bc4e4a3 100644 --- a/app/finders/users_finder.rb +++ b/app/finders/users_finder.rb @@ -43,13 +43,11 @@ class UsersFinder 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 a0665518cf5b6669689a2f79d6e87f6b90f64215..34efb22b35982a57b290a438e9415ff346cf67f2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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_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 d8d096416249cfe1ad1225fd777e50682d3d33f3..9e8585aabba8f5af1a6bf284a14d5d493a1b7d02 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 0000000000000000000000000000000000000000..b89778b6c239116d99f8a01d4c6a4f864d3ca4de --- /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 a351db99bbdabbeaa62772c46266a701f1840ca6..ec539e2d044415ee9e321e8bf6b437e4aa364558 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 07f03f9c827f937c3fa19d094176b27a27ed9bd1..ee24aa0915664ba9a9080ba71f89dde45d80d114 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 6f2422af13ae4a79075030611f6e4232e5567488..1331248699fd5794915bb215e5999d97d67cd0a6 100644 --- a/lib/api/features.rb +++ b/lib/api/features.rb @@ -20,7 +20,7 @@ module API 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 a7ba80662339b9b60e96981ae7c065ea7bc04473..60bf977f0e4d7b2d0cddb88cda3fbc1e5e829fd0 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -96,15 +96,9 @@ module API 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 6a264c4cc6d4b1673baf6050f48162f5963be357..4dd6b19e3536e7357ddd22671336f3f4a87633d0 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -40,7 +40,7 @@ module API 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 @@ module API 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 501c5cf1df33bc25e4e15a1fe75d62b2e8b577dd..47382b092074cb3e4c6a4de2323b250da2869c15 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -155,7 +155,6 @@ module API 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 @@ module API 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 @@ module API 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 94c1573923164db42c1653eee3a05887db3e5853..0c08c0fedaa704f073fb8d26cfe6fdeea6721499 100644 --- a/lib/gitlab/google_code_import/importer.rb +++ b/lib/gitlab/google_code_import/importer.rb @@ -102,7 +102,7 @@ module Gitlab 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 04135dac4ff5a0cfdd7f7c864d6592e5534f607d..ce9d3ec3de485f3593880f610786d6765156fc34 100644 --- a/lib/gitlab/metrics/influx_db.rb +++ b/lib/gitlab/metrics/influx_db.rb @@ -86,7 +86,7 @@ module Gitlab # 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 fc59b3f937d53ba500218ee56be3c891d6bdba32..a16d4c47273fd0746b40c3cb5b0f18a7ab0d152e 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 e53aa50dd33d8871d36b21da385b5d1ba2d8ed34..4771b878b8e53281cd83f6886048cd85b152c103 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 4249c52c481e9415da4ba094ceebf51b5ec622e3..fecf97dc64114d0984e7c000f5d3994c4294a6dc 100644 --- a/spec/finders/users_finder_spec.rb +++ b/spec/finders/users_finder_spec.rb @@ -22,6 +22,12 @@ describe UsersFinder do 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 0a789d58fd8125c737f0ea5c79b29e7fa8b3d415..cca449e9e56e859badd27202ca26adb03f9a9e48 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -368,6 +368,14 @@ describe API::Helpers do 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 @@ describe API::Helpers do 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 09c1d01608159adae66c909bb71fb556eada948e..e6d01c9689f08a071d5b0c683cde94a0f2a6a9cc 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -51,6 +51,15 @@ describe API::Users do 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 @@ describe API::Users do 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 @@ describe API::Users do 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 @@ describe API::Users do 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 @@ describe API::Users do 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