Commit 49f74420 authored by GitLab Bot's avatar GitLab Bot

Automatic merge of gitlab-org/gitlab master

parents 99ffa0a0 d77cb89d
<script> <script>
import { GlButton, GlLoadingIcon } from '@gitlab/ui'; import { GlButton, GlLoadingIcon } from '@gitlab/ui';
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import ciIcon from '../../vue_shared/components/ci_icon.vue'; import ciIcon from '../../vue_shared/components/ci_icon.vue';
export default { export default {
...@@ -8,6 +9,7 @@ export default { ...@@ -8,6 +9,7 @@ export default {
GlButton, GlButton,
GlLoadingIcon, GlLoadingIcon,
}, },
mixins: [glFeatureFlagMixin()],
props: { props: {
status: { status: {
type: String, type: String,
...@@ -42,7 +44,7 @@ export default { ...@@ -42,7 +44,7 @@ export default {
</div> </div>
<gl-button <gl-button
v-if="showDisabledButton" v-if="!glFeatures.restructuredMrWidget && showDisabledButton"
category="primary" category="primary"
variant="success" variant="success"
data-testid="disabled-merge-button" data-testid="disabled-merge-button"
......
<script> <script>
import { GlButton } from '@gitlab/ui';
import { s__ } from '~/locale'; import { s__ } from '~/locale';
import notesEventHub from '~/notes/event_hub';
import StatusIcon from '../mr_widget_status_icon.vue'; import StatusIcon from '../mr_widget_status_icon.vue';
export default { export default {
i18n: { i18n: {
pipelineFailed: s__(
'mrWidget|The pipeline for this merge request did not complete. Push a new commit to fix the failure.',
),
approvalNeeded: s__('mrWidget|Merge blocked: this merge request must be approved.'), approvalNeeded: s__('mrWidget|Merge blocked: this merge request must be approved.'),
unresolvedDiscussions: s__('mrWidget|Merge blocked: all threads must be resolved.'), blockingMergeRequests: s__(
'mrWidget|Merge blocked: you can only merge once above items are resolved.',
),
}, },
components: { components: {
StatusIcon, StatusIcon,
GlButton,
}, },
props: { props: {
mr: { mr: {
...@@ -24,22 +20,15 @@ export default { ...@@ -24,22 +20,15 @@ export default {
}, },
computed: { computed: {
failedText() { failedText() {
if (this.mr.isPipelineFailed) { if (this.mr.approvals && !this.mr.isApproved) {
return this.$options.i18n.pipelineFailed;
} else if (this.mr.approvals && !this.mr.isApproved) {
return this.$options.i18n.approvalNeeded; return this.$options.i18n.approvalNeeded;
} else if (this.mr.hasMergeableDiscussionsState) { } else if (this.mr.blockingMergeRequests?.total_count > 0) {
return this.$options.i18n.unresolvedDiscussions; return this.$options.i18n.blockingMergeRequests;
} }
return null; return null;
}, },
}, },
methods: {
jumpToFirstUnresolvedDiscussion() {
notesEventHub.$emit('jumpToFirstUnresolvedDiscussion');
},
},
}; };
</script> </script>
...@@ -48,28 +37,6 @@ export default { ...@@ -48,28 +37,6 @@ export default {
<status-icon status="warning" /> <status-icon status="warning" />
<p class="media-body gl-m-0! gl-font-weight-bold gl-text-black-normal!"> <p class="media-body gl-m-0! gl-font-weight-bold gl-text-black-normal!">
{{ failedText }} {{ failedText }}
<template v-if="failedText == $options.i18n.unresolvedDiscussions">
<gl-button
class="gl-ml-3"
size="small"
variant="confirm"
data-testid="jumpToUnresolved"
@click="jumpToFirstUnresolvedDiscussion"
>
{{ s__('mrWidget|Jump to first unresolved thread') }}
</gl-button>
<gl-button
v-if="mr.createIssueToResolveDiscussionsPath"
:href="mr.createIssueToResolveDiscussionsPath"
class="gl-ml-3"
size="small"
variant="confirm"
category="secondary"
data-testid="resolveIssue"
>
{{ s__('mrWidget|Create issue to resolve all threads') }}
</gl-button>
</template>
</p> </p>
</div> </div>
</template> </template>
<script> <script>
import { GlButton } from '@gitlab/ui'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import statusIcon from '../mr_widget_status_icon.vue'; import statusIcon from '../mr_widget_status_icon.vue';
export default { export default {
name: 'MRWidgetArchived', name: 'MRWidgetArchived',
components: { components: {
GlButton,
statusIcon, statusIcon,
}, },
mixins: [glFeatureFlagMixin()],
}; };
</script> </script>
<template> <template>
<div class="mr-widget-body media"> <div class="mr-widget-body media">
<div class="space-children"> <div class="space-children">
<status-icon status="warning" /> <status-icon status="warning" show-disabled-button />
<gl-button category="secondary" variant="success" :disabled="true">
{{ s__('mrWidget|Merge') }}
</gl-button>
</div> </div>
<div class="media-body"> <div class="media-body">
<span class="bold"> <span :class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }" class="bold">
{{ s__('mrWidget|Merge unavailable: merge requests are read-only on archived projects.') }} {{ s__('mrWidget|Merge unavailable: merge requests are read-only on archived projects.') }}
</span> </span>
</div> </div>
......
...@@ -12,9 +12,11 @@ export default { ...@@ -12,9 +12,11 @@ export default {
</script> </script>
<template> <template>
<div class="mr-widget-body media"> <div class="mr-widget-body media">
<status-icon :show-disabled-button="!glFeatures.restructuredMrWidget" status="loading" /> <status-icon :show-disabled-button="true" status="loading" />
<div class="media-body space-children"> <div class="media-body space-children">
<span class="bold"> {{ s__('mrWidget|Checking if merge request can be merged…') }} </span> <span :class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }" class="bold">
{{ s__('mrWidget|Checking if merge request can be merged…') }}
</span>
</div> </div>
</div> </div>
</template> </template>
...@@ -109,14 +109,18 @@ export default { ...@@ -109,14 +109,18 @@ export default {
</gl-skeleton-loader> </gl-skeleton-loader>
</div> </div>
<div v-else class="media-body space-children gl-display-flex gl-align-items-center"> <div v-else class="media-body space-children gl-display-flex gl-align-items-center">
<span v-if="shouldBeRebased" class="bold"> <span
v-if="shouldBeRebased"
:class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }"
class="bold"
>
{{ {{
s__(`mrWidget|Merge blocked: fast-forward merge is not possible. s__(`mrWidget|Merge blocked: fast-forward merge is not possible.
To merge this request, first rebase locally.`) To merge this request, first rebase locally.`)
}} }}
</span> </span>
<template v-else> <template v-else>
<span class="bold"> <span :class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }" class="bold">
{{ s__('mrWidget|Merge blocked: merge conflicts must be resolved.') }} {{ s__('mrWidget|Merge blocked: merge conflicts must be resolved.') }}
<span v-if="!canMerge"> <span v-if="!canMerge">
{{ {{
...@@ -129,6 +133,7 @@ export default { ...@@ -129,6 +133,7 @@ export default {
<gl-button <gl-button
v-if="showResolveButton" v-if="showResolveButton"
:href="mr.conflictResolutionPath" :href="mr.conflictResolutionPath"
:size="glFeatures.restructuredMrWidget ? 'small' : 'medium'"
data-testid="resolve-conflicts-button" data-testid="resolve-conflicts-button"
> >
{{ s__('mrWidget|Resolve conflicts') }} {{ s__('mrWidget|Resolve conflicts') }}
...@@ -136,6 +141,7 @@ export default { ...@@ -136,6 +141,7 @@ export default {
<gl-button <gl-button
v-if="canMerge" v-if="canMerge"
v-gl-modal-directive="'modal-merge-info'" v-gl-modal-directive="'modal-merge-info'"
:size="glFeatures.restructuredMrWidget ? 'small' : 'medium'"
data-testid="merge-locally-button" data-testid="merge-locally-button"
> >
{{ s__('mrWidget|Merge locally') }} {{ s__('mrWidget|Merge locally') }}
......
...@@ -74,10 +74,21 @@ export default { ...@@ -74,10 +74,21 @@ export default {
<status-icon :show-disabled-button="true" status="warning" /> <status-icon :show-disabled-button="true" status="warning" />
<div class="media-body space-children"> <div class="media-body space-children">
<span class="bold js-branch-text"> <span
:class="{
'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget,
}"
class="bold js-branch-text"
>
<span class="capitalize" data-testid="missingBranchName"> {{ missingBranchName }} </span> <span class="capitalize" data-testid="missingBranchName"> {{ missingBranchName }} </span>
{{ s__('mrWidget|branch does not exist.') }} {{ missingBranchNameMessage }} {{ s__('mrWidget|branch does not exist.') }} {{ missingBranchNameMessage }}
<gl-icon v-gl-tooltip :title="message" :aria-label="message" name="question-o" /> <gl-icon
v-gl-tooltip
:title="message"
:aria-label="message"
name="question-o"
class="gl-text-blue-600 gl-cursor-pointer"
/>
</span> </span>
</div> </div>
</div> </div>
......
<script> <script>
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import StatusIcon from '../mr_widget_status_icon.vue'; import StatusIcon from '../mr_widget_status_icon.vue';
export default { export default {
...@@ -6,13 +7,14 @@ export default { ...@@ -6,13 +7,14 @@ export default {
components: { components: {
StatusIcon, StatusIcon,
}, },
mixins: [glFeatureFlagMixin()],
}; };
</script> </script>
<template> <template>
<div class="mr-widget-body media"> <div class="mr-widget-body media">
<status-icon :show-disabled-button="true" status="warning" /> <status-icon :show-disabled-button="true" status="warning" />
<div class="media-body space-children"> <div class="media-body space-children">
<span class="bold"> <span :class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }" class="bold">
{{ {{
s__( s__(
`mrWidget|Merge blocked: pipeline must succeed. It's waiting for a manual action to continue.`, `mrWidget|Merge blocked: pipeline must succeed. It's waiting for a manual action to continue.`,
......
...@@ -152,12 +152,14 @@ export default { ...@@ -152,12 +152,14 @@ export default {
<div class="rebase-state-find-class-convention media media-body space-children"> <div class="rebase-state-find-class-convention media media-body space-children">
<span <span
v-if="rebaseInProgress || isMakingRequest" v-if="rebaseInProgress || isMakingRequest"
:class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }"
class="gl-font-weight-bold" class="gl-font-weight-bold"
data-testid="rebase-message" data-testid="rebase-message"
>{{ __('Rebase in progress') }}</span >{{ __('Rebase in progress') }}</span
> >
<span <span
v-if="!rebaseInProgress && !canPushToSourceBranch" v-if="!rebaseInProgress && !canPushToSourceBranch"
:class="{ 'gl-text-body!': glFeatures.restructuredMrWidget }"
class="gl-font-weight-bold gl-ml-0!" class="gl-font-weight-bold gl-ml-0!"
data-testid="rebase-message" data-testid="rebase-message"
>{{ fastForwardMergeText }}</span >{{ fastForwardMergeText }}</span
...@@ -167,6 +169,7 @@ export default { ...@@ -167,6 +169,7 @@ export default {
class="accept-merge-holder clearfix js-toggle-container accept-action media space-children" class="accept-merge-holder clearfix js-toggle-container accept-action media space-children"
> >
<gl-button <gl-button
v-if="!glFeatures.restructuredMrWidget"
:loading="isMakingRequest" :loading="isMakingRequest"
variant="confirm" variant="confirm"
data-qa-selector="mr_rebase_button" data-qa-selector="mr_rebase_button"
...@@ -176,6 +179,7 @@ export default { ...@@ -176,6 +179,7 @@ export default {
</gl-button> </gl-button>
<span <span
v-if="!rebasingError" v-if="!rebasingError"
:class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }"
class="gl-font-weight-bold" class="gl-font-weight-bold"
data-testid="rebase-message" data-testid="rebase-message"
data-qa-selector="no_fast_forward_message_content" data-qa-selector="no_fast_forward_message_content"
...@@ -186,6 +190,17 @@ export default { ...@@ -186,6 +190,17 @@ export default {
<span v-else class="gl-font-weight-bold danger" data-testid="rebase-message">{{ <span v-else class="gl-font-weight-bold danger" data-testid="rebase-message">{{
rebasingError rebasingError
}}</span> }}</span>
<gl-button
v-if="glFeatures.restructuredMrWidget"
:loading="isMakingRequest"
variant="confirm"
size="small"
data-qa-selector="mr_rebase_button"
class="gl-ml-3!"
@click="rebase"
>
{{ __('Rebase') }}
</gl-button>
</div> </div>
</div> </div>
</template> </template>
......
<script> <script>
import { GlLink, GlSprintf } from '@gitlab/ui'; import { GlLink, GlSprintf } from '@gitlab/ui';
import { helpPagePath } from '~/helpers/help_page_helper'; import { helpPagePath } from '~/helpers/help_page_helper';
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import { s__ } from '~/locale'; import { s__ } from '~/locale';
import statusIcon from '../mr_widget_status_icon.vue'; import statusIcon from '../mr_widget_status_icon.vue';
...@@ -11,6 +12,7 @@ export default { ...@@ -11,6 +12,7 @@ export default {
GlSprintf, GlSprintf,
statusIcon, statusIcon,
}, },
mixins: [glFeatureFlagMixin()],
computed: { computed: {
troubleshootingDocsPath() { troubleshootingDocsPath() {
return helpPagePath('ci/troubleshooting', { anchor: 'merge-request-status-messages' }); return helpPagePath('ci/troubleshooting', { anchor: 'merge-request-status-messages' });
...@@ -28,7 +30,7 @@ export default { ...@@ -28,7 +30,7 @@ export default {
<div class="mr-widget-body media"> <div class="mr-widget-body media">
<status-icon :show-disabled-button="true" status="warning" /> <status-icon :show-disabled-button="true" status="warning" />
<div class="media-body space-children"> <div class="media-body space-children">
<span class="bold"> <span :class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }" class="bold">
<gl-sprintf :message="$options.i18n.failedMessage"> <gl-sprintf :message="$options.i18n.failedMessage">
<template #link="{ content }"> <template #link="{ content }">
<gl-link :href="troubleshootingDocsPath" target="_blank"> <gl-link :href="troubleshootingDocsPath" target="_blank">
......
...@@ -529,7 +529,7 @@ export default { ...@@ -529,7 +529,7 @@ export default {
<template> <template>
<div <div
:class="{ :class="{
'gl-border-t-1 gl-border-t-solid gl-border-gray-100 gl-bg-gray-10 gl-pl-7': 'gl-border-t-1 gl-border-t-solid gl-border-gray-100 gl-bg-gray-10 gl-pl-7 gl-rounded-bottom-left-base gl-rounded-bottom-right-base':
glFeatures.restructuredMrWidget, glFeatures.restructuredMrWidget,
}" }"
> >
......
<script> <script>
import { GlButton } from '@gitlab/ui'; import { GlButton } from '@gitlab/ui';
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import { I18N_SHA_MISMATCH } from '../../i18n'; import { I18N_SHA_MISMATCH } from '../../i18n';
import statusIcon from '../mr_widget_status_icon.vue'; import statusIcon from '../mr_widget_status_icon.vue';
...@@ -12,6 +13,7 @@ export default { ...@@ -12,6 +13,7 @@ export default {
i18n: { i18n: {
I18N_SHA_MISMATCH, I18N_SHA_MISMATCH,
}, },
mixins: [glFeatureFlagMixin()],
props: { props: {
mr: { mr: {
type: Object, type: Object,
...@@ -25,7 +27,11 @@ export default { ...@@ -25,7 +27,11 @@ export default {
<div class="mr-widget-body media"> <div class="mr-widget-body media">
<status-icon :show-disabled-button="false" status="warning" /> <status-icon :show-disabled-button="false" status="warning" />
<div class="media-body"> <div class="media-body">
<span class="gl-font-weight-bold" data-qa-selector="head_mismatch_content"> <span
:class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }"
class="gl-font-weight-bold"
data-qa-selector="head_mismatch_content"
>
{{ $options.i18n.I18N_SHA_MISMATCH.warningMessage }} {{ $options.i18n.I18N_SHA_MISMATCH.warningMessage }}
</span> </span>
<gl-button <gl-button
......
<script> <script>
import { GlButton } from '@gitlab/ui'; import { GlButton } from '@gitlab/ui';
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import notesEventHub from '~/notes/event_hub'; import notesEventHub from '~/notes/event_hub';
import statusIcon from '../mr_widget_status_icon.vue'; import statusIcon from '../mr_widget_status_icon.vue';
...@@ -9,6 +10,7 @@ export default { ...@@ -9,6 +10,7 @@ export default {
statusIcon, statusIcon,
GlButton, GlButton,
}, },
mixins: [glFeatureFlagMixin()],
props: { props: {
mr: { mr: {
type: Object, type: Object,
...@@ -25,16 +27,24 @@ export default { ...@@ -25,16 +27,24 @@ export default {
<template> <template>
<div class="mr-widget-body media gl-flex-wrap"> <div class="mr-widget-body media gl-flex-wrap">
<status-icon :show-disabled-button="true" status="warning" /> <status-icon show-disabled-button status="warning" />
<div class="media-body"> <div class="media-body">
<span class="gl-ml-3 gl-font-weight-bold gl-display-block gl-w-100">{{ <span
s__('mrWidget|Merge blocked: all threads must be resolved.') :class="{
}}</span> 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget,
'gl-display-block': !glFeatures.restructuredMrWidget,
}"
class="gl-ml-3 gl-font-weight-bold gl-w-100"
>
{{ s__('mrWidget|Merge blocked: all threads must be resolved.') }}
</span>
<gl-button <gl-button
data-testid="jump-to-first" data-testid="jump-to-first"
class="gl-ml-3" class="gl-ml-3"
size="small" size="small"
icon="comment-next" :icon="glFeatures.restructuredMrWidget ? undefined : 'comment-next'"
:variant="glFeatures.restructuredMrWidget && 'confirm'"
:category="glFeatures.restructuredMrWidget && 'secondary'"
@click="jumpToFirstUnresolvedDiscussion" @click="jumpToFirstUnresolvedDiscussion"
> >
{{ s__('mrWidget|Jump to first unresolved thread') }} {{ s__('mrWidget|Jump to first unresolved thread') }}
...@@ -44,7 +54,7 @@ export default { ...@@ -44,7 +54,7 @@ export default {
:href="mr.createIssueToResolveDiscussionsPath" :href="mr.createIssueToResolveDiscussionsPath"
class="js-create-issue gl-ml-3" class="js-create-issue gl-ml-3"
size="small" size="small"
icon="issue-new" :icon="glFeatures.restructuredMrWidget ? undefined : 'issue-new'"
> >
{{ s__('mrWidget|Create issue to resolve all threads') }} {{ s__('mrWidget|Create issue to resolve all threads') }}
</gl-button> </gl-button>
......
...@@ -166,7 +166,10 @@ export default { ...@@ -166,7 +166,10 @@ export default {
<status-icon :show-disabled-button="canUpdate" status="warning" /> <status-icon :show-disabled-button="canUpdate" status="warning" />
<div class="media-body"> <div class="media-body">
<div class="float-left"> <div class="float-left">
<span class="gl-font-weight-bold"> <span
:class="{ 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget }"
class="gl-font-weight-bold"
>
{{ {{
__("Merge blocked: merge request must be marked as ready. It's still marked as draft.") __("Merge blocked: merge request must be marked as ready. It's still marked as draft.")
}} }}
......
...@@ -357,15 +357,13 @@ export default class MergeRequestStore { ...@@ -357,15 +357,13 @@ export default class MergeRequestStore {
setApprovals(data) { setApprovals(data) {
this.approvals = data; this.approvals = data;
this.isApproved = data.approved || false; this.isApproved = data.approved || false;
this.setState();
} }
// eslint-disable-next-line class-methods-use-this
get hasMergeChecksFailed() { get hasMergeChecksFailed() {
if (!window.gon?.features?.restructuredMrWidget) return false; return false;
return (
this.hasMergeableDiscussionsState ||
(this.onlyAllowMergeIfPipelineSucceeds && this.isPipelineFailed)
);
} }
// Because the state machine doesn't yet handle every state and transition, // Because the state machine doesn't yet handle every state and transition,
......
...@@ -41,6 +41,11 @@ class ProjectHook < WebHook ...@@ -41,6 +41,11 @@ class ProjectHook < WebHook
super.merge(project: project) super.merge(project: project)
end end
override :parent
def parent
project
end
private private
override :web_hooks_disable_failed? override :web_hooks_disable_failed?
......
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
class ServiceHook < WebHook class ServiceHook < WebHook
include Presentable include Presentable
extend ::Gitlab::Utils::Override
belongs_to :integration, foreign_key: :service_id belongs_to :integration, foreign_key: :service_id
validates :integration, presence: true validates :integration, presence: true
...@@ -9,4 +10,7 @@ class ServiceHook < WebHook ...@@ -9,4 +10,7 @@ class ServiceHook < WebHook
def execute(data, hook_name = 'service_hook') def execute(data, hook_name = 'service_hook')
super(data, hook_name) super(data, hook_name)
end end
override :parent
delegate :parent, to: :integration
end end
...@@ -122,6 +122,11 @@ class WebHook < ApplicationRecord ...@@ -122,6 +122,11 @@ class WebHook < ApplicationRecord
nil nil
end end
# Returns the associated Project or Group for the WebHook if one exists.
# Overridden by inheriting classes.
def parent
end
# Custom attributes to be included in the worker context. # Custom attributes to be included in the worker context.
def application_context def application_context
{ related_class: type } { related_class: type }
......
...@@ -26,7 +26,6 @@ class WebHookService ...@@ -26,7 +26,6 @@ class WebHookService
end end
REQUEST_BODY_SIZE_LIMIT = 25.megabytes REQUEST_BODY_SIZE_LIMIT = 25.megabytes
GITLAB_EVENT_HEADER = 'X-Gitlab-Event'
attr_accessor :hook, :data, :hook_name, :request_options attr_accessor :hook, :data, :hook_name, :request_options
attr_reader :uniqueness_token attr_reader :uniqueness_token
...@@ -50,6 +49,10 @@ class WebHookService ...@@ -50,6 +49,10 @@ class WebHookService
def execute def execute
return { status: :error, message: 'Hook disabled' } unless hook.executable? return { status: :error, message: 'Hook disabled' } unless hook.executable?
log_recursion_limit if recursion_blocked?
Gitlab::WebHooks::RecursionDetection.register!(hook)
start_time = Gitlab::Metrics::System.monotonic_time start_time = Gitlab::Metrics::System.monotonic_time
response = if parsed_url.userinfo.blank? response = if parsed_url.userinfo.blank?
...@@ -95,6 +98,10 @@ class WebHookService ...@@ -95,6 +98,10 @@ class WebHookService
Gitlab::ApplicationContext.with_context(hook.application_context) do Gitlab::ApplicationContext.with_context(hook.application_context) do
break log_rate_limit if rate_limited? break log_rate_limit if rate_limited?
log_recursion_limit if recursion_blocked?
data[:_gitlab_recursion_detection_request_uuid] = Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid
WebHookWorker.perform_async(hook.id, data, hook_name) WebHookWorker.perform_async(hook.id, data, hook_name)
end end
end end
...@@ -108,7 +115,7 @@ class WebHookService ...@@ -108,7 +115,7 @@ class WebHookService
def make_request(url, basic_auth = false) def make_request(url, basic_auth = false)
Gitlab::HTTP.post(url, Gitlab::HTTP.post(url,
body: Gitlab::Json::LimitedEncoder.encode(data, limit: REQUEST_BODY_SIZE_LIMIT), body: Gitlab::Json::LimitedEncoder.encode(data, limit: REQUEST_BODY_SIZE_LIMIT),
headers: build_headers(hook_name), headers: build_headers,
verify: hook.enable_ssl_verification, verify: hook.enable_ssl_verification,
basic_auth: basic_auth, basic_auth: basic_auth,
**request_options) **request_options)
...@@ -129,7 +136,7 @@ class WebHookService ...@@ -129,7 +136,7 @@ class WebHookService
trigger: trigger, trigger: trigger,
url: url, url: url,
execution_duration: execution_duration, execution_duration: execution_duration,
request_headers: build_headers(hook_name), request_headers: build_headers,
request_data: request_data, request_data: request_data,
response_headers: format_response_headers(response), response_headers: format_response_headers(response),
response_body: safe_response_body(response), response_body: safe_response_body(response),
...@@ -151,15 +158,16 @@ class WebHookService ...@@ -151,15 +158,16 @@ class WebHookService
end end
end end
def build_headers(hook_name) def build_headers
@headers ||= begin @headers ||= begin
{ headers = {
'Content-Type' => 'application/json', 'Content-Type' => 'application/json',
'User-Agent' => "GitLab/#{Gitlab::VERSION}", 'User-Agent' => "GitLab/#{Gitlab::VERSION}",
GITLAB_EVENT_HEADER => self.class.hook_to_event(hook_name) Gitlab::WebHooks::GITLAB_EVENT_HEADER => self.class.hook_to_event(hook_name)
}.tap do |hash| }
hash['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present?
end headers['X-Gitlab-Token'] = Gitlab::Utils.remove_line_breaks(hook.token) if hook.token.present?
headers.merge!(Gitlab::WebHooks::RecursionDetection.header(hook))
end end
end end
...@@ -186,6 +194,10 @@ class WebHookService ...@@ -186,6 +194,10 @@ class WebHookService
) )
end end
def recursion_blocked?
Gitlab::WebHooks::RecursionDetection.block?(hook)
end
def rate_limit def rate_limit
@rate_limit ||= hook.rate_limit @rate_limit ||= hook.rate_limit
end end
...@@ -199,4 +211,15 @@ class WebHookService ...@@ -199,4 +211,15 @@ class WebHookService
**Gitlab::ApplicationContext.current **Gitlab::ApplicationContext.current
) )
end end
def log_recursion_limit
Gitlab::AuthLogger.error(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: hook.id,
hook_type: hook.type,
hook_name: hook_name,
recursion_detection: ::Gitlab::WebHooks::RecursionDetection.to_log(hook),
**Gitlab::ApplicationContext.current
)
end
end end
...@@ -13,11 +13,21 @@ class WebHookWorker ...@@ -13,11 +13,21 @@ class WebHookWorker
worker_has_external_dependencies! worker_has_external_dependencies!
def perform(hook_id, data, hook_name) # Webhook recursion detection properties are passed through the `data` arg.
# This will be migrated to the `params` arg over the next few releases.
# See https://gitlab.com/gitlab-org/gitlab/-/issues/347389.
def perform(hook_id, data, hook_name, params = {})
hook = WebHook.find_by_id(hook_id) hook = WebHook.find_by_id(hook_id)
return unless hook return unless hook
data = data.with_indifferent_access data = data.with_indifferent_access
# Before executing the hook, reapply any recursion detection UUID that was
# initially present in the request header so the hook can pass this same header
# value in its request.
recursion_detection_uuid = data.delete(:_gitlab_recursion_detection_request_uuid)
Gitlab::WebHooks::RecursionDetection.set_request_uuid(recursion_detection_uuid)
WebHookService.new(hook, data, hook_name, jid).execute WebHookService.new(hook, data, hook_name, jid).execute
end end
end end
......
---
name: webhook_recursion_detection
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75821
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/349845
milestone: '14.7'
type: development
group: group::integrations
default_enabled: false
# frozen_string_literal: true
Rails.application.configure do |config|
config.middleware.insert_after RequestStore::Middleware, Gitlab::Middleware::WebhookRecursionDetection
end
...@@ -184,8 +184,8 @@ Each of the approaches we list can or does overwrite data in the target director ...@@ -184,8 +184,8 @@ Each of the approaches we list can or does overwrite data in the target director
### Recommended approach in all cases ### Recommended approach in all cases
The GitLab [backup and restore capability](../../raketasks/backup_restore.md) should be used. Git For either Gitaly or Gitaly Cluster targets, the GitLab [backup and restore capability](../../raketasks/backup_restore.md)
repositories are accessed, managed, and stored on GitLab servers by Gitaly as a database. Data loss should be used. Git repositories are accessed, managed, and stored on GitLab servers by Gitaly as a database. Data loss
can result from directly accessing and copying Gitaly's files using tools like `rsync`. can result from directly accessing and copying Gitaly's files using tools like `rsync`.
- From GitLab 13.3, backup performance can be improved by - From GitLab 13.3, backup performance can be improved by
...@@ -193,13 +193,15 @@ can result from directly accessing and copying Gitaly's files using tools like ` ...@@ -193,13 +193,15 @@ can result from directly accessing and copying Gitaly's files using tools like `
- Backups can be created of just the repositories using the - Backups can be created of just the repositories using the
[skip feature](../../raketasks/backup_restore.md#excluding-specific-directories-from-the-backup). [skip feature](../../raketasks/backup_restore.md#excluding-specific-directories-from-the-backup).
No other method works for Gitaly Cluster targets.
### Target directory is empty: use a `tar` pipe ### Target directory is empty: use a `tar` pipe
If the target directory `/mnt/gitlab/repositories` is empty the For Gitaly targets (use [recommended approach](#recommended-approach-in-all-cases) for Gitaly Cluster targets), if the
simplest thing to do is to use a `tar` pipe. This method has low target directory `/mnt/gitlab/repositories` is empty the simplest thing to do is to use a `tar` pipe. This method has
overhead and `tar` is almost always already installed on your system. low overhead and `tar` is almost always already installed on your system.
However, it is not possible to resume an interrupted `tar` pipe: if
that happens then all data must be copied again. However, it is not possible to resume an interrupted `tar` pipe; if that happens then all data must be copied again.
```shell ```shell
sudo -u git sh -c 'tar -C /var/opt/gitlab/git-data/repositories -cf - -- . |\ sudo -u git sh -c 'tar -C /var/opt/gitlab/git-data/repositories -cf - -- . |\
...@@ -210,9 +212,9 @@ If you want to see progress, replace `-xf` with `-xvf`. ...@@ -210,9 +212,9 @@ If you want to see progress, replace `-xf` with `-xvf`.
#### `tar` pipe to another server #### `tar` pipe to another server
You can also use a `tar` pipe to copy data to another server. If your For Gitaly targets (use [recommended approach](#recommended-approach-in-all-cases) for Gitaly Cluster targets), you can
`git` user has SSH access to the new server as `git@newserver`, you also use a `tar` pipe to copy data to another server. If your `git` user has SSH access to the new server as
can pipe the data through SSH. `git@newserver`, you can pipe the data through SSH.
```shell ```shell
sudo -u git sh -c 'tar -C /var/opt/gitlab/git-data/repositories -cf - -- . |\ sudo -u git sh -c 'tar -C /var/opt/gitlab/git-data/repositories -cf - -- . |\
...@@ -228,11 +230,11 @@ WARNING: ...@@ -228,11 +230,11 @@ WARNING:
Using `rsync` to migrate Git data can cause data loss and repository corruption. Using `rsync` to migrate Git data can cause data loss and repository corruption.
[These instructions are being reviewed](https://gitlab.com/gitlab-org/gitlab/-/issues/270422). [These instructions are being reviewed](https://gitlab.com/gitlab-org/gitlab/-/issues/270422).
If the target directory already contains a partial / outdated copy If the target directory already contains a partial or outdated copy of the repositories it may be wasteful to copy all
of the repositories it may be wasteful to copy all the data again the data again with `tar`. In this scenario it is better to use `rsync` for Gitaly targets (use
with `tar`. In this scenario it is better to use `rsync`. This utility [recommended approach](#recommended-approach-in-all-cases) for Gitaly Cluster targets).
is either already installed on your system, or installable
by using `apt` or `yum`. This utility is either already installed on your system, or installable using `apt` or `yum`.
```shell ```shell
sudo -u git sh -c 'rsync -a --delete /var/opt/gitlab/git-data/repositories/. \ sudo -u git sh -c 'rsync -a --delete /var/opt/gitlab/git-data/repositories/. \
...@@ -249,8 +251,9 @@ WARNING: ...@@ -249,8 +251,9 @@ WARNING:
Using `rsync` to migrate Git data can cause data loss and repository corruption. Using `rsync` to migrate Git data can cause data loss and repository corruption.
[These instructions are being reviewed](https://gitlab.com/gitlab-org/gitlab/-/issues/270422). [These instructions are being reviewed](https://gitlab.com/gitlab-org/gitlab/-/issues/270422).
If the `git` user on your source system has SSH access to the target For Gitaly targets (use [recommended approach](#recommended-approach-in-all-cases) for Gitaly Cluster targets), if the
server you can send the repositories over the network with `rsync`. `git` user on your source system has SSH access to the target server you can send the repositories over the network with
`rsync`.
```shell ```shell
sudo -u git sh -c 'rsync -a --delete /var/opt/gitlab/git-data/repositories/. \ sudo -u git sh -c 'rsync -a --delete /var/opt/gitlab/git-data/repositories/. \
...@@ -269,17 +272,18 @@ Every time you start an `rsync` job it must: ...@@ -269,17 +272,18 @@ Every time you start an `rsync` job it must:
- Inspect all files in the target directory. - Inspect all files in the target directory.
- Decide whether or not to copy files. - Decide whether or not to copy files.
If the source or target directory If the source or target directory has many contents, this startup phase of `rsync` can become a burden for your GitLab
has many contents, this startup phase of `rsync` can become a burden server. You can reduce the workload of `rsync` by dividing its work into smaller pieces, and sync one repository at a
for your GitLab server. You can reduce the workload of `rsync` by dividing its time.
work in smaller pieces, and sync one repository at a time.
In addition to `rsync` we use [GNU Parallel](http://www.gnu.org/software/parallel/). In addition to `rsync` we use [GNU Parallel](http://www.gnu.org/software/parallel/).
This utility is not included in GitLab, so you must install it yourself with `apt` This utility is not included in GitLab, so you must install it yourself with `apt`
or `yum`. or `yum`.
This process does not clean up repositories at the target location that no This process:
longer exist at the source.
- Doesn't clean up repositories at the target location that no longer exist at the source.
- Only works for Gitaly targets. Use [recommended approach](#recommended-approach-in-all-cases) for Gitaly Cluster targets.
#### Parallel `rsync` for all repositories known to GitLab #### Parallel `rsync` for all repositories known to GitLab
......
...@@ -638,7 +638,7 @@ These CI/CD variables are specific to DAST. They can be used to customize the be ...@@ -638,7 +638,7 @@ These CI/CD variables are specific to DAST. They can be used to customize the be
| `DAST_XML_REPORT` | string | The filename of the XML report written at the end of a scan. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/12652) in GitLab 13.1. | | `DAST_XML_REPORT` | string | The filename of the XML report written at the end of a scan. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/12652) in GitLab 13.1. |
| `DAST_WEBSITE` <sup>1</sup> | URL | The URL of the website to scan. The variable `DAST_API_OPENAPI` must be specified if this is omitted. | | `DAST_WEBSITE` <sup>1</sup> | URL | The URL of the website to scan. The variable `DAST_API_OPENAPI` must be specified if this is omitted. |
| `DAST_ZAP_CLI_OPTIONS` | string | ZAP server command-line options. For example, `-Xmx3072m` would set the Java maximum memory allocation pool size. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/12652) in GitLab 13.1. | | `DAST_ZAP_CLI_OPTIONS` | string | ZAP server command-line options. For example, `-Xmx3072m` would set the Java maximum memory allocation pool size. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/12652) in GitLab 13.1. |
| `DAST_ZAP_LOG_CONFIGURATION` | string | Set to a semicolon-separated list of additional log4j properties for the ZAP Server. Example: `log4j.logger.org.parosproxy.paros.network.HttpSender=DEBUG;log4j.logger.com.crawljax=DEBUG` | | `DAST_ZAP_LOG_CONFIGURATION` | string | Set to a semicolon-separated list of additional log4j properties for the ZAP Server. Example: `logger.httpsender.name=org.parosproxy.paros.network.HttpSender;logger.httpsender.level=debug;logger.sitemap.name=org.parosproxy.paros.model.SiteMap;logger.sitemap.level=debug;` |
| `SECURE_ANALYZERS_PREFIX` | URL | Set the Docker registry base address from which to download the analyzer. | | `SECURE_ANALYZERS_PREFIX` | URL | Set the Docker registry base address from which to download the analyzer. |
1. Available to an on-demand DAST scan. 1. Available to an on-demand DAST scan.
......
...@@ -211,7 +211,7 @@ Instead of adding users one by one, you can [share a project with an entire grou ...@@ -211,7 +211,7 @@ Instead of adding users one by one, you can [share a project with an entire grou
> - Enabled on GitLab.com. > - Enabled on GitLab.com.
> - Recommended for production use. > - Recommended for production use.
> - Replaces the existing form with buttons to open a modal window. > - Replaces the existing form with buttons to open a modal window.
> - To use in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-modal-window). **(FREE SELF)** > - To use in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-modal-window).
WARNING: WARNING:
This feature might not be available to you. Check the **version history** note above for details. This feature might not be available to you. Check the **version history** note above for details.
......
...@@ -52,7 +52,7 @@ Administrators can share projects with any group in the system. ...@@ -52,7 +52,7 @@ Administrators can share projects with any group in the system.
> - Enabled on GitLab.com. > - Enabled on GitLab.com.
> - Recommended for production use. > - Recommended for production use.
> - Replaces the existing form with buttons to open a modal window. > - Replaces the existing form with buttons to open a modal window.
> - To use in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-modal-window). **(FREE SELF)** > - To use in GitLab self-managed instances, ask a GitLab administrator to [enable it](#enable-or-disable-modal-window).
WARNING: WARNING:
This feature might not be available to you. Check the **version history** note above for details. This feature might not be available to you. Check the **version history** note above for details.
......
<script> <script>
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import StatusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue'; import StatusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue';
export default { export default {
...@@ -6,17 +7,23 @@ export default { ...@@ -6,17 +7,23 @@ export default {
components: { components: {
StatusIcon, StatusIcon,
}, },
mixins: [glFeatureFlagMixin()],
}; };
</script> </script>
<template> <template>
<div class="mr-widget-body media"> <div class="mr-widget-body media">
<status-icon :show-disabled-button="true" status="warning" /> <status-icon show-disabled-button status="warning" />
<div class="media-body"> <div class="media-body">
<strong class="gl-font-weight-bold gl-text-gray-700"> <span
:class="{
'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget,
}"
class="gl-font-weight-bold gl-text-gray-700"
>
{{ {{
s__('mrWidget|To merge, a Jira issue key must be mentioned in the title or description.') s__('mrWidget|To merge, a Jira issue key must be mentioned in the title or description.')
}} }}
</strong> </span>
</div> </div>
</div> </div>
</template> </template>
<script> <script>
import { GlButton } from '@gitlab/ui'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import StatusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue'; import StatusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue';
export default { export default {
name: 'MRWidgetPolicyViolation', name: 'MRWidgetPolicyViolation',
components: { components: {
GlButton,
StatusIcon, StatusIcon,
}, },
mixins: [glFeatureFlagMixin()],
}; };
</script> </script>
<template> <template>
<div class="mr-widget-body media"> <div class="mr-widget-body media">
<div class="space-children gl-display-flex"> <div class="space-children gl-display-flex">
<status-icon status="warning" /> <status-icon status="warning" show-disabled-button />
<gl-button category="primary" variant="success" disabled size="small">
{{ s__('mrWidget|Merge') }}
</gl-button>
</div> </div>
<div class="media-body"> <div class="media-body">
<strong class="gl-font-weight-bold gl-text-gray-700 gl-pl-2"> <span
:class="{
'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget,
}"
class="gl-font-weight-bold gl-text-gray-700 gl-pl-2"
>
{{ s__('mrWidget|Merge blocked: denied licenses must be removed.') }} {{ s__('mrWidget|Merge blocked: denied licenses must be removed.') }}
</strong> </span>
</div> </div>
</div> </div>
</template> </template>
<script> <script>
import { GlIcon, GlTooltipDirective } from '@gitlab/ui'; import { GlIcon, GlTooltipDirective } from '@gitlab/ui';
import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import statusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue'; import statusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue';
export default { export default {
...@@ -10,6 +11,7 @@ export default { ...@@ -10,6 +11,7 @@ export default {
directives: { directives: {
GlTooltip: GlTooltipDirective, GlTooltip: GlTooltipDirective,
}, },
mixins: [glFeatureFlagMixin()],
props: { props: {
mr: { mr: {
type: Object, type: Object,
...@@ -19,12 +21,17 @@ export default { ...@@ -19,12 +21,17 @@ export default {
}; };
</script> </script>
<template> <template>
<div class="media"> <div class="mr-widget-body media gl-flex-wrap">
<status-icon status="warning" show-disabled-button /> <status-icon status="warning" show-disabled-button />
<div class="media-body"> <div class="media-body">
<span class="bold">{{ <span
__('Merge unavailable: merge requests are read-only in a secondary Geo node.') :class="{
}}</span> 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget,
}"
class="bold"
>
{{ __('Merge unavailable: merge requests are read-only in a secondary Geo node.') }}
</span>
<a <a
v-gl-tooltip v-gl-tooltip
:href="mr.geoSecondaryHelpPath" :href="mr.geoSecondaryHelpPath"
......
...@@ -83,6 +83,8 @@ export default class MergeRequestStore extends CEMergeRequestStore { ...@@ -83,6 +83,8 @@ export default class MergeRequestStore extends CEMergeRequestStore {
this.approvals = mapApprovalsResponse(data); this.approvals = mapApprovalsResponse(data);
this.approvalsLeft = Boolean(data.approvals_left); this.approvalsLeft = Boolean(data.approvals_left);
this.preventMerge = !this.isApproved; this.preventMerge = !this.isApproved;
this.setState();
} }
setApprovalRules(data) { setApprovalRules(data) {
...@@ -94,6 +96,8 @@ export default class MergeRequestStore extends CEMergeRequestStore { ...@@ -94,6 +96,8 @@ export default class MergeRequestStore extends CEMergeRequestStore {
if (this.hasApprovalsAvailable && this.approvals && this.approvalsLeft) return !this.isApproved; if (this.hasApprovalsAvailable && this.approvals && this.approvalsLeft) return !this.isApproved;
if (this.blockingMergeRequests.total_count > 0) return true;
return super.hasMergeChecksFailed; return super.hasMergeChecksFailed;
} }
......
...@@ -44,6 +44,11 @@ class GroupHook < WebHook ...@@ -44,6 +44,11 @@ class GroupHook < WebHook
group.actual_limits.limit_for(:web_hook_calls) group.actual_limits.limit_for(:web_hook_calls)
end end
override :parent
def parent
group
end
private private
override :web_hooks_disable_failed? override :web_hooks_disable_failed?
......
import { GlButton } from '@gitlab/ui';
import { shallowMount } from '@vue/test-utils'; import { shallowMount } from '@vue/test-utils';
import StatusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon.vue';
import MrWidgetPolicyViolation from 'ee/vue_merge_request_widget/components/states/mr_widget_policy_violation.vue'; import MrWidgetPolicyViolation from 'ee/vue_merge_request_widget/components/states/mr_widget_policy_violation.vue';
describe('EE MrWidgetPolicyViolation', () => { describe('EE MrWidgetPolicyViolation', () => {
let wrapper; let wrapper;
const findButton = () => wrapper.find(GlButton); const findStatusIcon = () => wrapper.findComponent(StatusIcon);
const createComponent = () => { const createComponent = () => {
wrapper = shallowMount(MrWidgetPolicyViolation, {}); wrapper = shallowMount(MrWidgetPolicyViolation, {});
...@@ -22,7 +22,7 @@ describe('EE MrWidgetPolicyViolation', () => { ...@@ -22,7 +22,7 @@ describe('EE MrWidgetPolicyViolation', () => {
it('shows the disabled merge button', () => { it('shows the disabled merge button', () => {
expect(wrapper.text()).toContain('Merge'); expect(wrapper.text()).toContain('Merge');
expect(findButton().props().disabled).toBe(true); expect(findStatusIcon().props('showDisabledButton')).toBe(true);
}); });
it('shows the disabled reason', () => { it('shows the disabled reason', () => {
......
...@@ -30,6 +30,15 @@ RSpec.describe GroupHook do ...@@ -30,6 +30,15 @@ RSpec.describe GroupHook do
end end
end end
describe '#parent' do
it 'returns the associated group' do
group = build(:group)
hook = build(:group_hook, group: group)
expect(hook.parent).to eq(group)
end
end
describe '#application_context' do describe '#application_context' do
let_it_be(:hook) { build(:group_hook) } let_it_be(:hook) { build(:group_hook) }
......
...@@ -5,7 +5,7 @@ module API ...@@ -5,7 +5,7 @@ module API
class Triggers < ::API::Base class Triggers < ::API::Base
include PaginationParams include PaginationParams
HTTP_GITLAB_EVENT_HEADER = "HTTP_#{WebHookService::GITLAB_EVENT_HEADER}".underscore.upcase HTTP_GITLAB_EVENT_HEADER = "HTTP_#{::Gitlab::WebHooks::GITLAB_EVENT_HEADER}".underscore.upcase
feature_category :continuous_integration feature_category :continuous_integration
......
# frozen_string_literal: true
module Gitlab
module Middleware
class WebhookRecursionDetection
def initialize(app)
@app = app
end
def call(env)
headers = ActionDispatch::Request.new(env).headers
::Gitlab::WebHooks::RecursionDetection.set_from_headers(headers)
@app.call(env)
end
end
end
end
# frozen_string_literal: true
module Gitlab
module WebHooks
GITLAB_EVENT_HEADER = 'X-Gitlab-Event'
end
end
# frozen_string_literal: true
# This module detects and blocks recursive webhook requests.
#
# Recursion can happen when a webhook has been configured to make a call
# to its own GitLab instance (i.e., its API), and during the execution of
# the call the webhook is triggered again to create an infinite loop of
# being triggered.
#
# Additionally the module blocks a webhook once the number of requests to
# the instance made by a series of webhooks triggering other webhooks reaches
# a limit.
#
# Blocking recursive webhooks allows GitLab to continue to support workflows
# that use webhooks to call the API non-recursively, or do not go on to
# trigger an unreasonable number of other webhooks.
module Gitlab
module WebHooks
module RecursionDetection
COUNT_LIMIT = 100
TOUCH_CACHE_TTL = 30.minutes
class << self
def set_from_headers(headers)
uuid = headers[UUID::HEADER]
return unless uuid
set_request_uuid(uuid)
end
def set_request_uuid(uuid)
UUID.instance.request_uuid = uuid
end
# Before a webhook is executed, `.register!` should be called.
# Adds the webhook ID to a cache (see `#cache_key_for_hook` for
# details of the cache).
def register!(hook)
return if disabled?(hook)
cache_key = cache_key_for_hook(hook)
::Gitlab::Redis::SharedState.with do |redis|
redis.multi do
redis.sadd(cache_key, hook.id)
redis.expire(cache_key, TOUCH_CACHE_TTL)
end
end
end
# Returns true if the webhook ID is present in the cache, or if the
# number of IDs in the cache exceeds the limit (see
# `#cache_key_for_hook` for details of the cache).
def block?(hook)
return false if disabled?(hook)
# If a request UUID has not been set then we know the request was not
# made by a webhook, and no recursion is possible.
return false unless UUID.instance.request_uuid
cache_key = cache_key_for_hook(hook)
::Gitlab::Redis::SharedState.with do |redis|
redis.sismember(cache_key, hook.id) ||
redis.scard(cache_key) >= COUNT_LIMIT
end
end
def header(hook)
UUID.instance.header(hook)
end
def to_log(hook)
{
uuid: UUID.instance.uuid_for_hook(hook),
ids: ::Gitlab::Redis::SharedState.with { |redis| redis.smembers(cache_key_for_hook(hook)).map(&:to_i) }
}
end
private
def disabled?(hook)
Feature.disabled?(:webhook_recursion_detection, hook.parent)
end
# Returns a cache key scoped to a UUID.
#
# The particular UUID will be either:
#
# - A UUID that was recycled from the request headers if the request was made by a webhook.
# - a new UUID initialized for the webhook.
#
# This means that cycles of webhooks that are triggered from other webhooks
# will share the same cache, and other webhooks will use a new cache.
def cache_key_for_hook(hook)
[:webhook_recursion_detection, UUID.instance.uuid_for_hook(hook)].join(':')
end
end
end
end
end
# frozen_string_literal: true
module Gitlab
module WebHooks
module RecursionDetection
class UUID
HEADER = "#{::Gitlab::WebHooks::GITLAB_EVENT_HEADER}-UUID"
include Singleton
attr_accessor :request_uuid
def initialize
self.new_uuids_for_hooks = {}
end
class << self
# Back the Singleton with RequestStore so it is isolated to this request.
def instance
Gitlab::SafeRequestStore[:web_hook_recursion_detection_uuid] ||= new
end
end
# Returns a UUID, which will be either:
#
# - The UUID that was recycled from the request headers if the request was made by a webhook.
# - A new UUID initialized for the webhook.
def uuid_for_hook(hook)
request_uuid || new_uuid_for_hook(hook)
end
def header(hook)
{ HEADER => uuid_for_hook(hook) }
end
private
attr_accessor :new_uuids_for_hooks
def new_uuid_for_hook(hook)
new_uuids_for_hooks[hook.id] ||= SecureRandom.uuid
end
end
end
end
end
...@@ -42529,6 +42529,9 @@ msgstr "" ...@@ -42529,6 +42529,9 @@ msgstr ""
msgid "mrWidget|Merge blocked: this merge request must be approved." msgid "mrWidget|Merge blocked: this merge request must be approved."
msgstr "" msgstr ""
msgid "mrWidget|Merge blocked: you can only merge once above items are resolved."
msgstr ""
msgid "mrWidget|Merge failed." msgid "mrWidget|Merge failed."
msgstr "" msgstr ""
...@@ -42637,9 +42640,6 @@ msgstr "" ...@@ -42637,9 +42640,6 @@ msgstr ""
msgid "mrWidget|The pipeline for this merge request did not complete. Push a new commit to fix the failure, or check the %{linkStart}troubleshooting documentation%{linkEnd} to see other possible actions." msgid "mrWidget|The pipeline for this merge request did not complete. Push a new commit to fix the failure, or check the %{linkStart}troubleshooting documentation%{linkEnd} to see other possible actions."
msgstr "" msgstr ""
msgid "mrWidget|The pipeline for this merge request did not complete. Push a new commit to fix the failure."
msgstr ""
msgid "mrWidget|The source branch has been deleted" msgid "mrWidget|The source branch has been deleted"
msgstr "" msgstr ""
......
...@@ -16,34 +16,11 @@ describe('Merge request widget merge checks failed state component', () => { ...@@ -16,34 +16,11 @@ describe('Merge request widget merge checks failed state component', () => {
it.each` it.each`
mrState | displayText mrState | displayText
${{ isPipelineFailed: true }} | ${'pipelineFailed'}
${{ approvals: true, isApproved: false }} | ${'approvalNeeded'} ${{ approvals: true, isApproved: false }} | ${'approvalNeeded'}
${{ hasMergeableDiscussionsState: true }} | ${'unresolvedDiscussions'} ${{ blockingMergeRequests: { total_count: 1 } }} | ${'blockingMergeRequests'}
`('display $displayText text for $mrState', ({ mrState, displayText }) => { `('display $displayText text for $mrState', ({ mrState, displayText }) => {
factory({ mr: mrState }); factory({ mr: mrState });
expect(wrapper.text()).toContain(MergeChecksFailed.i18n[displayText]); expect(wrapper.text()).toContain(MergeChecksFailed.i18n[displayText]);
}); });
describe('unresolved discussions', () => {
it('renders jump to button', () => {
factory({ mr: { hasMergeableDiscussionsState: true } });
expect(wrapper.find('[data-testid="jumpToUnresolved"]').exists()).toBe(true);
});
it('renders resolve thread button', () => {
factory({
mr: {
hasMergeableDiscussionsState: true,
createIssueToResolveDiscussionsPath: 'https://gitlab.com',
},
});
expect(wrapper.find('[data-testid="resolveIssue"]').exists()).toBe(true);
expect(wrapper.find('[data-testid="resolveIssue"]').attributes('href')).toBe(
'https://gitlab.com',
);
});
});
}); });
# frozen_string_literal: true
require 'fast_spec_helper'
require 'action_dispatch'
require 'rack'
require 'request_store'
RSpec.describe Gitlab::Middleware::WebhookRecursionDetection do
let(:app) { double(:app) }
let(:middleware) { described_class.new(app) }
let(:env) { Rack::MockRequest.env_for("/").merge(headers) }
around do |example|
Gitlab::WithRequestStore.with_request_store { example.run }
end
describe '#call' do
subject(:call) { described_class.new(app).call(env) }
context 'when the recursion detection header is present' do
let(:new_uuid) { SecureRandom.uuid }
let(:headers) { { 'HTTP_X_GITLAB_EVENT_UUID' => new_uuid } }
it 'sets the request UUID from the header' do
expect(app).to receive(:call)
expect { call }.to change { Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid }.to(new_uuid)
end
end
context 'when recursion headers are not present' do
let(:headers) { {} }
it 'works without errors' do
expect(app).to receive(:call)
call
expect(Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid).to be_nil
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::WebHooks::RecursionDetection, :clean_gitlab_redis_shared_state, :request_store do
let_it_be(:web_hook) { create(:project_hook) }
let!(:uuid_class) { described_class::UUID }
describe '.set_from_headers' do
let(:old_uuid) { SecureRandom.uuid }
let(:rack_headers) { Rack::MockRequest.env_for("/").merge(headers) }
subject(:set_from_headers) { described_class.set_from_headers(rack_headers) }
# Note, having a previous `request_uuid` value set before `.set_from_headers` is
# called is contrived and should not normally happen. However, testing with this scenario
# allows us to assert the ideal outcome if it ever were to happen.
before do
uuid_class.instance.request_uuid = old_uuid
end
context 'when the detection header is present' do
let(:new_uuid) { SecureRandom.uuid }
let(:headers) do
{ uuid_class::HEADER => new_uuid }
end
it 'sets the request UUID value from the headers' do
set_from_headers
expect(uuid_class.instance.request_uuid).to eq(new_uuid)
end
end
context 'when detection header is not present' do
let(:headers) { {} }
it 'does not set the request UUID' do
set_from_headers
expect(uuid_class.instance.request_uuid).to eq(old_uuid)
end
end
end
describe '.set_request_uuid' do
it 'sets the request UUID value' do
new_uuid = SecureRandom.uuid
described_class.set_request_uuid(new_uuid)
expect(uuid_class.instance.request_uuid).to eq(new_uuid)
end
end
describe '.register!' do
let_it_be(:second_web_hook) { create(:project_hook) }
let_it_be(:third_web_hook) { create(:project_hook) }
def cache_key(hook)
described_class.send(:cache_key_for_hook, hook)
end
it 'stores IDs in the same cache when a request UUID is set, until the request UUID changes', :aggregate_failures do
# Register web_hook and second_web_hook against the same request UUID.
uuid_class.instance.request_uuid = SecureRandom.uuid
described_class.register!(web_hook)
described_class.register!(second_web_hook)
first_cache_key = cache_key(web_hook)
second_cache_key = cache_key(second_web_hook)
# Register third_web_hook against a new request UUID.
uuid_class.instance.request_uuid = SecureRandom.uuid
described_class.register!(third_web_hook)
third_cache_key = cache_key(third_web_hook)
expect(first_cache_key).to eq(second_cache_key)
expect(second_cache_key).not_to eq(third_cache_key)
::Gitlab::Redis::SharedState.with do |redis|
members = redis.smembers(first_cache_key).map(&:to_i)
expect(members).to contain_exactly(web_hook.id, second_web_hook.id)
members = redis.smembers(third_cache_key).map(&:to_i)
expect(members).to contain_exactly(third_web_hook.id)
end
end
it 'stores IDs in unique caches when no request UUID is present', :aggregate_failures do
described_class.register!(web_hook)
described_class.register!(second_web_hook)
described_class.register!(third_web_hook)
first_cache_key = cache_key(web_hook)
second_cache_key = cache_key(second_web_hook)
third_cache_key = cache_key(third_web_hook)
expect([first_cache_key, second_cache_key, third_cache_key].compact.length).to eq(3)
::Gitlab::Redis::SharedState.with do |redis|
members = redis.smembers(first_cache_key).map(&:to_i)
expect(members).to contain_exactly(web_hook.id)
members = redis.smembers(second_cache_key).map(&:to_i)
expect(members).to contain_exactly(second_web_hook.id)
members = redis.smembers(third_cache_key).map(&:to_i)
expect(members).to contain_exactly(third_web_hook.id)
end
end
it 'touches the storage ttl each time it is called', :aggregate_failures do
freeze_time do
described_class.register!(web_hook)
::Gitlab::Redis::SharedState.with do |redis|
expect(redis.ttl(cache_key(web_hook))).to eq(described_class::TOUCH_CACHE_TTL.to_i)
end
end
travel_to(1.minute.from_now) do
described_class.register!(second_web_hook)
::Gitlab::Redis::SharedState.with do |redis|
expect(redis.ttl(cache_key(web_hook))).to eq(described_class::TOUCH_CACHE_TTL.to_i)
end
end
end
it 'does not store anything if the feature flag is disabled' do
stub_feature_flags(webhook_recursion_detection: false)
described_class.register!(web_hook)
::Gitlab::Redis::SharedState.with do |redis|
expect(redis.exists(cache_key(web_hook))).to eq(false)
end
end
end
describe 'block?' do
let_it_be(:registered_web_hooks) { create_list(:project_hook, 2) }
subject(:block?) { described_class.block?(web_hook) }
before do
# Register some previous webhooks.
uuid_class.instance.request_uuid = SecureRandom.uuid
registered_web_hooks.each do |web_hook|
described_class.register!(web_hook)
end
end
it 'returns false if webhook should not be blocked' do
is_expected.to eq(false)
end
context 'when the webhook has previously fired' do
before do
described_class.register!(web_hook)
end
it 'returns true' do
is_expected.to eq(true)
end
it 'returns false if the feature flag is disabled' do
stub_feature_flags(webhook_recursion_detection: false)
is_expected.to eq(false)
end
context 'when the request UUID changes again' do
before do
uuid_class.instance.request_uuid = SecureRandom.uuid
end
it 'returns false' do
is_expected.to eq(false)
end
end
end
context 'when the count limit has been reached' do
let_it_be(:registered_web_hooks) { create_list(:project_hook, 2) }
before do
registered_web_hooks.each do |web_hook|
described_class.register!(web_hook)
end
stub_const("#{described_class.name}::COUNT_LIMIT", registered_web_hooks.size)
end
it 'returns true' do
is_expected.to eq(true)
end
it 'returns false if the feature flag is disabled' do
stub_feature_flags(webhook_recursion_detection: false)
is_expected.to eq(false)
end
context 'when the request UUID changes again' do
before do
uuid_class.instance.request_uuid = SecureRandom.uuid
end
it 'returns false' do
is_expected.to eq(false)
end
end
end
end
describe '.header' do
subject(:header) { described_class.header(web_hook) }
it 'returns a header with the UUID value' do
uuid = SecureRandom.uuid
allow(uuid_class.instance).to receive(:uuid_for_hook).and_return(uuid)
is_expected.to eq({ uuid_class::HEADER => uuid })
end
end
describe '.to_log' do
subject(:to_log) { described_class.to_log(web_hook) }
it 'returns the UUID value and all registered webhook IDs in a Hash' do
uuid = SecureRandom.uuid
allow(uuid_class.instance).to receive(:uuid_for_hook).and_return(uuid)
registered_web_hooks = create_list(:project_hook, 2)
registered_web_hooks.each { described_class.register!(_1) }
is_expected.to eq({ uuid: uuid, ids: registered_web_hooks.map(&:id) })
end
end
end
...@@ -40,6 +40,15 @@ RSpec.describe ProjectHook do ...@@ -40,6 +40,15 @@ RSpec.describe ProjectHook do
end end
end end
describe '#parent' do
it 'returns the associated project' do
project = build(:project)
hook = build(:project_hook, project: project)
expect(hook.parent).to eq(project)
end
end
describe '#application_context' do describe '#application_context' do
let_it_be(:hook) { build(:project_hook) } let_it_be(:hook) { build(:project_hook) }
......
...@@ -31,6 +31,36 @@ RSpec.describe ServiceHook do ...@@ -31,6 +31,36 @@ RSpec.describe ServiceHook do
end end
end end
describe '#parent' do
let(:hook) { build(:service_hook, integration: integration) }
context 'with a project-level integration' do
let(:project) { build(:project) }
let(:integration) { build(:integration, project: project) }
it 'returns the associated project' do
expect(hook.parent).to eq(project)
end
end
context 'with a group-level integration' do
let(:group) { build(:group) }
let(:integration) { build(:integration, :group, group: group) }
it 'returns the associated group' do
expect(hook.parent).to eq(group)
end
end
context 'with an instance-level integration' do
let(:integration) { build(:integration, :instance) }
it 'returns nil' do
expect(hook.parent).to be_nil
end
end
end
describe '#application_context' do describe '#application_context' do
let(:hook) { build(:service_hook) } let(:hook) { build(:service_hook) }
......
...@@ -167,7 +167,7 @@ RSpec.describe Integrations::Datadog do ...@@ -167,7 +167,7 @@ RSpec.describe Integrations::Datadog do
context 'with pipeline data' do context 'with pipeline data' do
let(:data) { pipeline_data } let(:data) { pipeline_data }
let(:expected_headers) { { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' } } let(:expected_headers) { { ::Gitlab::WebHooks::GITLAB_EVENT_HEADER => 'Pipeline Hook' } }
let(:expected_body) { data.with_retried_builds.to_json } let(:expected_body) { data.with_retried_builds.to_json }
it { expect(a_request(:post, expected_hook_url).with(headers: expected_headers, body: expected_body)).to have_been_made } it { expect(a_request(:post, expected_hook_url).with(headers: expected_headers, body: expected_body)).to have_been_made }
...@@ -175,7 +175,7 @@ RSpec.describe Integrations::Datadog do ...@@ -175,7 +175,7 @@ RSpec.describe Integrations::Datadog do
context 'with job data' do context 'with job data' do
let(:data) { build_data } let(:data) { build_data }
let(:expected_headers) { { WebHookService::GITLAB_EVENT_HEADER => 'Job Hook' } } let(:expected_headers) { { ::Gitlab::WebHooks::GITLAB_EVENT_HEADER => 'Job Hook' } }
let(:expected_body) { data.to_json } let(:expected_body) { data.to_json }
it { expect(a_request(:post, expected_hook_url).with(headers: expected_headers, body: expected_body)).to have_been_made } it { expect(a_request(:post, expected_hook_url).with(headers: expected_headers, body: expected_body)).to have_been_made }
......
...@@ -162,7 +162,7 @@ RSpec.describe API::Ci::Triggers do ...@@ -162,7 +162,7 @@ RSpec.describe API::Ci::Triggers do
expect do expect do
post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"), post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"),
params: { ref: 'refs/heads/other-branch' }, params: { ref: 'refs/heads/other-branch' },
headers: { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' } headers: { ::Gitlab::WebHooks::GITLAB_EVENT_HEADER => 'Pipeline Hook' }
end.not_to change(Ci::Pipeline, :count) end.not_to change(Ci::Pipeline, :count)
expect(response).to have_gitlab_http_status(:forbidden) expect(response).to have_gitlab_http_status(:forbidden)
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe 'Recursive webhook detection', :sidekiq_inline, :clean_gitlab_redis_shared_state, :request_store do
include StubRequests
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project, :repository, namespace: user.namespace, creator: user) }
let_it_be(:merge_request) { create(:merge_request, source_project: project) }
let_it_be(:project_hook) { create(:project_hook, project: project, merge_requests_events: true) }
let_it_be(:system_hook) { create(:system_hook, merge_requests_events: true) }
# Trigger a change to the merge request to fire the webhooks.
def trigger_web_hooks
params = { merge_request: { description: FFaker::Lorem.sentence } }
put project_merge_request_path(project, merge_request), params: params, headers: headers
end
def stub_requests
stub_full_request(project_hook.url, method: :post, ip_address: '8.8.8.8')
stub_full_request(system_hook.url, method: :post, ip_address: '8.8.8.9')
end
before do
login_as(user)
end
context 'when the request headers include the recursive webhook detection header' do
let(:uuid) { SecureRandom.uuid }
let(:headers) { { Gitlab::WebHooks::RecursionDetection::UUID::HEADER => uuid } }
it 'executes all webhooks, logs no errors, and the webhook requests contain the same UUID header', :aggregate_failures do
stub_requests
expect(Gitlab::AuthLogger).not_to receive(:error)
trigger_web_hooks
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
.with { |req| req.headers['X-Gitlab-Event-Uuid'] == uuid }
.once
expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url))
.with { |req| req.headers['X-Gitlab-Event-Uuid'] == uuid }
.once
end
shared_examples 'when the feature flag is disabled' do
it 'executes and logs no errors' do
stub_feature_flags(webhook_recursion_detection: false)
stub_requests
expect(Gitlab::AuthLogger).not_to receive(:error)
trigger_web_hooks
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).once
expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).once
end
end
context 'when one of the webhooks is recursive' do
before do
# Recreate the necessary state for the previous request to be
# considered made from the webhook.
Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid)
Gitlab::WebHooks::RecursionDetection.register!(project_hook)
Gitlab::WebHooks::RecursionDetection.set_request_uuid(nil)
end
it 'executes all webhooks and logs an error for the recursive hook', :aggregate_failures do
stub_requests
expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: project_hook.id,
recursion_detection: {
uuid: uuid,
ids: [project_hook.id]
}
)
).twice # Twice: once in `#async_execute`, and again in `#execute`.
trigger_web_hooks
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).once
expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).once
end
include_examples 'when the feature flag is disabled'
end
context 'when the count limit has been reached' do
let_it_be(:previous_hooks) { create_list(:project_hook, 3) }
before do
stub_const('Gitlab::WebHooks::RecursionDetection::COUNT_LIMIT', 2)
# Recreate the necessary state for a number of previous webhooks to
# have been triggered previously.
Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid)
previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) }
Gitlab::WebHooks::RecursionDetection.set_request_uuid(nil)
end
it 'executes and logs errors for all hooks', :aggregate_failures do
stub_requests
previous_hook_ids = previous_hooks.map(&:id)
expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: project_hook.id,
recursion_detection: {
uuid: uuid,
ids: include(*previous_hook_ids)
}
)
).twice
expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: system_hook.id,
recursion_detection: {
uuid: uuid,
ids: include(*previous_hook_ids)
}
)
).twice
trigger_web_hooks
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).once
expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).once
end
end
include_examples 'when the feature flag is disabled'
end
context 'when the recursive webhook detection header is absent' do
let(:headers) { {} }
let(:uuid_header_spy) do
Class.new do
attr_reader :values
def initialize
@values = []
end
def to_proc
proc do |method, *args|
method.call(*args).tap do |headers|
@values << headers[Gitlab::WebHooks::RecursionDetection::UUID::HEADER]
end
end
end
end.new
end
before do
allow(Gitlab::WebHooks::RecursionDetection).to receive(:header).at_least(:once).and_wrap_original(&uuid_header_spy)
end
it 'executes all webhooks, logs no errors, and the webhook requests contain different UUID headers', :aggregate_failures do
stub_requests
expect(Gitlab::AuthLogger).not_to receive(:error)
trigger_web_hooks
uuid_headers = uuid_header_spy.values
expect(uuid_headers).to all(be_present)
expect(uuid_headers.uniq.length).to eq(2)
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
.with { |req| uuid_headers.include?(req.headers['X-Gitlab-Event-Uuid']) }
.once
expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url))
.with { |req| uuid_headers.include?(req.headers['X-Gitlab-Event-Uuid']) }
.once
end
it 'uses new UUID values between requests' do
stub_requests
trigger_web_hooks
trigger_web_hooks
uuid_headers = uuid_header_spy.values
expect(uuid_headers).to all(be_present)
expect(uuid_headers.length).to eq(4)
expect(uuid_headers.uniq.length).to eq(4)
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).twice
expect(WebMock).to have_requested(:post, stubbed_hostname(system_hook.url)).twice
end
end
end
...@@ -2,20 +2,12 @@ ...@@ -2,20 +2,12 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe WebHookService do RSpec.describe WebHookService, :request_store, :clean_gitlab_redis_shared_state do
include StubRequests include StubRequests
let_it_be(:project) { create(:project) } let_it_be(:project) { create(:project) }
let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) } let_it_be_with_reload(:project_hook) { create(:project_hook, project: project) }
let(:headers) do
{
'Content-Type' => 'application/json',
'User-Agent' => "GitLab/#{Gitlab::VERSION}",
'X-Gitlab-Event' => 'Push Hook'
}
end
let(:data) do let(:data) do
{ before: 'oldrev', after: 'newrev', ref: 'ref' } { before: 'oldrev', after: 'newrev', ref: 'ref' }
end end
...@@ -61,6 +53,21 @@ RSpec.describe WebHookService do ...@@ -61,6 +53,21 @@ RSpec.describe WebHookService do
end end
describe '#execute' do describe '#execute' do
let!(:uuid) { SecureRandom.uuid }
let(:headers) do
{
'Content-Type' => 'application/json',
'User-Agent' => "GitLab/#{Gitlab::VERSION}",
'X-Gitlab-Event' => 'Push Hook',
'X-Gitlab-Event-UUID' => uuid
}
end
before do
# Set a stable value for the `X-Gitlab-Event-UUID` header.
Gitlab::WebHooks::RecursionDetection.set_request_uuid(uuid)
end
context 'when token is defined' do context 'when token is defined' do
let_it_be(:project_hook) { create(:project_hook, :token) } let_it_be(:project_hook) { create(:project_hook, :token) }
...@@ -127,11 +134,74 @@ RSpec.describe WebHookService do ...@@ -127,11 +134,74 @@ RSpec.describe WebHookService do
expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' }) expect(service_instance.execute).to eq({ status: :error, message: 'Hook disabled' })
end end
it 'executes and registers the hook with the recursion detection', :aggregate_failures do
stub_full_request(project_hook.url, method: :post)
cache_key = Gitlab::WebHooks::RecursionDetection.send(:cache_key_for_hook, project_hook)
::Gitlab::Redis::SharedState.with do |redis|
expect { service_instance.execute }.to change {
redis.sismember(cache_key, project_hook.id)
}.to(true)
end
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
.with(headers: headers)
.once
end
it 'executes and logs if a recursive web hook is detected', :aggregate_failures do
stub_full_request(project_hook.url, method: :post)
Gitlab::WebHooks::RecursionDetection.register!(project_hook)
expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: project_hook.id,
hook_type: 'ProjectHook',
hook_name: 'push_hooks',
recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
'correlation_id' => kind_of(String)
)
)
service_instance.execute
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
.with(headers: headers)
.once
end
it 'executes and logs if the recursion count limit would be exceeded', :aggregate_failures do
stub_full_request(project_hook.url, method: :post)
stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3)
previous_hooks = create_list(:project_hook, 3)
previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) }
expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: project_hook.id,
hook_type: 'ProjectHook',
hook_name: 'push_hooks',
recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
'correlation_id' => kind_of(String)
)
)
service_instance.execute
expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url))
.with(headers: headers)
.once
end
it 'handles exceptions' do it 'handles exceptions' do
exceptions = Gitlab::HTTP::HTTP_ERRORS + [ exceptions = Gitlab::HTTP::HTTP_ERRORS + [
Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError Gitlab::Json::LimitedEncoder::LimitExceeded, URI::InvalidURIError
] ]
allow(Gitlab::WebHooks::RecursionDetection).to receive(:block?).and_return(false)
exceptions.each do |exception_class| exceptions.each do |exception_class|
exception = exception_class.new('Exception message') exception = exception_class.new('Exception message')
project_hook.enable! project_hook.enable!
...@@ -420,6 +490,57 @@ RSpec.describe WebHookService do ...@@ -420,6 +490,57 @@ RSpec.describe WebHookService do
end end
end end
context 'recursion detection' do
before do
# Set a request UUID so `RecursionDetection.block?` will query redis.
Gitlab::WebHooks::RecursionDetection.set_request_uuid(SecureRandom.uuid)
end
it 'queues a worker and logs an error if the call chain limit would be exceeded' do
stub_const("#{Gitlab::WebHooks::RecursionDetection.name}::COUNT_LIMIT", 3)
previous_hooks = create_list(:project_hook, 3)
previous_hooks.each { Gitlab::WebHooks::RecursionDetection.register!(_1) }
expect(WebHookWorker).to receive(:perform_async)
expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: project_hook.id,
hook_type: 'ProjectHook',
hook_name: 'push_hooks',
recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
'correlation_id' => kind_of(String),
'meta.project' => project.full_path,
'meta.related_class' => 'ProjectHook',
'meta.root_namespace' => project.root_namespace.full_path
)
)
service_instance.async_execute
end
it 'queues a worker and logs an error if a recursive call chain is detected' do
Gitlab::WebHooks::RecursionDetection.register!(project_hook)
expect(WebHookWorker).to receive(:perform_async)
expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook recursion detected and will be blocked in future',
hook_id: project_hook.id,
hook_type: 'ProjectHook',
hook_name: 'push_hooks',
recursion_detection: Gitlab::WebHooks::RecursionDetection.to_log(project_hook),
'correlation_id' => kind_of(String),
'meta.project' => project.full_path,
'meta.related_class' => 'ProjectHook',
'meta.root_namespace' => project.root_namespace.full_path
)
)
service_instance.async_execute
end
end
context 'when hook has custom context attributes' do context 'when hook has custom context attributes' do
it 'includes the attributes in the worker context' do it 'includes the attributes in the worker context' do
expect(WebHookWorker).to receive(:perform_async) do expect(WebHookWorker).to receive(:perform_async) do
......
...@@ -19,6 +19,15 @@ RSpec.describe WebHookWorker do ...@@ -19,6 +19,15 @@ RSpec.describe WebHookWorker do
expect { subject.perform(non_existing_record_id, data, hook_name) }.not_to raise_error expect { subject.perform(non_existing_record_id, data, hook_name) }.not_to raise_error
end end
it 'retrieves recursion detection data, reinstates it, and cleans it from payload', :request_store, :aggregate_failures do
uuid = SecureRandom.uuid
full_data = data.merge({ _gitlab_recursion_detection_request_uuid: uuid })
expect_next(WebHookService, project_hook, data.with_indifferent_access, hook_name, anything).to receive(:execute)
expect { subject.perform(project_hook.id, full_data, hook_name) }
.to change { Gitlab::WebHooks::RecursionDetection::UUID.instance.request_uuid }.to(uuid)
end
it_behaves_like 'worker with data consistency', it_behaves_like 'worker with data consistency',
described_class, described_class,
data_consistency: :delayed data_consistency: :delayed
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment