Test various login scenarios if the limit gets enforced

9 files changed, 150 insertions, 36 deletions

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