Refactor ID card sign-in

- Extract to Devise custom strategy
- Use `SSL_CLIENT_S_DN_CN` env variable instead of `SSL_CLIENT_S_DN` to
get ID card data
- Remove `database_authenticatable` strategy from `RegistrantUser`

Closes #1047

app/controllers/registrant/sessions_controller.rb

@@ -1,26 +1,6 @@
 class Registrant::SessionsController < Devise::SessionsController
   layout 'registrant/application'
 
-  def new; end
-
-  def id
-    client_certificate_subject = request.env['SSL_CLIENT_S_DN']
-    client_certificate_issuer = request.env['SSL_CLIENT_I_DN_O']
-
-    if Rails.env.development?
-      client_certificate_subject = 'test'
-      client_certificate_issuer = RegistrantUser::ACCEPTED_ISSUER
-    end
-
-    @user = RegistrantUser.find_or_create_by_idc_data(client_certificate_subject, client_certificate_issuer)
-    if @user
-      sign_in_and_redirect(:registrant_user, @user, event: :authentication)
-    else
-      flash[:alert] = t('login_failed_check_id_card')
-      redirect_to new_registrant_user_session_url
-    end
-  end
-
   def login_mid
     @user = User.new
   end

app/controllers/registrar/sessions_controller.rb

@@ -48,15 +48,22 @@ class Registrar
       end
     end
 
-    def id
+    def id_card
-      @user = ApiUser.find_by_idc_data_and_allowed(request.env['SSL_CLIENT_S_DN'], request.ip)
+      self.resource = warden.authenticate!(auth_options)
 
-      if @user
+      restricted_ip = Authorization::RestrictedIP.new(request.ip)
-        sign_in_and_redirect(:registrar_user, @user, event: :authentication)
+      ip_allowed = restricted_ip.can_access_registrar_area?(resource.registrar)
-      else
+
-        flash[:alert] = t('no_such_user')
+      unless ip_allowed
-        redirect_to new_registrar_user_session_url
+        render text: t('registrar.authorization.ip_not_allowed', ip: request.ip)
+        warden.logout(:registrar_user)
+        return
       end
+
+      set_flash_message!(:notice, :signed_in)
+      sign_in(resource_name, resource)
+      yield resource if block_given?
+      respond_with resource, location: after_sign_in_path_for(resource)
     end
 
     def login_mid

app/models/api_user.rb

@@ -2,7 +2,8 @@ require 'open3'
 
 class ApiUser < User
   include EppErrors
