From ac95503618fce2b75babaad6e55ec7a9fb68f52c Mon Sep 17 00:00:00 2001 From: Gilad Shanan Date: Mon, 18 Dec 2023 16:06:06 -0600 Subject: [PATCH 1/7] Add custom exceptions app wrapper Also run `rails g controller errors not_found internal_server_error` --- app/controllers/errors_controller.rb | 7 ++++++ app/helpers/errors_helper.rb | 2 ++ .../errors/internal_server_error.html.erb | 2 ++ app/views/errors/not_found.html.erb | 2 ++ config/application.rb | 5 ++++ config/routes.rb | 2 ++ lib/custom_exceptions_app_wrapper.rb | 25 +++++++++++++++++++ spec/helpers/errors_helper_spec.rb | 15 +++++++++++ spec/requests/errors_spec.rb | 18 +++++++++++++ .../internal_server_error.html.erb_spec.rb | 5 ++++ spec/views/errors/not_found.html.erb_spec.rb | 5 ++++ 11 files changed, 88 insertions(+) create mode 100644 app/controllers/errors_controller.rb create mode 100644 app/helpers/errors_helper.rb create mode 100644 app/views/errors/internal_server_error.html.erb create mode 100644 app/views/errors/not_found.html.erb create mode 100644 lib/custom_exceptions_app_wrapper.rb create mode 100644 spec/helpers/errors_helper_spec.rb create mode 100644 spec/requests/errors_spec.rb create mode 100644 spec/views/errors/internal_server_error.html.erb_spec.rb create mode 100644 spec/views/errors/not_found.html.erb_spec.rb diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb new file mode 100644 index 0000000000..0ca5908c38 --- /dev/null +++ b/app/controllers/errors_controller.rb @@ -0,0 +1,7 @@ +class ErrorsController < ApplicationController + def not_found + end + + def internal_server_error + end +end diff --git a/app/helpers/errors_helper.rb b/app/helpers/errors_helper.rb new file mode 100644 index 0000000000..8e3b415c57 --- /dev/null +++ b/app/helpers/errors_helper.rb @@ -0,0 +1,2 @@ +module ErrorsHelper +end diff --git a/app/views/errors/internal_server_error.html.erb b/app/views/errors/internal_server_error.html.erb new file mode 100644 index 0000000000..6025472125 --- /dev/null +++ b/app/views/errors/internal_server_error.html.erb @@ -0,0 +1,2 @@ +

Errors#internal_server_error

+

Find me in app/views/errors/internal_server_error.html.erb

diff --git a/app/views/errors/not_found.html.erb b/app/views/errors/not_found.html.erb new file mode 100644 index 0000000000..7f18c8062a --- /dev/null +++ b/app/views/errors/not_found.html.erb @@ -0,0 +1,2 @@ +

Errors#not_found

+

Find me in app/views/errors/not_found.html.erb

