From 8edca598bb1a605d94d836764dd8965b747aab79 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 13 Oct 2025 13:11:40 +0100 Subject: [PATCH 01/12] Distinguish authenticated/unauthenticated ORCID IDs. #783 --- .../images/ORCID-iD_icon_unauth_vector.svg | 5 +++++ app/assets/images/ORCID-iD_icon_vector.svg | 4 ++++ app/helpers/users_helper.rb | 15 ++++++++++++++ app/views/trainers/show.html.erb | 2 +- app/views/users/show.html.erb | 2 +- ...523_add_orcid_authenticated_to_profiles.rb | 5 +++++ db/schema.rb | 3 ++- test/fixtures/profiles.yml | 4 +++- test/helpers/users_helper_test.rb | 20 +++++++++++++++++++ 9 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 app/assets/images/ORCID-iD_icon_unauth_vector.svg create mode 100644 app/assets/images/ORCID-iD_icon_vector.svg create mode 100644 db/migrate/20251013115523_add_orcid_authenticated_to_profiles.rb create mode 100644 test/helpers/users_helper_test.rb diff --git a/app/assets/images/ORCID-iD_icon_unauth_vector.svg b/app/assets/images/ORCID-iD_icon_unauth_vector.svg new file mode 100644 index 000000000..02b90a8eb --- /dev/null +++ b/app/assets/images/ORCID-iD_icon_unauth_vector.svg @@ -0,0 +1,5 @@ + + + + + diff --git a/app/assets/images/ORCID-iD_icon_vector.svg b/app/assets/images/ORCID-iD_icon_vector.svg new file mode 100644 index 000000000..375b457a4 --- /dev/null +++ b/app/assets/images/ORCID-iD_icon_vector.svg @@ -0,0 +1,4 @@ + + + + diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 17e2ccbbc..cbd5c6b90 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -5,4 +5,19 @@ def self.user_profile_resource_limit 30 end + def orcid_link(profile, *opts) + content_tag(:span) do + if profile.orcid_authenticated? + concat image_tag('ORCID-iD_icon_vector.svg', size: 16) + concat ' ' + concat external_link(profile.orcid.sub(OrcidValidator::ORCID_PREFIX, ''), profile.orcid, *opts) + else profile.orcid.present? + concat image_tag('ORCID-iD_icon_unauth_vector.svg', size: 16) + concat ' ' + concat external_link(profile.orcid.sub(OrcidValidator::ORCID_PREFIX, ''), profile.orcid, *opts) + concat ' (Unauthenticated)' + end + end + end + end diff --git a/app/views/trainers/show.html.erb b/app/views/trainers/show.html.erb index 900bb94da..c9d5ad2b8 100644 --- a/app/views/trainers/show.html.erb +++ b/app/views/trainers/show.html.erb @@ -21,7 +21,7 @@ <% if @trainer.orcid.present? %>
- <%= external_link @trainer.orcid, @trainer.orcid, class: 'h5', target: '_blank', rel: 'noopener', track: true %> + <%= orcid_link(@trainer, class: 'h5', track: true) %>
<% end %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 91be9917a..5d171eae2 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -50,7 +50,7 @@ <% if @user.profile.orcid.blank? %> None specified <% else %> - <%= link_to @user.profile.orcid, @user.profile.orcid, rel: 'nofollow', target: '_blank' %> + <%= orcid_link(@user.profile) %> <% end %>

