Add cookie to known sign in check

Previously GitLab only validated a user's remote IP address when
checking if the sign in was known. This adds an encrypted cookie
with an expiry. An unknown sign in email will only be sent if
neither the remote IP nor the cookie can be validated.

app/controllers/concerns/known_sign_in.rb

@@ -2,19 +2,34 @@
 
 module KnownSignIn
   include Gitlab::Utils::StrongMemoize
+  include CookiesHelper
+
+  KNOWN_SIGN_IN_COOKIE = :known_sign_in
+  KNOWN_SIGN_IN_COOKIE_EXPIRY = 14.days
 
   private
 
   def verify_known_sign_in
     return unless current_user
 
-    notify_user unless known_remote_ip?
+    notify_user unless known_device? || known_remote_ip?
+
+    update_cookie
   end
 
   def known_remote_ip?
     known_ip_addresses.include?(request.remote_ip)
   end
 
+  def known_device?
+    cookies.encrypted[KNOWN_SIGN_IN_COOKIE] == current_user.id
+  end
+
+  def update_cookie
+    set_secure_cookie(KNOWN_SIGN_IN_COOKIE, current_user.id,
+                      type: COOKIE_TYPE_ENCRYPTED, httponly: true, expires: KNOWN_SIGN_IN_COOKIE_EXPIRY)
+  end
+
   def sessions
     strong_memoize(:session) do
       ActiveSession.list(current_user).reject(&:is_impersonated)

app/controllers/projects/application_controller.rb

@@ -82,7 +82,7 @@ class Projects::ApplicationController < ApplicationController
   end
 
   def apply_diff_view_cookie!
-    set_secure_cookie(:diff_view, params.delete(:view), permanent: true) if params[:view].present?
+    set_secure_cookie(:diff_view, params.delete(:view), type: COOKIE_TYPE_PERMANENT) if params[:view].present?
   end
 
   def require_pages_enabled!

app/helpers/cookies_helper.rb

 # frozen_string_literal: true
 
 module CookiesHelper
-  def set_secure_cookie(key, value, httponly: false, permanent: false)
-    cookie_jar = permanent ? cookies.permanent : cookies
+  COOKIE_TYPE_PERMANENT = :permanent
+  COOKIE_TYPE_ENCRYPTED = :encrypted
 
-    cookie_jar[key] = { value: value, secure: Gitlab.config.gitlab.https, httponly: httponly }
+  def set_secure_cookie(key, value, httponly: false, expires: nil, type: nil)
+    cookie_jar = case type
+                 when COOKIE_TYPE_PERMANENT
+                   cookies.permanent
+                 when COOKIE_TYPE_ENCRYPTED
+                   cookies.encrypted
+                 else
+                   cookies
+                 end
+
+    cookie_jar[key] = { value: value, secure: Gitlab.config.gitlab.https, httponly: httponly, expires: expires }
   end
 end

changelogs/unreleased/dblessing_known_sign_in_.yml

+---
+title: Use IP or cookie in known sign-in check
+merge_request: 34102
+author:
+type: changed

doc/user/profile/index.md

@@ -22,7 +22,7 @@ See the [authentication topic](../../topics/authentication/index.md) for more de
 
 ### Unknown sign-in
 
-GitLab will notify you if a sign-in occurs that is from an unknown IP address.
+GitLab will notify you if a sign-in occurs that is from an unknown IP address or device.
 See [Unknown Sign-In Notification](unknown_sign_in_notification.md) for more details.
 
 ## User profile

doc/user/profile/unknown_sign_in_notification.md

@@ -9,16 +9,19 @@ info: To determine the technical writer assigned to the Stage/Group associated w
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/27211) in GitLab 13.0.
 
-When a user successfully signs in from a previously unknown IP address,
+When a user successfully signs in from a previously unknown IP address or device,
 GitLab notifies the user by email. In this way, GitLab proactively alerts users of potentially
 malicious or unauthorized sign-ins.
 
