Stop using session flash to communicate with callbacks

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
 

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

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

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])

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

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]

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