diff --git a/app/helpers/users/callouts_helper.rb b/app/helpers/users/callouts_helper.rb index 2b8368dd29fdfce2f1455e89a1c8370c59a98ca4..7a5dd8be77c2644049ee98cf1a1be1673e095687 100644 --- a/app/helpers/users/callouts_helper.rb +++ b/app/helpers/users/callouts_helper.rb @@ -59,17 +59,10 @@ def show_security_newsletter_user_callout? !user_dismissed?(SECURITY_NEWSLETTER_CALLOUT) end - def web_hook_disabled_dismissed?(project) - return false unless project - - last_failure = Gitlab::Redis::SharedState.with do |redis| - key = "web_hooks:last_failure:project-#{project.id}" - redis.get(key) - end + def web_hook_disabled_dismissed?(object) + return false unless object.class < WebHooks::HasWebHooks - last_failure = DateTime.parse(last_failure) if last_failure - - user_dismissed?(WEB_HOOK_DISABLED, last_failure, project: project) + user_dismissed?(WEB_HOOK_DISABLED, object.last_webhook_failure, object: object) end def show_merge_request_settings_callout?(project) @@ -79,22 +72,26 @@ def show_merge_request_settings_callout?(project) def ultimate_feature_removal_banner_dismissed?(project) return false unless project - user_dismissed?(ULTIMATE_FEATURE_REMOVAL_BANNER, project: project) + user_dismissed?(ULTIMATE_FEATURE_REMOVAL_BANNER, object: project) end private - def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil, project: nil) + def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil, object: nil) return false unless current_user query = { feature_name: feature_name, ignore_dismissal_earlier_than: ignore_dismissal_earlier_than } - if project - current_user.dismissed_callout_for_project?(project: project, **query) + if object + dismissed_callout?(object, query) else current_user.dismissed_callout?(**query) end end + + def dismissed_callout?(object, query) + current_user.dismissed_callout_for_project?(project: object, **query) + end end end diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb index 514db6ba8a2ff2ff19bc643be9cba9e66aa510d2..ad792f761f82e1e9e989035b9a739804f4656732 100644 --- a/app/helpers/web_hooks/web_hooks_helper.rb +++ b/app/helpers/web_hooks/web_hooks_helper.rb @@ -4,19 +4,31 @@ module WebHooks module WebHooksHelper def show_project_hook_failed_callout?(project:) return false if project_hook_page? + + show_hook_failed_callout?(project) + end + + private + + def show_hook_failed_callout?(object) return false unless current_user - return false unless Ability.allowed?(current_user, :read_web_hooks, project) + + return false unless can_access_web_hooks?(object) # Assumes include of Users::CalloutsHelper - return false if web_hook_disabled_dismissed?(project) + return false if web_hook_disabled_dismissed?(object) - project.fetch_web_hook_failure + object.fetch_web_hook_failure end - private - def project_hook_page? current_controller?('projects/hooks') || current_controller?('projects/hook_logs') end + + def can_access_web_hooks?(object) + Ability.allowed?(current_user, :admin_project, object) + end end end + +WebHooks::WebHooksHelper.prepend_mod diff --git a/app/models/concerns/web_hooks/has_web_hooks.rb b/app/models/concerns/web_hooks/has_web_hooks.rb index 161ce106b9b7ce3c4f5b1e811dd9f9c4782a8874..2183cc3c44b0b2c5e4299b4574731bc8004b8976 100644 --- a/app/models/concerns/web_hooks/has_web_hooks.rb +++ b/app/models/concerns/web_hooks/has_web_hooks.rb @@ -2,8 +2,6 @@ module WebHooks module HasWebHooks - extend ActiveSupport::Concern - WEB_HOOK_CACHE_EXPIRY = 1.hour def any_hook_failed? @@ -15,7 +13,7 @@ def web_hook_failure_redis_key end def last_failure_redis_key - "web_hooks:last_failure:project-#{id}" + "web_hooks:last_failure:#{self.class.name.underscore}-#{id}" end def get_web_hook_failure @@ -42,5 +40,13 @@ def cache_web_hook_failure(state = any_hook_failed?) state end end + + def last_webhook_failure + last_failure = Gitlab::Redis::SharedState.with do |redis| + redis.get(last_failure_redis_key) + end + + DateTime.parse(last_failure) if last_failure + end end end diff --git a/app/models/project.rb b/app/models/project.rb index cd35332d7dfaec5c62e0e299d2bd618a930a0f58..f61b20c1cbf0a50e9cbf8ecaa60b6c36a2ab9651 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -19,6 +19,7 @@ class Project < ApplicationRecord include Presentable include HasRepository include HasWiki + include WebHooks::HasWebHooks include CanMoveRepositoryStorage include Routable include GroupDescendant @@ -41,7 +42,6 @@ class Project < ApplicationRecord include BlocksUnsafeSerialization include Subquery include IssueParent - include WebHooks::HasWebHooks extend Gitlab::Cache::RequestCache extend Gitlab::Utils::Override diff --git a/app/models/users/group_callout.rb b/app/models/users/group_callout.rb index 2552407fa4cda8f4d95edb1ac05af12b422b06df..1e2afdc28d516b232fc120d78712b9d65c09fd58 100644 --- a/app/models/users/group_callout.rb +++ b/app/models/users/group_callout.rb @@ -24,7 +24,8 @@ class GroupCallout < ApplicationRecord namespace_storage_limit_banner_error_threshold: 13, # EE-only usage_quota_trial_alert: 14, # EE-only preview_usage_quota_free_plan_alert: 15, # EE-only - enforcement_at_limit_alert: 16 # EE-only + enforcement_at_limit_alert: 16, # EE-only + web_hook_disabled: 17 # EE-only } validates :group, presence: true diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 2cbe9ad83cc560f53770be3162ee677b9f97701c..1d635b45207dfd115490fec74cc2738403160a79 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -531,7 +531,6 @@ class ProjectPolicy < BasePolicy enable :update_runners_registration_token enable :admin_project_google_cloud enable :admin_secure_files - enable :read_web_hooks enable :read_upload enable :destroy_upload enable :admin_incident_management_timeline_event_tag diff --git a/app/views/layouts/group.html.haml b/app/views/layouts/group.html.haml index 95934f43a5185d54934b5ed80ef7f0e86ce64340..28579c7f7eaaecc750276658dbcd51b217219363 100644 --- a/app/views/layouts/group.html.haml +++ b/app/views/layouts/group.html.haml @@ -16,6 +16,7 @@ :plain window.uploads_path = "#{group_uploads_path(@group)}"; += dispensable_render_if_exists "shared/web_hooks/group_web_hook_disabled_alert" = dispensable_render_if_exists "shared/free_user_cap_alert", source: @group = render template: base_layout || "layouts/application" diff --git a/ee/app/helpers/ee/users/callouts_helper.rb b/ee/app/helpers/ee/users/callouts_helper.rb index 677662b38e95d0f0f92e6145186d54a3387698c0..a7ebf8ead8faa82e4dd7d2a864db9ed735f8764a 100644 --- a/ee/app/helpers/ee/users/callouts_helper.rb +++ b/ee/app/helpers/ee/users/callouts_helper.rb @@ -91,6 +91,13 @@ def show_verification_reminder? private + override :dismissed_callout? + def dismissed_callout?(object, query) + return super if object.is_a?(Project) + + current_user.dismissed_callout_for_group?(group: object, **query) + end + def eoa_bronze_plan_end_date Date.parse(EOA_BRONZE_PLAN_END_DATE) end diff --git a/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb new file mode 100644 index 0000000000000000000000000000000000000000..39052b2599c90dd2fc6ed342cc92d7e4a04020ed --- /dev/null +++ b/ee/app/helpers/ee/web_hooks/web_hooks_helper.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module EE + module WebHooks + module WebHooksHelper + def show_group_hook_failed_callout?(group:) + return false if group_hook_page? + + show_hook_failed_callout?(group) + end + + def group_hook_page? + current_controller?('groups/hooks') || current_controller?('groups/hook_logs') + end + + private + + def can_access_web_hooks?(object) + return super if object.is_a?(Project) + + Ability.allowed?(current_user, :admin_group, object) + end + end + end +end diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 85b6e3db55ca80bd624f1d951eb0d3b0f60dbad8..df4c4fbdef27e664b2fa9694296827337fa18345 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -13,6 +13,7 @@ module Group include TokenAuthenticatable include InsightsFeature include HasWiki + include ::WebHooks::HasWebHooks include CanMoveRepositoryStorage include ReactiveCaching @@ -646,6 +647,13 @@ def execute_hooks(data, hooks_scope) end end + override :any_hook_failed? + def any_hook_failed? + # morally `hooks.disabled.exists?`, but since the GroupHook model includes + # WebHooks::Unstoppable, we simply return `false`. + false + end + override :git_transfer_in_progress? def git_transfer_in_progress? reference_counter(type: ::Gitlab::GlRepository::WIKI).value > 0 diff --git a/ee/app/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml b/ee/app/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..f227a2dfbe2b08af835de502ff25f48301b07a5b --- /dev/null +++ b/ee/app/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml @@ -0,0 +1,13 @@ +- return unless show_group_hook_failed_callout?(group: @group) + +- content_for :after_flash_content do + = render Pajamas::AlertComponent.new(variant: :danger, + title: s_('Webhooks|Webhook disabled'), + alert_options: { class: 'gl-my-4 js-web-hook-disabled-callout', + data: { feature_id: Users::CalloutsHelper::WEB_HOOK_DISABLED, dismiss_endpoint: group_callouts_path, group_id: @group.id, defer_links: 'true'} }) do |c| + = c.body do + = s_('Webhooks|A webhook in this group was automatically disabled after being retried multiple times.') + = succeed '.' do + = link_to _('Learn more'), help_page_path('user/project/integrations/webhooks', anchor: 'troubleshoot-webhooks'), target: '_blank', rel: 'noopener noreferrer' + = c.actions do + = link_to s_('Webhooks|Go to webhooks'), group_hooks_path(@group, anchor: 'webhooks-index'), class: 'btn gl-alert-action btn-confirm gl-button' diff --git a/ee/spec/helpers/ee/users/callouts_helper_spec.rb b/ee/spec/helpers/ee/users/callouts_helper_spec.rb index 1a8b373ae6331c2d6de68dbaa4e9bbaaf7778773..63ec275806059647c047a0ec39d8df1744bc675e 100644 --- a/ee/spec/helpers/ee/users/callouts_helper_spec.rb +++ b/ee/spec/helpers/ee/users/callouts_helper_spec.rb @@ -370,4 +370,80 @@ end end end + + describe '#web_hook_disabled_dismissed?' do + let_it_be(:user, refind: true) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + context 'without a project or a group' do + it 'is false' do + expect(helper).not_to be_web_hook_disabled_dismissed(nil) + end + end + + shared_examples 'web_hook_disabled shared examples' do + context 'when the web-hook failure callout has never been dismissed' do + it 'is false' do + expect(helper).not_to be_web_hook_disabled_dismissed(container) + end + end + + context 'when the web-hook failure callout has been dismissed', :freeze_time, :clean_gitlab_redis_shared_state do + before do + create(factory, + feature_name: Users::CalloutsHelper::WEB_HOOK_DISABLED, + user: user, + dismissed_at: 1.week.ago, + container_key => container) + end + + it 'is true' do + expect(helper).to be_web_hook_disabled_dismissed(container) + end + + context 'when there was an older failure' do + before do + Gitlab::Redis::SharedState.with { |r| r.set(key, 1.month.ago.iso8601) } + end + + it 'is true' do + expect(helper).to be_web_hook_disabled_dismissed(container) + end + end + + context 'when there has been a more recent failure' do + before do + Gitlab::Redis::SharedState.with { |r| r.set(key, 1.day.ago.iso8601) } + end + + it 'is false' do + expect(helper).not_to be_web_hook_disabled_dismissed(container) + end + end + end + end + + context 'with a project' do + let_it_be(:project) { create(:project) } + let(:factory) { :project_callout } + let(:container_key) { :project } + let(:container) { project } + let(:key) { "web_hooks:last_failure:project-#{project.id}" } + + include_examples 'web_hook_disabled shared examples' + end + + context 'with a group' do + let_it_be(:group) { create(:group) } + let(:factory) { :group_callout } + let(:container_key) { :group } + let(:container) { group } + let(:key) { "web_hooks:last_failure:group-#{group.id}" } + + include_examples 'web_hook_disabled shared examples' + end + end end diff --git a/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..81b93372785b8608bc027d06cc0c35eb191c1a96 --- /dev/null +++ b/ee/spec/helpers/web_hooks/web_hooks_helper_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WebHooks::WebHooksHelper, :clean_gitlab_redis_shared_state, feature_category: :integrations do + let_it_be_with_reload(:group) { create(:group) } # rubocop: disable RSpec/FactoryBot/AvoidCreate + + let(:current_user) { nil } + let(:callout_dismissed) { false } + let(:viewing_hooks) { true } + let(:a_hook_has_failed) { false } + + before do + allow(helper).to receive(:current_user).and_return(current_user) + allow(helper).to receive(:web_hook_disabled_dismissed?).with(group).and_return(callout_dismissed) + allow(helper).to receive(:current_controller?).with('groups/hooks').and_return(viewing_hooks) + allow(helper).to receive(:current_controller?).with('groups/hook_logs').and_return(viewing_hooks) + allow(group).to receive(:any_hook_failed?).and_return(a_hook_has_failed) + end + + shared_context 'when we are not viewing the group hooks or the logs' do + let(:viewing_hooks) { false } + end + + shared_context 'when the user has permission' do + before do + group.add_owner(current_user) if current_user + end + end + + shared_context 'when a user is logged in' do + let(:current_user) { create(:user) } # rubocop: disable RSpec/FactoryBot/AvoidCreate + end + + shared_context 'when the user dismissed the callout' do + let(:callout_dismissed) { true } + end + + shared_context 'when a hook has failed' do + let(:a_hook_has_failed) { true } + end + + describe '#show_group_hook_failed_callout?' do + context 'when all conditions are met' do + include_context 'when a user is logged in' + include_context 'when the user has permission' + include_context 'when we are not viewing the group hooks or the logs' + include_context 'when a hook has failed' + + it 'is true' do + expect(helper).to be_show_group_hook_failed_callout(group: group) + end + end + + context 'when any one condition is not met' do + contexts = [ + 'when a user is logged in', + 'when the user has permission', + 'when we are not viewing the group hooks or the logs', + 'when a hook has failed' + ] + + contexts.each do |name| + context "namely #{name}" do + contexts.each { |ctx| include_context(ctx) unless ctx == name } + + it 'is false' do + expect(helper).not_to be_show_group_hook_failed_callout(group: group) + end + end + end + end + end +end diff --git a/ee/spec/models/ee/group_spec.rb b/ee/spec/models/ee/group_spec.rb index b0b459914aac6afc91246a5adff9bd38aa0312bf..de967401cb7d02ee5c05f2729556a42b9ec690b5 100644 --- a/ee/spec/models/ee/group_spec.rb +++ b/ee/spec/models/ee/group_spec.rb @@ -1773,6 +1773,14 @@ end end + describe '#any_hook_failed?' do + let_it_be(:group) { create(:group) } + + subject { group.any_hook_failed? } + + it { is_expected.to eq(false) } + end + describe "#execute_hooks" do context "group_webhooks", :request_store do let_it_be(:parent_group) { create(:group) } diff --git a/ee/spec/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml_spec.rb b/ee/spec/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..92fc62dead16ac557e3edbd241a9ba04034f7213 --- /dev/null +++ b/ee/spec/views/shared/web_hooks/_group_web_hook_disabled_alert.html.haml_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'shared/web_hooks/_group_web_hook_disabled_alert', feature_category: :integrations do + let_it_be(:group) { build_stubbed(:group) } + + def after_flash_content + view.content_for(:after_flash_content) + end + + before do + view.assign(group: group) + allow(view).to receive(:show_group_hook_failed_callout?).with(group: group).and_return(show_callout) + end + + context 'when the helper returns true' do + let(:show_callout) { true } + + it 'adds alert to `:after_flash_content`' do + view.render('shared/web_hooks/group_web_hook_disabled_alert') + + expect(after_flash_content).to have_content('Webhook disabled') + end + end + + context 'when helper returns false' do + let(:show_callout) { false } + + it 'does not add alert to `:after_flash_content`' do + view.render('shared/web_hooks/group_web_hook_disabled_alert') + + expect(after_flash_content).to be_nil + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8eabffd78b7129ac723010b13f68b65c40a4d817..6df41c03ab4e2023165df80222ac88d8f642f1c7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -48096,6 +48096,9 @@ msgstr "" msgid "Webhooks|A subgroup is created or removed." msgstr "" +msgid "Webhooks|A webhook in this group was automatically disabled after being retried multiple times." +msgstr "" + msgid "Webhooks|A webhook in this project was automatically disabled after being retried multiple times." msgstr "" diff --git a/spec/models/concerns/web_hooks/has_web_hooks_spec.rb b/spec/models/concerns/web_hooks/has_web_hooks_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..afb2406a96931dee556da839d076e5dc663b524c --- /dev/null +++ b/spec/models/concerns/web_hooks/has_web_hooks_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe WebHooks::HasWebHooks, feature_category: :integrations do + let(:minimal_test_class) do + Class.new do + include WebHooks::HasWebHooks + + def id + 1 + end + end + end + + before do + stub_const('MinimalTestClass', minimal_test_class) + end + + describe '#last_failure_redis_key' do + subject { MinimalTestClass.new.last_failure_redis_key } + + it { is_expected.to eq('web_hooks:last_failure:minimal_test_class-1') } + end + + describe 'last_webhook_failure', :clean_gitlab_redis_shared_state do + subject { MinimalTestClass.new.last_webhook_failure } + + it { is_expected.to eq(nil) } + + context 'when there was an older failure', :clean_gitlab_redis_shared_state do + let(:last_failure_date) { 1.month.ago.iso8601 } + + before do + Gitlab::Redis::SharedState.with { |r| r.set('web_hooks:last_failure:minimal_test_class-1', last_failure_date) } + end + + it { is_expected.to eq(last_failure_date) } + end + end +end diff --git a/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb index cd6eb8c77faf1c25c0a9cd6ef199274bfcfa583f..fa33db66c3bcf0a47c3d37bb4dc7b593e53f6a78 100644 --- a/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb @@ -83,7 +83,7 @@ describe '#fetch_web_hook_failure', :clean_gitlab_redis_shared_state do context 'when a value has not been stored' do - it 'does not call #any_hook_failed?' do + it 'calls #any_hook_failed?' do expect(object.get_web_hook_failure).to be_nil expect(object).to receive(:any_hook_failed?).and_return(true)