-There are two methods used to identify a known sign-in:
+There are several methods used to identify a known sign-in. All methods must fail
+for a notification email to be sent.
 
 - Last sign-in IP: The current sign-in IP address is checked against the last sign-in
   IP address.
 - Current active sessions: If the user has an existing active session from the
   same IP address. See [Active Sessions](active_sessions.md).
+- Cookie: After successful sign in, an encrypted cookie is stored in the browser.
+  This cookie is set to expire 14 days after the last successful sign in.
 
 ## Example email
 

ee/spec/lib/gitlab/auth/group_saml/failure_handler_spec.rb

@@ -18,9 +18,13 @@ RSpec.describe Gitlab::Auth::GroupSaml::FailureHandler do
       'omniauth.error.strategy' => strategy,
       'devise.mapping' => Devise.mappings[:user],
       'warden' => warden,
-      'action_dispatch.key_generator' => ActiveSupport::KeyGenerator.new('b2efbaccbdb9548217eebc73a896db73'), # necessary for setting signed cookies in lib/gitlab/experimentation.rb
-      'action_dispatch.signed_cookie_salt' => 'a4fb52b0ccb302eaef92bda18fedf5c3', # necessary for setting signed cookies in lib/gitlab/experimentation.rb
-      'action_dispatch.cookies_rotations' => OpenStruct.new(signed: []) # necessary for setting signed cookies in lib/gitlab/experimentation.rb
+      # The following are necessary for setting signed/encrypted cookies such as in
+      # lib/gitlab/experimentation.rb or app/controllers/concerns/known_sign_in.rb
+      'action_dispatch.key_generator' => ActiveSupport::KeyGenerator.new('b2efbaccbdb9548217eebc73a896db73'),
+      'action_dispatch.signed_cookie_salt' => 'a4fb52b0ccb302eaef92bda18fedf5c3',
+      'action_dispatch.encrypted_signed_cookie_salt' => 'a4fb52b0ccb302eaef92bda18fedf5c3',
+      'action_dispatch.encrypted_cookie_salt' => 'a4fb52b0ccb302eaef92bda18fedf5c3',
+      'action_dispatch.cookies_rotations' => OpenStruct.new(signed: [], encrypted: [])
     }
     Rack::MockRequest.env_for(path, params)
   end

spec/controllers/concerns/sorting_preference_spec.rb

@@ -75,7 +75,7 @@ RSpec.describe SortingPreference do
       it 'sets the cookie with the right values and flags' do
         subject
 
-        expect(cookies['issue_sort']).to eq(value: 'popularity', secure: false, httponly: false)
+        expect(cookies['issue_sort']).to eq(expires: nil, value: 'popularity', secure: false, httponly: false)
       end
     end
 
@@ -86,7 +86,7 @@ RSpec.describe SortingPreference do
       it 'sets the cookie with the right values and flags' do
         subject
 
-        expect(cookies['issue_sort']).to eq(value: 'created_asc', secure: false, httponly: false)
+        expect(cookies['issue_sort']).to eq(expires: nil, value: 'created_asc', secure: false, httponly: false)
       end
     end
   end

spec/helpers/cookies_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe CookiesHelper do
+  describe '#set_secure_cookie' do
+    it 'creates an encrypted cookie with expected attributes' do
+      stub_config_setting(https: true)
+      expiration = 1.month.from_now
+      key = :secure_cookie
+      value = 'secure value'
+
+      expect_next_instance_of(ActionDispatch::Cookies::EncryptedKeyRotatingCookieJar) do |instance|
+        expect(instance).to receive(:[]=).with(key, httponly: true, secure: true, expires: expiration, value: value)
+      end
+
+      helper.set_secure_cookie(key, value, httponly: true, expires: expiration, type: CookiesHelper::COOKIE_TYPE_ENCRYPTED)
+    end
+
+    it 'creates a permanent cookie with expected attributes' do
+      key = :permanent_cookie
+      value = 'permanent value'
+
+      expect_next_instance_of(ActionDispatch::Cookies::PermanentCookieJar) do |instance|
+        expect(instance).to receive(:[]=).with(key, httponly: false, secure: false, expires: nil, value: value)
+      end
+
+      helper.set_secure_cookie(key, value, type: CookiesHelper::COOKIE_TYPE_PERMANENT)
+    end
+
+    it 'creates a regular cookie with expected attributes' do
+      key = :regular_cookie
+      value = 'regular value'
+
+      expect_next_instance_of(ActionDispatch::Cookies::CookieJar) do |instance|
+        expect(instance).to receive(:[]=).with(key, httponly: false, secure: false, expires: nil, value: value)
+      end
+
+      helper.set_secure_cookie(key, value)
+    end
+  end
+end