diff --git a/config/application.rb b/config/application.rb index fb4ea6d559..e9a8d2cfa2 100644 --- a/config/application.rb +++ b/config/application.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative "boot" +require_relative "../lib/custom_exceptions_app_wrapper" require "rails/all" require "will_paginate/array" @@ -80,6 +81,10 @@ class Application < Rails::Application # Prevent invalid (usually malicious) URLs from causing exceptions/issues config.middleware.insert 0, Rack::UTF8Sanitizer + # Use a custom exceptions app to handle mime type exceptions + # https://guides.rubyonrails.org/configuring.html#config-exceptions-app + config.exceptions_app = CustomExceptionsAppWrapper.new(exceptions_app: routes) + config.active_storage.variant_processor = :vips end diff --git a/config/routes.rb b/config/routes.rb index 12a2506960..cb335f47a4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,6 +3,8 @@ require "facility_product_routing_concern" Rails.application.routes.draw do + get 'errors/not_found' + get 'errors/internal_server_error' get "/users/sign_in.pdf" => redirect("/users/sign_in") devise_for :users mount SangerSequencing::Engine => "/" if defined?(SangerSequencing) diff --git a/lib/custom_exceptions_app_wrapper.rb b/lib/custom_exceptions_app_wrapper.rb new file mode 100644 index 0000000000..c9526d58f8 --- /dev/null +++ b/lib/custom_exceptions_app_wrapper.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class CustomExceptionsAppWrapper + + def initialize(exceptions_app:) + @exceptions_app = exceptions_app + end + + def call(env) + request = ActionDispatch::Request.new(env) + + fallback_to_html_format_if_invalid_mime_type(request) + + @exceptions_app.call(env) + end + + private + + def fallback_to_html_format_if_invalid_mime_type(request) + request.formats + rescue ActionDispatch::Http::MimeNegotiation::InvalidType + request.set_header "CONTENT_TYPE", "text/html" + end + +end diff --git a/spec/helpers/errors_helper_spec.rb b/spec/helpers/errors_helper_spec.rb new file mode 100644 index 0000000000..e3592037d3 --- /dev/null +++ b/spec/helpers/errors_helper_spec.rb @@ -0,0 +1,15 @@ +require 'rails_helper' + +# Specs in this file have access to a helper object that includes +# the ErrorsHelper. For example: +# +# describe ErrorsHelper do +# describe "string concat" do +# it "concats two strings with spaces" do +# expect(helper.concat_strings("this","that")).to eq("this that") +# end +# end +# end +RSpec.describe ErrorsHelper, type: :helper do + pending "add some examples to (or delete) #{__FILE__}" +end diff --git a/spec/requests/errors_spec.rb b/spec/requests/errors_spec.rb new file mode 100644 index 0000000000..436204d3c7 --- /dev/null +++ b/spec/requests/errors_spec.rb @@ -0,0 +1,18 @@ +require 'rails_helper' + +RSpec.describe "Errors", type: :request do + describe "GET /not_found" do + it "returns http success" do + get "/errors/not_found" + expect(response).to have_http_status(:success) + end + end + + describe "GET /internal_server_error" do + it "returns http success" do + get "/errors/internal_server_error" + expect(response).to have_http_status(:success) + end + end + +end diff --git a/spec/views/errors/internal_server_error.html.erb_spec.rb b/spec/views/errors/internal_server_error.html.erb_spec.rb new file mode 100644 index 0000000000..2ae6ed2c2b --- /dev/null +++ b/spec/views/errors/internal_server_error.html.erb_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe "errors/internal_server_error.html.erb", type: :view do + pending "add some examples to (or delete) #{__FILE__}" +end diff --git a/spec/views/errors/not_found.html.erb_spec.rb b/spec/views/errors/not_found.html.erb_spec.rb new file mode 100644 index 0000000000..4e0eed49ea --- /dev/null +++ b/spec/views/errors/not_found.html.erb_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe "errors/not_found.html.erb", type: :view do + pending "add some examples to (or delete) #{__FILE__}" +end From 7d5af6590554218989f07ecca7c2544bdedb96a5 Mon Sep 17 00:00:00 2001 From: Gilad Shanan Date: Mon, 18 Dec 2023 16:19:31 -0600 Subject: [PATCH 2/7] Refactor to use ErrorsController for 404s --- app/controllers/application_controller.rb | 15 ------------- app/controllers/errors_controller.rb | 15 ++++++++++++- app/views/404.html.erb | 2 -- app/views/errors/not_found.html.erb | 4 ++-- config/routes.rb | 6 ++++-- public/404.html | 26 ----------------------- 6 files changed, 20 insertions(+), 48 deletions(-) delete mode 100644 app/views/404.html.erb delete mode 100644 public/404.html diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0b26c065ad..3cb977d1c1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -119,21 +119,6 @@ def acting_as? end # Global exception handlers - rescue_from ActiveRecord::RecordNotFound do |exception| - Rails.logger.debug("#{exception.message}: #{exception.backtrace.join("\n")}") unless Rails.env.production? - render_404(exception) - end - - rescue_from ActionController::RoutingError do |exception| - Rails.logger.debug("#{exception.message}: #{exception.backtrace.join("\n")}") unless Rails.env.production? - render_404(exception) - end - - def render_404(_exception) - # Add html fallback in case the 404 is a PDF or XML so the view can be found - render "/404", status: 404, layout: "application", formats: formats_with_html_fallback - end - rescue_from NUCore::PermissionDenied, CanCan::AccessDenied, with: :render_403 def render_403(_exception) # if current_user is nil, the user should be redirected to login diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index 0ca5908c38..59086931a0 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -1,7 +1,20 @@ +# frozen_string_literal: true + class ErrorsController < ApplicationController + def not_found + respond_to do |format| + format.html { render status: :not_found } + end + rescue ActionController::UnknownFormat + head :not_found end - def internal_server_error + def internal_error + respond_to do |format| + format.html { render status: :internal_server_error } + end + rescue ActionController::UnknownFormat + head :internal_server_error end end diff --git a/app/views/404.html.erb b/app/views/404.html.erb deleted file mode 100644 index 80d4b61f82..0000000000 --- a/app/views/404.html.erb +++ /dev/null @@ -1,2 +0,0 @@ -<% content_for :h1 do %>404 – Page Not Found<% end %> -Sorry, we could not find the page you are looking for. diff --git a/app/views/errors/not_found.html.erb b/app/views/errors/not_found.html.erb index 7f18c8062a..80d4b61f82 100644 --- a/app/views/errors/not_found.html.erb +++ b/app/views/errors/not_found.html.erb @@ -1,2 +1,2 @@ -

