Merge branch 'dblessing_known_sign_in_jwt' into 'master'

Add cookie to known sign in check

See merge request gitlab-org/gitlab!34102

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
-      before do
-        stub_user_ip('127.0.0.1')
-        stub_remote_ip('169.0.0.1')
-      end
+  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 'notifies the user when the cookie is expired' do
+      stub_cookie
 
-      it 'does not notify the user' do
-        expect_any_instance_of(NotificationService).not_to receive(:unknown_sign_in)
+      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
-        expect_any_instance_of(NotificationService).not_to receive(:unknown_sign_in)
+      post_action
+    end
 
-        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