From e2ebe0aa84d4e6226cea51ce1922919f7223855f Mon Sep 17 00:00:00 2001 From: Artur Beljajev Date: Mon, 9 Oct 2017 11:03:43 +0300 Subject: [PATCH] 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