Making the UID not include unsafe characters for routing (#1853)

external uids are the entire email, so we need to make them safe

Also makes all exising uids int he database safe

fixes #1823

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

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

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: "[email protected]", email: "[email protected]@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("[email protected]@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 }

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 "[email protected]" 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

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

spec/models/user_spec.rb

@@ -8,6 +8,7 @@
   let(:access_token) { OmniAuth::AuthHash.new(provider: "cas", uid: "who", extra: { mail: "[email protected]" }) }
   let(:access_token_pppl) { OmniAuth::AuthHash.new(provider: "cas", uid: "who", extra: { mail: "[email protected]", departmentnumber: "31000" }) }
   let(:access_token_super_admin) { OmniAuth::AuthHash.new(provider: "cas", uid: "fake1", extra: { mail: "[email protected]" }) }
+  let(:access_token_guest) { OmniAuth::AuthHash.new(provider: "cas", uid: "[email protected]", extra: { mail: "[email protected]@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 "[email protected]@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: "[email protected]@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 = "[email protected]@princeton.edu"
+      expect(user.uid).to eq("[email protected]@princeton.edu")
+    end
+  end
 end

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