diff --git a/db/migrate/20251013115523_add_orcid_authenticated_to_profiles.rb b/db/migrate/20251013115523_add_orcid_authenticated_to_profiles.rb new file mode 100644 index 000000000..cddda1127 --- /dev/null +++ b/db/migrate/20251013115523_add_orcid_authenticated_to_profiles.rb @@ -0,0 +1,5 @@ +class AddOrcidAuthenticatedToProfiles < ActiveRecord::Migration[7.2] + def change + add_column :profiles, :orcid_authenticated, :boolean, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index a6b3aa4f3..9e052f859 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_03_25_151745) do +ActiveRecord::Schema[7.2].define(version: 2025_10_13_115523) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -452,6 +452,7 @@ t.string "social_media", default: [], array: true t.string "type", default: "Profile" t.string "fields", default: [], array: true + t.boolean "orcid_authenticated", default: false t.index ["slug"], name: "index_profiles_on_slug", unique: true end diff --git a/test/fixtures/profiles.yml b/test/fixtures/profiles.yml index 483bd2f39..acef0a432 100644 --- a/test/fixtures/profiles.yml +++ b/test/fixtures/profiles.yml @@ -68,6 +68,7 @@ trainer_one_profile: expertise_technical: ['python', 'R'] public: true type: Trainer + orcid_authenticated: true trainer_two_profile: user: private_user @@ -76,13 +77,14 @@ trainer_two_profile: image_url: email: samael@research.org website: '' - orcid: '' + orcid: https://orcid.org/0000-0002-0048-3300 description: none location: 'Sydney, Australia' experience: much expertise_technical: ['java', 'python', 'ruby'] public: false type: Profile + orcid_authenticated: false admin_trainer_profile: user: admin_trainer diff --git a/test/helpers/users_helper_test.rb b/test/helpers/users_helper_test.rb new file mode 100644 index 000000000..d5e42aafb --- /dev/null +++ b/test/helpers/users_helper_test.rb @@ -0,0 +1,20 @@ +require 'test_helper' + +class UsersHelperTest < ActionView::TestCase + include ApplicationHelper + + test 'displays authenticated orcid link' do + profile = profiles(:trainer_one_profile) + 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) + l = orcid_link(profile) + assert_includes l, 'ORCID-iD_icon_unauth_vector.svg' + assert_includes l, 'Unauthenticated' + end + +end From cd566fd796f983c6844775d7adb6143107405922 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Fri, 28 Nov 2025 16:28:29 +0000 Subject: [PATCH 02/12] Button to allow ORCID authentication * Change how ORCIDs are stored in DB (`0000-`...) --- .../concerns/orcid_authentication.rb | 40 +++++++++++ app/controllers/users_controller.rb | 1 + app/helpers/users_helper.rb | 4 +- app/models/profile.rb | 23 +++++-- .../partials/_trainer_schemaorg.html.erb | 2 +- app/views/users/show.html.erb | 3 + config/locales/en.yml | 6 ++ config/routes.rb | 3 + config/secrets.example.yml | 4 ++ ...0251128160923_update_orcids_in_profiles.rb | 14 ++++ db/schema.rb | 2 +- test/config/test_secrets.yml | 3 + test/controllers/orcid_controller_test.rb | 6 ++ test/controllers/users_controller_test.rb | 68 +++++++++++++++++++ test/fixtures/profiles.yml | 4 +- test/integration/tracking_test.rb | 2 +- test/models/profile_test.rb | 55 +++++++++++++-- test/models/user_test.rb | 2 +- .../orcid/get_token_existing_orcid.yml | 56 +++++++++++++++ .../orcid/get_token_free_orcid.yml | 56 +++++++++++++++ .../orcid/get_token_unauth_orcid.yml | 56 +++++++++++++++ 21 files changed, 390 insertions(+), 20 deletions(-) create mode 100644 app/controllers/concerns/orcid_authentication.rb create mode 100644 db/migrate/20251128160923_update_orcids_in_profiles.rb create mode 100644 test/controllers/orcid_controller_test.rb create mode 100644 test/vcr_cassettes/orcid/get_token_existing_orcid.yml create mode 100644 test/vcr_cassettes/orcid/get_token_free_orcid.yml create mode 100644 test/vcr_cassettes/orcid/get_token_unauth_orcid.yml diff --git a/app/controllers/concerns/orcid_authentication.rb b/app/controllers/concerns/orcid_authentication.rb new file mode 100644 index 000000000..3f498c557 --- /dev/null +++ b/app/controllers/concerns/orcid_authentication.rb @@ -0,0 +1,40 @@ +module OrcidAuthentication + extend ActiveSupport::Concern + + included do + before_action :set_oauth2_client, only: [:authenticate_orcid, :orcid_callback] + end + + def authenticate_orcid + redirect_to @oauth2_client.authorization_uri(scope: '/authenticate'), allow_other_host: true + end + + def orcid_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'] + respond_to do |format| + profile = current_user.profile + if profile.authenticate_orcid(orcid) + flash[:notice] = t('orcid.authentication_success') + else + flash[:error] = profile.errors.full_messages.join(', ') + end + format.html { redirect_to current_user } + end + end + + private + + def set_oauth2_client + config = Rails.application.config.secrets.orcid + @oauth2_client ||= Rack::OAuth2::Client.new( + identifier: config[:client_id], + secret: config[:secret], + redirect_uri: 'https://localhost:3001/orcid/callback',#config[:redirect_uri].presence || orcid_callback_url, + authorization_endpoint: '/oauth/authorize', + token_endpoint: '/oauth/token', + host: config[:host].presence || (Rails.env.production? ? 'orcid.org' : 'sandbox.orcid.org') + ) + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 36a2e73f9..dceb1fa34 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -9,6 +9,7 @@ class UsersController < ApplicationController before_action :check_profile_id, only: [:update] include ActionView::Helpers::TextHelper + include OrcidAuthentication # GET /users # GET /users.json diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index cbd5c6b90..08543c56a 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -10,11 +10,11 @@ def orcid_link(profile, *opts) if profile.orcid_authenticated? concat image_tag('ORCID-iD_icon_vector.svg', size: 16) concat ' ' - concat external_link(profile.orcid.sub(OrcidValidator::ORCID_PREFIX, ''), profile.orcid, *opts) + concat external_link(profile.orcid, profile.orcid_url, *opts) else profile.orcid.present? concat image_tag('ORCID-iD_icon_unauth_vector.svg', size: 16) concat ' ' - concat external_link(profile.orcid.sub(OrcidValidator::ORCID_PREFIX, ''), profile.orcid, *opts) + concat external_link(profile.orcid, profile.orcid_url, *opts) concat ' (Unauthenticated)' end end diff --git a/app/models/profile.rb b/app/models/profile.rb index 70da09ec4..bcf0f0b94 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -25,6 +25,10 @@ def full_name "#{firstname} #{surname}".strip end + def orcid_url + "#{OrcidValidator::ORCID_PREFIX}#{orcid}" + end + def merge(*others) Profile.transaction do attrs = attributes @@ -43,16 +47,23 @@ def merge(*others) end end + def authenticate_orcid(orcid) + existing = Profile.where(orcid: orcid, orcid_authenticated: true) + if existing.any? + errors.add(:orcid, :orcid_taken) + false + else + self.orcid = orcid + self.orcid_authenticated = true + self.save + end + end + private def normalize_orcid return if orcid.blank? - self.orcid = orcid.strip - if orcid =~ OrcidValidator::ORCID_ID_REGEX - self.orcid = "#{OrcidValidator::ORCID_PREFIX}#{orcid}" - elsif orcid.start_with?(OrcidValidator::ORCID_DOMAIN_REGEX) - self.orcid = orcid.sub(OrcidValidator::ORCID_DOMAIN_REGEX, OrcidValidator::ORCID_PREFIX) - end + self.orcid = orcid.strip.sub(OrcidValidator::ORCID_DOMAIN_REGEX, '') end def check_public diff --git a/app/views/trainers/partials/_trainer_schemaorg.html.erb b/app/views/trainers/partials/_trainer_schemaorg.html.erb index 356315fe8..39a0e7ee9 100644 --- a/app/views/trainers/partials/_trainer_schemaorg.html.erb +++ b/app/views/trainers/partials/_trainer_schemaorg.html.erb @@ -13,7 +13,7 @@ <%= content_tag 'span', langs, { itemprop: 'knowsLanguage', content: langs, class: 'schemaorg-element' } %> <% end %> <%= content_tag :span, trainer.website, { itemprop: 'url', content: trainer.website, class: 'schemaorg-element' } %> - <%= content_tag :span, trainer.orcid, { itemprop: 'identifier', content: trainer.orcid, class: 'schemaorg-element' } %> + <%= content_tag :span, trainer.orcid_url, { itemprop: 'identifier', content: trainer.orcid_url, class: 'schemaorg-element' } %> <% if trainer.image_url and not trainer.image_url.empty? %> <%= content_tag :span, trainer.image_url, { itemprop: 'image', content: trainer.image_url, class: 'shemaorg-element' } %> <% end %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 5d171eae2..9c0fdacd7 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -51,6 +51,9 @@ None specified <% else %> <%= orcid_link(@user.profile) %> + <% if current_user == @user && !@user.profile.orcid_authenticated? %> + <%= link_to t('orcid.authenticate'), orcid_authenticate_path, method: :post, class: 'btn btn-default' %> + <% end %> <% end %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index c72613a1a..349ea545d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -839,6 +839,8 @@ en: 422: "The request you sent was well-formed but the change you wanted was rejected (422 Unprocessable Entity)." 404: "The requested page could not be found - you may have mistyped the address or the page may have moved (404 Not Found)." 406: "The requested format is not available (406 Not Acceptable)." + messages: + orcid_taken: 'has already been linked to another profile.' warnings: link_broken: > %{site_name} has been unable to access this %{resource_type}'s URL since %{fail_date} - the page may have been moved. @@ -1114,3 +1116,7 @@ en: title: What are spaces? description: | Spaces are customizable, community-managed sub-portals within %{site_name}, each with their own catalogue of training content. + orcid: + error: 'An error occurred whilst trying to authenticate your ORCID.' + authenticate: 'Authenticate your ORCID' + authentication_success: 'You have successfully authenticated your ORCID.' \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index ea28af4e4..fc076be11 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -179,6 +179,9 @@ match 'oai-pmh', to: "oai#index", via: [:get, :post] + post 'orcid/authenticate' => 'users#authenticate_orcid' + get 'orcid/callback' => 'users#orcid_callback' + # The priority is based upon order of creation: first created -> highest priority. # See how all your routes lay out with "rake routes". diff --git a/config/secrets.example.yml b/config/secrets.example.yml index 018b0439e..51f0fa8f1 100644 --- a/config/secrets.example.yml +++ b/config/secrets.example.yml @@ -38,6 +38,10 @@ external_api_keys: &external_api_keys password: gpt_api_key: willma_api_key: + orcid: + client_id: + secret: + host: #Internal config development: diff --git a/db/migrate/20251128160923_update_orcids_in_profiles.rb b/db/migrate/20251128160923_update_orcids_in_profiles.rb new file mode 100644 index 000000000..486d59dd1 --- /dev/null +++ b/db/migrate/20251128160923_update_orcids_in_profiles.rb @@ -0,0 +1,14 @@ +class UpdateOrcidsInProfiles < ActiveRecord::Migration[7.2] + ORCID_PREFIX = 'https://orcid.org/'.freeze + def up + Profile.where('orcid IS NOT NULL').find_each do |profile| + profile.update_column(:orcid, profile.orcid.gsub(ORCID_PREFIX, '')) + end + end + + def down + Profile.where('orcid IS NOT NULL').find_each do |profile| + profile.update_column(:orcid, "#{ORCID_PREFIX}#{profile.orcid}") unless profile.orcid.start_with?(ORCID_PREFIX) + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 9e052f859..f4395df2f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_10_13_115523) do +ActiveRecord::Schema[7.2].define(version: 2025_11_28_160923) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/test/config/test_secrets.yml b/test/config/test_secrets.yml index d80a913e1..54beb7019 100644 --- a/test/config/test_secrets.yml +++ b/test/config/test_secrets.yml @@ -19,3 +19,6 @@ eventbrite_api_v3: fairsharing: username: tess-test-bot password: abc123 +orcid: + client_id: test + secret: test diff --git a/test/controllers/orcid_controller_test.rb b/test/controllers/orcid_controller_test.rb new file mode 100644 index 000000000..17470eaaf --- /dev/null +++ b/test/controllers/orcid_controller_test.rb @@ -0,0 +1,6 @@ +require 'test_helper' + +class OrcidControllerTest < ActionController::TestCase + include Devise::Test::ControllerHelpers + +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 5b41abc13..d1172ae91 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -490,4 +490,72 @@ class UsersControllerTest < ActionController::TestCase assert_select '.masonry-brick-heading h4', text: 'Learn about plants' 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 '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 + + 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 :orcid_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 :orcid_callback, params: { code: '123xyz' } + + assert_redirected_to new_user_session_path + end end diff --git a/test/fixtures/profiles.yml b/test/fixtures/profiles.yml index acef0a432..ac7d208e5 100644 --- a/test/fixtures/profiles.yml +++ b/test/fixtures/profiles.yml @@ -58,7 +58,7 @@ trainer_one_profile: image_url: https://library.brown.edu/info/wp-content/uploads/2019/12/Carberry2-150x137.jpg email: jcarberry@research.org website: https://library.brown.edu/info/hay/carberry/ # demo page - orcid: https://orcid.org/000-0002-1825-0097 # demo orcid account + orcid: 0000-0002-1825-0097 # demo orcid account description: > Josiah Carberry is a fictitious person. This account is used as a demonstration account by ORCID, CrossRef and others who wish to demonstrate the @@ -77,7 +77,7 @@ trainer_two_profile: image_url: email: samael@research.org website: '' - orcid: https://orcid.org/0000-0002-0048-3300 + orcid: 0000-0002-0048-3300 description: none location: 'Sydney, Australia' experience: much diff --git a/test/integration/tracking_test.rb b/test/integration/tracking_test.rb index add1b7fbf..0be17857d 100644 --- a/test/integration/tracking_test.rb +++ b/test/integration/tracking_test.rb @@ -30,7 +30,7 @@ class TrackingTest < ActionDispatch::IntegrationTest get trainer_path(trainer) - assert_select 'a[href=?]', trainer.orcid do + assert_select 'a[href=?]', trainer.orcid_url do assert_select '[data-trackable]' assert_select '[data-trackable-id]', count: 0 end diff --git a/test/models/profile_test.rb b/test/models/profile_test.rb index 0a2c62f54..22720c4dd 100644 --- a/test/models/profile_test.rb +++ b/test/models/profile_test.rb @@ -21,7 +21,7 @@ class ProfileTest < ActiveSupport::TestCase assert_equal 'Space', profile.firstname assert_equal 'Spaceson', profile.surname assert_equal 'http://website.com', profile.website - assert_equal 'https://orcid.org/0000-0002-1825-0097', profile.orcid + assert_equal '0000-0002-1825-0097', profile.orcid end test 'validates orcid' do @@ -46,11 +46,8 @@ class ProfileTest < ActiveSupport::TestCase # check validation of valid orcid - fixes non-secure scheme assert profile.update(orcid: 'http://orcid.org/0000-0002-1825-0097') - assert_equal 'https://orcid.org/0000-0002-1825-0097', profile.orcid - - # check validation of invalid orcid - scheme and host only - refute profile.update(orcid: 'https://orcid.org/') - assert_equal "ORCID isn't a valid ORCID identifier", profile.errors.full_messages_for(:orcid).first + assert_equal '0000-0002-1825-0097', profile.orcid + assert_equal 'https://orcid.org/0000-0002-1825-0097', profile.orcid_url # check validation of invalid orcid, preserves original value refute profile.update(orcid: 'some junk') @@ -91,4 +88,50 @@ class ProfileTest < ActiveSupport::TestCase assert profile.update(website: 'http://slowhost.com') refute profile.errors.added?(:website, 'is not accessible') end + + test 'assigns authenticated orcid' do + profile = users(:regular_user).profile + refute profile.orcid.present? + refute profile.orcid_authenticated + + assert profile.authenticate_orcid('0009-0006-0987-5702') + + assert_empty profile.errors + assert_equal '0009-0006-0987-5702', profile.orcid + assert profile.orcid_authenticated + end + + test 'does not assign authenticated orcid if already taken' do + existing_profile = profiles(:trainer_one_profile) + profile = users(:regular_user).profile + assert existing_profile.orcid.present? + assert existing_profile.orcid_authenticated + refute profile.orcid.present? + refute profile.orcid_authenticated + + profile.authenticate_orcid(existing_profile.orcid) + + assert profile.errors.added?(:orcid, :orcid_taken) + assert existing_profile.orcid.present? + assert existing_profile.orcid_authenticated + refute profile.orcid.present? + refute profile.orcid_authenticated + end + + test 'assigns authenticated orcid if existing orcid is not authenticated' do + existing_profile = profiles(:trainer_two_profile) + profile = users(:regular_user).profile + assert existing_profile.orcid.present? + refute existing_profile.orcid_authenticated + refute profile.orcid.present? + refute profile.orcid_authenticated + + profile.authenticate_orcid(existing_profile.orcid) + + assert_empty profile.errors + assert existing_profile.orcid.present? + refute existing_profile.orcid_authenticated + assert profile.orcid.present? + assert profile.orcid_authenticated + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index e53976edb..4e0a42a26 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -390,7 +390,7 @@ class UserTest < ActiveSupport::TestCase profile = user1.profile assert_equal 'John', profile.firstname assert_equal 'Userton', profile.surname - assert_equal 'https://orcid.org/0000-0002-1825-0097', profile.orcid + assert_equal 'https://orcid.org/0000-0002-1825-0097', profile.orcid_url assert_equal 'Cool guy', profile.description assert_equal ['Python', 'Ruby', 'R', 'Java'], profile.expertise_technical diff --git a/test/vcr_cassettes/orcid/get_token_existing_orcid.yml b/test/vcr_cassettes/orcid/get_token_existing_orcid.yml new file mode 100644 index 000000000..fd884599d --- /dev/null +++ b/test/vcr_cassettes/orcid/get_token_existing_orcid.yml @@ -0,0 +1,56 @@ +--- +http_interactions: +- request: + method: post + uri: https://sandbox.orcid.org/oauth/token + body: + encoding: UTF-8 + string: code=123xyz&grant_type=authorization_code&redirect_uri=https%3A%2F%2Flocalhost%3A3001%2Forcid%2Fcallback + headers: + User-Agent: + - Rack::OAuth2 (2.2.0) + Content-Type: + - application/x-www-form-urlencoded + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Accept: + - "*/*" + response: + status: + code: 200 + message: OK + headers: + Date: + - Fri, 28 Nov 2025 15:23:49 GMT + Content-Type: + - application/json;charset=UTF-8 + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Cf-Ray: + - 9a5ade5f5be79527-LHR + Cache-Control: + - no-cache, no-store, max-age=0, must-revalidate + Pragma: + - no-cache + Expires: + - '0' + X-Xss-Protection: + - 1; mode=block + X-Frame-Options: + - DENY + X-Content-Type-Options: + - nosniff + Vary: + - accept-encoding + Cf-Cache-Status: + - DYNAMIC + Server: + - cloudflare + body: + encoding: ASCII-8BIT + string: '{"access_token":"12345678-1234-1234-1234-123412341234","token_type":"bearer","refresh_token":"12345678-1234-1234-1234-123412341234","expires_in":631138518,"scope":"/authenticate", + "name":"Josiah Carberry","orcid":"0000-0002-1825-0097"}' + recorded_at: Fri, 28 Nov 2025 15:23:49 GMT +recorded_with: VCR 6.2.0 diff --git a/test/vcr_cassettes/orcid/get_token_free_orcid.yml b/test/vcr_cassettes/orcid/get_token_free_orcid.yml new file mode 100644 index 000000000..7289abe81 --- /dev/null +++ b/test/vcr_cassettes/orcid/get_token_free_orcid.yml @@ -0,0 +1,56 @@ +--- +http_interactions: +- request: + method: post + uri: https://sandbox.orcid.org/oauth/token + body: + encoding: UTF-8 + string: code=123xyz&grant_type=authorization_code&redirect_uri=https%3A%2F%2Flocalhost%3A3001%2Forcid%2Fcallback + headers: + User-Agent: + - Rack::OAuth2 (2.2.0) + Content-Type: + - application/x-www-form-urlencoded + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Accept: + - "*/*" + response: + status: + code: 200 + message: OK + headers: + Date: + - Fri, 28 Nov 2025 15:23:49 GMT + Content-Type: + - application/json;charset=UTF-8 + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Cf-Ray: + - 9a5ade5f5be79527-LHR + Cache-Control: + - no-cache, no-store, max-age=0, must-revalidate + Pragma: + - no-cache + Expires: + - '0' + X-Xss-Protection: + - 1; mode=block + X-Frame-Options: + - DENY + X-Content-Type-Options: + - nosniff + Vary: + - accept-encoding + Cf-Cache-Status: + - DYNAMIC + Server: + - cloudflare + body: + encoding: ASCII-8BIT + string: '{"access_token":"12345678-1234-1234-1234-123412341234","token_type":"bearer","refresh_token":"12345678-1234-1234-1234-123412341234","expires_in":631138518,"scope":"/authenticate", + "name":"Joe Bloggs","orcid":"0009-0006-0987-5702"}' + recorded_at: Fri, 28 Nov 2025 15:23:49 GMT +recorded_with: VCR 6.2.0 diff --git a/test/vcr_cassettes/orcid/get_token_unauth_orcid.yml b/test/vcr_cassettes/orcid/get_token_unauth_orcid.yml new file mode 100644 index 000000000..3457fc498 --- /dev/null +++ b/test/vcr_cassettes/orcid/get_token_unauth_orcid.yml @@ -0,0 +1,56 @@ +--- +http_interactions: +- request: + method: post + uri: https://sandbox.orcid.org/oauth/token + body: + encoding: UTF-8 + string: code=123xyz&grant_type=authorization_code&redirect_uri=https%3A%2F%2Flocalhost%3A3001%2Forcid%2Fcallback + headers: + User-Agent: + - Rack::OAuth2 (2.2.0) + Content-Type: + - application/x-www-form-urlencoded + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Accept: + - "*/*" + response: + status: + code: 200 + message: OK + headers: + Date: + - Fri, 28 Nov 2025 15:23:49 GMT + Content-Type: + - application/json;charset=UTF-8 + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Cf-Ray: + - 9a5ade5f5be79527-LHR + Cache-Control: + - no-cache, no-store, max-age=0, must-revalidate + Pragma: + - no-cache + Expires: + - '0' + X-Xss-Protection: + - 1; mode=block + X-Frame-Options: + - DENY + X-Content-Type-Options: + - nosniff + Vary: + - accept-encoding + Cf-Cache-Status: + - DYNAMIC + Server: + - cloudflare + body: + encoding: ASCII-8BIT + string: '{"access_token":"12345678-1234-1234-1234-123412341234","token_type":"bearer","refresh_token":"12345678-1234-1234-1234-123412341234","expires_in":631138518,"scope":"/authenticate", + "name":"Lucifer MorningStar","orcid":"0000-0002-0048-3300"}' + recorded_at: Fri, 28 Nov 2025 15:23:49 GMT +recorded_with: VCR 6.2.0 From 690b3caf6d17ecd0071c2cf25d4b463d6d1f0e6d Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Fri, 28 Nov 2025 16:31:22 +0000 Subject: [PATCH 03/12] Remove debug code --- app/controllers/concerns/orcid_authentication.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/orcid_authentication.rb b/app/controllers/concerns/orcid_authentication.rb index 3f498c557..2b7552345 100644 --- a/app/controllers/concerns/orcid_authentication.rb +++ b/app/controllers/concerns/orcid_authentication.rb @@ -2,7 +2,7 @@ module OrcidAuthentication extend ActiveSupport::Concern included do - before_action :set_oauth2_client, only: [:authenticate_orcid, :orcid_callback] + before_action :set_orcid_oauth_client, only: [:authenticate_orcid, :orcid_callback] end def authenticate_orcid @@ -26,12 +26,12 @@ def orcid_callback private - def set_oauth2_client + def set_orcid_oauth_client config = Rails.application.config.secrets.orcid @oauth2_client ||= Rack::OAuth2::Client.new( identifier: config[:client_id], secret: config[:secret], - redirect_uri: 'https://localhost:3001/orcid/callback',#config[:redirect_uri].presence || orcid_callback_url, + redirect_uri: config[:redirect_uri].presence || orcid_callback_url, authorization_endpoint: '/oauth/authorize', token_endpoint: '/oauth/token', host: config[:host].presence || (Rails.env.production? ? 'orcid.org' : 'sandbox.orcid.org') From b74f36f2493ab662a727a8b55b8581f8e47dbc98 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 13:35:17 +0000 Subject: [PATCH 04/12] Fix condition not being checked Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- app/helpers/users_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 08543c56a..b0d686a4c 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -11,7 +11,7 @@ def orcid_link(profile, *opts) concat image_tag('ORCID-iD_icon_vector.svg', size: 16) concat ' ' concat external_link(profile.orcid, profile.orcid_url, *opts) - else profile.orcid.present? + elsif profile.orcid.present? concat image_tag('ORCID-iD_icon_unauth_vector.svg', size: 16) concat ' ' concat external_link(profile.orcid, profile.orcid_url, *opts) From 41541fe82ff3f6952621bc2d0445f239b9c88dc1 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 13:39:02 +0000 Subject: [PATCH 05/12] Handle blank orcid in `orcid_url` --- app/models/profile.rb | 1 + test/models/profile_test.rb | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/app/models/profile.rb b/app/models/profile.rb index bcf0f0b94..7e5c30554 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -26,6 +26,7 @@ def full_name end def orcid_url + return nil if orcid.blank? "#{OrcidValidator::ORCID_PREFIX}#{orcid}" end diff --git a/test/models/profile_test.rb b/test/models/profile_test.rb index 22720c4dd..aed538b2a 100644 --- a/test/models/profile_test.rb +++ b/test/models/profile_test.rb @@ -134,4 +134,10 @@ class ProfileTest < ActiveSupport::TestCase assert profile.orcid.present? assert profile.orcid_authenticated end + + test 'orcid_url' do + assert_nil Profile.new.orcid_url + assert_nil Profile.new(orcid: '').orcid_url + assert_equal 'https://orcid.org/0009-0006-0987-5702', Profile.new(orcid: '0009-0006-0987-5702').orcid_url + end end From 231e67d59113d51cc2dfb11c7f73601aceb6f42a Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 13:41:03 +0000 Subject: [PATCH 06/12] Splat options properly --- app/helpers/users_helper.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index b0d686a4c..7a0b752d4 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -5,16 +5,16 @@ def self.user_profile_resource_limit 30 end - def orcid_link(profile, *opts) + def orcid_link(profile, **opts) content_tag(:span) do if profile.orcid_authenticated? concat image_tag('ORCID-iD_icon_vector.svg', size: 16) concat ' ' - concat external_link(profile.orcid, profile.orcid_url, *opts) + concat external_link(profile.orcid, profile.orcid_url, **opts) elsif profile.orcid.present? concat image_tag('ORCID-iD_icon_unauth_vector.svg', size: 16) concat ' ' - concat external_link(profile.orcid, profile.orcid_url, *opts) + concat external_link(profile.orcid, profile.orcid_url, **opts) concat ' (Unauthenticated)' end end From 009c4063f647c7ef23e821913659f71acaa90878 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 13:41:58 +0000 Subject: [PATCH 07/12] Prefer `button_to` over `link_to ... method: :post` --- app/views/users/show.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 9c0fdacd7..f8417e7b4 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -52,7 +52,7 @@ <% else %> <%= orcid_link(@user.profile) %> <% if current_user == @user && !@user.profile.orcid_authenticated? %> - <%= link_to t('orcid.authenticate'), orcid_authenticate_path, method: :post, class: 'btn btn-default' %> + <%= button_to t('orcid.authenticate'), orcid_authenticate_path, class: 'btn btn-default' %> <% end %> <% end %>

