diff options
| -rw-r--r-- | app/controllers/autocomplete_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/profiles/keys_controller.rb | 2 | ||||
| -rw-r--r-- | app/controllers/snippets_controller.rb | 6 | ||||
| -rw-r--r-- | app/finders/issuable_finder.rb | 4 | ||||
| -rw-r--r-- | app/finders/user_finder.rb | 52 | ||||
| -rw-r--r-- | app/finders/users_finder.rb | 4 | ||||
| -rw-r--r-- | app/models/user.rb | 2 | ||||
| -rwxr-xr-x | bin/profile-url | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/38304-username-API-call-case-sensitive.yml | 5 | ||||
| -rw-r--r-- | doc/api/README.md | 5 | ||||
| -rw-r--r-- | doc/api/users.md | 3 | ||||
| -rw-r--r-- | lib/api/features.rb | 2 | ||||
| -rw-r--r-- | lib/api/helpers.rb | 8 | ||||
| -rw-r--r-- | lib/api/internal.rb | 4 | ||||
| -rw-r--r-- | lib/api/users.rb | 14 | ||||
| -rw-r--r-- | lib/gitlab/google_code_import/importer.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/metrics/influx_db.rb | 2 | ||||
| -rw-r--r-- | lib/tasks/import.rake | 2 | ||||
| -rw-r--r-- | spec/finders/user_finder_spec.rb | 154 | ||||
| -rw-r--r-- | spec/finders/users_finder_spec.rb | 6 | ||||
| -rw-r--r-- | spec/requests/api/helpers_spec.rb | 16 | ||||
| -rw-r--r-- | spec/requests/api/users_spec.rb | 43 |
22 files changed, 284 insertions, 56 deletions
diff --git a/app/controllers/autocomplete_controller.rb b/app/controllers/autocomplete_controller.rb index 3766b64a091..0d5c8657c9e 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 01801c31327..3c3dc03a4ee 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 694c3a59e2b..dd9bf17cf0c 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 1f98ecf95ca..8abfe0c4c17 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 815388c894e..556be4c4338 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 f2ad9b4bda5..81ae50c0bd1 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 a0665518cf5..34efb22b359 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 d8d09641624..9e8585aabba 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 00000000000..b89778b6c23 --- /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 a351db99bbd..ec539e2d044 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 07f03f9c827..ee24aa09156 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 6f2422af13a..1331248699f 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 a7ba8066233..60bf977f0e4 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 6a264c4cc6d..4dd6b19e353 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 501c5cf1df3..47382b09207 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 94c15739231..0c08c0fedaa 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 04135dac4ff..ce9d3ec3de4 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 fc59b3f937d..a16d4c47273 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 e53aa50dd33..4771b878b8e 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 4249c52c481..fecf97dc641 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 0a789d58fd8..cca449e9e56 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 09c1d016081..e6d01c9689f 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 |