Commit 0ef8a643 authored by Pawel Chojnacki's avatar Pawel Chojnacki
Browse files

Remove unecessary calls to limit_user!, UniqueIps Middleware, and address MR review

 - cleanup formating in haml
 - clarify time window is in seconds
 - cleanup straneous chunks in db/schema
 - rename count_uniqe_ips to update_and_return_ips_count
 - other
parent 02f5b7dbd883
......@@ -40,6 +40,10 @@ class ApplicationController < ActionController::Base
render_403
end
rescue_from Gitlab::Auth::TooManyIps do |e|
head :forbidden, retry_after: UniqueIpsLimiter.config.unique_ips_limit_time_window
end
def redirect_back_or_default(default: root_path, options: {})
redirect_to request.referer.present? ? :back : default, options
end
......
......@@ -67,12 +67,10 @@ def user_params
end
def find_user
Gitlab::Auth::UniqueIpsLimiter.limit_user! do
if session[:otp_user_id]
User.find(session[:otp_user_id])
elsif user_params[:login]
User.by_login(user_params[:login])
end
if session[:otp_user_id]
User.find(session[:otp_user_id])
elsif user_params[:login]
User.by_login(user_params[:login])
end
end
......
......@@ -366,7 +366,8 @@
= f.label :unique_ips_limit_enabled do
= f.check_box :unique_ips_limit_enabled
Limit sign in from multiple ips
%span.help-block#recaptcha_help_block Helps prevent malicious users hide their activity
%span.help-block#unique_ip_help_block
Helps prevent malicious users hide their activity
.form-group
= f.label :unique_ips_limit_per_user, 'IPs per user', class: 'control-label col-sm-2'
......@@ -380,7 +381,7 @@
.col-sm-10
= f.number_field :unique_ips_limit_time_window, class: 'form-control'
.help-block
How long an IP will be counted towards the limit
How many seconds an IP will be counted towards the limit
%fieldset
%legend Abuse reports
......
......@@ -8,8 +8,6 @@ module Gitlab
class Application < Rails::Application
require_dependency Rails.root.join('lib/gitlab/redis')
require_dependency Rails.root.join('lib/gitlab/request_context')
require_dependency Rails.root.join('lib/gitlab/auth')
require_dependency Rails.root.join('lib/gitlab/auth/unique_ips_limiter')
# Settings in config/environments/* take precedence over those specified here.
# Application configuration should go into files in config/initializers
......@@ -114,8 +112,6 @@ class Application < Rails::Application
config.middleware.insert_before Warden::Manager, Rack::Attack
config.middleware.insert_before Warden::Manager, Gitlab::Auth::UniqueIpsLimiter
# Allow access to GitLab API from other domains
config.middleware.insert_before Warden::Manager, Rack::Cors do
allow do
......
......@@ -12,10 +12,8 @@
end
resource_owner_from_credentials do |routes|
Gitlab::Auth::UniqueIpsLimiter.limit_user! do
user = Gitlab::Auth.find_with_user_password(params[:username], params[:password])
user unless user.try(:two_factor_enabled?)
end
user = Gitlab::Auth.find_with_user_password(params[:username], params[:password])
user unless user.try(:two_factor_enabled?)
end
# If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below.
......
......@@ -4,14 +4,14 @@ class AddUniqueIpsLimitToApplicationSettings < ActiveRecord::Migration
disable_ddl_transaction!
def up
add_column_with_default(:application_settings, :unique_ips_limit_per_user, :integer, default: 10)
add_column_with_default(:application_settings, :unique_ips_limit_time_window, :integer, default: 3600)
add_column_with_default(:application_settings, :unique_ips_limit_enabled, :boolean, default: false)
add_column_with_default :application_settings, :unique_ips_limit_per_user, :integer, default: 10
add_column_with_default :application_settings, :unique_ips_limit_time_window, :integer, default: 3600
add_column_with_default :application_settings, :unique_ips_limit_enabled, :boolean, default: false
end
def down
remove_column(:application_settings, :unique_ips_limit_per_user)
remove_column(:application_settings, :unique_ips_limit_time_window)
remove_column(:application_settings, :unique_ips_limit_enabled)
remove_column :application_settings, :unique_ips_limit_per_user
remove_column :application_settings, :unique_ips_limit_time_window
remove_column :application_settings, :unique_ips_limit_enabled
end
end
......@@ -586,9 +586,9 @@
end
add_index "labels", ["group_id", "project_id", "title"], name: "index_labels_on_group_id_and_project_id_and_title", unique: true, using: :btree
add_index "labels", ["type", "project_id"], name: "index_labels_on_type_and_project_id", using: :btree
add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree
add_index "labels", ["title"], name: "index_labels_on_title", using: :btree
add_index "labels", ["type", "project_id"], name: "index_labels_on_type_and_project_id", using: :btree
create_table "lfs_objects", force: :cascade do |t|
t.string "oid", null: false
......
......@@ -22,9 +22,8 @@ def find_for_git_client(login, password, project:, ip:)
user_with_password_for_git(login, password) ||
Gitlab::Auth::Result.new
Gitlab::Auth::UniqueIpsLimiter.limit_user!(result.actor)
rate_limit!(ip, success: result.success?, login: login)
Gitlab::Auth::UniqueIpsLimiter.limit_user!(result.actor)
result
end
......
module Gitlab
module Auth
class TooManyIps < StandardError
attr_reader :user_id, :ip, :unique_ips_count
def initialize(user_id, ip, unique_ips_count)
@user_id = user_id
@ip = ip
@unique_ips_count = unique_ips_count
end
def message
"User #{user_id} from IP: #{ip} tried logging from too many ips: #{unique_ips_count}"
end
end
end
end
module Gitlab
module Auth
class TooManyIps < StandardError
attr_reader :user_id, :ip, :unique_ips_count
def initialize(user_id, ip, unique_ips_count)
@user_id = user_id
@ip = ip
@unique_ips_count = unique_ips_count
end
def message
"User #{user_id} from IP: #{ip} tried logging from too many ips: #{unique_ips_count}"
end
end
class UniqueIpsLimiter
USER_UNIQUE_IPS_PREFIX = 'user_unique_ips'
......@@ -21,7 +7,7 @@ class << self
def limit_user_id!(user_id)
if config.unique_ips_limit_enabled
ip = RequestContext.client_ip
unique_ips = count_unique_ips(user_id, ip)
unique_ips = update_and_return_ips_count(user_id, ip)
raise TooManyIps.new(user_id, ip, unique_ips) if unique_ips > config.unique_ips_limit_per_user
end
end
......@@ -36,8 +22,8 @@ def config
Gitlab::CurrentSettings.current_application_settings
end
def count_unique_ips(user_id, ip)
time = Time.now.to_i
def update_and_return_ips_count(user_id, ip)
time = Time.now.utc.to_i
key = "#{USER_UNIQUE_IPS_PREFIX}:#{user_id}"
Gitlab::Redis.with do |redis|
......@@ -51,20 +37,6 @@ def count_unique_ips(user_id, ip)
end
end
end
def initialize(app)
@app = app
end
def call(env)
begin
@app.call(env)
rescue TooManyIps => ex
Rails.logger.info ex.message
[403, { 'Content-Type' => 'text/plain', 'Retry-After' => UniqueIpsLimiter.config.unique_ips_limit_time_window }, ["Too many logins from different IPs\n"]]
end
end
end
end
end
......@@ -14,7 +14,6 @@ def initialize(app)
end
def call(env)
raise RequestStoreNotActive.new unless RequestStore.active?
req = Rack::Request.new(env)
RequestStore[:client_ip] = req.ip
......
......@@ -33,6 +33,7 @@
include_examples 'user login operation with unique ip limit' do
def operation
post(:create, user: { login: user.username, password: user.password })
expect(subject.current_user).to eq user
end
end
......
......@@ -7,36 +7,35 @@
describe '#count_unique_ips' do
context 'non unique IPs' do
it 'properly counts them' do
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip1')).to eq(1)
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip1')).to eq(1)
expect(described_class.update_and_return_ips_count(user.id, 'ip1')).to eq(1)
expect(described_class.update_and_return_ips_count(user.id, 'ip1')).to eq(1)
end
end
context 'unique IPs' do
it 'properly counts them' do
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip2')).to eq(1)
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip3')).to eq(2)
expect(described_class.update_and_return_ips_count(user.id, 'ip2')).to eq(1)
expect(described_class.update_and_return_ips_count(user.id, 'ip3')).to eq(2)
end
end
it 'resets count after specified time window' do
cur_time = Time.now
allow(Time).to receive(:now).and_return(cur_time)
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip2')).to eq(1)
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip3')).to eq(2)
allow(Time).to receive(:now).and_return(cur_time + Gitlab::Auth::UniqueIpsLimiter.config.unique_ips_limit_time_window)
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip4')).to eq(1)
expect(Gitlab::Auth::UniqueIpsLimiter.count_unique_ips(user.id, 'ip5')).to eq(2)
Timecop.freeze do
expect(described_class.update_and_return_ips_count(user.id, 'ip2')).to eq(1)
expect(described_class.update_and_return_ips_count(user.id, 'ip3')).to eq(2)
Timecop.travel(Time.now.utc + described_class.config.unique_ips_limit_time_window) do
expect(described_class.update_and_return_ips_count(user.id, 'ip4')).to eq(1)
expect(described_class.update_and_return_ips_count(user.id, 'ip5')).to eq(2)
end
end
end
end
describe '#limit_user!' do
include_examples 'user login operation with unique ip limit' do
def operation
Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }
described_class.limit_user! { user }
end
end
......@@ -45,13 +44,13 @@ def operation
it 'blocks user trying to login from third ip' do
change_ip('ip1')
expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user)
expect(described_class.limit_user! { user }).to eq(user)
change_ip('ip2')
expect(Gitlab::Auth::UniqueIpsLimiter.limit_user! { user }).to eq(user)
expect(described_class.limit_user! { user }).to eq(user)
change_ip('ip3')
expect { Gitlab::Auth::UniqueIpsLimiter.limit_user! { user } }.to raise_error(Gitlab::Auth::TooManyIps)
expect { described_class.limit_user! { user } }.to raise_error(Gitlab::Auth::TooManyIps)
end
end
end
......
......@@ -206,7 +206,7 @@ def operation
include_examples 'user login operation with unique ip limit' do
def operation
expect(gl_auth.find_with_user_password(username, password)).to eql user
expect(gl_auth.find_with_user_password(username, password)).to eq(user)
end
end
......
......@@ -26,15 +26,5 @@
it { is_expected.to be_nil }
end
end
context 'RequestStore is not active' do
it { is_expected.to be_nil }
context 'when RequestContext mw is run' do
subject { -> { Gitlab::RequestContext.new(app).call(env) } }
it { is_expected.to raise_error(Gitlab::RequestStoreNotActive) }
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