Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

311 - limit colleges page and admin pages to admin access #313

Merged
merged 9 commits into from
Jan 29, 2024

Conversation

Janell-Huyck
Copy link
Contributor

@Janell-Huyck Janell-Huyck commented Nov 21, 2023

Fixes #311

Limits admin pages and "colleges" page to admin-only access.

Previously a non-admin could gain access to the admin pages /colleges, /citations, and to the csv commands. This PR makes those pages admin-only.

This PR addresses the concern for submitter access to admin pages raised in PR #314.

What the PR does

Makes the Colleges page/method and Admin pages/methods(csv, citations) restricted to only admin users.

Creates a new "concern", AdminOnlyAccess, which is added to the colleges and admin controller files. This concern checks to see if a user has an active admin session, and if they don't, they are shown a 404 page. The check is skipped for the admin login and validate methods because the user would not be logged in as an admin when using them.

How it is tested

The biggest testing changes center around the colleges controller, which was not previously restricted. The PR introduces some new "shared examples", restricts to only admin access, restricted access, and allowed access to determine whether or not a user should be able to complete an action (update, create, edit, destroy, etc) based on the user's role (admin, submitter, or none).

The shared examples are called like this:

  it_behaves_like 'restricts to only admin access', {
    'index' => :get,
    'show' => :get,
    'new' => :get,
    'edit' => :get,
    'create' => :post,
    'update' => :put,
    'destroy' => :delete
  }

The above goes through all the actions for the colleges controller and calls each one of them for each of the roles mentioned.

Many of the tests inside the colleges_controller_spec.rb file were only looking for a success response upon calling the method. This functionality is checked inside the shared examples so I removed these duplicate tests from inside the colleges_controller_spec.

The actual logic for the admin-only concern is tested in spec/controllers/concerns/admin_only_access_spec.rb

Additional changes

  • Clarifies (refactors) the "citations" method in the admin controller to make it easier to read and update, and updated the test to work with the find_each syntax used.
  • Adds html status: :unprocessable_entity to failed college creation or update
  • Adds test for csv for 'when an id is sent in the params'
  • Adds admin session = true for tests where appropriate
  • Minor DRY refactoring for college tests by moving college creation to the top of the file

What is not covered in this PR

@Janell-Huyck Janell-Huyck changed the title 311 - limit colleges page and admin pages to admin access WIP 311 - limit colleges page and admin pages to admin access Nov 27, 2023
@Janell-Huyck Janell-Huyck force-pushed the security-311-limit-colleges-page-to-admins branch from 5389ceb to c67aa83 Compare November 28, 2023 17:04
@Janell-Huyck Janell-Huyck changed the title WIP 311 - limit colleges page and admin pages to admin access 311 - limit colleges page and admin pages to admin access Nov 28, 2023
@Janell-Huyck Janell-Huyck force-pushed the security-311-limit-colleges-page-to-admins branch from 5730790 to 6334c42 Compare December 7, 2023 17:31
@Janell-Huyck Janell-Huyck requested review from haitzlm and scherztc and removed request for haitzlm and scherztc January 18, 2024 19:05
@Janell-Huyck Janell-Huyck force-pushed the security-311-limit-colleges-page-to-admins branch from e478345 to 93ebb3e Compare January 24, 2024 22:14
@scherztc
Copy link
Contributor

@haitzlm

I ran the test locally. Do NOT use bundle exec rake spec. Use bundle exec rspec spec instead.
I tested routes as submitter (/citations, /colleges)
I tested routes as admin (/citations, /colleges, /manage

I did a code read to make sure the concern was use correctly in each of the controllers.
I did a code read on the shared_examples test and other new controller test.

I merged the PR.

@scherztc scherztc merged commit dafb988 into qa Jan 29, 2024
2 checks passed
@scherztc scherztc deleted the security-311-limit-colleges-page-to-admins branch January 29, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security/Bug: non-admin can delete colleges
3 participants