Merge branch 'security-soft-email-confirmation-revert' into 'master'

Do not enable soft email confirmation by default

See merge request gitlab-org/security/gitlab!313

app/controllers/concerns/confirm_email_warning.rb

@@ -10,7 +10,7 @@ module ConfirmEmailWarning
   protected
 
   def show_confirm_warning?
-    html_request? && request.get?
+    html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
   end
 
   def set_confirm_warning

app/controllers/confirmations_controller.rb

@@ -11,6 +11,8 @@ class ConfirmationsController < Devise::ConfirmationsController
   protected
 
   def after_resending_confirmation_instructions_path_for(resource)
+    return users_almost_there_path unless Feature.enabled?(:soft_email_confirmation)
+
     stored_location_for(resource) || dashboard_projects_path
   end
 

app/controllers/registrations_controller.rb

@@ -54,7 +54,7 @@ class RegistrationsController < Devise::RegistrationsController
 
   def welcome
     return redirect_to new_user_registration_path unless current_user
-    return redirect_to stored_location_or_dashboard(current_user) if current_user.role.present? && !current_user.setup_for_company.nil?
+    return redirect_to path_for_signed_in_user(current_user) if current_user.role.present? && !current_user.setup_for_company.nil?
   end
 
   def update_registration
@@ -64,7 +64,7 @@ class RegistrationsController < Devise::RegistrationsController
     if result[:status] == :success
       track_experiment_event(:signup_flow, 'end') # We want this event to be tracked when the user is _in_ the experimental group
       set_flash_message! :notice, :signed_up
-      redirect_to stored_location_or_dashboard(current_user)
+      redirect_to path_for_signed_in_user(current_user)
     else
       render :welcome
     end
@@ -111,14 +111,12 @@ class RegistrationsController < Devise::RegistrationsController
 
     return users_sign_up_welcome_path if experiment_enabled?(:signup_flow)
 
-    stored_location_or_dashboard(user)
+    path_for_signed_in_user(user)
   end
 
   def after_inactive_sign_up_path_for(resource)
-    # With the current `allow_unconfirmed_access_for` Devise setting in config/initializers/8_devise.rb,
-    # this method is never called. Leaving this here in case that value is set to 0.
     Gitlab::AppLogger.info(user_created_message)
-    users_almost_there_path
+    Feature.enabled?(:soft_email_confirmation) ? dashboard_projects_path : users_almost_there_path
   end
 
   private
@@ -180,9 +178,21 @@ class RegistrationsController < Devise::RegistrationsController
     Gitlab::Utils.to_boolean(params[:terms_opt_in])
   end
 
-  def stored_location_or_dashboard(user)
+  def path_for_signed_in_user(user)
+    if requires_confirmation?(user)
+      users_almost_there_path
+    else
       stored_location_for(user) || dashboard_projects_path
     end
+  end
+
+  def requires_confirmation?(user)
+    return false if user.confirmed?
+    return false if Feature.enabled?(:soft_email_confirmation)
+    return false if experiment_enabled?(:signup_flow)
+
+    true
+  end
 
   def load_recaptcha
     Gitlab::Recaptcha.load_configurations!

app/models/user.rb

@@ -1683,6 +1683,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

changelogs/unreleased/security-soft-email-confirmation-revert.yml

+---
+title: Do not enable soft email confirmation by default
+merge_request:
+author:
+type: security

doc/security/user_email_confirmation.md

@@ -5,12 +5,8 @@ type: howto
 # User email confirmation at sign-up
 
 GitLab can be configured to require confirmation of a user's email address when
-the user signs up. When this setting is enabled:
-
-- For GitLab 12.7 and earlier, the user is unable to sign in until they confirm their
-  email address.
-- For GitLab 12.8 and later, the user [has 30 days to confirm their email address](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/31245).
-  After 30 days, they will be unable to log in and access GitLab features.
+the user signs up. When this setting is enabled, the user is unable to sign in until
+they confirm their email address.
 
 In **Admin Area > Settings** (`/admin/application_settings/general`), go to the section
 **Sign-up Restrictions** and look for the **Send confirmation email on sign-up** option.