From d75e94d85d5b5a6b13a2c38ec0fc32589dc447af Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 13:43:21 +0000 Subject: [PATCH 08/12] Check ORCID prefix present in migration to avoid unnecessary updates --- db/migrate/20251128160923_update_orcids_in_profiles.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20251128160923_update_orcids_in_profiles.rb b/db/migrate/20251128160923_update_orcids_in_profiles.rb index 486d59dd1..5f2bf7e4b 100644 --- a/db/migrate/20251128160923_update_orcids_in_profiles.rb +++ b/db/migrate/20251128160923_update_orcids_in_profiles.rb @@ -2,7 +2,7 @@ class UpdateOrcidsInProfiles < ActiveRecord::Migration[7.2] ORCID_PREFIX = 'https://orcid.org/'.freeze def up Profile.where('orcid IS NOT NULL').find_each do |profile| - profile.update_column(:orcid, profile.orcid.gsub(ORCID_PREFIX, '')) + profile.update_column(:orcid, profile.orcid.gsub(ORCID_PREFIX, '')) if profile.orcid.start_with?(ORCID_PREFIX) end end From 90f765c66363778b34296b8f7e1a1e068fa70ef5 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 14:17:32 +0000 Subject: [PATCH 09/12] Move ORCID authentication into its own controller Add error handling --- app/controllers/application_controller.rb | 2 +- ..._authentication.rb => orcid_controller.rb} | 16 +-- app/controllers/users_controller.rb | 1 - app/views/users/show.html.erb | 2 +- config/routes.rb | 4 +- test/controllers/orcid_controller_test.rb | 99 +++++++++++++++++++ test/controllers/users_controller_test.rb | 75 ++++---------- test/helpers/users_helper_test.rb | 9 +- test/vcr_cassettes/orcid/error_500.yml | 55 +++++++++++ test/vcr_cassettes/orcid/error_unauth.yml | 56 +++++++++++ 10 files changed, 252 insertions(+), 67 deletions(-) rename app/controllers/{concerns/orcid_authentication.rb => orcid_controller.rb} (72%) create mode 100644 test/vcr_cassettes/orcid/error_500.yml create mode 100644 test/vcr_cassettes/orcid/error_unauth.yml diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index dd748642c..a2c25b90d 100644 --- a/app/controllers/application_controller.rb +++ b/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 } diff --git a/app/controllers/concerns/orcid_authentication.rb b/app/controllers/orcid_controller.rb similarity index 72% rename from app/controllers/concerns/orcid_authentication.rb rename to app/controllers/orcid_controller.rb index 2b7552345..a262e1b50 100644 --- a/app/controllers/concerns/orcid_authentication.rb +++ b/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], diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index dceb1fa34..36a2e73f9 100644 --- a/app/controllers/users_controller.rb +++ b/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 diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index f8417e7b4..c3424da77 100644 --- a/app/views/users/show.html.erb +++ b/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 %>

