Move ORCID authentication into its own controller

Add error handling

Author: fbacall

app/controllers/application_controller.rb

@@ -48,7 +48,7 @@ def handle_error(status_code = 500, message = nil)
     status_code = status_code.to_i
     @message = message
     respond_to do |format|
-      format.html  { render 'static/error', status: status_code }
+      format.html { render 'static/error', status: status_code }
       format.json { render json: { error: { message: message, code: status_code } }, status: status_code }
       format.json_api { render json: { error: { message: message, code: status_code } }, status: status_code }
       format.any { head status_code }

…rollers/concerns/orcid_authentication.rb app/controllers/orcid_controller.rbapp/controllers/concerns/orcid_authentication.rb renamed to app/controllers/orcid_controller.rb app/controllers/concerns/orcid_authentication.rb renamed to app/controllers/orcid_controller.rb

@@ -1,15 +1,17 @@
-module OrcidAuthentication
-  extend ActiveSupport::Concern
+class OrcidController < ApplicationController
+  before_action :authenticate_user!
+  before_action :set_oauth_client, only: [:authenticate, :callback]
 
-  included do
-    before_action :set_orcid_oauth_client, only: [:authenticate_orcid, :orcid_callback]
+  # Faraday::ParsingError occurs in rack-oauth2 if the response does not contain JSON
+  rescue_from(Rack::OAuth2::Client::Error, Faraday::ParsingError) do
+    handle_error(:unprocessable_entity, t('orcid.error'))
   end
 
-  def authenticate_orcid
+  def authenticate
     redirect_to @oauth2_client.authorization_uri(scope: '/authenticate'), allow_other_host: true
   end
 
-  def orcid_callback
+  def callback
     @oauth2_client.authorization_code = params[:code]
     token = Rack::OAuth2::AccessToken::Bearer.new(access_token: @oauth2_client.access_token!)
     orcid = token.access_token&.raw_attributes['orcid']
@@ -26,7 +28,7 @@ def orcid_callback
 
   private
 
