From 1e12f3a0546a975911ca9d211a5c50704ccc016b Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Wed, 6 May 2026 17:19:39 -0700 Subject: [PATCH 1/6] Adding role-based auth and refactor require_framework_admin --- app/controllers/concerns/auth_support.rb | 15 +++++--- app/models/user.rb | 25 ++++++++++++- spec/models/user_spec.rb | 47 ++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 7 deletions(-) diff --git a/app/controllers/concerns/auth_support.rb b/app/controllers/concerns/auth_support.rb index 8c8c4e37..1fdf7849 100644 --- a/app/controllers/concerns/auth_support.rb +++ b/app/controllers/concerns/auth_support.rb @@ -12,6 +12,15 @@ def authenticate! raise Error::UnauthorizedError, "Endpoint #{controller_name}/#{action_name} requires authentication" unless authenticated? end + # Require current user be authenticated and have a role + def authenticate_with_role!(*roles) + authenticate! + return true if current_user.any_role?(*roles) + + raise Error::ForbiddenError, + "Endpoint #{controller_name}/#{action_name} requires one of: #{roles.join(', ')}" + end + # Return whether the current user is authenticated # # @return [Boolean] @@ -46,11 +55,7 @@ def sign_out end def require_framework_admin! - authenticate! - return true if framework_admin? - - raise Error::ForbiddenError, - "Endpoint #{controller_name}/#{action_name} requires framework admin CalGroup" + authenticate_with_role!(:framework_admin) end def framework_admin? diff --git a/app/models/user.rb b/app/models/user.rb index 19e0100b..7a4014d7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -75,16 +75,37 @@ def primary_patron_record # TODO: Unify this, faculty/staff checks, framework/alma admin checks # (and improve the design) def role?(role) - # First check if user is a hardcoded admin + role_value = + if role.respond_to?(:role) + role.role + elsif role.respond_to?(:name) + role.name + else + role + end + + role_name = role_value.to_sym + + case role_name + when :framework_admin + return true if framework_admin? + when :alma_admin + return true if alma_admin? + end + + # TODO: Remove this hackery!!! return true if FrameworkUsers.hardcoded_admin?(uid) - # If user is not, then check if the user was added to the DB as an admin: user = FrameworkUsers.find_by(lcasid: uid) return false unless user user.assignments.exists?(role:) end + def any_role?(*roles) + roles.flatten.any? { |role| role?(role) } + end + def ucb_faculty? affiliations&.include?('EMPLOYEE-TYPE-ACADEMIC') end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 9e8743e5..829caff1 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -163,6 +163,53 @@ end end + describe :role? do + it 'returns true for framework admin users' do + user = User.new(framework_admin: true) + + expect(user.role?(:framework_admin)).to be(true) + end + + it 'returns true for Alma admin users' do + user = User.new(alma_admin: true) + + expect(user.role?(:alma_admin)).to be(true) + end + + it 'returns false when the user does not have the role' do + user = User.new(uid: '12345') + + allow(FrameworkUsers).to receive(:hardcoded_admin?).with('12345').and_return(false) + allow(FrameworkUsers).to receive(:find_by).with(lcasid: '12345').and_return(nil) + + expect(user.role?(:framework_admin)).to be(false) + end + + it 'accepts a Role object' do + role = double('Role', name: 'framework_admin') + user = User.new(framework_admin: true) + + expect(user.role?(role)).to be(true) + end + end + + describe :any_role? do + it 'returns true when the user has at least one requested role' do + user = User.new(framework_admin: true) + + expect(user.any_role?(:alma_admin, :framework_admin)).to be(true) + end + + it 'returns false when the user has none of the requested roles' do + user = User.new(uid: '12345') + + allow(FrameworkUsers).to receive(:hardcoded_admin?).with('12345').and_return(false) + allow(FrameworkUsers).to receive(:find_by).with(lcasid: '12345').and_return(nil) + + expect(user.any_role?(:alma_admin, :framework_admin)).to be(false) + end + end + describe :verify_calnet_attributes! do it 'allows employee-affiliated users without berkeleyEduStuID' do auth_extra = { From 5b109810b18a41b5e9d85ed42cd5f70637a72f83 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Thu, 7 May 2026 12:21:33 -0700 Subject: [PATCH 2/6] Tweak stackpass auth --- app/controllers/stack_pass_admin_controller.rb | 3 ++- app/controllers/stack_pass_forms_controller.rb | 4 +--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/stack_pass_admin_controller.rb b/app/controllers/stack_pass_admin_controller.rb index 35287322..f5d3b1f1 100644 --- a/app/controllers/stack_pass_admin_controller.rb +++ b/app/controllers/stack_pass_admin_controller.rb @@ -51,7 +51,8 @@ def init_form!; end # You shall not pass....unless you're an admin def require_admin! - @user_is_admin = current_user.role?(Role.stackpass_admin) + authenticate! + @user_is_admin = current_user.any_role?(Role.stackpass_admin) redirect_to stack_pass_forms_path unless @user_is_admin end diff --git a/app/controllers/stack_pass_forms_controller.rb b/app/controllers/stack_pass_forms_controller.rb index 2bc34e73..ce3e38d5 100644 --- a/app/controllers/stack_pass_forms_controller.rb +++ b/app/controllers/stack_pass_forms_controller.rb @@ -86,8 +86,6 @@ def validate_recaptcha! end def require_admin! - authenticate! - @user_is_admin = current_user.role?(Role.stackpass_admin) - raise Error::ForbiddenError unless @user_is_admin + @user_is_admin = authenticate_with_role!(Role.stackpass_admin) end end From c19ffdf03cab007e36ece0fd30a9ce58da136310 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Thu, 7 May 2026 13:04:51 -0700 Subject: [PATCH 3/6] Tweak proxy borrower; remove redundant authorize call from S.P. --- app/controllers/proxy_borrower_admin_controller.rb | 2 +- app/controllers/proxy_borrower_forms_controller.rb | 5 +---- app/controllers/stack_pass_admin_controller.rb | 1 - 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/controllers/proxy_borrower_admin_controller.rb b/app/controllers/proxy_borrower_admin_controller.rb index 4a7e8c5b..99bc64ef 100644 --- a/app/controllers/proxy_borrower_admin_controller.rb +++ b/app/controllers/proxy_borrower_admin_controller.rb @@ -109,7 +109,7 @@ def init_form!; end # You shall not pass....unless you're an admin def require_admin! - @user_is_admin = current_user.role?(Role.proxyborrow_admin) + @user_is_admin = current_user.any_role?(Role.proxyborrow_admin) redirect_to proxy_borrower_forms_path unless @user_is_admin end diff --git a/app/controllers/proxy_borrower_forms_controller.rb b/app/controllers/proxy_borrower_forms_controller.rb index d2c2e277..b536725a 100644 --- a/app/controllers/proxy_borrower_forms_controller.rb +++ b/app/controllers/proxy_borrower_forms_controller.rb @@ -9,7 +9,7 @@ def index # I think I want to get the users role now... if they're in the DB # then I'll want to pass that info so they have the # admin link...otherwise, NO admin link! - @user_is_admin = current_user.role?(Role.proxyborrow_admin) + @user_is_admin = current_user.any_role?(Role.proxyborrow_admin) end def dsp_form @@ -25,9 +25,6 @@ def faculty_form department: @current_user.department_number) end - # TODO: do we still need this? - def forbidden; end - # Processes a request from DSP form: (eventually dry this up) def process_dsp_request @form = ProxyBorrowerRequests.new form_params(:student_name, :dsp_rep) diff --git a/app/controllers/stack_pass_admin_controller.rb b/app/controllers/stack_pass_admin_controller.rb index f5d3b1f1..a20729c3 100644 --- a/app/controllers/stack_pass_admin_controller.rb +++ b/app/controllers/stack_pass_admin_controller.rb @@ -51,7 +51,6 @@ def init_form!; end # You shall not pass....unless you're an admin def require_admin! - authenticate! @user_is_admin = current_user.any_role?(Role.stackpass_admin) redirect_to stack_pass_forms_path unless @user_is_admin end From 8882f4c8037947d7a96acaa7ade950791f703e84 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Fri, 8 May 2026 17:01:32 -0700 Subject: [PATCH 4/6] Remove hardcoded admin functionality --- .../proxy_borrower_admin_controller.rb | 2 +- .../proxy_borrower_forms_controller.rb | 2 +- .../reference_card_forms_controller.rb | 2 +- .../stack_pass_admin_controller.rb | 2 +- .../stack_pass_forms_controller.rb | 2 +- app/controllers/stack_requests_controller.rb | 2 +- app/helpers/field_builder.rb | 2 - app/models/framework_users.rb | 14 -- app/models/user.rb | 29 ++-- spec/calnet_helper.rb | 6 +- spec/jobs_helper.rb | 2 +- .../doemoff_patron_email_forms_spec.rb | 2 +- spec/request/framework_user_request_spec.rb | 2 +- .../proxy_borrower_forms_request_spec.rb | 151 +----------------- .../reference_card_forms_request_spec.rb | 8 +- .../security_incident_report_forms_spec.rb | 2 +- spec/request/stack_pass_form_request_spec.rb | 32 +--- .../proxy_borrower_admin_system_spec.rb | 2 +- .../reference_card_forms_system_spec.rb | 2 +- spec/system/stack_pass_admin_system_spec.rb | 2 +- spec/system/stack_pass_forms_system_spec.rb | 2 +- 21 files changed, 39 insertions(+), 231 deletions(-) diff --git a/app/controllers/proxy_borrower_admin_controller.rb b/app/controllers/proxy_borrower_admin_controller.rb index 99bc64ef..9f5eb89f 100644 --- a/app/controllers/proxy_borrower_admin_controller.rb +++ b/app/controllers/proxy_borrower_admin_controller.rb @@ -109,7 +109,7 @@ def init_form!; end # You shall not pass....unless you're an admin def require_admin! - @user_is_admin = current_user.any_role?(Role.proxyborrow_admin) + @user_is_admin = current_user.any_role?(Role.proxyborrow_admin, :framework_admin) redirect_to proxy_borrower_forms_path unless @user_is_admin end diff --git a/app/controllers/proxy_borrower_forms_controller.rb b/app/controllers/proxy_borrower_forms_controller.rb index b536725a..74586cd6 100644 --- a/app/controllers/proxy_borrower_forms_controller.rb +++ b/app/controllers/proxy_borrower_forms_controller.rb @@ -9,7 +9,7 @@ def index # I think I want to get the users role now... if they're in the DB # then I'll want to pass that info so they have the # admin link...otherwise, NO admin link! - @user_is_admin = current_user.any_role?(Role.proxyborrow_admin) + @user_is_admin = current_user.any_role?(Role.proxyborrow_admin, :framework_admin) end def dsp_form diff --git a/app/controllers/reference_card_forms_controller.rb b/app/controllers/reference_card_forms_controller.rb index ed2b4611..0b765343 100644 --- a/app/controllers/reference_card_forms_controller.rb +++ b/app/controllers/reference_card_forms_controller.rb @@ -85,7 +85,7 @@ def validate_recaptcha! def require_admin! authenticate! - @user_is_admin = current_user.role?(Role.stackpass_admin) + @user_is_admin = current_user.any_role?(Role.stackpass_admin, :framework_admin) raise Error::ForbiddenError unless @user_is_admin end end diff --git a/app/controllers/stack_pass_admin_controller.rb b/app/controllers/stack_pass_admin_controller.rb index a20729c3..e7eb08bb 100644 --- a/app/controllers/stack_pass_admin_controller.rb +++ b/app/controllers/stack_pass_admin_controller.rb @@ -51,7 +51,7 @@ def init_form!; end # You shall not pass....unless you're an admin def require_admin! - @user_is_admin = current_user.any_role?(Role.stackpass_admin) + @user_is_admin = current_user.any_role?(Role.stackpass_admin, :framework_admin) redirect_to stack_pass_forms_path unless @user_is_admin end diff --git a/app/controllers/stack_pass_forms_controller.rb b/app/controllers/stack_pass_forms_controller.rb index ce3e38d5..8e185397 100644 --- a/app/controllers/stack_pass_forms_controller.rb +++ b/app/controllers/stack_pass_forms_controller.rb @@ -86,6 +86,6 @@ def validate_recaptcha! end def require_admin! - @user_is_admin = authenticate_with_role!(Role.stackpass_admin) + @user_is_admin = authenticate_with_role!(Role.stackpass_admin, :framework_admin) end end diff --git a/app/controllers/stack_requests_controller.rb b/app/controllers/stack_requests_controller.rb index faafaaa8..da4e7228 100644 --- a/app/controllers/stack_requests_controller.rb +++ b/app/controllers/stack_requests_controller.rb @@ -8,7 +8,7 @@ class StackRequestsController < ApplicationController def forbidden; end def index - @user_is_admin = current_user.role?(Role.stackpass_admin) + @user_is_admin = current_user.any_role?(Role.stackpass_admin, :framework_admin) end end diff --git a/app/helpers/field_builder.rb b/app/helpers/field_builder.rb index 939d1b65..8902eaf4 100644 --- a/app/helpers/field_builder.rb +++ b/app/helpers/field_builder.rb @@ -1,6 +1,5 @@ require 'action_view/helpers/tag_helper' -# rubocop:disable Rails/HelperInstanceVariable class FieldBuilder attr_reader :tag_helper attr_reader :builder @@ -109,4 +108,3 @@ def error_feedback_tag content_tag(:div, first_error, class: 'invalid-feedback') end end -# rubocop:enable Rails/HelperInstanceVariable diff --git a/app/models/framework_users.rb b/app/models/framework_users.rb index c67818d5..1e3f0689 100644 --- a/app/models/framework_users.rb +++ b/app/models/framework_users.rb @@ -21,18 +21,4 @@ class FrameworkUsers < ActiveRecord::Base validates :role, presence: true - class << self - # Hardcoded admins - so if for some reason all of the - # admins in the DB are deleted, we still have a way of - # getting in and managing things! - HARDCODED_ADMIN_UIDS = [ - '7165', # Lisa Weber - '1707532' # Steve Sullivan - ].freeze - - def hardcoded_admin?(uid) - HARDCODED_ADMIN_UIDS.include?(uid.to_s) - end - end - end diff --git a/app/models/user.rb b/app/models/user.rb index 7a4014d7..4d8ae26d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -72,19 +72,8 @@ def primary_patron_record @primary_patron_record ||= find_primary_record end - # TODO: Unify this, faculty/staff checks, framework/alma admin checks - # (and improve the design) def role?(role) - role_value = - if role.respond_to?(:role) - role.role - elsif role.respond_to?(:name) - role.name - else - role - end - - role_name = role_value.to_sym + role_name = role_name_for(role) case role_name when :framework_admin @@ -93,9 +82,6 @@ def role?(role) return true if alma_admin? end - # TODO: Remove this hackery!!! - return true if FrameworkUsers.hardcoded_admin?(uid) - user = FrameworkUsers.find_by(lcasid: uid) return false unless user @@ -124,4 +110,17 @@ def uid_patron_record def find_primary_record uid_patron_record end + + def role_name_for(role) + role_value = + if role.respond_to?(:role) + role.role + elsif role.respond_to?(:name) + role.name + else + role + end + + role_value.to_sym + end end diff --git a/spec/calnet_helper.rb b/spec/calnet_helper.rb index 0a87bfe7..a14c3fef 100644 --- a/spec/calnet_helper.rb +++ b/spec/calnet_helper.rb @@ -2,8 +2,8 @@ require 'alma_helper' module CalnetHelper - # Lisa Weber's UID, hard-coded in FrameworkUsers - STACK_REQUEST_ADMIN_UID = '7165'.freeze + # UID used for test authentication + TEST_UID = '7165'.freeze # Mocks a calnet login as the specified patron, and stubs the corresponding # Millennium patron dump file. Suitable for calling from a before() block. @@ -31,7 +31,7 @@ def with_patron_login(patron_id) user = login_as_patron(patron_id) yield user rescue StandardError => e - puts "#{e}\n\t#{e.backtrace.join("\n\t")}" # rubocop:disable Rails/Output + puts "#{e}\n\t#{e.backtrace.join("\n\t")}" raise ensure logout! diff --git a/spec/jobs_helper.rb b/spec/jobs_helper.rb index 1d2706e1..071c02f9 100644 --- a/spec/jobs_helper.rb +++ b/spec/jobs_helper.rb @@ -112,7 +112,7 @@ expect { job.perform_now(patron.id) }.to( raise_error(StandardError).and( - (change { ActionMailer::Base.deliveries.count }).by(1) + change { ActionMailer::Base.deliveries.count }.by(1) ) ) last_email = ActionMailer::Base.deliveries.last diff --git a/spec/request/doemoff_patron_email_forms_spec.rb b/spec/request/doemoff_patron_email_forms_spec.rb index 7b6652bd..4e35bffd 100644 --- a/spec/request/doemoff_patron_email_forms_spec.rb +++ b/spec/request/doemoff_patron_email_forms_spec.rb @@ -16,7 +16,7 @@ context 'specs for logged in user' do before do - mock_login(CalnetHelper::STACK_REQUEST_ADMIN_UID) + mock_login(CalnetHelper::TEST_UID) @required_params = { patron_email: 'test@berkeley.edu', patron_message: 'test message', diff --git a/spec/request/framework_user_request_spec.rb b/spec/request/framework_user_request_spec.rb index 6af61846..4d5abe12 100644 --- a/spec/request/framework_user_request_spec.rb +++ b/spec/request/framework_user_request_spec.rb @@ -3,7 +3,7 @@ describe :forms_proxy_borrower_admin, type: :request do context 'specs with hardcoded admin' do before do - mock_login(CalnetHelper::STACK_REQUEST_ADMIN_UID) + mock_login(CalnetHelper::TEST_UID) end it 'removes an admin user' do diff --git a/spec/request/proxy_borrower_forms_request_spec.rb b/spec/request/proxy_borrower_forms_request_spec.rb index ffcc6dbd..dba6ca40 100644 --- a/spec/request/proxy_borrower_forms_request_spec.rb +++ b/spec/request/proxy_borrower_forms_request_spec.rb @@ -10,7 +10,7 @@ let(:alma_api_key) { 'totally-fake-key' } - context 'specs without admin privledges' do + context 'specs without admin privileges' do before do allow(Rails.application.config).to receive(:alma_api_key).and_return(alma_api_key) @patron_id = Alma::Type.sample_id_for(Alma::Type::FACULTY) @@ -33,7 +33,7 @@ end end - context 'specs with created admin privledges' do + context 'specs with created admin privileges' do before do allow(Rails.application.config).to receive(:alma_api_key).and_return(alma_api_key) @@ -83,151 +83,4 @@ expect(response).to have_http_status :ok end end - - context 'specs with hard-coded admin privledges' do - before do - allow(Rails.application.config).to receive(:alma_api_key).and_return(alma_api_key) - - # Need to create a request for search!!! - @req = ProxyBorrowerRequests.create( - faculty_name: 'Test Search User', - department: 'ABCD', - faculty_id: '12345', - student_name: nil, - student_dsp: nil, - dsp_rep: nil, - research_last: 'RLast', - research_first: 'RFirst', - research_middle: nil, - date_term: Date.tomorrow, - renewal: 0, - status: nil - ) - - @patron_id = Alma::Type.sample_id_for(Alma::Type::FACULTY) - @user = login_as_patron(patron_id) - @patron = Alma::User.find(patron_id) - - admin_user = User.new(uid: '1707532', affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) - allow_any_instance_of(ProxyBorrowerAdminController).to receive(:current_user).and_return(admin_user) - end - - it 'DSP Form renders correct form' do - get forms_proxy_borrower_dsp_path - expect(response.body).to include('