diff --git a/config/routes.rb b/config/routes.rb index fc076be11..60ca008c6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -179,8 +179,8 @@ match 'oai-pmh', to: "oai#index", via: [:get, :post] - 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". diff --git a/test/controllers/orcid_controller_test.rb b/test/controllers/orcid_controller_test.rb index 17470eaaf..eb92210fb 100644 --- a/test/controllers/orcid_controller_test.rb +++ b/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 diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index d1172ae91..64e80f0bb 100644 --- a/test/controllers/users_controller_test.rb +++ b/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 diff --git a/test/helpers/users_helper_test.rb b/test/helpers/users_helper_test.rb index d5e42aafb..11c176710 100644 --- a/test/helpers/users_helper_test.rb +++ b/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 diff --git a/test/vcr_cassettes/orcid/error_500.yml b/test/vcr_cassettes/orcid/error_500.yml new file mode 100644 index 000000000..e01a8728a --- /dev/null +++ b/test/vcr_cassettes/orcid/error_500.yml @@ -0,0 +1,55 @@ +--- +http_interactions: +- request: + method: post + uri: https://sandbox.orcid.org/oauth/token + body: + encoding: UTF-8 + string: code=123xyz&grant_type=authorization_code&redirect_uri=https%3A%2F%2Flocalhost%3A3001%2Forcid%2Fcallback + headers: + User-Agent: + - Rack::OAuth2 (2.2.0) + Content-Type: + - application/x-www-form-urlencoded + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Accept: + - "*/*" + response: + status: + code: 500 + message: Internal Server Error + headers: + Date: + - Fri, 28 Nov 2025 15:23:49 GMT + Content-Type: + - application/json;charset=UTF-8 + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Cf-Ray: + - 9a5ade5f5be79527-LHR + Cache-Control: + - no-cache, no-store, max-age=0, must-revalidate + Pragma: + - no-cache + Expires: + - '0' + X-Xss-Protection: + - 1; mode=block + X-Frame-Options: + - DENY + X-Content-Type-Options: + - nosniff + Vary: + - accept-encoding + Cf-Cache-Status: + - DYNAMIC + Server: + - cloudflare + body: + encoding: ASCII-8BIT + string: 'An error occurred' + recorded_at: Fri, 28 Nov 2025 15:23:49 GMT +recorded_with: VCR 6.2.0 diff --git a/test/vcr_cassettes/orcid/error_unauth.yml b/test/vcr_cassettes/orcid/error_unauth.yml new file mode 100644 index 000000000..e3245d1a9 --- /dev/null +++ b/test/vcr_cassettes/orcid/error_unauth.yml @@ -0,0 +1,56 @@ +--- +http_interactions: +- request: + method: post + uri: https://sandbox.orcid.org/oauth/token + body: + encoding: UTF-8 + string: code=123xyz&grant_type=authorization_code&redirect_uri=http%3A%2F%2Ftest.host%2Forcid%2Fcallback + headers: + User-Agent: + - Rack::OAuth2 (2.2.0) + Content-Type: + - application/x-www-form-urlencoded + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Accept: + - "*/*" + response: + status: + code: 401 + message: Unauthorized + headers: + Date: + - Mon, 01 Dec 2025 14:02:44 GMT + Content-Type: + - application/json;charset=UTF-8 + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Cf-Ray: + - 9a731fbcfdc7776d-LHR + Cache-Control: + - no-store + Pragma: + - no-cache + Www-Authenticate: + - 'Bearer realm="orcid", error="invalid_client", error_description="Client not + found: test"' + X-Xss-Protection: + - 1; mode=block + X-Frame-Options: + - DENY + X-Content-Type-Options: + - nosniff + Vary: + - accept-encoding + Cf-Cache-Status: + - DYNAMIC + Server: + - cloudflare + body: + encoding: ASCII-8BIT + string: '{"error":"invalid_client","error_description":"Client not found: test"}' + recorded_at: Mon, 01 Dec 2025 14:02:44 GMT +recorded_with: VCR 6.2.0 From 83f14b505f66258ab82a125fed6dcfe277d521ac Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 14:34:45 +0000 Subject: [PATCH 10/12] Handle missing ORCID from metadata --- app/controllers/orcid_controller.rb | 10 +++- config/locales/en.yml | 3 +- test/controllers/orcid_controller_test.rb | 22 +++++++- .../orcid/get_token_orcid_missing.yml | 55 +++++++++++++++++++ 4 files changed, 83 insertions(+), 7 deletions(-) create mode 100644 test/vcr_cassettes/orcid/get_token_orcid_missing.yml diff --git a/app/controllers/orcid_controller.rb b/app/controllers/orcid_controller.rb index a262e1b50..970c1a572 100644 --- a/app/controllers/orcid_controller.rb +++ b/app/controllers/orcid_controller.rb @@ -17,10 +17,14 @@ def callback orcid = token.access_token&.raw_attributes['orcid'] respond_to do |format| profile = current_user.profile - if profile.authenticate_orcid(orcid) - flash[:notice] = t('orcid.authentication_success') + if orcid.present? + if profile.authenticate_orcid(orcid) + flash[:notice] = t('orcid.authentication_success') + else + flash[:error] = profile.errors.full_messages.join(', ') + end else - flash[:error] = profile.errors.full_messages.join(', ') + flash[:error] = t('orcid.authentication_failure') end format.html { redirect_to current_user } end diff --git a/config/locales/en.yml b/config/locales/en.yml index 349ea545d..610ec5f2d 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1119,4 +1119,5 @@ en: orcid: error: 'An error occurred whilst trying to authenticate your ORCID.' authenticate: 'Authenticate your ORCID' - authentication_success: 'You have successfully authenticated your ORCID.' \ No newline at end of file + authentication_success: 'You have successfully authenticated your ORCID.' + authentication_failure: 'Failed to authenticate your ORCID.' \ No newline at end of file diff --git a/test/controllers/orcid_controller_test.rb b/test/controllers/orcid_controller_test.rb index eb92210fb..a39c2cd80 100644 --- a/test/controllers/orcid_controller_test.rb +++ b/test/controllers/orcid_controller_test.rb @@ -81,7 +81,7 @@ class OrcidControllerTest < ActionController::TestCase end assert_response :unprocessable_entity - assert_select '#error-message', text: /ORCID/ + assert_select '#error-message', text: /error occurred.+ORCID/ profile = user.profile.reload assert profile.orcid.blank? refute profile.orcid_authenticated? @@ -92,12 +92,28 @@ class OrcidControllerTest < ActionController::TestCase user = users(:regular_user) sign_in user - VCR.use_cassette('orcid/error_500') do + VCR.use_cassette('orcid/get_token_orcid_missing') do get :callback, params: { code: '123xyz' } end assert_response :unprocessable_entity - assert_select '#error-message', text: /ORCID/ + assert_select '#error-message', text: /error occurred.+ORCID/ + profile = user.profile.reload + assert profile.orcid.blank? + refute profile.orcid_authenticated? + end + + test 'handle missing orcid during callback' do + mock_images + user = users(:regular_user) + sign_in user + + VCR.use_cassette('orcid/get_token_orcid_missing') do + get :callback, params: { code: '123xyz' } + end + + assert_redirected_to user + assert_includes flash[:error], 'Failed to authenticat' profile = user.profile.reload assert profile.orcid.blank? refute profile.orcid_authenticated? diff --git a/test/vcr_cassettes/orcid/get_token_orcid_missing.yml b/test/vcr_cassettes/orcid/get_token_orcid_missing.yml new file mode 100644 index 000000000..18305052a --- /dev/null +++ b/test/vcr_cassettes/orcid/get_token_orcid_missing.yml @@ -0,0 +1,55 @@ +--- +http_interactions: +- request: + method: post + uri: https://sandbox.orcid.org/oauth/token + body: + encoding: UTF-8 + string: code=123xyz&grant_type=authorization_code&redirect_uri=https%3A%2F%2Flocalhost%3A3001%2Forcid%2Fcallback + headers: + User-Agent: + - Rack::OAuth2 (2.2.0) + Content-Type: + - application/x-www-form-urlencoded + Accept-Encoding: + - gzip;q=1.0,deflate;q=0.6,identity;q=0.3 + Accept: + - "*/*" + response: + status: + code: 200 + message: OK + headers: + Date: + - Fri, 28 Nov 2025 15:23:49 GMT + Content-Type: + - application/json;charset=UTF-8 + Transfer-Encoding: + - chunked + Connection: + - keep-alive + Cf-Ray: + - 9a5ade5f5be79527-LHR + Cache-Control: + - no-cache, no-store, max-age=0, must-revalidate + Pragma: + - no-cache + Expires: + - '0' + X-Xss-Protection: + - 1; mode=block + X-Frame-Options: + - DENY + X-Content-Type-Options: + - nosniff + Vary: + - accept-encoding + Cf-Cache-Status: + - DYNAMIC + Server: + - cloudflare + body: + encoding: ASCII-8BIT + string: '{"access_token":"12345678-1234-1234-1234-123412341234","token_type":"bearer","refresh_token":"12345678-1234-1234-1234-123412341234","expires_in":631138518,"scope":"/authenticate"}' + recorded_at: Fri, 28 Nov 2025 15:23:49 GMT +recorded_with: VCR 6.2.0 From 359b7801cf879e41b734ceb99d15d70aa52772e5 Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 14:36:49 +0000 Subject: [PATCH 11/12] Index profiles on ORCID --- db/migrate/20251201143501_add_index_on_orcid_to_profiles.rb | 5 +++++ db/schema.rb | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20251201143501_add_index_on_orcid_to_profiles.rb diff --git a/db/migrate/20251201143501_add_index_on_orcid_to_profiles.rb b/db/migrate/20251201143501_add_index_on_orcid_to_profiles.rb new file mode 100644 index 000000000..d61e959d8 --- /dev/null +++ b/db/migrate/20251201143501_add_index_on_orcid_to_profiles.rb @@ -0,0 +1,5 @@ +class AddIndexOnOrcidToProfiles < ActiveRecord::Migration[7.2] + def change + add_index :profiles, :orcid + end +end diff --git a/db/schema.rb b/db/schema.rb index f4395df2f..7354d68e5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_11_28_160923) do +ActiveRecord::Schema[7.2].define(version: 2025_12_01_143501) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -453,6 +453,7 @@ t.string "type", default: "Profile" t.string "fields", default: [], array: true t.boolean "orcid_authenticated", default: false + t.index ["orcid"], name: "index_profiles_on_orcid" t.index ["slug"], name: "index_profiles_on_slug", unique: true end From fdbcf98e79976550a54b40c52e090e88d844b47e Mon Sep 17 00:00:00 2001 From: Finn Bacall Date: Mon, 1 Dec 2025 15:43:10 +0000 Subject: [PATCH 12/12] Authenticating ORCID should de-authenticate existing ORCID users instead of throwing error --- app/models/profile.rb | 18 +++++++++++------- test/controllers/orcid_controller_test.rb | 13 ++++++++----- test/models/profile_test.rb | 16 +++++++++------- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/app/models/profile.rb b/app/models/profile.rb index 7e5c30554..9935bcca9 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -50,14 +50,18 @@ def merge(*others) def authenticate_orcid(orcid) existing = Profile.where(orcid: orcid, orcid_authenticated: true) - if existing.any? - errors.add(:orcid, :orcid_taken) - false - else - self.orcid = orcid - self.orcid_authenticated = true - self.save + self.orcid = orcid + self.orcid_authenticated = true + out = self.save + + if out + existing.each do |profile| + next if profile == self + profile.update_column(:orcid_authenticated, false) + end end + + out end private diff --git a/test/controllers/orcid_controller_test.rb b/test/controllers/orcid_controller_test.rb index a39c2cd80..18c12aa8c 100644 --- a/test/controllers/orcid_controller_test.rb +++ b/test/controllers/orcid_controller_test.rb @@ -47,9 +47,11 @@ class OrcidControllerTest < ActionController::TestCase assert_redirected_to user end - test 'handle callback but do not assign orcid if already used' do + test 'handle callback and assign orcid even if already used' do mock_images user = users(:regular_user) + existing_orcid_user = users(:trainer_user) + assert existing_orcid_user.profile.orcid_authenticated? sign_in user VCR.use_cassette('orcid/get_token_existing_orcid') do @@ -57,10 +59,11 @@ class OrcidControllerTest < ActionController::TestCase end profile = user.profile.reload - assert profile.orcid.blank? - refute profile.orcid_authenticated? + assert_equal '0000-0002-1825-0097', profile.orcid + assert profile.orcid_authenticated? + refute existing_orcid_user.reload.profile.orcid_authenticated? assert_redirected_to user - assert_includes flash[:error], 'ORCID has already been' + assert flash[:error].blank? end test 'do not handle callback if not logged-in' do @@ -92,7 +95,7 @@ class OrcidControllerTest < ActionController::TestCase user = users(:regular_user) sign_in user - VCR.use_cassette('orcid/get_token_orcid_missing') do + VCR.use_cassette('orcid/error_500') do get :callback, params: { code: '123xyz' } end diff --git a/test/models/profile_test.rb b/test/models/profile_test.rb index aed538b2a..93355dafa 100644 --- a/test/models/profile_test.rb +++ b/test/models/profile_test.rb @@ -101,21 +101,23 @@ class ProfileTest < ActiveSupport::TestCase assert profile.orcid_authenticated end - test 'does not assign authenticated orcid if already taken' do + test 'assigns authenticated orcid and de-authenticates other profiles that use it' do existing_profile = profiles(:trainer_one_profile) profile = users(:regular_user).profile assert existing_profile.orcid.present? assert existing_profile.orcid_authenticated refute profile.orcid.present? refute profile.orcid_authenticated + existing_orcid = existing_profile.orcid - profile.authenticate_orcid(existing_profile.orcid) + profile.authenticate_orcid(existing_orcid) - assert profile.errors.added?(:orcid, :orcid_taken) - assert existing_profile.orcid.present? - assert existing_profile.orcid_authenticated - refute profile.orcid.present? - refute profile.orcid_authenticated + assert_empty profile.errors + assert_equal existing_orcid, profile.orcid + assert profile.orcid_authenticated + existing_profile.reload + assert_equal existing_orcid, existing_profile.orcid + refute existing_profile.orcid_authenticated end test 'assigns authenticated orcid if existing orcid is not authenticated' do