From 8dc700535c6bc2612fddde8c9d1b65798bf81fb3 Mon Sep 17 00:00:00 2001 From: Sruthi Chandran Date: Thu, 12 Mar 2020 21:24:23 +0530 Subject: [PATCH] New upstream version 12.8.6 --- CHANGELOG-EE.md | 4 ++ CHANGELOG.md | 7 ++ GITALY_SERVER_VERSION | 2 +- VERSION | 2 +- .../concerns/confirm_email_warning.rb | 2 +- app/controllers/confirmations_controller.rb | 2 + app/controllers/registrations_controller.rb | 26 ++++--- app/models/user.rb | 7 ++ .../concerns/confirm_email_warning_spec.rb | 4 ++ .../registrations_controller_spec.rb | 32 +++++---- spec/features/invites_spec.rb | 51 +++++++++++-- spec/features/users/login_spec.rb | 1 + spec/features/users/signup_spec.rb | 72 ++++++++++++++----- 13 files changed, 162 insertions(+), 50 deletions(-) diff --git a/CHANGELOG-EE.md b/CHANGELOG-EE.md index 806beb9775..e72168bfc2 100644 --- a/CHANGELOG-EE.md +++ b/CHANGELOG-EE.md @@ -1,5 +1,9 @@ Please view this file on the master branch, on stable branches it's out of date. +## 12.8.5 + +- No changes. + ## 12.8.4 - Unreleased due to tagging failure. diff --git a/CHANGELOG.md b/CHANGELOG.md index a12a34ca1f..e95c28605b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. +## 12.8.6 (2020-03-11) + +### Security (1 change) + +- Do not enable soft email confirmation by default. + + ## 12.8.5 ### Fixed (8 changes) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 090faaf2a2..51fbd82b8c 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -12.8.5 +12.8.6 diff --git a/VERSION b/VERSION index 090faaf2a2..51fbd82b8c 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -12.8.5 +12.8.6 diff --git a/app/controllers/concerns/confirm_email_warning.rb b/app/controllers/concerns/confirm_email_warning.rb index f1c0bcd491..32e1a46e58 100644 --- a/app/controllers/concerns/confirm_email_warning.rb +++ b/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 diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index f99345fa99..a27c402738 100644 --- a/app/controllers/confirmations_controller.rb +++ b/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 diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index 1c6cbf72cf..06751dfbec 100644 --- a/app/controllers/registrations_controller.rb +++ b/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,8 +178,20 @@ class RegistrationsController < Devise::RegistrationsController Gitlab::Utils.to_boolean(params[:terms_opt_in]) end - def stored_location_or_dashboard(user) - stored_location_for(user) || dashboard_projects_path + 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 diff --git a/app/models/user.rb b/app/models/user.rb index ec9bc7ae01..3051df822d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1670,6 +1670,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 diff --git a/spec/controllers/concerns/confirm_email_warning_spec.rb b/spec/controllers/concerns/confirm_email_warning_spec.rb index 2aad380203..93e3423261 100644 --- a/spec/controllers/concerns/confirm_email_warning_spec.rb +++ b/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 diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 8d79e505e5..d7fe3e8705 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -79,31 +79,33 @@ 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 + 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]) + expect(subject.current_user).to be_nil + 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 'sends a confirmation email and redirects to the dashboard' 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(dashboard_projects_path) end end - - context 'when no grace period is active for confirming the email address' do - before do - allow(User).to receive(:allow_unconfirmed_access_for).and_return 0 - end - - it 'sends a confirmation email and redirects to the almost there page' 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) - end - end end context 'when signup_enabled? is false' do diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb index 7884a16c11..9cd0189457 100644 --- a/spec/features/invites_spec.rb +++ b/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,14 +203,32 @@ describe 'Invites' do context 'the user sign-up using a different email address' do let(:invite_email) { build_stubbed(:user).email } - before do - allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days + 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 - it 'signs up and redirects to the invitation page' do - fill_in_sign_up_form(new_user) + 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 - expect(current_path).to eq(invite_path(group_invite.raw_invite_token)) + 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 diff --git a/spec/features/users/login_spec.rb b/spec/features/users/login_spec.rb index 0bef61a485..5a8db3c070 100644 --- a/spec/features/users/login_spec.rb +++ b/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 diff --git a/spec/features/users/signup_spec.rb b/spec/features/users/signup_spec.rb index 8d5c0657fa..daa987ea38 100644 --- a/spec/features/users/signup_spec.rb +++ b/spec/features/users/signup_spec.rb @@ -129,29 +129,63 @@ shared_examples 'Signup' do stub_application_setting(send_user_confirmation_email: true) 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 + context 'when soft email confirmation is not enabled' do + before do + stub_feature_flags(soft_email_confirmation: false) end - fill_in 'new_user_password', with: new_user.password + it 'creates the user account and sends a confirmation email' do + visit new_user_registration_path - expect { click_button 'Register' }.to change { User.count }.by(1) + fill_in 'new_user_username', with: new_user.username + fill_in 'new_user_email', with: new_user.email - if Gitlab::Experimentation.enabled?(:signup_flow) - expect(current_path).to eq users_sign_up_welcome_path - else - 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 and unlock the power of CI/CD.") + 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 + + 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) + + if Gitlab::Experimentation.enabled?(:signup_flow) + expect(current_path).to eq users_sign_up_welcome_path + else + 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 and unlock the power of CI/CD.") + end end end end