-  def set_orcid_oauth_client
+  def set_oauth_client
     config = Rails.application.config.secrets.orcid
     @oauth2_client ||= Rack::OAuth2::Client.new(
       identifier: config[:client_id],

app/controllers/users_controller.rb

@@ -9,7 +9,6 @@ class UsersController < ApplicationController
   before_action :check_profile_id, only: [:update]
 
   include ActionView::Helpers::TextHelper
-  include OrcidAuthentication
 
   # GET /users
   # GET /users.json

app/views/users/show.html.erb

@@ -52,7 +52,7 @@
         <% else %>
           <%= orcid_link(@user.profile) %>
           <% if current_user == @user && !@user.profile.orcid_authenticated? %>
-            <%= button_to t('orcid.authenticate'), orcid_authenticate_path, class: 'btn btn-default' %>
+            <%= button_to t('orcid.authenticate'), authenticate_orcid_path, class: 'btn btn-default' %>
           <% end %>
         <% end %>
       </p>

config/routes.rb

@@ -177,8 +177,8 @@
 
   get 'up' => 'health_check#show'
 
-  post 'orcid/authenticate' => 'users#authenticate_orcid'
-  get 'orcid/callback' => 'users#orcid_callback'
+  post 'orcid/authenticate' => 'orcid#authenticate', as: :authenticate_orcid
+  get 'orcid/callback' => 'orcid#callback', as: :orcid_callback
 
   # The priority is based upon order of creation: first created -> highest priority.
   # See how all your routes lay out with "rake routes".

test/controllers/orcid_controller_test.rb

@@ -3,4 +3,103 @@
 class OrcidControllerTest < ActionController::TestCase
   include Devise::Test::ControllerHelpers
 
+  test 'authenticate orcid logged-in user' do
+    sign_in users(:regular_user)
+
+    post :authenticate
+
+    assert_redirected_to /https:\/\/sandbox\.orcid\.org\/oauth\/authorize\?.+/
+  end
+
+  test 'do not authenticate orcid if user not logged-in' do
+    post :authenticate
+
+    assert_redirected_to new_user_session_path
+  end
+
+  test 'handle callback and assign orcid if free' do
+    mock_images
+    user = users(:regular_user)
+    sign_in user
+
+    VCR.use_cassette('orcid/get_token_free_orcid') do
+      get :callback, params: { code: '123xyz' }
+    end
+
+    profile = user.profile.reload
+    assert_equal '0009-0006-0987-5702', profile.orcid
+    assert profile.orcid_authenticated?
+    assert_redirected_to user
+  end
+
+  test 'handle callback and assign orcid if unauthenticated' do
+    mock_images
+    user = users(:regular_user)
+    sign_in user
+
+    VCR.use_cassette('orcid/get_token_unauth_orcid') do
+      get :callback, params: { code: '123xyz' }
+    end
+
+    profile = user.profile.reload
+    assert_equal '0000-0002-0048-3300', profile.orcid
+    assert profile.orcid_authenticated?
+    assert_redirected_to user
+  end
+
+  test 'handle callback but do not assign orcid if already used' do
+    mock_images
+    user = users(:regular_user)
+    sign_in user
+
+    VCR.use_cassette('orcid/get_token_existing_orcid') do
+      get :callback, params: { code: '123xyz' }
+    end
+
+    profile = user.profile.reload
+    assert profile.orcid.blank?
+    refute profile.orcid_authenticated?
+    assert_redirected_to user
+    assert_includes flash[:error], 'ORCID has already been'
+  end
+
+  test 'do not handle callback if not logged-in' do
+    mock_images
+
+    get :callback, params: { code: '123xyz' }
+
+    assert_redirected_to new_user_session_path
+  end
+
+  test 'handle unauth error during callback' do
+    mock_images
+    user = users(:regular_user)
+    sign_in user
+
+    VCR.use_cassette('orcid/error_unauth') do
+      get :callback, params: { code: '123xyz' }
+    end
+
+    assert_response :unprocessable_entity
+    assert_select '#error-message', text: /ORCID/
+    profile = user.profile.reload
+    assert profile.orcid.blank?
+    refute profile.orcid_authenticated?
+  end
+
+  test 'handle unexpected error during callback' do
+    mock_images
+    user = users(:regular_user)
+    sign_in user
+
+    VCR.use_cassette('orcid/error_500') do
+      get :callback, params: { code: '123xyz' }
+    end
+
+    assert_response :unprocessable_entity
+    assert_select '#error-message', text: /ORCID/
+    profile = user.profile.reload
+    assert profile.orcid.blank?
+    refute profile.orcid_authenticated?
+  end
 end

test/controllers/users_controller_test.rb

@@ -491,71 +491,38 @@ class UsersControllerTest < ActionController::TestCase
     end
   end
 
-  test 'authenticate orcid logged-in user' do
-    sign_in users(:regular_user)
-
-    post :authenticate_orcid
-
-    assert_redirected_to /https:\/\/sandbox\.orcid\.org\/oauth\/authorize\?.+/
-  end
-
-  test 'do not authenticate orcid if user not logged-in' do
-    post :authenticate_orcid
-
-    assert_redirected_to new_user_session_path
-  end
+  test 'should show authenticate orcid button if own profile' do
+    user = users(:private_user)
+    assert user.profile.orcid.present?
+    refute user.profile.orcid_authenticated?
 
-  test 'handle callback and assign orcid if free' do
-    mock_images
-    user = users(:regular_user)
     sign_in user
 
-    VCR.use_cassette('orcid/get_token_free_orcid') do
-      get :orcid_callback, params: { code: '123xyz' }
-    end
-
-    profile = user.profile.reload
-    assert_equal '0009-0006-0987-5702', profile.orcid
-    assert profile.orcid_authenticated?
-    assert_redirected_to user
-  end
-
-  test 'handle callback and assign orcid if unauthenticated' do
-    mock_images
-    user = users(:regular_user)
-    sign_in user
-
-    VCR.use_cassette('orcid/get_token_unauth_orcid') do
-      get :orcid_callback, params: { code: '123xyz' }
-    end
+    get :show, params: { id: user }
 
-    profile = user.profile.reload
-    assert_equal '0000-0002-0048-3300', profile.orcid
-    assert profile.orcid_authenticated?
-    assert_redirected_to user
+    assert_response :success
+    assert_select '#sidebar button', text: 'Authenticate your ORCID'
   end
 
-  test 'handle callback but do not assign orcid if already used' do
-    mock_images
-    user = users(:regular_user)
-    sign_in user
+  test 'should not show authenticate orcid button if not own profile' do
+    user = users(:private_user)
+    assert user.profile.orcid.present?
+    refute user.profile.orcid_authenticated?
 
-    VCR.use_cassette('orcid/get_token_existing_orcid') do
-      get :orcid_callback, params: { code: '123xyz' }
-    end
+    get :show, params: { id: user }
 
-    profile = user.profile.reload
-    assert profile.orcid.blank?
-    refute profile.orcid_authenticated?
-    assert_redirected_to user
-    assert_includes flash[:error], 'ORCID has already been'
+    assert_response :success
+    assert_select '#sidebar button', text: 'Authenticate your ORCID', count: 0
   end
 
-  test 'do not handle callback if not logged-in' do
-    mock_images
+  test 'should not show authenticate orcid button if already authenticated' do
+    user = users(:trainer_user)
+    assert user.profile.orcid.present?
+    assert user.profile.orcid_authenticated?
 
-    get :orcid_callback, params: { code: '123xyz' }
+    get :show, params: { id: user }
 
-    assert_redirected_to new_user_session_path
+    assert_response :success
+    assert_select '#sidebar button', text: 'Authenticate your ORCID', count: 0
   end
 end

test/helpers/users_helper_test.rb

@@ -5,16 +5,23 @@ class UsersHelperTest < ActionView::TestCase
 
   test 'displays authenticated orcid link' do
     profile = profiles(:trainer_one_profile)
+    assert profile.orcid.present?
+    assert profile.orcid_authenticated?
+
     l = orcid_link(profile)
+
     assert_includes l, 'ORCID-iD_icon_vector.svg'
     assert_not_includes l, 'Unauthenticated'
   end
 
   test 'displays unauthenticated orcid link' do
     profile = profiles(:trainer_two_profile)
+    assert profile.orcid.present?
+    refute profile.orcid_authenticated?
+
     l = orcid_link(profile)
+
     assert_includes l, 'ORCID-iD_icon_unauth_vector.svg'
     assert_includes l, 'Unauthenticated'
   end
-
 end