-  devise :database_authenticatable, :trackable, :timeoutable, authentication_keys: [:username]
+  devise :database_authenticatable, :trackable, :timeoutable, :id_card_authenticatable,
+         authentication_keys: [:username]
 
   def epp_code_map
     {
@@ -47,20 +48,10 @@ class ApiUser < User
   end
 
   class << self
-    def find_by_idc_data_and_allowed(idc_data, ip)
+    def find_by_id_card(id_card)
-      return false if idc_data.blank?
+      find_by(identity_code: id_card.personal_code)
-      identity_code = idc_data.scan(/serialNumber=(\d+)/).flatten.first
-
-      return false if ip.blank?
-      possible_users = where(identity_code: identity_code)
-      possible_users.each do |selected_user|
-        if selected_user.registrar.white_ips.registrar_area.include_ip?(ip)
-          return selected_user
     end
   end
-    end
-
-  end
 
   def registrar_typeahead
     @registrar_typeahead || registrar || nil

app/models/id_card.rb

@@ -0,0 +1,6 @@
+class IdCard
+  attr_accessor :first_name
+  attr_accessor :last_name
+  attr_accessor :personal_code
+  attr_accessor :country_code
+end

app/models/registrant_user.rb

@@ -1,8 +1,7 @@
 class RegistrantUser < User
-  ACCEPTED_ISSUER = 'AS Sertifitseerimiskeskus'.freeze
   attr_accessor :idc_data
 
-  devise :database_authenticatable, :trackable, :timeoutable
+  devise :trackable, :timeoutable, :id_card_authenticatable
 
   def ability
     @ability ||= Ability.new(self)
@@ -56,30 +55,6 @@ class RegistrantUser < User
   end
 
   class << self
-    def find_or_create_by_idc_data(idc_data, issuer_organization)
-      return false if idc_data.blank?
-      return false if issuer_organization != ACCEPTED_ISSUER
-
-      idc_data.force_encoding('UTF-8')
-      user_data = {}
-
-      # handling here new and old mode
-      if idc_data.starts_with?('/')
-        user_data[:ident] = idc_data.scan(/serialNumber=(\d+)/).flatten.first
-        user_data[:country_code] = idc_data.scan(/^\/C=(.{2})/).flatten.first
-        user_data[:first_name] = idc_data.scan(%r{/GN=(.+)/serialNumber}).flatten.first
-        user_data[:last_name] = idc_data.scan(%r{/SN=(.+)/GN}).flatten.first
-      else
-        parse_str = ',' + idc_data
-        user_data[:ident] = parse_str.scan(/,serialNumber=(\d+)/).flatten.first
-        user_data[:country_code] = parse_str.scan(/,C=(.{2})/).flatten.first
-        user_data[:first_name] = parse_str.scan(/,GN=([^,]+)/).flatten.first
-        user_data[:last_name] = parse_str.scan(/,SN=([^,]+)/).flatten.first
-      end
-
-      find_or_create_by_user_data(user_data)
-    end
-
     def find_or_create_by_api_data(user_data = {})
       return false unless user_data[:ident]
       return false unless user_data[:first_name]
@@ -98,6 +73,16 @@ class RegistrantUser < User
       find_or_create_by_user_data(user_data)
     end
 
+    def find_by_id_card(id_card)
+      registrant_ident = "#{id_card.country_code}-#{id_card.personal_code}"
+      username = [id_card.first_name, id_card.last_name].join("\s")
+
+      user = find_or_initialize_by(registrant_ident: registrant_ident)
+      user.username = username
+      user.save!
+      user
+    end
+
     private
 
     def find_or_create_by_user_data(user_data = {})

app/views/registrant/sessions/new.html.erb

@@ -11,7 +11,7 @@
         <%= link_to '/registrant/login/mid' do %>
             <%= image_tag 'mid.gif' %>
         <% end %>
-        <%= link_to '/registrant/id', method: :post do %>
+        <%= link_to registrant_id_card_sign_in_path, method: :post do %>
             <%= image_tag 'id_card.gif' %>
         <% end %>
     </div>

app/views/registrar/sessions/new.html.erb

@@ -23,7 +23,7 @@
             <%= image_tag 'mid.gif' %>
         <% end %>
 
-        <%= link_to '/registrar/id', method: :post do %>
+        <%= link_to registrar_id_card_sign_in_path, method: :post do %>
             <%= image_tag 'id_card.gif' %>
         <% end %>
     </div>

config/initializers/devise.rb

@@ -280,4 +280,10 @@ Devise.setup do |config|
   # When using OmniAuth, Devise cannot automatically set OmniAuth path,
   # so you need to do it manually. For the users scope, it would be:
   # config.omniauth_path_prefix = '/my_engine/users/auth'
+
+  require 'devise/models/id_card_authenticatable'
+  require 'devise/strategies/id_card_authenticatable'
+
+  routes = [nil, :new, :destroy]
+  config.add_module :id_card_authenticatable, strategy: true, route: { session: routes }
 end

config/locales/en.yml

@@ -585,7 +585,6 @@ en:
   pending_delete_expired_notification_subject: "Domeeni %{name} kustutamise taotlus on tühistatud / %{name} deletion cancelled"
   delete_confirmation_subject: "Domeeni %{name} kustutatud / %{name} deleted"
   whois: WHOIS
-  login_failed_check_id_card: 'Log in failed, check ID card'
   not_valid_domain_verification_title: Domain verification not available
   not_valid_domain_verification_body: This could mean your verification has been expired or done already.<br><br>Please contact us if you think something is wrong.
   upload_crt: 'Upload CRT'

config/routes.rb

@@ -46,7 +46,10 @@ Rails.application.routes.draw do
       get 'login/mid' => 'sessions#login_mid'
       post 'login/mid' => 'sessions#mid'
       post 'login/mid_status' => 'sessions#mid_status'
-      post 'id' => 'sessions#id'
+
+      # /registrar/id path is hardcoded in Apache config for authentication with Estonian ID-card
+      post 'id' => 'sessions#id_card', as: :id_card_sign_in
+
       post 'mid' => 'sessions#mid'
     end
 
@@ -137,7 +140,11 @@ Rails.application.routes.draw do
       post 'login/mid' => 'sessions#mid'
       post 'login/mid_status' => 'sessions#mid_status'
       post 'mid' => 'sessions#mid'
-      post 'id' => 'sessions#id'
+
+      # /registrant/id path is hardcoded in Apache config for authentication with Estonian ID-card
+      # Client certificate is asked only on login form submission, therefore the path must be different from the one in
+      # `new_registrant_user_session_path` route, in case some other auth type will be implemented
+      post 'id' => 'sessions#create', as: :id_card_sign_in
     end
 
     resources :registrars, only: :show

lib/devise/models/id_card_authenticatable.rb

@@ -0,0 +1,7 @@
+module Devise
+  module Models
+    # Devise fails without this module (and model: false does not help)
+    module IdCardAuthenticatable
+    end
+  end
+end

lib/devise/strategies/id_card_authenticatable.rb

@@ -0,0 +1,49 @@
+module Devise
+  module Strategies
+    class IdCardAuthenticatable < Devise::Strategies::Authenticatable
+      def valid?
+        env['SSL_CLIENT_S_DN_CN'].present?
+      end
+
+      def authenticate!
+        resource = mapping.to
+        user = resource.find_by_id_card(id_card)
+
+        if user
+          success!(user)
+        else
+          fail
+        end
+      end
+
+      private
+
+      def id_card
+        id_card = IdCard.new
+        id_card.first_name = first_name
+        id_card.last_name = last_name
+        id_card.personal_code = personal_code
+        id_card.country_code = country_code
+        id_card
+      end
+
+      def first_name
+        env['SSL_CLIENT_S_DN_CN'].split(',').second.force_encoding('utf-8')
+      end
+
+      def last_name
+        env['SSL_CLIENT_S_DN_CN'].split(',').first.force_encoding('utf-8')
+      end
+
+      def personal_code
+        env['SSL_CLIENT_S_DN_CN'].split(',').last
+      end
+
+      def country_code
+        env['SSL_CLIENT_I_DN_C']
+      end
+    end
+  end
+end
+
+Warden::Strategies.add(:id_card_authenticatable, Devise::Strategies::IdCardAuthenticatable)

test/fixtures/users.yml

@@ -1,6 +1,7 @@
 api_bestnames:
   username: test_bestnames
   plain_text_password: testtest
+  identity_code: 1234
   type: ApiUser
   registrar: bestnames
   active: true

test/fixtures/white_ips.yml

@@ -0,0 +1,6 @@
+one:
+  registrar: bestnames
+  ipv4: 127.0.0.1
+  interfaces:
+    - <%= WhiteIp::REGISTRAR %>
+    - <%= WhiteIp::API %>

test/integration/registrant_area/sign_in/id_card_test.rb

@@ -0,0 +1,31 @@
+require 'test_helper'
+
+class RegistrantAreaIdCardSignInTest < ApplicationIntegrationTest
+  setup do
+    allow_business_registry_component_reach_server
+  end
+
+  def test_succeeds
+    post_via_redirect registrant_id_card_sign_in_path, nil,
+                      'SSL_CLIENT_S_DN_CN' => 'DOE,JOHN,1234',
+                      'SSL_CLIENT_I_DN_C' => 'US'
+
+    assert_response :ok
+    assert_equal registrant_root_path, path
+    assert_not_nil controller.current_registrant_user
+  end
+
+  def test_fails_when_certificate_is_absent
+    post_via_redirect registrant_id_card_sign_in_path, nil, 'SSL_CLIENT_S_DN_CN' => ''
+
+    assert_response :ok
+    assert_equal registrant_id_card_sign_in_path, path
+    assert_nil controller.current_registrant_user
+  end
+
+  private
+
+  def allow_business_registry_component_reach_server
+    WebMock.allow_net_connect!
+  end
+end

test/integration/registrar_area/sign_in/id_card_test.rb

@@ -0,0 +1,61 @@
+require 'test_helper'
+
+class RegistrarAreaIdCardSignInTest < ApplicationIntegrationTest
+  setup do
+    @user = users(:api_bestnames)
+  end
+
+  def test_signs_in_a_user_when_id_card_owner_is_found
+    assert_equal '1234', @user.identity_code
+
+    post_via_redirect registrar_id_card_sign_in_path, nil, 'SSL_CLIENT_S_DN_CN' => 'DOE,JOHN,1234'
+
+    assert_response :ok
+    assert_equal registrar_root_path, path
+    assert_not_nil controller.current_registrar_user
+  end
+
+  def test_does_not_sign_in_a_user_when_id_card_owner_is_not_found
+    post_via_redirect registrar_id_card_sign_in_path, nil,
+                      'SSL_CLIENT_S_DN_CN' => 'DOE,JOHN,unacceptable-personal-code'
+
+    assert_nil controller.current_registrar_user
+    assert_equal registrar_id_card_sign_in_path, path
+    assert_includes response.body, 'Failed to Login'
+  end
+
+  def test_does_not_sign_in_a_user_when_id_card_owner_is_found_but_ip_is_not_allowed
+    allow_access_to_sign_in_page
+    assert_equal '127.0.0.1', white_ips(:one).ipv4
+    assert_equal '1234', @user.identity_code
+
+    Setting.registrar_ip_whitelist_enabled = true
+
+    post registrar_id_card_sign_in_path, nil, 'SSL_CLIENT_S_DN_CN' => 'DOE,JOHN,1234',
+         'REMOTE_ADDR' => '127.0.0.2'
+
+    assert_equal registrar_id_card_sign_in_path, path
+    assert_equal 'Access denied from IP 127.0.0.2', response.body
+
+    get registrar_root_path
+    assert_redirected_to new_registrar_user_session_path
+
+    Setting.registrar_ip_whitelist_enabled = false
+  end
+
+  def test_does_not_sign_in_a_user_when_certificate_is_absent
+    post_via_redirect registrar_id_card_sign_in_path, nil, 'SSL_CLIENT_S_DN_CN' => ''
+
+    assert_nil controller.current_registrar_user
+    assert_equal registrar_id_card_sign_in_path, path
+  end
+
+  private
+
+  def allow_access_to_sign_in_page
+    another_registrar_white_ip = white_ips(:one).dup
+    another_registrar_white_ip.ipv4 = '127.0.0.2'
+    another_registrar_white_ip.registrar = registrars(:goodnames)
+    another_registrar_white_ip.save!
+  end
+end

test/lib/devise/strategies/id_card_authenticatable_test.rb

@@ -0,0 +1,13 @@
+require 'test_helper'
+
+class IdCardAuthenticatableTest < ActiveSupport::TestCase
+  def test_valid_when_id_card_data_is_present_in_env
+    strategy = Devise::Strategies::IdCardAuthenticatable.new({ 'SSL_CLIENT_S_DN_CN' => 'some' })
+    assert strategy.valid?
+  end
+
+  def test_not_valid_when_id_card_data_is_absent_in_env
+    strategy = Devise::Strategies::IdCardAuthenticatable.new({})
+    assert_not strategy.valid?
+  end
+end

test/models/api_user_test.rb

@@ -0,0 +1,18 @@
+require 'test_helper'
+
+class ApiUserTest < ActiveSupport::TestCase
+  setup do
+    @user = users(:api_bestnames)
+  end
+
+  def test_finds_user_by_id_card
+    id_card = IdCard.new
+    id_card.personal_code = 'one'
+
+    @user.update!(identity_code: 'one')
+    assert_equal @user, ApiUser.find_by_id_card(id_card)
+
+    @user.update!(identity_code: 'another')
+    assert_nil ApiUser.find_by_id_card(id_card)
+  end
+end

test/models/registrant_user/registrant_user_creation_test.rb

@@ -35,22 +35,4 @@ class RegistrantUserCreationTest < ActiveSupport::TestCase
     user = User.find_by(registrant_ident: 'EE-37710100070')
     assert_equal('JOHN SMITH', user.username)
   end
-
-  def test_find_or_create_by_idc_with_legacy_header_creates_a_user
-    header = '/C=EE/O=ESTEID/OU=authentication/CN=SMITH,JOHN,37710100070/SN=SMITH/GN=JOHN/serialNumber=37710100070'
-
-    RegistrantUser.find_or_create_by_idc_data(header, RegistrantUser::ACCEPTED_ISSUER)
-
-    user = User.find_by(registrant_ident: 'EE-37710100070')
-    assert_equal('JOHN SMITH', user.username)
-  end
-
-  def test_find_or_create_by_idc_with_rfc2253_header_creates_a_user
-    header = 'serialNumber=37710100070,GN=JOHN,SN=SMITH,CN=SMITH\\,JOHN\\,37710100070,OU=authentication,O=ESTEID,C=EE'
-
-    RegistrantUser.find_or_create_by_idc_data(header, RegistrantUser::ACCEPTED_ISSUER)
-
-    user = User.find_by(registrant_ident: 'EE-37710100070')
-    assert_equal('JOHN SMITH', user.username)
-  end
 end

test/models/registrant_user_test.rb

@@ -30,6 +30,34 @@ class RegistrantUserTest < ActiveSupport::TestCase
     assert_equal Country.new('US'), user.country
   end
 
+  def test_finding_by_id_card_creates_new_user_upon_first_sign_in
+    assert_not_equal 'US-5555', @user.registrant_ident
+    id_card = IdCard.new
+    id_card.first_name = 'John'
+    id_card.last_name = 'Doe'
+    id_card.personal_code = '5555'
+    id_card.country_code = 'US'
+
+    assert_difference 'RegistrantUser.count' do
+      RegistrantUser.find_by_id_card(id_card)
+    end
+
+    user = RegistrantUser.last
+    assert_equal 'US-5555', user.registrant_ident
+    assert_equal 'John Doe', user.username
+  end
+
+  def test_finding_by_id_card_reuses_existing_user_upon_subsequent_id_card_sign_ins
+    @user.update!(registrant_ident: 'US-5555')
+    id_card = IdCard.new
+    id_card.personal_code = '5555'
+    id_card.country_code = 'US'
+
+    assert_no_difference 'RegistrantUser.count' do
+      RegistrantUser.find_by_id_card(id_card)
+    end
+  end
+
   def test_queries_company_register_for_associated_companies
     assert_equal 'US-1234', @user.registrant_ident