From 733b31eb605974ff91fce01c06afec55c1574d77 Mon Sep 17 00:00:00 2001 From: maebeale Date: Fri, 15 May 2026 10:43:23 -0400 Subject: [PATCH] Fix people index for non-admin access (#1490) Prepares the people index to be safe when exposed to non-admins: qualifies the ambiguous `end_date` in `Affiliation.active`, hides people whose user account is locked from the non-admin scope, and gates the index's person/organization links by their show policy so they degrade gracefully when the viewer lacks access. Co-Authored-By: Claude Opus 4.7 --- app/models/affiliation.rb | 2 +- app/views/people/people_results.html.erb | 35 ++++++++++++++++++------ spec/models/affiliation_spec.rb | 6 ++++ spec/models/person_spec.rb | 6 ++++ spec/policies/person_policy_spec.rb | 16 +++++++++++ 5 files changed, 55 insertions(+), 10 deletions(-) diff --git a/app/models/affiliation.rb b/app/models/affiliation.rb index 0ad1cfb55..e5c77d274 100644 --- a/app/models/affiliation.rb +++ b/app/models/affiliation.rb @@ -7,7 +7,7 @@ class Affiliation < ApplicationRecord scope :active, -> { where(inactive: false) - .where("end_date IS NULL OR end_date >= ?", Date.current) + .where("affiliations.end_date IS NULL OR affiliations.end_date >= ?", Date.current) } scope :facilitators, -> { where("title LIKE ?", "%facilitator%") } diff --git a/app/views/people/people_results.html.erb b/app/views/people/people_results.html.erb index 5c35921e7..86871a79f 100644 --- a/app/views/people/people_results.html.erb +++ b/app/views/people/people_results.html.erb @@ -19,11 +19,18 @@ <% @people.each do |person| %> - <% cache [person, current_user.super_user?] do %> + <% cache [ person, current_user.super_user?, current_user.person_id == person.id ] do %> "> <% show_email = person.profile_show_email? || allowed_to?(:manage?, Person) %> - <%= person_profile_button(person, subtitle: (person.preferred_email if show_email), data: { turbo_frame: "_top" }) %> + <% if allowed_to?(:show?, person) %> + <%= person_profile_button(person, subtitle: (person.preferred_email if show_email), data: { turbo_frame: "_top" }) %> + <% else %> + <%= person.name %> + <% if show_email && person.preferred_email.present? %> +
<%= person.preferred_email %>
+ <% end %> + <% end %> <%= person.member_since&.year %> @@ -50,15 +57,25 @@ <% if affiliations.any? %>
<% affiliations.first(3).each do |affiliation| %> - <%= link_to affiliation.organization.name, organization_path(affiliation.organization), - data: { turbo_frame: "_top" }, - class: "inline-block px-3 py-1 rounded-md text-sm font-medium #{DomainTheme.bg_class_for(:organizations, intensity: 100)} #{DomainTheme.text_class_for(:organizations)} #{DomainTheme.bg_class_for(:organizations, intensity: 100, hover: true)}", - style: affiliation.facilitator? ? nil : "border-left: 4px solid #d1d5db" %> + <% org_classes = "inline-block px-3 py-1 rounded-md text-sm font-medium #{DomainTheme.bg_class_for(:organizations, intensity: 100)} #{DomainTheme.text_class_for(:organizations)}" %> + <% org_style = affiliation.facilitator? ? nil : "border-left: 4px solid #d1d5db" %> + <% if allowed_to?(:show?, affiliation.organization) %> + <%= link_to affiliation.organization.name, organization_path(affiliation.organization), + data: { turbo_frame: "_top" }, + class: "#{org_classes} #{DomainTheme.bg_class_for(:organizations, intensity: 100, hover: true)}", + style: org_style %> + <% else %> + <%= affiliation.organization.name %> + <% end %> <% end %> <% if affiliations.size > 3 %> - <%= link_to "+#{affiliations.size - 3}", person_path(person), - data: { turbo_frame: "_top" }, - class: "inline-block px-3 py-1 rounded-md text-sm font-medium text-gray-500 hover:text-gray-700" %> + <% if allowed_to?(:show?, person) %> + <%= link_to "+#{affiliations.size - 3}", person_path(person), + data: { turbo_frame: "_top" }, + class: "inline-block px-3 py-1 rounded-md text-sm font-medium text-gray-500 hover:text-gray-700" %> + <% else %> + +<%= affiliations.size - 3 %> + <% end %> <% end %>
<% else %> diff --git a/spec/models/affiliation_spec.rb b/spec/models/affiliation_spec.rb index d7d766354..ad5ee09c9 100644 --- a/spec/models/affiliation_spec.rb +++ b/spec/models/affiliation_spec.rb @@ -35,6 +35,12 @@ it 'excludes records with past end date' do expect(described_class.active).not_to include(inactive_by_end_date) end + + it 'qualifies end_date when joined with organizations (which also has end_date)' do + expect { + described_class.active.joins(:organization).to_a + }.not_to raise_error + end end describe '#set_inactive_from_dates' do diff --git a/spec/models/person_spec.rb b/spec/models/person_spec.rb index fe35f4540..381d23544 100644 --- a/spec/models/person_spec.rb +++ b/spec/models/person_spec.rb @@ -299,6 +299,12 @@ expect(results).to include(person_alice) expect(results).not_to include(person_bob) end + + it 'chains with_active_affiliations and organization_name without an ambiguous end_date error' do + expect { + Person.with_active_affiliations.search_by_params(organization_name: 'Alpha').to_a + }.not_to raise_error + end end describe ".published" do diff --git a/spec/policies/person_policy_spec.rb b/spec/policies/person_policy_spec.rb index 77aedbf97..c299741f0 100644 --- a/spec/policies/person_policy_spec.rb +++ b/spec/policies/person_policy_spec.rb @@ -132,6 +132,22 @@ def policy_for(record: nil, user:) expect(sql).to include('`affiliations`.`inactive` = FALSE') expect(sql).to include('`users`.`locked_at` IS NULL') end + + it "excludes people whose user account is locked" do + regular = create(:user) + searchable = create(:person, profile_is_searchable: true) + create(:affiliation, person: searchable, inactive: false, end_date: nil) + locked = create(:person, profile_is_searchable: true, user: create(:user, :locked)) + create(:affiliation, person: locked, inactive: false, end_date: nil) + unlinked = create(:person, profile_is_searchable: true, user: nil) + create(:affiliation, person: unlinked, inactive: false, end_date: nil) + + policy = described_class.new(Person, user: regular) + scope = policy.apply_scope(Person.all, type: :active_record_relation) + + expect(scope).to include(searchable, unlinked) + expect(scope).not_to include(locked) + end end end end