Merge branch 'bvl-terms-on-registration-ee' into 'master'

[EE-Port] Users can accept terms during registration

See merge request gitlab-org/gitlab-ee!6060

app/controllers/registrations_controller.rb

@@ -5,6 +5,9 @@ class RegistrationsController < Devise::RegistrationsController
   prepend EE::RegistrationsController
 
   before_action :whitelist_query_limiting, only: [:destroy]
+  before_action :ensure_terms_accepted,
+                if: -> { Gitlab::CurrentSettings.current_application_settings.enforce_terms? },
+                only: [:create]
 
   def new
     redirect_to(new_user_session_path)
@@ -20,7 +23,9 @@ class RegistrationsController < Devise::RegistrationsController
 
     if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha
       accept_pending_invitations
-      super
+      super do |new_user|
+        persist_accepted_terms_if_required(new_user)
+      end
     else
       flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
       flash.delete :recaptcha_error
@@ -42,6 +47,16 @@ class RegistrationsController < Devise::RegistrationsController
 
   protected
 
+  def persist_accepted_terms_if_required(new_user)
+    return unless new_user.persisted?
+    return unless Gitlab::CurrentSettings.current_application_settings.enforce_terms?
+
+    if terms_accepted?
+      terms = ApplicationSetting::Term.latest
+      Users::RespondToTermsService.new(new_user, terms).execute(accepted: true)
+    end
+  end
+
   def destroy_confirmation_valid?
     if current_user.confirm_deletion_with_password?
       current_user.valid_password?(params[:password])
@@ -93,4 +108,14 @@ class RegistrationsController < Devise::RegistrationsController
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42380')
   end
+
+  def ensure_terms_accepted
+    return if terms_accepted?
+
+    redirect_to new_user_session_path, alert: _('You must accept our Terms of Service and privacy policy in order to register an account')
+  end
+
+  def terms_accepted?
+    Gitlab::Utils.to_boolean(params[:terms_opt_in])
+  end
 end

app/controllers/users/terms_controller.rb

@@ -2,6 +2,7 @@ module Users
   class TermsController < ApplicationController
     include InternalRedirect
 
+    skip_before_action :authenticate_user!
     skip_before_action :enforce_terms!
     skip_before_action :check_password_expiration
     skip_before_action :check_two_factor_requirement
@@ -14,7 +15,7 @@ module Users
     def index
       @redirect = redirect_path
 
-      if @term.accepted_by_user?(current_user)
+      if current_user && @term.accepted_by_user?(current_user)
         flash.now[:notice] = "You have already accepted the Terms of Service as #{current_user.to_reference}"
       end
     end

app/views/admin/application_settings/_terms.html.haml

-= form_for @application_setting, url: admin_application_settings_path do |f|
+= form_for @application_setting, url: admin_application_settings_path, html: { class: 'fieldset-form' } do |f|
   = form_errors(@application_setting)
 
   %fieldset
@@ -7,13 +7,13 @@
         .form-check
           = f.check_box :enforce_terms, class: 'form-check-input'
           = f.label :enforce_terms, class: 'form-check-label' do
-            = _("Require all users to accept Terms of Service when they access GitLab.")
+            = _("Require all users to accept Terms of Service and Privacy Policy when they access GitLab.")
           .form-text.text-muted
             = _("When enabled, users cannot use GitLab until the terms have been accepted.")
     .form-group.row
       .col-sm-12
         = f.label :terms do
-          = _("Terms of Service Agreement")
+          = _("Terms of Service Agreement and Privacy Policy")
       .col-sm-12
         = f.text_area :terms, class: 'form-control', rows: 8
         .form-text.text-muted

app/views/admin/application_settings/show.html.haml

@@ -50,11 +50,11 @@
 %section.settings.as-terms.no-animate#js-terms-settings{ class: ('expanded' if expanded) }
   .settings-header
     %h4
-      = _('Terms of Service')
+      = _('Terms of Service and Privacy Policy')
     %button.btn.btn-default.js-settings-toggle{ type: 'button' }
       = expanded ? _('Collapse') : _('Expand')
     %p
-      = _('Include a Terms of Service agreement that all users must accept.')
+      = _('Include a Terms of Service agreement and Privacy Policy that all users must accept.')
   .settings-content
     = render 'terms'
 

app/views/devise/shared/_signup_box.html.haml

@@ -22,6 +22,13 @@
         = f.label :password
         = f.password_field :password, class: "form-control bottom", required: true, pattern: ".{#{@minimum_password_length},}", title: "Minimum length is #{@minimum_password_length} characters."
         %p.gl-field-hint Minimum length is #{@minimum_password_length} characters
+      - if Gitlab::CurrentSettings.current_application_settings.enforce_terms?
+        .form-group
+          = check_box_tag :terms_opt_in, '1', false, required: true
+          = label_tag :terms_opt_in do
+            - terms_link = link_to s_("I accept the|Terms of Service and Privacy Policy"), terms_path, target: "_blank"
+            - accept_terms_label = _("I accept the %{terms_link}") % { terms_link: terms_link }
+            = accept_terms_label.html_safe
       = render 'devise/shared/ee/email_opted_in', f: f
       %div
       - if Gitlab::Recaptcha.enabled?

