Merge branch...

Merge branch '47003-user-onboarding-replace-current-email-confirmation-flow-with-a-soft-email-confirmation-flow' into 'master'

Soft email confirmation flow

Closes #47003

See merge request gitlab-org/gitlab-ce!31245

app/assets/stylesheets/framework/flash.scss

@@ -43,6 +43,7 @@
     @extend .alert;
     background-color: $orange-100;
     color: $orange-900;
+    cursor: default;
     margin: 0;
   }
 

app/controllers/application_controller.rb

@@ -12,6 +12,7 @@ class ApplicationController < ActionController::Base
   include EnforcesTwoFactorAuthentication
   include WithPerformanceBar
   include SessionlessAuthentication
+  include ConfirmEmailWarning
 
   before_action :authenticate_user!
   before_action :enforce_terms!, if: :should_enforce_terms?

app/controllers/concerns/confirm_email_warning.rb

+# frozen_string_literal: true
+
+module ConfirmEmailWarning
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
+  end
+
+  protected
+
+  def set_confirm_warning
+    return unless current_user
+    return if current_user.confirmed?
+    return if peek_request? || json_request? || !request.get?
+
+    email = current_user.unconfirmed_email || current_user.email
+
+    flash.now[:warning] = _("Please check your email (%{email}) to verify that you own this address. Didn't receive it? %{resend_link}. Wrong email address? %{update_link}.").html_safe % {
+      email: email,
+      resend_link: view_context.link_to(_('Resend it'), user_confirmation_path(user: { email: email }), method: :post),
+      update_link: view_context.link_to(_('Update it'), profile_path)
+    }
+  end
+end

app/controllers/confirmations_controller.rb

@@ -11,7 +11,7 @@ class ConfirmationsController < Devise::ConfirmationsController
   protected
 
   def after_resending_confirmation_instructions_path_for(resource)
-    users_almost_there_path
+    Feature.enabled?(:soft_email_confirmation) ? stored_location_for(resource) || dashboard_projects_path : users_almost_there_path
   end
 
   def after_confirmation_path_for(resource_name, resource)

app/controllers/registrations_controller.rb

@@ -69,12 +69,12 @@ class RegistrationsController < Devise::RegistrationsController
 
   def after_sign_up_path_for(user)
     Gitlab::AppLogger.info(user_created_message(confirmed: user.confirmed?))
-    user.confirmed? ? stored_location_for(user) || dashboard_projects_path : users_almost_there_path
+    confirmed_or_unconfirmed_access_allowed(user) ? stored_location_or_dashboard(user) : users_almost_there_path
   end
 
   def after_inactive_sign_up_path_for(resource)
     Gitlab::AppLogger.info(user_created_message)
-    users_almost_there_path
+    Feature.enabled?(:soft_email_confirmation) ? dashboard_projects_path : users_almost_there_path
   end
 
   private
@@ -135,4 +135,12 @@ class RegistrationsController < Devise::RegistrationsController
   def terms_accepted?
     Gitlab::Utils.to_boolean(params[:terms_opt_in])
   end
+
+  def confirmed_or_unconfirmed_access_allowed(user)
+    user.confirmed? || Feature.enabled?(:soft_email_confirmation)
+  end
+
+  def stored_location_or_dashboard(user)
+    stored_location_for(user) || dashboard_projects_path
+  end
 end

app/models/user.rb

@@ -1507,6 +1507,13 @@ class User < ApplicationRecord
     super
   end
 
+  # override from Devise::Confirmable
+  def confirmation_period_valid?
+    return false if Feature.disabled?(:soft_email_confirmation)
+
+    super
+  end
+
   private
 
   def default_private_profile_to_false

config/initializers/8_devise.rb

@@ -81,7 +81,7 @@ Devise.setup do |config|
   # You can use this to let your user access some features of your application
   # without confirming the account, but blocking it after a certain period
   # (ie 2 days).
-  # config.allow_unconfirmed_access_for = 2.days
+  config.allow_unconfirmed_access_for = 30.days
 
   # Defines which key will be used when confirming an account
   # config.confirmation_keys = [ :email ]

locale/gitlab.pot

@@ -8146,6 +8146,9 @@ msgstr ""
 msgid "Please add a list to your board first"
 msgstr ""
 
+msgid "Please check your email (%{email}) to verify that you own this address. Didn't receive it? %{resend_link}. Wrong email address? %{update_link}."
+msgstr ""
+
 msgid "Please choose a group URL with no special characters."
 msgstr ""
 
@@ -9528,6 +9531,9 @@ msgstr ""
 msgid "Resend invite"
 msgstr ""
 
+msgid "Resend it"
+msgstr ""
+
 msgid "Reset health check access token"
 msgstr ""
 
@@ -12304,6 +12310,9 @@ msgstr ""
 msgid "Update failed"
 msgstr ""
 
+msgid "Update it"
+msgstr ""
+
 msgid "Update now"
 msgstr ""
 

