From: Tom Hughes Date: Wed, 12 Feb 2025 00:03:32 +0000 (+0000) Subject: Stop using session flash to communicate with callbacks X-Git-Tag: live~148^2 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/43f40c5d036aaea80f5dae040c88f2a4cabdf8ff?ds=inline Stop using session flash to communicate with callbacks --- diff --git a/app/controllers/accounts/terms_controller.rb b/app/controllers/accounts/terms_controller.rb index 45e0cc239..13e9de890 100644 --- a/app/controllers/accounts/terms_controller.rb +++ b/app/controllers/accounts/terms_controller.rb @@ -4,8 +4,7 @@ module Accounts layout "site" - before_action :disable_terms_redirect - before_action :authorize_web + before_action -> { authorize_web(:skip_terms => true) } before_action :set_locale before_action :check_database_readable diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb index 462b4ba3c..58b02a489 100644 --- a/app/controllers/api/users_controller.rb +++ b/app/controllers/api/users_controller.rb @@ -1,8 +1,7 @@ module Api class UsersController < ApiController - before_action :disable_terms_redirect, :only => [:details] before_action :setup_user_auth, :only => [:show, :index] - before_action :authorize, :only => [:details] + before_action -> { authorize(:skip_terms => true) }, :only => [:details] authorize_resource @@ -46,14 +45,5 @@ module Api format.json { render :show } end end - - private - - def disable_terms_redirect - # this is necessary otherwise going to the user terms page, when - # having not agreed already would cause an infinite redirect loop. - # it's .now so that this doesn't propagate to other pages. - flash.now[:skip_terms] = true - end end end diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 23f35a40e..5faa39165 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -49,9 +49,9 @@ class ApiController < ApplicationController end end - def authorize(errormessage = "Couldn't authenticate you") + def authorize(errormessage: "Couldn't authenticate you", skip_terms: false) # make the current_user object from any auth sources we have - setup_user_auth + setup_user_auth(:skip_terms => skip_terms) # handle authenticate pass/fail unless current_user @@ -92,7 +92,7 @@ class ApiController < ApplicationController # sets up the current_user for use by other methods. this is mostly called # from the authorize method, but can be called elsewhere if authorisation # is optional. - def setup_user_auth + def setup_user_auth(skip_terms: false) logger.info " setup_user_auth" # try and setup using OAuth self.current_user = User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token&.accessible? @@ -113,7 +113,7 @@ class ApiController < ApplicationController # if the user hasn't seen the contributor terms then don't # allow editing - they have to go to the web site and see # (but can decline) the CTs to continue. - if !current_user.terms_seen && flash[:skip_terms].nil? + if !current_user.terms_seen && !skip_terms set_locale report_error t("application.setup_user_auth.need_to_see_terms"), :forbidden end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a7235241c..25de71f20 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -39,7 +39,7 @@ class ApplicationController < ActionController::Base private - def authorize_web + def authorize_web(skip_terms: false) if session[:user] self.current_user = User.find_by(:id => session[:user], :status => %w[active confirmed suspended]) @@ -55,7 +55,7 @@ class ApplicationController < ActionController::Base # don't allow access to any auth-requiring part of the site unless # the new CTs have been seen (and accept/decline chosen). - elsif !current_user.terms_seen && flash[:skip_terms].nil? + elsif !current_user.terms_seen && !skip_terms flash[:notice] = t "accounts.terms.show.you need to accept or decline" if params[:referer] redirect_to account_terms_path(:referer => params[:referer]) diff --git a/app/controllers/concerns/session_methods.rb b/app/controllers/concerns/session_methods.rb index 4bbfac34f..d0fb0c419 100644 --- a/app/controllers/concerns/session_methods.rb +++ b/app/controllers/concerns/session_methods.rb @@ -81,13 +81,4 @@ module SessionMethods session.delete(:remember_me) end - - ## - # - def disable_terms_redirect - # this is necessary otherwise going to the user terms page, when - # having not agreed already would cause an infinite redirect loop. - # it's .now so that this doesn't propagate to other pages. - flash.now[:skip_terms] = true - end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index abbaf5e92..19fe05f30 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -3,8 +3,8 @@ class SessionsController < ApplicationController layout "site" - before_action :disable_terms_redirect, :only => [:destroy] - before_action :authorize_web + before_action :authorize_web, :except => [:destroy] + before_action -> { authorize_web(:skip_terms => true) }, :only => [:destroy] before_action :set_locale before_action :check_database_readable before_action :require_cookies, :only => [:new] diff --git a/test/integration/user_blocks_test.rb b/test/integration/user_blocks_test.rb index 5147733e3..4e7315ede 100644 --- a/test/integration/user_blocks_test.rb +++ b/test/integration/user_blocks_test.rb @@ -38,6 +38,8 @@ class UserBlocksTest < ActionDispatch::IntegrationTest # revoke the ban get "/login" + assert_response :redirect + follow_redirect! assert_response :success post "/login", :params => { "username" => moderator.email, "password" => "test", :referer => "/user_blocks/#{block.id}/edit" } assert_response :redirect