From 8993801f0cefdc64b46b8fe30622cc78eaa03173 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Fri, 17 Feb 2017 12:52:27 +0100 Subject: [PATCH] Test various login scenarios if the limit gets enforced --- lib/api/api.rb | 4 ++ lib/api/helpers.rb | 15 ++--- lib/gitlab/auth.rb | 2 +- lib/gitlab/auth/unique_ips_limiter.rb | 2 +- spec/controllers/sessions_controller_spec.rb | 30 +++++++++- .../gitlab/auth/unique_ips_limiter_spec.rb | 22 +++---- spec/lib/gitlab/auth_spec.rb | 24 ++++++++ spec/requests/api/doorkeeper_access_spec.rb | 60 +++++++++++++++---- .../unique_ip_check_shared_examples.rb | 27 +++++++++ 9 files changed, 150 insertions(+), 36 deletions(-) create mode 100644 spec/support/unique_ip_check_shared_examples.rb diff --git a/lib/api/api.rb b/lib/api/api.rb index 89449ce8813..6f37fa9d8e9 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -60,6 +60,10 @@ module API error! e.message, e.status, e.headers end + rescue_from Gitlab::Auth::TooManyIps do |e| + rack_response({'message'=>'403 Forbidden'}.to_json, 403) + end + rescue_from :all do |exception| handle_api_exception(exception) end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index a43252a4661..f325f0a3050 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -336,16 +336,17 @@ module API def initial_current_user return @initial_current_user if defined?(@initial_current_user) + Gitlab::Auth::UniqueIpsLimiter.limit_user! do + @initial_current_user ||= find_user_by_private_token(scopes: @scopes) + @initial_current_user ||= doorkeeper_guard(scopes: @scopes) + @initial_current_user ||= find_user_from_warden - @initial_current_user ||= find_user_by_private_token(scopes: @scopes) - @initial_current_user ||= doorkeeper_guard(scopes: @scopes) - @initial_current_user ||= find_user_from_warden + unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed? + @initial_current_user = nil + end - unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed? - @initial_current_user = nil + @initial_current_user end - - @initial_current_user end def sudo! diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index be055080853..8e2aee2d7a0 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -22,7 +22,7 @@ module Gitlab user_with_password_for_git(login, password) || Gitlab::Auth::Result.new - Gitlab::Auth::UniqueIpsLimiter.limit_user! { result.actor } + Gitlab::Auth::UniqueIpsLimiter.limit_user!(result.actor) rate_limit!(ip, success: result.success?, login: login) diff --git a/lib/gitlab/auth/unique_ips_limiter.rb b/lib/gitlab/auth/unique_ips_limiter.rb index 01850ae31e8..7f849ef4c38 100644 --- a/lib/gitlab/auth/unique_ips_limiter.rb +++ b/lib/gitlab/auth/unique_ips_limiter.rb @@ -62,7 +62,7 @@ module Gitlab rescue TooManyIps => ex Rails.logger.info ex.message - [429, { 'Content-Type' => 'text/plain', 'Retry-After' => UniqueIpsLimiter.config.unique_ips_limit_time_window }, ["Retry later\n"]] + [403, { 'Content-Type' => 'text/plain', 'Retry-After' => UniqueIpsLimiter.config.unique_ips_limit_time_window }, ["Too many logins from different IPs\n"]] end end end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index b56c7880b64..f1157d159ab 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -25,9 +25,35 @@ describe SessionsController do expect(subject.current_user). to eq user end - it "creates an audit log record" do + it 'creates an audit log record' do expect { post(:create, user: { login: user.username, password: user.password }) }.to change { SecurityEvent.count }.by(1) - expect(SecurityEvent.last.details[:with]).to eq("standard") + expect(SecurityEvent.last.details[:with]).to eq('standard') + end + + context 'unique ip limit is enabled and set to 1', :redis do + before do + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_enabled).and_return(true) + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_time_window).and_return(10) + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_per_user).and_return(1) + end + + it 'allows user authenticating from the same ip' do + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('ip') + post(:create, user: { login: user.username, password: user.password }) + expect(subject.current_user).to eq user + + post(:create, user: { login: user.username, password: user.password }) + expect(subject.current_user).to eq user + end + + it 'blocks user authenticating from two distinct ips' do + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('ip') + post(:create, user: { login: user.username, password: user.password }) + expect(subject.current_user).to eq user + + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('ip2') + expect { post(:create, user: { login: user.username, password: user.password }) }.to raise_error(Gitlab::Auth::TooManyIps) + end end end end diff --git a/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb b/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb index ccaddddf98f..f2472b4310f 100644 --- a/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb +++ b/spec/lib/gitlab/auth/unique_ips_limiter_spec.rb @@ -1,14 +1,8 @@ require 'spec_helper' -describe Gitlab::Auth::UniqueIpsLimiter, lib: true do +describe Gitlab::Auth::UniqueIpsLimiter, :redis, lib: true do let(:user) { create(:user) } - before(:each) do - Gitlab::Redis.with do |redis| - redis.del("user_unique_ips:#{user.id}") - end - end - describe '#count_unique_ips' do context 'non unique IPs' do it 'properly counts them' do @@ -25,7 +19,7 @@ describe Gitlab::Auth::UniqueIpsLimiter, lib: true do end it 'resets count after specified time window' do - cur_time = Time.now.to_i + cur_time = Time.now allow(Time).to receive(:now).and_return(cur_time) expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, '192.168.1.2')).to eq(1) @@ -51,15 +45,15 @@ describe Gitlab::Auth::UniqueIpsLimiter, lib: true do end it 'blocks user trying to login from second ip' do - RequestStore[:client_ip] = '192.168.1.1' + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('192.168.1.1') expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) - RequestStore[:client_ip] = '192.168.1.2' + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('192.168.1.2') expect { Gitlab::Auth::UniqueIpsLimiter.limit_user! { user } }.to raise_error(Gitlab::Auth::TooManyIps) end it 'allows user trying to login from the same ip twice' do - RequestStore[:client_ip] = '192.168.1.1' + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('192.168.1.1') expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) end @@ -71,13 +65,13 @@ describe Gitlab::Auth::UniqueIpsLimiter, lib: true do end it 'blocks user trying to login from third ip' do - RequestStore[:client_ip] = '192.168.1.1' + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('192.168.1.1') expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) - RequestStore[:client_ip] = '192.168.1.2' + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('192.168.1.2') expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user) - RequestStore[:client_ip] = '192.168.1.3' + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('192.168.1.3') expect { Gitlab::Auth::UniqueIpsLimiter.limit_user! { user } }.to raise_error(Gitlab::Auth::TooManyIps) end end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index b234de4c772..ee70ef34f4f 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -58,6 +58,30 @@ describe Gitlab::Auth, lib: true do expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) end + + context 'unique ip limit is enabled and set to 1', :redis do + before do + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_enabled).and_return(true) + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_time_window).and_return(10) + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_per_user).and_return(1) + end + + it 'allows user authenticating from the same ip' do + user = create(:user, password: 'password') + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('ip') + expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) + expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) + end + + it 'blocks user authenticating from two distinct ips' do + user = create(:user, password: 'password') + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('ip') + expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) + allow(Gitlab::RequestContext).to receive(:client_ip).and_return('ip2') + expect { gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip2') }.to raise_error(Gitlab::Auth::TooManyIps) + end + end + context 'while using LFS authenticate' do it 'recognizes user lfs tokens' do user = create(:user) diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb index bd9ecaf2685..1cd0701d955 100644 --- a/spec/requests/api/doorkeeper_access_spec.rb +++ b/spec/requests/api/doorkeeper_access_spec.rb @@ -4,27 +4,65 @@ describe API::API, api: true do include ApiHelpers let!(:user) { create(:user) } - let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) } - let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "api" } + let!(:application) { Doorkeeper::Application.create!(name: 'MyApp', redirect_uri: 'https://app.com', owner: user) } + let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: 'api' } - describe "when unauthenticated" do - it "returns authentication success" do - get api("/user"), access_token: token.token + describe 'when unauthenticated' do + it 'returns authentication success' do + get api('/user'), access_token: token.token expect(response).to have_http_status(200) end + + include_context 'limit login to only one ip' do + it 'allows login twice from the same ip' do + get api('/user'), access_token: token.token + expect(response).to have_http_status(200) + + get api('/user'), access_token: token.token + expect(response).to have_http_status(200) + end + + it 'blocks login from two different ips' do + get api('/user'), access_token: token.token + expect(response).to have_http_status(200) + + change_ip('ip2') + get api('/user'), access_token: token.token + expect(response).to have_http_status(403) + end + end end - describe "when token invalid" do - it "returns authentication error" do - get api("/user"), access_token: "123a" + describe 'when token invalid' do + it 'returns authentication error' do + get api('/user'), access_token: '123a' expect(response).to have_http_status(401) end end - describe "authorization by private token" do - it "returns authentication success" do - get api("/user", user) + describe 'authorization by private token' do + it 'returns authentication success' do + get api('/user', user) expect(response).to have_http_status(200) end + + include_context 'limit login to only one ip' do + it 'allows login twice from the same ip' do + get api('/user', user) + expect(response).to have_http_status(200) + + get api('/user', user) + expect(response).to have_http_status(200) + end + + it 'blocks login from two different ips' do + get api('/user', user) + expect(response).to have_http_status(200) + + change_ip('ip2') + get api('/user', user) + expect(response).to have_http_status(403) + end + end end end diff --git a/spec/support/unique_ip_check_shared_examples.rb b/spec/support/unique_ip_check_shared_examples.rb new file mode 100644 index 00000000000..ab693b91d4a --- /dev/null +++ b/spec/support/unique_ip_check_shared_examples.rb @@ -0,0 +1,27 @@ +shared_context 'limit login to only one ip', :redis do + before do + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_enabled).and_return(true) + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_time_window).and_return(1000) + allow(Gitlab::Auth::UniqueIpsLimiter).to receive_message_chain(:config, :unique_ips_limit_per_user).and_return(1) + end + + def change_ip(ip) + allow(Gitlab::RequestContext).to receive(:client_ip).and_return(ip) + end +end + +shared_examples 'user login operation with unique ip limit' do + include_context 'limit login to only one ip' do + it 'allows user authenticating from the same ip' do + expect { operation }.not_to raise_error + expect { operation }.not_to raise_error + end + + it 'blocks user authenticating from two distinct ips' do + expect { operation }.not_to raise_error + + change_ip('ip2') + expect { operation }.to raise_error(Gitlab::Auth::TooManyIps) + end + end +end