Merge branch '328229-do-not-require-invited-users-to-confirm-their-email-address' into 'master'

Do not require invited users to confirm their email address

See merge request gitlab-org/gitlab!59790

app/controllers/invites_controller.rb

@@ -93,6 +93,8 @@ class InvitesController < ApplicationController
     store_location_for(:user, invite_landing_url) if member
 
     if user_sign_up?
+      session[:invite_email] = member.invite_email
+
       redirect_to new_user_registration_path(invite_email: member.invite_email), notice: _("To accept this invitation, create an account or sign in.")
     else
       redirect_to new_user_session_path(sign_in_redirect_params), notice: sign_in_notice

app/controllers/registrations_controller.rb

@@ -155,13 +155,21 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def resource
-    @resource ||= Users::BuildService.new(current_user, sign_up_params).execute
+    @resource ||= Users::RegistrationsBuildService
+                    .new(current_user, sign_up_params.merge({ skip_confirmation: skip_email_confirmation? }))
+                    .execute
   end
 
   def devise_mapping
     @devise_mapping ||= Devise.mappings[:user]
   end
 
+  def skip_email_confirmation?
+    invite_email = session.delete(:invite_email)
+
+    sign_up_params[:email] == invite_email
+  end
+
   def load_recaptcha
     Gitlab::Recaptcha.load_configurations!
   end

app/services/users/build_service.rb

@@ -14,9 +14,11 @@ module Users
     end
 
     def execute(skip_authorization: false)
+      @skip_authorization = skip_authorization
+
       raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_create_user?
 
-      user_params = build_user_params(skip_authorization: skip_authorization)
+      user_params = build_user_params
       user = User.new(user_params)
 
       if current_user&.admin?
@@ -37,6 +39,8 @@ module Users
 
     private
 
+    attr_reader :skip_authorization
+
     def identity_attributes
       [:extern_uid, :provider]
     end
@@ -102,7 +106,7 @@ module Users
       ]
     end
 
-    def build_user_params(skip_authorization:)
+    def build_user_params
       if current_user&.admin?
         user_params = params.slice(*admin_create_params)
 
@@ -111,10 +115,10 @@ module Users
         end
       else
         allowed_signup_params = signup_params
-        allowed_signup_params << :skip_confirmation if skip_authorization
+        allowed_signup_params << :skip_confirmation if allow_caller_to_request_skip_confirmation?
 
         user_params = params.slice(*allowed_signup_params)
-        if user_params[:skip_confirmation].nil?
+        if assign_skip_confirmation_from_settings?(user_params)
           user_params[:skip_confirmation] = skip_user_confirmation_email_from_setting
         end
 
@@ -136,6 +140,14 @@ module Users
       user_params
     end
 
+    def allow_caller_to_request_skip_confirmation?
+      skip_authorization
+    end
+
+    def assign_skip_confirmation_from_settings?(user_params)
+      user_params[:skip_confirmation].nil?
+    end
+
     def skip_user_confirmation_email_from_setting
       !Gitlab::CurrentSettings.send_user_confirmation_email
     end

app/services/users/registrations_build_service.rb

+# frozen_string_literal: true
+
+module Users
+  class RegistrationsBuildService < BuildService
+    extend ::Gitlab::Utils::Override
+
+    private
+
+    override :allow_caller_to_request_skip_confirmation?
+    def allow_caller_to_request_skip_confirmation?
+      true
+    end
+
+    override :assign_skip_confirmation_from_settings?
+    def assign_skip_confirmation_from_settings?(user_params)
+      user_params[:skip_confirmation].blank?
+    end
+  end
+end

changelogs/unreleased/328229-do-not-require-invited-users-to-confirm-their-email-address.yml

+---
+title: Do not require invited users to confirm their email address
+merge_request: 59790
+author:
+type: other

spec/controllers/registrations_controller_spec.rb

@@ -22,8 +22,9 @@ RSpec.describe RegistrationsController do
   describe '#create' do
     let(:base_user_params) { { first_name: 'first', last_name: 'last', username: 'new_username', email: 'new@user.com', password: 'Any_password' } }
     let(:user_params) { { user: base_user_params } }
+    let(:session_params) { {} }
 
-    subject { post(:create, params: user_params) }
+    subject { post(:create, params: user_params, session: session_params) }
 
     context '`blocked_pending_approval` state' do
       context 'when the `require_admin_approval_after_user_signup` setting is turned on' do
@@ -148,6 +149,26 @@ RSpec.describe RegistrationsController do
             expect { subject }.to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
             expect(controller.current_user).to be_nil
           end
+
+          context 'when registration is triggered from an accepted invite' do
+            context 'when invite email matches email used on registration' do
+              let(:session_params) { { invite_email: user_params.dig(:user, :email) } }
+
+              it 'signs the user in without sending a confirmation email', :aggregate_failures do
+                expect { subject }.not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
+                expect(controller.current_user).to be_confirmed
+              end
+            end
+
+            context 'when invite email does not match the email used on registration' do
+              let(:session_params) { { invite_email: 'bogus@email.com' } }
+
+              it 'does not authenticate the user and sends a confirmation email', :aggregate_failures do
+                expect { subject }.to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
+                expect(controller.current_user).to be_nil
+              end
+            end
+          end
         end
 
         context 'when soft email confirmation is enabled' do
