Enforces terms in the web application

This enforces the terms in the web application. These cases are
specced:

- Logging in: When terms are enforced, and a user logs in that has not
  accepted the terms, they are presented with the screen. They get
  directed to their customized root path afterwards.
- Signing up: After signing up, the first screen the user is presented
  with the screen to accept the terms. After they accept they are
  directed to the dashboard.
- While a session is active:
  - For a GET: The user will be directed to the terms page first,
    after they accept the terms, they will be directed to the page
    they were going to
  - For any other request: They are directed to the terms, after they
    accept the terms, they are directed back to the page they came
    from to retry the request. Any information entered would be
    persisted in localstorage and available on the page.

app/assets/javascripts/issuable_form.js

@@ -30,7 +30,7 @@ export default class IssuableForm {
     }
 
     this.initAutosave();
-    this.form.on('submit', this.handleSubmit);
+    this.form.on('submit:success', this.handleSubmit);
     this.form.on('click', '.btn-cancel', this.resetAutosave);
     this.initWip();
 

app/assets/stylesheets/framework/terms.scss

 .terms {
+  .alert-wrapper {
+    min-height: $header-height + $gl-padding;
+  }
+
+  .content {
+    padding-top: $gl-padding;
+  }
+
   .panel {
     .panel-heading {
       display: -webkit-flex;
@@ -7,17 +15,15 @@
       justify-content: space-between;
 
       .title {
-        display: -webkit-flex;
         display: flex;
         align-items: center;
-        padding: 2px 8px;
-        margin: 5px 2px 5px -8px;
-        border-radius: 4px;
 
         .logo-text {
           width: 55px;
           height: 24px;
-          margin: 0 15px;
+          display: flex;
+          flex-direction: column;
+          justify-content: center;
         }
       }
 
@@ -31,15 +37,19 @@
     }
 
     .panel-content {
-      padding: 0 $gl-padding;
+      padding: $gl-padding;
+
+      *:first-child {
+        margin-top: 0;
+      }
+
+      *:last-child {
+        margin-bottom: 0;
+      }
     }
 
     .footer-block {
       margin: 0;
-
-      .btn {
-        margin-left: 5px;
-      }
     }
   }
 }

app/controllers/application_controller.rb

@@ -13,12 +13,14 @@ class ApplicationController < ActionController::Base
 
   before_action :authenticate_sessionless_user!
   before_action :authenticate_user!
+  before_action :enforce_terms!, if: -> { Gitlab::CurrentSettings.current_application_settings.enforce_terms },
+                                 unless: :peek_request?
   before_action :validate_user_service_ticket!
   before_action :check_password_expiration
   before_action :ldap_security_check
   before_action :sentry_context
   before_action :default_headers
-  before_action :add_gon_variables, unless: -> { request.path.start_with?('/-/peek') }
+  before_action :add_gon_variables, unless: :peek_request?
   before_action :configure_permitted_parameters, if: :devise_controller?
   before_action :require_email, unless: :devise_controller?
 
@@ -269,6 +271,27 @@ class ApplicationController < ActionController::Base
     end
   end
 
+  def enforce_terms!
+    return unless current_user
+    return if current_user.terms_accepted?
+
+    if sessionless_user?
+      render_403
+    else
+      # Redirect to the destination if the request is a get.
+      # Redirect to the source if it was a post, so the user can re-submit after
+      # accepting the terms.
+      redirect_path = if request.get?
+                        request.fullpath
+                      else
+                        URI(request.referer).path if request.referer
+                      end
+
+      flash[:notice] = _("Please accept the Terms of Service before continuing.")
+      redirect_to terms_path(redirect: redirect_path), status: :found
+    end
+  end
+
   def import_sources_enabled?
     !Gitlab::CurrentSettings.import_sources.empty?
   end
@@ -342,4 +365,12 @@ class ApplicationController < ActionController::Base
     # Per https://tools.ietf.org/html/rfc5987, headers need to be ISO-8859-1, not UTF-8
     response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
   end
+
+  def sessionless_user?
+    current_user && !session.keys.include?('warden.user.user.key')
+  end
+
+  def peek_request?
+    request.path.start_with?('/-/peek')
+  end
 end

