diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index eb37d90d..177ef2bc 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -1,8 +1,11 @@ # frozen_string_literal: true class AdminController < ApplicationController + include AdminOnlyAccess + skip_before_action :require_authenticated_user, only: %i[login validate] skip_before_action :check_date + skip_before_action :check_admin, only: %i[login validate] ALLOWED_CONTROLLERS_TO_MODELS = { 'artworks' => Artwork, @@ -29,12 +32,15 @@ def validate session[:admin] = true redirect_to publications_path else - redirect_to manage_path, notice: 'Invalid Credentails' + flash.keep[:danger] = 'Invalid Credentails' + redirect_to manage_path end end + # Generates a CSV file for a given model. This method is restricted + # to admin users through the AdminOnlyAccess concern. def csv - if session[:admin] && params[:id].nil? && allowed_model + if params[:id].nil? && allowed_model begin @instance_variable = allowed_model.all respond_to do |format| @@ -51,15 +57,18 @@ def csv end end + # Generates a citations report. This method is restricted to admin + # users through the AdminOnlyAccess concern. def citations - if session[:admin] - all = fetch_all_records - @college_array = [] - (1..College.count).each do |i| - @college_array << [i, all.select { |p| p.respond_to?(:college_ids) && p.college_ids.include?(i) }.group_by(&:uc_department)] + all_publications = fetch_all_records + @college_array = [] + + College.find_each do |college| + publications_in_college = all_publications.select do |publication| + publication.respond_to?(:college_ids) && publication.college_ids.include?(college.id) end - else - redirect_to publications_path + grouped_by_department = publications_in_college.group_by(&:uc_department) + @college_array << [college.id, grouped_by_department] end end diff --git a/app/controllers/colleges_controller.rb b/app/controllers/colleges_controller.rb index 7c19598d..b8ac83d4 100644 --- a/app/controllers/colleges_controller.rb +++ b/app/controllers/colleges_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class CollegesController < ApplicationController + include AdminOnlyAccess + before_action :set_college, only: %i[show edit update destroy] # GET /colleges @@ -32,7 +34,7 @@ def create format.html { redirect_to @college } format.json { render :show, status: :created, location: @college } else - format.html { render :new } + format.html { render :new, status: :unprocessable_entity } format.json { render json: @college.errors, status: :unprocessable_entity } end end @@ -47,7 +49,7 @@ def update format.html { redirect_to @college } format.json { render :show, status: :ok, location: @college } else - format.html { render :edit } + format.html { render :edit, status: :unprocessable_entity } format.json { render json: @college.errors, status: :unprocessable_entity } end end diff --git a/app/controllers/concerns/admin_only_access.rb b/app/controllers/concerns/admin_only_access.rb new file mode 100644 index 00000000..5a0b4957 --- /dev/null +++ b/app/controllers/concerns/admin_only_access.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# AdminOnlyAccess provides a mechanism to restrict access to certain +# controller actions to admin users only. It hooks into the controller's +# lifecycle using a before_action to check the user's admin status. +module AdminOnlyAccess + extend ActiveSupport::Concern + + included do + before_action :check_admin + end + + private + + def check_admin + return if session[:admin] + + render template: 'errors/404', status: :not_found + end +end diff --git a/spec/controllers/admin_controller/admin_controller_citations_spec.rb b/spec/controllers/admin_controller/admin_controller_citations_spec.rb index 97744870..50d71e68 100644 --- a/spec/controllers/admin_controller/admin_controller_citations_spec.rb +++ b/spec/controllers/admin_controller/admin_controller_citations_spec.rb @@ -6,8 +6,10 @@ describe 'GET #citations' do context 'when admin is logged in' do before do - allow(College).to receive(:count).and_return(2) - allow(controller).to receive(:session).and_return(admin: true) + session[:admin] = true + mock_college1 = double('College', id: 1) + mock_college2 = double('College', id: 2) + allow(College).to receive(:find_each).and_yield(mock_college1).and_yield(mock_college2) allow(controller).to receive(:fetch_all_records).and_return(mocked_records) end @@ -19,15 +21,15 @@ end end - context 'when admin is not logged in' do + context 'when logged in as a submitter' do before do session[:admin] = false session[:submitter_id] = FactoryBot.create(:submitter).id end - it 'redirects to publications_path' do + it 'gives a 404 response' do get :citations - expect(response).to redirect_to(publications_path) + expect(response).to have_http_status(:not_found) end end end diff --git a/spec/controllers/admin_controller/admin_controller_csv_spec.rb b/spec/controllers/admin_controller/admin_controller_csv_spec.rb index 33aab698..708bb82d 100644 --- a/spec/controllers/admin_controller/admin_controller_csv_spec.rb +++ b/spec/controllers/admin_controller/admin_controller_csv_spec.rb @@ -7,6 +7,9 @@ let(:admin_session) { { 'admin' => true } } let(:submitter_session) { { 'submitter_id' => FactoryBot.create(:submitter).id } } + let(:submitter) { FactoryBot.create(:submitter) } + let(:submitter_session) { { submitter_id: submitter.id } } + describe 'GET #csv' do context 'when the user is an admin' do it 'returns a 200 status when requesting a CSV format' do @@ -19,6 +22,13 @@ expect(response).to redirect_to('/publications') end + context 'when an id is sent in the params' do + it 'redirects to publications path' do + get(:csv, params: common_params.merge({ id: 1, format: 'csv' }), session: admin_session) + expect(response).to redirect_to('/publications') + end + end + context 'when a StandardError is raised' do before do allow(OtherPublication).to receive(:to_csv).and_raise(StandardError) @@ -33,10 +43,9 @@ end context 'when the user is not an admin' do - it 'redirects even if a valid format is provided' do + it 'redirects to a 404 page even if a valid format is provided' do get(:csv, params: common_params.merge({ format: 'csv' }), session: submitter_session) - expect(response).to have_http_status(302) - expect(response).to redirect_to('/publications') + expect(response).to have_http_status(:not_found) end end end diff --git a/spec/controllers/colleges_controller_spec.rb b/spec/controllers/colleges_controller_spec.rb index 3dfd4988..4cc9da9b 100644 --- a/spec/controllers/colleges_controller_spec.rb +++ b/spec/controllers/colleges_controller_spec.rb @@ -2,75 +2,22 @@ require 'rails_helper' -# This spec was generated by rspec-rails when you ran the scaffold generator. -# It demonstrates how one might use RSpec to specify the controller code that -# was generated by Rails when you ran the scaffold generator. -# -# It assumes that the implementation code is generated by the rails scaffold -# generator. If you are using any extension libraries to generate different -# controller code, this generated spec may or may not pass. -# -# It only uses APIs available in rails and/or rspec-rails. There are a number -# of tools you can use to make these specs even more expressive, but we're -# sticking to rails and rspec-rails APIs to keep things simple and stable. -# -# Compared to earlier versions of this generator, there is very limited use of -# stubs and message expectations in this spec. Stubs are only used when there -# is no simpler way to get a handle on the object needed for the example. -# Message expectations are only used when there is no simpler way to specify -# that an instance is receiving a specific message. -# -# Also compared to earlier versions of this generator, there are no longer any -# expectations of assigns and templates rendered. These features have been -# removed from Rails core in Rails 5, but can be added back in via the -# `rails-controller-testing` gem. - RSpec.describe CollegesController, type: :controller do - # This should return the minimal set of attributes required to create a valid - # College. As you add validations to College, be sure to - # adjust the attributes here as well. - let(:valid_attributes) do - { name: 'Test' } - end - - let(:invalid_attributes) do - { name: '' } - end - - # This should return the minimal set of values that should be in the session - # in order to pass any filters (e.g. authentication) defined in - # CollegesController. Be sure to keep this updated too. - let(:submitter) { FactoryBot.create(:submitter) } - let(:valid_session) { { submitter_id: submitter.id } } - let(:college) { College.create! valid_attributes } - - describe 'GET #index' do - it 'returns a success response' do - get :index, params: {}, session: valid_session - expect(response).to be_successful - end - end - - describe 'GET #show' do - it 'returns a success response' do - get :show, params: { id: college.to_param }, session: valid_session - expect(response).to be_successful - end - end - - describe 'GET #new' do - it 'returns a success response' do - get :new, params: {}, session: valid_session - expect(response).to be_successful - end - end - - describe 'GET #edit' do - it 'returns a success response' do - get :edit, params: { id: college.to_param }, session: valid_session - expect(response).to be_successful - end - end + let(:valid_attributes) { { name: 'Test' } } + let(:invalid_attributes) { { name: '' } } + let(:valid_session) { { admin: true } } + + let(:college) { FactoryBot.create(:college, valid_attributes) } + + it_behaves_like 'restricts non-admin access', { + 'index' => :get, + 'show' => :get, + 'new' => :get, + 'edit' => :get, + 'create' => :post, + 'update' => :put, + 'destroy' => :delete + } describe 'POST #create' do context 'with valid params' do @@ -87,53 +34,55 @@ end context 'with invalid params' do - it "returns a success response (i.e. to display the 'new' template)" do + it 'renders the :new template with an unprocessable_entity response for invalid attributes' do post :create, params: { college: invalid_attributes }, session: valid_session - expect(response).to be_successful + expect(response).to have_http_status(:unprocessable_entity) + expect(response).to render_template(:new) + end + + it 'does not create a new College' do + expect do + post :create, params: { college: invalid_attributes }, session: valid_session + end.to_not change(College, :count) end end end describe 'PUT #update' do context 'with valid params' do - let(:new_attributes) do - { name: 'Test2' } - end + let(:new_attributes) { { name: 'Test2' } } it 'updates the requested college' do - put :update, params: { id: college.to_param, college: new_attributes }, session: valid_session + put :update, params: { id: college.id, college: new_attributes }, session: valid_session college.reload - college.name == new_attributes[:name] + expect(college.name).to eq(new_attributes[:name]) end it 'redirects to the college' do - put :update, params: { id: college.to_param, college: valid_attributes }, session: valid_session + put :update, params: { id: college.id, college: valid_attributes }, session: valid_session expect(response).to redirect_to(college) end end context 'with invalid params' do - it "returns a success response (i.e. to display the 'edit' template)" do - put :update, params: { id: college.to_param, college: invalid_attributes }, session: valid_session - expect(response).to be_successful + it 'renders the :edit template with an unprocessable_entity response for invalid attributes' do + put :update, params: { id: college.id, college: invalid_attributes }, session: valid_session + expect(response).to have_http_status(:unprocessable_entity) expect(response).to render_template(:edit) end end end describe 'DELETE #destroy' do - before do - college - end - + before { college } # create the college before the test so that it can be destroyed it 'destroys the requested college' do expect do - delete :destroy, params: { id: college.to_param }, session: valid_session + delete :destroy, params: { id: college.id }, session: valid_session end.to change(College, :count).by(-1) end it 'redirects to the colleges list' do - delete :destroy, params: { id: college.to_param }, session: valid_session + delete :destroy, params: { id: college.id }, session: valid_session expect(response).to redirect_to(colleges_url) end end diff --git a/spec/controllers/concerns/admin_only_access_spec.rb b/spec/controllers/concerns/admin_only_access_spec.rb new file mode 100644 index 00000000..4c7e0bb4 --- /dev/null +++ b/spec/controllers/concerns/admin_only_access_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'AdminOnlyAccess Concern', type: :controller do + controller(ApplicationController) do + include AdminOnlyAccess + + def dummy_action + render plain: 'Access granted' + end + end + + before do + routes.draw { get 'dummy_action' => 'anonymous#dummy_action' } + end + + describe 'GET #dummy_action' do + context 'as an admin user' do + before { session[:admin] = true } + + it 'allows access to the action' do + get :dummy_action + expect(response.body).to eq('Access granted') + end + end + + context 'as a submitter' do + before do + session[:admin] = false + session[:submitter_id] = FactoryBot.create(:submitter).id + end + + it 'denies access to the action' do + get :dummy_action + expect(response).to have_http_status(:not_found) + end + end + + context 'as a non-admin user' do + before { session[:admin] = false } + + it 'redirects the user to log in' do + get :dummy_action + expect(response).to have_http_status(302) + expect(response).to redirect_to(root_path) + end + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 8c8757d7..84ff5fd6 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -30,7 +30,6 @@ # Checks for pending migrations and applies them before tests are run. # If you are not using ActiveRecord, you can remove these lines. - begin ActiveRecord::Migration.maintain_test_schema! rescue ActiveRecord::PendingMigrationError => e diff --git a/spec/support/shared_examples/allowed_access.rb b/spec/support/shared_examples/allowed_access.rb index d94ba0f0..12c4bda6 100644 --- a/spec/support/shared_examples/allowed_access.rb +++ b/spec/support/shared_examples/allowed_access.rb @@ -19,7 +19,11 @@ instance_var = instance_variable_get("@#{controller.controller_name.singularize}") expect(response).to redirect_to(instance_var) when 'destroy' - expect(response).to redirect_to(publications_url) + if controller.controller_name == 'colleges' + expect(response).to redirect_to(colleges_url) + else + expect(response).to redirect_to(publications_url) + end when 'new', 'edit', 'show' expect(response).to be_successful when 'index' diff --git a/spec/support/shared_examples/restricted_access.rb b/spec/support/shared_examples/restricted_access.rb new file mode 100644 index 00000000..debcb30e --- /dev/null +++ b/spec/support/shared_examples/restricted_access.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# Shared examples for 'restricted access' to test access control. +# This example verifies that users with the specified role (e.g., :admin, :submitter, +# or :none for non-logged-in users) are properly restricted from accessing certain +# controller actions, resulting in a 404 response. +# +# It configures the user session according to the given role and then makes a request to the specified action. +# +# Parameters: +# - action: The controller action being tested (e.g., :index, :show). +# - method: The HTTP method used for the request (e.g., :get, :post). +# - user_role: The user role being tested for restriction (default is :none for non-logged-in users). +RSpec.shared_examples 'restricted access' do |action, method, user_role = :none| + it 'redirects to a 404 page' do + configure_user_session(user_role) + public_send(method, action, params: params_for(action)) + expect(response).to have_http_status(404) + end +end diff --git a/spec/support/shared_examples/restricts_non_admin_acces.rb b/spec/support/shared_examples/restricts_non_admin_acces.rb new file mode 100644 index 00000000..944a0292 --- /dev/null +++ b/spec/support/shared_examples/restricts_non_admin_acces.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +# Shared examples for testing that only admin users have access to certain actions. +# This example dynamically determines the model based on the controller and creates a record for it. +# It then iterates over a provided hash of actions and methods, setting up tests for each. +# For each action, it tests three contexts: +# 1. Non-admin and non-submitter users are restricted (expecting a 404 response). +# 2. Submitter users are restricted (also expecting a 404 response). +# 3. Admin users are allowed access (expecting a successful response or redirection as appropriate). +# This ensures that only admin users can access these specific actions in the controller. +# Parameters: +# - actions: A hash where keys are action names and values are the corresponding HTTP methods. + +RSpec.shared_examples 'restricts non-admin access' do |actions| + actions.each do |action, method| + describe "#{method.upcase} ##{action}" do + context 'when non-admin and non-submitter user' do + include_examples 'redirect to root', action, method, :none + end + + context 'when submitter user' do + include_examples 'restricted access', action, method, :submitter + end + + context 'when admin user' do + include_examples 'allowed access', action, method, :admin + end + end + end +end