Ap 602 refactor roles

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?

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, :framework_admin)
     redirect_to proxy_borrower_forms_path unless @user_is_admin
   end
 

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, :framework_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)

app/controllers/reference_card_forms_controller.rb

@@ -84,8 +84,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, :framework_admin)
   end
 end

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.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
 

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, :framework_admin)
   end
 end

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

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

app/models/user.rb

@@ -72,19 +72,26 @@ 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)
-    # First check if user is a hardcoded admin
-    return true if FrameworkUsers.hardcoded_admin?(uid)
+    role_name = role_name_for(role)
+
+    case role_name
+    when :framework_admin
+      return true if framework_admin?
+    when :alma_admin
+      return true if alma_admin?
+    end
 
-    # 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
@@ -103,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

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.

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

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(: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)
+    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(: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)
+    end
+  end
+
   describe :verify_calnet_attributes! do
     it 'allows employee-affiliated users without berkeleyEduStuID' do
       auth_extra = {

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',

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::STACK_REQUEST_ADMIN_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