app/views/users/terms/index.html.haml

@@ -2,7 +2,8 @@
 
 .card-body.rendered-terms
   = markdown_field(@term, :terms)
-.card-footer.footer-block.clearfix
+- if current_user
+  .card-footer.footer-block.clearfix
     - if can?(current_user, :accept_terms, @term)
       .float-right
         = button_to accept_term_path(@term, redirect_params), class: 'btn btn-success prepend-left-8' do

changelogs/unreleased/bvl-terms-on-registration.yml

+---
+title: Users can accept terms during registration
+merge_request: 19583
+author:
+type: other

doc/user/admin_area/settings/terms.md

@@ -20,6 +20,19 @@ When an admin enables this feature, they will automattically be
 directed to the page to accept the terms themselves. After they
 accept, they will be directed back to the settings page.
 
+## New registrations
+
+When this feature is enabled, a checkbox will be available in the
+sign-up form.
+
+![Sign up form](img/sign_up_terms.png)
+
+This checkbox will be required during sign up.
+
+Users can review the terms entered in the admin panel before
+accepting. The page will be opened in a new window so they can
+continue their registration afterwards.
+
 ## Accepting terms
 
 When this feature was enabled, the users that have not accepted the

spec/controllers/registrations_controller_spec.rb

 require 'spec_helper'
 
 describe RegistrationsController do
+  include TermsHelper
+
   describe '#create' do
     let(:user_params) { { user: { name: 'new_user', username: 'new_username', email: 'new@user.com', password: 'Any_password' } } }
 
@@ -67,6 +69,25 @@ describe RegistrationsController do
         expect(flash[:notice]).to include 'Welcome! You have signed up successfully.'
       end
     end
+
+    context 'when terms are enforced' do
+      before do
+        enforce_terms
+      end
+
+      it 'redirects back with a notice when the checkbox was not checked' do
+        post :create, user_params
+
+        expect(flash[:alert]).to match /you must accept our terms/i
+      end
+
+      it 'creates the user with agreement when terms are accepted' do
+        post :create, user_params.merge(terms_opt_in: '1')
+
+        expect(subject.current_user).to be_present
+        expect(subject.current_user.terms_accepted?).to be(true)
+      end
+    end
   end
 
   describe '#destroy' do

spec/features/admin/admin_settings_spec.rb

@@ -119,7 +119,7 @@ feature 'Admin updates settings' do
     accept_terms(admin)
 
     page.within('.as-terms') do
-      check 'Require all users to accept Terms of Service when they access GitLab.'
+      check 'Require all users to accept Terms of Service and Privacy Policy when they access GitLab.'
       fill_in 'Terms of Service Agreement', with: 'Be nice!'
       click_button 'Save changes'
     end

spec/features/users/signup_spec.rb

@@ -140,7 +140,7 @@ describe 'Signup' do
       enforce_terms
     end
 
-    it 'asks the user to accept terms before going to the dashboard' do
+    it 'requires the user to check the checkbox' do
       visit root_path
 
       fill_in 'new_user_name',                with: new_user.name
@@ -148,11 +148,24 @@ describe 'Signup' do
       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
-      click_button "Register"
 
-      expect_to_be_on_terms_page
+      click_button 'Register'
+
+      expect(current_path).to eq new_user_session_path
+      expect(page).to have_content(/you must accept our terms of service/i)
+    end
+
+    it 'asks the user to accept terms before going to the dashboard' 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
+      check :terms_opt_in
 
-      click_button 'Accept terms'
+      click_button "Register"
 
       expect(current_path).to eq dashboard_projects_path
     end

spec/features/users/terms_spec.rb

@@ -3,12 +3,10 @@ require 'spec_helper'
 describe 'Users > Terms' do
   include TermsHelper
 
-  let(:user) { create(:user) }
   let!(:term) { create(:term, terms: 'By accepting, you promise to be nice!') }
 
   before do
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
-    sign_in(user)
   end
 
   it 'shows the terms' do
@@ -17,6 +15,22 @@ describe 'Users > Terms' do
     expect(page).to have_content('By accepting, you promise to be nice!')
   end
 
+  it 'does not show buttons to accept, decline or sign out', :aggregate_failures do
+    visit terms_path
+
+    expect(page).not_to have_css('.footer-block')
+    expect(page).not_to have_content('Accept terms')
+    expect(page).not_to have_content('Decline and sign out')
+    expect(page).not_to have_content('Continue')
+  end
+
+  context 'when signed in' do
+    let(:user) { create(:user) }
+
+    before do
+      sign_in(user)
+    end
+
     context 'declining the terms' do
       it 'returns the user to the app' do
         visit terms_path
@@ -116,4 +130,5 @@ describe 'Users > Terms' do
         end
       end
     end
+  end
 end