diff --git a/app/models/user.rb b/app/models/user.rb index afea088c..a82c2870 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -9,6 +9,8 @@ class User < ApplicationRecord devise :rememberable, :omniauthable + after_initialize :safe_uid_check + # GroupOptions model extensible options set for Groups and Users has_many :group_options, dependent: :destroy has_many :group_messaging_options, -> { where(option_type: GroupOption::EMAIL_MESSAGES) }, class_name: "GroupOption", dependent: :destroy @@ -28,7 +30,7 @@ class User < ApplicationRecord end def self.from_cas(access_token) - user = User.find_by(uid: access_token.uid) + user = User.find_by(uid: safe_uid(access_token.uid)) if user.nil? user = new_from_cas(access_token) elsif user.provider.blank? @@ -41,7 +43,7 @@ def self.from_cas(access_token) def self.new_from_cas(access_token) user = User.new user.provider = access_token.provider - user.uid = access_token.uid # this is the netid + user.uid = safe_uid(access_token.uid) # this is the netid user.email = access_token.extra.mail user.given_name = access_token.extra.givenname || access_token.uid # Harriet user.family_name = access_token.extra.sn || access_token.uid # Tubman @@ -51,6 +53,11 @@ def self.new_from_cas(access_token) user end + def self.safe_uid(uid) + return uid if uid.blank? + uid.gsub(/[^(0-9a-zA-Z_\-)]/, "_") + end + # Updates an existing User record with some information from CAS. This is useful # for records created before the user ever logged in (e.g. to gran them permissions # to groups). @@ -237,5 +244,9 @@ def assign_default_role def email_messages_enabled? email_messages_enabled end + + def safe_uid_check + self.uid = self.class.safe_uid(uid) + end end # rubocop:enable Metrics/ClassLength diff --git a/db/migrate/20240705120614_safe_uid.rb b/db/migrate/20240705120614_safe_uid.rb new file mode 100644 index 00000000..e16d0d9c --- /dev/null +++ b/db/migrate/20240705120614_safe_uid.rb @@ -0,0 +1,10 @@ +class SafeUid < ActiveRecord::Migration[6.1] + def change + # update any exisitng user to have a safe uid so that correct user will be found when the user logs in + User.where(" uid like '%@%'").each do |user| + safe_uid = User.safe_uid(user.uid) + user.uid = safe_uid + user.save + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 65e0544b..d80c27b9 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.define(version: 2023_10_25_155725) do +ActiveRecord::Schema.define(version: 2024_07_05_120614) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index daf88570..fabcf601 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -14,6 +14,17 @@ end end + context "a guest cas user" do + it "redirects to home page with success notice" do + allow(User).to receive(:from_cas) { FactoryBot.create(:user, uid: "test.user@example.com", email: "test.user@example.com@princeton.edu") } + get :cas + expect(response).to redirect_to(root_path) + expect(flash[:notice]).to eq("Successfully authenticated from Princeton Central Authentication Service account.") + expect(User.first.email).to eq("test.user@example.com@princeton.edu") + expect(User.first.uid).to eq("test_user_example_com") + end + end + context "invalid user" do it "redirects to home page with warning notice" do allow(User).to receive(:from_cas) { nil } diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index f11fa45c..8d30a6a5 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -19,6 +19,13 @@ expect(response).to render_template("show") end + it "renders the show page for external users" do + # Notice that for external users like "pppltest@gmail.com" Rails splits the ".com" in the URL + sign_in user_external + get :show, params: { id: user_external.friendly_id } + expect(response).to render_template("show") + end + describe "#edit" do context "when authenticated and the current user is authorized" do before do diff --git a/spec/factories/user.rb b/spec/factories/user.rb index e453cb1a..7bf64a03 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -58,7 +58,7 @@ end factory :external_user, class: "User" do - uid { FFaker::InternetSE.user_name + "@gmail.com" } + uid { User.safe_uid(FFaker::InternetSE.user_name + ".abc@gmail.com") } email { "#{uid}@princeton.edu" } provider { :cas } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0f615116..0a8563e8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -8,6 +8,7 @@ let(:access_token) { OmniAuth::AuthHash.new(provider: "cas", uid: "who", extra: { mail: "who@princeton.edu" }) } let(:access_token_pppl) { OmniAuth::AuthHash.new(provider: "cas", uid: "who", extra: { mail: "who@princeton.edu", departmentnumber: "31000" }) } let(:access_token_super_admin) { OmniAuth::AuthHash.new(provider: "cas", uid: "fake1", extra: { mail: "fake@princeton.edu" }) } + let(:access_token_guest) { OmniAuth::AuthHash.new(provider: "cas", uid: "test.user@example.com", extra: { mail: "test.user@example.com@princeton.edu" }) } let(:access_token_full_extras) do OmniAuth::AuthHash.new(provider: "cas", uid: "test123", @@ -72,6 +73,14 @@ expect(user.given_name).to eq "Who" expect(user.family_name).to eq "Areyou" end + + context "a guest cas user" do + it "redirects to home page with success notice" do + user = described_class.from_cas(access_token_guest) + expect(user.email).to eq "test.user@example.com@princeton.edu" + expect(user.uid).to eq "test_user_example_com" + end + end end describe "#super_admin?" do @@ -281,4 +290,17 @@ expect(user.full_name_safe).to eq(user.uid) end end + + describe "#uid" do + it "updates the uid" do + user = User.new(uid: "test.abc@example.com@princeton.edu") + expect(user.uid).to eq("test_abc_example_com_princeton_edu") + end + + it "doesn't update the uid after initialize" do + user = User.new + user.uid = "test.abc@example.com@princeton.edu" + expect(user.uid).to eq("test.abc@example.com@princeton.edu") + end + end end diff --git a/spec/system/user_show_spec.rb b/spec/system/user_show_spec.rb index e7f449a6..fe50bf44 100644 --- a/spec/system/user_show_spec.rb +++ b/spec/system/user_show_spec.rb @@ -25,23 +25,32 @@ describe "Search feature" do let(:pppl_moderator) { FactoryBot.create :pppl_moderator } let(:rd_moderator) { FactoryBot.create :research_data_moderator } + let(:external_user) { FactoryBot.create :external_user } let(:user_admin) { FactoryBot.create :super_admin_user } + let(:draft_work) { FactoryBot.create(:new_draft_work, created_by_user_id: external_user.id) } before do FactoryBot.create(:tokamak_work) FactoryBot.create(:shakespeare_and_company_work) + draft_work end it "finds works for the user logged in", js: true do sign_in pppl_moderator visit user_path(pppl_moderator) + "?q=tokamak" - expect(page.html.include?("Electron Temperature Gradient Driven Transport Model for Tokamak Plasmas")).to be true - expect(page.html.include?("Shakespeare and Company Project Dataset: Lending Library Members, Books, Events")).to be false + expect(page).to have_content("Electron Temperature Gradient Driven Transport Model for Tokamak Plasmas") + expect(page).not_to have_content("Shakespeare and Company Project Dataset: Lending Library Members, Books, Events") sign_in rd_moderator visit user_path(rd_moderator) + "?q=shakespeare" - expect(page.html.include?("Electron Temperature Gradient Driven Transport Model for Tokamak Plasmas")).to be false - expect(page.html.include?("Shakespeare and Company Project Dataset: Lending Library Members, Books, Events")).to be true + expect(page).not_to have_content("Electron Temperature Gradient Driven Transport Model for Tokamak Plasmas") + expect(page).to have_content("Shakespeare and Company Project Dataset: Lending Library Members, Books, Events") + + sign_in external_user + visit user_path(external_user) + expect(page).to have_content(draft_work.title) + expect(page).not_to have_content("Electron Temperature Gradient Driven Transport Model for Tokamak Plasmas") + expect(page).not_to have_content("Shakespeare and Company Project Dataset: Lending Library Members, Books, Events") end it "allows administrators to find works regardless of the group" do