Merge branch 'sh-ensure-short-ttl-sessions' into 'master'

Set shorter TTL for all unauthenticated requests

See merge request gitlab-org/gitlab!19064

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/features/users/anonymous_sessions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Session TTLs', :clean_gitlab_redis_shared_state do
+  it 'creates a session with a short TTL when login fails' do
+    visit new_user_session_path
+    # The session key only gets created after a post
+    fill_in 'user_login', with: 'non-existant@gitlab.org'
+    fill_in 'user_password', with: '12345678'
+    click_button 'Sign in'
+
+    expect(page).to have_content('Invalid Login or password')
+
+    expect_single_session_with_expiration(Settings.gitlab['unauthenticated_session_expire_delay'])
+  end
+
+  it 'increases the TTL when the login succeeds' do
+    user = create(:user)
+    gitlab_sign_in(user)
+
+    expect(page).to have_content(user.name)
+
+    expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60)
+  end
+
+  def expect_single_session_with_expiration(expiration)
+    session_keys = get_session_keys
+
+    expect(session_keys.size).to eq(1)
+    expect(get_ttl(session_keys.first)).to eq expiration
+  end
+
+  def get_session_keys
+    Gitlab::Redis::SharedState.with { |redis| redis.scan_each(match: 'session:gitlab:*').to_a }
+  end
+
+  def get_ttl(key)
+    Gitlab::Redis::SharedState.with { |redis| redis.ttl(key) }
+  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