Ensure ORCID authentication works within spaces

Author: fbacall

app/controllers/callbacks_controller.rb

@@ -1,5 +1,6 @@
 # The controller for callback actions
 class CallbacksController < Devise::OmniauthCallbacksController
+  include SpaceRedirect
 
   Devise.omniauth_configs.each do |provider, config|
     define_method(provider) do
@@ -41,18 +42,4 @@ def handle_callback(provider, config)
       redirect_to_space(after_sign_in_path_for(@user), space)
     end
   end
-
-  private
-
-  def redirect_to_space(path, space)
-    if space&.is_subdomain?
-      port_part = ''
-      port_part = ":#{request.port}" if (request.protocol == "http://" && request.port != 80) ||
-                                        (request.protocol == "https://" && request.port != 443)
-      redirect_to URI.join("#{request.protocol}#{space.host}#{port_part}", path).to_s, allow_other_host: true
-    else
-      redirect_to path
-    end
-  end
-
 end

app/controllers/concerns/space_redirect.rb

@@ -0,0 +1,16 @@
+module SpaceRedirect
+  extend ActiveSupport::Concern
+
+  private
+
+  def redirect_to_space(path, space)
+    if space&.is_subdomain?
+      port_part = ''
+      port_part = ":#{request.port}" if (request.protocol == "http://" && request.port != 80) ||
+        (request.protocol == "https://" && request.port != 443)
+      redirect_to URI.join("#{request.protocol}#{space.host}#{port_part}", path).to_s, allow_other_host: true
+    else
+      redirect_to path
+    end
+  end
+end

app/controllers/orcid_controller.rb

@@ -1,4 +1,6 @@
 class OrcidController < ApplicationController
+  include SpaceRedirect
+
   before_action :orcid_auth_enabled
   before_action :authenticate_user!
   before_action :set_oauth_client, only: [:authenticate, :callback]
@@ -9,12 +11,17 @@ class OrcidController < ApplicationController
   end
 
   def authenticate
-    redirect_to @oauth2_client.authorization_uri(scope: '/authenticate'), allow_other_host: true
+    params = Space.current_space&.default? ? {} : { state: "space_id:#{Space.current_space.id}" }
+    redirect_to @oauth2_client.authorization_uri(scope: '/authenticate', **params), allow_other_host: true
   end
 
   def callback
     @oauth2_client.authorization_code = params[:code]
     token = Rack::OAuth2::AccessToken::Bearer.new(access_token: @oauth2_client.access_token!)
+    if params[:state].present?
+      m = params[:state].match(/space_id:(\d*)/)
+      space = Space.find_by_id(m[1]) if m
+    end
     orcid = token.access_token&.raw_attributes['orcid']
     respond_to do |format|
       profile = current_user.profile
@@ -27,18 +34,29 @@ def callback
       else
         flash[:error] = t('orcid.authentication_failure')
       end
-      format.html { redirect_to current_user }
+      format.html { redirect_to_space(user_path(current_user), space) }
     end
   end
 
   private
 
