311 - limit colleges page and admin pages to admin access

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
 

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

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

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

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

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

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

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