From 1493cb8700d3dfdcc0dfcc98ddbf3dacf421aa87 Mon Sep 17 00:00:00 2001 From: Georg Kahest Date: Wed, 27 Sep 2017 18:16:38 +0300 Subject: [PATCH 01/28] use http code (403) when registrar portal access is denied by ip whitelist, show client ip --- app/controllers/registrar/sessions_controller.rb | 2 +- app/views/registrar/sessions/denied.haml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 app/views/registrar/sessions/denied.haml diff --git a/app/controllers/registrar/sessions_controller.rb b/app/controllers/registrar/sessions_controller.rb index cd33590c2..4209270d8 100644 --- a/app/controllers/registrar/sessions_controller.rb +++ b/app/controllers/registrar/sessions_controller.rb @@ -188,7 +188,7 @@ class Registrar def check_ip return if Rails.env.development? return if WhiteIp.registrar_ip_white?(request.ip) - render text: t('access_denied') and return + render :denied, :layout => false, status: :forbidden, :locals => { :ip => request.ip } and return end end end diff --git a/app/views/registrar/sessions/denied.haml b/app/views/registrar/sessions/denied.haml new file mode 100644 index 000000000..a1aea470b --- /dev/null +++ b/app/views/registrar/sessions/denied.haml @@ -0,0 +1 @@ +#{t('access_denied')} from #{ip} From d7f1aee47becc8276081e928bfef883faba146bb Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 29 Sep 2017 14:53:22 +0300 Subject: [PATCH 02/28] Do not check environment on IP whitelist verification #600 --- app/controllers/registrar/base_controller.rb | 2 +- app/controllers/registrar/sessions_controller.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/registrar/base_controller.rb b/app/controllers/registrar/base_controller.rb index c91f2e075..d2b2edd69 100644 --- a/app/controllers/registrar/base_controller.rb +++ b/app/controllers/registrar/base_controller.rb @@ -16,7 +16,7 @@ class Registrar sign_out(current_user) return end - return if Rails.env.development? + registrar_ip_whitelisted = current_user.registrar.registrar_ip_white?(request.ip) return if registrar_ip_whitelisted diff --git a/app/controllers/registrar/sessions_controller.rb b/app/controllers/registrar/sessions_controller.rb index 4209270d8..a9317e701 100644 --- a/app/controllers/registrar/sessions_controller.rb +++ b/app/controllers/registrar/sessions_controller.rb @@ -186,7 +186,6 @@ class Registrar private def check_ip - return if Rails.env.development? return if WhiteIp.registrar_ip_white?(request.ip) render :denied, :layout => false, status: :forbidden, :locals => { :ip => request.ip } and return end From 48b230f87e6655ddd60e08a1f6a4369e7f89ca81 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 29 Sep 2017 15:35:47 +0300 Subject: [PATCH 03/28] Turn setting "registrar_ip_whitelist_enabled" off in dev env by default #600 --- lib/tasks/dev.rake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/tasks/dev.rake b/lib/tasks/dev.rake index 67cf856a2..470949c14 100644 --- a/lib/tasks/dev.rake +++ b/lib/tasks/dev.rake @@ -175,6 +175,8 @@ namespace :dev do end end + Setting.registrar_ip_whitelist_enabled = false + ActiveRecord::Base.transaction do generate_default_data generate_random_data if with_random_data From e840719b8a37732423366484355775f5dd1515fa Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 29 Sep 2017 17:18:59 +0300 Subject: [PATCH 04/28] Remove unneeded specs #600 --- spec/models/white_ip_spec.rb | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/spec/models/white_ip_spec.rb b/spec/models/white_ip_spec.rb index e824fbdd6..316a6b263 100644 --- a/spec/models/white_ip_spec.rb +++ b/spec/models/white_ip_spec.rb @@ -28,17 +28,6 @@ describe WhiteIp do @white_ip = Fabricate(:white_ip) end - it 'should be valid' do - @white_ip.valid? - @white_ip.errors.full_messages.should match_array([]) - end - - it 'should be valid twice' do - @white_ip = Fabricate(:white_ip) - @white_ip.valid? - @white_ip.errors.full_messages.should match_array([]) - end - it 'should have one version' do with_versioning do @white_ip.versions.should == [] From 687d56c41274819063db4890c4698705c9d428b7 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Fri, 29 Sep 2017 19:08:59 +0300 Subject: [PATCH 05/28] Use factory instead of fabricator for WhiteIp #600 --- spec/fabricators/registrar_fabricator.rb | 1 - spec/fabricators/white_ip_fabricator.rb | 8 -------- spec/factories/white_ip.rb | 5 +++++ spec/models/white_ip_spec.rb | 2 +- 4 files changed, 6 insertions(+), 10 deletions(-) delete mode 100644 spec/fabricators/white_ip_fabricator.rb create mode 100644 spec/factories/white_ip.rb diff --git a/spec/fabricators/registrar_fabricator.rb b/spec/fabricators/registrar_fabricator.rb index f3a3fdc59..37236cb87 100644 --- a/spec/fabricators/registrar_fabricator.rb +++ b/spec/fabricators/registrar_fabricator.rb @@ -9,7 +9,6 @@ Fabricator(:registrar) do country_code 'EE' code { sequence(:code) { |i| "REGISTRAR#{i}" } } reference_no { sequence(:reference_no) { |i| "RF#{i}" } } - white_ips { [Fabricate(:white_ip), Fabricate(:white_ip, interfaces: [WhiteIp::REGISTRAR])] } end Fabricator(:registrar_with_no_account_activities, from: :registrar) do diff --git a/spec/fabricators/white_ip_fabricator.rb b/spec/fabricators/white_ip_fabricator.rb deleted file mode 100644 index 6eb574893..000000000 --- a/spec/fabricators/white_ip_fabricator.rb +++ /dev/null @@ -1,8 +0,0 @@ -Fabricator(:white_ip) do - ipv4 '127.0.0.1' - interfaces [WhiteIp::API] -end - -Fabricator(:white_ip_registrar, from: :white_ip) do - interfaces [WhiteIp::REGISTRAR] -end diff --git a/spec/factories/white_ip.rb b/spec/factories/white_ip.rb new file mode 100644 index 000000000..eb4033fc9 --- /dev/null +++ b/spec/factories/white_ip.rb @@ -0,0 +1,5 @@ +FactoryGirl.define do + factory :white_ip do + ipv4 '127.0.0.1' + end +end diff --git a/spec/models/white_ip_spec.rb b/spec/models/white_ip_spec.rb index 316a6b263..931431c1e 100644 --- a/spec/models/white_ip_spec.rb +++ b/spec/models/white_ip_spec.rb @@ -25,7 +25,7 @@ describe WhiteIp do context 'with valid attributes' do before :all do - @white_ip = Fabricate(:white_ip) + @white_ip = create(:white_ip) end it 'should have one version' do From 92b125b4a7fd1b4cc7535dd022d58b58a75ac2cf Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 2 Oct 2017 08:54:04 +0300 Subject: [PATCH 06/28] Include "factory_girl_rails" gem in development #600 --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 0b4c4abc0..2f7587fc9 100644 --- a/Gemfile +++ b/Gemfile @@ -116,6 +116,7 @@ group :development do end group :development, :test do + gem 'factory_girl_rails' gem 'capybara' gem 'rspec-rails', '~> 3.6' gem 'fabrication', '2.13.2' # Replacement for fixtures @@ -144,7 +145,6 @@ end group :test do gem 'database_cleaner' - gem 'factory_girl_rails' gem 'codeclimate-test-reporter', "~> 1.0.0" gem 'simplecov' gem 'webmock' From c1875ec88e11c33286fa76b0b091f2697a8e3e8c Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 2 Oct 2017 08:55:19 +0300 Subject: [PATCH 07/28] Remove unused code #600 --- app/helpers/registrant/application_helper.rb | 7 ------- app/helpers/registrar/application_helper.rb | 7 ------- 2 files changed, 14 deletions(-) diff --git a/app/helpers/registrant/application_helper.rb b/app/helpers/registrant/application_helper.rb index c1b1de6fb..6451f91a2 100644 --- a/app/helpers/registrant/application_helper.rb +++ b/app/helpers/registrant/application_helper.rb @@ -3,11 +3,4 @@ module Registrant::ApplicationHelper return '' if unstable_env.nil? "background-image: url(#{image_path("registrar/bg-#{unstable_env}.png")});" end - - def pagination_details - params[:page] ||= 1 - limit = ENV['depp_records_on_page'] || 20 - offset = ((params[:page].to_i - 1) * limit.to_i) - [limit, offset] - end end diff --git a/app/helpers/registrar/application_helper.rb b/app/helpers/registrar/application_helper.rb index 920260c4b..7710cf55c 100644 --- a/app/helpers/registrar/application_helper.rb +++ b/app/helpers/registrar/application_helper.rb @@ -3,11 +3,4 @@ module Registrar::ApplicationHelper return '' if unstable_env.nil? "background-image: url(#{image_path("registrar/bg-#{unstable_env}.png")});" end - - def pagination_details - params[:page] ||= 1 - limit = ENV['depp_records_on_page'] || 20 - offset = ((params[:page].to_i - 1) * limit.to_i) - [limit, offset] - end end From 787cca8e4cf320d718f58755613b2424c59e9f89 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 01:00:24 +0300 Subject: [PATCH 08/28] Extract Registrar::CurrentUserController from SessionsController #600 --- .../registrar/current_user_controller.rb | 12 ++++ .../registrar/sessions_controller.rb | 21 ------- app/models/api_user.rb | 11 ++-- app/views/registrar/base/_navbar.haml | 5 +- config/locales/registrar/current_user.en.yml | 5 ++ config/routes.rb | 2 + .../registrar/current_user/switch_spec.rb | 16 ++++++ spec/models/api_user_spec.rb | 55 +++++++++++++------ spec/requests/registrar/current_user_spec.rb | 53 ++++++++++++++++++ 9 files changed, 134 insertions(+), 46 deletions(-) create mode 100644 app/controllers/registrar/current_user_controller.rb create mode 100644 config/locales/registrar/current_user.en.yml create mode 100644 spec/features/registrar/current_user/switch_spec.rb create mode 100644 spec/requests/registrar/current_user_spec.rb diff --git a/app/controllers/registrar/current_user_controller.rb b/app/controllers/registrar/current_user_controller.rb new file mode 100644 index 000000000..4661318e7 --- /dev/null +++ b/app/controllers/registrar/current_user_controller.rb @@ -0,0 +1,12 @@ +class Registrar + class CurrentUserController < BaseController + skip_authorization_check + + def switch + new_user = ApiUser.find(params[:new_user_id]) + sign_in(new_user) if new_user.identity_code == current_user.identity_code + + redirect_to :back, notice: t('.switched', new_user: new_user) + end + end +end diff --git a/app/controllers/registrar/sessions_controller.rb b/app/controllers/registrar/sessions_controller.rb index a9317e701..6bf31b03a 100644 --- a/app/controllers/registrar/sessions_controller.rb +++ b/app/controllers/registrar/sessions_controller.rb @@ -61,27 +61,6 @@ class Registrar end end - # rubocop:enable Metrics/MethodLength - # rubocop:enable Metrics/AbcSize - - def switch_user - @api_user = ApiUser.find(params[:id]) - - unless Rails.env.development? - unless @api_user.registrar.registrar_ip_white?(request.ip) - flash[:alert] = I18n.t(:ip_is_not_whitelisted) - redirect_to :back and return - end - end - - sign_in @api_user if @api_user.identity_code == current_user.identity_code - - redirect_to registrar_root_url - end - - # rubocop:enable Metrics/CyclomaticComplexity - # rubocop:enable Metrics/PerceivedComplexity - def id @user = ApiUser.find_by_idc_data(request.env['SSL_CLIENT_S_DN']) diff --git a/app/models/api_user.rb b/app/models/api_user.rb index 6928a933f..a5030c096 100644 --- a/app/models/api_user.rb +++ b/app/models/api_user.rb @@ -51,11 +51,6 @@ class ApiUser < User find_by(identity_code: identity_code) end - - def all_by_identity_code(identity_code) - ApiUser.where(identity_code: identity_code) - .where("identity_code is NOT NULL and identity_code != ''").includes(:registrar) - end end def registrar_typeahead @@ -93,4 +88,10 @@ class ApiUser < User md5 = OpenSSL::Digest::MD5.new(cert.to_der).to_s certificates.api.exists?(md5: md5, common_name: cn) end + + def linked_users + self.class.where(identity_code: identity_code) + .where("identity_code is NOT NULL and identity_code != ''") + .where.not(id: id) + end end diff --git a/app/views/registrar/base/_navbar.haml b/app/views/registrar/base/_navbar.haml index 608fb9cb4..160b455ed 100644 --- a/app/views/registrar/base/_navbar.haml +++ b/app/views/registrar/base/_navbar.haml @@ -22,7 +22,8 @@ = "#{current_user} (#{current_user.roles.first}) - #{current_user.registrar}" %span.caret %ul.dropdown-menu{role: "menu"} - - ApiUser.all_by_identity_code(current_user.identity_code).each do |x| - %li= link_to "#{x} (#{x.roles.first}) - #{x.registrar}", "/registrar/switch_user/#{x.id}" + - current_user.linked_users.each do |user| + %li= link_to "#{user} (#{user.roles.first}) - #{user.registrar}", registrar_switch_current_user_path(user), + id: "switch-current-user-#{user.id}-btn" - if user_signed_in? %li= link_to t(:log_out_), '/registrar/logout' diff --git a/config/locales/registrar/current_user.en.yml b/config/locales/registrar/current_user.en.yml new file mode 100644 index 000000000..5f25cd81f --- /dev/null +++ b/config/locales/registrar/current_user.en.yml @@ -0,0 +1,5 @@ +en: + registrar: + current_user: + switch: + switched: You are now signed in as a user "%{new_user}" diff --git a/config/routes.rb b/config/routes.rb index e9fb61989..e19028a68 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -47,6 +47,8 @@ Rails.application.routes.draw do get 'logout' => '/devise/sessions#destroy' end + get 'current_user/switch/:new_user_id', to: 'current_user#switch', as: :switch_current_user + resources :domains do collection do post 'update', as: 'update' diff --git a/spec/features/registrar/current_user/switch_spec.rb b/spec/features/registrar/current_user/switch_spec.rb new file mode 100644 index 000000000..a4c19bcba --- /dev/null +++ b/spec/features/registrar/current_user/switch_spec.rb @@ -0,0 +1,16 @@ +require 'rails_helper' + +RSpec.feature 'Registrar area current user switch', settings: false do + given!(:current_user) { create(:api_user, id: 1, identity_code: 'test') } + given!(:new_user) { create(:api_user, id: 2, identity_code: 'test', username: 'new-user-name') } + + background do + sign_in_to_registrar_area(user: current_user) + end + + it 'switches current user' do + visit registrar_root_path + click_link_or_button 'switch-current-user-2-btn' + expect(page).to have_text('You are now signed in as a user "new-user-name"') + end +end diff --git a/spec/models/api_user_spec.rb b/spec/models/api_user_spec.rb index 20ac4afff..fc5388b8c 100644 --- a/spec/models/api_user_spec.rb +++ b/spec/models/api_user_spec.rb @@ -1,24 +1,8 @@ require 'rails_helper' RSpec.describe ApiUser do - context 'class methods' do - before do - Fabricate(:api_user, identity_code: '') - Fabricate(:api_user, identity_code: 14212128025) - end - - it 'should return all api users with given identity code' do - ApiUser.all_by_identity_code('14212128025').size.should == 1 - ApiUser.all_by_identity_code(14212128025).size.should == 1 - end - - it 'should not return any api user with blank identity code' do - ApiUser.all_by_identity_code('').size.should == 0 - end - end - context 'with invalid attribute' do - before :all do + before do @api_user = ApiUser.new end @@ -43,7 +27,7 @@ RSpec.describe ApiUser do end context 'with valid attributes' do - before :all do + before do @api_user = Fabricate(:api_user) end @@ -74,4 +58,39 @@ RSpec.describe ApiUser do expect(described_class.min_password_length).to eq(6) end end + + describe '#linked_users' do + it 'returns users with the same identity code' do + api_user = create(:api_user, id: 1, identity_code: 'test') + create(:api_user, id: 2, identity_code: 'test') + + expect(api_user.linked_users.ids).to include(2) + end + + it 'does not return users with another identity code' do + api_user = create(:api_user, id: 1, identity_code: 'test') + create(:api_user, id: 2, identity_code: 'another') + + expect(api_user.linked_users.ids).to_not include(2) + end + + it 'does not return itself' do + api_user = create(:api_user) + expect(api_user.linked_users).to be_empty + end + + it 'returns none if identity code is absent' do + api_user = create(:api_user, identity_code: nil) + create(:api_user, identity_code: nil) + + expect(api_user.linked_users).to be_empty + end + + it 'returns none if identity code is empty' do + api_user = create(:api_user, identity_code: '') + create(:api_user, identity_code: '') + + expect(api_user.linked_users).to be_empty + end + end end diff --git a/spec/requests/registrar/current_user_spec.rb b/spec/requests/registrar/current_user_spec.rb new file mode 100644 index 000000000..77d69c76b --- /dev/null +++ b/spec/requests/registrar/current_user_spec.rb @@ -0,0 +1,53 @@ +require 'rails_helper' + +RSpec.describe 'Registrar current user', db: false do + describe 'GET /registrar/current_user/switch/2' do + context 'when user is authenticated', db: true do + let!(:current_user) { create(:api_user, id: 1, identity_code: 'test') } + let!(:new_user) { create(:api_user, id: 2, identity_code: 'test') } + + before do + sign_in_to_registrar_area(user: current_user) + end + + context 'when ip is allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area?: true) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + specify do + make_request + expect(response).to redirect_to('http://previous.url') + end + end + + context 'when ip is not allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area?: false) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + specify do + make_request + expect(response).to redirect_to(registrar_login_url) + end + end + end + + context 'when user is not authenticated' do + specify do + make_request + expect(response).to redirect_to(registrar_login_url) + end + end + + def make_request + get '/registrar/current_user/switch/2', nil, { 'HTTP_REFERER' => 'http://previous.url' } + end + end +end From 35afbf1f8ce923b3bb5d6a354e7d616fa07108cb Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 01:03:32 +0300 Subject: [PATCH 09/28] Refactor IP registrar restriction #600 --- app/api/repp/api.rb | 2 +- app/controllers/registrar/base_controller.rb | 49 +++++----- .../registrar/sessions_controller.rb | 23 +++-- app/models/authorization/restricted_ip.rb | 25 +++++ app/models/registrar.rb | 5 - app/models/white_ip.rb | 7 +- app/views/registrar/sessions/denied.haml | 1 - config/locales/api/authorization.en.yml | 4 + config/locales/en.yml | 2 - config/locales/registrar/authorization.en.yml | 4 + doc/controllers_complete.svg | 5 - spec/features/registrar/sessions/new_spec.rb | 42 +++++++++ .../authorization/restricted_ip_spec.rb | 94 +++++++++++++++++++ spec/models/white_ip_spec.rb | 28 ++++++ spec/requests/registrar/sessions_spec.rb | 67 +++++++++++++ 15 files changed, 304 insertions(+), 54 deletions(-) create mode 100644 app/models/authorization/restricted_ip.rb delete mode 100644 app/views/registrar/sessions/denied.haml create mode 100644 config/locales/api/authorization.en.yml create mode 100644 config/locales/registrar/authorization.en.yml create mode 100644 spec/features/registrar/sessions/new_spec.rb create mode 100644 spec/models/authorization/restricted_ip_spec.rb create mode 100644 spec/requests/registrar/sessions_spec.rb diff --git a/app/api/repp/api.rb b/app/api/repp/api.rb index 27d0322b0..394edfdad 100644 --- a/app/api/repp/api.rb +++ b/app/api/repp/api.rb @@ -15,7 +15,7 @@ module Repp before do webclient_request = ENV['webclient_ips'].split(',').map(&:strip).include?(request.ip) unless webclient_request - error! I18n.t('ip_is_not_whitelisted'), 401 unless @current_user.registrar.api_ip_white?(request.ip) + error! I18n.t('api.authorization.ip_not_allowed', ip: request.ip), 401 unless @current_user.registrar.api_ip_white?(request.ip) end if @current_user.cannot?(:view, :repp) diff --git a/app/controllers/registrar/base_controller.rb b/app/controllers/registrar/base_controller.rb index d2b2edd69..0455b2a2e 100644 --- a/app/controllers/registrar/base_controller.rb +++ b/app/controllers/registrar/base_controller.rb @@ -1,40 +1,37 @@ class Registrar class BaseController < ApplicationController - before_action :authenticate_user!, :check_ip - include Registrar::ApplicationHelper + before_action :authenticate_user! + before_action :check_ip_restriction helper_method :depp_controller? - - def depp_controller? - false - end - - def check_ip - return unless current_user - unless current_user.is_a? ApiUser - sign_out(current_user) - return - end - - registrar_ip_whitelisted = current_user.registrar.registrar_ip_white?(request.ip) - - return if registrar_ip_whitelisted - flash[:alert] = t('ip_is_not_whitelisted') - sign_out(current_user) - redirect_to registrar_login_path and return - end - helper_method :head_title_sufix - def head_title_sufix - t(:registrar_head_title_sufix) - end - protected def current_ability @current_ability ||= Ability.new(current_user, request.remote_ip) end + + private + + def check_ip_restriction + ip_restriction = Authorization::RestrictedIP.new(request.ip) + allowed = ip_restriction.can_access_registrar_area?(current_user.registrar) + + unless allowed + flash[:alert] = t('registrar.authorization.ip_not_allowed', ip: request.ip) + sign_out current_user + redirect_to registrar_login_url + end + end + + def depp_controller? + false + end + + def head_title_sufix + t(:registrar_head_title_sufix) + end end end diff --git a/app/controllers/registrar/sessions_controller.rb b/app/controllers/registrar/sessions_controller.rb index 6bf31b03a..9f6189c71 100644 --- a/app/controllers/registrar/sessions_controller.rb +++ b/app/controllers/registrar/sessions_controller.rb @@ -1,13 +1,8 @@ class Registrar class SessionsController < Devise::SessionsController + before_action :check_ip_restriction helper_method :depp_controller? - def depp_controller? - false - end - - before_action :check_ip - def login @depp_user = Depp::User.new end @@ -157,16 +152,24 @@ class Registrar # rubocop: enable Metrics/CyclomaticComplexity # rubocop: enable Metrics/MethodLength + private + + def depp_controller? + false + end + def find_user_by_idc(idc) return User.new unless idc ApiUser.find_by(identity_code: idc) || User.new end - private + def check_ip_restriction + ip_restriction = Authorization::RestrictedIP.new(request.ip) + allowed = ip_restriction.can_access_registrar_area_sign_in_page? - def check_ip - return if WhiteIp.registrar_ip_white?(request.ip) - render :denied, :layout => false, status: :forbidden, :locals => { :ip => request.ip } and return + unless allowed + render text: t('registrar.authorization.ip_not_allowed', ip: request.ip), status: :forbidden + end end end end diff --git a/app/models/authorization/restricted_ip.rb b/app/models/authorization/restricted_ip.rb new file mode 100644 index 000000000..b3c7b7cdb --- /dev/null +++ b/app/models/authorization/restricted_ip.rb @@ -0,0 +1,25 @@ +module Authorization + class RestrictedIP + def initialize(ip) + @ip = ip + end + + def self.enabled? + Setting.registrar_ip_whitelist_enabled + end + + def can_access_registrar_area?(registrar) + return true unless self.class.enabled? + registrar.white_ips.registrar_area.include_ip?(ip) + end + + def can_access_registrar_area_sign_in_page? + return true unless self.class.enabled? + WhiteIp.registrar_area.include_ip?(ip) + end + + private + + attr_reader :ip + end +end diff --git a/app/models/registrar.rb b/app/models/registrar.rb index 734e68898..b906beee3 100644 --- a/app/models/registrar.rb +++ b/app/models/registrar.rb @@ -162,9 +162,4 @@ class Registrar < ActiveRecord::Base return true unless Setting.api_ip_whitelist_enabled white_ips.api.pluck(:ipv4, :ipv6).flatten.include?(ip) end - - def registrar_ip_white?(ip) - return true unless Setting.registrar_ip_whitelist_enabled - white_ips.registrar.pluck(:ipv4, :ipv6).flatten.include?(ip) - end end diff --git a/app/models/white_ip.rb b/app/models/white_ip.rb index 918315004..f11606610 100644 --- a/app/models/white_ip.rb +++ b/app/models/white_ip.rb @@ -18,16 +18,15 @@ class WhiteIp < ActiveRecord::Base INTERFACES = [API, REGISTRAR] scope :api, -> { where("interfaces @> ?::varchar[]", "{#{API}}") } - scope :registrar, -> { where("interfaces @> ?::varchar[]", "{#{REGISTRAR}}") } + scope :registrar_area, -> { where("interfaces @> ?::varchar[]", "{#{REGISTRAR}}") } def interfaces=(interfaces) super(interfaces.reject(&:blank?)) end class << self - def registrar_ip_white?(ip) - return true unless Setting.registrar_ip_whitelist_enabled - WhiteIp.where(ipv4: ip).registrar.any? + def include_ip?(ip) + where("#{table_name}.ipv4 = '#{ip}' OR #{table_name}.ipv6 = '#{ip}'").any? end end end diff --git a/app/views/registrar/sessions/denied.haml b/app/views/registrar/sessions/denied.haml deleted file mode 100644 index a1aea470b..000000000 --- a/app/views/registrar/sessions/denied.haml +++ /dev/null @@ -1 +0,0 @@ -#{t('access_denied')} from #{ip} diff --git a/config/locales/api/authorization.en.yml b/config/locales/api/authorization.en.yml new file mode 100644 index 000000000..53c49d146 --- /dev/null +++ b/config/locales/api/authorization.en.yml @@ -0,0 +1,4 @@ +en: + api: + authorization: + ip_not_allowed: Access denied from IP %{ip} diff --git a/config/locales/en.yml b/config/locales/en.yml index 2627b112a..a9b54c210 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -797,7 +797,6 @@ en: domain_delete_rejected_title: 'Domain deletion rejection has been received successfully' domain_delete_rejected_body: 'You have rejected pending domain deletion. You will receive confirmation by email.' no_permission: 'No permission' - access_denied: 'Access denied' common_name: 'Common name' md5: 'Md5' interface: 'Interface' @@ -815,7 +814,6 @@ 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' billing_settings: 'Billing settings' registry_settings: 'Registry settings' registry_billing_email: 'Billing e-mail' diff --git a/config/locales/registrar/authorization.en.yml b/config/locales/registrar/authorization.en.yml new file mode 100644 index 000000000..1e4395cb3 --- /dev/null +++ b/config/locales/registrar/authorization.en.yml @@ -0,0 +1,4 @@ +en: + registrar: + authorization: + ip_not_allowed: Access denied from IP %{ip} diff --git a/doc/controllers_complete.svg b/doc/controllers_complete.svg index f48433540..1c9bc94ec 100644 --- a/doc/controllers_complete.svg +++ b/doc/controllers_complete.svg @@ -136,9 +136,6 @@ RegistrarController -check_ip -depp_controller? -head_title_sufix _layout @@ -491,8 +488,6 @@ Registrar::SessionsController create -depp_controller? -find_user_by_idc id login login_mid diff --git a/spec/features/registrar/sessions/new_spec.rb b/spec/features/registrar/sessions/new_spec.rb new file mode 100644 index 000000000..feec6cae1 --- /dev/null +++ b/spec/features/registrar/sessions/new_spec.rb @@ -0,0 +1,42 @@ +require 'rails_helper' + +RSpec.feature 'Registrar area ip restriction', settings: false do + context 'when enabled' do + background do + Setting.registrar_ip_whitelist_enabled = true + end + + context 'when ip is allowed' do + given!(:white_ip) { create(:white_ip, + ipv4: '127.0.0.1', + interfaces: [WhiteIp::REGISTRAR]) } + + it 'does not show error message' do + visit registrar_login_path + expect(page).to_not have_text(error_message) + end + end + + context 'when ip is not allowed' do + it 'shows error message' do + visit registrar_login_path + expect(page).to have_text(error_message) + end + end + end + + context 'when disabled' do + background do + Setting.registrar_ip_whitelist_enabled = false + end + + it 'does not show error message' do + visit registrar_login_path + expect(page).to_not have_text(error_message) + end + end + + def error_message + t('registrar.authorization.ip_not_allowed', ip: '127.0.0.1') + end +end diff --git a/spec/models/authorization/restricted_ip_spec.rb b/spec/models/authorization/restricted_ip_spec.rb new file mode 100644 index 000000000..6fba76657 --- /dev/null +++ b/spec/models/authorization/restricted_ip_spec.rb @@ -0,0 +1,94 @@ +require 'rails_helper' + +RSpec.describe Authorization::RestrictedIP do + describe '#enabled?', db: true, settings: false do + context 'when "registrar_ip_whitelist_enabled" is true' do + before do + Setting.registrar_ip_whitelist_enabled = true + end + + specify do + expect(described_class).to be_enabled + end + end + + context 'when "registrar_ip_whitelist_enabled" is false' do + before do + Setting.registrar_ip_whitelist_enabled = false + end + + specify do + expect(described_class).to_not be_enabled + end + end + end + + describe '#can_access_registrar_area?', db: true do + let(:registrar) { create(:registrar) } + subject(:allowed) { described_class.new('127.0.0.1').can_access_registrar_area?(registrar) } + + context 'when enabled' do + before do + allow(described_class).to receive(:enabled?).and_return(true) + end + + context 'when ip is whitelisted', db: true do + let!(:white_ip) { create(:white_ip, registrar: registrar, ipv4: '127.0.0.1', interfaces: [WhiteIp::REGISTRAR]) } + + specify do + expect(allowed).to be true + end + end + + context 'when ip is not whitelisted' do + specify do + expect(allowed).to be false + end + end + end + + context 'when disabled' do + before do + allow(described_class).to receive(:enabled?).and_return(false) + end + + specify do + expect(allowed).to be true + end + end + end + + describe '#can_access_registrar_area_sign_in_page?' do + subject(:allowed) { described_class.new('127.0.0.1').can_access_registrar_area_sign_in_page? } + + context 'when enabled' do + before do + allow(described_class).to receive(:enabled?).and_return(true) + end + + context 'when ip is whitelisted', db: true do + let!(:white_ip) { create(:white_ip, ipv4: '127.0.0.1', interfaces: [WhiteIp::REGISTRAR]) } + + specify do + expect(allowed).to be true + end + end + + context 'when ip is not whitelisted' do + specify do + expect(allowed).to be false + end + end + end + + context 'when disabled' do + before do + allow(described_class).to receive(:enabled?).and_return(false) + end + + specify do + expect(allowed).to be true + end + end + end +end diff --git a/spec/models/white_ip_spec.rb b/spec/models/white_ip_spec.rb index 931431c1e..bb286c0ca 100644 --- a/spec/models/white_ip_spec.rb +++ b/spec/models/white_ip_spec.rb @@ -38,4 +38,32 @@ describe WhiteIp do end end end + + describe '#include_ip?' do + context 'when given ip v4 exists' do + before do + create(:white_ip, ipv4: '127.0.0.1') + end + + specify do + expect(described_class.include_ip?('127.0.0.1')).to be true + end + end + + context 'when given ip v6 exists' do + before do + create(:white_ip, ipv6: '::1') + end + + specify do + expect(described_class.include_ip?('::1')).to be true + end + end + + context 'when given ip does not exists', db: false do + specify do + expect(described_class.include_ip?('127.0.0.1')).to be false + end + end + end end diff --git a/spec/requests/registrar/sessions_spec.rb b/spec/requests/registrar/sessions_spec.rb new file mode 100644 index 000000000..674cd8f0c --- /dev/null +++ b/spec/requests/registrar/sessions_spec.rb @@ -0,0 +1,67 @@ +require 'rails_helper' + +RSpec.describe 'Registrar session management', db: false do + describe 'GET /registrar/login' do + context 'when ip is allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area_sign_in_page?: true) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + specify do + get registrar_login_path + expect(response).to be_success + end + end + + context 'when ip is not allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area_sign_in_page?: false) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + specify do + get registrar_login_path + expect(response).to be_forbidden + end + end + end + + describe 'POST /registrar/sessions' do + context 'when ip is allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area_sign_in_page?: true) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + specify do + make_request + expect(response).to be_success + end + end + + context 'when ip is not allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area_sign_in_page?: false) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + specify do + make_request + expect(response).to be_forbidden + end + end + + def make_request + post registrar_sessions_path, depp_user: { tag: 'test', password: 'test' } + end + end +end From c8e3839f0469d68ce7433b14742f5e7e0c867d13 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 01:04:49 +0300 Subject: [PATCH 10/28] Clean up #600 --- app/views/layouts/registrar/base.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/registrar/base.haml b/app/views/layouts/registrar/base.haml index a2add9f20..c58e271aa 100644 --- a/app/views/layouts/registrar/base.haml +++ b/app/views/layouts/registrar/base.haml @@ -24,7 +24,7 @@ %span.icon-bar %span.icon-bar %span.icon-bar - = link_to main_app.registrar_root_path, class: 'navbar-brand' do + = link_to registrar_root_path, class: 'navbar-brand' do = t(:registrar_head_title) - if unstable_env.present? .text-center From a82625c6359f8738eb9bca1781e7ce26a20d00fd Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 01:07:19 +0300 Subject: [PATCH 11/28] Remove rubocop tags #600 --- app/controllers/registrar/sessions_controller.rb | 14 -------------- app/models/white_ip.rb | 2 -- 2 files changed, 16 deletions(-) diff --git a/app/controllers/registrar/sessions_controller.rb b/app/controllers/registrar/sessions_controller.rb index 9f6189c71..8d6692350 100644 --- a/app/controllers/registrar/sessions_controller.rb +++ b/app/controllers/registrar/sessions_controller.rb @@ -7,10 +7,6 @@ class Registrar @depp_user = Depp::User.new end - # rubocop:disable Metrics/PerceivedComplexity - # rubocop:disable Metrics/CyclomaticComplexity - # rubocop:disable Metrics/MethodLength - # rubocop:disable Metrics/AbcSize def create @depp_user = Depp::User.new(params[:depp_user].merge(pki: !(Rails.env.development? || Rails.env.test?))) @@ -72,7 +68,6 @@ class Registrar @user = User.new end - # rubocop:disable Metrics/MethodLength def mid phone = params[:user][:phone] endpoint = "#{ENV['sk_digi_doc_service_endpoint']}" @@ -106,11 +101,6 @@ class Registrar end end - # rubocop:enable Metrics/MethodLength - - # rubocop: disable Metrics/AbcSize - # rubocop: disable Metrics/CyclomaticComplexity - # rubocop: disable Metrics/MethodLength def mid_status endpoint = "#{ENV['sk_digi_doc_service_endpoint']}" client = Digidoc::Client.new(endpoint) @@ -148,10 +138,6 @@ class Registrar end end - # rubocop: enable Metrics/AbcSize - # rubocop: enable Metrics/CyclomaticComplexity - # rubocop: enable Metrics/MethodLength - private def depp_controller? diff --git a/app/models/white_ip.rb b/app/models/white_ip.rb index f11606610..80ba14506 100644 --- a/app/models/white_ip.rb +++ b/app/models/white_ip.rb @@ -2,10 +2,8 @@ class WhiteIp < ActiveRecord::Base include Versions belongs_to :registrar - # rubocop: disable Metrics/LineLength validates :ipv4, format: { with: /\A(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\z/, allow_blank: true } validates :ipv6, format: { with: /(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))/, allow_blank: true } - # rubocop: enable Metrics/LineLength validate :validate_ipv4_and_ipv6 def validate_ipv4_and_ipv6 From c6bd590b384e584d4958ffb7ff6c97db002ab177 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 01:37:03 +0300 Subject: [PATCH 12/28] Fix SQL injection #600 --- app/models/white_ip.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/white_ip.rb b/app/models/white_ip.rb index 80ba14506..251fc60ac 100644 --- a/app/models/white_ip.rb +++ b/app/models/white_ip.rb @@ -24,7 +24,7 @@ class WhiteIp < ActiveRecord::Base class << self def include_ip?(ip) - where("#{table_name}.ipv4 = '#{ip}' OR #{table_name}.ipv6 = '#{ip}'").any? + where('ipv4 = :ip OR ipv6 = :ip', ip: ip).any? end end end From ff466e57ac6db9a62609fb0c75ab9909765bcc13 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 01:40:41 +0300 Subject: [PATCH 13/28] Use a guard clause instead of condition #600 --- app/controllers/registrar/base_controller.rb | 10 +++++----- app/controllers/registrar/sessions_controller.rb | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/registrar/base_controller.rb b/app/controllers/registrar/base_controller.rb index 0455b2a2e..2a0d0a7aa 100644 --- a/app/controllers/registrar/base_controller.rb +++ b/app/controllers/registrar/base_controller.rb @@ -19,11 +19,11 @@ class Registrar ip_restriction = Authorization::RestrictedIP.new(request.ip) allowed = ip_restriction.can_access_registrar_area?(current_user.registrar) - unless allowed - flash[:alert] = t('registrar.authorization.ip_not_allowed', ip: request.ip) - sign_out current_user - redirect_to registrar_login_url - end + return if allowed + + flash[:alert] = t('registrar.authorization.ip_not_allowed', ip: request.ip) + sign_out current_user + redirect_to registrar_login_url end def depp_controller? diff --git a/app/controllers/registrar/sessions_controller.rb b/app/controllers/registrar/sessions_controller.rb index 8d6692350..8f9c71b1b 100644 --- a/app/controllers/registrar/sessions_controller.rb +++ b/app/controllers/registrar/sessions_controller.rb @@ -153,9 +153,9 @@ class Registrar ip_restriction = Authorization::RestrictedIP.new(request.ip) allowed = ip_restriction.can_access_registrar_area_sign_in_page? - unless allowed - render text: t('registrar.authorization.ip_not_allowed', ip: request.ip), status: :forbidden - end + return if allowed + + render text: t('registrar.authorization.ip_not_allowed', ip: request.ip), status: :forbidden end end end From 4d41f0ef14df6d2477ebb3b861d33fd2909d9836 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 01:56:05 +0300 Subject: [PATCH 14/28] Disable reek IrresponsibleModule check #600 --- .codeclimate.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.codeclimate.yml b/.codeclimate.yml index 871a1abe5..03f849185 100644 --- a/.codeclimate.yml +++ b/.codeclimate.yml @@ -29,6 +29,9 @@ engines: enabled: true reek: enabled: true + checks: + IrresponsibleModule: + enabled: false ratings: paths: - Gemfile.lock From d568cc09ae69de260a0828d0c41d87ad280ce5e8 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 20:29:08 +0300 Subject: [PATCH 15/28] Improve test readability #600 --- spec/requests/registrar/current_user_spec.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/spec/requests/registrar/current_user_spec.rb b/spec/requests/registrar/current_user_spec.rb index 77d69c76b..0d5f22eb0 100644 --- a/spec/requests/registrar/current_user_spec.rb +++ b/spec/requests/registrar/current_user_spec.rb @@ -19,7 +19,7 @@ RSpec.describe 'Registrar current user', db: false do end specify do - make_request + get '/registrar/current_user/switch/2', nil, { HTTP_REFERER: 'http://previous.url' } expect(response).to redirect_to('http://previous.url') end end @@ -33,7 +33,7 @@ RSpec.describe 'Registrar current user', db: false do end specify do - make_request + get '/registrar/current_user/switch/2' expect(response).to redirect_to(registrar_login_url) end end @@ -41,13 +41,9 @@ RSpec.describe 'Registrar current user', db: false do context 'when user is not authenticated' do specify do - make_request + get '/registrar/current_user/switch/2' expect(response).to redirect_to(registrar_login_url) end end - - def make_request - get '/registrar/current_user/switch/2', nil, { 'HTTP_REFERER' => 'http://previous.url' } - end end end From e6f6d24169e5cf3ac5bf525c7ec47055f90a2089 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Wed, 4 Oct 2017 20:39:40 +0300 Subject: [PATCH 16/28] Improve test readability #600 --- .../{current_user/switch_spec.rb => user_switch_spec.rb} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename spec/features/registrar/{current_user/switch_spec.rb => user_switch_spec.rb} (81%) diff --git a/spec/features/registrar/current_user/switch_spec.rb b/spec/features/registrar/user_switch_spec.rb similarity index 81% rename from spec/features/registrar/current_user/switch_spec.rb rename to spec/features/registrar/user_switch_spec.rb index a4c19bcba..0927a908a 100644 --- a/spec/features/registrar/current_user/switch_spec.rb +++ b/spec/features/registrar/user_switch_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -RSpec.feature 'Registrar area current user switch', settings: false do +RSpec.feature 'Registrar area user switch', settings: false do given!(:current_user) { create(:api_user, id: 1, identity_code: 'test') } given!(:new_user) { create(:api_user, id: 2, identity_code: 'test', username: 'new-user-name') } @@ -8,7 +8,7 @@ RSpec.feature 'Registrar area current user switch', settings: false do sign_in_to_registrar_area(user: current_user) end - it 'switches current user' do + scenario 'successful user switch' do visit registrar_root_path click_link_or_button 'switch-current-user-2-btn' expect(page).to have_text('You are now signed in as a user "new-user-name"') From c257983cdd40edc2ddf4d283f9c157e6468988bc Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 04:43:37 +0300 Subject: [PATCH 17/28] Use DELETE when signing out from registrar area #599 --- .../registrar/base/_current_user.html.erb | 3 +++ app/views/registrar/base/_navbar.haml | 13 ++---------- config/locales/en.yml | 1 - config/locales/registrar/base.en.yml | 5 +++++ config/routes.rb | 2 +- spec/features/registrar/sign_out_spec.rb | 15 ++++++++++++++ spec/requests/registrar/sign_out_spec.rb | 20 +++++++++++++++++++ 7 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 app/views/registrar/base/_current_user.html.erb create mode 100644 config/locales/registrar/base.en.yml create mode 100644 spec/features/registrar/sign_out_spec.rb create mode 100644 spec/requests/registrar/sign_out_spec.rb diff --git a/app/views/registrar/base/_current_user.html.erb b/app/views/registrar/base/_current_user.html.erb new file mode 100644 index 000000000..9c82b37f0 --- /dev/null +++ b/app/views/registrar/base/_current_user.html.erb @@ -0,0 +1,3 @@ +<%= "#{current_user} (#{current_user.roles.first}) - #{current_user.registrar}" %> +| +<%= link_to t('.sign_out'), registrar_destroy_user_session_path, method: :delete %> diff --git a/app/views/registrar/base/_navbar.haml b/app/views/registrar/base/_navbar.haml index 160b455ed..2f6f77171 100644 --- a/app/views/registrar/base/_navbar.haml +++ b/app/views/registrar/base/_navbar.haml @@ -16,14 +16,5 @@ - active_class = ['registrar/xml_consoles'].include?(params[:controller]) ? 'active' :nil %li{class: active_class}= link_to t(:xml_console), registrar_xml_console_path - %ul.nav.navbar-nav.navbar-right - %li.dropdown - %a.dropdown-toggle{"data-toggle" => "dropdown", href: "#"} - = "#{current_user} (#{current_user.roles.first}) - #{current_user.registrar}" - %span.caret - %ul.dropdown-menu{role: "menu"} - - current_user.linked_users.each do |user| - %li= link_to "#{user} (#{user.roles.first}) - #{user.registrar}", registrar_switch_current_user_path(user), - id: "switch-current-user-#{user.id}-btn" - - if user_signed_in? - %li= link_to t(:log_out_), '/registrar/logout' + %div.navbar-text.navbar-right + = render 'current_user' diff --git a/config/locales/en.yml b/config/locales/en.yml index 6153e319f..ef8762917 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -816,7 +816,6 @@ en: notes: Notes active_price_for_this_operation_is: 'Active price for this operation is %{price}' active_price_missing_for_this_operation: 'Active price missing for this operation!' - log_out_: 'Log out' valid_to_from: 'Valid to from' valid_to_until: 'Valid to until' registrant_ident: 'Registrant ident' diff --git a/config/locales/registrar/base.en.yml b/config/locales/registrar/base.en.yml new file mode 100644 index 000000000..a00df6750 --- /dev/null +++ b/config/locales/registrar/base.en.yml @@ -0,0 +1,5 @@ +en: + registrar: + base: + current_user: + sign_out: Log out diff --git a/config/routes.rb b/config/routes.rb index 685c7dd07..0f83e9aef 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -44,7 +44,7 @@ Rails.application.routes.draw do post 'id' => 'sessions#id' post 'mid' => 'sessions#mid' get 'switch_user/:id' => 'sessions#switch_user' - get 'logout' => '/devise/sessions#destroy' + delete 'logout', to: '/devise/sessions#destroy', as: :destroy_user_session end get 'current_user/switch/:new_user_id', to: 'current_user#switch', as: :switch_current_user diff --git a/spec/features/registrar/sign_out_spec.rb b/spec/features/registrar/sign_out_spec.rb new file mode 100644 index 000000000..33acc52ab --- /dev/null +++ b/spec/features/registrar/sign_out_spec.rb @@ -0,0 +1,15 @@ +require 'rails_helper' + +RSpec.feature 'Registrar area sign-out', settings: false do + background do + Setting.registrar_ip_whitelist_enabled = false + sign_in_to_registrar_area(user: create(:api_user_with_unlimited_balance)) + end + + scenario 'signs the user out' do + visit registrar_root_path + click_on t('registrar.base.current_user.sign_out') + + expect(page).to have_text('Signed out successfully.') + end +end diff --git a/spec/requests/registrar/sign_out_spec.rb b/spec/requests/registrar/sign_out_spec.rb new file mode 100644 index 000000000..fa8ccdd10 --- /dev/null +++ b/spec/requests/registrar/sign_out_spec.rb @@ -0,0 +1,20 @@ +require 'rails_helper' + +RSpec.describe 'Registrar area sign-out', settings: false do + describe 'sign-out' do + before do + sign_in_to_registrar_area + end + + it 'signs the user out' do + delete registrar_destroy_user_session_path + follow_redirect! + expect(controller.current_user).to be_nil + end + + it 'redirects to login url' do + delete registrar_destroy_user_session_path + expect(response).to redirect_to(registrar_login_url) + end + end +end From 9c6c78e6f845912442ee0c42a93b9d932b32c131 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 04:44:51 +0300 Subject: [PATCH 18/28] Clean up #599 --- app/views/layouts/registrar/base.haml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/views/layouts/registrar/base.haml b/app/views/layouts/registrar/base.haml index c58e271aa..f7c24690e 100644 --- a/app/views/layouts/registrar/base.haml +++ b/app/views/layouts/registrar/base.haml @@ -4,8 +4,6 @@ %meta{charset: "utf-8"}/ %meta{content: "IE=edge", "http-equiv" => "X-UA-Compatible"}/ %meta{content: "width=device-width, initial-scale=1", name: "viewport"}/ - %meta{content: "Full stack top-level domain (TLD) management.", name: "description"}/ - %meta{content: "Gitlab LTD", name: "author"}/ - if content_for? :head_title = yield :head_title - else @@ -15,7 +13,6 @@ = javascript_include_tag 'registrar-manifest', 'data-turbolinks-track' => true = favicon_link_tag 'favicon.ico' %body - / Fixed navbar %nav.navbar.navbar-default.navbar-fixed-top .container .navbar-header @@ -29,8 +26,7 @@ - if unstable_env.present? .text-center %small{style: 'color: #0074B3;'}= unstable_env - - if current_user - = render 'navbar' + = render 'navbar' .container = render 'shared/flash' From cdc58f5cedbd2335fd247b2673ff4641a03dd3bb Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 04:48:55 +0300 Subject: [PATCH 19/28] Improve registrar sign-out spec #599 --- spec/requests/registrar/sign_out_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/requests/registrar/sign_out_spec.rb b/spec/requests/registrar/sign_out_spec.rb index fa8ccdd10..bdbd1a778 100644 --- a/spec/requests/registrar/sign_out_spec.rb +++ b/spec/requests/registrar/sign_out_spec.rb @@ -3,6 +3,7 @@ require 'rails_helper' RSpec.describe 'Registrar area sign-out', settings: false do describe 'sign-out' do before do + Setting.registrar_ip_whitelist_enabled = false sign_in_to_registrar_area end From bd78c9d5c8fc6198058217f5e7d5e5a147018c50 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 10:52:20 +0300 Subject: [PATCH 20/28] Fix invalid HTML #600 --- app/views/layouts/registrar/base.haml | 2 +- app/views/layouts/registrar/sessions.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/layouts/registrar/base.haml b/app/views/layouts/registrar/base.haml index f7c24690e..e0bdd5f24 100644 --- a/app/views/layouts/registrar/base.haml +++ b/app/views/layouts/registrar/base.haml @@ -36,7 +36,7 @@ %footer.footer .container - %row + .row .col-md-6 = image_tag 'eis-logo-et.png' .col-md-6.text-right diff --git a/app/views/layouts/registrar/sessions.haml b/app/views/layouts/registrar/sessions.haml index 974c47a40..49c71ab2c 100644 --- a/app/views/layouts/registrar/sessions.haml +++ b/app/views/layouts/registrar/sessions.haml @@ -37,7 +37,7 @@ %footer.footer .container - %row + .row .col-md-6 = image_tag 'eis-logo-et.png' .col-md-6.text-right From e2ebe0aa84d4e6226cea51ce1922919f7223855f Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 11:03:43 +0300 Subject: [PATCH 21/28] Improve registrar area linked user switch - Introduce profile - Move linked users to profile - Use PUT #599 --- .../registrar/current_user_controller.rb | 10 ++- .../registrar/profile_controller.rb | 17 ++++ app/models/api_user.rb | 10 ++- app/presenters/user_presenter.rb | 19 +++++ .../registrar/base/_current_user.html.erb | 3 +- .../registrar/profile/_linked_users.html.erb | 18 +++++ app/views/registrar/profile/show.html.erb | 9 +++ config/locales/registrar/profile.en.yml | 9 +++ config/routes.rb | 4 +- spec/features/registrar/linked_users_spec.rb | 18 +++++ spec/features/registrar/profile_spec.rb | 14 ++++ spec/features/registrar/user_switch_spec.rb | 16 ---- spec/models/api_user_spec.rb | 32 ++++++++ spec/presenters/user_presenter_spec.rb | 16 ++++ spec/requests/registrar/current_user_spec.rb | 49 ------------ spec/requests/registrar/linked_users_spec.rb | 77 +++++++++++++++++++ 16 files changed, 248 insertions(+), 73 deletions(-) create mode 100644 app/controllers/registrar/profile_controller.rb create mode 100644 app/presenters/user_presenter.rb create mode 100644 app/views/registrar/profile/_linked_users.html.erb create mode 100644 app/views/registrar/profile/show.html.erb create mode 100644 config/locales/registrar/profile.en.yml create mode 100644 spec/features/registrar/linked_users_spec.rb create mode 100644 spec/features/registrar/profile_spec.rb delete mode 100644 spec/features/registrar/user_switch_spec.rb create mode 100644 spec/presenters/user_presenter_spec.rb delete mode 100644 spec/requests/registrar/current_user_spec.rb create mode 100644 spec/requests/registrar/linked_users_spec.rb diff --git a/app/controllers/registrar/current_user_controller.rb b/app/controllers/registrar/current_user_controller.rb index 4661318e7..266e4b915 100644 --- a/app/controllers/registrar/current_user_controller.rb +++ b/app/controllers/registrar/current_user_controller.rb @@ -3,10 +3,16 @@ class Registrar skip_authorization_check def switch - new_user = ApiUser.find(params[:new_user_id]) - sign_in(new_user) if new_user.identity_code == current_user.identity_code + raise 'Cannot switch to unlinked user' unless current_user.linked_with?(new_user) + sign_in(new_user) redirect_to :back, notice: t('.switched', new_user: new_user) end + + private + + def new_user + @new_user ||= ApiUser.find(params[:new_user_id]) + end end end diff --git a/app/controllers/registrar/profile_controller.rb b/app/controllers/registrar/profile_controller.rb new file mode 100644 index 000000000..5f202a894 --- /dev/null +++ b/app/controllers/registrar/profile_controller.rb @@ -0,0 +1,17 @@ +class Registrar + class ProfileController < BaseController + skip_authorization_check + + helper_method :linked_users + + def show + @user = current_user + end + + private + + def linked_users + current_user.linked_users + end + end +end diff --git a/app/models/api_user.rb b/app/models/api_user.rb index a5030c096..5e20db24a 100644 --- a/app/models/api_user.rb +++ b/app/models/api_user.rb @@ -23,9 +23,9 @@ class ApiUser < User validates :password, length: { minimum: min_password_length } validates :username, uniqueness: true - # TODO: probably cache, because it's requested on every EPP - delegate :code, to: :registrar, prefix: true + delegate :code, :name, to: :registrar, prefix: true + alias_attribute :login, :username attr_accessor :registrar_typeahead SUPER = 'super' @@ -91,7 +91,11 @@ class ApiUser < User def linked_users self.class.where(identity_code: identity_code) - .where("identity_code is NOT NULL and identity_code != ''") + .where("identity_code IS NOT NULL AND identity_code != ''") .where.not(id: id) end + + def linked_with?(another_api_user) + another_api_user.identity_code == self.identity_code + end end diff --git a/app/presenters/user_presenter.rb b/app/presenters/user_presenter.rb new file mode 100644 index 000000000..29aeaa187 --- /dev/null +++ b/app/presenters/user_presenter.rb @@ -0,0 +1,19 @@ +class UserPresenter + def initialize(user:, view:) + @user = user + @view = view + end + + def login_with_role + "#{user.login} (#{role_name}) - #{user.registrar_name}" + end + + private + + def role_name + user.roles.first + end + + attr_reader :user + attr_reader :view +end diff --git a/app/views/registrar/base/_current_user.html.erb b/app/views/registrar/base/_current_user.html.erb index 9c82b37f0..01dc5262b 100644 --- a/app/views/registrar/base/_current_user.html.erb +++ b/app/views/registrar/base/_current_user.html.erb @@ -1,3 +1,4 @@ -<%= "#{current_user} (#{current_user.roles.first}) - #{current_user.registrar}" %> +<% current_user_presenter = UserPresenter.new(user: current_user, view: self) %> +<%= link_to current_user_presenter.login_with_role, registrar_profile_path, id: 'registrar-profile-btn' %> | <%= link_to t('.sign_out'), registrar_destroy_user_session_path, method: :delete %> diff --git a/app/views/registrar/profile/_linked_users.html.erb b/app/views/registrar/profile/_linked_users.html.erb new file mode 100644 index 000000000..fa8d868b4 --- /dev/null +++ b/app/views/registrar/profile/_linked_users.html.erb @@ -0,0 +1,18 @@ +
+
<%= t '.header' %>
+
+
    + <% linked_users.each do |user| %> + <% user_presenter = UserPresenter.new(user: user, view: self) %> +
  • <%= user_presenter.login_with_role %> + <%= link_to t('.switch_btn'), + registrar_switch_current_user_path(user), + method: :put, + id: "switch-current-user-#{user.id}-btn", + class: 'btn btn-primary btn-xs' %> +
    • + <% end %> +
