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 45e0cc239f4dcb465e3c50755a1fc111996c70d6..13e9de890925c4704d93b71b4701fcf52f144c72 100644 (file)
--- 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 462b4ba3c48f8ae0ea5f37af172e28c79d68fd3d..58b02a48941feab848f7466558d2f9d4d31a38e3 100644 (file)
--- 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 23f35a40eeaf73662765d1d8cf137f2605f40f9c..5faa39165d089a249179b506ddaf9ee5dea6dedb 100644 (file)
--- 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 a7235241c14f95f46661073c7fe01e7165d6d246..25de71f20a1cdba2cc8b211ce316ad321f1604d7 100644 (file)
--- 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 4bbfac34f6dff3305b576e343a6e119e95784249..d0fb0c419e726cc85e0443376ccd925bb2bb9863 100644 (file)
--- 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 abbaf5e921e45aedf89e60d7057023b9a0374143..19fe05f3082574961423f1f3965abcd53e386a43 100644 (file)
--- 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 5147733e3edc4c50d46b9ac49992f050f976d100..4e7315edef3d89287cb0982853ff3f746d32749f 100644 (file)
--- 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