Commit 78b58ef1 authored by azul's avatar azul

Merge branch '191-reduce-use-of-error-app' into 'staging'

handle 404 ourself rather than with exception app

See merge request !268
parents 5e5a2487 bcef262d
......@@ -11,7 +11,7 @@ class AssetsController < ApplicationController
type: file.content_type,
disposition: disposition(file)
rescue ActionController::MissingFile
raise_not_found
raise_not_found :file
end
def destroy
......@@ -33,7 +33,7 @@ class AssetsController < ApplicationController
def fetch_asset
@asset = Asset.find(params[:id]).version_or_self(params[:version])
raise ErrorNotFound, :file unless @asset
raise_not_found :file unless @asset
@page = @asset.page
true
end
......@@ -55,7 +55,7 @@ class AssetsController < ApplicationController
def file_to_send
if thumb_name
thumb = @asset.thumbnail(thumb_name)
raise ErrorNotFound, :file unless thumb
raise_not_found :file unless thumb
thumb.generate
thumb
else
......@@ -63,7 +63,7 @@ class AssetsController < ApplicationController
end
rescue Errno::ENOENT => e
Rails.logger.warn "WARNING: Asset not found: #{thumb_name}"
raise ErrorNotFound, :file
raise_not_found :file
end
#
......
......@@ -96,17 +96,17 @@ module Common::Application::AlertMessages
end
#
# We use the default rails error to trigger the ErrorApp middleware.
# This will then in turn call ExceptionsController#show as defined in
# config.exceptions_app
# Use this function with a symbol for the thing you are missing:
# `raise_not_found :file`
#
# Why?
# Because we need to get rid of all Controller state.
# * Instance variables may leak information.
# * Controller functions like setup_navigation may crash.
# It will wrap the thing in an exception.
# `Common::Application::RescueErrors` will `rescue_from` this exception
# with `render_not_found`.
#
# `render_not_found` in turn will use `translate_exception`
# as defined below to find the right translation for the error
# message.
#
# At the same time redirect would alter the url in the users browser.
# Maybe they just typed it wrong. So we better leave it there.
#
def raise_not_found(thing = nil)
raise ErrorNotFound.new(thing)
......
......@@ -2,7 +2,6 @@ module Common::Application::RenderWithViewSetup
def render(*args)
setup_theme
setup_context
super(*args)
end
......
#
# methods and helpers for context and navigation that should
# be available in all controllers.
#
module Common::Application::ContextNavigation
def self.included(base)
base.class_eval do
helper_method :setup_navigation
end
end
protected
##
## OVERRIDE
##
def setup_navigation(nav)
nav
# this can be implemented by controller subclasses
end
end
......@@ -43,9 +43,9 @@ module Common::Application::RescueErrors
# Use the ExceptionApp with ExceptionsController for these:
# ( this is the default for errors that do not inherit from
# one of the above)
rescue_from ErrorNotFound, with: :raise
rescue_from AuthenticationRequired, with: :raise
rescue_from PermissionDenied, with: :raise
rescue_from ErrorNotFound, with: :render_not_found
rescue_from AuthenticationRequired, with: :render_exception
rescue_from PermissionDenied, with: :render_exception
rescue_from Pundit::NotAuthorizedError, with: :log_and_permission_denied
end
......@@ -115,10 +115,37 @@ module Common::Application::RescueErrors
end
end
#
# shows a generic not found page or error message, customized
# by any message in the exception.
#
# Please do not call this directly but rather use
# `raise_not_found :file`
#
def render_not_found(exception=nil)
render_exception exception || ErrorNotFound.new(:page)
end
def render_exception(exception)
@exception = exception
status = status_for_exception(exception)
respond_to do |format|
format.html do
render 'exceptions/show', status: status, layout: (!request.xhr? && 'notice')
end
format.js do
render_error_js exception, status: status
end
format.any do
render status: status, body: nil
end
end
end
def log_and_permission_denied(exception)
Rails.logger.info exception
Rails.logger.info "User id: #{current_user.id}"
raise PermissionDenied
render_exception PermissionDenied.new
end
def status_for_exception(exception)
......
......@@ -86,6 +86,8 @@ module Common::Requests
if params[:code] && @req.recipient != current_user
@req.try.redeem_code!(current_user)
end
rescue ActiveRecord::RecordNotFound => e
raise_not_found
end
def request_context
......
......@@ -33,7 +33,11 @@ class ContextPagesController < DispatchController
@page = finder.page
@group = finder.group
@user = finder.user
new_controller controller_name
if @page
new_controller @page.controller
else
controller_for_missing_page
end
end
def finder
......@@ -41,21 +45,12 @@ class ContextPagesController < DispatchController
params[:id]
end
def controller_name
if @page
@page.controller
else
controller_for_missing_page
end
end
def controller_for_missing_page
if create_page?
prepare_params_to_create_page
'page/create'
new_controller 'page/create'
else
prepare_params_for_not_found
'exceptions'
not_found
end
end
......@@ -75,10 +70,6 @@ class ContextPagesController < DispatchController
page: { title: new_title }
end
def prepare_params_for_not_found
request.env['action_dispatch.exception'] = ErrorNotFound.new(:page)
end
def new_title
params[:id].sub(/\+\d*/, '').split('+').join(' ').humanize
end
......
......@@ -13,22 +13,24 @@ class ContextsController < DispatchController
def find_controller
name = params[:id]
controller_for_group(name)
rescue ActiveRecord::RecordNotFound
controller_for_person(name)
controller_for_group(name) ||
controller_for_person(name) ||
not_found
end
def controller_for_group(name)
# we are dealing with a committee!
name.sub!(' ', '+') if name =~ /\ /
@group = Group.where(name: name).first!
@group = Group.where(name: name).first
return unless @group
params[:group_id] = params[:id]
new_controller 'group/home'
end
def controller_for_person(login)
@user = User.where(login: login).first!
@user = User.where(login: login).first
return unless @user
params[:person_id] = params[:id]
new_controller 'person/home'
end
......
......@@ -30,6 +30,15 @@ class DispatchController < ApplicationController
end
end
def not_found
prepare_params_for_not_found
new_controller 'exceptions'
end
def prepare_params_for_not_found
request.env['action_dispatch.exception'] = ErrorNotFound.new(:page)
end
# We want the modification to also apply to the newly instantiated controller.
# So we have to modify the request - not just the Parameters instance.
def modify_params(options = {})
......
......@@ -3,6 +3,7 @@ class Group::BaseController < ApplicationController
# default permission for all group controllers
before_action :login_required
before_action :setup_context
after_action :verify_authorized
helper 'group/links'
......@@ -12,7 +13,7 @@ class Group::BaseController < ApplicationController
def fetch_group
# group might be preloaded by DispatchController
@group ||= Group.find_by_name(params[:group_id] || params[:id])
raise_not_found if @group.nil? || !policy(@group).show?
raise_not_found(:page) if @group.nil? || !policy(@group).show?
@membership = Group::Membership.where(group: @group, user: current_user).
first_or_initialize
end
......
......@@ -7,18 +7,18 @@ class Group::PagesController < Group::BaseController
authorize @group, :show?
@path = apply_path_modifiers(parsed_path)
@pages = Page.paginate_by_path(@path, options_for_group(@group), pagination_params)
@page_search_navigation = page_search_navigation
render template: 'common/pages/search/index', locals: { hide_owner: true }
end
protected
def setup_navigation(nav)
nav[:local] = [
def page_search_navigation
[
{ active: false, visible: policy(@group).edit?, html: { partial: 'common/pages/search/create' } },
{ active: true, visible: true, html: { partial: 'common/pages/search/controls_active' } },
{ active: false, visible: true, html: { partial: 'common/pages/search/controls_possible' } }
]
nav
end
#
......
......@@ -2,7 +2,7 @@
# Abstract super class of all the Me controllers.
#
class Me::BaseController < ApplicationController
before_action :login_required, :fetch_user
before_action :login_required, :fetch_user, :setup_context
protected
......
......@@ -6,6 +6,7 @@ class Me::PagesController < Me::BaseController
#
def index
@path = apply_path_modifiers(parsed_path)
@page_search_navigation = page_search_navigation
if request.xhr?
# note: pagination_params is used just for defaults,
# normal pagination is done through @path.
......@@ -16,13 +17,12 @@ class Me::PagesController < Me::BaseController
protected
def setup_navigation(nav)
nav[:local] = [
def page_search_navigation
[
{ active: false, visible: true, html: { partial: 'common/pages/search/create' } },
{ active: true, visible: true, html: { partial: 'common/pages/search/controls_active' } },
{ active: false, visible: true, html: { partial: 'common/pages/search/controls_possible' } }
]
nav
end
#
......
......@@ -7,6 +7,7 @@ class Page::BaseController < ApplicationController
before_action :login_required, except: :show
before_action :bust_cache, only: :show
before_action :setup_context
after_action :verify_authorized
layout 'page'
......
......@@ -17,7 +17,7 @@ module Page::BeforeFilters
#
def default_fetch_data
@page ||= Page.find(params[:page_id] || params[:id])
raise ErrorNotFound, :page unless @page && policy(@page).show?
raise_not_found unless @page && policy(@page).show?
if logged_in?
# grab the current user's participation from memory
......@@ -28,11 +28,6 @@ module Page::BeforeFilters
fetch_data
true
rescue ErrorNotFound => e
@exception = e
render 'exceptions/show',
status: 404,
layout: request.xhr? ? nil : 'notice'
end
def default_setup_options
......
......@@ -16,7 +16,7 @@ class Page::CreateController < ApplicationController
include Common::Tracking::Action
before_action :login_required
before_action :init_options, :set_owner, :catch_cancel
before_action :init_options, :set_owner, :setup_context, :catch_cancel
after_action :verify_authorized, only: :create
helper 'page/share', 'page/owner', 'page/creation'
track_actions :create
......
class Person::BaseController < ApplicationController
before_action :fetch_person
before_action :setup_context
after_action :verify_authorized
helper 'people/base'
......@@ -8,8 +9,8 @@ class Person::BaseController < ApplicationController
def fetch_person
# person might be preloaded by DispatchController
@user ||= User.where(login: (params[:person_id] || params[:id])).first!
raise_not_found unless policy(@user).show?
@user ||= User.where(login: (params[:person_id] || params[:id])).first
raise_not_found(:page) unless @user && policy(@user).show?
end
def setup_context
......
......@@ -5,17 +5,17 @@ class Person::PagesController < Person::BaseController
authorize @user, :show?
@path = apply_path_modifiers(parsed_path)
@pages = Page.paginate_by_path(@path, options_for_user(@user), pagination_params)
@page_search_navigation = page_search_navigation
render template: 'common/pages/search/index'
end
protected
def setup_navigation(nav)
nav[:local] = [
def page_search_navigation
[
{ active: true, visible: true, html: { partial: 'common/pages/search/controls_active' } },
{ active: false, visible: true, html: { partial: 'common/pages/search/controls_possible' } }
]
nav
end
#
......
class Wiki::BaseController < ApplicationController
before_action :fetch_wiki
before_action :login_required
before_action :setup_context
after_action :verify_authorized
helper 'wikis/base'
......
......@@ -24,8 +24,6 @@ module Common::Utility::ContextHelper
navigation[:local] = navigation[:context].currently_active_item
end
end
# allow controller change to modify @navigation
navigation = setup_navigation(navigation)
navigation
end
end
......
- @local_layout = 'sidebar'
- content_for :title do
= render 'layouts/section/heading_with_info', section: :page_search, first: true
- content_for :left_column do
= render 'layouts/local/side_navigation', items: @page_search_navigation
= render 'common/pages/search/top_controls'
#search_results{data: {search: 'path', href: page_search_path}}
......
......@@ -6,8 +6,10 @@
-# the navigation elements from other ul, li, and <a>'s that might be around in the sidebar.
-#
- items = local_assigns[:items] || @navigation[:local]
%ul.nav.nav-pills.nav-stacked
- @navigation[:local].each do |item|
- items.each do |item|
- next unless item[:visible]
- active = item[:active] ? 'active' : ''
%li{class: [active]}
......
......@@ -7,18 +7,13 @@ class GalleryImageController < Page::BaseController
def show
authorize @page
@showing = @page.try.showings.includes(:asset).find_by_asset_id(params[:id])
raise ErrorNotFound, :file unless @showing.try.asset
raise_not_found :file unless @showing.try.asset
@image = @showing.asset
# position sometimes starts at 0 and sometimes at 1?
@image_index = @page.images.index(@image).next
@image_count = @page.showings.count
@next = @showing.lower_item
@previous = @showing.higher_item
rescue ErrorNotFound => e
@exception = e
render 'exceptions/show',
status: 404,
layout: request.xhr? ? nil : 'notice'
end
# removed an non ajax fallback, azul
......
......@@ -111,7 +111,7 @@ define_navigation do
global_section :people do
label { :people.t }
url controller: 'person/directory'
active { controller?('person/') or (context?(:user) && visible_context?) }
active { controller?('person/directory') || (context?(:user) && visible_context?)}
html partial: '/layouts/global/nav/people_menu'
context_section :no_context do
......@@ -163,7 +163,7 @@ define_navigation do
global_section :group do
label { :groups.t }
url { groups_directory_path }
active { controller?('group/') or (context?(:group) && visible_context?) }
active { controller?('group/directory') || ( context?(:group) && visible_context? ) }
html partial: '/layouts/global/nav/groups_menu'
context_section :directory do
......
......@@ -16,9 +16,8 @@ class AssetsControllerTest < ActionController::TestCase
def test_get_permissions
page = FactoryBot.create :page
asset = FactoryBot.create :image_asset, parent_page: page
assert_permission_denied do
get :show, params: { id: asset.id, path: asset.basename }
end
get :show, params: { id: asset.id, path: asset.basename }
assert_permission_denied
end
def test_get_with_escaped_chars
......@@ -66,9 +65,8 @@ class AssetsControllerTest < ActionController::TestCase
asset = page.add_attachment! uploaded_data: upload_data('photo.jpg')
user.updated(page)
login_as user
assert_permission_denied do
delete :destroy, params: { id: asset.id, page_id: page.id }
end
delete :destroy, params: { id: asset.id, page_id: page.id }
assert_permission_denied
end
private
......
......@@ -6,9 +6,8 @@ class Group::DirectoryControllerTest < ActionController::TestCase
end
def test_index_requires_login
assert_login_required do
get :index
end
get :index
assert_login_required
end
def test_index
......
......@@ -6,9 +6,8 @@ class Group::GroupsControllerTest < ActionController::TestCase
end
def test_new_group_requires_login
assert_login_required do
get :new
end
get :new
assert_login_required
end
def test_choose_group_type
......@@ -111,9 +110,8 @@ class Group::GroupsControllerTest < ActionController::TestCase
user = FactoryBot.create(:user)
group = FactoryBot.create(:group)
login_as user
assert_not_found do
delete :destroy, params: { id: group.to_param }
end
delete :destroy, params: { id: group.to_param }
assert_not_found
end
end
......@@ -37,8 +37,7 @@ class Group::HomeControllerTest < ActionController::TestCase
def test_may_not_show
login_as FactoryBot.create(:user)
@group.revoke_access! public: :view
assert_not_found do
get :show, params: { group_id: @group.to_param }
end
get :show, params: { group_id: @group.to_param }
assert_not_found
end
end
......@@ -16,9 +16,8 @@ class Group::PermissionsControllerTest < ActionController::TestCase
def test_index_no_access
login_as @other_user
assert_not_found do
get :index, params: { group_id: @group.to_param }
end
get :index, params: { group_id: @group.to_param }
assert_not_found
end
def test_update
......
......@@ -16,9 +16,8 @@ class Group::ProfilesControllerTest < ActionController::TestCase
def test_edit_not_allowed
stranger = FactoryBot.create(:user)
login_as stranger
assert_not_found do
get :edit, params: { group_id: @group.to_param }
end
get :edit, params: { group_id: @group.to_param }
assert_not_found
end
def test_update
......@@ -33,9 +32,8 @@ class Group::ProfilesControllerTest < ActionController::TestCase
def test_update
stranger = FactoryBot.create(:user)
login_as stranger
assert_not_found do
post :update, params: { group_id: @group.to_param, profile: { summary: 'test profile', entity_id: 1 } }
end
post :update, params: { group_id: @group.to_param, profile: { summary: 'test profile', entity_id: 1 } }
assert_not_found
end
end
......@@ -16,9 +16,8 @@ class Group::RequestsControllerTest < ActionController::TestCase
def test_index_not_allowed
stranger = FactoryBot.create(:user)
login_as stranger
assert_not_found do
get :index, params: { group_id: @group.to_param }
end
get :index, params: { group_id: @group.to_param }
assert_not_found
end
def test_create
......@@ -31,9 +30,8 @@ class Group::RequestsControllerTest < ActionController::TestCase
def test_create_not_allowed
stranger = FactoryBot.create(:user)
login_as stranger
assert_not_found do
get :create, params: { group_id: @group.to_param, type: 'destroy_group' }
end
get :create, params: { group_id: @group.to_param, type: 'destroy_group' }
assert_not_found
end
def test_request_to_create_council
......@@ -50,9 +48,8 @@ class Group::RequestsControllerTest < ActionController::TestCase
def test_request_to_create_council_not_allowed
group = groups(:animals)
assert_no_difference 'RequestToCreateCouncil.count' do
assert_permission_denied do
get :create, params: { group_id: group.to_param, type: 'create_council' }
end
get :create, params: { group_id: group.to_param, type: 'create_council' }
assert_permission_denied
end
end
......
......@@ -16,17 +16,15 @@ class Group::SettingsControllerTest < ActionController::TestCase
end
def test_not_logged_in
assert_login_required do
get :show, params: { group_id: @group.to_param }
end
get :show, params: { group_id: @group.to_param }
assert_login_required
end
def test_not_a_member
stranger = FactoryBot.create(:user)
login_as stranger
assert_permission_denied do
get :show, params: { group_id: @group.to_param }
end
get :show, params: { group_id: @group.to_param }
assert_permission_denied
end
def test_member_can_see_private
......@@ -47,9 +45,8 @@ class Group::SettingsControllerTest < ActionController::TestCase
def test_update_not_allowed
stranger = FactoryBot.create(:user)
login_as stranger
assert_permission_denied do
post :update, params: { group: { full_name: 'full name' }, group_id: @group.to_param }
end
post :update, params: { group: { full_name: 'full name' }, group_id: @group.to_param }
assert_permission_denied
end
end
......@@ -24,9 +24,8 @@ class Group::StructuresControllerTest < ActionController::TestCase
def test_create_committee_not_allowed
stranger = FactoryBot.create(:user)
login_as stranger
assert_not_found do
get :create, params: { group_id: @group.to_param, type: 'committee', group: committee_attributes }
end
get :create, params: { group_id: @group.to_param, type: 'committee', group: committee_attributes }
assert_not_found
end
def test_create_committee_namespace
......@@ -61,9 +60,8 @@ class Group::StructuresControllerTest < ActionController::TestCase
def test_create_council_not_allowed
stranger = FactoryBot.create(:user)
login_as stranger
assert_not_found do
get :create, params: { group_id: @group.to_param, type: 'council', group: committee_attributes }
end
get :create, params: { group_id: @group.to_param, type: 'council', group: committee_attributes }
assert_not_found
end
def committee_attributes(attrs = {})
......
......@@ -17,9 +17,8 @@ class Group::WikisControllerTest < ActionController::TestCase
def test_show_wiki_settings_no_member
login_as @user2
assert_permission_denied do
get :index, params: { group_id: @group.to_param }, xhr: true
end
get :index, params: { group_id: @group.to_param }, xhr: true
assert_permission_denied
end
# TODO: maybe another test wich proves that
......@@ -28,9 +27,8 @@ class Group::WikisControllerTest < ActionController::TestCase
# group wiki
def test_show_wiki_settings_no_council_member
login_as @user3