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

238 - Implement User authentication for submitters #314

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

Janell-Huyck
Copy link
Contributor

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

Fixes #238

Implements User authentication for submitters

We were having problems where if a user was trying to access submitter pages such as submitting a new publication, the app would crash. While the crash was due to us trying to find the submitter id in a brittle way, the underlying issue was that an unauthorized user was able to access the page in the first place.

What the PR does

This PR creates a new "concern" called UserAuthentication that is included in all application pages through being part of AplicationController. Aside from places where it's obviously not needed (the login pages), it restricts access to all pages unless the user is logged in as either an admin or a submitter. If the user is not allowed to view a page, they are redirected back to the root path (where they can log in as a submitter).

How it is tested

There is a file specifically for testing the authentication methods: spec/controllers/concerns/user_authentication_spec.rb

Additionally, how it works in relation to all of our publication types is checked by a new shared example, called like this

it_behaves_like 'restricts non-logged-in users', {
    'index' => :get,
    'show' => :get,
    'new' => :get,
    'edit' => :get,
    'create' => :post,
    'update' => :put,
    'destroy' => :delete
  }

The code for the shared example is at spec/support/shared_examples/restricts_non_logged_in_users.rb. Additional helper methods and shared examples to support this shared example are in the /support folder.

The shared example takes each of the actions shown and tests them against users who aren't logged in, users who are logged in as submitters, and users who are logged in as admins. It checks by calling the action (index, show, update, etc) on the controller that's being tested, and sees what the results are. (Does it successfully show an index, show an item, allow for an update, etc appropriately if the user is authorized to do that action? If the user isn't authorized, are they appropriately redirected?)

The checking of different user statuses for pages created some redundancies in the publications tests, so any tests in the publications files that were only checking for a "success" response upon calling the method as user or admin were deleted.

Additional changes

  • Sets html status 'unprocessable_entity' for failed publication update or create methods
  • Fixes brittle find_submitter(id) helper method so it returns nil if not found rather than crashing app
  • Adds a guard to publication_mailer to return if no submitter is given
  • Adjusts tests' "session"s to have a valid submitter id or admin parameters when indicated
  • Adds a new check for all publication types that if creation with invalid attributes is attempted, there is no creation of the publication type.
  • Minor DRY tweaks like pulling out publication creation from multiple tests and putting it at the top of a file.
  • Adds the shared example to rails_helper.rb so that it can be accessed in all tests.

What this PR does not do

Both those concerns will need to be addressed in separate PR's.

@Janell-Huyck Janell-Huyck force-pushed the security-238-implement-user-authentication branch from 8e2d645 to 7f960f1 Compare November 27, 2023 21:28
@Janell-Huyck Janell-Huyck changed the title WIP 238 - Implement User authentication for submitters 238 - Implement User authentication for submitters Nov 27, 2023
@hortongn hortongn assigned hortongn and crowesn and unassigned hortongn Nov 28, 2023
@Janell-Huyck Janell-Huyck force-pushed the security-238-implement-user-authentication branch from 7f960f1 to 1ad2fcd Compare November 28, 2023 22:00
@Janell-Huyck Janell-Huyck changed the title 238 - Implement User authentication for submitters WIP 238 - Implement User authentication for submitters Nov 28, 2023
@Janell-Huyck Janell-Huyck changed the title WIP 238 - Implement User authentication for submitters 238 - Implement User authentication for submitters Nov 28, 2023
@crowesn crowesn self-requested a review December 1, 2023 14:25
Copy link
Contributor

@crowesn crowesn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@crowesn crowesn merged commit 22802d4 into qa Dec 1, 2023
2 checks passed
@crowesn crowesn deleted the security-238-implement-user-authentication branch December 1, 2023 14:26
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.

Trying to access submission page when logged out crashes app
3 participants