Proxy Borrower Card Application Form - DSP

') - expect(response).to have_http_status :ok - end - - it 'Faculty Form forbids non-faculty members' do - get forms_proxy_borrower_faculty_path - expect(response).to have_http_status :forbidden - end - - it 'Admin page renders' do - get forms_proxy_borrower_admin_path - expect(response).to have_http_status :ok - end - - it 'Admin View DB page renders' do - get forms_proxy_borrower_admin_view_path - expect(response).to have_http_status :ok - end - - it 'Admin Export redirects' do - get forms_proxy_borrower_admin_export_path - expect(response).to have_http_status :found - end - - it 'Admin Search renders page' do - get forms_proxy_borrower_admin_search_path - expect(response).to have_http_status :ok - end - - it 'Admin Search indicates if there are no search results found' do - get(forms_proxy_borrower_admin_search_path, params: { - search_term: 'SEARCHVALUE' - }) - - expect(response).to have_http_status :ok - expect(response.body).to match(/we could not find any results/) - end - - it 'Admin Search finds a record' do - get(forms_proxy_borrower_admin_search_path, params: { - search_term: 'RLast' - }) - - expect(response).to have_http_status :ok - expect(response.body).to match(/Test Search User/) - end - - it 'Admin Search handles date searches' do - get(forms_proxy_borrower_admin_search_path, params: { - search_term: '4/13/1996' - }) - - expect(response).to have_http_status :ok - expect(response.body).to match(/we could not find any results/) - end - - it 'Admin Add/Edit Users form renders' do - get forms_proxy_borrower_admin_users_path - expect(response).to have_http_status :ok - end - - it 'Results page renders' do - get forms_proxy_borrower_result_path - expect(response).to have_http_status :ok - end - - it 'Faculty Form rejects a submission with missing fields' do - post(forms_proxy_borrower_request_faculty_path, params: { proxy_borrower_requests: { - faculty_name: 'John Doe', - research_last: '', - research_first: '', - date_term: '', - renewal: '' - } }) - - expect(response).to have_http_status :unprocessable_content - expect(response.body).to match(/Please correct these and resubmit the form/) - end - - it 'submission enqueues confirmation + alert emails' do - expect do - post(forms_proxy_borrower_request_dsp_path, params: { - proxy_borrower_requests: { - student_name: 'Veronica Names', - dsp_rep: 'DSP Rep', - research_last: 'last', - research_first: 'first', - date_term: Date.tomorrow, - renewal: 0 - } - }) - end.to have_enqueued_job(ActionMailer::MailDeliveryJob).exactly(2).times - - expect(response).to have_http_status(:created) - end - - it 'Faculty submission enqueues confirmation + alert emails' do - allow_any_instance_of(User).to receive(:ucb_faculty?).and_return(true) - - expect do - post(forms_proxy_borrower_request_faculty_path, params: { - proxy_borrower_requests: { - faculty_name: 'Thelonius Monk', - department: 'History', - research_last: 'Last', - research_first: 'First', - date_term: Date.tomorrow, - renewal: 0 - } - }) - end.to have_enqueued_job(ActionMailer::MailDeliveryJob).exactly(2).times - - expect(response).to have_http_status(:created) - end - - end end diff --git a/spec/request/reference_card_forms_request_spec.rb b/spec/request/reference_card_forms_request_spec.rb index c297c076..c3d52557 100644 --- a/spec/request/reference_card_forms_request_spec.rb +++ b/spec/request/reference_card_forms_request_spec.rb @@ -8,10 +8,10 @@ attr_reader :patron attr_reader :user - context 'specs without admin privledges' do + context 'specs without admin privileges' do before do login_as_patron(Alma::NON_FRAMEWORK_ADMIN_ID) - allow_any_instance_of(User).to receive(:role?).with(Role.stackpass_admin).and_return(false) + allow_any_instance_of(User).to receive(:any_role?).with(Role.stackpass_admin, :framework_admin).and_return(false) end it 'reference card index page redirects to form' do @@ -88,9 +88,9 @@ end end - context 'specs with admin privledges' do + context 'specs with admin privileges' do before do - admin_user = User.new(display_name: 'Test Admin', uid: '1707532', affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) + admin_user = User.new(display_name: 'Test Admin', uid: '1707532', framework_admin: true, affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) allow_any_instance_of(ReferenceCardFormsController).to receive(:current_user).and_return(admin_user) allow_any_instance_of(StackRequestsController).to receive(:current_user).and_return(admin_user) end diff --git a/spec/request/security_incident_report_forms_spec.rb b/spec/request/security_incident_report_forms_spec.rb index e9fec30f..a6e8f310 100644 --- a/spec/request/security_incident_report_forms_spec.rb +++ b/spec/request/security_incident_report_forms_spec.rb @@ -16,7 +16,7 @@ context 'specs for logged in user' do before do - mock_login(CalnetHelper::STACK_REQUEST_ADMIN_UID) + mock_login(CalnetHelper::TEST_UID) @required_params = { incident_location: 'Camponille Tower', incident_date: '2024-05-03', diff --git a/spec/request/stack_pass_form_request_spec.rb b/spec/request/stack_pass_form_request_spec.rb index 50a99f6b..bd898924 100644 --- a/spec/request/stack_pass_form_request_spec.rb +++ b/spec/request/stack_pass_form_request_spec.rb @@ -14,7 +14,7 @@ context 'specs without admin privledges' do before do login_as_patron(Alma::NON_FRAMEWORK_ADMIN_ID) - allow_any_instance_of(User).to receive(:role?).with(Role.stackpass_admin).and_return(false) + allow_any_instance_of(User).to receive(:any_role?).with(Role.stackpass_admin, :framework_admin).and_return(false) end it 'index page does not include admin link for non admins' do @@ -59,7 +59,7 @@ context 'specs with admin privledges' do before do - admin_user = User.new(display_name: 'Test Admin', uid: '1707532', affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) + admin_user = User.new(display_name: 'Test Admin', uid: '1707532', framework_admin: true, affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) allow_any_instance_of(StackPassFormsController).to receive(:current_user).and_return(admin_user) allow_any_instance_of(StackRequestsController).to receive(:current_user).and_return(admin_user) end @@ -127,34 +127,6 @@ end end - context 'specs with hard-coded admin privledges' do - before do - admin_user = User.new(uid: '1707532', affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) - allow_any_instance_of(StackPassAdminController).to receive(:current_user).and_return(admin_user) - allow_any_instance_of(StackRequestsController).to receive(:current_user).and_return(admin_user) - end - - it 'Admin page renders' do - get forms_stack_pass_admin_path - expect(response).to have_http_status :ok - end - - it 'Stack Pass views page renders' do - get forms_stack_pass_admin_stack_passes_path - expect(response).to have_http_status :ok - end - - it 'Reference Card views page renders' do - get forms_stack_pass_admin_reference_cards_path - expect(response).to have_http_status :ok - end - - it 'Admin Add/Edit Users form renders' do - get forms_stack_pass_admin_users_path - expect(response).to have_http_status :ok - end - end - context 'specs with non-admin user logged in' do before do allow(Rails.application.config).to receive(:alma_api_key).and_return(alma_api_key) diff --git a/spec/system/proxy_borrower_admin_system_spec.rb b/spec/system/proxy_borrower_admin_system_spec.rb index 6a24ffa9..a058c028 100644 --- a/spec/system/proxy_borrower_admin_system_spec.rb +++ b/spec/system/proxy_borrower_admin_system_spec.rb @@ -10,7 +10,7 @@ @assignment = Assignment.create(framework_users_id: user.id, role_id: Role.proxyborrow_admin.id) # These functions require admin privledges: - mock_login(CalnetHelper::STACK_REQUEST_ADMIN_UID) + mock_login(CalnetHelper::TEST_UID) # Go to the Admin Users View Page: visit forms_proxy_borrower_admin_users_path diff --git a/spec/system/reference_card_forms_system_spec.rb b/spec/system/reference_card_forms_system_spec.rb index b4140807..4c3b0524 100644 --- a/spec/system/reference_card_forms_system_spec.rb +++ b/spec/system/reference_card_forms_system_spec.rb @@ -94,7 +94,7 @@ affiliation: 'Red Bull', pass_date: Date.current, pass_date_end: Date.current + 1, local_id: '8675309') # These functions require admin privledges: - admin_user = User.new(display_name: 'Test Admin', uid: '1707532', affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) + admin_user = User.new(display_name: 'Test Admin', uid: '1707532', framework_admin: true, affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) allow_any_instance_of(ReferenceCardFormsController).to receive(:current_user).and_return(admin_user) visit "/forms/reference-card/#{form.id}" diff --git a/spec/system/stack_pass_admin_system_spec.rb b/spec/system/stack_pass_admin_system_spec.rb index 4874c738..37869f2d 100644 --- a/spec/system/stack_pass_admin_system_spec.rb +++ b/spec/system/stack_pass_admin_system_spec.rb @@ -9,7 +9,7 @@ Assignment.create(framework_users_id: user.id, role_id: Role.stackpass_admin.id) # These functions require admin privledges: - mock_login(CalnetHelper::STACK_REQUEST_ADMIN_UID) + mock_login(CalnetHelper::TEST_UID) # Go to the Admin Users View Page: visit forms_stack_pass_admin_users_path diff --git a/spec/system/stack_pass_forms_system_spec.rb b/spec/system/stack_pass_forms_system_spec.rb index 2b3199eb..feb075f0 100644 --- a/spec/system/stack_pass_forms_system_spec.rb +++ b/spec/system/stack_pass_forms_system_spec.rb @@ -57,7 +57,7 @@ phone: '925-555-1234', pass_date: Date.current, main_stack: true, local_id: '8675309') # These functions require admin privledges: - admin_user = User.new(display_name: 'Test Admin', uid: '1707532', affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) + admin_user = User.new(display_name: 'Test Admin', uid: '1707532', framework_admin: true, affiliations: ['EMPLOYEE-TYPE-ACADEMIC']) allow_any_instance_of(StackPassFormsController).to receive(:current_user).and_return(admin_user) visit "/forms/stack-pass/#{form.id}" From 082b9720568ace35965d81fd3db46e7351750103 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Mon, 11 May 2026 11:27:19 -0700 Subject: [PATCH 5/6] Rolling back rubocop disables --- app/helpers/field_builder.rb | 2 ++ spec/calnet_helper.rb | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/helpers/field_builder.rb b/app/helpers/field_builder.rb index 8902eaf4..939d1b65 100644 --- a/app/helpers/field_builder.rb +++ b/app/helpers/field_builder.rb @@ -1,5 +1,6 @@ require 'action_view/helpers/tag_helper' +# rubocop:disable Rails/HelperInstanceVariable class FieldBuilder attr_reader :tag_helper attr_reader :builder @@ -108,3 +109,4 @@ def error_feedback_tag content_tag(:div, first_error, class: 'invalid-feedback') end end +# rubocop:enable Rails/HelperInstanceVariable diff --git a/spec/calnet_helper.rb b/spec/calnet_helper.rb index a14c3fef..e3e32ba7 100644 --- a/spec/calnet_helper.rb +++ b/spec/calnet_helper.rb @@ -31,7 +31,7 @@ def with_patron_login(patron_id) user = login_as_patron(patron_id) yield user rescue StandardError => e - puts "#{e}\n\t#{e.backtrace.join("\n\t")}" + puts "#{e}\n\t#{e.backtrace.join("\n\t")}" # rubocop:disable Rails/Output raise ensure logout! From 004d07e0f5279c5a5fc6d5713007937c170f2856 Mon Sep 17 00:00:00 2001 From: Steve Sullivan Date: Tue, 12 May 2026 08:42:18 -0700 Subject: [PATCH 6/6] Use authenticate_with_role in ref card; update specs --- .../reference_card_forms_controller.rb | 4 +- spec/models/user_spec.rb | 4 +- spec/request/framework_user_request_spec.rb | 70 ++++++++++--------- .../proxy_borrower_admin_system_spec.rb | 30 ++++++-- spec/system/proxy_borrower_dsp_system_spec.rb | 28 +++----- spec/system/stack_pass_admin_system_spec.rb | 30 ++++++-- 6 files changed, 94 insertions(+), 72 deletions(-) diff --git a/app/controllers/reference_card_forms_controller.rb b/app/controllers/reference_card_forms_controller.rb index 0b765343..d24ac1b7 100644 --- a/app/controllers/reference_card_forms_controller.rb +++ b/app/controllers/reference_card_forms_controller.rb @@ -84,8 +84,6 @@ def validate_recaptcha! end def require_admin! - authenticate! - @user_is_admin = current_user.any_role?(Role.stackpass_admin, :framework_admin) - raise Error::ForbiddenError unless @user_is_admin + @user_is_admin = authenticate_with_role!(Role.stackpass_admin, :framework_admin) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 829caff1..b7ebb621 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -179,7 +179,7 @@ it 'returns false when the user does not have the role' do user = User.new(uid: '12345') - allow(FrameworkUsers).to receive(:hardcoded_admin?).with('12345').and_return(false) + allow(FrameworkUsers).to receive(:TEST_UID?).with('12345').and_return(false) allow(FrameworkUsers).to receive(:find_by).with(lcasid: '12345').and_return(nil) expect(user.role?(:framework_admin)).to be(false) @@ -203,7 +203,7 @@ it 'returns false when the user has none of the requested roles' do user = User.new(uid: '12345') - allow(FrameworkUsers).to receive(:hardcoded_admin?).with('12345').and_return(false) + allow(FrameworkUsers).to receive(:TEST_UID?).with('12345').and_return(false) allow(FrameworkUsers).to receive(:find_by).with(lcasid: '12345').and_return(nil) expect(user.any_role?(:alma_admin, :framework_admin)).to be(false) diff --git a/spec/request/framework_user_request_spec.rb b/spec/request/framework_user_request_spec.rb index 4d5abe12..6ec2df23 100644 --- a/spec/request/framework_user_request_spec.rb +++ b/spec/request/framework_user_request_spec.rb @@ -1,38 +1,42 @@ require 'forms_helper' describe :forms_proxy_borrower_admin, type: :request do - context 'specs with hardcoded admin' do - before do - mock_login(CalnetHelper::TEST_UID) - end - - it 'removes an admin user' do - # First, create the user (directly) - user = FrameworkUsers.create(lcasid: 112_233, name: 'John Doe', role: 'Admin') - Assignment.create(framework_users: user, role: Role.proxyborrow_admin) - - # Then, delete via the controller - delete "/forms/proxy-borrower/delete_admin/#{user.id}" - expect(response).to redirect_to(forms_proxy_borrower_admin_users_path) - get(forms_proxy_borrower_admin_users_path) - expect(response.body).to include('Removed John Doe from administrator list') - expect(Assignment.count).to eq(0) - end - - it 'adds an admin user' do - post '/forms/proxy-borrower/add_admin', params: { lcasid: '12345678', name: 'Jane Doe' } - - expect(response).to redirect_to(forms_proxy_borrower_admin_users_path) - get(forms_proxy_borrower_admin_users_path) - - expect(response).to have_http_status(:ok) - expect(response.body).to include('Jane Doe') - - created_user = FrameworkUsers.find_by(lcasid: '12345678') - expect(created_user).not_to be_nil - - assignment = Assignment.find_by(framework_users_id: created_user.id, role_id: Role.proxyborrow_admin.id) - expect(assignment).not_to be_nil - end + let(:admin_role) { Role.proxyborrow_admin } + + before do + mock_login(CalnetHelper::TEST_UID) + end + + it 'removes an admin user' do + user = FrameworkUsers.create(lcasid: 112_233, name: 'John Doe', role: 'Admin') + Assignment.create(framework_users: user, role: admin_role) + + delete "/forms/proxy-borrower/delete_admin/#{user.id}" + + expect(response).to redirect_to(forms_proxy_borrower_admin_users_path) + + get forms_proxy_borrower_admin_users_path + + expect(response.body).to include('Removed John Doe from administrator list') + expect(Assignment.count).to eq(0) + end + + it 'adds an admin user' do + post '/forms/proxy-borrower/add_admin', + params: { lcasid: '12345678', name: 'Jane Doe' } + + expect(response).to redirect_to(forms_proxy_borrower_admin_users_path) + + get forms_proxy_borrower_admin_users_path + + expect(response).to have_http_status(:ok) + expect(response.body).to include('Jane Doe') + + created_user = FrameworkUsers.find_by(lcasid: '12345678') + + expect(created_user).not_to be_nil + expect( + Assignment.find_by(framework_users: created_user, role: admin_role) + ).not_to be_nil end end diff --git a/spec/system/proxy_borrower_admin_system_spec.rb b/spec/system/proxy_borrower_admin_system_spec.rb index a058c028..7c3b58d1 100644 --- a/spec/system/proxy_borrower_admin_system_spec.rb +++ b/spec/system/proxy_borrower_admin_system_spec.rb @@ -3,22 +3,38 @@ describe :forms_proxy_borrower_admin, type: :system do - context 'specs with hardcoded admin' do + context 'specs with proxyborrower admin' do before do - # First create a DSP Rep and assignment - user = FrameworkUsers.create(lcasid: 112_233, name: 'John Doe', role: 'Admin') - @assignment = Assignment.create(framework_users_id: user.id, role_id: Role.proxyborrow_admin.id) + admin = FrameworkUsers.create( + lcasid: CalnetHelper::TEST_UID, + name: 'Test Admin', + role: 'Admin' + ) + + Assignment.create( + framework_users: admin, + role: Role.proxyborrow_admin + ) + + user = FrameworkUsers.create( + lcasid: 112_233, + name: 'John Doe', + role: 'Admin' + ) + + @assignment = Assignment.create( + framework_users: user, + role: Role.proxyborrow_admin + ) - # These functions require admin privledges: mock_login(CalnetHelper::TEST_UID) - # Go to the Admin Users View Page: visit forms_proxy_borrower_admin_users_path end it 'removes an admin user' do accept_confirm 'Are you sure you want to delete John Doe?' do - click_link 'Remove' + click_link 'Remove', match: :first end expect(page).to have_no_content('
John Doe') expect(page).to have_content('Removed John Doe from administrator list') diff --git a/spec/system/proxy_borrower_dsp_system_spec.rb b/spec/system/proxy_borrower_dsp_system_spec.rb index e09b8e24..abf1d221 100644 --- a/spec/system/proxy_borrower_dsp_system_spec.rb +++ b/spec/system/proxy_borrower_dsp_system_spec.rb @@ -3,25 +3,15 @@ require 'time' describe :forms_proxy_borrower_dsp, type: :system do - attr_reader :patron_id - attr_reader :patron - attr_reader :user - let(:alma_api_key) { 'totally-fake-key' } - let(:field_prefix) { 'proxy_borrower_requests_' } - - before(:all) do - # Calculate and define the max date and an invalid date: - @max_date = ProxyBorrowerRequests.max_term - - # Thou shalt pass parameters as Dates, since we use native date fields now: - @invalid_date = Time.zone.today - 1.day - end + let(:max_date) { ProxyBorrowerRequests.max_term } + let(:invalid_date) { Time.zone.today - 1.day } + let(:patron_id) { Alma::Type.sample_id_for(Alma::Type::UNDERGRAD_SLE) } before do - @patron_id = Alma::Type.sample_id_for(Alma::Type::UNDERGRAD_SLE) - @user = login_as_patron(patron_id) + login_as_patron(patron_id) + allow(Rails.application.config).to receive(:alma_api_key).and_return(alma_api_key) req_url = "https://api-na.hosted.exlibrisgroup.com/almaws/v1/users/#{patron_id}?expand=fees&view=full" @@ -30,8 +20,6 @@ .with(headers: { 'Accept' => 'application/json', 'Authorization' => "apikey #{alma_api_key}" }) .to_return(status: 200, body: File.new("spec/data/alma_patrons/#{patron_id}.json")) - @patron = Alma::User.find(patron_id) - visit forms_proxy_borrower_dsp_path end @@ -62,7 +50,7 @@ fill_in("#{field_prefix}research_last", with: ' ') fill_in("#{field_prefix}research_first", with: ' ') fill_in("#{field_prefix}dsp_rep", with: ' ') # TODO: add server-side validation for this (currently only JS) - fill_in("#{field_prefix}date_term", with: @max_date) + fill_in("#{field_prefix}date_term", with: max_date) submit_button = find(:xpath, "//input[@type='submit']") submit_button.click @@ -77,7 +65,7 @@ fill_in("#{field_prefix}research_first", with: 'John') fill_in("#{field_prefix}dsp_rep", with: 'Jane Roe') - fill_in("#{field_prefix}date_term", with: @invalid_date) + fill_in("#{field_prefix}date_term", with: invalid_date) submit_button = find(:xpath, "//input[@type='submit']") submit_button.click @@ -90,7 +78,7 @@ fill_in("#{field_prefix}research_last", with: 'Doe') fill_in("#{field_prefix}research_first", with: 'John') fill_in("#{field_prefix}dsp_rep", with: 'Jane Roe') - fill_in("#{field_prefix}date_term", with: @max_date) + fill_in("#{field_prefix}date_term", with: max_date) submit_button = find(:xpath, "//input[@type='submit']") submit_button.click diff --git a/spec/system/stack_pass_admin_system_spec.rb b/spec/system/stack_pass_admin_system_spec.rb index 37869f2d..6bb1298d 100644 --- a/spec/system/stack_pass_admin_system_spec.rb +++ b/spec/system/stack_pass_admin_system_spec.rb @@ -2,22 +2,38 @@ require 'calnet_helper' describe :forms_stack_pass_admin, type: :system do - context 'specs with hardcoded admin' do + context 'specs with stack pass admin' do before do - # First create an admin and assignment - user = FrameworkUsers.create(lcasid: 112_233, name: 'John Doe', role: 'Admin') - Assignment.create(framework_users_id: user.id, role_id: Role.stackpass_admin.id) + admin = FrameworkUsers.create( + lcasid: CalnetHelper::TEST_UID, + name: 'Test Admin', + role: 'Admin' + ) + + Assignment.create( + framework_users: admin, + role: Role.stackpass_admin + ) + + user = FrameworkUsers.create( + lcasid: 112_233, + name: 'John Doe', + role: 'Admin' + ) + + Assignment.create( + framework_users: user, + role: Role.stackpass_admin + ) - # These functions require admin privledges: mock_login(CalnetHelper::TEST_UID) - # Go to the Admin Users View Page: visit forms_stack_pass_admin_users_path end it 'removes an admin user' do accept_confirm 'Are you sure you want to delete John Doe?' do - click_link 'Remove' + click_link 'Remove', match: :first end expect(page).to have_no_content('
John Doe') expect(page).to have_content('Removed John Doe from administrator list')