Errors#not_found

-

Find me in app/views/errors/not_found.html.erb

+<% content_for :h1 do %>404 – Page Not Found<% end %> +Sorry, we could not find the page you are looking for. diff --git a/config/routes.rb b/config/routes.rb index cb335f47a4..43fb3a5cca 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,8 +3,6 @@ require "facility_product_routing_concern" Rails.application.routes.draw do - get 'errors/not_found' - get 'errors/internal_server_error' get "/users/sign_in.pdf" => redirect("/users/sign_in") devise_for :users mount SangerSequencing::Engine => "/" if defined?(SangerSequencing) @@ -431,4 +429,8 @@ # See config/initializers/health_check.rb for more information health_check_routes + + # Handle errors + match "/404", to: "errors#not_found", via: :all + match "/500", to: "errors#internal_server_error", via: :all end diff --git a/public/404.html b/public/404.html deleted file mode 100644 index 9a48320a5f..0000000000 --- a/public/404.html +++ /dev/null @@ -1,26 +0,0 @@ - - - - The page you were looking for doesn't exist (404) - - - - - -
-

The page you were looking for doesn't exist.

-

You may have mistyped the address or the page may have moved.

-
- - From 94809817c8260cc1980e10bdc8d8964f213d09c8 Mon Sep 17 00:00:00 2001 From: Gilad Shanan Date: Mon, 18 Dec 2023 16:26:22 -0600 Subject: [PATCH 3/7] Use ErrorsController for 500s --- app/controllers/errors_controller.rb | 2 +- .../errors/internal_server_error.html.erb | 5 ++-- public/500.html | 26 ------------------- 3 files changed, 4 insertions(+), 29 deletions(-) delete mode 100644 public/500.html diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index 59086931a0..d77f1faf37 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -10,7 +10,7 @@ def not_found head :not_found end - def internal_error + def internal_server_error respond_to do |format| format.html { render status: :internal_server_error } end diff --git a/app/views/errors/internal_server_error.html.erb b/app/views/errors/internal_server_error.html.erb index 6025472125..589bfa30d8 100644 --- a/app/views/errors/internal_server_error.html.erb +++ b/app/views/errors/internal_server_error.html.erb @@ -1,2 +1,3 @@ -

Errors#internal_server_error

-

Find me in app/views/errors/internal_server_error.html.erb

+<% content_for :h1 do %>500 – Internal Server Error<% end %> +We're sorry, but something went wrong. +We've been notified about this issue and we'll take a look at it shortly. diff --git a/public/500.html b/public/500.html deleted file mode 100644 index b80307fc16..0000000000 --- a/public/500.html +++ /dev/null @@ -1,26 +0,0 @@ - - - - We're sorry, but something went wrong (500) - - - - - -
-

We're sorry, but something went wrong.

-

We've been notified about this issue and we'll take a look at it shortly.

-
- - From 11a37cddf7066822d9076ec3bfc5811bcf9cc9ea Mon Sep 17 00:00:00 2001 From: Gilad Shanan Date: Mon, 18 Dec 2023 16:26:38 -0600 Subject: [PATCH 4/7] Remove likely unused 422 page --- public/422.html | 26 -------------------------- 1 file changed, 26 deletions(-) delete mode 100644 public/422.html diff --git a/public/422.html b/public/422.html deleted file mode 100644 index 83660ab187..0000000000 --- a/public/422.html +++ /dev/null @@ -1,26 +0,0 @@ - - - - The change you wanted was rejected (422) - - - - - -
-

The change you wanted was rejected.

-

Maybe you tried to change something you didn't have access to.