+
+
+ diff --git a/app/views/registrar/profile/show.html.erb b/app/views/registrar/profile/show.html.erb new file mode 100644 index 000000000..d6a90d64c --- /dev/null +++ b/app/views/registrar/profile/show.html.erb @@ -0,0 +1,9 @@ +
+

<%= t '.header' %>

+
+ +
+
+ <%= render 'linked_users', linked_users: linked_users %> +
+
diff --git a/config/locales/registrar/profile.en.yml b/config/locales/registrar/profile.en.yml new file mode 100644 index 000000000..2ccec1406 --- /dev/null +++ b/config/locales/registrar/profile.en.yml @@ -0,0 +1,9 @@ +en: + registrar: + profile: + show: + header: My profile + + linked_users: + header: Linked users + switch_btn: Switch diff --git a/config/routes.rb b/config/routes.rb index 0f83e9aef..9a513b434 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -43,11 +43,11 @@ Rails.application.routes.draw do post 'sessions' => 'sessions#create' post 'id' => 'sessions#id' post 'mid' => 'sessions#mid' - get 'switch_user/:id' => 'sessions#switch_user' delete 'logout', to: '/devise/sessions#destroy', as: :destroy_user_session end - get 'current_user/switch/:new_user_id', to: 'current_user#switch', as: :switch_current_user + put 'current_user/switch/:new_user_id', to: 'current_user#switch', as: :switch_current_user + resource :profile, controller: :profile, only: :show resources :domains do collection do diff --git a/spec/features/registrar/linked_users_spec.rb b/spec/features/registrar/linked_users_spec.rb new file mode 100644 index 000000000..2f5fbbd28 --- /dev/null +++ b/spec/features/registrar/linked_users_spec.rb @@ -0,0 +1,18 @@ +require 'rails_helper' + +RSpec.feature 'Registrar area linked users', settings: false do + given!(:current_user) { create(:api_user_with_unlimited_balance, id: 1, identity_code: 'test') } + given!(:linked_user) { create(:api_user_with_unlimited_balance, id: 2, identity_code: 'test', + username: 'new-user-name') } + + background do + Setting.registrar_ip_whitelist_enabled = false + sign_in_to_registrar_area(user: current_user) + end + + scenario 'switches current user to a linked one' do + visit registrar_profile_path + click_link_or_button 'switch-current-user-2-btn' + expect(page).to have_text('You are now signed in as a user "new-user-name"') + end +end diff --git a/spec/features/registrar/profile_spec.rb b/spec/features/registrar/profile_spec.rb new file mode 100644 index 000000000..195458576 --- /dev/null +++ b/spec/features/registrar/profile_spec.rb @@ -0,0 +1,14 @@ +require 'rails_helper' + +RSpec.feature 'Registrar area profile', settings: false do + background do + Setting.registrar_ip_whitelist_enabled = false + sign_in_to_registrar_area(user: create(:api_user_with_unlimited_balance)) + end + + scenario 'shows profile' do + visit registrar_root_path + click_on 'registrar-profile-btn' + expect(page).to have_text(t('registrar.profile.show.header')) + end +end diff --git a/spec/features/registrar/user_switch_spec.rb b/spec/features/registrar/user_switch_spec.rb deleted file mode 100644 index 0927a908a..000000000 --- a/spec/features/registrar/user_switch_spec.rb +++ /dev/null @@ -1,16 +0,0 @@ -require 'rails_helper' - -RSpec.feature 'Registrar area user switch', settings: false do - given!(:current_user) { create(:api_user, id: 1, identity_code: 'test') } - given!(:new_user) { create(:api_user, id: 2, identity_code: 'test', username: 'new-user-name') } - - background do - sign_in_to_registrar_area(user: current_user) - end - - scenario 'successful user switch' do - visit registrar_root_path - click_link_or_button 'switch-current-user-2-btn' - expect(page).to have_text('You are now signed in as a user "new-user-name"') - end -end diff --git a/spec/models/api_user_spec.rb b/spec/models/api_user_spec.rb index fc5388b8c..12213c819 100644 --- a/spec/models/api_user_spec.rb +++ b/spec/models/api_user_spec.rb @@ -93,4 +93,36 @@ RSpec.describe ApiUser do expect(api_user.linked_users).to be_empty end end + + describe '#linked_with?', db: false do + it 'returns true if identity codes match' do + api_user = described_class.new(identity_code: 'test') + another_api_user = described_class.new(identity_code: 'test') + + expect(api_user.linked_with?(another_api_user)).to be true + end + + it 'returns false if identity codes do not match' do + api_user = described_class.new(identity_code: 'test') + another_api_user = described_class.new(identity_code: 'another-test') + + expect(api_user.linked_with?(another_api_user)).to be false + end + end + + describe '#login', db: false do + it 'is alias to #username' do + user = described_class.new(username: 'test-username') + expect(user.login).to eq('test-username') + end + end + + describe '#registrar_name', db: false do + it 'delegates to registrar' do + registrar = Registrar.new(name: 'test name') + user = described_class.new(registrar: registrar) + + expect(user.registrar_name).to eq('test name') + end + end end diff --git a/spec/presenters/user_presenter_spec.rb b/spec/presenters/user_presenter_spec.rb new file mode 100644 index 000000000..ba9e1673f --- /dev/null +++ b/spec/presenters/user_presenter_spec.rb @@ -0,0 +1,16 @@ +require 'rails_helper' + +RSpec.describe UserPresenter do + let(:presenter) { described_class.new(user: user, view: view) } + + describe '#login_with_role' do + let(:user) { instance_double(ApiUser, + login: 'login', + roles: %w[role], + registrar_name: 'registrar') } + + it 'returns username with role and registrar' do + expect(presenter.login_with_role).to eq('login (role) - registrar') + end + end +end diff --git a/spec/requests/registrar/current_user_spec.rb b/spec/requests/registrar/current_user_spec.rb deleted file mode 100644 index 0d5f22eb0..000000000 --- a/spec/requests/registrar/current_user_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -require 'rails_helper' - -RSpec.describe 'Registrar current user', db: false do - describe 'GET /registrar/current_user/switch/2' do - context 'when user is authenticated', db: true do - let!(:current_user) { create(:api_user, id: 1, identity_code: 'test') } - let!(:new_user) { create(:api_user, id: 2, identity_code: 'test') } - - before do - sign_in_to_registrar_area(user: current_user) - end - - context 'when ip is allowed' do - let(:restricted_ip) { instance_double(Authorization::RestrictedIP, - can_access_registrar_area?: true) } - - before do - allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) - end - - specify do - get '/registrar/current_user/switch/2', nil, { HTTP_REFERER: 'http://previous.url' } - expect(response).to redirect_to('http://previous.url') - end - end - - context 'when ip is not allowed' do - let(:restricted_ip) { instance_double(Authorization::RestrictedIP, - can_access_registrar_area?: false) } - - before do - allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) - end - - specify do - get '/registrar/current_user/switch/2' - expect(response).to redirect_to(registrar_login_url) - end - end - end - - context 'when user is not authenticated' do - specify do - get '/registrar/current_user/switch/2' - expect(response).to redirect_to(registrar_login_url) - end - end - end -end diff --git a/spec/requests/registrar/linked_users_spec.rb b/spec/requests/registrar/linked_users_spec.rb new file mode 100644 index 000000000..d5d51e3c6 --- /dev/null +++ b/spec/requests/registrar/linked_users_spec.rb @@ -0,0 +1,77 @@ +require 'rails_helper' + +RSpec.describe 'Registrar area linked users', db: false do + describe 'user switch' do + context 'when user is authenticated', db: true do + let!(:current_user) { create(:api_user, id: 1, identity_code: 'code') } + + before do + sign_in_to_registrar_area(user: current_user) + end + + context 'when ip is allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area?: true) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + context 'when new user is linked' do + let!(:new_user) { create(:api_user, id: 2, identity_code: 'code') } + + it 'signs in as a new user' do + put '/registrar/current_user/switch/2', nil, { HTTP_REFERER: registrar_contacts_url } + follow_redirect! + expect(controller.current_user.id).to eq(2) + end + + it 'redirects back' do + put '/registrar/current_user/switch/2', nil, { HTTP_REFERER: 'http://previous.url' } + expect(response).to redirect_to('http://previous.url') + end + end + + context 'when new user is unlinked' do + let!(:new_user) { create(:api_user, id: 2, identity_code: 'another-code') } + + it 'throws exception' do + expect do + put '/registrar/current_user/switch/2', nil, { HTTP_REFERER: registrar_contacts_path } + end.to raise_error('Cannot switch to unlinked user') + end + + it 'does not sign in as a new user' do + suppress StandardError do + put '/registrar/current_user/switch/2', nil, { HTTP_REFERER: registrar_contacts_path } + end + + follow_redirect! + expect(controller.current_user.id).to eq(1) + end + end + end + + context 'when ip is not allowed' do + let(:restricted_ip) { instance_double(Authorization::RestrictedIP, + can_access_registrar_area?: false) } + + before do + allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) + end + + specify do + put '/registrar/current_user/switch/2' + expect(response).to redirect_to(registrar_login_url) + end + end + end + + context 'when user is not authenticated' do + specify do + put '/registrar/current_user/switch/2' + expect(response).to redirect_to(registrar_login_url) + end + end + end +end From b545bc97672a790170a7e9ed77ec69e581d8c678 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 11:19:50 +0300 Subject: [PATCH 22/28] Add "users.registrar_id" foreign key #599 --- db/migrate/20171009080822_add_user_registrar_id_fk.rb | 5 +++++ db/structure.sql | 10 ++++++++++ 2 files changed, 15 insertions(+) create mode 100644 db/migrate/20171009080822_add_user_registrar_id_fk.rb diff --git a/db/migrate/20171009080822_add_user_registrar_id_fk.rb b/db/migrate/20171009080822_add_user_registrar_id_fk.rb new file mode 100644 index 000000000..12bb2c92c --- /dev/null +++ b/db/migrate/20171009080822_add_user_registrar_id_fk.rb @@ -0,0 +1,5 @@ +class AddUserRegistrarIdFk < ActiveRecord::Migration + def change + add_foreign_key :users, :registrars, name: 'user_registrar_id_fk' + end +end diff --git a/db/structure.sql b/db/structure.sql index be366e71a..2b1c1f78a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -4665,6 +4665,14 @@ ALTER TABLE ONLY account_activities ADD CONSTRAINT fk_rails_d2cc3c2fa9 FOREIGN KEY (price_id) REFERENCES prices(id); +-- +-- Name: user_registrar_id_fk; Type: FK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY users + ADD CONSTRAINT user_registrar_id_fk FOREIGN KEY (registrar_id) REFERENCES registrars(id); + + -- -- PostgreSQL database dump complete -- @@ -5167,3 +5175,5 @@ INSERT INTO schema_migrations (version) VALUES ('20170606150352'); INSERT INTO schema_migrations (version) VALUES ('20170606202859'); +INSERT INTO schema_migrations (version) VALUES ('20171009080822'); + From 26eb47ae09391c149871aa1463b8bce3a5cf6b20 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 11:54:44 +0300 Subject: [PATCH 23/28] Drop "api_users" table #599 --- app/models/version/api_user_version.rb | 5 - db/migrate/20171009082321_drop_api_users.rb | 6 + db/structure.sql | 129 +------------------- doc/models_brief.svg | 11 -- doc/models_complete.svg | 22 ---- 5 files changed, 8 insertions(+), 165 deletions(-) delete mode 100644 app/models/version/api_user_version.rb create mode 100644 db/migrate/20171009082321_drop_api_users.rb diff --git a/app/models/version/api_user_version.rb b/app/models/version/api_user_version.rb deleted file mode 100644 index 77d5bcf70..000000000 --- a/app/models/version/api_user_version.rb +++ /dev/null @@ -1,5 +0,0 @@ -class ApiUserVersion < PaperTrail::Version - include VersionSession - self.table_name = :log_api_users - self.sequence_name = :log_api_users_id_seq -end diff --git a/db/migrate/20171009082321_drop_api_users.rb b/db/migrate/20171009082321_drop_api_users.rb new file mode 100644 index 000000000..dd1a9737b --- /dev/null +++ b/db/migrate/20171009082321_drop_api_users.rb @@ -0,0 +1,6 @@ +class DropApiUsers < ActiveRecord::Migration + def change + drop_table :api_users + drop_table :log_api_users + end +end diff --git a/db/structure.sql b/db/structure.sql index 2b1c1f78a..ffce69261 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -357,44 +357,6 @@ CREATE SEQUENCE accounts_id_seq ALTER SEQUENCE accounts_id_seq OWNED BY accounts.id; --- --- Name: api_users; Type: TABLE; Schema: public; Owner: -; Tablespace: --- - -CREATE TABLE api_users ( - id integer NOT NULL, - registrar_id integer, - username character varying, - password character varying, - active boolean DEFAULT false, - csr text, - crt text, - created_at timestamp without time zone, - updated_at timestamp without time zone, - creator_str character varying, - updator_str character varying -); - - --- --- Name: api_users_id_seq; Type: SEQUENCE; Schema: public; Owner: - --- - -CREATE SEQUENCE api_users_id_seq - START WITH 1 - INCREMENT BY 1 - NO MINVALUE - NO MAXVALUE - CACHE 1; - - --- --- Name: api_users_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - --- - -ALTER SEQUENCE api_users_id_seq OWNED BY api_users.id; - - -- -- Name: bank_statements; Type: TABLE; Schema: public; Owner: -; Tablespace: -- @@ -1380,44 +1342,6 @@ CREATE SEQUENCE log_accounts_id_seq ALTER SEQUENCE log_accounts_id_seq OWNED BY log_accounts.id; --- --- Name: log_api_users; Type: TABLE; Schema: public; Owner: -; Tablespace: --- - -CREATE TABLE log_api_users ( - id integer NOT NULL, - item_type character varying NOT NULL, - item_id integer NOT NULL, - event character varying NOT NULL, - whodunnit character varying, - object json, - object_changes json, - created_at timestamp without time zone, - session character varying, - children json, - uuid character varying -); - - --- --- Name: log_api_users_id_seq; Type: SEQUENCE; Schema: public; Owner: - --- - -CREATE SEQUENCE log_api_users_id_seq - START WITH 1 - INCREMENT BY 1 - NO MINVALUE - NO MAXVALUE - CACHE 1; - - --- --- Name: log_api_users_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - --- - -ALTER SEQUENCE log_api_users_id_seq OWNED BY log_api_users.id; - - -- -- Name: log_bank_statements; Type: TABLE; Schema: public; Owner: -; Tablespace: -- @@ -2868,13 +2792,6 @@ ALTER TABLE ONLY account_activities ALTER COLUMN id SET DEFAULT nextval('account ALTER TABLE ONLY accounts ALTER COLUMN id SET DEFAULT nextval('accounts_id_seq'::regclass); --- --- Name: id; Type: DEFAULT; Schema: public; Owner: - --- - -ALTER TABLE ONLY api_users ALTER COLUMN id SET DEFAULT nextval('api_users_id_seq'::regclass); - - -- -- Name: id; Type: DEFAULT; Schema: public; Owner: - -- @@ -3043,13 +2960,6 @@ ALTER TABLE ONLY log_account_activities ALTER COLUMN id SET DEFAULT nextval('log ALTER TABLE ONLY log_accounts ALTER COLUMN id SET DEFAULT nextval('log_accounts_id_seq'::regclass); --- --- Name: id; Type: DEFAULT; Schema: public; Owner: - --- - -ALTER TABLE ONLY log_api_users ALTER COLUMN id SET DEFAULT nextval('log_api_users_id_seq'::regclass); - - -- -- Name: id; Type: DEFAULT; Schema: public; Owner: - -- @@ -3325,14 +3235,6 @@ ALTER TABLE ONLY accounts ADD CONSTRAINT accounts_pkey PRIMARY KEY (id); --- --- Name: api_users_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: --- - -ALTER TABLE ONLY api_users - ADD CONSTRAINT api_users_pkey PRIMARY KEY (id); - - -- -- Name: bank_statements_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: -- @@ -3525,14 +3427,6 @@ ALTER TABLE ONLY log_accounts ADD CONSTRAINT log_accounts_pkey PRIMARY KEY (id); --- --- Name: log_api_users_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: --- - -ALTER TABLE ONLY log_api_users - ADD CONSTRAINT log_api_users_pkey PRIMARY KEY (id); - - -- -- Name: log_bank_statements_pkey; Type: CONSTRAINT; Schema: public; Owner: -; Tablespace: -- @@ -3865,13 +3759,6 @@ CREATE INDEX index_account_activities_on_invoice_id ON account_activities USING CREATE INDEX index_accounts_on_registrar_id ON accounts USING btree (registrar_id); --- --- Name: index_api_users_on_registrar_id; Type: INDEX; Schema: public; Owner: -; Tablespace: --- - -CREATE INDEX index_api_users_on_registrar_id ON api_users USING btree (registrar_id); - - -- -- Name: index_blocked_domains_on_name; Type: INDEX; Schema: public; Owner: -; Tablespace: -- @@ -4145,20 +4032,6 @@ CREATE INDEX index_log_accounts_on_item_type_and_item_id ON log_accounts USING b CREATE INDEX index_log_accounts_on_whodunnit ON log_accounts USING btree (whodunnit); --- --- Name: index_log_api_users_on_item_type_and_item_id; Type: INDEX; Schema: public; Owner: -; Tablespace: --- - -CREATE INDEX index_log_api_users_on_item_type_and_item_id ON log_api_users USING btree (item_type, item_id); - - --- --- Name: index_log_api_users_on_whodunnit; Type: INDEX; Schema: public; Owner: -; Tablespace: --- - -CREATE INDEX index_log_api_users_on_whodunnit ON log_api_users USING btree (whodunnit); - - -- -- Name: index_log_bank_statements_on_item_type_and_item_id; Type: INDEX; Schema: public; Owner: -; Tablespace: -- @@ -5177,3 +5050,5 @@ INSERT INTO schema_migrations (version) VALUES ('20170606202859'); INSERT INTO schema_migrations (version) VALUES ('20171009080822'); +INSERT INTO schema_migrations (version) VALUES ('20171009082321'); + diff --git a/doc/models_brief.svg b/doc/models_brief.svg index 6a2b3f428..76dd9a83f 100644 --- a/doc/models_brief.svg +++ b/doc/models_brief.svg @@ -304,17 +304,6 @@ - -ApiUserVersion - -ApiUserVersion - - -ApiUserVersion->VersionAssociation - - - - DomainVersion diff --git a/doc/models_complete.svg b/doc/models_complete.svg index 2a67d0970..58b86b986 100644 --- a/doc/models_complete.svg +++ b/doc/models_complete.svg @@ -600,28 +600,6 @@ - -ApiUserVersion - -ApiUserVersion - -id :integer -item_type :string -item_id :integer -event :string -whodunnit :string -object :json -object_changes :json -created_at :datetime -session :string -children :json - - -ApiUserVersion->VersionAssociation - - - - DomainVersion From 6e597f39e89fd461298524bbd67a05d14f992468 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Tue, 10 Oct 2017 06:02:22 +0300 Subject: [PATCH 24/28] Improve registrar area sign-in/out specs #599 --- .../features/registrar/ip_restriction_spec.rb | 12 +++ spec/features/registrar/sessions/new_spec.rb | 42 --------- .../{login => sign_in}/mobile_id_spec.rb | 0 .../registrar/sign_in/password_spec.rb | 43 +++++++++ .../authorization/restricted_ip_spec.rb | 2 +- .../requests/registrar/ip_restriction_spec.rb | 90 +++++++++++++++++++ spec/requests/registrar/sessions_spec.rb | 67 -------------- .../registrar/sign_in/password_spec.rb | 20 +++++ spec/requests/registrar/sign_out_spec.rb | 26 +++--- 9 files changed, 178 insertions(+), 124 deletions(-) create mode 100644 spec/features/registrar/ip_restriction_spec.rb delete mode 100644 spec/features/registrar/sessions/new_spec.rb rename spec/features/registrar/{login => sign_in}/mobile_id_spec.rb (100%) create mode 100644 spec/features/registrar/sign_in/password_spec.rb create mode 100644 spec/requests/registrar/ip_restriction_spec.rb delete mode 100644 spec/requests/registrar/sessions_spec.rb create mode 100644 spec/requests/registrar/sign_in/password_spec.rb diff --git a/spec/features/registrar/ip_restriction_spec.rb b/spec/features/registrar/ip_restriction_spec.rb new file mode 100644 index 000000000..dc9631057 --- /dev/null +++ b/spec/features/registrar/ip_restriction_spec.rb @@ -0,0 +1,12 @@ +require 'rails_helper' + +RSpec.feature 'Registrar area IP restriction', settings: false do + background do + Setting.registrar_ip_whitelist_enabled = true + end + + scenario 'notifies the user if his IP is not allowed' do + visit registrar_root_path + expect(page).to have_text('Access denied from IP 127.0.0.1') + end +end diff --git a/spec/features/registrar/sessions/new_spec.rb b/spec/features/registrar/sessions/new_spec.rb deleted file mode 100644 index feec6cae1..000000000 --- a/spec/features/registrar/sessions/new_spec.rb +++ /dev/null @@ -1,42 +0,0 @@ -require 'rails_helper' - -RSpec.feature 'Registrar area ip restriction', settings: false do - context 'when enabled' do - background do - Setting.registrar_ip_whitelist_enabled = true - end - - context 'when ip is allowed' do - given!(:white_ip) { create(:white_ip, - ipv4: '127.0.0.1', - interfaces: [WhiteIp::REGISTRAR]) } - - it 'does not show error message' do - visit registrar_login_path - expect(page).to_not have_text(error_message) - end - end - - context 'when ip is not allowed' do - it 'shows error message' do - visit registrar_login_path - expect(page).to have_text(error_message) - end - end - end - - context 'when disabled' do - background do - Setting.registrar_ip_whitelist_enabled = false - end - - it 'does not show error message' do - visit registrar_login_path - expect(page).to_not have_text(error_message) - end - end - - def error_message - t('registrar.authorization.ip_not_allowed', ip: '127.0.0.1') - end -end diff --git a/spec/features/registrar/login/mobile_id_spec.rb b/spec/features/registrar/sign_in/mobile_id_spec.rb similarity index 100% rename from spec/features/registrar/login/mobile_id_spec.rb rename to spec/features/registrar/sign_in/mobile_id_spec.rb diff --git a/spec/features/registrar/sign_in/password_spec.rb b/spec/features/registrar/sign_in/password_spec.rb new file mode 100644 index 000000000..f0cc3ed49 --- /dev/null +++ b/spec/features/registrar/sign_in/password_spec.rb @@ -0,0 +1,43 @@ +require 'rails_helper' + +RSpec.feature 'Registrar area password sign-in' do + background do + Setting.registrar_ip_whitelist_enabled = false + end + + scenario 'signs in the user with valid credentials' do + create(:api_user_with_unlimited_balance, + active: true, + login: 'test', + password: 'testtest') + + visit registrar_login_path + sign_in_with 'test', 'testtest' + + expect(page).to have_text(t('registrar.base.current_user.sign_out')) + end + + scenario 'notifies the user with invalid credentials' do + create(:api_user, login: 'test', password: 'testtest') + + visit registrar_login_path + sign_in_with 'test', 'invalid' + + expect(page).to have_text('No such user') + end + + scenario 'notifies the user with inactive account' do + create(:api_user, active: false, login: 'test', password: 'testtest') + + visit registrar_login_path + sign_in_with 'test', 'testtest' + + expect(page).to have_text('User is not active') + end + + def sign_in_with(username, password) + fill_in 'depp_user_tag', with: username + fill_in 'depp_user_password', with: password + click_button 'Login' + end +end diff --git a/spec/models/authorization/restricted_ip_spec.rb b/spec/models/authorization/restricted_ip_spec.rb index 6fba76657..e64a1739f 100644 --- a/spec/models/authorization/restricted_ip_spec.rb +++ b/spec/models/authorization/restricted_ip_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe Authorization::RestrictedIP do - describe '#enabled?', db: true, settings: false do + describe '::enabled?', db: true, settings: false do context 'when "registrar_ip_whitelist_enabled" is true' do before do Setting.registrar_ip_whitelist_enabled = true diff --git a/spec/requests/registrar/ip_restriction_spec.rb b/spec/requests/registrar/ip_restriction_spec.rb new file mode 100644 index 000000000..5fd07402e --- /dev/null +++ b/spec/requests/registrar/ip_restriction_spec.rb @@ -0,0 +1,90 @@ +require 'rails_helper' + +RSpec.describe 'Registrar area IP restriction', settings: false do + describe 'authenticated area' do + before do + sign_in_to_registrar_area + end + + context 'when IP restriction is enabled' do + before do + Setting.registrar_ip_whitelist_enabled = true + end + + context 'when ip is allowed' do + let!(:white_ip) { create(:white_ip, + ipv4: '127.0.0.1', + registrar: controller.current_user.registrar, + interfaces: [WhiteIp::REGISTRAR]) } + + specify do + get registrar_root_url + follow_redirect! + expect(response).to be_success + end + end + + context 'when ip is not allowed' do + it 'signs the user out' do + get registrar_root_url + follow_redirect! + expect(controller.current_user).to be_nil + end + + it 'redirects to login url' do + get registrar_root_url + expect(response).to redirect_to(registrar_login_url) + end + end + end + + context 'when IP restriction is disabled' do + before do + Setting.registrar_ip_whitelist_enabled = false + end + + specify do + get registrar_root_url + follow_redirect! + expect(response).to be_success + end + end + end + + describe 'unauthenticated area' do + context 'when IP restriction is enabled' do + before do + Setting.registrar_ip_whitelist_enabled = true + end + + context 'when ip is allowed' do + let!(:white_ip) { create(:white_ip, + ipv4: '127.0.0.1', + interfaces: [WhiteIp::REGISTRAR]) } + + specify do + get registrar_login_path + expect(response).to be_success + end + end + + context 'when ip is not allowed' do + specify do + get registrar_login_path + expect(response).to be_forbidden + end + end + end + + context 'when IP restriction is disabled' do + before do + Setting.registrar_ip_whitelist_enabled = false + end + + specify do + get registrar_login_path + expect(response).to be_success + end + end + end +end diff --git a/spec/requests/registrar/sessions_spec.rb b/spec/requests/registrar/sessions_spec.rb deleted file mode 100644 index 674cd8f0c..000000000 --- a/spec/requests/registrar/sessions_spec.rb +++ /dev/null @@ -1,67 +0,0 @@ -require 'rails_helper' - -RSpec.describe 'Registrar session management', db: false do - describe 'GET /registrar/login' do - context 'when ip is allowed' do - let(:restricted_ip) { instance_double(Authorization::RestrictedIP, - can_access_registrar_area_sign_in_page?: true) } - - before do - allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) - end - - specify do - get registrar_login_path - expect(response).to be_success - end - end - - context 'when ip is not allowed' do - let(:restricted_ip) { instance_double(Authorization::RestrictedIP, - can_access_registrar_area_sign_in_page?: false) } - - before do - allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) - end - - specify do - get registrar_login_path - expect(response).to be_forbidden - end - end - end - - describe 'POST /registrar/sessions' do - context 'when ip is allowed' do - let(:restricted_ip) { instance_double(Authorization::RestrictedIP, - can_access_registrar_area_sign_in_page?: true) } - - before do - allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) - end - - specify do - make_request - expect(response).to be_success - end - end - - context 'when ip is not allowed' do - let(:restricted_ip) { instance_double(Authorization::RestrictedIP, - can_access_registrar_area_sign_in_page?: false) } - - before do - allow(Authorization::RestrictedIP).to receive(:new).and_return(restricted_ip) - end - - specify do - make_request - expect(response).to be_forbidden - end - end - - def make_request - post registrar_sessions_path, depp_user: { tag: 'test', password: 'test' } - end - end -end diff --git a/spec/requests/registrar/sign_in/password_spec.rb b/spec/requests/registrar/sign_in/password_spec.rb new file mode 100644 index 000000000..f419ffa01 --- /dev/null +++ b/spec/requests/registrar/sign_in/password_spec.rb @@ -0,0 +1,20 @@ +require 'rails_helper' + +RSpec.describe 'Registrar area password sign-in', settings: false do + let!(:user) { create(:api_user, active: true, login: 'test', password: 'testtest') } + + before do + Setting.registrar_ip_whitelist_enabled = false + end + + it 'signs the user in' do + post registrar_sessions_path, depp_user: { tag: 'test', password: 'testtest' } + follow_redirect! + expect(controller.current_user).to eq(user) + end + + it 'redirects to root url' do + post registrar_sessions_path, depp_user: { tag: 'test', password: 'testtest' } + expect(response).to redirect_to(registrar_root_url) + end +end diff --git a/spec/requests/registrar/sign_out_spec.rb b/spec/requests/registrar/sign_out_spec.rb index bdbd1a778..086b95f64 100644 --- a/spec/requests/registrar/sign_out_spec.rb +++ b/spec/requests/registrar/sign_out_spec.rb @@ -1,21 +1,19 @@ require 'rails_helper' RSpec.describe 'Registrar area sign-out', settings: false do - describe 'sign-out' do - before do - Setting.registrar_ip_whitelist_enabled = false - sign_in_to_registrar_area - end + before do + Setting.registrar_ip_whitelist_enabled = false + sign_in_to_registrar_area + end - it 'signs the user out' do - delete registrar_destroy_user_session_path - follow_redirect! - expect(controller.current_user).to be_nil - end + it 'signs the user out' do + delete registrar_destroy_user_session_path + follow_redirect! + expect(controller.current_user).to be_nil + end - it 'redirects to login url' do - delete registrar_destroy_user_session_path - expect(response).to redirect_to(registrar_login_url) - end + it 'redirects to login url' do + delete registrar_destroy_user_session_path + expect(response).to redirect_to(registrar_login_url) end end From 457b73a28e95000dde566982e99464749eadd328 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Tue, 10 Oct 2017 06:05:11 +0300 Subject: [PATCH 25/28] Improve registrar area sign-in/out specs #599 --- spec/requests/registrar/ip_restriction_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/requests/registrar/ip_restriction_spec.rb b/spec/requests/registrar/ip_restriction_spec.rb index 5fd07402e..bcde36b02 100644 --- a/spec/requests/registrar/ip_restriction_spec.rb +++ b/spec/requests/registrar/ip_restriction_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe 'Registrar area IP restriction', settings: false do - describe 'authenticated area' do + context 'when authenticated' do before do sign_in_to_registrar_area end @@ -51,7 +51,7 @@ RSpec.describe 'Registrar area IP restriction', settings: false do end end - describe 'unauthenticated area' do + context 'when unauthenticated' do context 'when IP restriction is enabled' do before do Setting.registrar_ip_whitelist_enabled = true From eecdc6c9b3a53edc5e21d021e8f7eb8d2f5169c4 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Tue, 10 Oct 2017 06:10:43 +0300 Subject: [PATCH 26/28] Set flash message after signing out the user #599 --- app/controllers/registrar/base_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/registrar/base_controller.rb b/app/controllers/registrar/base_controller.rb index 2a0d0a7aa..90f2f5210 100644 --- a/app/controllers/registrar/base_controller.rb +++ b/app/controllers/registrar/base_controller.rb @@ -21,8 +21,9 @@ class Registrar return if allowed - flash[:alert] = t('registrar.authorization.ip_not_allowed', ip: request.ip) sign_out current_user + + flash[:alert] = t('registrar.authorization.ip_not_allowed', ip: request.ip) redirect_to registrar_login_url end From 5932710295100f14796b0bc82fd43b019c4a5ea4 Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Tue, 10 Oct 2017 11:15:18 +0300 Subject: [PATCH 27/28] Clean up #599 --- app/views/registrar/base/_navbar.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/registrar/base/_navbar.haml b/app/views/registrar/base/_navbar.haml index 2f6f77171..d94097bd3 100644 --- a/app/views/registrar/base/_navbar.haml +++ b/app/views/registrar/base/_navbar.haml @@ -1,5 +1,5 @@ .navbar-collapse.collapse - %ul.nav.navbar-nav.public-nav + %ul.nav.navbar-nav - if can? :view, Depp::Domain - active_class = %w(registrar/domains registrar/check registrar/renew registrar/tranfer registrar/keyrelays).include?(params[:controller]) ? 'active' :nil %li{class: active_class}= link_to t(:domains), registrar_domains_path From 721e6c586fff8e45e5ced4a5b1160f397bec1cdd Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Tue, 10 Oct 2017 11:15:49 +0300 Subject: [PATCH 28/28] Fix registrar navbar UI #599 --- app/views/registrar/base/_current_user.html.erb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/views/registrar/base/_current_user.html.erb b/app/views/registrar/base/_current_user.html.erb index 01dc5262b..75d159bfb 100644 --- a/app/views/registrar/base/_current_user.html.erb +++ b/app/views/registrar/base/_current_user.html.erb @@ -1,4 +1,5 @@ <% current_user_presenter = UserPresenter.new(user: current_user, view: self) %> -<%= link_to current_user_presenter.login_with_role, registrar_profile_path, id: 'registrar-profile-btn' %> +<%= link_to current_user_presenter.login_with_role, registrar_profile_path, id: 'registrar-profile-btn', + class: 'navbar-link' %> | -<%= link_to t('.sign_out'), registrar_destroy_user_session_path, method: :delete %> +<%= link_to t('.sign_out'), registrar_destroy_user_session_path, method: :delete, class: 'navbar-link' %>