diff --git a/app/assets/stylesheets/framework/terms.scss b/app/assets/stylesheets/framework/terms.scss index 0e9bdba034c..c73274540ac 100644 --- a/app/assets/stylesheets/framework/terms.scss +++ b/app/assets/stylesheets/framework/terms.scss @@ -33,5 +33,13 @@ .panel-content { padding: 0 $gl-padding; } + + .footer-block { + margin: 0; + + .btn { + margin-left: 5px; + } + } } } diff --git a/app/controllers/users/terms_controller.rb b/app/controllers/users/terms_controller.rb index 778e388ff5a..32507bdb7b1 100644 --- a/app/controllers/users/terms_controller.rb +++ b/app/controllers/users/terms_controller.rb @@ -2,18 +2,55 @@ module Users class TermsController < ApplicationController before_action :terms - layout 'terms' def index + @redirect = redirect_path + end + + def accept + agreement = Users::RespondToTermsService.new(current_user, viewed_term) + .execute(accepted: true) + + if agreement.persisted? + redirect_to redirect_path + else + flash[:alert] = agreement.errors.full_messages.join(', ') + redirect_to terms_path, redirect: redirect_path + end + end + + def decline + agreement = Users::RespondToTermsService.new(current_user, viewed_term) + .execute(accepted: false) + + if agreement.persisted? + sign_out(current_user) + redirect_to root_path + else + flash[:alert] = agreement.errors.full_messages.join(', ') + redirect_to terms_path, redirect: redirect_path + end end private + def viewed_term + @viewed_term ||= ApplicationSetting::Term.find(params[:id]) + end + def terms - unless @terms = Gitlab::CurrentSettings.current_application_settings.latest_terms - redirect_to request.referer || root_path + unless @term = Gitlab::CurrentSettings.current_application_settings.latest_terms + redirect_to redirect_path end end + + def redirect_path + referer = if request.referer && !request.referer.include?(terms_path) + URI(request.referer).path + end + + params[:redirect] || referer || root_path + end end end diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index 517268175e6..e803cd3a8d8 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -38,13 +38,24 @@ module UsersHelper end def get_current_user_menu_items - items = [:help, :sign_out] + items = [] - if can?(current_user, :read_user, current_user) + items << :sign_out if current_user + + # TODO: Remove these conditions when the permissions are prevented in + # https://gitlab.com/gitlab-org/gitlab-ce/issues/45849 + terms_not_enforced = !Gitlab::CurrentSettings + .current_application_settings + .enforce_terms? + required_terms_accepted = terms_not_enforced || current_user.terms_accepted? + + items << :help if required_terms_accepted + + if can?(current_user, :read_user, current_user) && required_terms_accepted items << :profile end - if can?(current_user, :update_user, current_user) + if can?(current_user, :update_user, current_user) && required_terms_accepted items << :settings end diff --git a/app/models/user.rb b/app/models/user.rb index 4a602ffbb05..a9cfd39f604 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -138,6 +138,8 @@ class User < ActiveRecord::Base has_many :custom_attributes, class_name: 'UserCustomAttribute' has_many :callouts, class_name: 'UserCallout' has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_many :term_agreements + belongs_to :accepted_term, class_name: 'ApplicationSetting::Term' # # Validations @@ -1187,6 +1189,10 @@ class User < ActiveRecord::Base max_member_access_for_group_ids([group_id])[group_id] end + def terms_accepted? + accepted_term_id.present? + end + protected # override, from Devise::Validatable diff --git a/app/policies/application_setting/term_policy.rb b/app/policies/application_setting/term_policy.rb new file mode 100644 index 00000000000..648932ddba9 --- /dev/null +++ b/app/policies/application_setting/term_policy.rb @@ -0,0 +1,30 @@ +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 + agreement&.accepted + end + + rule { logged_in & current_terms }.policy do + enable :accept_terms + enable :decline_terms + end + + rule { terms_accepted }.prevent :accept_terms + + def agreement + strong_memoize(:agreement) do + next nil if @user.nil? || @subject.nil? + + @user.term_agreements.find_by(term: @subject) + end + end + end +end diff --git a/app/services/users/respond_to_terms_service.rb b/app/services/users/respond_to_terms_service.rb new file mode 100644 index 00000000000..06d660186cf --- /dev/null +++ b/app/services/users/respond_to_terms_service.rb @@ -0,0 +1,24 @@ +module Users + class RespondToTermsService + def initialize(user, term) + @user, @term = user, term + end + + def execute(accepted:) + agreement = @user.term_agreements.find_or_initialize_by(term: @term) + agreement.accepted = accepted + + if agreement.save + store_accepted_term(accepted) + end + + agreement + end + + private + + def store_accepted_term(accepted) + @user.update_column(:accepted_term_id, accepted ? @term.id : nil) + end + end +end diff --git a/app/views/layouts/terms.html.haml b/app/views/layouts/terms.html.haml index 09c4567a362..b04dbc595a5 100644 --- a/app/views/layouts/terms.html.haml +++ b/app/views/layouts/terms.html.haml @@ -3,11 +3,18 @@ %html{ lang: I18n.locale, class: page_class } = render "layouts/head" - %body{ class: "#{user_application_theme} #{@body_class}", data: { page: body_data_page } } + %body{ data: { page: body_data_page } } = render 'peek/bar' .layout-page.terms .content-wrapper - %div{ class: "#{(container_class unless @no_container)} #{@content_class}" } + .mobile-overlay + .alert-wrapper + = render "layouts/broadcast" + = render 'layouts/header/read_only_banner' + = yield :flash_message + = render "layouts/flash" + + %div{ class: "#{container_class}" } .content{ id: "content-body" } .panel.panel-default .panel-heading diff --git a/app/views/users/terms/index.html.haml b/app/views/users/terms/index.html.haml index 49fdab84069..614dab57331 100644 --- a/app/views/users/terms/index.html.haml +++ b/app/views/users/terms/index.html.haml @@ -1,2 +1,12 @@ +- redirect_params = { redirect: @redirect } if @redirect .panel-content.rendered-terms - = markdown_field(@terms, :terms) + = markdown_field(@term, :terms) +.row-content-block.footer-block.clearfix + - if can?(current_user, :accept_terms, @term) + .pull-right + = button_to accept_term_path(@term, redirect_params), class: 'btn btn-success prepend-left-8' do + = _('Accept terms') + - if can?(current_user, :decline_terms, @term) + .pull-right + = button_to decline_term_path(@term, redirect_params), class: 'btn btn-default prepend-left-8' do + = _('Decline and sign out') diff --git a/spec/controllers/users/terms_controller_spec.rb b/spec/controllers/users/terms_controller_spec.rb index 74d17748093..50e818a4520 100644 --- a/spec/controllers/users/terms_controller_spec.rb +++ b/spec/controllers/users/terms_controller_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' describe Users::TermsController do let(:user) { create(:user) } + let(:term) { create(:term) } before do sign_in user @@ -15,9 +16,42 @@ describe Users::TermsController do end it 'shows terms when they exist' do - create(:term) + term expect(response).to have_gitlab_http_status(:success) end end + + describe 'POST #accept' do + it 'saves that the user accepted the terms' do + post :accept, id: term.id + + agreement = user.term_agreements.find_by(term: term) + + expect(agreement.accepted).to eq(true) + end + + it 'redirects to a path when specified' do + post :accept, id: term.id, redirect: groups_path + + expect(response).to redirect_to(groups_path) + end + end + + describe 'POST #decline' do + it 'stores that the user declined the terms' do + post :decline, id: term.id + + agreement = user.term_agreements.find_by(term: term) + + expect(agreement.accepted).to eq(false) + end + + it 'signs out the user' do + post :decline, id: term.id + + expect(response).to redirect_to(root_path) + expect(assigns(:current_user)).to be_nil + end + end end diff --git a/spec/factories/term_agreements.rb b/spec/factories/term_agreements.rb new file mode 100644 index 00000000000..557599e663d --- /dev/null +++ b/spec/factories/term_agreements.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :term_agreement do + term + user + end +end diff --git a/spec/features/users/terms_spec.rb b/spec/features/users/terms_spec.rb index 34e759bc56a..a33f0937fab 100644 --- a/spec/features/users/terms_spec.rb +++ b/spec/features/users/terms_spec.rb @@ -5,12 +5,35 @@ describe 'Users > Terms' do let!(:term) { create(:term, terms: 'By accepting, you promise to be nice!') } before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') sign_in(user) - - visit terms_path end it 'shows the terms' do + visit terms_path + expect(page).to have_content('By accepting, you promise to be nice!') end + + context 'declining the terms' do + it 'returns the user to the app' do + visit terms_path + + click_button 'Decline and sign out' + + expect(page).not_to have_content(term.terms) + expect(user.reload.terms_accepted?).to be(false) + end + end + + context 'accepting the terms' do + it 'returns the user to the app' do + visit terms_path + + click_button 'Accept terms' + + expect(page).not_to have_content(term.terms) + expect(user.reload.terms_accepted?).to be(true) + end + end end diff --git a/spec/helpers/users_helper_spec.rb b/spec/helpers/users_helper_spec.rb index 9f67277801f..b18c045848f 100644 --- a/spec/helpers/users_helper_spec.rb +++ b/spec/helpers/users_helper_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' describe UsersHelper do + include TermsHelper + let(:user) { create(:user) } describe '#user_link' do @@ -51,5 +53,15 @@ describe UsersHelper do expect(items).to include(:profile) end + + context 'when terms are enforced' do + before do + enforce_terms + end + + it 'hides the profile and the settings tab' do + expect(items).not_to include(:settings, :profile, :help) + end + end end end diff --git a/spec/policies/application_setting/term_policy_spec.rb b/spec/policies/application_setting/term_policy_spec.rb new file mode 100644 index 00000000000..93b5ebf5f72 --- /dev/null +++ b/spec/policies/application_setting/term_policy_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +describe ApplicationSetting::TermPolicy do + include TermsHelper + + set(:term) { create(:term) } + let(:user) { create(:user) } + + subject(:policy) { described_class.new(user, term) } + + before do + stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false') + end + + it 'has the correct permissions', :aggregate_failures do + is_expected.to be_allowed(:accept_terms) + is_expected.to be_allowed(:decline_terms) + end + + context 'for anonymous users' do + let(:user) { nil } + + it 'has the correct permissions', :aggregate_failures do + is_expected.to be_disallowed(:accept_terms) + is_expected.to be_disallowed(:decline_terms) + end + end + + context 'when the terms are not current' do + before do + create(:term) + end + + it 'has the correct permissions', :aggregate_failures do + is_expected.to be_disallowed(:accept_terms) + is_expected.to be_disallowed(:decline_terms) + end + end + + context 'when the user already accepted the terms' do + before do + accept_terms(user) + end + + it 'has the correct permissions', :aggregate_failures do + is_expected.to be_disallowed(:accept_terms) + is_expected.to be_allowed(:decline_terms) + end + end +end diff --git a/spec/services/users/respond_to_terms_service_spec.rb b/spec/services/users/respond_to_terms_service_spec.rb new file mode 100644 index 00000000000..fb08dd10b87 --- /dev/null +++ b/spec/services/users/respond_to_terms_service_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe Users::RespondToTermsService do + let(:user) { create(:user) } + let(:term) { create(:term) } + + subject(:service) { described_class.new(user, term) } + + describe '#execute' do + it 'creates a new agreement if it did not exist' do + expect { service.execute(accepted: true) } + .to change { user.term_agreements.size }.by(1) + end + + it 'updates an agreement if it existed' do + agreement = create(:term_agreement, user: user, term: term, accepted: true) + + service.execute(accepted: true) + + expect(agreement.reload.accepted).to be_truthy + end + + it 'adds the accepted terms to the user' do + service.execute(accepted: true) + + expect(user.reload.accepted_term).to eq(term) + end + + it 'removes accepted terms when declining' do + user.update!(accepted_term: term) + + service.execute(accepted: false) + + expect(user.reload.accepted_term).to be_nil + end + end +end