spec/support/shared_examples/controllers/known_sign_in_shared_examples.rb

@@ -9,13 +9,38 @@ RSpec.shared_examples 'known sign in' do
     user.update!(current_sign_in_ip: ip)
   end
 
-  context 'with a valid post' do
-    context 'when remote IP does not match user last sign in IP' do
+  def stub_cookie(value = user.id)
+    cookies.encrypted[KnownSignIn::KNOWN_SIGN_IN_COOKIE] = {
+      value: value, expires: KnownSignIn::KNOWN_SIGN_IN_COOKIE_EXPIRY
+    }
+  end
+
+  context 'when the remote IP and the last sign in IP match' do
+    before do
+      stub_user_ip('169.0.0.1')
+      stub_remote_ip('169.0.0.1')
+    end
+
+    it 'does not notify the user' do
+      expect_any_instance_of(NotificationService).not_to receive(:unknown_sign_in)
+
+      post_action
+    end
+
+    it 'sets/updates the encrypted cookie' do
+      post_action
+
+      expect(cookies.encrypted[KnownSignIn::KNOWN_SIGN_IN_COOKIE]).to eq(user.id)
+    end
+  end
+
+  context 'when the remote IP and the last sign in IP do not match' do
     before do
       stub_user_ip('127.0.0.1')
       stub_remote_ip('169.0.0.1')
     end
 
+    context 'when the cookie is not previously set' do
       it 'notifies the user' do
         expect_next_instance_of(NotificationService) do |instance|
           expect(instance).to receive(:unknown_sign_in)
@@ -23,37 +48,50 @@ RSpec.shared_examples 'known sign in' do
 
         post_action
       end
-    end
-
-    context 'when remote IP matches an active session' do
-      before do
-        existing_sessions = ActiveSession.session_ids_for_user(user.id)
-        existing_sessions.each { |sessions| ActiveSession.destroy(user, sessions) }
 
-        stub_user_ip('169.0.0.1')
-        stub_remote_ip('127.0.0.1')
+      it 'sets the encrypted cookie' do
+        post_action
 
-        ActiveSession.set(user, request)
+        expect(cookies.encrypted[KnownSignIn::KNOWN_SIGN_IN_COOKIE]).to eq(user.id)
+      end
     end
 
-      it 'does not notify the user' do
-        expect_any_instance_of(NotificationService).not_to receive(:unknown_sign_in)
+    it 'notifies the user when the cookie is expired' do
+      stub_cookie
+
+      Timecop.freeze((KnownSignIn::KNOWN_SIGN_IN_COOKIE_EXPIRY + 1.day).from_now) do
+        expect_next_instance_of(NotificationService) do |instance|
+          expect(instance).to receive(:unknown_sign_in)
+        end
 
         post_action
       end
     end
 
-    context 'when remote IP address matches last sign in IP' do
-      before do
-        stub_user_ip('127.0.0.1')
-        stub_remote_ip('127.0.0.1')
+    it 'notifies the user when the cookie is for another user' do
+      stub_cookie(create(:user).id)
+
+      expect_next_instance_of(NotificationService) do |instance|
+        expect(instance).to receive(:unknown_sign_in)
       end
 
-      it 'does not notify the user' do
+      post_action
+    end
+
+    it 'does not notify the user when remote IP matches an active session' do
+      ActiveSession.set(user, request)
+
       expect_any_instance_of(NotificationService).not_to receive(:unknown_sign_in)
 
       post_action
     end
+
+    it 'does not notify the user when the cookie is present and not expired' do
+      stub_cookie
+
+      expect_any_instance_of(NotificationService).not_to receive(:unknown_sign_in)
+
+      post_action
     end
   end
 end