-
- - From 822748b7631c84592c4ee954f5fdcd3ebefa8049 Mon Sep 17 00:00:00 2001 From: Gilad Shanan Date: Mon, 18 Dec 2023 22:06:09 -0600 Subject: [PATCH 5/7] Handle 403s --- app/controllers/application_controller.rb | 17 ----------------- app/controllers/errors_controller.rb | 13 +++++++++++++ app/views/{ => errors}/403.html.erb | 0 app/views/{ => errors}/acting_error.html.haml | 2 +- config/application.rb | 4 ++++ config/routes.rb | 1 + 6 files changed, 19 insertions(+), 18 deletions(-) rename app/views/{ => errors}/403.html.erb (100%) rename app/views/{ => errors}/acting_error.html.haml (83%) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3cb977d1c1..ecf06ede13 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -118,23 +118,6 @@ def acting_as? acting_user.object_id != session_user.object_id end - # Global exception handlers - rescue_from NUCore::PermissionDenied, CanCan::AccessDenied, with: :render_403 - def render_403(_exception) - # if current_user is nil, the user should be redirected to login - if current_user - render "/403", status: 403, layout: "application", formats: formats_with_html_fallback - else - store_location_for(:user, request.fullpath) - redirect_to new_user_session_path - end - end - - rescue_from NUCore::NotPermittedWhileActingAs, with: :render_acting_error - def render_acting_error - render "/acting_error", status: 403, layout: "application", formats: formats_with_html_fallback - end - def after_sign_out_path_for(_) if current_facility.present? facility_path(current_facility) diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index d77f1faf37..cec56be8c5 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -17,4 +17,17 @@ def internal_server_error rescue ActionController::UnknownFormat head :internal_server_error end + + def forbidden + if request.env["action_dispatch.exception"].instance_of? NUCore::NotPermittedWhileActingAs + render "acting_error", status: 403, formats: formats_with_html_fallback + elsif current_user + render "403", status: 403, formats: formats_with_html_fallback + else + # if current_user is nil, the user should be redirected to login + store_location_for(:user, request.fullpath) + redirect_to new_user_session_path + end + end + end diff --git a/app/views/403.html.erb b/app/views/errors/403.html.erb similarity index 100% rename from app/views/403.html.erb rename to app/views/errors/403.html.erb diff --git a/app/views/acting_error.html.haml b/app/views/errors/acting_error.html.haml similarity index 83% rename from app/views/acting_error.html.haml rename to app/views/errors/acting_error.html.haml index 94a7f8bd00..b6dfdf6f07 100644 --- a/app/views/acting_error.html.haml +++ b/app/views/errors/acting_error.html.haml @@ -1 +1 @@ -%p.notice This function is unavailable while you are acting as another user. \ No newline at end of file +%p.notice This function is unavailable while you are acting as another user. diff --git a/config/application.rb b/config/application.rb index e9a8d2cfa2..ac7b39b347 100644 --- a/config/application.rb +++ b/config/application.rb @@ -85,6 +85,10 @@ class Application < Rails::Application # https://guides.rubyonrails.org/configuring.html#config-exceptions-app config.exceptions_app = CustomExceptionsAppWrapper.new(exceptions_app: routes) + config.action_dispatch.rescue_responses["NUCore::PermissionDenied"] = :forbidden + config.action_dispatch.rescue_responses["CanCan::AccessDenied"] = :forbidden + config.action_dispatch.rescue_responses["NUCore::NotPermittedWhileActingAs"] = :forbidden + config.active_storage.variant_processor = :vips end diff --git a/config/routes.rb b/config/routes.rb index 43fb3a5cca..d70d589fe5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -431,6 +431,7 @@ health_check_routes # Handle errors + match "/403", to: "errors#forbidden", via: :all match "/404", to: "errors#not_found", via: :all match "/500", to: "errors#internal_server_error", via: :all end From f0b96ce22808aa96d516342b11322a070d7f8413 Mon Sep 17 00:00:00 2001 From: Gilad Shanan Date: Mon, 18 Dec 2023 22:20:22 -0600 Subject: [PATCH 6/7] Refactor 403 handling for consistency --- app/controllers/errors_controller.rb | 20 +++++++++++++++---- app/views/errors/acting_error.html.haml | 1 - .../{403.html.erb => forbidden.html.erb} | 2 +- 3 files changed, 17 insertions(+), 6 deletions(-) delete mode 100644 app/views/errors/acting_error.html.haml rename app/views/errors/{403.html.erb => forbidden.html.erb} (53%) diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb index cec56be8c5..5b7753192d 100644 --- a/app/controllers/errors_controller.rb +++ b/app/controllers/errors_controller.rb @@ -19,15 +19,27 @@ def internal_server_error end def forbidden - if request.env["action_dispatch.exception"].instance_of? NUCore::NotPermittedWhileActingAs - render "acting_error", status: 403, formats: formats_with_html_fallback - elsif current_user - render "403", status: 403, formats: formats_with_html_fallback + @error_message = if acting_error? + "This function is unavailable while you are acting as another user." + else + "Sorry, you don't have permission to access this page." + end + + if current_user || acting_error? + respond_to do |format| + format.html { render status: :forbidden } + end else # if current_user is nil, the user should be redirected to login store_location_for(:user, request.fullpath) redirect_to new_user_session_path end + rescue ActionController::UnknownFormat + head :forbidden + end + + def acting_error? + request.env["action_dispatch.exception"].instance_of?(NUCore::NotPermittedWhileActingAs) end end diff --git a/app/views/errors/acting_error.html.haml b/app/views/errors/acting_error.html.haml deleted file mode 100644 index b6dfdf6f07..0000000000 --- a/app/views/errors/acting_error.html.haml +++ /dev/null @@ -1 +0,0 @@ -%p.notice This function is unavailable while you are acting as another user. diff --git a/app/views/errors/403.html.erb b/app/views/errors/forbidden.html.erb similarity index 53% rename from app/views/errors/403.html.erb rename to app/views/errors/forbidden.html.erb index bbc7a7ec42..0ba1d1064e 100644 --- a/app/views/errors/403.html.erb +++ b/app/views/errors/forbidden.html.erb @@ -1,2 +1,2 @@ <% content_for :h1 do %>403 – Permission Denied<% end %> -Sorry, you don't have permission to access this page. +