+  def redirect_to_space(path, space)
+    if space&.is_subdomain?
+      port_part = ''
+      port_part = ":#{request.port}" if (request.protocol == "http://" && request.port != 80) ||
+        (request.protocol == "https://" && request.port != 443)
+      redirect_to URI.join("#{request.protocol}#{space.host}#{port_part}", path).to_s, allow_other_host: true
+    else
+      redirect_to path
+    end
+  end
+
   def set_oauth_client
     config = Rails.application.config.secrets.orcid
     @oauth2_client ||= Rack::OAuth2::Client.new(
       identifier: config[:client_id],
       secret: config[:secret],
-      redirect_uri: config[:redirect_uri].presence || orcid_callback_url,
+      redirect_uri: config[:redirect_uri].presence || orcid_callback_url(host: TeSS::Config.base_uri.host),
       authorization_endpoint: '/oauth/authorize',
       token_endpoint: '/oauth/token',
       host: config[:host].presence || (Rails.env.production? ? 'orcid.org' : 'sandbox.orcid.org')

app/views/users/show.html.erb

@@ -51,7 +51,10 @@
         <% else %>
           <%= orcid_link(@user.profile) %>
         <% end %>
-        <% if TeSS::Config.orcid_authentication_enabled? && current_user == @user && [email protected]_authenticated? %>
+        <% if TeSS::Config.orcid_authentication_enabled? &&
+            current_user == @user &&
+            [email protected]_authenticated? &&
+            space_supports_omniauth? %>
           <%= button_to t(@user.profile.orcid.blank? ? 'orcid.link' : 'orcid.authenticate'), authenticate_orcid_path, class: 'btn btn-default' %>
         <% end %>
       </p>

test/controllers/orcid_controller_test.rb

@@ -9,6 +9,23 @@ class OrcidControllerTest < ActionController::TestCase
     post :authenticate
 
     assert_redirected_to /https:\/\/sandbox\.orcid\.org\/oauth\/authorize\?.+/
+    params = Rack::Utils.parse_query(URI.parse(response.location).query)
+    assert_equal "#{TeSS::Config.base_url}/orcid/callback", params['redirect_uri']
+    assert_nil params['state']
+  end
+
+  test 'authenticating orcid in space uses root app redirect URI and sets space state' do
+    plant_space = spaces(:plants)
+    with_host(plant_space.host) do
+      sign_in users(:regular_user)
+
+      post :authenticate
+
+      assert_redirected_to /https:\/\/sandbox\.orcid\.org\/oauth\/authorize\?.+/
+      params = Rack::Utils.parse_query(URI.parse(response.location).query)
+      assert_equal "#{TeSS::Config.base_url}/orcid/callback", params['redirect_uri']
+      assert_equal "space_id:#{plant_space.id}", params['state']
+    end
   end
 
   test 'do not authenticate orcid if user not logged-in' do
@@ -148,4 +165,61 @@ class OrcidControllerTest < ActionController::TestCase
       end
     end
   end
+
+  test 'redirect to subdomain space in callback' do
+    space = spaces(:astro)
+    space.update!(host: 'space.example.com')
+    mock_images
+    user = users(:regular_user)
+    assert user.profile.orcid.blank?
+    sign_in user
+
+    VCR.use_cassette('orcid/get_token_free_orcid') do
+      get :callback, params: { code: '123xyz', state: "space_id:#{space.id}" }
+    end
+
+    profile = user.profile.reload
+    assert_equal '0009-0006-0987-5702', profile.orcid
+    assert profile.orcid_authenticated?
+    assert_redirected_to user_url(user, host: 'space.example.com')
+    assert response.headers['Location'].starts_with?('http://space.example.com/users/')
+    assert flash[:error].blank?
+  end
+
+  test 'do not redirect to non-subdomain space in callback' do
+    space = spaces(:astro)
+    space.update!(host: 'space.golf.com')
+    mock_images
+    user = users(:regular_user)
+    assert user.profile.orcid.blank?
+    sign_in user
+
+    VCR.use_cassette('orcid/get_token_free_orcid') do
+      get :callback, params: { code: '123xyz', state: "space_id:#{space.id}" }
+    end
+
+    profile = user.profile.reload
+    assert_equal '0009-0006-0987-5702', profile.orcid
+    assert profile.orcid_authenticated?
+    assert_redirected_to user
+    refute response.headers['Location'].starts_with?('http://space.golf.com/users/')
+    assert flash[:error].blank?
+  end
+
+  test 'ignore bad space when redirecting in callback' do
+    mock_images
+    user = users(:regular_user)
+    assert user.profile.orcid.blank?
+    sign_in user
+
+    VCR.use_cassette('orcid/get_token_free_orcid') do
+      get :callback, params: { code: '123xyz', state: "space_id:banana🍌" }
+    end
+
+    profile = user.profile.reload
+    assert_equal '0009-0006-0987-5702', profile.orcid
+    assert profile.orcid_authenticated?
+    assert_redirected_to user
+    assert flash[:error].blank?
+  end
 end

test/controllers/users_controller_test.rb

@@ -531,6 +531,7 @@ class UsersControllerTest < ActionController::TestCase
 
       assert_response :success
       assert_select '#sidebar button', text: 'Authenticate your ORCID', count: 0
+      assert_select '#sidebar button', text: 'Link your ORCID', count: 0
     end
   end
 
@@ -543,6 +544,7 @@ class UsersControllerTest < ActionController::TestCase
 
     assert_response :success
     assert_select '#sidebar button', text: 'Authenticate your ORCID', count: 0
+    assert_select '#sidebar button', text: 'Link your ORCID', count: 0
   end
 
   test 'should not show authenticate orcid button if already authenticated' do
@@ -554,6 +556,7 @@ class UsersControllerTest < ActionController::TestCase
 
     assert_response :success
     assert_select '#sidebar button', text: 'Authenticate your ORCID', count: 0
+    assert_select '#sidebar button', text: 'Link your ORCID', count: 0
   end
 
   test 'should show material and event in trainer page as bioschemas JSON-LD' do
@@ -600,4 +603,39 @@ class UsersControllerTest < ActionController::TestCase
     assert_not_nil external_resource
     assert_equal external_resource.url, json_event['mentions'].first['url']
   end
+
+  test 'should show authenticate orcid button on subdomain space' do
+    space = spaces(:astro)
+    space.update!(host: 'space.example.com')
+    with_host(space.host) do
+      user = users(:private_user)
+      assert user.profile.orcid.present?
+      refute user.profile.orcid_authenticated?
+
+      sign_in user
+
+      get :show, params: { id: user }
+
+      assert_response :success
+      assert_select '#sidebar button', text: 'Authenticate your ORCID'
+    end
+  end
+
+  test 'should not show authenticate orcid button on non-subdomain space' do
+    space = spaces(:astro)
+    space.update!(host: 'space.golf.com')
+    with_host(space.host) do
+      user = users(:private_user)
+      assert user.profile.orcid.present?
+      refute user.profile.orcid_authenticated?
+
+      sign_in user
+
+      get :show, params: { id: user }
+
+      assert_response :success
+      assert_select '#sidebar button', text: 'Authenticate your ORCID', count: 0
+      assert_select '#sidebar button', text: 'Link your ORCID', count: 0
+    end
+  end
 end

test/integration/omniauth_test.rb

@@ -418,7 +418,7 @@ class OmniauthTest < ActionDispatch::IntegrationTest
     post user_oidc_omniauth_authorize_url(space_id: space.id)
     follow_redirect! # OmniAuth redirect
     assert_equal "http://space.example.com/users/aaf_user/edit", response.headers['Location']
-    end
+  end
 
   test 'authentication does not redirect user to entirely different domain' do
     space = spaces(:astro)