@@ -161,6 +182,24 @@ RSpec.describe RegistrationsController do
             expect(controller.current_user).to be_present
             expect(response).to redirect_to(users_sign_up_welcome_path)
           end
+
+          context 'when invite email matches email used on registration' do
+            let(:session_params) { { invite_email: user_params.dig(:user, :email) } }
+
+            it 'signs the user in without sending a confirmation email', :aggregate_failures do
+              expect { subject }.not_to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
+              expect(controller.current_user).to be_confirmed
+            end
+          end
+
+          context 'when invite email does not match the email used on registration' do
+            let(:session_params) { { invite_email: 'bogus@email.com' } }
+
+            it 'authenticates the user and sends a confirmation email without confirming', :aggregate_failures do
+              expect { subject }.to have_enqueued_mail(DeviseMailer, :confirmation_instructions)
+              expect(controller.current_user).not_to be_confirmed
+            end
+          end
         end
       end
 

spec/features/invites_spec.rb

@@ -90,7 +90,6 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
     let(:new_user) { build_stubbed(:user) }
     let(:invite_email) { new_user.email }
     let(:group_invite) { create(:group_member, :invited, group: group, invite_email: invite_email, created_by: owner) }
-    let!(:project_invite) { create(:project_member, :invited, project: project, invite_email: invite_email) }
 
     context 'when registering using invitation email' do
       before do
@@ -122,12 +121,6 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
 
           expect(current_path).to eq(activity_group_path(group))
           expect(page).to have_content('You have been granted Owner access to group Owned.')
-
-          visit group_path(group)
-          expect(page).to have_content(group.full_name)
-
-          visit project_path(project)
-          expect(page).to have_content(project.name)
         end
 
         context 'the user sign-up using a different email address' do
@@ -150,18 +143,11 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures 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
+          it 'signs up and redirects to the group activity 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)
             fill_in_welcome_form
 
-            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)
+            expect(current_path).to eq(activity_group_path(group))
           end
         end
 
@@ -170,27 +156,12 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures 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
+          it 'signs up and redirects to to the group activity page with all the project/groups invitation automatically accepted' do
             fill_in_sign_up_form(new_user)
             fill_in_welcome_form
-            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
+            expect(current_path).to eq(activity_group_path(group))
           end
-
-        it "doesn't accept invitations until the user confirms their email" do
-          fill_in_sign_up_form(new_user)
-          fill_in_welcome_form
-          sign_in(owner)
-
-          visit project_project_members_path(project)
-          expect(page).to have_content 'Invited'
         end
 
         context 'the user sign-up using a different email address' do
@@ -202,7 +173,7 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
               allow(User).to receive(:allow_unconfirmed_access_for).and_return 0
             end
 
-            it 'signs up and redirects to the invitation page' do
+            it 'signs up and redirects to the group activity page' do
               fill_in_sign_up_form(new_user)
               confirm_email(new_user)
               fill_in_sign_in_form(new_user)
@@ -218,7 +189,7 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
               allow(User).to receive(:allow_unconfirmed_access_for).and_return 2.days
             end
 
-            it 'signs up and redirects to the invitation page' do
+            it 'signs up and redirects to the group activity page' do
               fill_in_sign_up_form(new_user)
               fill_in_welcome_form
 
@@ -282,7 +253,7 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do
         visit invite_path(group_invite.raw_invite_token)
       end
 
-      it 'grants access and redirects to group page' do
+      it 'grants access and redirects to the group activity page' do
         expect(group.users.include?(user)).to be false
 
         page.click_link 'Accept invitation'

spec/services/users/registrations_build_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::RegistrationsBuildService do
+  describe '#execute' do
+    let(:base_params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
+    let(:skip_param) { {} }
+    let(:params) { base_params.merge(skip_param) }
+
+    subject(:service) { described_class.new(nil, params) }
+
+    before do
+      stub_application_setting(signup_enabled?: true)
+    end
+
+    context 'when automatic user confirmation is not enabled' do
+      before do
+        stub_application_setting(send_user_confirmation_email: true)
+      end
+
+      context 'when skip_confirmation is true' do
+        let(:skip_param) { { skip_confirmation: true } }
+
+        it 'confirms the user' do
+          expect(service.execute).to be_confirmed
+        end
+      end
+
+      context 'when skip_confirmation is not set' do
+        it 'does not confirm the user' do
+          expect(service.execute).not_to be_confirmed
+        end
+      end
+
+      context 'when skip_confirmation is false' do
+        let(:skip_param) { { skip_confirmation: false } }
+
+        it 'does not confirm the user' do
+          expect(service.execute).not_to be_confirmed
+        end
+      end
+    end
+
+    context 'when automatic user confirmation is enabled' do
+      before do
+        stub_application_setting(send_user_confirmation_email: false)
+      end
+
+      context 'when skip_confirmation is true' do
+        let(:skip_param) { { skip_confirmation: true } }
+
+        it 'confirms the user' do
+          expect(service.execute).to be_confirmed
+        end
+      end
+
+      context 'when skip_confirmation is not set the application setting takes precedence' do
+        it 'confirms the user' do
+          expect(service.execute).to be_confirmed
+        end
+      end
+
+      context 'when skip_confirmation is false the application setting takes precedence' do
+        let(:skip_param) { { skip_confirmation: false } }
+
+        it 'confirms the user' do
+          expect(service.execute).to be_confirmed
+        end
+      end
+    end
+  end
+end