<%= @error_message %>

From 85d411636d2049efe45ef5bb79cacf8006f3e368 Mon Sep 17 00:00:00 2001 From: Leticia Errandonea Date: Thu, 21 Dec 2023 15:04:07 -0300 Subject: [PATCH 7/7] Remove unused boilerplate --- app/helpers/errors_helper.rb | 2 -- spec/helpers/errors_helper_spec.rb | 15 --------------- .../errors/internal_server_error.html.erb_spec.rb | 5 ----- spec/views/errors/not_found.html.erb_spec.rb | 5 ----- 4 files changed, 27 deletions(-) delete mode 100644 app/helpers/errors_helper.rb delete mode 100644 spec/helpers/errors_helper_spec.rb delete mode 100644 spec/views/errors/internal_server_error.html.erb_spec.rb delete mode 100644 spec/views/errors/not_found.html.erb_spec.rb diff --git a/app/helpers/errors_helper.rb b/app/helpers/errors_helper.rb deleted file mode 100644 index 8e3b415c57..0000000000 --- a/app/helpers/errors_helper.rb +++ /dev/null @@ -1,2 +0,0 @@ -module ErrorsHelper -end diff --git a/spec/helpers/errors_helper_spec.rb b/spec/helpers/errors_helper_spec.rb deleted file mode 100644 index e3592037d3..0000000000 --- a/spec/helpers/errors_helper_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -require 'rails_helper' - -# Specs in this file have access to a helper object that includes -# the ErrorsHelper. For example: -# -# describe ErrorsHelper do -# describe "string concat" do -# it "concats two strings with spaces" do -# expect(helper.concat_strings("this","that")).to eq("this that") -# end -# end -# end -RSpec.describe ErrorsHelper, type: :helper do - pending "add some examples to (or delete) #{__FILE__}" -end diff --git a/spec/views/errors/internal_server_error.html.erb_spec.rb b/spec/views/errors/internal_server_error.html.erb_spec.rb deleted file mode 100644 index 2ae6ed2c2b..0000000000 --- a/spec/views/errors/internal_server_error.html.erb_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'rails_helper' - -RSpec.describe "errors/internal_server_error.html.erb", type: :view do - pending "add some examples to (or delete) #{__FILE__}" -end diff --git a/spec/views/errors/not_found.html.erb_spec.rb b/spec/views/errors/not_found.html.erb_spec.rb deleted file mode 100644 index 4e0eed49ea..0000000000 --- a/spec/views/errors/not_found.html.erb_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'rails_helper' - -RSpec.describe "errors/not_found.html.erb", type: :view do - pending "add some examples to (or delete) #{__FILE__}" -end