spec/controllers/concerns/confirm_email_warning_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ConfirmEmailWarning do
+  before do
+    stub_feature_flags(soft_email_confirmation: true)
+    allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days
+  end
+
+  controller(ApplicationController) do
+    # `described_class` is not available in this context
+    include ConfirmEmailWarning # rubocop:disable RSpec/DescribedClass
+
+    def index
+      head :ok
+    end
+  end
+
+  RSpec::Matchers.define :set_confirm_warning_for do |email|
+    match do |response|
+      expect(response).to set_flash.now[:warning].to include("Please check your email (#{email}) to verify that you own this address.")
+    end
+  end
+
+  describe 'confirm email flash warning' do
+    context 'when not signed in' do
+      let(:user) { create(:user, confirmed_at: nil) }
+
+      before do
+        get :index
+      end
+
+      it { is_expected.not_to set_confirm_warning_for(user.email) }
+    end
+
+    context 'when signed in' do
+      before do
+        sign_in(user)
+      end
+
+      context 'with a confirmed user' do
+        let(:user) { create(:user) }
+
+        before do
+          get :index
+        end
+
+        it { is_expected.not_to set_confirm_warning_for(user.email) }
+      end
+
+      context 'with an unconfirmed user' do
+        let(:user) { create(:user, confirmed_at: nil) }
+
+        context 'when executing a peek request' do
+          before do
+            request.path = '/-/peek'
+            get :index
+          end
+
+          it { is_expected.not_to set_confirm_warning_for(user.email) }
+        end
+
+        context 'when executing a json request' do
+          before do
+            get :index, format: :json
+          end
+
+          it { is_expected.not_to set_confirm_warning_for(user.email) }
+        end
+
+        context 'when executing a post request' do
+          before do
+            post :index
+          end
+
+          it { is_expected.not_to set_confirm_warning_for(user.email) }
+        end
+
+        context 'when executing a get request' do
+          before do
+            get :index
+          end
+
+          context 'with an unconfirmed email address present' do
+            let(:user) { create(:user, confirmed_at: nil, unconfirmed_email: 'unconfirmed@gitlab.com') }
+
+            it { is_expected.to set_confirm_warning_for(user.unconfirmed_email) }
+          end
+
+          context 'without an unconfirmed email address present' do
+            it { is_expected.to set_confirm_warning_for(user.email) }
+          end
+        end
+      end
+    end
+  end
+end

spec/controllers/registrations_controller_spec.rb

@@ -30,9 +30,17 @@ describe RegistrationsController do
       end
 
       context 'when send_user_confirmation_email is true' do
-        it 'does not authenticate user and sends confirmation email' do
+        before do
           stub_application_setting(send_user_confirmation_email: true)
+        end
+
+        context 'when soft email confirmation is not enabled' do
+          before do
+            stub_feature_flags(soft_email_confirmation: false)
+            allow(User).to receive(:allow_unconfirmed_access_for).and_return 0
+          end
 
+          it 'does not authenticate the user and sends a confirmation email' do
             post(:create, params: user_params)
 
             expect(ActionMailer::Base.deliveries.last.to.first).to eq(user_params[:user][:email])
@@ -40,6 +48,21 @@ describe RegistrationsController do
           end
         end
 
+        context 'when soft email confirmation is enabled' do
+          before do
+            stub_feature_flags(soft_email_confirmation: true)
+            allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days
+          end
+
+          it 'authenticates the user and sends a confirmation email' do
+            post(:create, params: user_params)
+
+            expect(ActionMailer::Base.deliveries.last.to.first).to eq(user_params[:user][:email])
+            expect(response).to redirect_to(dashboard_projects_path)
+          end
+        end
+      end
+
       context 'when signup_enabled? is false' do
         it 'redirects to sign_in' do
           stub_application_setting(signup_enabled: false)

spec/features/invites_spec.rb

@@ -17,11 +17,10 @@ describe 'Invites' do
     group_invite.generate_invite_token!
   end
 
-  def confirm_email_and_sign_in(new_user)
+  def confirm_email(new_user)
     new_user_token = User.find_by_email(new_user.email).confirmation_token
 
     visit user_confirmation_path(confirmation_token: new_user_token)
-    fill_in_sign_in_form(new_user)
   end
 
   def fill_in_sign_up_form(new_user)
@@ -155,17 +154,41 @@ describe 'Invites' do
     context 'email confirmation enabled' do
       let(:send_email_confirmation) { true }
 
+      context 'when soft email confirmation is not enabled' do
+        before do
+          # stub_feature_flags(soft_email_confirmation: false)
+          allow(User).to receive(:allow_unconfirmed_access_for).and_return 0
+        end
+
+        it 'signs up and redirects to root page with all the project/groups invitation automatically accepted' do
+          fill_in_sign_up_form(new_user)
+          confirm_email(new_user)
+          fill_in_sign_in_form(new_user)
+
+          expect(current_path).to eq(root_path)
+          expect(page).to have_content(project.full_name)
+          visit group_path(group)
+          expect(page).to have_content(group.full_name)
+        end
+      end
+
+      context 'when soft email confirmation is enabled' do
+        before do
+          allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days
+        end
+
         it 'signs up and redirects to root page with all the project/groups invitation automatically accepted' do
           fill_in_sign_up_form(new_user)