app/controllers/concerns/internal_redirect.rb

+module InternalRedirect
+  extend ActiveSupport::Concern
+
+  def safe_redirect_path(path)
+    return unless path
+    # Verify that the string starts with a `/` but not a double `/`.
+    return unless path =~ %r{^/\w.*$}
+
+    uri = URI(path)
+    # Ignore anything path of the redirect except for the path, querystring and,
+    # fragment, forcing the redirect within the same host.
+    full_path_for_uri(uri)
+  rescue URI::InvalidURIError
+    nil
+  end
+
+  def safe_redirect_path_for_url(url)
+    return unless url
+
+    uri = URI(url)
+    safe_redirect_path(full_path_for_uri(uri)) if host_allowed?(uri)
+  rescue URI::InvalidURIError
+    nil
+  end
+
+  def host_allowed?(uri)
+    uri.host == request.host &&
+      uri.port == request.port
+  end
+
+  def full_path_for_uri(uri)
+    path_with_query = [uri.path, uri.query].compact.join('?')
+    [path_with_query, uri.fragment].compact.join("#")
+  end
+end

app/controllers/users/terms_controller.rb

 module Users
   class TermsController < ApplicationController
+    include InternalRedirect
+
+    skip_before_action :enforce_terms!
     before_action :terms
 
     layout 'terms'
@@ -46,11 +49,18 @@ module Users
     end
 
     def redirect_path
-      referer = if request.referer && !request.referer.include?(terms_path)
-                  URI(request.referer).path
-                end
+      redirect_to_path = safe_redirect_path(params[:redirect]) || safe_redirect_path_for_url(request.referer)
+
+      if redirect_to_path &&
+          excluded_redirect_paths.none? { |excluded| redirect_to_path.include?(excluded) }
+        redirect_to_path
+      else
+        root_path
+      end
+    end
 
-      params[:redirect] || referer || root_path
+    def excluded_redirect_paths
+      [terms_path, new_user_session_path]
     end
   end
 end

app/policies/application_setting/term_policy.rb

@@ -2,17 +2,15 @@ class ApplicationSetting
   class TermPolicy < BasePolicy
     include Gitlab::Utils::StrongMemoize
 
-    condition(:logged_in, scope: :user) { @user }
-
     condition(:current_terms, scope: :subject) do
       Gitlab::CurrentSettings.current_application_settings.latest_terms == @subject
     end
 
-    condition(:terms_accepted, scope: :user, score: 1) do
+    condition(:terms_accepted, score: 1) do
       agreement&.accepted
     end
 
-    rule { logged_in & current_terms }.policy do
+    rule { ~anonymous & current_terms }.policy do
       enable :accept_terms
       enable :decline_terms
     end

app/views/layouts/_flash.html.haml

+- extra_flash_class = local_assigns.fetch(:extra_flash_class, nil)
+
 .flash-container.flash-container-page
   -# We currently only support `alert`, `notice`, `success`
   - flash.each do |key, value|
     -# Don't show a flash message if the message is nil
     - if value
       %div{ class: "flash-#{key}" }
-        %div{ class: (container_class) }
+        %div{ class: "#{container_class} #{extra_flash_class}" }
           %span= value

app/views/layouts/terms.html.haml

@@ -4,17 +4,15 @@
   = render "layouts/head"
 
   %body{ data: { page: body_data_page } }
-    = render 'peek/bar'
-    .layout-page.terms
-      .content-wrapper
+    .layout-page.terms{ class: page_class }
+      .content-wrapper.prepend-top-0
         .mobile-overlay
         .alert-wrapper
           = render "layouts/broadcast"
           = render 'layouts/header/read_only_banner'
-          = yield :flash_message
-          = render "layouts/flash"
+          = render "layouts/flash", extra_flash_class: 'limit-container-width'
 
-        %div{ class: "#{container_class}" }
+        %div{ class: "#{container_class} limit-container-width" }
           .content{ id: "content-body" }
             .panel.panel-default
               .panel-heading
