From 533e3bc65cc474f629431771cf52ac42f7710030 Mon Sep 17 00:00:00 2001 From: Martin Lensment Date: Mon, 29 Jun 2015 11:13:19 +0300 Subject: [PATCH 1/3] Add detailed error messages on EPP login #2716 --- app/controllers/epp/sessions_controller.rb | 39 ++++++++++++++++------ config/locales/en.yml | 3 +- spec/epp/session_spec.rb | 4 +-- spec/features/registrar/sessions_spec.rb | 4 +-- 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/app/controllers/epp/sessions_controller.rb b/app/controllers/epp/sessions_controller.rb index b6b75fcfd..f0a1e2458 100644 --- a/app/controllers/epp/sessions_controller.rb +++ b/app/controllers/epp/sessions_controller.rb @@ -7,17 +7,39 @@ class Epp::SessionsController < EppController # rubocop: disable Metrics/PerceivedComplexity # rubocop: disable Metrics/CyclomaticComplexity + # rubocop: disable Metrics/MethodLength def login - cert_valid = true + success = true @api_user = ApiUser.find_by(login_params) if request.ip != ENV['webclient_ip'] && @api_user unless @api_user.api_pki_ok?(request.env['HTTP_SSL_CLIENT_CERT'], request.env['HTTP_SSL_CLIENT_S_DN_CN']) - cert_valid = false + @msg = 'Authentication error; server closing connection (certificate is not valid)' + success = false end end - if @api_user.try(:active) && cert_valid && ip_white? && connection_limit_ok? + if success && !@api_user + @msg = 'Authentication error; server closing connection (API user not found)' + success = false + end + + if success && !@api_user.try(:active) + @msg = 'Authentication error; server closing connection (API user is not active)' + success = false + end + + if success && !ip_white? + @msg = 'Authentication error; server closing connection (IP is not whitelisted)' + success = false + end + + if success && !connection_limit_ok? + @msg = 'Authentication error; server closing connection (connection limit reached)' + success = false + end + + if success if parsed_frame.css('newPW').first unless @api_user.update(password: parsed_frame.css('newPW').first.text) response.headers['X-EPP-Returncode'] = '2200' @@ -33,14 +55,12 @@ class Epp::SessionsController < EppController render_epp_response('login_fail') end end + # rubocop: enable Metrics/MethodLength def ip_white? return true if request.ip == ENV['webclient_ip'] if @api_user - unless @api_user.registrar.api_ip_white?(request.ip) - @msg = t('ip_is_not_whitelisted') - return false - end + return false unless @api_user.registrar.api_ip_white?(request.ip) end true end @@ -51,10 +71,7 @@ class Epp::SessionsController < EppController 'registrar_id = ? AND updated_at >= ?', @api_user.registrar_id, Time.zone.now - 5.minutes ).count - if c >= 4 - @msg = t('connection_limit_reached') - return false - end + return false if c >= 4 true end diff --git a/config/locales/en.yml b/config/locales/en.yml index 6864c37c8..ede57ad79 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -807,10 +807,8 @@ en: domain_delete_confirmed_body: 'You have successfully submitted delete confirmation. You will receive registry final confirmation to email.' domain_delete_rejected_title: 'Domain deletion has been rejected successfully' domain_delete_rejected_body: 'You have rejected domain deletion.' - ip_is_not_whitelisted: 'IP is not whitelisted' no_permission: 'No permission' access_denied: 'Access denied' - connection_limit_reached: 'Connection limit reached' common_name: 'Common name' md5: 'Md5' interface: 'Interface' @@ -834,3 +832,4 @@ en: create_bank_statement: 'Create bank statement' create_bank_transaction: 'Create bank transaction' create_new_invoice: 'Create new invoice' + ip_is_not_whitelisted: 'IP is not whitelisted' diff --git a/spec/epp/session_spec.rb b/spec/epp/session_spec.rb index b1ad5d136..e0a7c6dfd 100644 --- a/spec/epp/session_spec.rb +++ b/spec/epp/session_spec.rb @@ -25,7 +25,7 @@ describe 'EPP Session', epp: true do it 'does not log in with invalid user' do wrong_user = @epp_xml.session.login(clID: { value: 'wrong-user' }, pw: { value: 'ghyt9e4fu' }) response = epp_plain_request(wrong_user) - response[:msg].should == 'Authentication error; server closing connection' + response[:msg].should == 'Authentication error; server closing connection (API user not found)' response[:result_code].should == '2501' response[:clTRID].should == 'ABC-12345' end @@ -36,7 +36,7 @@ describe 'EPP Session', epp: true do inactive = @epp_xml.session.login(clID: { value: 'inactive-user' }, pw: { value: 'ghyt9e4fu' }) response = epp_plain_request(inactive) - response[:msg].should == 'Authentication error; server closing connection' + response[:msg].should == 'Authentication error; server closing connection (API user is not active)' response[:result_code].should == '2501' end diff --git a/spec/features/registrar/sessions_spec.rb b/spec/features/registrar/sessions_spec.rb index 5d5f3095a..af68065df 100644 --- a/spec/features/registrar/sessions_spec.rb +++ b/spec/features/registrar/sessions_spec.rb @@ -114,7 +114,7 @@ feature 'Sessions', type: :feature do fill_in 'user_phone', with: '00007' click_button 'Log in' - page.should have_text('Check your phone for confirmation code') + page.should have_text('Confirmation sms was sent to your phone. Verification code is') page.should have_text('SIM application error') end @@ -143,7 +143,7 @@ feature 'Sessions', type: :feature do fill_in 'user_phone', with: '00007' click_button 'Log in' - page.should have_text('Check your phone for confirmation code') + page.should have_text('Confirmation sms was sent to your phone. Verification code is') page.should have_text('Welcome!') end From 5a0fd06e208f0971deb4ef7af8bc9577db248861 Mon Sep 17 00:00:00 2001 From: Martin Lensment Date: Mon, 29 Jun 2015 11:22:33 +0300 Subject: [PATCH 2/3] Fix registrar link in admin domain view #2632 --- app/views/admin/domains/partials/_general.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/domains/partials/_general.haml b/app/views/admin/domains/partials/_general.haml index 53c73c65b..91b4aeabd 100644 --- a/app/views/admin/domains/partials/_general.haml +++ b/app/views/admin/domains/partials/_general.haml @@ -10,7 +10,7 @@ %dd= l(@domain.registered_at) %dt= t(:registrar) - %dd= link_to(@domain.registrar, root_path) + %dd= link_to(@domain.registrar, admin_registrar_path(@domain.registrar)) %dt= t(:password) %dd From ffebff72dd54805238df2a7e6fdc50aab848e166 Mon Sep 17 00:00:00 2001 From: Martin Lensment Date: Mon, 29 Jun 2015 12:31:47 +0300 Subject: [PATCH 3/3] Add feature to disable IP whitelist #2707 --- app/controllers/registrar_controller.rb | 1 - app/models/registrar.rb | 4 +++- app/models/white_ip.rb | 2 ++ app/views/admin/settings/index.haml | 2 ++ config/initializers/initial_settings.rb | 3 +++ spec/features/registrar/sessions_spec.rb | 22 ++++++++++++++++++++++ 6 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/controllers/registrar_controller.rb b/app/controllers/registrar_controller.rb index e1b6b5a3c..37ec99bfd 100644 --- a/app/controllers/registrar_controller.rb +++ b/app/controllers/registrar_controller.rb @@ -1,6 +1,5 @@ class RegistrarController < ApplicationController before_action :authenticate_user!, :check_ip - # before_action :check_ip layout 'registrar/application' include Registrar::ApplicationHelper diff --git a/app/models/registrar.rb b/app/models/registrar.rb index bc4eb3297..2cb375ea6 100644 --- a/app/models/registrar.rb +++ b/app/models/registrar.rb @@ -75,7 +75,7 @@ class Registrar < ActiveRecord::Base # rubocop:disable Metrics/AbcSize # rubocop:disable Metrics/MethodLength - def issue_prepayment_invoice(amount, description = nil) + def issue_prepayment_invoice(amount, description = nil) # Currently only EIS can issue invoices eis = self.class.eis @@ -157,10 +157,12 @@ class Registrar < ActiveRecord::Base end def api_ip_white?(ip) + return true unless Setting.api_ip_whitelist_enabled white_ips.api.pluck(:ipv4, :ipv6).flatten.include?(ip) || global_ip_white?(ip) end def registrar_ip_white?(ip) + return true unless Setting.registrar_ip_whitelist_enabled white_ips.registrar.pluck(:ipv4, :ipv6).flatten.include?(ip) || global_ip_white?(ip) end diff --git a/app/models/white_ip.rb b/app/models/white_ip.rb index f62cb2f9f..d8f9dd7fa 100644 --- a/app/models/white_ip.rb +++ b/app/models/white_ip.rb @@ -24,6 +24,8 @@ class WhiteIp < ActiveRecord::Base class << self def registrar_ip_white?(ip) + return true unless Setting.registrar_ip_whitelist_enabled + at = WhiteIp.arel_table WhiteIp.where( at[:interface].eq(REGISTRAR).or( diff --git a/app/views/admin/settings/index.haml b/app/views/admin/settings/index.haml index cd0e1d9a4..3838e7e5b 100644 --- a/app/views/admin/settings/index.haml +++ b/app/views/admin/settings/index.haml @@ -67,6 +67,8 @@ = render 'setting_row', var: :transfer_wait_time = render 'setting_row', var: :ds_algorithm = render 'setting_row', var: :client_side_status_editing_enabled + = render 'setting_row', var: :api_ip_whitelist_enabled + = render 'setting_row', var: :registrar_ip_whitelist_enabled .row .col-md-12.text-right %button.btn.btn-primary=t(:save) diff --git a/config/initializers/initial_settings.rb b/config/initializers/initial_settings.rb index 3f63f3ecd..b34cb2510 100644 --- a/config/initializers/initial_settings.rb +++ b/config/initializers/initial_settings.rb @@ -34,6 +34,9 @@ if con.present? && con.table_exists?('settings') Setting.save_default(:days_to_renew_domain_before_expire, 90) Setting.save_default(:expire_warning_period, 15) Setting.save_default(:redemption_grace_period, 30) + + Setting.save_default(:registrar_ip_whitelist_enabled, true) + Setting.save_default(:api_ip_whitelist_enabled, true) end # dev only setting diff --git a/spec/features/registrar/sessions_spec.rb b/spec/features/registrar/sessions_spec.rb index af68065df..07db0774a 100644 --- a/spec/features/registrar/sessions_spec.rb +++ b/spec/features/registrar/sessions_spec.rb @@ -8,6 +8,14 @@ feature 'Sessions', type: :feature do page.should have_text('Access denied') end + it 'should see login page when whitelist disabled' do + Setting.registrar_ip_whitelist_enabled = false + WhiteIp.destroy_all + visit registrar_login_path + page.should_not have_text('Access denied') + Setting.registrar_ip_whitelist_enabled = true + end + it 'should see log in' do @fixed_registrar.white_ips = [Fabricate(:white_ip_registrar)] visit registrar_login_path @@ -26,6 +34,20 @@ feature 'Sessions', type: :feature do page.should have_text('Access denied') end + it 'should get in with invalid when whitelist disabled' do + Setting.registrar_ip_whitelist_enabled = false + Fabricate(:registrar, white_ips: [Fabricate(:white_ip), Fabricate(:white_ip_registrar)]) + @api_user_invalid_ip = Fabricate( + :api_user, identity_code: '37810013294', registrar: Fabricate(:registrar, white_ips: []) + ) + visit registrar_login_path + fill_in 'depp_user_tag', with: @api_user_invalid_ip.username + fill_in 'depp_user_password', with: @api_user_invalid_ip.password + click_button 'Log in' + page.should have_text('Log out') + Setting.registrar_ip_whitelist_enabled = true + end + it 'should not get in with invalid user' do visit registrar_login_path fill_in 'depp_user_tag', with: 'bla'