Set shorter TTL for all unauthenticated requests

GitLab 11.2 limited the time-to-live (TTL) of unauthenticated sessions
via https://gitlab.com/gitlab-org/gitlab/merge_requests/6586 using
`before_action` in `ApplicationController`. However, this broke OAuth2
logins, which set the `current_user` **after** a login is successful, so
we moved it to an `after_action` in
https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/21144. However,
`after_action` isn't called if a exception is raised in the request
cycle. Thus, in some situations, TTLs weren't always being set to a
short value.

This commit adds the TTL limiting to the Devise Failure App, which is
run anytime the user is redirected to the sign-in page.

Relates to
https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/8247

app/controllers/application_controller.rb

@@ -12,6 +12,7 @@ class ApplicationController < ActionController::Base
   include EnforcesTwoFactorAuthentication
   include WithPerformanceBar
   include SessionlessAuthentication
+  include SessionsHelper
   include ConfirmEmailWarning
   include Gitlab::Tracking::ControllerConcern
   include Gitlab::Experimentation::ControllerConcern
@@ -35,7 +36,7 @@ class ApplicationController < ActionController::Base
   around_action :set_session_storage
 
   after_action :set_page_title_header, if: :json_request?
-  after_action :limit_unauthenticated_session_times
+  after_action :limit_session_time, if: -> { !current_user }
 
   protect_from_forgery with: :exception, prepend: true
 
@@ -101,24 +102,6 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  # By default, all sessions are given the same expiration time configured in
-  # the session store (e.g. 1 week). However, unauthenticated users can
-  # generate a lot of sessions, primarily for CSRF verification. It makes
-  # sense to reduce the TTL for unauthenticated to something much lower than
-  # the default (e.g. 1 hour) to limit Redis memory. In addition, Rails
-  # creates a new session after login, so the short TTL doesn't even need to
-  # be extended.
-  def limit_unauthenticated_session_times
-    return if current_user
-
-    # Rack sets this header, but not all tests may have it: https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L251-L259
-    return unless request.env['rack.session.options']
-
-    # This works because Rack uses these options every time a request is handled:
-    # https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L342
-    request.env['rack.session.options'][:expire_after] = Settings.gitlab['unauthenticated_session_expire_delay']
-  end
-
   def render(*args)
     super.tap do
       # Set a header for custom error pages to prevent them from being intercepted by gitlab-workhorse

app/helpers/sessions_helper.rb

@@ -4,4 +4,20 @@ module SessionsHelper
   def unconfirmed_email?
     flash[:alert] == t(:unconfirmed, scope: [:devise, :failure])
   end
+
+  # By default, all sessions are given the same expiration time configured in
+  # the session store (e.g. 1 week). However, unauthenticated users can
+  # generate a lot of sessions, primarily for CSRF verification. It makes
+  # sense to reduce the TTL for unauthenticated to something much lower than
+  # the default (e.g. 1 hour) to limit Redis memory. In addition, Rails
+  # creates a new session after login, so the short TTL doesn't even need to
+  # be extended.
+  def limit_session_time
+    # Rack sets this header, but not all tests may have it: https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L251-L259
+    return unless request.env['rack.session.options']
+
+    # This works because Rack uses these options every time a request is handled:
+    # https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L342
+    request.env['rack.session.options'][:expire_after] = Settings.gitlab['unauthenticated_session_expire_delay']
+  end
 end

changelogs/unreleased/sh-ensure-short-ttl-sessions.yml

+---
+title: Set shorter TTL for all unauthenticated requests
+merge_request: 19064
+author:
+type: fixed

lib/gitlab/devise_failure.rb

@@ -2,6 +2,8 @@
 
 module Gitlab
   class DeviseFailure < Devise::FailureApp
+    include ::SessionsHelper
+
     # If the request format is not known, send a redirect instead of a 401
     # response, since this is the outcome we're most likely to want
     def http_auth?
@@ -9,5 +11,11 @@ module Gitlab
 
       request_format && super
     end
+
+    def respond
+      limit_session_time
+
+      super
+    end
   end
 end

spec/lib/gitlab/devise_failure_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::DeviseFailure do
+  let(:env) do
+    {
+      'REQUEST_URI' => 'http://test.host/',
+      'HTTP_HOST' => 'test.host',
+      'REQUEST_METHOD' => 'GET',
+      'warden.options' => { scope: :user },
+      'rack.session' => {},
+      'rack.session.options' => {},
+      'rack.input' => "",
+      'warden' => OpenStruct.new(message: nil)
+    }
+  end
+
+  let(:response) { described_class.call(env).to_a }
+  let(:request) { ActionDispatch::Request.new(env) }
+
+  context 'When redirecting' do
+    it 'sets the expire_after key' do
+      response
+
+      expect(env['rack.session.options']).to have_key(:expire_after)
+    end
+
+    it 'returns to the default redirect location' do
+      expect(response.first).to eq(302)
+      expect(request.flash[:alert]).to eq('You need to sign in or sign up before continuing.')
+      expect(response.second['Location']).to eq('http://test.host/users/sign_in')
+    end
+  end
+end