@@ -22,7 +20,7 @@
                   = brand_header_logo
                   - logo_text = brand_header_logo_type
                   - if logo_text.present?
-                    %span.logo-text.hidden-xs
+                    %span.logo-text.hidden-xs.prepend-left-8
                       = logo_text
                 - if header_link?(:user_dropdown)
                   .navbar-collapse.collapse
@@ -34,4 +32,3 @@
                         .dropdown-menu-nav.dropdown-menu-align-right
                           = render 'layouts/header/current_user_dropdown'
               = yield
-    = yield :scripts_body

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

 - redirect_params = { redirect: @redirect } if @redirect
+
 .panel-content.rendered-terms
   = markdown_field(@term, :terms)
 .row-content-block.footer-block.clearfix

changelogs/unreleased/bvl-enforce-terms.yml

+---
+title: Allow admins to enforce accepting Terms of Service on an instance
+merge_request: 18570
+author:
+type: added

doc/administration/index.md

@@ -40,6 +40,7 @@ Learn how to install, configure, update, and maintain your GitLab instance.
 [source installations](../install/installation.md#installation-from-source).
 - [Environment variables](environment_variables.md): Supported environment variables that can be used to override their defaults values in order to configure GitLab.
 - [Plugins](plugins.md): With custom plugins, GitLab administrators can introduce custom integrations without modifying GitLab's source code.
+- [Enforcing Terms of Service](../user/admin_area/settings/terms.md)
 
 #### Customizing GitLab's appearance
 

doc/user/admin_area/settings/terms.md

+# Enforce accepting Terms of Service
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/18570)
+> in [GitLab Core](https://about.gitlab.com/pricing/) 10.8
+
+## Configuration
+
+When it is required for all users of the GitLab instance to accept the
+Terms of Service, this can be configured by an admin on the settings
+page:
+
+![Enable enforcing Terms of Service](img/enforce_terms.png).
+
+The terms itself can be entered using Markdown. For each update to the
+terms, a new version is stored. When a user accepts or declines the
+terms, GitLab will keep track of which version they accepted or
+declined.
+
+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.
+
+## Accepting terms
+
+When this feature was enabled, the users that have not accepted the
+terms of service will be presented with a screen where they can either
+accept or decline the terms.
+
+![Respond to terms](img/respond_to_terms.png)
+
+When the user accepts the terms, they will be directed to where they
+were going. After a sign-in or sign-up this will most likely be the
+dashboard.
+
+When the user was already logged in when the feature was turned on,
+they will be asked to accept the terms on their next interaction.
+
+When a user declines the terms, they will be signed out.

spec/controllers/application_controller_spec.rb

 require 'spec_helper'
 
 describe ApplicationController do
+  include TermsHelper
+
   let(:user) { create(:user) }
 
   describe '#check_password_expiration' do
@@ -406,4 +408,65 @@ describe ApplicationController do
       end
     end
   end
+
+  context 'terms' do
+    controller(described_class) do
+      def index
+        render text: 'authenticated'
+      end
+    end
+
+    before do
+      stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+      sign_in user
+    end
+
+    it 'does not query more when terms are enforced' do
+      control = ActiveRecord::QueryRecorder.new { get :index }
+
+      enforce_terms
+
+      expect { get :index }.not_to exceed_query_limit(control)
+    end
+
+    context 'when terms are enforced' do
+      before do
+        enforce_terms
+      end
+
+      it 'redirects if the user did not accept the terms'  do
+        get :index
+
+        expect(response).to have_gitlab_http_status(302)
+      end
+
+      it 'does not redirect when the user accepted terms' do
+        accept_terms(user)
+
+        get :index
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+
+      context 'for sessionless users' do
+        before do
+          sign_out user
+        end
+
+        it 'renders a 403 when the sessionless user did not accept the terms' do
+          get :index, rss_token: user.rss_token, format: :atom
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+
+        it 'renders a 200 when the sessionless user accepted the terms' do
+          accept_terms(user)
+
+          get :index, rss_token: user.rss_token, format: :atom
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+      end
+    end
+  end
 end

spec/controllers/concerns/internal_redirect_spec.rb

+require 'spec_helper'
+
+describe InternalRedirect do
+  let(:controller_class) do
+    Class.new do
+      include InternalRedirect
+
+      def request
+        @request ||= Struct.new(:host, :port).new('test.host', 80)
+      end
+    end
+  end
+  subject(:controller) { controller_class.new }
+
+  describe '#safe_redirect_path' do
+    it 'is `nil` for invalid uris' do
+      expect(controller.safe_redirect_path('Hello world')).to be_nil
+    end
+
+    it 'is `nil` for paths trying to include a host' do
+      expect(controller.safe_redirect_path('//example.com/hello/world')).to be_nil
+    end
+
+    it 'returns the path if it is valid' do
+      expect(controller.safe_redirect_path('/hello/world')).to eq('/hello/world')
+    end
+
+    it 'returns the path with querystring if it is valid' do
+      expect(controller.safe_redirect_path('/hello/world?hello=world#L123'))
+        .to eq('/hello/world?hello=world#L123')
+    end
+  end
+
+  describe '#safe_redirect_path_for_url' do
+    it 'is `nil` for invalid urls' do
+      expect(controller.safe_redirect_path_for_url('Hello world')).to be_nil
+    end
+
+    it 'is `nil` for urls from a with a different host' do
+      expect(controller.safe_redirect_path_for_url('http://example.com/hello/world')).to be_nil
+    end
+
+    it 'is `nil` for urls from a with a different port' do
+      expect(controller.safe_redirect_path_for_url('http://test.host:3000/hello/world')).to be_nil
+    end
+
+    it 'returns the path if the url is on the same host' do
+      expect(controller.safe_redirect_path_for_url('http://test.host/hello/world')).to eq('/hello/world')
+    end
+
+    it 'returns the path including querystring if the url is on the same host' do
+      expect(controller.safe_redirect_path_for_url('http://test.host/hello/world?hello=world#L123'))
+        .to eq('/hello/world?hello=world#L123')
+    end
+  end
+
+  describe '#host_allowed?' do
+    it 'allows uris with the same host and port' do
+      expect(controller.host_allowed?(URI('http://test.host/test'))).to be(true)
+    end
+
+    it 'rejects uris with other host and port' do
+      expect(controller.host_allowed?(URI('http://example.com/test'))).to be(false)
+    end
+  end
+end

spec/controllers/users/terms_controller_spec.rb

@@ -36,6 +36,30 @@ describe Users::TermsController do
 
       expect(response).to redirect_to(groups_path)
     end
+
+    it 'redirects to the referer when no redirect specified' do
+      request.env["HTTP_REFERER"] = groups_url
+
+      post :accept, id: term.id
+
+      expect(response).to redirect_to(groups_path)
+    end
+
+    context 'redirecting to another domain' do
+      it 'is prevented when passing a redirect param' do
+        post :accept, id: term.id, redirect: '//example.com/random/path'
+
+        expect(response).to redirect_to(root_path)
+      end
+
+      it 'is prevented when redirecting to the referer' do
+        request.env["HTTP_REFERER"] = 'http://example.com/and/a/path'
+
+        post :accept, id: term.id
+
+        expect(response).to redirect_to(root_path)
+      end
+    end
   end
 
   describe 'POST #decline' do

spec/features/admin/admin_settings_spec.rb

@@ -2,10 +2,13 @@ require 'spec_helper'
 
 feature 'Admin updates settings' do
   include StubENV
+  include TermsHelper
+
+  let(:admin) { create(:admin) }
 
   before do
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
-    sign_in(create(:admin))
+    sign_in(admin)
     visit admin_application_settings_path
   end
 
@@ -86,6 +89,10 @@ feature 'Admin updates settings' do
   end
 
   scenario 'Terms of Service' do
+    # Already have the admin accept terms, so they don't need to accept in this spec.
+    _existing_terms = create(:term)
+    accept_terms(admin)
+
     page.within('.as-terms') do
       check 'Require all users to accept Terms of Service when they access GitLab.'
       fill_in 'Terms of Service Agreement', with: 'Be nice!'

spec/features/users/login_spec.rb

 require 'spec_helper'
 
 feature 'Login' do
+  include TermsHelper
+
   scenario 'Successful user signin invalidates password reset token' do
     user = create(:user)
 
@@ -399,4 +401,41 @@ feature 'Login' do
       expect(page).to have_selector('.tab-pane.active', count: 1)
     end
   end
+
+  context 'when terms are enforced' do
+    let(:user) { create(:user) }
+
+    before do
+      enforce_terms
+    end
+
+    it 'asks to accept the terms on first login' do
+      visit new_user_session_path
+
+      fill_in 'user_login', with: user.email
+      fill_in 'user_password', with: '12345678'
+
+      click_button 'Sign in'
+
+      expect_to_be_on_terms_page
+
+      click_button 'Accept terms'
+
+      expect(current_path).to eq(root_path)
+      expect(page).not_to have_content('You are already signed in.')
+    end
+
+    it 'does not ask for terms when the user already accepted them' do
+      accept_terms(user)
+
+      visit new_user_session_path
+
+      fill_in 'user_login', with: user.email
+      fill_in 'user_password', with: '12345678'
+
+      click_button 'Sign in'
+
+      expect(current_path).to eq(root_path)
+    end
+  end
 end

spec/features/users/signup_spec.rb

 require 'spec_helper'
 
 describe 'Signup' do
+  include TermsHelper
+
   let(:new_user) { build_stubbed(:user) }
 
   describe 'username validation', :js do
@@ -132,4 +134,27 @@ describe 'Signup' do
       expect(page.body).not_to match(/#{new_user.password}/)
     end
   end
+
+  context 'when terms are enforced' do
+    before do
+      enforce_terms
+    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
+      click_button "Register"
+
+      expect_to_be_on_terms_page
+
+      click_button 'Accept terms'
+
+      expect(current_path).to eq dashboard_projects_path
+    end
+  end
 end

spec/features/users/terms_spec.rb

 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!') }
 
@@ -36,4 +38,47 @@ describe 'Users > Terms' do
       expect(user.reload.terms_accepted?).to be(true)
     end
   end
+
+  context 'terms were enforced while session is active', :js do
+    let(:project) { create(:project) }
+
+    before do
+      project.add_developer(user)
+    end
+
+    it 'redirects to terms and back to where the user was going'  do
+      visit project_path(project)
+
+      enforce_terms
+
+      within('.nav-sidebar') do
+        click_link 'Issues'
+      end
+
+      expect_to_be_on_terms_page
+
+      click_button('Accept terms')
+
+      expect(current_path).to eq(project_issues_path(project))
+    end
+
+    it 'redirects back to the page the user was trying to save' do
+      visit new_project_issue_path(project)
+
+      fill_in :issue_title, with: 'Hello world, a new issue'
+      fill_in :issue_description, with: "We don't want to lose what the user typed"
+
+      enforce_terms
+
+      click_button 'Submit issue'
+
+      expect(current_path).to eq(terms_path)
+
+      click_button('Accept terms')
+
+      expect(current_path).to eq(new_project_issue_path(project))
+      expect(find_field('issue_title').value).to eq('Hello world, a new issue')
+      expect(find_field('issue_description').value).to eq("We don't want to lose what the user typed")
+    end
+  end
 end

spec/policies/global_policy_spec.rb

 require 'spec_helper'
 
 describe GlobalPolicy do
+  include TermsHelper
+
   let(:current_user) { create(:user) }
   let(:user) { create(:user) }
 

spec/support/helpers/terms_helper.rb

+module TermsHelper
+  def enforce_terms
+    stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
+    settings = Gitlab::CurrentSettings.current_application_settings
+    ApplicationSettings::UpdateService.new(
+      settings, nil, terms: 'These are the terms', enforce_terms: true
+    ).execute
+  end
+
+  def accept_terms(user)
+    terms = Gitlab::CurrentSettings.current_application_settings.latest_terms
+    Users::RespondToTermsService.new(user, terms).execute(accepted: true)
+  end
+
+  def expect_to_be_on_terms_page
+    expect(current_path).to eq terms_path
+    expect(page).to have_content('Please accept the Terms of Service before continuing.')
+  end
+end