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

CRIMRE-374 address error handling, pages and reporting #330

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

timpeat
Copy link
Member

@timpeat timpeat commented Jul 4, 2023

Description of change

  1. Standardise error responses to reduce information leakages:
  • When not authenticated, all requests to service and admin URL redirect to sign in regardless of the existing of the resource.
  • When authenticated admin, all requests to the service redirect to the user admin
  • When authenticated service user, all requests to user admin render 404
  1. Simplify all but application_not_found error page to reduce risk of error loops.

  2. Ensure error pages do not render flash messages by default.

  3. User errors controller to render all application and middleware errors.

Link to relevant ticket

CRIMRE-374

Notes for reviewer

Screenshots of changes (if applicable)

Before changes:

After changes:

How to manually test the feature

See ticket for criteria.

To render the error pages in development for testing add the following line to config/environments/development.rb

config.action_dispatch.show_exceptions = true
config.consider_all_requests_local = false

@timpeat timpeat force-pushed the CRIMRE-374-secure-error-handling branch 11 times, most recently from 8fd1c35 to c841d07 Compare July 6, 2023 12:01
Remove rails default error pages
Use rails 7 error handling and reporting
@timpeat timpeat force-pushed the CRIMRE-374-secure-error-handling branch from c841d07 to 1c174e3 Compare July 6, 2023 12:45
@timpeat timpeat marked this pull request as ready for review July 6, 2023 12:56
@timpeat timpeat requested a review from a team as a code owner July 6, 2023 12:56
@arthurashman
Copy link
Contributor

arthurashman commented Jul 6, 2023

For logged in users, I don't believe this is working as expected. The ACs are wanting this error page to have the link to all open applications (current behaviour in staging)
image

@timpeat
Copy link
Member Author

timpeat commented Jul 6, 2023

For logged in users, are we able to give them any nav options? I worry that they'll get stuck if the only way back to the app is via the service name or editing the URL

When logged in as a caseworker, if users come across a missing crime application, we do display the primary navigation. However, I prefer not to include the primary navigation on other error pages due to its dependency on database requests.

@arthurashman
Copy link
Contributor

Sorry, Ive annoyingly updated my previous comment. I realised that the ACs said we wanted the link to all open applications which would solve my issue with navigation - but the page rendered isn't that one, so something isn't quite right

@arthurashman
Copy link
Contributor

It's routing to page not found, not application not found

@timpeat
Copy link
Member Author

timpeat commented Jul 6, 2023

This is the page that should be shown when logged in and the missing page is a crime application.

Screenshot 2023-07-06 at 15 47 23

Copy link
Contributor

@willmcb willmcb left a comment

Choose a reason for hiding this comment

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

Thanks looks really good - have have added a few non-blocking comments for you to consider :)

app/controllers/errors_controller.rb Outdated Show resolved Hide resolved
end

it 'shows the application' do
expect(page).to have_content 'AJ 12 34 56'
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to change this if it is a nino as I just merged that nino change sorry 😬

spec/system/errors_spec.rb Show resolved Hide resolved
spec/system/errors_spec.rb Show resolved Hide resolved
spec/system/errors_spec.rb Outdated Show resolved Hide resolved
@timpeat timpeat merged commit 0fcfc81 into main Jul 6, 2023
4 checks passed
@timpeat timpeat deleted the CRIMRE-374-secure-error-handling branch July 6, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants