From 7f30e1f73f9835f40e8b2f4b6e01c3e6a8b346dc Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Fri, 1 Jul 2022 11:45:16 +0200 Subject: [PATCH 1/6] Require email verification When trying to log in from an unknown IP address or after 3 or more failed login attempts, require email verification. Changelog: added --- .../concerns/verifies_with_email.rb | 157 +++++++++++ app/controllers/sessions_controller.rb | 1 + app/helpers/emails_helper.rb | 8 + app/helpers/sessions_helper.rb | 7 + app/mailers/emails/abuse.rb | 15 ++ app/mailers/notify.rb | 1 + app/mailers/previews/notify_preview.rb | 4 + app/models/authentication_event.rb | 5 + .../concerns/require_email_verification.rb | 53 ++++ app/models/user.rb | 3 +- .../devise/sessions/email_verification.haml | 18 ++ .../sessions/successful_verification.haml | 11 + .../verification_instructions_email.html.haml | 12 + .../verification_instructions_email.text.erb | 8 + .../ops/require_email_verification.yml | 8 + config/routes/user.rb | 2 + ..._success_index_to_authentication_events.rb | 15 ++ db/schema_migrations/20220601152916 | 1 + db/structure.sql | 2 + .../javascripts/pages/sessions/new/index.js | 2 +- locale/gitlab.pot | 48 ++++ .../users/email_verification_on_login_spec.rb | 247 ++++++++++++++++++ spec/helpers/sessions_helper_spec.rb | 22 ++ spec/models/authentication_event_spec.rb | 27 ++ spec/spec_helper.rb | 4 + 25 files changed, 679 insertions(+), 2 deletions(-) create mode 100644 app/controllers/concerns/verifies_with_email.rb create mode 100644 app/mailers/emails/abuse.rb create mode 100644 app/models/concerns/require_email_verification.rb create mode 100644 app/views/devise/sessions/email_verification.haml create mode 100644 app/views/devise/sessions/successful_verification.haml create mode 100644 app/views/notify/verification_instructions_email.html.haml create mode 100644 app/views/notify/verification_instructions_email.text.erb create mode 100644 config/feature_flags/ops/require_email_verification.yml create mode 100644 db/migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb create mode 100644 db/schema_migrations/20220601152916 create mode 100644 spec/features/users/email_verification_on_login_spec.rb diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb new file mode 100644 index 00000000000000..0b797f4c77aed5 --- /dev/null +++ b/app/controllers/concerns/verifies_with_email.rb @@ -0,0 +1,157 @@ +# frozen_string_literal: true + +# == VerifiesWithEmail +# +# Controller concern to handle verification by email +# +module VerifiesWithEmail + extend ActiveSupport::Concern + + TOKEN_LENGTH = 6 + TOKEN_VALID_FOR_MINUTES = 60 + + included do + prepend_before_action :verify_with_email, only: :create, unless: -> { two_factor_enabled? } + end + + def verify_with_email + return unless user = find_user || find_verification_user + + if session[:verification_user_id] && token = verification_params[:verification_token].presence + # The verification token is submitted, verify it + verify_token(user, token) + elsif require_email_verification_enabled? && user.valid_password?(user_params[:password]) + # The user has logged in successfully. + if user.unlock_token + # Prompt for the token if it already has been set + prompt_for_email_verification(user) + elsif user.access_locked? || !AuthenticationEvent.initial_login_or_known_ip_address?(user, request.ip) + # require email verification if: + # - their account has been locked because of too many failed login attempts, or + # - they have logged in before, but never from the current ip address + send_verification_instructions(user) + prompt_for_email_verification(user) + end + end + end + + def resend_verification_code + return unless user = find_verification_user + + send_verification_instructions(user) + prompt_for_email_verification(user) + end + + def successful_verification + session.delete(:verification_user_id) + @redirect_url = after_sign_in_path_for(current_user) # rubocop:disable Gitlab/ModuleWithInstanceVariables + + render layout: 'minimal' + end + + private + + def find_verification_user + return unless session[:verification_user_id] + + User.find(session[:verification_user_id]) + end + + # After successful verification and calling sign_in, devise redirects the + # user to this path. Override it to show the successful verified page. + def after_sign_in_path_for(resource) + if action_name == 'create' && session[:verification_user_id] + return users_successful_verification_path + end + + super + end + + def send_verification_instructions(user) + raw_token, encrypted_token = generate_token + user.unlock_token = encrypted_token + user.lock_access!({ send_instructions: false }) + send_verification_instructions_email(user, raw_token) + log_verification(user, :instructions_sent) + end + + def send_verification_instructions_email(user, token) + return unless user.can?(:receive_notifications) + + Notify.verification_instructions_email( + user.id, + token: token, + expires_in: TOKEN_VALID_FOR_MINUTES).deliver_later + end + + def verify_token(user, token) + if invalid_token?(user, token) + handle_verification_failure(user, :invalid) + elsif expired_token?(user) + handle_verification_failure(user, :expired) + else + handle_verification_success(user) + end + end + + def generate_token + raw_token = SecureRandom.random_number(10**TOKEN_LENGTH).to_s.rjust(TOKEN_LENGTH, '0') + encrypted_token = digest_token(raw_token) + [raw_token, encrypted_token] + end + + def digest_token(token) + Devise.token_generator.digest(User, :unlock_token, token) + end + + def expired_token?(user) + user.locked_at < (Time.current - TOKEN_VALID_FOR_MINUTES.minutes) + end + + def invalid_token?(user, token) + user.unlock_token != digest_token(token) + end + + def handle_verification_failure(user, reason) + message = case reason + when :expired + _('The code has expired. Resend a new code and try again.') + when :invalid + _('The code is incorrect. Enter it again, or resend a new code.') + end + + user.errors.add(:base, message) + log_verification(user, :failed_attempt, reason) + + prompt_for_email_verification(user) + end + + def handle_verification_success(user) + user.unlock_access! + log_verification(user, :successful) + + sign_in(user) + end + + def prompt_for_email_verification(user) + session[:verification_user_id] = user.id + self.resource = user + + render 'devise/sessions/email_verification' + end + + def verification_params + params.require(:user).permit(:verification_token) + end + + def log_verification(user, event, reason = nil) + message = "Email Verification #{event.to_s.titlecase}: username=#{user.username} ip=#{request.ip}" + message = "#{message} reason=#{reason}" if reason + + Gitlab::AppLogger.info(message) + end + + def require_email_verification_enabled? + Feature.enabled?(:require_email_verification, type: :ops) + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 9000e9c39dec2c..fc51ee47bd2a3e 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -11,6 +11,7 @@ class SessionsController < Devise::SessionsController include Gitlab::Utils::StrongMemoize include OneTrustCSP include BizibleCSP + include VerifiesWithEmail skip_before_action :check_two_factor_requirement, only: [:destroy] skip_before_action :check_password_expiration, only: [:destroy] diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb index c23d905a0089d0..dc4e2e9709ae1c 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -276,6 +276,14 @@ def instance_access_request_link(user, format: nil) end end + def link_start(url) + ''.html_safe % { url: url } + end + + def link_end + ''.html_safe + end + def contact_your_administrator_text _('Please contact your administrator with any questions.') end diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index f0389000eb392d..8486be7e9a3230 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -39,4 +39,11 @@ def set_session_time(expiry_s) # 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 + + def obfuscated_email(email) + email.sub(/^(..?)(.*)(@.?)(.*)(\..*)$/) do + match = Regexp.last_match + match[1] + '*' * match[2].length + match[3] + '*' * match[4].length + match[5] + end + end end diff --git a/app/mailers/emails/abuse.rb b/app/mailers/emails/abuse.rb new file mode 100644 index 00000000000000..be2ffdf68c1aef --- /dev/null +++ b/app/mailers/emails/abuse.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Emails + module Abuse + def verification_instructions_email(user_id, token:, expires_in:) + @token = token + @expires_in_minutes = expires_in + @password_link = edit_profile_password_url + @two_fa_link = help_page_url('user/profile/account/two_factor_authentication') + + user = User.find(user_id) + email_with_layout(to: user.email, subject: _('Verify your identity')) + end + end +end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index b70ce1d365502f..95ab6c02b84d64 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -23,6 +23,7 @@ class Notify < ApplicationMailer include Emails::ServiceDesk include Emails::InProductMarketing include Emails::AdminNotification + include Emails::Abuse helper TimeboxesHelper helper MergeRequestsHelper diff --git a/app/mailers/previews/notify_preview.rb b/app/mailers/previews/notify_preview.rb index 074aec54b1057c..be8d96012cc54c 100644 --- a/app/mailers/previews/notify_preview.rb +++ b/app/mailers/previews/notify_preview.rb @@ -213,6 +213,10 @@ def user_auto_banned_namespace_email ::Notify.user_auto_banned_email(user.id, user.id, max_project_downloads: 5, within_seconds: 600, group: group).message end + def verification_instructions_email + Notify.verification_instructions_email(user.id, token: '123456', expires_in: 60).message + end + private def project diff --git a/app/models/authentication_event.rb b/app/models/authentication_event.rb index 1e822629ba150a..0ed197f32dff3c 100644 --- a/app/models/authentication_event.rb +++ b/app/models/authentication_event.rb @@ -25,4 +25,9 @@ class AuthenticationEvent < ApplicationRecord def self.providers STATIC_PROVIDERS | Devise.omniauth_providers.map(&:to_s) end + + def self.initial_login_or_known_ip_address?(user, ip_address) + !where(user_id: user).exists? || + where(user_id: user, ip_address: ip_address).success.exists? + end end diff --git a/app/models/concerns/require_email_verification.rb b/app/models/concerns/require_email_verification.rb new file mode 100644 index 00000000000000..0cf93dd781ae5e --- /dev/null +++ b/app/models/concerns/require_email_verification.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +# == Require Email Verification module +# +# Contains functionality to handle email verification +# +# +module RequireEmailVerification + extend ActiveSupport::Concern + + # This value is twice the amount we want it to be, because due to a bug in the devise-two-factor + # gem every failed login attempt increments the value of failed_attempts by 2 instead of 1. + # See: https://github.com/tinfoil/devise-two-factor/issues/127 + MAXIMUM_ATTEMPTS = 3 * 2 + UNLOCK_IN = 24.hours + + included do + # Virtual attribute for the email verification token form + attr_accessor :verification_token + end + + # When overriden, do not send Devise unlock instructions when locking access. + def lock_access!(opts = {}) + return super unless override_devise_lockable? + + super({ send_instructions: false }) + end + + protected + + # We cannot override the class methods `maximum_attempts` and `unlock_in`, because we want to + # check for 2FA being enabled on the instance. So instead override the Devise Lockable methods + # where those values are used. + def attempts_exceeded? + return super unless override_devise_lockable? + + failed_attempts >= MAXIMUM_ATTEMPTS + end + + def lock_expired? + return super unless override_devise_lockable? + + locked_at && locked_at < UNLOCK_IN.ago + end + + private + + def override_devise_lockable? + strong_memoize(:override_devise_lockable) do + Feature.enabled?(:require_email_verification, type: :ops) && !two_factor_enabled? + end + end +end diff --git a/app/models/user.rb b/app/models/user.rb index b35036d0af7cc9..20ff796aba07bf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -88,6 +88,7 @@ class User < ApplicationRecord # and should be added after Devise modules are initialized. include AsyncDeviseEmail include ForcedEmailConfirmation + include RequireEmailVerification MINIMUM_INACTIVE_DAYS = 90 MINIMUM_DAYS_CREATED = 7 @@ -1899,7 +1900,7 @@ def read_only_attribute?(attribute) end # override, from Devise - def lock_access! + def lock_access!(opts = {}) Gitlab::AppLogger.info("Account Locked: username=#{username}") super end diff --git a/app/views/devise/sessions/email_verification.haml b/app/views/devise/sessions/email_verification.haml new file mode 100644 index 00000000000000..03c1bee01617ff --- /dev/null +++ b/app/views/devise/sessions/email_verification.haml @@ -0,0 +1,18 @@ +%div + = render 'devise/shared/tab_single', tab_title: _('Help us protect your account') + .login-box.gl-p-5 + .login-body + = form_for(resource, as: resource_name, url: session_path(resource_name), method: :post, html: { class: 'gl-show-field-errors' }) do |f| + %p + = _("For added security, you'll need to verify your identity. We've sent a verification code to %{email}").html_safe % { email: "#{sanitize(obfuscated_email(resource.email))}".html_safe } + %div + = f.label :verification_token, _('Verification code') + = f.text_field :verification_token, class: 'form-control gl-form-input', required: true, autofocus: true, autocomplete: 'off', title: _('Please enter a valid code'), inputmode: 'numeric', maxlength: 6, pattern: '[0-9]{6}' + %p.gl-field-error.gl-mt-2 + = resource.errors.full_messages.to_sentence + .gl-mt-5 + = f.submit _('Verify code'), class: 'gl-button btn btn-confirm' + = link_to _('Resend code'), users_resend_verification_code_path, method: :post, class: 'form-control gl-button btn-link gl-mt-3 gl-mb-0' + %p.gl-p-5.gl-text-secondary + = _("If you've lost access to the email associated to this account or having trouble with the code,") + = link_to _('here are some other steps you can take.'), 'https://about.gitlab.com/support/', target: '_blank', rel: 'noopener noreferrer' diff --git a/app/views/devise/sessions/successful_verification.haml b/app/views/devise/sessions/successful_verification.haml new file mode 100644 index 00000000000000..b4a57a513442d1 --- /dev/null +++ b/app/views/devise/sessions/successful_verification.haml @@ -0,0 +1,11 @@ += content_for :meta_tags do + %meta{ 'http-equiv': 'refresh', content: "3; url=#{@redirect_url}" } +.gl-text-center.gl-max-w-62.gl-mx-auto + .svg-content.svg-80 + = image_tag 'illustrations/success-sm.svg' + %h2 + = _('Verification successful') + %p.gl-pt-2 + - redirect_url_start = ''.html_safe % { url: @redirect_url } + - redirect_url_end = ''.html_safe + = html_escape(_("Your account has been successfully verified. You'll be redirected to your account in just a moment or %{redirect_url_start}click here%{redirect_url_end} to refresh.")) % { redirect_url_start: redirect_url_start, redirect_url_end: redirect_url_end } diff --git a/app/views/notify/verification_instructions_email.html.haml b/app/views/notify/verification_instructions_email.html.haml new file mode 100644 index 00000000000000..3a664b3e759172 --- /dev/null +++ b/app/views/notify/verification_instructions_email.html.haml @@ -0,0 +1,12 @@ +%div{ style: 'text-align:center;color:#1F1F1F;line-height:1.25em;max-width:400px;margin:0 auto;' } + %h3 + = _('Help us protect your account') + %p{ style: 'font-size:0.9em' } + = _('Before you sign in, we need to verify your identity. Enter the following code on the sign-in page.') + %div{ style: 'margin:26px 0;width:207px;height:53px;background-color:#F0F0F0;line-height:53px;font-weight:700;font-size:1.5em;color:#303030;' } + = @token + %p{ style: 'font-size:0.75em' } + = _('If you have not recently tried to sign into GitLab, we recommend %{password_link_start}changing your password%{link_end} and %{two_fa_link_start}setting up Two-Factor Authentication%{link_end} to keep your account safe. Your verification code expires after %{expires_in_minutes} minutes.').html_safe % { link_end: link_end, + password_link_start: link_start(@password_link), + two_fa_link_start: link_start(@two_fa_link), + expires_in_minutes: @expires_in_minutes } diff --git a/app/views/notify/verification_instructions_email.text.erb b/app/views/notify/verification_instructions_email.text.erb new file mode 100644 index 00000000000000..5118ed549c9f57 --- /dev/null +++ b/app/views/notify/verification_instructions_email.text.erb @@ -0,0 +1,8 @@ +<%= _('Help us protect your account') %> + +<%= _('Before you sign in, we need to verify your identity. Enter the following code on the sign-in page.') %> + +<%= @token %> + +<%= _('If you have not recently tried to sign into GitLab, we recommend changing your password (%{password_link}) and setting up Two-Factor Authentication (%{two_fa_link}) to keep your account safe.') % { password_link: @password_link, two_fa_link: @two_fa_link } %> +<%= _('Your verification code expires after %{expires_in_minutes} minutes.') % { expires_in_minutes: @expires_in_minutes } %> diff --git a/config/feature_flags/ops/require_email_verification.yml b/config/feature_flags/ops/require_email_verification.yml new file mode 100644 index 00000000000000..e895e2812b8d6d --- /dev/null +++ b/config/feature_flags/ops/require_email_verification.yml @@ -0,0 +1,8 @@ +--- +name: require_email_verification +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86352 +rollout_issue_url: +milestone: '15.1' +type: ops +group: group::anti-abuse +default_enabled: false diff --git a/config/routes/user.rb b/config/routes/user.rb index ccacf817cc5f53..a5fc53b61d2c56 100644 --- a/config/routes/user.rb +++ b/config/routes/user.rb @@ -53,6 +53,8 @@ def override_omniauth(provider, controller, path_prefix = '/users/auth') devise_scope :user do get '/users/almost_there' => 'confirmations#almost_there' + post '/users/resend_verification_code', to: 'sessions#resend_verification_code' + get '/users/successful_verification', to: 'sessions#successful_verification' end scope '-/users', module: :users do diff --git a/db/migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb b/db/migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb new file mode 100644 index 00000000000000..3e3d5659eabe9d --- /dev/null +++ b/db/migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class AddUserIdAndIpAddressSuccessIndexToAuthenticationEvents < Gitlab::Database::Migration[2.0] + INDEX_NAME = 'index_authentication_events_on_user_id_and_ip_address_success' + + disable_ddl_transaction! + + def up + add_concurrent_index :authentication_events, [:user_id, :ip_address], where: 'result = 1', name: INDEX_NAME + end + + def down + remove_concurrent_index_by_name :authentication_events, INDEX_NAME + end +end diff --git a/db/schema_migrations/20220601152916 b/db/schema_migrations/20220601152916 new file mode 100644 index 00000000000000..4858976aa3a660 --- /dev/null +++ b/db/schema_migrations/20220601152916 @@ -0,0 +1 @@ +f460407888e289580dec15ea27e19fa5cc2d2116a831105b71b980c617971743 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index e511bd88618248..e53ae0364f4cae 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -27284,6 +27284,8 @@ CREATE INDEX index_authentication_events_on_provider_user_id_created_at ON authe CREATE INDEX index_authentication_events_on_user_id ON authentication_events USING btree (user_id); +CREATE INDEX index_authentication_events_on_user_id_and_ip_address_success ON authentication_events USING btree (user_id, ip_address) WHERE (result = 1); + CREATE INDEX index_award_emoji_on_awardable_type_and_awardable_id ON award_emoji USING btree (awardable_type, awardable_id); CREATE UNIQUE INDEX index_aws_roles_on_role_external_id ON aws_roles USING btree (role_external_id); diff --git a/ee/app/assets/javascripts/pages/sessions/new/index.js b/ee/app/assets/javascripts/pages/sessions/new/index.js index c3dfc113cc6eaf..744341910141ec 100644 --- a/ee/app/assets/javascripts/pages/sessions/new/index.js +++ b/ee/app/assets/javascripts/pages/sessions/new/index.js @@ -1,6 +1,6 @@ import '~/pages/sessions/new/index'; -if (gon.features.arkoseLabsLoginChallenge) { +if (gon.features?.arkoseLabsLoginChallenge) { import('ee/arkose_labs') .then(({ setupArkoseLabs }) => { setupArkoseLabs(); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index da702e4b5e900f..e02834aa95b41a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5829,6 +5829,9 @@ msgstr "" msgid "Before this can be merged, a Jira issue must be linked in the title or description" msgstr "" +msgid "Before you sign in, we need to verify your identity. Enter the following code on the sign-in page." +msgstr "" + msgid "Begin with the selected commit" msgstr "" @@ -16447,6 +16450,9 @@ msgstr "" msgid "For a faster browsing experience, some files are collapsed by default." msgstr "" +msgid "For added security, you'll need to verify your identity. We've sent a verification code to %{email}" +msgstr "" + msgid "For additional information, review your %{link_to} or contact your group owner." msgstr "" @@ -19002,6 +19008,9 @@ msgstr "" msgid "Help translate GitLab into your language" msgstr "" +msgid "Help us protect your account" +msgstr "" + msgid "Helps prevent bots from brute-force attacks." msgstr "" @@ -19411,6 +19420,12 @@ msgstr "" msgid "If you get a lot of false alarms from repository checks, you can clear all repository check information from the database." msgstr "" +msgid "If you have not recently tried to sign into GitLab, we recommend %{password_link_start}changing your password%{link_end} and %{two_fa_link_start}setting up Two-Factor Authentication%{link_end} to keep your account safe. Your verification code expires after %{expires_in_minutes} minutes." +msgstr "" + +msgid "If you have not recently tried to sign into GitLab, we recommend changing your password (%{password_link}) and setting up Two-Factor Authentication (%{two_fa_link}) to keep your account safe." +msgstr "" + msgid "If you lose your recovery codes you can generate new ones, invalidating all previous codes." msgstr "" @@ -19429,6 +19444,9 @@ msgstr "" msgid "If you want to remove this email address, visit the %{settings_link_to} page." msgstr "" +msgid "If you've lost access to the email associated to this account or having trouble with the code," +msgstr "" + msgid "If you've purchased or renewed your subscription and have an activation code, please enter it below to start the activation process." msgstr "" @@ -28827,6 +28845,9 @@ msgstr "" msgid "Please enter a valid URL format, ex: http://www.example.com/home" msgstr "" +msgid "Please enter a valid code" +msgstr "" + msgid "Please enter a valid hex (#RRGGBB or #RGB) color value" msgstr "" @@ -32725,6 +32746,9 @@ msgstr "" msgid "Resend Request" msgstr "" +msgid "Resend code" +msgstr "" + msgid "Resend confirmation email" msgstr "" @@ -38352,6 +38376,12 @@ msgstr "" msgid "The character highlighter helps you keep the subject line to %{titleLength} characters and wrap the body at %{bodyLength} so they are readable in git." msgstr "" +msgid "The code has expired. Resend a new code and try again." +msgstr "" + +msgid "The code is incorrect. Enter it again, or resend a new code." +msgstr "" + msgid "The comment you are editing has been changed by another user. Would you like to keep your changes and overwrite the new description or discard your changes?" msgstr "" @@ -42259,9 +42289,15 @@ msgstr "" msgid "Various settings that affect GitLab performance." msgstr "" +msgid "Verification code" +msgstr "" + msgid "Verification status" msgstr "" +msgid "Verification successful" +msgstr "" + msgid "VerificationReminder|Pipeline failing? To keep GitLab spam and abuse free we ask that you verify your identity." msgstr "" @@ -42286,6 +42322,9 @@ msgstr "" msgid "Verify configuration" msgstr "" +msgid "Verify your identity" +msgstr "" + msgid "Version" msgstr "" @@ -44502,6 +44541,9 @@ msgstr "" msgid "Your account has been deactivated. You will not be able to: " msgstr "" +msgid "Your account has been successfully verified. You'll be redirected to your account in just a moment or %{redirect_url_start}click here%{redirect_url_end} to refresh." +msgstr "" + msgid "Your account is authenticated with SSO or SAML. To %{push_pull_link_start}push and pull%{link_end} over %{protocol} with Git using this account, you must %{set_password_link_start}set a password%{link_end} or %{set_up_pat_link_start}set up a Personal Access Token%{link_end} to use instead of a password. For more information, see %{clone_with_https_link_start}Clone with HTTPS%{link_end}." msgstr "" @@ -44719,6 +44761,9 @@ msgstr[1] "" msgid "Your username is %{username}." msgstr "" +msgid "Your verification code expires after %{expires_in_minutes} minutes." +msgstr "" + msgid "ZenTaoIntegration|Failed to load ZenTao issue. View the issue in ZenTao, or reload the page." msgstr "" @@ -45602,6 +45647,9 @@ msgstr "" msgid "help" msgstr "" +msgid "here are some other steps you can take." +msgstr "" + msgid "http:" msgstr "" diff --git a/spec/features/users/email_verification_on_login_spec.rb b/spec/features/users/email_verification_on_login_spec.rb new file mode 100644 index 00000000000000..b1e74e175cd30c --- /dev/null +++ b/spec/features/users/email_verification_on_login_spec.rb @@ -0,0 +1,247 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Email Verification On Login' do + include EmailHelpers + + let_it_be(:user) { create(:user) } + + let(:require_email_verification_enabled) { true } + + before do + stub_feature_flags(require_email_verification: require_email_verification_enabled) + end + + shared_examples 'email verification required' do + before do + allow(Gitlab::AppLogger).to receive(:info) + end + + it 'requires email verification before being able to access GitLab' do + perform_enqueued_jobs do + # When logging in + gitlab_sign_in(user) + expect_log_message("Account Locked: username=#{user.username}") + expect_log_message("Email Verification Instructions Sent: username=#{user.username} ip=127.0.0.1") + + # Expect the user to be locked and the unlock_token to be set + user.reload + expect(user.locked_at).not_to be_nil + expect(user.unlock_token).not_to be_nil + + # Expect to see the verification form on the login page + expect(page).to have_current_path(new_user_session_path) + expect(page).to have_content('Help us protect your account') + + # Expect an instructions email to be sent with a code + code = expect_instructions_email_and_extract_code + + # Request a new code + click_link 'Resend code' + expect_log_message("Email Verification Instructions Sent: username=#{user.username} ip=127.0.0.1", 2) + new_code = expect_instructions_email_and_extract_code + + # Verify the old code is different from the new code + expect(code).not_to eq(new_code) + + # Verify the old code is invalid + verify_code(code) + expect_log_message("Email Verification Failed Attempt: username=#{user.username} ip=127.0.0.1 reason=invalid") + expect(page).to have_content('The code is incorrect. Enter it again, or resend a new code.') + + # Wait for the token to expire + travel VerifiesWithEmail::TOKEN_VALID_FOR_MINUTES.minutes + 1.second + + # Verify the new code is expired + verify_code(new_code) + expect_log_message("Email Verification Failed Attempt: username=#{user.username} ip=127.0.0.1 reason=expired") + expect(page).to have_content('The code has expired. Resend a new code and try again.') + + # Request another code + click_link 'Resend code' + expect_log_message("Email Verification Instructions Sent: username=#{user.username} ip=127.0.0.1", 3) + another_code = expect_instructions_email_and_extract_code + + # Signing in again prompts for the code and doesn't send a new one + gitlab_sign_in(user) + expect(page).to have_current_path(new_user_session_path) + expect(page).to have_content('Help us protect your account') + + # Verify the code + verify_code(another_code) + expect_log_message("Email Verification Successful: username=#{user.username} ip=127.0.0.1") + expect_log_message("Successful Login: username=#{user.username} ip=127.0.0.1 method=standard admin=false") + + # Expect the user to be unlocked + expect_user_to_be_unlocked + + # Expect a confirmation page with a meta refresh tag for 3 seconds to the root + expect(page).to have_current_path(users_successful_verification_path) + expect(page).to have_content('Verification successful') + expect(page).to have_selector("meta[http-equiv='refresh'][content='3; url=#{root_path}']", visible: false) + end + end + end + + shared_examples 'no email verification required' do |**login_args| + it 'does not lock the user and redirects to the root page after logging in' do + gitlab_sign_in(user, **login_args) + + expect_user_to_be_unlocked + + expect(page).to have_current_path(root_path) + end + end + + shared_examples 'no email verification required when 2fa enabled or ff disabled' do + context 'when 2FA is enabled' do + let_it_be(:user) { create(:user, :two_factor) } + + it_behaves_like 'no email verification required', two_factor_auth: true + end + + context 'when the feature flag is disabled' do + let(:require_email_verification_enabled) { false } + + it_behaves_like 'no email verification required' + end + end + + describe 'when failing to login the maximum allowed number of times' do + before do + # See comment in RequireEmailVerification::MAXIMUM_ATTEMPTS on why this is divided by 2 + (RequireEmailVerification::MAXIMUM_ATTEMPTS / 2).times do + gitlab_sign_in(user, password: 'wrong_password') + end + end + + it 'locks the user, but does not set the unlock token', :aggregate_failures do + user.reload + expect(user.locked_at).not_to be_nil + expect(user.unlock_token).to be_nil # The unlock token is only set after logging in with valid credentials + expect(user.failed_attempts).to eq(RequireEmailVerification::MAXIMUM_ATTEMPTS) + end + + it_behaves_like 'email verification required' + it_behaves_like 'no email verification required when 2fa enabled or ff disabled' + + describe 'when waiting for the auto unlock time' do + before do + travel User::UNLOCK_IN + 1.second + end + + it_behaves_like 'no email verification required' + end + end + + describe 'when no previous authentication event exists' do + it_behaves_like 'no email verification required' + end + + describe 'when a previous authentication event exists for another ip address' do + before do + create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') + end + + it_behaves_like 'email verification required' + it_behaves_like 'no email verification required when 2fa enabled or ff disabled' + end + + describe 'inconsistent states' do + context 'when the feature flag is toggled off after being prompted for a verification token' do + before do + create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') + end + + it 'stil accepts the token' do + perform_enqueued_jobs do + # The user is prompted for a verification code + gitlab_sign_in(user) + expect(page).to have_content('Help us protect your account') + code = expect_instructions_email_and_extract_code + + # We toggle the feature flag off + stub_feature_flags(require_email_verification: false) + + # Resending and veryfying the code work as expected + click_link 'Resend code' + new_code = expect_instructions_email_and_extract_code + verify_code(code) + expect(page).to have_content('The code is incorrect. Enter it again, or resend a new code.') + travel VerifiesWithEmail::TOKEN_VALID_FOR_MINUTES.minutes + 1.second + verify_code(new_code) + expect(page).to have_content('The code has expired. Resend a new code and try again.') + click_link 'Resend code' + another_code = expect_instructions_email_and_extract_code + verify_code(another_code) + expect_user_to_be_unlocked + expect(page).to have_current_path(users_successful_verification_path) + end + end + end + + context 'when the feature flag is toggled on after Devise sent unlock instructions' do + let(:require_email_verification_enabled) { false } + + before do + perform_enqueued_jobs do + (User.maximum_attempts / 2).times do + gitlab_sign_in(user, password: 'wrong_password') + end + end + end + + it 'the unlock link still works' do + # The user is locked and unlock instructions are sent + expect(page).to have_content('Invalid login or password.') + user.reload + expect(user.locked_at).not_to be_nil + expect(user.unlock_token).not_to be_nil + mail = find_email_for(user) + expect(mail.to).to match_array([user.email]) + expect(mail.subject).to eq('Unlock instructions') + unlock_url = mail.body.parts.first.to_s[/http.*/] + + # We toggle the feature flag on + stub_feature_flags(require_email_verification: true) + + # Unlocking works as expected + visit unlock_url + expect_user_to_be_unlocked + expect(page).to have_current_path(new_user_session_path) + expect(page).to have_content('Your account has been unlocked successfully') + gitlab_sign_in(user) + expect(page).to have_current_path(root_path) + end + end + end + + def expect_user_to_be_unlocked + user.reload + + aggregate_failures do + expect(user.locked_at).to be_nil + expect(user.unlock_token).to be_nil + expect(user.failed_attempts).to eq(0) + end + end + + def expect_instructions_email_and_extract_code + mail = find_email_for(user) + expect(mail.to).to match_array([user.email]) + expect(mail.subject).to eq('Verify your identity') + code = mail.body.parts.first.to_s[/\d{#{VerifiesWithEmail::TOKEN_LENGTH}}/] + reset_delivered_emails! + code + end + + def verify_code(code) + fill_in 'Verification code', with: code + click_button 'Verify code' + end + + def expect_log_message(message, times = 1) + expect(Gitlab::AppLogger).to have_received(:info).exactly(times).times.with(message) + end +end diff --git a/spec/helpers/sessions_helper_spec.rb b/spec/helpers/sessions_helper_spec.rb index fd3d7100ba1631..575a4a8016bc05 100644 --- a/spec/helpers/sessions_helper_spec.rb +++ b/spec/helpers/sessions_helper_spec.rb @@ -50,4 +50,26 @@ expect(helper.unconfirmed_email?).to be_falsey end end + + describe '#obfuscated_email' do + subject { helper.obfuscated_email(email) } + + context 'when an email address is normal length' do + let(:email) { 'alex@gitlab.com' } + + it { is_expected.to eq('al**@g*****.com') } + end + + context 'when an email address contains multiple top level domains' do + let(:email) { 'alex@gl.co.uk' } + + it { is_expected.to eq('al**@g****.uk') } + end + + context 'when an email address is very short' do + let(:email) { 'a@b' } + + it { is_expected.to eq('a@b') } + end + end end diff --git a/spec/models/authentication_event_spec.rb b/spec/models/authentication_event_spec.rb index 83598fa6765dda..23e253c2a289bd 100644 --- a/spec/models/authentication_event_spec.rb +++ b/spec/models/authentication_event_spec.rb @@ -44,4 +44,31 @@ expect(described_class.providers).to match_array %w(ldapmain google_oauth2 standard two-factor two-factor-via-u2f-device two-factor-via-webauthn-device) end end + + describe '.initial_login_or_known_ip_address?' do + let_it_be(:user) { create(:user) } + let_it_be(:ip_address) { '127.0.0.1' } + + subject { described_class.initial_login_or_known_ip_address?(user, ip_address) } + + context 'on first login, when no record exists yet' do + it { is_expected.to eq(true) } + end + + context 'on second login from the same ip address' do + before do + create(:authentication_event, :successful, user: user, ip_address: ip_address) + end + + it { is_expected.to eq(true) } + end + + context 'on second login from another ip address' do + before do + create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') + end + + it { is_expected.to eq(false) } + end + end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 6bd16edcb72237..d01af996141415 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -316,6 +316,10 @@ # most cases. We do test the CAPTCHA flow in the appropriate specs. stub_feature_flags(arkose_labs_login_challenge: false) + # Specs should not require email verification by default, this makes the sign-in flow simpler in + # most cases. We do test the email verification flow in the appropriate specs. + stub_feature_flags(require_email_verification: false) + allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(enable_rugged) else unstub_all_feature_flags -- GitLab From ed1f630a48c6872f916903f71af9000deff9c3c5 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 22 Jun 2022 12:02:17 +0200 Subject: [PATCH 2/6] Implement reviewer feedback Initial reviews --- .../concerns/verifies_with_email.rb | 43 +++-- app/helpers/sessions_helper.rb | 13 +- .../{abuse.rb => identity_verification.rb} | 4 +- app/mailers/notify.rb | 2 +- .../concerns/require_email_verification.rb | 7 +- .../devise/sessions/email_verification.haml | 17 +- .../sessions/successful_verification.haml | 4 +- .../verification_instructions_email.html.haml | 6 +- .../verification_instructions_email.text.erb | 8 +- .../require_email_verification.yml | 4 +- ..._success_index_to_authentication_events.rb | 0 lib/gitlab/application_rate_limiter.rb | 4 +- locale/gitlab.pot | 96 +++++------ .../users/email_verification_on_login_spec.rb | 157 +++++++++++++----- spec/helpers/sessions_helper_spec.rb | 25 +++ ...ess_index_to_authentication_events_spec.rb | 21 +++ .../require_email_verification_spec.rb | 103 ++++++++++++ 17 files changed, 388 insertions(+), 126 deletions(-) rename app/mailers/emails/{abuse.rb => identity_verification.rb} (73%) rename config/feature_flags/{ops => development}/require_email_verification.yml (66%) rename db/{migrate => post_migrate}/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb (100%) create mode 100644 spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb create mode 100644 spec/models/concerns/require_email_verification_spec.rb diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index 0b797f4c77aed5..7ce23e2e8f13bd 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -3,9 +3,9 @@ # == VerifiesWithEmail # # Controller concern to handle verification by email -# module VerifiesWithEmail extend ActiveSupport::Concern + include ActionView::Helpers::DateHelper TOKEN_LENGTH = 6 TOKEN_VALID_FOR_MINUTES = 60 @@ -54,7 +54,7 @@ def successful_verification def find_verification_user return unless session[:verification_user_id] - User.find(session[:verification_user_id]) + User.find_by_id(session[:verification_user_id]) end # After successful verification and calling sign_in, devise redirects the @@ -68,6 +68,8 @@ def after_sign_in_path_for(resource) end def send_verification_instructions(user) + return if send_rate_limited?(user) + raw_token, encrypted_token = generate_token user.unlock_token = encrypted_token user.lock_access!({ send_instructions: false }) @@ -85,7 +87,9 @@ def send_verification_instructions_email(user, token) end def verify_token(user, token) - if invalid_token?(user, token) + if verification_rate_limited?(user) + handle_verification_failure(user, :rate_limited) + elsif invalid_token?(user, token) handle_verification_failure(user, :invalid) elsif expired_token?(user) handle_verification_failure(user, :expired) @@ -104,6 +108,14 @@ def digest_token(token) Devise.token_generator.digest(User, :unlock_token, token) end + def verification_rate_limited?(user) + Gitlab::ApplicationRateLimiter.throttled?(:email_verification, scope: user.unlock_token) + end + + def send_rate_limited?(user) + Gitlab::ApplicationRateLimiter.throttled?(:email_verification_code_send, scope: user) + end + def expired_token?(user) user.locked_at < (Time.current - TOKEN_VALID_FOR_MINUTES.minutes) end @@ -114,10 +126,13 @@ def invalid_token?(user, token) def handle_verification_failure(user, reason) message = case reason + when :rate_limited + s_("IdentityVerification|You've reached the maximum amount of tries. "\ + 'Wait %{interval} or resend a new code and try again.') % { interval: email_verification_interval } when :expired - _('The code has expired. Resend a new code and try again.') + s_('IdentityVerification|The code has expired. Resend a new code and try again.') when :invalid - _('The code is incorrect. Enter it again, or resend a new code.') + s_('IdentityVerification|The code is incorrect. Enter it again, or resend a new code.') end user.errors.add(:base, message) @@ -126,6 +141,11 @@ def handle_verification_failure(user, reason) prompt_for_email_verification(user) end + def email_verification_interval + interval_in_seconds = Gitlab::ApplicationRateLimiter.rate_limits[:email_verification][:interval] + distance_of_time_in_words(interval_in_seconds) + end + def handle_verification_success(user) user.unlock_access! log_verification(user, :successful) @@ -145,13 +165,16 @@ def verification_params end def log_verification(user, event, reason = nil) - message = "Email Verification #{event.to_s.titlecase}: username=#{user.username} ip=#{request.ip}" - message = "#{message} reason=#{reason}" if reason - - Gitlab::AppLogger.info(message) + Gitlab::AppLogger.info( + message: 'Email Verification', + event: event.to_s.titlecase, + username: user.username, + ip: request.ip, + reason: reason.to_s + ) end def require_email_verification_enabled? - Feature.enabled?(:require_email_verification, type: :ops) + Feature.enabled?(:require_email_verification) end end diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index 8486be7e9a3230..129180d1ccfc54 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -40,10 +40,15 @@ def set_session_time(expiry_s) request.env['rack.session.options'][:expire_after] = expiry_s end + def send_rate_limited?(user) + Gitlab::ApplicationRateLimiter.peek(:email_verification_code_send, scope: user) + end + def obfuscated_email(email) - email.sub(/^(..?)(.*)(@.?)(.*)(\..*)$/) do - match = Regexp.last_match - match[1] + '*' * match[2].length + match[3] + '*' * match[4].length + match[5] - end + regex = ::Gitlab::UntrustedRegexp.new('^(..?)(.*)(@.?)(.*)(\..*)$') + match = regex.match(email) + return email unless match + + match[1] + '*' * match[2].length + match[3] + '*' * match[4].length + match[5] end end diff --git a/app/mailers/emails/abuse.rb b/app/mailers/emails/identity_verification.rb similarity index 73% rename from app/mailers/emails/abuse.rb rename to app/mailers/emails/identity_verification.rb index be2ffdf68c1aef..2fc8cae06fe484 100644 --- a/app/mailers/emails/abuse.rb +++ b/app/mailers/emails/identity_verification.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Emails - module Abuse + module IdentityVerification def verification_instructions_email(user_id, token:, expires_in:) @token = token @expires_in_minutes = expires_in @@ -9,7 +9,7 @@ def verification_instructions_email(user_id, token:, expires_in:) @two_fa_link = help_page_url('user/profile/account/two_factor_authentication') user = User.find(user_id) - email_with_layout(to: user.email, subject: _('Verify your identity')) + email_with_layout(to: user.email, subject: s_('IdentityVerification|Verify your identity')) end end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 95ab6c02b84d64..ed7681e595fd2f 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -23,7 +23,7 @@ class Notify < ApplicationMailer include Emails::ServiceDesk include Emails::InProductMarketing include Emails::AdminNotification - include Emails::Abuse + include Emails::IdentityVerification helper TimeboxesHelper helper MergeRequestsHelper diff --git a/app/models/concerns/require_email_verification.rb b/app/models/concerns/require_email_verification.rb index 0cf93dd781ae5e..eb00a9647a347a 100644 --- a/app/models/concerns/require_email_verification.rb +++ b/app/models/concerns/require_email_verification.rb @@ -3,10 +3,9 @@ # == Require Email Verification module # # Contains functionality to handle email verification -# -# module RequireEmailVerification extend ActiveSupport::Concern + include Gitlab::Utils::StrongMemoize # This value is twice the amount we want it to be, because due to a bug in the devise-two-factor # gem every failed login attempt increments the value of failed_attempts by 2 instead of 1. @@ -19,7 +18,7 @@ module RequireEmailVerification attr_accessor :verification_token end - # When overriden, do not send Devise unlock instructions when locking access. + # When overridden, do not send Devise unlock instructions when locking access. def lock_access!(opts = {}) return super unless override_devise_lockable? @@ -47,7 +46,7 @@ def lock_expired? def override_devise_lockable? strong_memoize(:override_devise_lockable) do - Feature.enabled?(:require_email_verification, type: :ops) && !two_factor_enabled? + Feature.enabled?(:require_email_verification) && !two_factor_enabled? end end end diff --git a/app/views/devise/sessions/email_verification.haml b/app/views/devise/sessions/email_verification.haml index 03c1bee01617ff..6cafcb941b4970 100644 --- a/app/views/devise/sessions/email_verification.haml +++ b/app/views/devise/sessions/email_verification.haml @@ -1,18 +1,19 @@ %div - = render 'devise/shared/tab_single', tab_title: _('Help us protect your account') + = render 'devise/shared/tab_single', tab_title: s_('IdentityVerification|Help us protect your account') .login-box.gl-p-5 .login-body = form_for(resource, as: resource_name, url: session_path(resource_name), method: :post, html: { class: 'gl-show-field-errors' }) do |f| %p - = _("For added security, you'll need to verify your identity. We've sent a verification code to %{email}").html_safe % { email: "#{sanitize(obfuscated_email(resource.email))}".html_safe } + = s_("IdentityVerification|For added security, you'll need to verify your identity. We've sent a verification code to %{email}").html_safe % { email: "#{sanitize(obfuscated_email(resource.email))}".html_safe } %div - = f.label :verification_token, _('Verification code') - = f.text_field :verification_token, class: 'form-control gl-form-input', required: true, autofocus: true, autocomplete: 'off', title: _('Please enter a valid code'), inputmode: 'numeric', maxlength: 6, pattern: '[0-9]{6}' + = f.label :verification_token, s_('IdentityVerification|Verification code') + = f.text_field :verification_token, class: 'form-control gl-form-input', required: true, autofocus: true, autocomplete: 'off', title: s_('IdentityVerification|Please enter a valid code'), inputmode: 'numeric', maxlength: 6, pattern: '[0-9]{6}' %p.gl-field-error.gl-mt-2 = resource.errors.full_messages.to_sentence .gl-mt-5 - = f.submit _('Verify code'), class: 'gl-button btn btn-confirm' - = link_to _('Resend code'), users_resend_verification_code_path, method: :post, class: 'form-control gl-button btn-link gl-mt-3 gl-mb-0' + = f.submit s_('IdentityVerification|Verify code'), class: 'gl-button btn btn-confirm' + - unless send_rate_limited?(resource) + = link_to s_('IdentityVerification|Resend code'), users_resend_verification_code_path, method: :post, class: 'form-control gl-button btn-link gl-mt-3 gl-mb-0' %p.gl-p-5.gl-text-secondary - = _("If you've lost access to the email associated to this account or having trouble with the code,") - = link_to _('here are some other steps you can take.'), 'https://about.gitlab.com/support/', target: '_blank', rel: 'noopener noreferrer' + - support_link_start = ''.html_safe + = s_("IdentityVerification|If you've lost access to the email associated to this account or having trouble with the code, %{link_start}here are some other steps you can take.%{link_end}").html_safe % { link_start: support_link_start, link_end: ''.html_safe } diff --git a/app/views/devise/sessions/successful_verification.haml b/app/views/devise/sessions/successful_verification.haml index b4a57a513442d1..8af80fbdceb5ff 100644 --- a/app/views/devise/sessions/successful_verification.haml +++ b/app/views/devise/sessions/successful_verification.haml @@ -4,8 +4,8 @@ .svg-content.svg-80 = image_tag 'illustrations/success-sm.svg' %h2 - = _('Verification successful') + = s_('IdentityVerification|Verification successful') %p.gl-pt-2 - redirect_url_start = ''.html_safe % { url: @redirect_url } - redirect_url_end = ''.html_safe - = html_escape(_("Your account has been successfully verified. You'll be redirected to your account in just a moment or %{redirect_url_start}click here%{redirect_url_end} to refresh.")) % { redirect_url_start: redirect_url_start, redirect_url_end: redirect_url_end } + = html_escape(s_("IdentityVerification|Your account has been successfully verified. You'll be redirected to your account in just a moment or %{redirect_url_start}click here%{redirect_url_end} to refresh.")) % { redirect_url_start: redirect_url_start, redirect_url_end: redirect_url_end } diff --git a/app/views/notify/verification_instructions_email.html.haml b/app/views/notify/verification_instructions_email.html.haml index 3a664b3e759172..63d8d1b2461e03 100644 --- a/app/views/notify/verification_instructions_email.html.haml +++ b/app/views/notify/verification_instructions_email.html.haml @@ -1,12 +1,12 @@ %div{ style: 'text-align:center;color:#1F1F1F;line-height:1.25em;max-width:400px;margin:0 auto;' } %h3 - = _('Help us protect your account') + = s_('IdentityVerification|Help us protect your account') %p{ style: 'font-size:0.9em' } - = _('Before you sign in, we need to verify your identity. Enter the following code on the sign-in page.') + = s_('IdentityVerification|Before you sign in, we need to verify your identity. Enter the following code on the sign-in page.') %div{ style: 'margin:26px 0;width:207px;height:53px;background-color:#F0F0F0;line-height:53px;font-weight:700;font-size:1.5em;color:#303030;' } = @token %p{ style: 'font-size:0.75em' } - = _('If you have not recently tried to sign into GitLab, we recommend %{password_link_start}changing your password%{link_end} and %{two_fa_link_start}setting up Two-Factor Authentication%{link_end} to keep your account safe. Your verification code expires after %{expires_in_minutes} minutes.').html_safe % { link_end: link_end, + = s_('IdentityVerification|If you have not recently tried to sign into GitLab, we recommend %{password_link_start}changing your password%{link_end} and %{two_fa_link_start}setting up Two-Factor Authentication%{link_end} to keep your account safe. Your verification code expires after %{expires_in_minutes} minutes.').html_safe % { link_end: link_end, password_link_start: link_start(@password_link), two_fa_link_start: link_start(@two_fa_link), expires_in_minutes: @expires_in_minutes } diff --git a/app/views/notify/verification_instructions_email.text.erb b/app/views/notify/verification_instructions_email.text.erb index 5118ed549c9f57..df507b5db719d9 100644 --- a/app/views/notify/verification_instructions_email.text.erb +++ b/app/views/notify/verification_instructions_email.text.erb @@ -1,8 +1,8 @@ -<%= _('Help us protect your account') %> +<%= s_('IdentityVerification|Help us protect your account') %> -<%= _('Before you sign in, we need to verify your identity. Enter the following code on the sign-in page.') %> +<%= s_('IdentityVerification|Before you sign in, we need to verify your identity. Enter the following code on the sign-in page.') %> <%= @token %> -<%= _('If you have not recently tried to sign into GitLab, we recommend changing your password (%{password_link}) and setting up Two-Factor Authentication (%{two_fa_link}) to keep your account safe.') % { password_link: @password_link, two_fa_link: @two_fa_link } %> -<%= _('Your verification code expires after %{expires_in_minutes} minutes.') % { expires_in_minutes: @expires_in_minutes } %> +<%= s_('IdentityVerification|If you have not recently tried to sign into GitLab, we recommend changing your password (%{password_link}) and setting up Two-Factor Authentication (%{two_fa_link}) to keep your account safe.') % { password_link: @password_link, two_fa_link: @two_fa_link } %> +<%= s_('IdentityVerification|Your verification code expires after %{expires_in_minutes} minutes.') % { expires_in_minutes: @expires_in_minutes } %> diff --git a/config/feature_flags/ops/require_email_verification.yml b/config/feature_flags/development/require_email_verification.yml similarity index 66% rename from config/feature_flags/ops/require_email_verification.yml rename to config/feature_flags/development/require_email_verification.yml index e895e2812b8d6d..85c96fed814940 100644 --- a/config/feature_flags/ops/require_email_verification.yml +++ b/config/feature_flags/development/require_email_verification.yml @@ -1,8 +1,8 @@ --- name: require_email_verification introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86352 -rollout_issue_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/364835 milestone: '15.1' -type: ops +type: development group: group::anti-abuse default_enabled: false diff --git a/db/migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb b/db/post_migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb similarity index 100% rename from db/migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb rename to db/post_migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index fd54375bf51363..f50b14c9f58cd1 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -46,7 +46,9 @@ def rate_limits # rubocop:disable Metrics/AbcSize gitlab_shell_operation: { threshold: 600, interval: 1.minute }, pipelines_create: { threshold: -> { application_settings.pipeline_limit_per_project_user_sha }, interval: 1.minute }, temporary_email_failure: { threshold: 50, interval: 1.day }, - project_testing_integration: { threshold: 5, interval: 1.minute } + project_testing_integration: { threshold: 5, interval: 1.minute }, + email_verification: { threshold: 10, interval: 10.minutes }, + email_verification_code_send: { threshold: 10, interval: 1.hour } }.freeze end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e02834aa95b41a..bb13a8d64c427e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5829,9 +5829,6 @@ msgstr "" msgid "Before this can be merged, a Jira issue must be linked in the title or description" msgstr "" -msgid "Before you sign in, we need to verify your identity. Enter the following code on the sign-in page." -msgstr "" - msgid "Begin with the selected commit" msgstr "" @@ -16450,9 +16447,6 @@ msgstr "" msgid "For a faster browsing experience, some files are collapsed by default." msgstr "" -msgid "For added security, you'll need to verify your identity. We've sent a verification code to %{email}" -msgstr "" - msgid "For additional information, review your %{link_to} or contact your group owner." msgstr "" @@ -19008,9 +19002,6 @@ msgstr "" msgid "Help translate GitLab into your language" msgstr "" -msgid "Help us protect your account" -msgstr "" - msgid "Helps prevent bots from brute-force attacks." msgstr "" @@ -19336,15 +19327,63 @@ msgstr "" msgid "IdentityVerification|Before you create your group, we need you to verify your identity with a valid payment method. You will not be charged during this step. If we ever need to charge you, we will let you know." msgstr "" +msgid "IdentityVerification|Before you sign in, we need to verify your identity. Enter the following code on the sign-in page." +msgstr "" + msgid "IdentityVerification|Create a project" msgstr "" +msgid "IdentityVerification|For added security, you'll need to verify your identity. We've sent a verification code to %{email}" +msgstr "" + +msgid "IdentityVerification|Help us protect your account" +msgstr "" + +msgid "IdentityVerification|If you have not recently tried to sign into GitLab, we recommend %{password_link_start}changing your password%{link_end} and %{two_fa_link_start}setting up Two-Factor Authentication%{link_end} to keep your account safe. Your verification code expires after %{expires_in_minutes} minutes." +msgstr "" + +msgid "IdentityVerification|If you have not recently tried to sign into GitLab, we recommend changing your password (%{password_link}) and setting up Two-Factor Authentication (%{two_fa_link}) to keep your account safe." +msgstr "" + +msgid "IdentityVerification|If you've lost access to the email associated to this account or having trouble with the code, %{link_start}here are some other steps you can take.%{link_end}" +msgstr "" + +msgid "IdentityVerification|Please enter a valid code" +msgstr "" + +msgid "IdentityVerification|Resend code" +msgstr "" + +msgid "IdentityVerification|The code has expired. Resend a new code and try again." +msgstr "" + +msgid "IdentityVerification|The code is incorrect. Enter it again, or resend a new code." +msgstr "" + +msgid "IdentityVerification|Verification code" +msgstr "" + +msgid "IdentityVerification|Verification successful" +msgstr "" + +msgid "IdentityVerification|Verify code" +msgstr "" + msgid "IdentityVerification|Verify your identity" msgstr "" msgid "IdentityVerification|You can always verify your account at a later time to create a group." msgstr "" +msgid "IdentityVerification|You've reached the maximum amount of tries. Wait %{interval} or resend a new code and try again." +msgstr "" + +msgid "IdentityVerification|Your account has been successfully verified. You'll be redirected to your account in just a moment or %{redirect_url_start}click here%{redirect_url_end} to refresh." +msgstr "" + +msgid "IdentityVerification|Your verification code expires after %{expires_in_minutes} minutes." +msgstr "" + msgid "If any indexed field exceeds this limit, it is truncated to this number of characters. The rest of the content is neither indexed nor searchable. This does not apply to repository and wiki indexing. For unlimited characters, set this to 0." msgstr "" @@ -19420,12 +19459,6 @@ msgstr "" msgid "If you get a lot of false alarms from repository checks, you can clear all repository check information from the database." msgstr "" -msgid "If you have not recently tried to sign into GitLab, we recommend %{password_link_start}changing your password%{link_end} and %{two_fa_link_start}setting up Two-Factor Authentication%{link_end} to keep your account safe. Your verification code expires after %{expires_in_minutes} minutes." -msgstr "" - -msgid "If you have not recently tried to sign into GitLab, we recommend changing your password (%{password_link}) and setting up Two-Factor Authentication (%{two_fa_link}) to keep your account safe." -msgstr "" - msgid "If you lose your recovery codes you can generate new ones, invalidating all previous codes." msgstr "" @@ -19444,9 +19477,6 @@ msgstr "" msgid "If you want to remove this email address, visit the %{settings_link_to} page." msgstr "" -msgid "If you've lost access to the email associated to this account or having trouble with the code," -msgstr "" - msgid "If you've purchased or renewed your subscription and have an activation code, please enter it below to start the activation process." msgstr "" @@ -28845,9 +28875,6 @@ msgstr "" msgid "Please enter a valid URL format, ex: http://www.example.com/home" msgstr "" -msgid "Please enter a valid code" -msgstr "" - msgid "Please enter a valid hex (#RRGGBB or #RGB) color value" msgstr "" @@ -32746,9 +32773,6 @@ msgstr "" msgid "Resend Request" msgstr "" -msgid "Resend code" -msgstr "" - msgid "Resend confirmation email" msgstr "" @@ -38376,12 +38400,6 @@ msgstr "" msgid "The character highlighter helps you keep the subject line to %{titleLength} characters and wrap the body at %{bodyLength} so they are readable in git." msgstr "" -msgid "The code has expired. Resend a new code and try again." -msgstr "" - -msgid "The code is incorrect. Enter it again, or resend a new code." -msgstr "" - msgid "The comment you are editing has been changed by another user. Would you like to keep your changes and overwrite the new description or discard your changes?" msgstr "" @@ -42289,15 +42307,9 @@ msgstr "" msgid "Various settings that affect GitLab performance." msgstr "" -msgid "Verification code" -msgstr "" - msgid "Verification status" msgstr "" -msgid "Verification successful" -msgstr "" - msgid "VerificationReminder|Pipeline failing? To keep GitLab spam and abuse free we ask that you verify your identity." msgstr "" @@ -42322,9 +42334,6 @@ msgstr "" msgid "Verify configuration" msgstr "" -msgid "Verify your identity" -msgstr "" - msgid "Version" msgstr "" @@ -44541,9 +44550,6 @@ msgstr "" msgid "Your account has been deactivated. You will not be able to: " msgstr "" -msgid "Your account has been successfully verified. You'll be redirected to your account in just a moment or %{redirect_url_start}click here%{redirect_url_end} to refresh." -msgstr "" - msgid "Your account is authenticated with SSO or SAML. To %{push_pull_link_start}push and pull%{link_end} over %{protocol} with Git using this account, you must %{set_password_link_start}set a password%{link_end} or %{set_up_pat_link_start}set up a Personal Access Token%{link_end} to use instead of a password. For more information, see %{clone_with_https_link_start}Clone with HTTPS%{link_end}." msgstr "" @@ -44761,9 +44767,6 @@ msgstr[1] "" msgid "Your username is %{username}." msgstr "" -msgid "Your verification code expires after %{expires_in_minutes} minutes." -msgstr "" - msgid "ZenTaoIntegration|Failed to load ZenTao issue. View the issue in ZenTao, or reload the page." msgstr "" @@ -45647,9 +45650,6 @@ msgstr "" msgid "help" msgstr "" -msgid "here are some other steps you can take." -msgstr "" - msgid "http:" msgstr "" diff --git a/spec/features/users/email_verification_on_login_spec.rb b/spec/features/users/email_verification_on_login_spec.rb index b1e74e175cd30c..3402df6c0b779a 100644 --- a/spec/features/users/email_verification_on_login_spec.rb +++ b/spec/features/users/email_verification_on_login_spec.rb @@ -22,8 +22,8 @@ perform_enqueued_jobs do # When logging in gitlab_sign_in(user) - expect_log_message("Account Locked: username=#{user.username}") - expect_log_message("Email Verification Instructions Sent: username=#{user.username} ip=127.0.0.1") + expect_log_message(message: "Account Locked: username=#{user.username}") + expect_log_message('Instructions Sent') # Expect the user to be locked and the unlock_token to be set user.reload @@ -37,41 +37,16 @@ # Expect an instructions email to be sent with a code code = expect_instructions_email_and_extract_code - # Request a new code - click_link 'Resend code' - expect_log_message("Email Verification Instructions Sent: username=#{user.username} ip=127.0.0.1", 2) - new_code = expect_instructions_email_and_extract_code - - # Verify the old code is different from the new code - expect(code).not_to eq(new_code) - - # Verify the old code is invalid - verify_code(code) - expect_log_message("Email Verification Failed Attempt: username=#{user.username} ip=127.0.0.1 reason=invalid") - expect(page).to have_content('The code is incorrect. Enter it again, or resend a new code.') - - # Wait for the token to expire - travel VerifiesWithEmail::TOKEN_VALID_FOR_MINUTES.minutes + 1.second - - # Verify the new code is expired - verify_code(new_code) - expect_log_message("Email Verification Failed Attempt: username=#{user.username} ip=127.0.0.1 reason=expired") - expect(page).to have_content('The code has expired. Resend a new code and try again.') - - # Request another code - click_link 'Resend code' - expect_log_message("Email Verification Instructions Sent: username=#{user.username} ip=127.0.0.1", 3) - another_code = expect_instructions_email_and_extract_code - # Signing in again prompts for the code and doesn't send a new one gitlab_sign_in(user) expect(page).to have_current_path(new_user_session_path) expect(page).to have_content('Help us protect your account') # Verify the code - verify_code(another_code) - expect_log_message("Email Verification Successful: username=#{user.username} ip=127.0.0.1") - expect_log_message("Successful Login: username=#{user.username} ip=127.0.0.1 method=standard admin=false") + verify_code(code) + expect_log_message('Successful') + expect_log_message(message: "Successful Login: username=#{user.username} "\ + "ip=127.0.0.1 method=standard admin=false") # Expect the user to be unlocked expect_user_to_be_unlocked @@ -82,6 +57,108 @@ expect(page).to have_selector("meta[http-equiv='refresh'][content='3; url=#{root_path}']", visible: false) end end + + describe 'resending a new code' do + it 'resends a new code' do + perform_enqueued_jobs do + # When logging in + gitlab_sign_in(user) + + # Expect an instructions email to be sent with a code + code = expect_instructions_email_and_extract_code + + # Request a new code + click_link 'Resend code' + expect_log_message('Instructions Sent', 2) + new_code = expect_instructions_email_and_extract_code + + # Verify the old code is different from the new code + expect(code).not_to eq(new_code) + end + end + + it 'rate limits resends', :clean_gitlab_redis_rate_limiting do + # When logging in + gitlab_sign_in(user) + + # It shows a resend button + expect(page).to have_link 'Resend code' + + # Resend more than the rate limited amount of times + 10.times do + click_link 'Resend code' + end + + # Expect the link to be gone + expect(page).not_to have_link 'Resend code' + + # Wait for 1 hour + travel 1.hour + + # Now it's visible again + gitlab_sign_in(user) + expect(page).to have_link 'Resend code' + end + end + + describe 'verification errors' do + it 'rate limits verifications' do + perform_enqueued_jobs do + # When logging in + gitlab_sign_in(user) + + # Expect an instructions email to be sent with a code + code = expect_instructions_email_and_extract_code + + # Verify an invalid token more than the rate limited amount of times + 11.times do + verify_code('123456') + end + + # Expect an error message + expect_log_message('Failed Attempt', reason: 'rate_limited') + expect(page).to have_content("You've reached the maximum amount of tries. "\ + 'Wait 10 minutes or resend a new code and try again.') + + # Wait for 10 minutes + travel 10.minutes + + # Now it works again + verify_code(code) + expect_log_message('Successful') + end + end + + it 'verifies invalid codes' do + # When logging in + gitlab_sign_in(user) + + # Verify an invalid code + verify_code('123456') + + # Expect an error message + expect_log_message('Failed Attempt', reason: 'invalid') + expect(page).to have_content('The code is incorrect. Enter it again, or resend a new code.') + end + + it 'verifies expired codes' do + perform_enqueued_jobs do + # When logging in + gitlab_sign_in(user) + + # Expect an instructions email to be sent with a code + code = expect_instructions_email_and_extract_code + + # Wait for the code to expire before verifying + travel VerifiesWithEmail::TOKEN_VALID_FOR_MINUTES.minutes + 1.second + verify_code(code) + + # Expect an error message + expect_log_message('Failed Attempt', reason: 'expired') + expect(page).to have_content('The code has expired. Resend a new code and try again.') + end + end + end end shared_examples 'no email verification required' do |**login_args| @@ -135,11 +212,11 @@ end end - describe 'when no previous authentication event exists' do + describe 'when no previous authentication event exists', :clean_gitlab_redis_rate_limiting do it_behaves_like 'no email verification required' end - describe 'when a previous authentication event exists for another ip address' do + describe 'when a previous authentication event exists for another ip address', :clean_gitlab_redis_rate_limiting do before do create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') end @@ -148,13 +225,13 @@ it_behaves_like 'no email verification required when 2fa enabled or ff disabled' end - describe 'inconsistent states' do + describe 'inconsistent states', :clean_gitlab_redis_rate_limiting do context 'when the feature flag is toggled off after being prompted for a verification token' do before do create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') end - it 'stil accepts the token' do + it 'still accepts the token' do perform_enqueued_jobs do # The user is prompted for a verification code gitlab_sign_in(user) @@ -241,7 +318,13 @@ def verify_code(code) click_button 'Verify code' end - def expect_log_message(message, times = 1) - expect(Gitlab::AppLogger).to have_received(:info).exactly(times).times.with(message) + def expect_log_message(event = nil, times = 1, reason: '', message: nil) + expect(Gitlab::AppLogger).to have_received(:info) + .exactly(times).times + .with(message || hash_including(message: 'Email Verification', + event: event, + username: user.username, + ip: '127.0.0.1', + reason: reason)) end end diff --git a/spec/helpers/sessions_helper_spec.rb b/spec/helpers/sessions_helper_spec.rb index 575a4a8016bc05..15424425060319 100644 --- a/spec/helpers/sessions_helper_spec.rb +++ b/spec/helpers/sessions_helper_spec.rb @@ -51,6 +51,31 @@ end end + describe '#send_rate_limited?' do + let_it_be(:user) { build(:user) } + + subject { helper.send_rate_limited?(user) } + + before do + allow(::Gitlab::ApplicationRateLimiter) + .to receive(:peek) + .with(:email_verification_code_send, scope: user) + .and_return(rate_limited) + end + + context 'when rate limited' do + let(:rate_limited) { true } + + it { is_expected.to eq(true) } + end + + context 'when not rate limited' do + let(:rate_limited) { false } + + it { is_expected.to eq(false) } + end + end + describe '#obfuscated_email' do subject { helper.obfuscated_email(email) } diff --git a/spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb b/spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb new file mode 100644 index 00000000000000..6fa20efc1a65ca --- /dev/null +++ b/spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe AddUserIdAndIpAddressSuccessIndexToAuthenticationEvents do + let(:db) { described_class.new } + let(:index) { described_class::INDEX_NAME } + + it 'correctly migrates up and down' do + reversible_migration do |migration| + migration.before -> { + expect(db.connection.indexes(:authentication_events).map(&:name)).not_to include(index) + } + + migration.after -> { + expect(db.connection.indexes(:authentication_events).map(&:name)).to include(index) + } + end + end +end diff --git a/spec/models/concerns/require_email_verification_spec.rb b/spec/models/concerns/require_email_verification_spec.rb new file mode 100644 index 00000000000000..66e35563c7fd36 --- /dev/null +++ b/spec/models/concerns/require_email_verification_spec.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe RequireEmailVerification do + let_it_be(:model) do + Class.new(ApplicationRecord) do + self.table_name = 'users' + + devise :lockable + + include RequireEmailVerification + end + end + + using RSpec::Parameterized::TableSyntax + + where(:feature_flag_enabled, :two_factor_enabled, :overridden) do + false | false | false + false | true | false + true | false | true + true | true | false + end + + with_them do + let(:instance) { model.new } + + before do + stub_feature_flags(require_email_verification: feature_flag_enabled) + allow(instance).to receive(:two_factor_enabled?).and_return(two_factor_enabled) + end + + describe '#lock_access!' do + subject { instance.lock_access! } + + before do + allow(instance).to receive(:save) + end + + it 'sends Devise unlock instructions unless overridden and always sets locked_at' do + expect(instance).to receive(:send_unlock_instructions).exactly(overridden ? 0 : 1).times + + expect { subject }.to change { instance.locked_at }.from(nil) + end + end + + describe '#attempts_exceeded?' do + subject { instance.send(:attempts_exceeded?) } + + context 'when failed_attempts is LT overridden amount' do + before do + instance.failed_attempts = 5 + end + + it { is_expected.to eq(false) } + end + + context 'when failed_attempts is GTE overridden amount but LT Devise default amount' do + before do + instance.failed_attempts = 6 + end + + it { is_expected.to eq(overridden) } + end + + context 'when failed_attempts is GTE Devise default amount' do + before do + instance.failed_attempts = 10 + end + + it { is_expected.to eq(true) } + end + end + + describe '#lock_expired?' do + subject { instance.send(:lock_expired?) } + + context 'when locked shorter ago than Devise default time' do + before do + instance.locked_at = 9.minutes.ago + end + + it { is_expected.to eq(false) } + end + + context 'when locked longer ago than Devise default time but shorter ago than overriden time' do + before do + instance.locked_at = 11.minutes.ago + end + + it { is_expected.to eq(!overridden) } + end + + context 'when locked longer ago than overriden time' do + before do + instance.locked_at = (24.hours + 1.minute).ago + end + + it { is_expected.to eq(true) } + end + end + end +end -- GitLab From 2cc91630550c910930693fc760bd8815a65ac90a Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 22 Jun 2022 17:32:51 +0200 Subject: [PATCH 3/6] Add rate limiting for user sign in Limit the amount of password guesses, since we now display the email verification page when the password is correct, which could be a giveaway when brute-forced. --- .../concerns/verifies_with_email.rb | 39 +++++++++++++------ lib/gitlab/application_rate_limiter.rb | 1 + locale/gitlab.pot | 3 ++ .../users/email_verification_on_login_spec.rb | 22 ++++++++--- 4 files changed, 49 insertions(+), 16 deletions(-) diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index 7ce23e2e8f13bd..2f8c40f1cf7349 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -20,17 +20,23 @@ def verify_with_email if session[:verification_user_id] && token = verification_params[:verification_token].presence # The verification token is submitted, verify it verify_token(user, token) - elsif require_email_verification_enabled? && user.valid_password?(user_params[:password]) - # The user has logged in successfully. - if user.unlock_token - # Prompt for the token if it already has been set - prompt_for_email_verification(user) - elsif user.access_locked? || !AuthenticationEvent.initial_login_or_known_ip_address?(user, request.ip) - # require email verification if: - # - their account has been locked because of too many failed login attempts, or - # - they have logged in before, but never from the current ip address - send_verification_instructions(user) - prompt_for_email_verification(user) + elsif require_email_verification_enabled? + # Limit the amount of password guesses, since we now display the email verification page + # when the password is correct, which could be a giveaway when brute-forced. + return render_sign_in_rate_limited if check_rate_limit!(:user_sign_in, scope: user) { true } + + if user.valid_password?(user_params[:password]) + # The user has logged in successfully. + if user.unlock_token + # Prompt for the token if it already has been set + prompt_for_email_verification(user) + elsif user.access_locked? || !AuthenticationEvent.initial_login_or_known_ip_address?(user, request.ip) + # require email verification if: + # - their account has been locked because of too many failed login attempts, or + # - they have logged in before, but never from the current ip address + send_verification_instructions(user) + prompt_for_email_verification(user) + end end end end @@ -108,6 +114,17 @@ def digest_token(token) Devise.token_generator.digest(User, :unlock_token, token) end + def render_sign_in_rate_limited + message = s_('IdentityVerification|Maximum login attempts exceeded. '\ + 'Wait %{interval} and try again.') % { interval: user_sign_in_interval } + redirect_to new_user_session_path, alert: message + end + + def user_sign_in_interval + interval_in_seconds = Gitlab::ApplicationRateLimiter.rate_limits[:user_sign_in][:interval] + distance_of_time_in_words(interval_in_seconds) + end + def verification_rate_limited?(user) Gitlab::ApplicationRateLimiter.throttled?(:email_verification, scope: user.unlock_token) end diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index f50b14c9f58cd1..0c52ce8aba48df 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -37,6 +37,7 @@ def rate_limits # rubocop:disable Metrics/AbcSize users_get_by_id: { threshold: -> { application_settings.users_get_by_id_limit }, interval: 10.minutes }, username_exists: { threshold: 20, interval: 1.minute }, user_sign_up: { threshold: 20, interval: 1.minute }, + user_sign_in: { threshold: 5, interval: 10.minutes }, profile_resend_email_confirmation: { threshold: 5, interval: 1.minute }, profile_update_username: { threshold: 10, interval: 1.minute }, update_environment_canary_ingress: { threshold: 1, interval: 1.minute }, diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bb13a8d64c427e..3615ce476fae3b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19348,6 +19348,9 @@ msgstr "" msgid "IdentityVerification|If you've lost access to the email associated to this account or having trouble with the code, %{link_start}here are some other steps you can take.%{link_end}" msgstr "" +msgid "IdentityVerification|Maximum login attempts exceeded. Wait %{interval} and try again." +msgstr "" + msgid "IdentityVerification|Please enter a valid code" msgstr "" diff --git a/spec/features/users/email_verification_on_login_spec.rb b/spec/features/users/email_verification_on_login_spec.rb index 3402df6c0b779a..3fd14de1a818d9 100644 --- a/spec/features/users/email_verification_on_login_spec.rb +++ b/spec/features/users/email_verification_on_login_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe 'Email Verification On Login' do +RSpec.describe 'Email Verification On Login', :clean_gitlab_redis_rate_limiting do include EmailHelpers let_it_be(:user) { create(:user) } @@ -77,7 +77,7 @@ end end - it 'rate limits resends', :clean_gitlab_redis_rate_limiting do + it 'rate limits resends' do # When logging in gitlab_sign_in(user) @@ -212,11 +212,11 @@ end end - describe 'when no previous authentication event exists', :clean_gitlab_redis_rate_limiting do + describe 'when no previous authentication event exists' do it_behaves_like 'no email verification required' end - describe 'when a previous authentication event exists for another ip address', :clean_gitlab_redis_rate_limiting do + describe 'when a previous authentication event exists for another ip address' do before do create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') end @@ -225,7 +225,19 @@ it_behaves_like 'no email verification required when 2fa enabled or ff disabled' end - describe 'inconsistent states', :clean_gitlab_redis_rate_limiting do + describe 'rate limiting password guessing' do + before do + 5.times { gitlab_sign_in(user, password: 'wrong_password') } + gitlab_sign_in(user) + end + + it 'shows an error message on on the login page' do + expect(page).to have_current_path(new_user_session_path) + expect(page).to have_content('Maximum login attempts exceeded. Wait 10 minutes and try again.') + end + end + + describe 'inconsistent states' do context 'when the feature flag is toggled off after being prompted for a verification token' do before do create(:authentication_event, :successful, user: user, ip_address: '1.2.3.4') -- GitLab From f563f0a228e2d350879ad6437b73b52e5154138f Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Thu, 23 Jun 2022 12:23:46 +0200 Subject: [PATCH 4/6] Consolidate indexes on authentication_events This reduces the amount of indexes on authentication_events. --- ..._ip_address_success_index_to_authentication_events.rb | 9 ++++++--- db/structure.sql | 4 +--- ...ddress_success_index_to_authentication_events_spec.rb | 9 ++++++--- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/db/post_migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb b/db/post_migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb index 3e3d5659eabe9d..aa860959c20795 100644 --- a/db/post_migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb +++ b/db/post_migrate/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events.rb @@ -1,15 +1,18 @@ # frozen_string_literal: true class AddUserIdAndIpAddressSuccessIndexToAuthenticationEvents < Gitlab::Database::Migration[2.0] - INDEX_NAME = 'index_authentication_events_on_user_id_and_ip_address_success' + OLD_INDEX_NAME = 'index_authentication_events_on_user_id' + NEW_INDEX_NAME = 'index_authentication_events_on_user_and_ip_address_and_result' disable_ddl_transaction! def up - add_concurrent_index :authentication_events, [:user_id, :ip_address], where: 'result = 1', name: INDEX_NAME + add_concurrent_index :authentication_events, [:user_id, :ip_address, :result], name: NEW_INDEX_NAME + remove_concurrent_index_by_name :authentication_events, OLD_INDEX_NAME end def down - remove_concurrent_index_by_name :authentication_events, INDEX_NAME + add_concurrent_index :authentication_events, :user_id, name: OLD_INDEX_NAME + remove_concurrent_index_by_name :authentication_events, NEW_INDEX_NAME end end diff --git a/db/structure.sql b/db/structure.sql index e53ae0364f4cae..21880fea0af471 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -27282,9 +27282,7 @@ CREATE INDEX index_authentication_events_on_provider ON authentication_events US CREATE INDEX index_authentication_events_on_provider_user_id_created_at ON authentication_events USING btree (provider, user_id, created_at) WHERE (result = 1); -CREATE INDEX index_authentication_events_on_user_id ON authentication_events USING btree (user_id); - -CREATE INDEX index_authentication_events_on_user_id_and_ip_address_success ON authentication_events USING btree (user_id, ip_address) WHERE (result = 1); +CREATE INDEX index_authentication_events_on_user_and_ip_address_and_result ON authentication_events USING btree (user_id, ip_address, result); CREATE INDEX index_award_emoji_on_awardable_type_and_awardable_id ON award_emoji USING btree (awardable_type, awardable_id); diff --git a/spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb b/spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb index 6fa20efc1a65ca..8cb6ab23fef09f 100644 --- a/spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb +++ b/spec/migrations/20220601152916_add_user_id_and_ip_address_success_index_to_authentication_events_spec.rb @@ -5,16 +5,19 @@ RSpec.describe AddUserIdAndIpAddressSuccessIndexToAuthenticationEvents do let(:db) { described_class.new } - let(:index) { described_class::INDEX_NAME } + let(:old_index) { described_class::OLD_INDEX_NAME } + let(:new_index) { described_class::NEW_INDEX_NAME } it 'correctly migrates up and down' do reversible_migration do |migration| migration.before -> { - expect(db.connection.indexes(:authentication_events).map(&:name)).not_to include(index) + expect(db.connection.indexes(:authentication_events).map(&:name)).to include(old_index) + expect(db.connection.indexes(:authentication_events).map(&:name)).not_to include(new_index) } migration.after -> { - expect(db.connection.indexes(:authentication_events).map(&:name)).to include(index) + expect(db.connection.indexes(:authentication_events).map(&:name)).to include(new_index) + expect(db.connection.indexes(:authentication_events).map(&:name)).not_to include(old_index) } end end -- GitLab From 061f4735d82214be16cc20d971c1014efea6d93c Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Thu, 23 Jun 2022 12:53:24 +0200 Subject: [PATCH 5/6] Refactor verify_token method With guard clauses instead of branching --- .../concerns/verifies_with_email.rb | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index 2f8c40f1cf7349..1854acf2d5e2bc 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -93,15 +93,11 @@ def send_verification_instructions_email(user, token) end def verify_token(user, token) - if verification_rate_limited?(user) - handle_verification_failure(user, :rate_limited) - elsif invalid_token?(user, token) - handle_verification_failure(user, :invalid) - elsif expired_token?(user) - handle_verification_failure(user, :expired) - else - handle_verification_success(user) - end + return handle_verification_failure(user, :rate_limited) if verification_rate_limited?(user) + return handle_verification_failure(user, :invalid) unless valid_token?(user, token) + return handle_verification_failure(user, :expired) if expired_token?(user) + + handle_verification_success(user) end def generate_token @@ -137,8 +133,8 @@ def expired_token?(user) user.locked_at < (Time.current - TOKEN_VALID_FOR_MINUTES.minutes) end - def invalid_token?(user, token) - user.unlock_token != digest_token(token) + def valid_token?(user, token) + user.unlock_token == digest_token(token) end def handle_verification_failure(user, reason) -- GitLab From 42524d8a87767614bc3b2de5bf1bc241930c364c Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Thu, 30 Jun 2022 00:10:48 +0200 Subject: [PATCH 6/6] Add request specs For the VerifiesWithEmail controller concern --- .../concerns/verifies_with_email.rb | 3 +- .../require_email_verification.yml | 2 +- .../users/email_verification_on_login_spec.rb | 15 ++ spec/requests/verifies_with_email_spec.rb | 232 ++++++++++++++++++ 4 files changed, 250 insertions(+), 2 deletions(-) create mode 100644 spec/requests/verifies_with_email_spec.rb diff --git a/app/controllers/concerns/verifies_with_email.rb b/app/controllers/concerns/verifies_with_email.rb index 1854acf2d5e2bc..0dcc95e30e73db 100644 --- a/app/controllers/concerns/verifies_with_email.rb +++ b/app/controllers/concerns/verifies_with_email.rb @@ -80,7 +80,6 @@ def send_verification_instructions(user) user.unlock_token = encrypted_token user.lock_access!({ send_instructions: false }) send_verification_instructions_email(user, raw_token) - log_verification(user, :instructions_sent) end def send_verification_instructions_email(user, token) @@ -90,6 +89,8 @@ def send_verification_instructions_email(user, token) user.id, token: token, expires_in: TOKEN_VALID_FOR_MINUTES).deliver_later + + log_verification(user, :instructions_sent) end def verify_token(user, token) diff --git a/config/feature_flags/development/require_email_verification.yml b/config/feature_flags/development/require_email_verification.yml index 85c96fed814940..e6cb78ffcf7a39 100644 --- a/config/feature_flags/development/require_email_verification.yml +++ b/config/feature_flags/development/require_email_verification.yml @@ -2,7 +2,7 @@ name: require_email_verification introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86352 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/364835 -milestone: '15.1' +milestone: "15.2" type: development group: group::anti-abuse default_enabled: false diff --git a/spec/features/users/email_verification_on_login_spec.rb b/spec/features/users/email_verification_on_login_spec.rb index 3fd14de1a818d9..54eb11ca8576db 100644 --- a/spec/features/users/email_verification_on_login_spec.rb +++ b/spec/features/users/email_verification_on_login_spec.rb @@ -225,6 +225,14 @@ it_behaves_like 'no email verification required when 2fa enabled or ff disabled' end + describe 'when a previous authentication event exists for the same ip address' do + before do + create(:authentication_event, :successful, user: user) + end + + it_behaves_like 'no email verification required' + end + describe 'rate limiting password guessing' do before do 5.times { gitlab_sign_in(user, password: 'wrong_password') } @@ -256,13 +264,18 @@ # Resending and veryfying the code work as expected click_link 'Resend code' new_code = expect_instructions_email_and_extract_code + verify_code(code) expect(page).to have_content('The code is incorrect. Enter it again, or resend a new code.') + travel VerifiesWithEmail::TOKEN_VALID_FOR_MINUTES.minutes + 1.second + verify_code(new_code) expect(page).to have_content('The code has expired. Resend a new code and try again.') + click_link 'Resend code' another_code = expect_instructions_email_and_extract_code + verify_code(another_code) expect_user_to_be_unlocked expect(page).to have_current_path(users_successful_verification_path) @@ -288,6 +301,7 @@ expect(user.locked_at).not_to be_nil expect(user.unlock_token).not_to be_nil mail = find_email_for(user) + expect(mail.to).to match_array([user.email]) expect(mail.subject).to eq('Unlock instructions') unlock_url = mail.body.parts.first.to_s[/http.*/] @@ -300,6 +314,7 @@ expect_user_to_be_unlocked expect(page).to have_current_path(new_user_session_path) expect(page).to have_content('Your account has been unlocked successfully') + gitlab_sign_in(user) expect(page).to have_current_path(root_path) end diff --git a/spec/requests/verifies_with_email_spec.rb b/spec/requests/verifies_with_email_spec.rb new file mode 100644 index 00000000000000..5b568c1d3ed53a --- /dev/null +++ b/spec/requests/verifies_with_email_spec.rb @@ -0,0 +1,232 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'VerifiesWithEmail', :clean_gitlab_redis_sessions, :clean_gitlab_redis_rate_limiting do + include SessionHelpers + include EmailHelpers + + let(:user) { create(:user) } + + shared_examples_for 'send verification instructions' do + it 'locks the user' do + user.reload + expect(user.unlock_token).not_to be_nil + expect(user.locked_at).not_to be_nil + end + + it 'sends an email' do + mail = find_email_for(user) + expect(mail.to).to match_array([user.email]) + expect(mail.subject).to eq('Verify your identity') + end + end + + shared_examples_for 'prompt for email verification' do + it 'sets the verification_user_id session variable and renders the email verification template' do + expect(request.session[:verification_user_id]).to eq(user.id) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('devise/sessions/email_verification') + end + end + + describe 'verify_with_email' do + context 'when user is locked and a verification_user_id session variable exists' do + before do + encrypted_token = Devise.token_generator.digest(User, :unlock_token, 'token') + user.update!(locked_at: Time.current, unlock_token: encrypted_token) + stub_session(verification_user_id: user.id) + end + + context 'when rate limited and a verification_token param exists' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) + + post(user_session_path(user: { verification_token: 'token' })) + end + + it_behaves_like 'prompt for email verification' + + it 'adds a verification error message' do + expect(response.body) + .to include("You've reached the maximum amount of tries. "\ + 'Wait 10 minutes or resend a new code and try again.') + end + end + + context 'when an invalid verification_token param exists' do + before do + post(user_session_path(user: { verification_token: 'invalid_token' })) + end + + it_behaves_like 'prompt for email verification' + + it 'adds a verification error message' do + expect(response.body).to include(('The code is incorrect. Enter it again, or resend a new code.')) + end + end + + context 'when an expired verification_token param exists' do + before do + user.update!(locked_at: 1.hour.ago) + post(user_session_path(user: { verification_token: 'token' })) + end + + it_behaves_like 'prompt for email verification' + + it 'adds a verification error message' do + expect(response.body).to include(('The code has expired. Resend a new code and try again.')) + end + end + + context 'when a valid verification_token param exists' do + before do + post(user_session_path(user: { verification_token: 'token' })) + end + + it 'unlocks the user' do + user.reload + expect(user.unlock_token).to be_nil + expect(user.locked_at).to be_nil + end + + it 'redirects to the successful verification path' do + expect(response).to redirect_to(users_successful_verification_path) + end + end + end + + context 'when signing in with a valid password' do + let(:sign_in) { post(user_session_path(user: { login: user.username, password: user.password })) } + + context 'when the feature flag is toggled on' do + before do + stub_feature_flags(require_email_verification: true) + end + + context 'when rate limited' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) + sign_in + end + + it 'redirects to the login form and shows an alert message' do + expect(response).to redirect_to(new_user_session_path) + expect(flash[:alert]).to eq('Maximum login attempts exceeded. Wait 10 minutes and try again.') + end + end + + context 'when the user already has an unlock_token set' do + before do + user.update!(unlock_token: 'token') + sign_in + end + + it_behaves_like 'prompt for email verification' + end + + context 'when the user is already locked' do + before do + user.update!(locked_at: Time.current) + perform_enqueued_jobs { sign_in } + end + + it_behaves_like 'send verification instructions' + it_behaves_like 'prompt for email verification' + end + + context 'when the user is signing in from an unknown ip address' do + before do + allow(AuthenticationEvent) + .to receive(:initial_login_or_known_ip_address?) + .and_return(false) + + perform_enqueued_jobs { sign_in } + end + + it_behaves_like 'send verification instructions' + it_behaves_like 'prompt for email verification' + end + end + + context 'when the feature flag is toggled off' do + before do + stub_feature_flags(require_email_verification: false) + sign_in + end + + it 'redirects to the root path' do + expect(response).to redirect_to(root_path) + end + end + end + end + + describe 'resend_verification_code' do + context 'when no verification_user_id session variable exists' do + before do + post(users_resend_verification_code_path) + end + + it 'returns 204 No Content' do + expect(response).to have_gitlab_http_status(:no_content) + expect(response.body).to be_empty + end + end + + context 'when a verification_user_id session variable exists' do + before do + stub_session(verification_user_id: user.id) + + perform_enqueued_jobs do + post(users_resend_verification_code_path) + end + end + + it_behaves_like 'send verification instructions' + it_behaves_like 'prompt for email verification' + end + + context 'when exceeding the rate limit' do + before do + allow(Gitlab::ApplicationRateLimiter).to receive(:throttled?).and_return(true) + + stub_session(verification_user_id: user.id) + + perform_enqueued_jobs do + post(users_resend_verification_code_path) + end + end + + it 'does not lock the user' do + user.reload + expect(user.unlock_token).to be_nil + expect(user.locked_at).to be_nil + end + + it 'does not send an email' do + mail = find_email_for(user) + expect(mail).to be_nil + end + + it_behaves_like 'prompt for email verification' + end + end + + describe 'successful_verification' do + before do + sign_in(user) + end + + it 'renders the template and removes the verification_user_id session variable' do + stub_session(verification_user_id: user.id) + + get(users_successful_verification_path) + + expect(request.session.has_key?(:verification_user_id)).to eq(false) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template('successful_verification', layout: 'minimal') + expect(response.body).to include(root_path) + end + end +end -- GitLab