Merge branch 'sh-limit-unauth-project-session-ttl' into 'master'

Shorten session TTL of anonymous blob access

See merge request gitlab-org/gitlab!70444

app/controllers/application_controller.rb

@@ -42,6 +42,7 @@ class ApplicationController < ActionController::Base
   # Make sure the `auth_user` is memoized so it can be logged, we do this after
   # all other before filters that could have set the user.
   before_action :auth_user
+  before_action :limit_session_time, if: -> { !current_user }
 
   prepend_around_action :set_current_context
 
@@ -51,7 +52,7 @@ class ApplicationController < ActionController::Base
   around_action :set_current_admin
 
   after_action :set_page_title_header, if: :json_request?
-  after_action :limit_session_time, if: -> { !current_user }
+  after_action :ensure_authenticated_session_time, if: -> { current_user }
 
   protect_from_forgery with: :exception, prepend: true
 

app/helpers/sessions_helper.rb

@@ -22,11 +22,21 @@ module SessionsHelper
   # creates a new session after login, so the short TTL doesn't even need to
   # be extended.
   def limit_session_time
+    set_session_time(Settings.gitlab['unauthenticated_session_expire_delay'])
+  end
+
+  def ensure_authenticated_session_time
+    set_session_time(nil)
+  end
+
+  def set_session_time(expiry_s)
     # 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']
+    # This works because Rack uses these options every time a request is handled, and redis-store
+    # uses the Rack setting first:
+    # 1. https://github.com/rack/rack/blob/fdcd03a3c5a1c51d1f96fc97f9dfa1a9deac0c77/lib/rack/session/abstract/id.rb#L342
+    # 2. https://github.com/redis-store/redis-store/blob/3acfa95f4eb6260c714fdb00a3d84be8eedc13b2/lib/redis/store/ttl.rb#L32
+    request.env['rack.session.options'][:expire_after] = expiry_s
   end
 end

lib/gitlab/devise_failure.rb

@@ -2,18 +2,10 @@
 
 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?
       request_format && super
     end
-
-    def respond
-      limit_session_time
-
-      super
-    end
   end
 end

spec/features/users/anonymous_sessions_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe 'Session TTLs', :clean_gitlab_redis_shared_state do
+  include SessionHelpers
+
   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
@@ -12,7 +14,7 @@ RSpec.describe 'Session TTLs', :clean_gitlab_redis_shared_state do
 
     expect(page).to have_content('Invalid login or password')
 
-    expect_single_session_with_expiration(Settings.gitlab['unauthenticated_session_expire_delay'])
+    expect_single_session_with_short_ttl
   end
 
   it 'increases the TTL when the login succeeds' do
@@ -21,21 +23,17 @@ RSpec.describe 'Session TTLs', :clean_gitlab_redis_shared_state do
 
     expect(page).to have_content(user.name)
 
-    expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60)
+    expect_single_session_with_authenticated_ttl
   end
 
-  def expect_single_session_with_expiration(expiration)
-    session_keys = get_session_keys
+  context 'with an unauthorized project' do
+    let_it_be(:project) { create(:project, :repository) }
 
-    expect(session_keys.size).to eq(1)
-    expect(get_ttl(session_keys.first)).to eq expiration
-  end
+    it 'creates a session with a short TTL' do
+      visit project_raw_path(project, 'master/README.md')
 
-  def get_session_keys
-    Gitlab::Redis::SharedState.with { |redis| redis.scan_each(match: 'session:gitlab:*').to_a }
+      expect_single_session_with_short_ttl
+      expect(page).to have_current_path(new_user_session_path)
     end
-
-  def get_ttl(key)
-    Gitlab::Redis::SharedState.with { |redis| redis.ttl(key) }
   end
 end

spec/features/users/login_spec.rb

@@ -2,9 +2,10 @@
 
 require 'spec_helper'
 
-RSpec.describe 'Login' do
+RSpec.describe 'Login', :clean_gitlab_redis_shared_state do
   include TermsHelper
   include UserLoginHelper
+  include SessionHelpers
 
   before do
     stub_authentication_activity_metrics(debug: true)
@@ -59,6 +60,7 @@ RSpec.describe 'Login' do
       fill_in 'user_password', with: 'password'
       click_button 'Sign in'
 
+      expect_single_session_with_authenticated_ttl
       expect(current_path).to eq root_path
     end
 
@@ -192,6 +194,7 @@ RSpec.describe 'Login' do
         enter_code(user.current_otp)
 
         expect(page).not_to have_content(I18n.t('devise.failure.already_authenticated'))
+        expect_single_session_with_authenticated_ttl
       end
 
       it 'does not allow sign-in if the user password is updated before entering a one-time code' do
@@ -210,6 +213,7 @@ RSpec.describe 'Login' do
 
           enter_code(user.current_otp)
 
+          expect_single_session_with_authenticated_ttl
           expect(current_path).to eq root_path
         end
 
@@ -237,6 +241,8 @@ RSpec.describe 'Login' do
           expect(page).to have_content('Invalid two-factor code')
 
           enter_code(user.current_otp)
+
+          expect_single_session_with_authenticated_ttl
           expect(current_path).to eq root_path
         end
 
@@ -353,6 +359,7 @@ RSpec.describe 'Login' do
 
           sign_in_using_saml!
 
+          expect_single_session_with_authenticated_ttl
           expect(page).not_to have_content('Two-Factor Authentication')
           expect(current_path).to eq root_path
         end
@@ -371,6 +378,7 @@ RSpec.describe 'Login' do
 
           enter_code(user.current_otp)
 
+          expect_single_session_with_authenticated_ttl
           expect(current_path).to eq root_path
         end
       end
@@ -391,6 +399,7 @@ RSpec.describe 'Login' do
 
         gitlab_sign_in(user)
 
+        expect_single_session_with_authenticated_ttl
         expect(current_path).to eq root_path
         expect(page).not_to have_content(I18n.t('devise.failure.already_authenticated'))
       end
@@ -402,6 +411,7 @@ RSpec.describe 'Login' do
         gitlab_sign_in(user)
         visit new_user_session_path
 
+        expect_single_session_with_authenticated_ttl
         expect(page).not_to have_content(I18n.t('devise.failure.already_authenticated'))
       end
 
@@ -443,6 +453,7 @@ RSpec.describe 'Login' do
 
         gitlab_sign_in(user)
 
+        expect_single_session_with_short_ttl
         expect(page).to have_content('Invalid login or password.')
       end
     end

spec/lib/gitlab/devise_failure_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.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

spec/support/helpers/session_helpers.rb

+# frozen_string_literal: true
+
+module SessionHelpers
+  def expect_single_session_with_authenticated_ttl
+    expect_single_session_with_expiration(Settings.gitlab['session_expire_delay'] * 60)
+  end
+
+  def expect_single_session_with_short_ttl
+    expect_single_session_with_expiration(Settings.gitlab['unauthenticated_session_expire_delay'])
+  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