From ab2127cf39b78b9bf9ce8a30551469670e15a2df Mon Sep 17 00:00:00 2001
From: Doug Stull <dstull@gitlab.com>
Date: Thu, 7 Nov 2024 14:59:25 -0500
Subject: [PATCH] Add custom access denied page for ultimate trials in-app
- more helpful UX for the user.
- only deny if the namespace_id is provided and is not
eligible. Otherwise do not observe access denied as
this trials path can create trials also
Changelog: other
EE: true
---
.rubocop.yml | 1 +
.rubocop_todo/rails/strong_params.yml | 1 -
.../gitlab_subscriptions/trials/index.js | 3 +
.../gitlab_subscriptions/trials/duo_common.rb | 2 +-
.../gitlab_subscriptions/trials_controller.rb | 39 ++++++--
ee/app/models/gitlab_subscriptions/trials.rb | 11 +++
.../gitlab_subscriptions/trials/add_ons.rb | 13 ---
.../trials/base_create_add_on_service.rb | 2 +-
.../trials/create_service.rb | 6 +-
.../trials/access_denied_spec.rb | 11 +++
.../trials/add_ons_spec.rb | 30 ------
.../gitlab_subscriptions/trials_spec.rb | 41 ++++++++
.../trials_controller_spec.rb | 93 ++++++++++---------
13 files changed, 147 insertions(+), 106 deletions(-)
create mode 100644 ee/app/assets/javascripts/pages/gitlab_subscriptions/trials/index.js
delete mode 100644 ee/app/models/gitlab_subscriptions/trials/add_ons.rb
create mode 100644 ee/spec/features/gitlab_subscriptions/trials/access_denied_spec.rb
delete mode 100644 ee/spec/models/gitlab_subscriptions/trials/add_ons_spec.rb
diff --git a/.rubocop.yml b/.rubocop.yml
index 2adbb1a270ce..60d2d8fd37f0 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -556,6 +556,7 @@ Gitlab/RSpec/AvoidSetup:
Include:
- 'ee/spec/features/registrations/saas/**/*'
- 'ee/spec/features/gitlab_subscriptions/trials/creation_*'
+ - 'ee/spec/features/gitlab_subscriptions/trials/access_denied_spec.rb'
- 'ee/spec/features/gitlab_subscriptions/trials/duo_pro/**/*'
- 'ee/spec/features/gitlab_subscriptions/trials/duo_enterprise/**/*'
diff --git a/.rubocop_todo/rails/strong_params.yml b/.rubocop_todo/rails/strong_params.yml
index dec7be0263a3..1ec833b7a503 100644
--- a/.rubocop_todo/rails/strong_params.yml
+++ b/.rubocop_todo/rails/strong_params.yml
@@ -264,7 +264,6 @@ Rails/StrongParams:
- 'ee/app/controllers/ee/sessions_controller.rb'
- 'ee/app/controllers/ee/users_controller.rb'
- 'ee/app/controllers/gitlab_subscriptions/subscriptions_controller.rb'
- - 'ee/app/controllers/gitlab_subscriptions/trials_controller.rb'
- 'ee/app/controllers/groups/analytics/application_controller.rb'
- 'ee/app/controllers/groups/analytics/ci_cd_analytics_controller.rb'
- 'ee/app/controllers/groups/analytics/coverage_reports_controller.rb'
diff --git a/ee/app/assets/javascripts/pages/gitlab_subscriptions/trials/index.js b/ee/app/assets/javascripts/pages/gitlab_subscriptions/trials/index.js
new file mode 100644
index 000000000000..780f47d9f06c
--- /dev/null
+++ b/ee/app/assets/javascripts/pages/gitlab_subscriptions/trials/index.js
@@ -0,0 +1,3 @@
+import initializeGoBack from 'ee/trials/init_go_back';
+
+initializeGoBack();
diff --git a/ee/app/controllers/concerns/gitlab_subscriptions/trials/duo_common.rb b/ee/app/controllers/concerns/gitlab_subscriptions/trials/duo_common.rb
index 0772fdea890d..f4c429d64bf7 100644
--- a/ee/app/controllers/concerns/gitlab_subscriptions/trials/duo_common.rb
+++ b/ee/app/controllers/concerns/gitlab_subscriptions/trials/duo_common.rb
@@ -28,7 +28,7 @@ def check_trial_eligibility!
def eligible_namespaces_exist?
return false if eligible_namespaces.none?
- GitlabSubscriptions::Trials::AddOns.eligible_namespace?(general_params[:namespace_id], eligible_namespaces)
+ GitlabSubscriptions::Trials.eligible_namespace?(general_params[:namespace_id], eligible_namespaces)
end
def namespace
diff --git a/ee/app/controllers/gitlab_subscriptions/trials_controller.rb b/ee/app/controllers/gitlab_subscriptions/trials_controller.rb
index 668c2bad5874..ecfb7f16d545 100644
--- a/ee/app/controllers/gitlab_subscriptions/trials_controller.rb
+++ b/ee/app/controllers/gitlab_subscriptions/trials_controller.rb
@@ -11,17 +11,16 @@ class TrialsController < ApplicationController
layout 'minimal'
skip_before_action :set_confirm_warning
- before_action :check_feature_available!
before_action :authenticate_user!
- # https://gitlab.com/gitlab-org/gitlab/-/issues/500686 will likely move this to being performed
- # during an eligibility check similar to Trials::DuoEnterpriseController.
- before_action :eligible_namespaces
+ before_action :check_feature_available!
+ before_action :check_trial_eligibility!
+ before_action :eligible_namespaces # needed when namespace_id isn't provided or is 0(new group)
feature_category :plan_provisioning
urgency :low
def new
- if params[:step] == GitlabSubscriptions::Trials::CreateService::TRIAL
+ if general_params[:step] == GitlabSubscriptions::Trials::CreateService::TRIAL
render :step_namespace
else
render :step_lead
@@ -30,7 +29,7 @@ def new
def create
@result = GitlabSubscriptions::Trials::CreateService.new(
- step: params[:step], lead_params: lead_params, trial_params: trial_params, user: current_user
+ step: general_params[:step], lead_params: lead_params, trial_params: trial_params, user: current_user
).execute
if @result.success?
@@ -57,12 +56,12 @@ def create
render :step_lead_failed
elsif @result.reason == GitlabSubscriptions::Trials::CreateService::NAMESPACE_CREATE_FAILED
# namespace creation failed
- params[:namespace_id] = @result.payload[:namespace_id]
+ params[:namespace_id] = @result.payload[:namespace_id] # rubocop:disable Rails/StrongParams -- Not working for assignment
render :step_namespace_failed
else
# trial creation failed
- params[:namespace_id] = @result.payload[:namespace_id]
+ params[:namespace_id] = @result.payload[:namespace_id] # rubocop:disable Rails/StrongParams -- Not working for assignment
render :trial_failed
end
@@ -82,11 +81,15 @@ def authenticate_user!
return if current_user
redirect_to(
- new_trial_registration_path(::Onboarding::Status.glm_tracking_params(params)),
+ new_trial_registration_path(::Onboarding::Status.glm_tracking_params(params)), # rubocop:disable Rails/StrongParams -- method performs strong params
alert: I18n.t('devise.failure.unauthenticated')
)
end
+ def general_params
+ params.permit(:step)
+ end
+
def lead_params
params.permit(
*::Onboarding::Status::GLM_PARAMS,
@@ -105,13 +108,29 @@ def eligible_namespaces
end
def discover_group_security_flow?
- %w[discover-group-security discover-project-security].include?(params[:glm_content])
+ %w[discover-group-security discover-project-security].include?(trial_params[:glm_content])
end
def check_feature_available!
render_404 unless ::Gitlab::Saas.feature_available?(:subscriptions_trials)
end
+ def check_trial_eligibility!
+ return unless should_check_eligibility?
+ return if eligible_for_trial?
+
+ render 'gitlab_subscriptions/trials/duo/access_denied', status: :forbidden
+ end
+
+ def should_check_eligibility?
+ namespace_id = trial_params[:namespace_id]
+ namespace_id.present? && !GitlabSubscriptions::Trials.creating_group_trigger?(namespace_id)
+ end
+
+ def eligible_for_trial?
+ GitlabSubscriptions::Trials.eligible_namespace?(trial_params[:namespace_id], eligible_namespaces)
+ end
+
def success_flash_message(add_on_purchase)
if discover_group_security_flow?
s_("BillingPlans|Congratulations, your free trial is activated.")
diff --git a/ee/app/models/gitlab_subscriptions/trials.rb b/ee/app/models/gitlab_subscriptions/trials.rb
index f58c84ecc71f..704ab487119d 100644
--- a/ee/app/models/gitlab_subscriptions/trials.rb
+++ b/ee/app/models/gitlab_subscriptions/trials.rb
@@ -8,6 +8,17 @@ def self.single_eligible_namespace?(eligible_namespaces)
eligible_namespaces.count == 1
end
+ def self.creating_group_trigger?(namespace_id)
+ # The value of 0 is the option in the select for creating a new group
+ namespace_id.to_s == '0'
+ end
+
+ def self.eligible_namespace?(namespace_id, eligible_namespaces)
+ return true if namespace_id.blank?
+
+ namespace_id.to_i.in?(eligible_namespaces.pluck_primary_key)
+ end
+
def self.namespace_eligible?(namespace)
namespace_plan_eligible?(namespace) && namespace_add_on_eligible?(namespace)
end
diff --git a/ee/app/models/gitlab_subscriptions/trials/add_ons.rb b/ee/app/models/gitlab_subscriptions/trials/add_ons.rb
deleted file mode 100644
index 31ffa079e97a..000000000000
--- a/ee/app/models/gitlab_subscriptions/trials/add_ons.rb
+++ /dev/null
@@ -1,13 +0,0 @@
-# frozen_string_literal: true
-
-module GitlabSubscriptions
- module Trials
- module AddOns
- def self.eligible_namespace?(namespace_id, eligible_namespaces)
- return true if namespace_id.blank?
-
- namespace_id.to_i.in?(eligible_namespaces.pluck_primary_key)
- end
- end
- end
-end
diff --git a/ee/app/services/gitlab_subscriptions/trials/base_create_add_on_service.rb b/ee/app/services/gitlab_subscriptions/trials/base_create_add_on_service.rb
index 4019dfebc8b4..db3eab3a6cea 100644
--- a/ee/app/services/gitlab_subscriptions/trials/base_create_add_on_service.rb
+++ b/ee/app/services/gitlab_subscriptions/trials/base_create_add_on_service.rb
@@ -7,7 +7,7 @@ class BaseCreateAddOnService < ::GitlabSubscriptions::Trials::BaseCreateService
override :execute
def execute
- return not_found unless AddOns.eligible_namespace?(trial_params[:namespace_id], namespaces_eligible_for_trial)
+ return not_found unless Trials.eligible_namespace?(trial_params[:namespace_id], namespaces_eligible_for_trial)
super
end
diff --git a/ee/app/services/gitlab_subscriptions/trials/create_service.rb b/ee/app/services/gitlab_subscriptions/trials/create_service.rb
index 7ffaabd83978..bb7ff914085e 100644
--- a/ee/app/services/gitlab_subscriptions/trials/create_service.rb
+++ b/ee/app/services/gitlab_subscriptions/trials/create_service.rb
@@ -8,10 +8,8 @@ class CreateService < BaseCreateService
private
def trial_flow
- # The value of 0 is the option in the select for creating a new group
- create_new_group_selected = trial_params[:namespace_id] == '0'
-
- if trial_params[:namespace_id].present? && !create_new_group_selected
+ if trial_params[:namespace_id].present? &&
+ !GitlabSubscriptions::Trials.creating_group_trigger?(trial_params[:namespace_id])
existing_namespace_flow
elsif trial_params.key?(:new_group_name)
create_group_flow
diff --git a/ee/spec/features/gitlab_subscriptions/trials/access_denied_spec.rb b/ee/spec/features/gitlab_subscriptions/trials/access_denied_spec.rb
new file mode 100644
index 000000000000..3dfeb489e82a
--- /dev/null
+++ b/ee/spec/features/gitlab_subscriptions/trials/access_denied_spec.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Ultimate trial access denied flow', :saas_trial, :js, feature_category: :acquisition do
+ # rubocop:disable Gitlab/RSpec/AvoidSetup -- reuse of common flow
+ it_behaves_like 'duo access denied flow' do
+ let(:duo_path) { new_trial_path(namespace_id: non_existing_record_id) }
+ end
+ # rubocop:enable Gitlab/RSpec/AvoidSetup
+end
diff --git a/ee/spec/models/gitlab_subscriptions/trials/add_ons_spec.rb b/ee/spec/models/gitlab_subscriptions/trials/add_ons_spec.rb
deleted file mode 100644
index 45c2eb69cba8..000000000000
--- a/ee/spec/models/gitlab_subscriptions/trials/add_ons_spec.rb
+++ /dev/null
@@ -1,30 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe GitlabSubscriptions::Trials::AddOns, feature_category: :subscription_management do
- describe '.eligible_namespace?' do
- context 'when namespace_id is blank' do
- it 'returns true for nil' do
- expect(described_class.eligible_namespace?(nil, [])).to be(true)
- end
-
- it 'returns true for empty string' do
- expect(described_class.eligible_namespace?('', [])).to be(true)
- end
- end
-
- context 'when namespace_id is present' do
- let_it_be(:namespace) { create(:group) }
- let(:eligible_namespaces) { Namespace.id_in(namespace.id) }
-
- it 'returns true for an eligible namespace' do
- expect(described_class.eligible_namespace?(namespace.id.to_s, eligible_namespaces)).to be(true)
- end
-
- it 'returns false for an in-eligible namespace' do
- expect(described_class.eligible_namespace?(non_existing_record_id.to_s, eligible_namespaces)).to be(false)
- end
- end
- end
-end
diff --git a/ee/spec/models/gitlab_subscriptions/trials_spec.rb b/ee/spec/models/gitlab_subscriptions/trials_spec.rb
index 73908fa50480..49c65f7c2044 100644
--- a/ee/spec/models/gitlab_subscriptions/trials_spec.rb
+++ b/ee/spec/models/gitlab_subscriptions/trials_spec.rb
@@ -25,6 +25,47 @@
end
end
+ describe '.eligible_namespace?' do
+ context 'when namespace_id is blank' do
+ it 'returns true for nil' do
+ expect(described_class.eligible_namespace?(nil, [])).to be(true)
+ end
+
+ it 'returns true for empty string' do
+ expect(described_class.eligible_namespace?('', [])).to be(true)
+ end
+ end
+
+ context 'when namespace_id is present' do
+ let_it_be(:namespace) { create(:group) }
+ let(:eligible_namespaces) { Namespace.id_in(namespace.id) }
+
+ it 'returns true for an eligible namespace' do
+ expect(described_class.eligible_namespace?(namespace.id.to_s, eligible_namespaces)).to be(true)
+ end
+
+ it 'returns false for an in-eligible namespace' do
+ expect(described_class.eligible_namespace?(non_existing_record_id.to_s, eligible_namespaces)).to be(false)
+ end
+ end
+ end
+
+ describe '.creating_group_trigger?' do
+ subject { described_class.creating_group_trigger?(namespace_id) }
+
+ where(:namespace_id, :expected_result) do
+ [
+ [0, true],
+ [nil, false],
+ [1, false]
+ ]
+ end
+
+ with_them do
+ it { is_expected.to be(expected_result) }
+ end
+ end
+
describe '.namespace_eligible?', :saas do
subject { described_class.namespace_eligible?(namespace) }
diff --git a/ee/spec/requests/gitlab_subscriptions/trials_controller_spec.rb b/ee/spec/requests/gitlab_subscriptions/trials_controller_spec.rb
index 1db3c729686e..2c6d36febecf 100644
--- a/ee/spec/requests/gitlab_subscriptions/trials_controller_spec.rb
+++ b/ee/spec/requests/gitlab_subscriptions/trials_controller_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe GitlabSubscriptions::TrialsController, feature_category: :plan_provisioning do
+RSpec.describe GitlabSubscriptions::TrialsController, :saas, feature_category: :plan_provisioning do
let_it_be(:user, reload: true) { create(:user) }
let(:glm_params) { { glm_source: '_glm_source_', glm_content: '_glm_content_' } }
let(:subscriptions_trials_enabled) { true }
@@ -11,8 +11,29 @@
stub_saas_features(subscriptions_trials: subscriptions_trials_enabled, marketing_google_tag_manager: false)
end
+ shared_examples 'namespace_id is passed' do
+ context 'when namespace_id is 0' do
+ let(:namespace_id) { { namespace_id: 0 } }
+
+ it { is_expected.to have_gitlab_http_status(:ok) }
+ end
+
+ context 'for an ineligible group due to subscription level' do
+ let(:namespace_id) { { namespace_id: create(:group_with_plan, plan: :ultimate_plan, owners: user) } }
+
+ it { is_expected.to have_gitlab_http_status(:forbidden) }
+ end
+
+ context 'for an ineligible group due to user permissions' do
+ let(:namespace_id) { { namespace_id: create(:group) } }
+
+ it { is_expected.to have_gitlab_http_status(:forbidden) }
+ end
+ end
+
describe 'GET new' do
- let(:base_params) { glm_params }
+ let(:namespace_id) { {} }
+ let(:base_params) { glm_params.merge(namespace_id) }
subject(:get_new) do
get new_trial_path, params: base_params
@@ -30,23 +51,13 @@
it { is_expected.to have_gitlab_http_status(:ok) }
- context 'with eligible_namespaces' do
- context 'and there are no eligible namespaces' do
- it 'is empty' do
- get_new
-
- expect(assigns(:eligible_namespaces)).to be_empty
- end
- end
+ it_behaves_like 'namespace_id is passed'
- context 'and there are eligible namespaces' do
- it 'assigns eligible_namespaces with namespace' do
- namespace = create(:group, owners: user)
-
- get_new
+ context 'when there are no eligible namespaces' do
+ it 'is empty' do
+ get_new
- expect(assigns(:eligible_namespaces)).to match_array([namespace])
- end
+ expect(assigns(:eligible_namespaces)).to be_empty
end
end
@@ -69,13 +80,19 @@
context 'when on the trial step' do
let(:base_params) { { step: 'trial' } }
+ before_all do
+ create(:group, owners: user)
+ end
+
it { is_expected.to render_select_namespace }
end
end
end
describe 'POST create' do
+ let_it_be(:namespace, reload: true) { create(:group_with_plan, plan: :free_plan, owners: user) }
let(:step) { 'lead' }
+ let(:namespace_id) { { namespace_id: namespace.id.to_s } }
let(:lead_params) do
{
company_name: '_company_name_',
@@ -91,10 +108,9 @@
let(:trial_params) do
{
- namespace_id: non_existing_record_id.to_s,
trial_entity: '_trial_entity_',
organization_id: anything
- }.with_indifferent_access
+ }.merge(namespace_id).with_indifferent_access
end
let(:base_params) { lead_params.merge(trial_params).merge(glm_params).merge(step: step) }
@@ -129,6 +145,15 @@
login_as(user)
end
+ it_behaves_like 'namespace_id is passed' do
+ before do
+ allow_next_instance_of(GitlabSubscriptions::Trials::CreateService) do |instance|
+ response = ServiceResponse.error(message: '_message_', payload: { namespace: namespace })
+ allow(instance).to receive(:execute).and_return(response)
+ end
+ end
+ end
+
context 'when user is then banned' do
before do
user.ban!
@@ -142,8 +167,7 @@
end
end
- context 'when successful', :saas do
- let_it_be(:namespace, reload: true) { create(:group_with_plan, plan: :free_plan) }
+ context 'when successful' do
let_it_be(:add_on) { create(:gitlab_subscription_add_on, :duo_enterprise) }
let_it_be(:ultimate_trial_plan) { create(:ultimate_trial_plan) }
@@ -172,7 +196,7 @@
end
context 'when the namespace applying is on the premium plan' do
- let_it_be(:namespace) { create(:group_with_plan, plan: :premium_plan) }
+ let_it_be(:namespace) { create(:group_with_plan, plan: :premium_plan, owners: user) }
let_it_be(:ultimate_trial_paid_customer_plan) { create(:ultimate_trial_paid_customer_plan) }
context 'when add_on_purchase exists' do
@@ -354,31 +378,10 @@ def update_with_applied_trials(namespace)
expect(response).to redirect_to(new_trial_path(payload[:trial_selection_params]))
end
-
- context 'with eligible_namespaces' do
- context 'and there are no eligible namespaces' do
- it 'is empty' do
- post_create
-
- expect(assigns(:eligible_namespaces)).to be_empty
- end
- end
-
- context 'and there are eligible namespaces' do
- it 'assigns eligible_namespaces with namespace' do
- namespace = create(:group, owners: user)
-
- post_create
-
- expect(assigns(:eligible_namespaces)).to match_array([namespace])
- end
- end
- end
end
context 'with namespace creation failure' do
let(:failure_reason) { :namespace_create_failed }
- let(:namespace) { build_stubbed(:namespace) }
let(:payload) { { namespace_id: namespace.id } }
it 'renders the select namespace form again with namespace creation errors only' do
@@ -391,7 +394,6 @@ def update_with_applied_trials(namespace)
context 'with trial failure' do
let(:failure_reason) { :trial_failed }
- let(:namespace) { build_stubbed(:namespace) }
let(:payload) { { namespace_id: namespace.id } }
it 'renders the select namespace form again with trial creation errors only' do
@@ -403,7 +405,6 @@ def update_with_applied_trials(namespace)
context 'with random failure' do
let(:failure_reason) { :random_error }
- let(:namespace) { build_stubbed(:namespace) }
let(:payload) { { namespace_id: namespace.id } }
it { is_expected.to render_select_namespace }
@@ -429,7 +430,7 @@ def expect_create_failure(reason, payload = {})
RSpec::Matchers.define :render_select_namespace do
match do |response|
expect(response).to have_gitlab_http_status(:ok)
- expect(response.body).to include(_('Apply your trial to a new group'))
+ expect(response.body).to include(_('Apply your trial to a new or existing group'))
expect(response.body).to include(_('Activate my trial'))
end
end
--
GitLab