-        confirm_email_and_sign_in(new_user)
+          confirm_email(new_user)
 
           expect(current_path).to eq(root_path)
           expect(page).to have_content(project.full_name)
           visit group_path(group)
           expect(page).to have_content(group.full_name)
         end
+      end
 
-      it "doesn't accept invitations until the user confirm his email" do
+      it "doesn't accept invitations until the user confirms his email" do
         fill_in_sign_up_form(new_user)
         sign_in(owner)
 
@@ -176,13 +199,34 @@ describe 'Invites' do
       context 'the user sign-up using a different email address' do
         let(:invite_email) { build_stubbed(:user).email }
 
+        context 'when soft email confirmation is not enabled' do
+          before do
+            stub_feature_flags(soft_email_confirmation: false)
+            allow(User).to receive(:allow_unconfirmed_access_for).and_return 0
+          end
+
           it 'signs up and redirects to the invitation page' do
             fill_in_sign_up_form(new_user)
-          confirm_email_and_sign_in(new_user)
+            confirm_email(new_user)
+            fill_in_sign_in_form(new_user)
 
             expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
           end
         end
+
+        context 'when soft email confirmation is enabled' do
+          before do
+            stub_feature_flags(soft_email_confirmation: true)
+            allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days
+          end
+
+          it 'signs up and redirects to the invitation page' do
+            fill_in_sign_up_form(new_user)
+
+            expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
+          end
+        end
+      end
     end
   end
 end

spec/features/users/login_spec.rb

@@ -781,4 +781,39 @@ describe 'Login' do
       end
     end
   end
+
+  context 'when sending confirmation email and not yet confirmed' do
+    let!(:user) { create(:user, confirmed_at: nil) }
+    let(:grace_period) { 2.days }
+
+    before do
+      stub_application_setting(send_user_confirmation_email: true)
+      stub_feature_flags(soft_email_confirmation: true)
+      allow(User).to receive(:allow_unconfirmed_access_for).and_return grace_period
+    end
+
+    it 'allows login and shows a flash warning to confirm the email address' do
+      expect(authentication_metrics).to increment(:user_authenticated_counter)
+
+      gitlab_sign_in(user)
+
+      expect(current_path).to eq root_path
+      expect(page).to have_content("Please check your email (#{user.email}) to verify that you own this address.")
+    end
+
+    context "when not having confirmed within Devise's allow_unconfirmed_access_for time" do
+      it 'does not allow login and shows a flash alert to confirm the email address' do
+        travel_to((grace_period + 1.day).from_now) do
+          expect(authentication_metrics)
+            .to increment(:user_unauthenticated_counter)
+            .and increment(:user_session_destroyed_counter).twice
+
+          gitlab_sign_in(user)
+
+          expect(current_path).to eq new_user_session_path
+          expect(page).to have_content('You have to confirm your email address before continuing.')
+        end
+      end
+    end
+  end
 end

spec/features/users/signup_spec.rb

@@ -166,11 +166,16 @@ describe 'Signup' do
   end
 
   context 'with no errors' do
-    context "when sending confirmation email" do
+    context 'when sending confirmation email' do
       before do
         stub_application_setting(send_user_confirmation_email: true)
       end
 
+      context 'when soft email confirmation is not enabled' do
+        before do
+          stub_feature_flags(soft_email_confirmation: false)
+        end
+
         it 'creates the user account and sends a confirmation email' do
           visit root_path
 
@@ -183,7 +188,29 @@ describe 'Signup' do
           expect { click_button 'Register' }.to change { User.count }.by(1)
 
           expect(current_path).to eq users_almost_there_path
-        expect(page).to have_content("Please check your email to confirm your account")
+          expect(page).to have_content('Please check your email to confirm your account')
+        end
+      end
+
+      context 'when soft email confirmation is enabled' do
+        before do
+          stub_feature_flags(soft_email_confirmation: true)
+        end
+
+        it 'creates the user account and sends a confirmation email' do
+          visit root_path
+
+          fill_in 'new_user_name',                with: new_user.name
+          fill_in 'new_user_username',            with: new_user.username
+          fill_in 'new_user_email',               with: new_user.email
+          fill_in 'new_user_email_confirmation',  with: new_user.email
+          fill_in 'new_user_password',            with: new_user.password
+
+          expect { click_button 'Register' }.to change { User.count }.by(1)
+
+          expect(current_path).to eq dashboard_projects_path
+          expect(page).to have_content("Please check your email (#{new_user.email}) to verify that you own this address.")
+        end
       end
     end