doc/user/admin_area/settings/sign_up_restrictions.md

@@ -37,12 +37,7 @@ email domains to prevent malicious users from creating accounts.
 ## Require email confirmation
 
 You can send confirmation emails during sign-up and require that users confirm
-their email address. If this setting is selected:
-
-- For GitLab 12.7 and earlier, the user is unable to sign in until they confirm their
-  email address.
-- For GitLab 12.8 and later, the user [has 30 days to confirm their email address](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/31245).
-  After 30 days, they will be unable to log in and access GitLab features.
+their email address before they are allowed to sign in.
 
 ![Email confirmation](img/email_confirmation_v12_7.png)
 

spec/controllers/concerns/confirm_email_warning_spec.rb

@@ -3,6 +3,10 @@
 require 'spec_helper'
 
 describe ConfirmEmailWarning do
+  before do
+    stub_feature_flags(soft_email_confirmation: true)
+  end
+
   controller(ApplicationController) do
     # `described_class` is not available in this context
     include ConfirmEmailWarning

spec/controllers/registrations_controller_spec.rb

@@ -79,29 +79,31 @@ describe RegistrationsController do
           stub_application_setting(send_user_confirmation_email: true)
         end
 
-        context 'when a grace period is active for confirming the email address' do
+        context 'when soft email confirmation is not enabled' do
           before do
-            allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days
+            stub_feature_flags(soft_email_confirmation: false)
+            allow(User).to receive(:allow_unconfirmed_access_for).and_return 0
           end
 
-          it 'sends a confirmation email and redirects to the dashboard' do
+          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])
-            expect(response).to redirect_to(dashboard_projects_path)
+            expect(subject.current_user).to be_nil
           end
         end
 
-        context 'when no grace period is active for confirming the email address' do
+        context 'when soft email confirmation is enabled' do
           before do
-            allow(User).to receive(:allow_unconfirmed_access_for).and_return 0
+            stub_feature_flags(soft_email_confirmation: true)
+            allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days
           end
 
-          it 'sends a confirmation email and redirects to the almost there page' do
+          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(users_almost_there_path)
+            expect(response).to redirect_to(dashboard_projects_path)
           end
         end
       end

spec/features/invites_spec.rb

@@ -135,7 +135,9 @@ describe 'Invites' do
 
         expect(current_path).to eq(dashboard_projects_path)
         expect(page).to have_content(project.full_name)
+
         visit group_path(group)
+
         expect(page).to have_content(group.full_name)
       end
 
@@ -153,6 +155,25 @@ describe 'Invites' do
     context 'email confirmation enabled' do
       let(:send_email_confirmation) { true }
 
+      context 'when soft email confirmation is not enabled' do
+        before do
+          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
@@ -164,7 +185,9 @@ describe 'Invites' do
 
           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
@@ -180,7 +203,24 @@ 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(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
 
@@ -192,4 +232,5 @@ describe 'Invites' do
         end
       end
     end
+  end
 end

spec/features/users/login_spec.rb

@@ -797,6 +797,7 @@ describe 'Login' do
 
     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
 

spec/features/users/signup_spec.rb

@@ -129,6 +129,39 @@ shared_examples 'Signup' 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 new_user_registration_path
+
+          fill_in 'new_user_username', with: new_user.username
+          fill_in 'new_user_email', with: new_user.email
+
+          if Gitlab::Experimentation.enabled?(:signup_flow)
+            fill_in 'new_user_first_name', with: new_user.first_name
+            fill_in 'new_user_last_name', with: new_user.last_name
+          else
+            fill_in 'new_user_name', with: new_user.name
+            fill_in 'new_user_email_confirmation', with: new_user.email
+          end
+
+          fill_in 'new_user_password', with: new_user.password
+
+          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')
+        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 new_user_registration_path
 
@@ -155,6 +188,7 @@ shared_examples 'Signup' do
           end
         end
       end
+    end
 
     context "when sigining up with different cased emails" do
       it "creates the user successfully" do