diff options
47 files changed, 480 insertions, 650 deletions
@@ -239,7 +239,7 @@ gem 'ruby-progressbar', '~> 1.10' gem 'settingslogic', '~> 2.0.9' # Linear-time regex library for untrusted regular expressions -gem 're2', '~> 1.5.0' +gem 're2', '~> 1.6.0' # Misc diff --git a/Gemfile.checksum b/Gemfile.checksum index 4515a62d10e..37655fcb569 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -455,7 +455,7 @@ {"name":"rbtree","version":"0.4.4","platform":"ruby","checksum":"c1277a502a96fe8fd8656cb619db1ac87145df809ea4db35f7242b50bb161d5c"}, {"name":"rchardet","version":"1.8.0","platform":"ruby","checksum":"693acd5253d5ade81a51940697955f6dd4bb2f0d245bda76a8e23deec70a52c7"}, {"name":"rdoc","version":"6.3.2","platform":"ruby","checksum":"def4a720235c27d56c176ae73555e647eb04ea58a8bbaa927f8f9f79de7805a6"}, -{"name":"re2","version":"1.5.0","platform":"ruby","checksum":"35fe8b408de9f1ef609b1e54e01ea1e55413ca3e9daf1e4b20756d9a02f630cc"}, +{"name":"re2","version":"1.6.0","platform":"ruby","checksum":"2e37f27971f6a76223eac688c04f3e48aea374f34b302ec22d75b4635cd64bc1"}, {"name":"recaptcha","version":"4.13.1","platform":"ruby","checksum":"dc6c2cb78afa87034358b7ba1c6f7175972b5709fdf7500e2550623e119e3788"}, {"name":"recursive-open-struct","version":"1.1.3","platform":"ruby","checksum":"a3538a72552fcebcd0ada657bdff313641a4a5fbc482c08cfb9a65acb1c9de5a"}, {"name":"redcarpet","version":"3.5.1","platform":"ruby","checksum":"717f64cb6ec11c8d9ec9b521ed26ca2eeda68b4fe1fc3388a641176dbd47732f"}, diff --git a/Gemfile.lock b/Gemfile.lock index 05252a7a2f8..0447008023e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1139,7 +1139,7 @@ GEM rbtree (0.4.4) rchardet (1.8.0) rdoc (6.3.2) - re2 (1.5.0) + re2 (1.6.0) recaptcha (4.13.1) json recursive-open-struct (1.1.3) @@ -1753,7 +1753,7 @@ DEPENDENCIES rainbow (~> 3.0) rbtrace (~> 0.4) rdoc (~> 6.3.2) - re2 (~> 1.5.0) + re2 (~> 1.6.0) recaptcha (~> 4.11) redis (~> 4.7.0) redis-actionpack (~> 5.3.0) diff --git a/app/assets/javascripts/members/components/action_buttons/access_request_action_buttons.vue b/app/assets/javascripts/members/components/action_buttons/access_request_action_buttons.vue index d092283338c..f4893721b9e 100644 --- a/app/assets/javascripts/members/components/action_buttons/access_request_action_buttons.vue +++ b/app/assets/javascripts/members/components/action_buttons/access_request_action_buttons.vue @@ -7,15 +7,12 @@ import RemoveMemberButton from './remove_member_button.vue'; export default { name: 'AccessRequestActionButtons', components: { ActionButtonGroup, RemoveMemberButton, ApproveAccessRequestButton }, + inheritAttrs: false, props: { member: { type: Object, required: true, }, - permissions: { - type: Object, - required: true, - }, isCurrentUser: { type: Boolean, required: true, @@ -43,10 +40,10 @@ export default { <template> <action-button-group> - <div v-if="permissions.canUpdate" class="gl-px-1"> + <div class="gl-px-1"> <approve-access-request-button :member-id="member.id" /> </div> - <div v-if="permissions.canRemove" class="gl-px-1"> + <div class="gl-px-1"> <remove-member-button :member-id="member.id" :message="message" diff --git a/app/assets/javascripts/members/components/members_tabs.vue b/app/assets/javascripts/members/components/members_tabs.vue index b824a013f3b..5ac8b30614d 100644 --- a/app/assets/javascripts/members/components/members_tabs.vue +++ b/app/assets/javascripts/members/components/members_tabs.vue @@ -27,13 +27,13 @@ export const TABS = [ { namespace: MEMBER_TYPES.invite, title: __('Invited'), - canManageMembersPermissionsRequired: true, + requiredPermissions: ['canManageMembers'], queryParamValue: TAB_QUERY_PARAM_VALUES.invite, }, { namespace: MEMBER_TYPES.accessRequest, title: __('Access requests'), - canManageMembersPermissionsRequired: true, + requiredPermissions: ['canManageAccessRequests'], queryParamValue: TAB_QUERY_PARAM_VALUES.accessRequest, }, ...EE_TABS, @@ -44,7 +44,7 @@ export default { ACTIVE_TAB_QUERY_PARAM_NAME, TABS, components: { MembersApp, GlTabs, GlTab, GlBadge, GlButton }, - inject: ['canManageMembers', 'canExportMembers', 'exportCsvPath'], + inject: ['canManageMembers', 'canManageAccessRequests', 'canExportMembers', 'exportCsvPath'], data() { return { selectedTabIndex: 0, @@ -96,15 +96,13 @@ export default { return true; } - const { canManageMembersPermissionsRequired = false } = tab; + const { requiredPermissions = [] } = tab; const tabCanBeShown = this.getTabCount(tab) > 0 || this.activeTabIndexCalculatedFromUrlParams === index; - if (canManageMembersPermissionsRequired) { - return this.canManageMembers && tabCanBeShown; - } - - return tabCanBeShown; + return ( + tabCanBeShown && requiredPermissions.every((requiredPermission) => this[requiredPermission]) + ); }, }, }; diff --git a/app/assets/javascripts/members/index.js b/app/assets/javascripts/members/index.js index 34660f8f499..359239c5c0c 100644 --- a/app/assets/javascripts/members/index.js +++ b/app/assets/javascripts/members/index.js @@ -17,6 +17,7 @@ export const initMembersApp = (el, options) => { const { sourceId, canManageMembers, + canManageAccessRequests, canExportMembers, canFilterByEnterprise, exportCsvPath, @@ -61,6 +62,7 @@ export const initMembersApp = (el, options) => { currentUserId: gon.current_user_id || null, sourceId, canManageMembers, + canManageAccessRequests, canFilterByEnterprise, canExportMembers, exportCsvPath, diff --git a/app/assets/javascripts/vue_shared/components/markdown_drawer/makrdown_drawer.stories.js b/app/assets/javascripts/vue_shared/components/markdown_drawer/makrdown_drawer.stories.js deleted file mode 100644 index 03bd64e2a57..00000000000 --- a/app/assets/javascripts/vue_shared/components/markdown_drawer/makrdown_drawer.stories.js +++ /dev/null @@ -1,54 +0,0 @@ -import { GlButton } from '@gitlab/ui'; -import { MOCK_HTML } from '../../../../../../spec/frontend/vue_shared/components/markdown_drawer/mock_data'; -import MarkdownDrawer from './markdown_drawer.vue'; - -export default { - component: MarkdownDrawer, - title: 'vue_shared/markdown_drawer', - parameters: { - mirage: { - timing: 1000, - handlers: { - get: { - '/help/user/search/global_search/advanced_search_syntax.json': [ - 200, - {}, - { html: MOCK_HTML }, - ], - }, - }, - }, - }, -}; - -const createStory = ({ ...options }) => (_, { argTypes }) => ({ - components: { MarkdownDrawer, GlButton }, - props: Object.keys(argTypes), - data() { - return { - render: false, - }; - }, - methods: { - toggleDrawer() { - this.$refs.drawer.toggleDrawer(); - }, - }, - mounted() { - window.requestAnimationFrame(() => { - this.render = true; - }); - }, - template: ` - <div v-if="render"> - <gl-button @click="toggleDrawer">Open Drawer</gl-button> - <markdown-drawer - :documentPath="'user/search/global_search/advanced_search_syntax.json'" - ref="drawer" - /> - </div> - `, - ...options, -}); - -export const Default = createStory({}); diff --git a/app/assets/javascripts/vue_shared/components/markdown_drawer/markdown_drawer.vue b/app/assets/javascripts/vue_shared/components/markdown_drawer/markdown_drawer.vue deleted file mode 100644 index a4b509f8656..00000000000 --- a/app/assets/javascripts/vue_shared/components/markdown_drawer/markdown_drawer.vue +++ /dev/null @@ -1,117 +0,0 @@ -<script> -import { GlSafeHtmlDirective as SafeHtml, GlDrawer, GlAlert, GlSkeletonLoader } from '@gitlab/ui'; -import $ from 'jquery'; -import '~/behaviors/markdown/render_gfm'; -import { s__ } from '~/locale'; -import { contentTop } from '~/lib/utils/common_utils'; -import { getRenderedMarkdown } from './utils/fetch'; - -export const cache = {}; - -export default { - name: 'MarkdownDrawer', - components: { - GlDrawer, - GlAlert, - GlSkeletonLoader, - }, - directives: { - SafeHtml, - }, - i18n: { - alert: s__('MardownDrawer|Could not fetch help contents.'), - }, - props: { - documentPath: { - type: String, - required: true, - }, - }, - data() { - return { - loading: false, - hasFetchError: false, - title: '', - body: null, - open: false, - }; - }, - computed: { - drawerOffsetTop() { - return `${contentTop()}px`; - }, - }, - watch: { - documentPath: { - immediate: true, - handler: 'fetchMarkdown', - }, - open(open) { - if (open && this.body) { - this.renderGLFM(); - } - }, - }, - methods: { - async fetchMarkdown() { - const cached = cache[this.documentPath]; - this.hasFetchError = false; - this.title = ''; - if (cached) { - this.title = cached.title; - this.body = cached.body; - if (this.open) { - this.renderGLFM(); - } - } else { - this.loading = true; - const { body, title, hasFetchError } = await getRenderedMarkdown(this.documentPath); - this.title = title; - this.body = body; - this.loading = false; - this.hasFetchError = hasFetchError; - if (this.open) { - this.renderGLFM(); - } - cache[this.documentPath] = { title, body }; - } - }, - renderGLFM() { - this.$nextTick(() => { - $(this.$refs['content-element']).renderGFM(); - }); - }, - closeDrawer() { - this.open = false; - }, - toggleDrawer() { - this.open = !this.open; - }, - openDrawer() { - this.open = true; - }, - }, - safeHtmlConfig: { - ADD_TAGS: ['copy-code'], - }, -}; -</script> -<template> - <gl-drawer :header-height="drawerOffsetTop" :open="open" header-sticky @close="closeDrawer"> - <template #title> - <h4 data-testid="title-element" class="gl-m-0">{{ title }}</h4> - </template> - <template #default> - <div v-if="hasFetchError"> - <gl-alert :dismissible="false" variant="danger">{{ $options.i18n.alert }}</gl-alert> - </div> - <gl-skeleton-loader v-else-if="loading" /> - <div - v-else - ref="content-element" - v-safe-html:[$options.safeHtmlConfig]="body" - class="md" - ></div> - </template> - </gl-drawer> -</template> diff --git a/app/assets/javascripts/vue_shared/components/markdown_drawer/utils/fetch.js b/app/assets/javascripts/vue_shared/components/markdown_drawer/utils/fetch.js deleted file mode 100644 index 7c8e1bc160a..00000000000 --- a/app/assets/javascripts/vue_shared/components/markdown_drawer/utils/fetch.js +++ /dev/null @@ -1,32 +0,0 @@ -import * as Sentry from '@sentry/browser'; -import { helpPagePath } from '~/helpers/help_page_helper'; -import axios from '~/lib/utils/axios_utils'; - -export const splitDocument = (htmlString) => { - const htmlDocument = new DOMParser().parseFromString(htmlString, 'text/html'); - const title = htmlDocument.querySelector('h1')?.innerText; - htmlDocument.querySelector('h1')?.remove(); - return { - title, - body: htmlDocument.querySelector('body').innerHTML.toString(), - }; -}; - -export const getRenderedMarkdown = (documentPath) => { - return axios - .get(helpPagePath(documentPath)) - .then(({ data }) => { - const { body, title } = splitDocument(data.html); - return { - body, - title, - hasFetchError: false, - }; - }) - .catch((e) => { - Sentry.captureException(e); - return { - hasFetchError: true, - }; - }); -}; diff --git a/app/controllers/concerns/web_hooks/hook_actions.rb b/app/controllers/concerns/web_hooks/hook_actions.rb index 69c0f2f30d5..75065ef9d24 100644 --- a/app/controllers/concerns/web_hooks/hook_actions.rb +++ b/app/controllers/concerns/web_hooks/hook_actions.rb @@ -53,6 +53,8 @@ module WebHooks ps = params.require(:hook).permit(*permitted).to_h + ps.delete(:token) if action_name == 'update' && ps[:token] == WebHook::SECRET_MASK + ps[:url_variables] = ps[:url_variables].to_h { [_1[:key], _1[:value].presence] } if ps.key?(:url_variables) if action_name == 'update' && ps.key?(:url_variables) diff --git a/app/finders/access_requests_finder.rb b/app/finders/access_requests_finder.rb index 2cc8a978877..9b1407e2971 100644 --- a/app/finders/access_requests_finder.rb +++ b/app/finders/access_requests_finder.rb @@ -24,6 +24,6 @@ class AccessRequestsFinder private def can_see_access_requests?(current_user) - source && Ability.allowed?(current_user, :"admin_#{source.class.to_s.underscore}", source) + source && Ability.allowed?(current_user, :admin_member_access_request, source) end end diff --git a/app/helpers/groups/group_members_helper.rb b/app/helpers/groups/group_members_helper.rb index 776666add63..5034a4cb9b4 100644 --- a/app/helpers/groups/group_members_helper.rb +++ b/app/helpers/groups/group_members_helper.rb @@ -12,7 +12,8 @@ module Groups::GroupMembersHelper invite: group_members_list_data(group, invited.nil? ? [] : invited, { param_name: :invited_members_page, params: { page: nil } }), access_request: group_members_list_data(group, access_requests.nil? ? [] : access_requests), source_id: group.id, - can_manage_members: can?(current_user, :admin_group_member, group) + can_manage_members: can?(current_user, :admin_group_member, group), + can_manage_access_requests: can?(current_user, :admin_member_access_request, group) } end diff --git a/app/helpers/projects/project_members_helper.rb b/app/helpers/projects/project_members_helper.rb index e35277eabaa..540877e3fb8 100644 --- a/app/helpers/projects/project_members_helper.rb +++ b/app/helpers/projects/project_members_helper.rb @@ -8,7 +8,8 @@ module Projects::ProjectMembersHelper invite: project_members_list_data(project, invited.nil? ? [] : invited), access_request: project_members_list_data(project, access_requests.nil? ? [] : access_requests), source_id: project.id, - can_manage_members: Ability.allowed?(current_user, :admin_project_member, project) + can_manage_members: Ability.allowed?(current_user, :admin_project_member, project), + can_manage_access_requests: Ability.allowed?(current_user, :admin_member_access_request, project) }) end diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb index 76bf75016a6..793c6a89c0a 100644 --- a/app/models/hooks/web_hook.rb +++ b/app/models/hooks/web_hook.rb @@ -11,6 +11,7 @@ class WebHook < ApplicationRecord INITIAL_BACKOFF = 1.minute MAX_BACKOFF = 1.day BACKOFF_GROWTH_FACTOR = 2.0 + SECRET_MASK = '************' attr_encrypted :token, mode: :per_attribute_iv, @@ -210,6 +211,10 @@ class WebHook < ApplicationRecord # Overridden in child classes. end + def masked_token + token.present? ? SECRET_MASK : nil + end + private def next_failure_count diff --git a/app/policies/concerns/member_policy_helpers.rb b/app/policies/concerns/member_policy_helpers.rb new file mode 100644 index 00000000000..6c4a3caf8bf --- /dev/null +++ b/app/policies/concerns/member_policy_helpers.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module MemberPolicyHelpers + extend ActiveSupport::Concern + + private + + def record_is_access_request_of_self? + record_is_access_request? && record_belongs_to_self? + end + + def record_is_access_request? + @subject.request? # rubocop:disable Gitlab/ModuleWithInstanceVariables + end + + def record_belongs_to_self? + @user && @subject.user == @user # rubocop:disable Gitlab/ModuleWithInstanceVariables + end +end diff --git a/app/policies/group_member_policy.rb b/app/policies/group_member_policy.rb index a394b63fc8e..f61f758a8e8 100644 --- a/app/policies/group_member_policy.rb +++ b/app/policies/group_member_policy.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class GroupMemberPolicy < BasePolicy + include MemberPolicyHelpers + delegate :group with_scope :subject @@ -9,7 +11,11 @@ class GroupMemberPolicy < BasePolicy desc "Membership is users' own" with_score 0 - condition(:is_target_user) { @user && @subject.user_id == @user.id } + condition(:target_is_self) { record_belongs_to_self? } + + desc "Membership is users' own access request" + with_score 0 + condition(:access_request_of_self) { record_is_access_request_of_self? } rule { anonymous }.policy do prevent :update_group_member @@ -28,9 +34,13 @@ class GroupMemberPolicy < BasePolicy rule { project_bot & can?(:admin_group_member) }.enable :destroy_project_bot_member - rule { is_target_user }.policy do + rule { target_is_self }.policy do enable :destroy_group_member end + + rule { access_request_of_self }.policy do + enable :withdraw_member_access_request + end end GroupMemberPolicy.prepend_mod_with('GroupMemberPolicy') diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 341f22120eb..806c57bab74 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -283,6 +283,11 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy prevent :create_resource_access_tokens end + rule { can?(:admin_group_member) }.policy do + # ability to read, approve or reject member access requests of other users + enable :admin_member_access_request + end + rule { support_bot & has_project_with_service_desk_enabled }.policy do enable :read_label end diff --git a/app/policies/project_member_policy.rb b/app/policies/project_member_policy.rb index 40ba30fce5e..bcfc7c87d41 100644 --- a/app/policies/project_member_policy.rb +++ b/app/policies/project_member_policy.rb @@ -1,13 +1,18 @@ # frozen_string_literal: true class ProjectMemberPolicy < BasePolicy + include MemberPolicyHelpers delegate { @subject.project } condition(:target_is_holder_of_the_personal_namespace, scope: :subject) do @subject.project.personal_namespace_holder?(@subject.user) end - condition(:target_is_self) { @user && @subject.user == @user } + desc "Membership is users' own access request" + with_score 0 + condition(:access_request_of_self) { record_is_access_request_of_self? } + + condition(:target_is_self) { record_belongs_to_self? } condition(:project_bot) { @subject.user&.project_bot? } rule { anonymous }.prevent_all @@ -24,5 +29,11 @@ class ProjectMemberPolicy < BasePolicy rule { project_bot & can?(:admin_project_member) }.enable :destroy_project_bot_member - rule { target_is_self }.enable :destroy_project_member + rule { target_is_self }.policy do + enable :destroy_project_member + end + + rule { access_request_of_self }.policy do + enable :withdraw_member_access_request + end end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 3ba753ab60d..c71b26987a0 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -834,6 +834,8 @@ class ProjectPolicy < BasePolicy rule { can?(:admin_project_member) }.policy do enable :import_project_members_from_another_project + # ability to read, approve or reject member access requests of other users + enable :admin_member_access_request end rule { registry_enabled & can?(:admin_container_image) }.policy do diff --git a/app/services/members/approve_access_request_service.rb b/app/services/members/approve_access_request_service.rb index 5337279f702..51f9492ec91 100644 --- a/app/services/members/approve_access_request_service.rb +++ b/app/services/members/approve_access_request_service.rb @@ -16,7 +16,7 @@ module Members private def validate_access!(access_requester) - raise Gitlab::Access::AccessDeniedError unless can_update_access_requester?(access_requester) + raise Gitlab::Access::AccessDeniedError unless can_approve_access_requester?(access_requester) if approving_member_with_owner_access_level?(access_requester) && cannot_assign_owner_responsibilities_to_member_in_project?(access_requester) @@ -24,8 +24,8 @@ module Members end end - def can_update_access_requester?(access_requester) - can?(current_user, update_member_permission(access_requester), access_requester) + def can_approve_access_requester?(access_requester) + can?(current_user, :admin_member_access_request, access_requester.source) end def approving_member_with_owner_access_level?(access_requester) diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index ce79907e8a8..f18269454e3 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -48,6 +48,10 @@ module Members def authorized?(member, destroy_bot) return can_destroy_bot_member?(member) if destroy_bot + if member.request? + return can_destroy_member_access_request?(member) || can_withdraw_member_access_request?(member) + end + can_destroy_member?(member) end @@ -106,6 +110,14 @@ module Members can?(current_user, destroy_bot_member_permission(member), member) end + def can_destroy_member_access_request?(member) + can?(current_user, :admin_member_access_request, member.source) + end + + def can_withdraw_member_access_request?(member) + can?(current_user, :withdraw_member_access_request, member) + end + def destroying_member_with_owner_access_level?(member) member.owner? end diff --git a/app/views/shared/members/_access_request_links.html.haml b/app/views/shared/members/_access_request_links.html.haml index 7af946377be..0b38b9d7945 100644 --- a/app/views/shared/members/_access_request_links.html.haml +++ b/app/views/shared/members/_access_request_links.html.haml @@ -8,9 +8,10 @@ data: { confirm: leave_confirmation_message(source), confirm_btn_variant: 'danger', qa_selector: 'leave_group_link' }, class: 'js-leave-link' - elsif requester = source.requesters.find_by(user_id: current_user.id) # rubocop: disable CodeReuse/ActiveRecord - = link_to _('Withdraw Access Request'), polymorphic_path([:leave, source, :members]), - method: :delete, - data: { confirm: remove_member_message(requester) } + - if can?(current_user, :withdraw_member_access_request, requester) + = link_to _('Withdraw Access Request'), polymorphic_path([:leave, source, :members]), + method: :delete, + data: { confirm: remove_member_message(requester) } - elsif source.request_access_enabled && can?(current_user, :request_access, source) = link_to _('Request Access'), polymorphic_path([:request_access, source, :members]), method: :post diff --git a/app/views/shared/web_hooks/_form.html.haml b/app/views/shared/web_hooks/_form.html.haml index d5b03647f85..ecb736dac4f 100644 --- a/app/views/shared/web_hooks/_form.html.haml +++ b/app/views/shared/web_hooks/_form.html.haml @@ -10,11 +10,11 @@ = s_('Webhooks|URL must be percent-encoded if it contains one or more special characters.') .form-group = form.label :token, s_('Webhooks|Secret token'), class: 'label-bold' - = form.text_field :token, class: 'form-control gl-form-input', placeholder: '' + = form.password_field :token, value: hook.masked_token, autocomplete: 'new-password', class: 'form-control gl-form-input' %p.form-text.text-muted - code_start = '<code>'.html_safe - code_end = '</code>'.html_safe - = s_('Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token HTTP%{code_end} header.').html_safe % { code_start: code_start, code_end: code_end } + = s_('Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token%{code_end} HTTP header.').html_safe % { code_start: code_start, code_end: code_end } .form-group = form.label :url, s_('Webhooks|Trigger'), class: 'label-bold' %ul.list-unstyled diff --git a/config/initializers/0_marginalia.rb b/config/initializers/0_marginalia.rb index e88599fd93c..c776747939f 100644 --- a/config/initializers/0_marginalia.rb +++ b/config/initializers/0_marginalia.rb @@ -13,7 +13,8 @@ require 'marginalia' # matching against the raw SQL, and prepending the comment prevents color # coding from working in the development log. Marginalia::Comment.prepend_comment = true if Rails.env.production? -Marginalia::Comment.components = [:application, :correlation_id, :jid, :endpoint_id, :db_config_name] +Marginalia::Comment.components = [:application, :correlation_id, :jid, :endpoint_id, :db_config_name, + :console_hostname, :console_username] # As mentioned in https://github.com/basecamp/marginalia/pull/93/files, # adding :line has some overhead because a regexp on the backtrace has diff --git a/config/open_api.yml b/config/open_api.yml index d7a170f2a22..3c767d3baa2 100644 --- a/config/open_api.yml +++ b/config/open_api.yml @@ -20,5 +20,9 @@ metadata: description: Operations related to access requests - name: merge_requests description: Operations related to merge requests + - name: deploy_keys + description: Operations related to deploy keys - name: deployments description: Operations related to deployments + - name: release_links + description: Operations related to release assets (links) diff --git a/doc/development/testing_guide/flaky_tests.md b/doc/development/testing_guide/flaky_tests.md index 1c83faa4c67..a06f2cf6570 100644 --- a/doc/development/testing_guide/flaky_tests.md +++ b/doc/development/testing_guide/flaky_tests.md @@ -11,9 +11,141 @@ info: To determine the technical writer assigned to the Stage/Group associated w It's a test that sometimes fails, but if you retry it enough times, it passes, eventually. +## What are the potential cause for a test to be flaky? + +### Unclean environment + +**Label:** `flaky-test::unclean environment` + +**Description:** The environment got dirtied by a previous test. The actual cause is probably not the flaky test here. + +**Difficulty to reproduce:** Moderate. Usually, running the same spec files until the one that's failing reproduces the problem. + +**Resolution:** Fix the previous tests and/or places where the environment is modified, so that +it's reset to a pristine test after each test. + +**Examples:** + +- [Example 1](https://gitlab.com/gitlab-org/gitlab/-/issues/378414#note_1142026988): A migration + test might roll-back the database, perform its testing, and then roll-up the database in an + inconsistent state, so that following tests might not know about certain columns. +- [Example 2](https://gitlab.com/gitlab-org/gitlab/-/issues/368500): A test modifies data that is + used by a following test. + +### Ordering assertion + +**Label:** `flaky-test::ordering assertion` + +**Description:** The test is expecting a specific order in the data under test yet the data is in +a non-deterministic order. + +**Difficulty to reproduce:** Easy. Usually, running the test locally several times would reproduce +the problem. + +**Resolution:** Depending on the problem, you might want to: + +- loosen the assertion if the test shouldn't care about ordering but only on the elements +- fix the test by specifying a deterministic ordering +- fix the app code by specifying a deterministic ordering + +**Examples:** + +- [Example 1](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/10148/diffs): Without + specifying `ORDER BY`, database will not give deterministic ordering, or data race happening + in the tests. + +### Dataset-specific + +**Label:** `flaky-test::dataset-specific` + +**Description:** The test assumes the dataset is in a particular (usually limited) state, which +might not be true depending on when the test run during the test suite. + +**Difficulty to reproduce:** Moderate, as the amount of data needed to reproduce the issue might be +difficult to achieve locally. + +**Resolution:** Fix the test to not assume that the dataset is in a particular state, don't hardcode IDs. + +**Examples:** + +- [Example 1](https://gitlab.com/gitlab-org/gitlab/-/issues/378381): The database is recreated when + any table has more than 500 columns. It could pass in the merge request, but fail later in + `master` if the order of tests changes. +- [Example 2](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91016/diffs): A test asserts + that trying to find a record with an unexisting ID retuns an error message. The test uses an + hardcoded ID that's supposed to not exist (e.g. `42`). If the test is run early in the test + suite, it might pass as not enough records were created before it, but as soon as it would run + later in the suite, there could be a record that actually has the ID `42`, hence the test would + start to fail. + +### Random input + +**Label:** `flaky-test::random input` + +**Description:** The test use random values, that sometimes match the expectations, and sometimes not. + +**Difficulty to reproduce:** Easy, as the test can be modified locally to use the "random value" +used at the time the test failed + +**Resolution:** Once the problem is reproduced, it should be easy to debug and fix either the test +or the app. + +**Examples:** + +- [Example 1](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/20121): The test isn't robust enough to handle a specific data, that only appears sporadically since the data input is random. + +### Unreliable DOM Selector + +**Label:** `flaky-test::unreliable dom selector` + +**Description:** The DOM selector used in the test is unreliable. + +**Difficulty to reproduce:** Moderate to difficult. Depending on whether the DOM selector is duplicated, or appears after a delay etc. + +**Resolution:** It really depends on the problem here. It could be to wait for requests to finish, to scroll down the page etc. + +**Examples:** + +- [Example 1](https://gitlab.com/gitlab-org/gitlab/-/issues/338341): A non-unique CSS selector + matching more than one element, or a non-waiting selector method that does not allow rendering + time before throwing an `element not found` error. +- [Example 2](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/101728/diffs): A CSS selector + only appears after a GraphQL requests has finished, and the UI has updated. + +### Datetime-sensitive + +**Label:** `flaky-test::datetime-sensitive` + +**Description:** The test is assuming a specific date or time. + +**Difficulty to reproduce:** Easy to moderate, depending on whether the test consistently fails after a certain date, or only fails at a given time or date. + +**Resolution:** Freezing the time is usually a good solution. + +**Examples:** + +- [Example 1](https://gitlab.com/gitlab-org/gitlab/-/issues/118612): A test that breaks after some time passed. + +### Unstable infrastructure + +**Label:** `flaky-test::unstable infrastructure` + +**Description:** The test fails from time to time due to infrastructure issues. + +**Difficulty to reproduce:** Hard. It's really hard to reproduce CI infrastructure issues. It might +be possible by using containers locally. + +**Resolution:** Starting a conversation with the Infrastructure department in a dedicated issue is +usually a good idea. + +**Examples:** + +- [Example 1](https://gitlab.com/gitlab-org/gitlab/-/issues/363214): The runner is under heavy load at this time. +- [Example 2](https://gitlab.com/gitlab-org/gitlab/-/issues/360559): The runner is having networking issues, making a job failing early + ## Quarantined tests -When a test frequently fails in `main`, +When a test frequently fails in `master`, create [a ~"failure::flaky-test" issue](https://about.gitlab.com/handbook/engineering/workflow/#broken-master). If the test cannot be fixed in a timely fashion, there is an impact on the diff --git a/lib/api/api.rb b/lib/api/api.rb index d07e0bcf33c..f4746fa375b 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -171,11 +171,13 @@ module API namespace do mount ::API::AccessRequests mount ::API::Appearance + mount ::API::DeployKeys mount ::API::Deployments mount ::API::Metadata mount ::API::MergeRequestDiffs mount ::API::UserCounts mount ::API::ProjectRepositoryStorageMoves + mount ::API::Release::Links mount ::API::SnippetRepositoryStorageMoves mount ::API::Statistics @@ -219,7 +221,6 @@ module API mount ::API::DebianGroupPackages mount ::API::DebianProjectPackages mount ::API::DependencyProxy - mount ::API::DeployKeys mount ::API::DeployTokens mount ::API::Discussions mount ::API::Environments @@ -294,7 +295,6 @@ module API mount ::API::ProtectedBranches mount ::API::ProtectedTags mount ::API::PypiPackages - mount ::API::Release::Links mount ::API::Releases mount ::API::RemoteMirrors mount ::API::Repositories diff --git a/lib/api/deploy_keys.rb b/lib/api/deploy_keys.rb index c53f4bca5a7..c8ddf081ffc 100644 --- a/lib/api/deploy_keys.rb +++ b/lib/api/deploy_keys.rb @@ -21,7 +21,12 @@ module API # rubocop: enable CodeReuse/ActiveRecord end - desc 'Return all deploy keys' + desc 'List all deploy keys' do + detail 'Get a list of all deploy keys across all projects of the GitLab instance. This endpoint requires administrator access and is not available on GitLab.com.' + success Entities::DeployKey + is_array true + tags %w[deploy_keys] + end params do use :pagination optional :public, type: Boolean, default: false, desc: "Only return deploy keys that are public" @@ -35,13 +40,16 @@ module API end params do - requires :id, type: String, desc: 'The ID of the project' + requires :id, types: [String, Integer], desc: 'The ID or URL-encoded path of the project owned by the authenticated user' end resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do before { authorize_admin_project } - desc "Get a specific project's deploy keys" do + desc 'List deploy keys for project' do + detail "Get a list of a project's deploy keys." success Entities::DeployKeysProject + is_array true + tags %w[deploy_keys] end params do use :pagination @@ -54,8 +62,10 @@ module API end # rubocop: enable CodeReuse/ActiveRecord - desc 'Get single deploy key' do + desc 'Get a single deploy key' do + detail 'Get a single key.' success Entities::DeployKeysProject + tags %w[deploy_keys] end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' @@ -66,12 +76,14 @@ module API present key, with: Entities::DeployKeysProject end - desc 'Add new deploy key to a project' do + desc 'Add deploy key' do + detail "Creates a new deploy key for a project. If the deploy key already exists in another project, it's joined to the current project only if the original one is accessible by the same user." success Entities::DeployKeysProject + tags %w[deploy_keys] end params do - requires :key, type: String, desc: 'The new deploy key' - requires :title, type: String, desc: 'The name of the deploy key' + requires :key, type: String, desc: 'New deploy key' + requires :title, type: String, desc: "New deploy key's title" optional :can_push, type: Boolean, desc: "Can deploy key push to the project's repository" end # rubocop: disable CodeReuse/ActiveRecord @@ -109,12 +121,14 @@ module API end # rubocop: enable CodeReuse/ActiveRecord - desc 'Update an existing deploy key for a project' do + desc 'Update deploy key' do + detail 'Updates a deploy key for a project.' success Entities::DeployKey + tags %w[deploy_keys] end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' - optional :title, type: String, desc: 'The name of the deploy key' + optional :title, type: String, desc: "New deploy key's title" optional :can_push, type: Boolean, desc: "Can deploy key push to the project's repository" at_least_one_of :title, :can_push end @@ -143,9 +157,10 @@ module API end end - desc 'Enable a deploy key for a project' do - detail 'This feature was added in GitLab 8.11' + desc 'Enable a deploy key' do + detail 'Enables a deploy key for a project so this can be used. Returns the enabled key, with a status code 201 when successful. This feature was added in GitLab 8.11.' success Entities::DeployKey + tags %w[deploy_keys] end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' @@ -161,7 +176,10 @@ module API end end - desc 'Delete deploy key for a project' + desc 'Delete deploy key' do + detail "Removes a deploy key from the project. If the deploy key is used only for this project, it's deleted from the system." + tags %w[deploy_keys] + end params do requires :key_id, type: Integer, desc: 'The ID of the deploy key' end diff --git a/lib/api/release/links.rb b/lib/api/release/links.rb index 1616b143ceb..3338ea56b8a 100644 --- a/lib/api/release/links.rb +++ b/lib/api/release/links.rb @@ -14,17 +14,19 @@ module API urgency :low params do - requires :id, type: String, desc: 'The ID of a project' + requires :id, type: [String, Integer], desc: 'The ID or URL-encoded path of the project' end resource 'projects/:id', requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do params do - requires :tag_name, type: String, desc: 'The name of the tag', as: :tag + requires :tag_name, type: String, desc: 'The tag associated with the release', as: :tag end resource 'releases/:tag_name', requirements: RELEASE_ENDPOINT_REQUIREMENTS do resource :assets do - desc 'Get a list of links of a release' do - detail 'This feature was introduced in GitLab 11.7.' + desc 'List links of a release' do + detail 'Get assets as links from a release. This feature was introduced in GitLab 11.7.' success Entities::Releases::Link + is_array true + tags %w[release_links] end params do use :pagination @@ -36,16 +38,20 @@ module API present paginate(release.links.sorted), with: Entities::Releases::Link end - desc 'Create a link of a release' do - detail 'This feature was introduced in GitLab 11.7.' + desc 'Create a release link' do + detail 'Create an asset as a link from a release. This feature was introduced in GitLab 11.7.' success Entities::Releases::Link + tags %w[release_links] end params do - requires :name, type: String, desc: 'The name of the link' - requires :url, type: String, desc: 'The URL of the link' - optional :filepath, type: String, desc: 'The filepath of the link' - optional :link_type, type: String, values: %w[other runbook image package], default: 'other', - desc: 'The link type, one of: "runbook", "image", "package" or "other"' + requires :name, type: String, desc: 'The name of the link. Link names must be unique in the release' + requires :url, type: String, desc: 'The URL of the link. Link URLs must be unique in the release.' + optional :filepath, type: String, desc: 'Optional path for a direct asset link' + optional :link_type, + type: String, + values: %w[other runbook image package], + default: 'other', + desc: 'The type of the link: `other`, `runbook`, `image`, or `package`. Defaults to `other`' end route_setting :authentication, job_token_allowed: true post 'links' do @@ -61,12 +67,13 @@ module API end params do - requires :link_id, type: String, desc: 'The ID of the link' + requires :link_id, type: Integer, desc: 'The ID of the link' end resource 'links/:link_id' do - desc 'Get a link detail of a release' do - detail 'This feature was introduced in GitLab 11.7.' + desc 'Get a release link' do + detail 'Get an asset as a link from a release. This feature was introduced in GitLab 11.7.' success Entities::Releases::Link + tags %w[release_links] end route_setting :authentication, job_token_allowed: true get do @@ -75,16 +82,21 @@ module API present link, with: Entities::Releases::Link end - desc 'Update a link of a release' do - detail 'This feature was introduced in GitLab 11.7.' + desc 'Update a release link' do + detail 'Update an asset as a link from a release. This feature was introduced in GitLab 11.7.' success Entities::Releases::Link + tags %w[release_links] end params do optional :name, type: String, desc: 'The name of the link' optional :url, type: String, desc: 'The URL of the link' - optional :filepath, type: String, desc: 'The filepath of the link' - optional :link_type, type: String, values: %w[other runbook image package], default: 'other', - desc: 'The link type, one of: "runbook", "image", "package" or "other"' + optional :filepath, type: String, desc: 'Optional path for a direct asset link' + optional :link_type, + type: String, + values: %w[other runbook image package], + default: 'other', + desc: 'The type of the link: `other`, `runbook`, `image`, or `package`. Defaults to `other`' + at_least_one_of :name, :url end route_setting :authentication, job_token_allowed: true @@ -98,9 +110,10 @@ module API end end - desc 'Delete a link of a release' do - detail 'This feature was introduced in GitLab 11.7.' + desc 'Delete a release link' do + detail 'Deletes an asset as a link from a release. This feature was introduced in GitLab 11.7.' success Entities::Releases::Link + tags %w[release_links] end route_setting :authentication, job_token_allowed: true delete do diff --git a/lib/gitlab/marginalia/comment.rb b/lib/gitlab/marginalia/comment.rb index aab58bfa211..1997ebb952b 100644 --- a/lib/gitlab/marginalia/comment.rb +++ b/lib/gitlab/marginalia/comment.rb @@ -45,6 +45,18 @@ module Gitlab def db_config_name ::Gitlab::Database.db_config_name(marginalia_adapter) end + + def console_hostname + return unless ::Gitlab::Runtime.console? + + @cached_console_hostname ||= Socket.gethostname # rubocop:disable Gitlab/ModuleWithInstanceVariables + end + + def console_username + return unless ::Gitlab::Runtime.console? + + ENV['SUDO_USER'] || ENV['USER'] + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7d9663108bf..e0106db1181 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -24768,9 +24768,6 @@ msgstr "" msgid "March" msgstr "" -msgid "MardownDrawer|Could not fetch help contents." -msgstr "" - msgid "Mark as done" msgstr "" @@ -45517,7 +45514,7 @@ msgstr "" msgid "Webhooks|URL preview" msgstr "" -msgid "Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token HTTP%{code_end} header." +msgid "Webhooks|Used to validate received payloads. Sent with the request in the %{code_start}X-Gitlab-Token%{code_end} HTTP header." msgstr "" msgid "Webhooks|Webhook disabled" diff --git a/spec/controllers/admin/hooks_controller_spec.rb b/spec/controllers/admin/hooks_controller_spec.rb index 14f4a2f40e7..82e4b873bf6 100644 --- a/spec/controllers/admin/hooks_controller_spec.rb +++ b/spec/controllers/admin/hooks_controller_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe Admin::HooksController do - let(:admin) { create(:admin) } + let_it_be(:admin) { create(:admin) } before do sign_in(admin) @@ -33,7 +33,23 @@ RSpec.describe Admin::HooksController do end describe 'POST #update' do - let!(:hook) { create(:system_hook) } + let_it_be_with_reload(:hook) { create(:system_hook) } + + context 'with an existing token' do + hook_params = { + token: WebHook::SECRET_MASK, + url: "http://example.com" + } + + it 'does not change a token' do + expect do + post :update, params: { id: hook.id, hook: hook_params } + end.not_to change { hook.reload.token } + + expect(response).to have_gitlab_http_status(:found) + expect(flash[:alert]).to be_blank + end + end it 'sets all parameters' do hook.update!(url_variables: { 'foo' => 'bar', 'baz' => 'woo' }) @@ -61,8 +77,8 @@ RSpec.describe Admin::HooksController do end describe 'DELETE #destroy' do - let!(:hook) { create(:system_hook) } - let!(:log) { create(:web_hook_log, web_hook: hook) } + let_it_be(:hook) { create(:system_hook) } + let_it_be(:log) { create(:web_hook_log, web_hook: hook) } let(:params) { { id: hook } } it_behaves_like 'Web hook destroyer' diff --git a/spec/controllers/projects/hooks_controller_spec.rb b/spec/controllers/projects/hooks_controller_spec.rb index 54c958d6350..dbab3575831 100644 --- a/spec/controllers/projects/hooks_controller_spec.rb +++ b/spec/controllers/projects/hooks_controller_spec.rb @@ -29,6 +29,22 @@ RSpec.describe Projects::HooksController do { namespace_id: project.namespace, project_id: project, id: hook.id } end + context 'with an existing token' do + hook_params = { + token: WebHook::SECRET_MASK, + url: "http://example.com" + } + + it 'does not change a token' do + expect do + post :update, params: params.merge({ hook: hook_params }) + end.not_to change { hook.reload.token } + + expect(response).to have_gitlab_http_status(:found) + expect(flash[:alert]).to be_blank + end + end + it 'adds, updates and deletes URL variables' do hook.update!(url_variables: { 'a' => 'bar', 'b' => 'woo' }) diff --git a/spec/frontend/members/components/action_buttons/access_request_action_buttons_spec.js b/spec/frontend/members/components/action_buttons/access_request_action_buttons_spec.js index 3dac47974e7..df5c884f42e 100644 --- a/spec/frontend/members/components/action_buttons/access_request_action_buttons_spec.js +++ b/spec/frontend/members/components/action_buttons/access_request_action_buttons_spec.js @@ -24,86 +24,52 @@ describe('AccessRequestActionButtons', () => { wrapper.destroy(); }); - describe('when user has `canRemove` permissions', () => { - beforeEach(() => { - createComponent({ - permissions: { - canRemove: true, - }, - }); - }); + it('renders remove member button', () => { + createComponent(); - it('renders remove member button', () => { - expect(findRemoveMemberButton().exists()).toBe(true); - }); - - it('sets props correctly', () => { - expect(findRemoveMemberButton().props()).toMatchObject({ - memberId: member.id, - title: 'Deny access', - isAccessRequest: true, - isInvite: false, - icon: 'close', - }); - }); - - describe('when member is the current user', () => { - it('sets `message` prop correctly', () => { - expect(findRemoveMemberButton().props('message')).toBe( - `Are you sure you want to withdraw your access request for "${member.source.fullName}"`, - ); - }); - }); + expect(findRemoveMemberButton().exists()).toBe(true); + }); - describe('when member is not the current user', () => { - it('sets `message` prop correctly', () => { - createComponent({ - isCurrentUser: false, - permissions: { - canRemove: true, - }, - }); + it('sets props correctly on remove member button', () => { + createComponent(); - expect(findRemoveMemberButton().props('message')).toBe( - `Are you sure you want to deny ${member.user.name}'s request to join "${member.source.fullName}"`, - ); - }); + expect(findRemoveMemberButton().props()).toMatchObject({ + memberId: member.id, + title: 'Deny access', + isAccessRequest: true, + isInvite: false, + icon: 'close', }); }); - describe('when user does not have `canRemove` permissions', () => { - it('does not render remove member button', () => { - createComponent({ - permissions: { - canRemove: false, - }, - }); + describe('when member is the current user', () => { + it('sets `message` prop correctly', () => { + createComponent(); - expect(findRemoveMemberButton().exists()).toBe(false); + expect(findRemoveMemberButton().props('message')).toBe( + `Are you sure you want to withdraw your access request for "${member.source.fullName}"`, + ); }); }); - describe('when user has `canUpdate` permissions', () => { - it('renders the approve button', () => { + describe('when member is not the current user', () => { + it('sets `message` prop correctly', () => { createComponent({ + isCurrentUser: false, permissions: { - canUpdate: true, + canRemove: true, }, }); - expect(findApproveButton().exists()).toBe(true); + expect(findRemoveMemberButton().props('message')).toBe( + `Are you sure you want to deny ${member.user.name}'s request to join "${member.source.fullName}"`, + ); }); }); - describe('when user does not have `canUpdate` permissions', () => { - it('does not render the approve button', () => { - createComponent({ - permissions: { - canUpdate: false, - }, - }); + it('renders the approve button', () => { + createComponent(); - expect(findApproveButton().exists()).toBe(false); - }); + expect(findApproveButton().exists()).toBe(true); }); }); diff --git a/spec/frontend/members/components/members_tabs_spec.js b/spec/frontend/members/components/members_tabs_spec.js index 1354b938d77..77af5e7293e 100644 --- a/spec/frontend/members/components/members_tabs_spec.js +++ b/spec/frontend/members/components/members_tabs_spec.js @@ -81,6 +81,7 @@ describe('MembersTabs', () => { stubs: ['members-app'], provide: { canManageMembers: true, + canManageAccessRequests: true, canExportMembers: true, exportCsvPath: '', ...provide, @@ -181,7 +182,9 @@ describe('MembersTabs', () => { describe('when `canManageMembers` is `false`', () => { it('shows all tabs except `Invited` and `Access requests`', async () => { - await createComponent({ provide: { canManageMembers: false } }); + await createComponent({ + provide: { canManageMembers: false, canManageAccessRequests: false }, + }); expect(findTabByText('Members')).not.toBeUndefined(); expect(findTabByText('Groups')).not.toBeUndefined(); diff --git a/spec/frontend/vue_shared/components/markdown_drawer/markdown_drawer_spec.js b/spec/frontend/vue_shared/components/markdown_drawer/markdown_drawer_spec.js deleted file mode 100644 index 8edcb905096..00000000000 --- a/spec/frontend/vue_shared/components/markdown_drawer/markdown_drawer_spec.js +++ /dev/null @@ -1,205 +0,0 @@ -import { GlDrawer, GlAlert, GlSkeletonLoader } from '@gitlab/ui'; -import { nextTick } from 'vue'; -import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; -import MarkdownDrawer, { cache } from '~/vue_shared/components/markdown_drawer/markdown_drawer.vue'; -import { getRenderedMarkdown } from '~/vue_shared/components/markdown_drawer/utils/fetch'; -import { contentTop } from '~/lib/utils/common_utils'; - -jest.mock('~/vue_shared/components/markdown_drawer/utils/fetch', () => ({ - getRenderedMarkdown: jest.fn().mockReturnValue({ - title: 'test title test', - body: `<div id="content-body"> - <div class="documentation md gl-mt-3"> - test body - </div> - </div>`, - }), -})); - -jest.mock('~/lib/utils/common_utils', () => ({ - contentTop: jest.fn(), -})); - -describe('MarkdownDrawer', () => { - let wrapper; - const defaultProps = { - documentPath: 'user/search/global_search/advanced_search_syntax.json', - }; - - const createComponent = (props) => { - wrapper = shallowMountExtended(MarkdownDrawer, { - propsData: { - ...defaultProps, - ...props, - }, - }); - }; - - afterEach(() => { - wrapper.destroy(); - wrapper = null; - Object.keys(cache).forEach((key) => delete cache[key]); - }); - - const findDrawer = () => wrapper.findComponent(GlDrawer); - const findAlert = () => wrapper.findComponent(GlAlert); - const findSkeleton = () => wrapper.findComponent(GlSkeletonLoader); - const findDrawerTitle = () => wrapper.findComponent('[data-testid="title-element"]'); - const findDrawerBody = () => wrapper.findComponent({ ref: 'content-element' }); - - describe('component', () => { - beforeEach(() => { - createComponent(); - }); - - it('renders correctly', () => { - expect(findDrawer().exists()).toBe(true); - expect(findDrawerTitle().text()).toBe('test title test'); - expect(findDrawerBody().text()).toBe('test body'); - }); - }); - - describe.each` - hasNavbar | navbarHeight - ${false} | ${0} - ${true} | ${100} - `('computes offsetTop', ({ hasNavbar, navbarHeight }) => { - beforeEach(() => { - global.document.querySelector = jest.fn(() => - hasNavbar - ? { - dataset: { - page: 'test', - }, - } - : undefined, - ); - contentTop.mockReturnValue(navbarHeight); - createComponent(); - }); - - afterEach(() => { - contentTop.mockClear(); - }); - - it(`computes offsetTop ${hasNavbar ? 'with' : 'without'} .navbar-gitlab`, () => { - expect(findDrawer().attributes('headerheight')).toBe(`${navbarHeight}px`); - }); - }); - - describe('watcher', () => { - let renderGLFMSpy; - let fetchMarkdownSpy; - - beforeEach(async () => { - renderGLFMSpy = jest.spyOn(MarkdownDrawer.methods, 'renderGLFM'); - fetchMarkdownSpy = jest.spyOn(MarkdownDrawer.methods, 'fetchMarkdown'); - global.document.querySelector = jest.fn(() => ({ - getBoundingClientRect: jest.fn(() => ({ bottom: 100 })), - dataset: { - page: 'test', - }, - })); - createComponent(); - await nextTick(); - }); - - afterEach(() => { - renderGLFMSpy.mockClear(); - fetchMarkdownSpy.mockClear(); - }); - - it('for documentPath triggers fetch', async () => { - expect(fetchMarkdownSpy).toHaveBeenCalledTimes(1); - - await wrapper.setProps({ documentPath: '/test/me' }); - await nextTick(); - - expect(fetchMarkdownSpy).toHaveBeenCalledTimes(2); - }); - - it('for open triggers renderGLFM', async () => { - wrapper.vm.fetchMarkdown(); - wrapper.vm.openDrawer(); - await nextTick(); - expect(renderGLFMSpy).toHaveBeenCalled(); - }); - }); - - describe('Markdown fetching', () => { - let renderGLFMSpy; - - beforeEach(async () => { - renderGLFMSpy = jest.spyOn(MarkdownDrawer.methods, 'renderGLFM'); - createComponent(); - await nextTick(); - }); - - afterEach(() => { - renderGLFMSpy.mockClear(); - }); - - it('fetches the Markdown and caches it', async () => { - expect(getRenderedMarkdown).toHaveBeenCalledTimes(1); - expect(Object.keys(cache)).toHaveLength(1); - }); - - it('when the document changes, fetches it and caches it as well', async () => { - expect(getRenderedMarkdown).toHaveBeenCalledTimes(1); - expect(Object.keys(cache)).toHaveLength(1); - - await wrapper.setProps({ documentPath: '/test/me2' }); - await nextTick(); - - expect(getRenderedMarkdown).toHaveBeenCalledTimes(2); - expect(Object.keys(cache)).toHaveLength(2); - }); - - it('when re-using an already fetched document, gets it from the cache', async () => { - await wrapper.setProps({ documentPath: '/test/me2' }); - await nextTick(); - - expect(getRenderedMarkdown).toHaveBeenCalledTimes(2); - expect(Object.keys(cache)).toHaveLength(2); - - await wrapper.setProps({ documentPath: defaultProps.documentPath }); - await nextTick(); - - expect(getRenderedMarkdown).toHaveBeenCalledTimes(2); - expect(Object.keys(cache)).toHaveLength(2); - }); - }); - - describe('Markdown fetching returns error', () => { - beforeEach(async () => { - getRenderedMarkdown.mockReturnValue({ - hasFetchError: true, - }); - - createComponent(); - await nextTick(); - }); - afterEach(() => { - getRenderedMarkdown.mockClear(); - }); - it('shows alert', () => { - expect(findAlert().exists()).toBe(true); - }); - }); - - describe('While Markdown is fetching', () => { - beforeEach(async () => { - getRenderedMarkdown.mockReturnValue(new Promise(() => {})); - - createComponent(); - }); - - afterEach(() => { - getRenderedMarkdown.mockClear(); - }); - - it('shows skeleton', async () => { - expect(findSkeleton().exists()).toBe(true); - }); - }); -}); diff --git a/spec/frontend/vue_shared/components/markdown_drawer/mock_data.js b/spec/frontend/vue_shared/components/markdown_drawer/mock_data.js deleted file mode 100644 index 53b40407556..00000000000 --- a/spec/frontend/vue_shared/components/markdown_drawer/mock_data.js +++ /dev/null @@ -1,42 +0,0 @@ -export const MOCK_HTML = `<!DOCTYPE html> -<html> -<body> - <div id="content-body"> - <h1>test title <strong>test</strong></h1> - <div class="documentation md gl-mt-3"> - <a href="../advanced_search.md">Advanced Search</a> - <a href="../advanced_search2.md">Advanced Search2</a> - <h2>test header h2</h2> - <table class="testClass"> - <tr> - <td>Emil</td> - <td>Tobias</td> - <td>Linus</td> - </tr> - <tr> - <td>16</td> - <td>14</td> - <td>10</td> - </tr> - </table> - </div> - </div> -</body> -</html>`.replace(/\n/g, ''); - -export const MOCK_DRAWER_DATA = { - hasFetchError: false, - title: 'test title test', - body: ` <div id="content-body"> <div class="documentation md gl-mt-3"> <a href="../advanced_search.md">Advanced Search</a> <a href="../advanced_search2.md">Advanced Search2</a> <h2>test header h2</h2> <table class="testClass"> <tbody><tr> <td>Emil</td> <td>Tobias</td> <td>Linus</td> </tr> <tr> <td>16</td> <td>14</td> <td>10</td> </tr> </tbody></table> </div> </div>`, -}; - -export const MOCK_DRAWER_DATA_ERROR = { - hasFetchError: true, -}; - -export const MOCK_TABLE_DATA_BEFORE = `<head></head><body><h1>test</h1></test><table><tbody><tr><td></td></tr></tbody></table></body>`; - -export const MOCK_HTML_DATA_AFTER = { - body: '<table><tbody><tr><td></td></tr></tbody></table>', - title: 'test', -}; diff --git a/spec/frontend/vue_shared/components/markdown_drawer/utils/fetch_spec.js b/spec/frontend/vue_shared/components/markdown_drawer/utils/fetch_spec.js deleted file mode 100644 index ff07b2cf838..00000000000 --- a/spec/frontend/vue_shared/components/markdown_drawer/utils/fetch_spec.js +++ /dev/null @@ -1,43 +0,0 @@ -import MockAdapter from 'axios-mock-adapter'; -import { - getRenderedMarkdown, - splitDocument, -} from '~/vue_shared/components/markdown_drawer/utils/fetch'; -import axios from '~/lib/utils/axios_utils'; -import { - MOCK_HTML, - MOCK_DRAWER_DATA, - MOCK_DRAWER_DATA_ERROR, - MOCK_TABLE_DATA_BEFORE, - MOCK_HTML_DATA_AFTER, -} from '../mock_data'; - -describe('utils/fetch', () => { - let mock; - - afterEach(() => { - mock.restore(); - }); - - describe.each` - axiosMock | type | toExpect - ${{ code: 200, res: { html: MOCK_HTML } }} | ${'success'} | ${MOCK_DRAWER_DATA} - ${{ code: 500, res: null }} | ${'error'} | ${MOCK_DRAWER_DATA_ERROR} - `('process markdown data', ({ axiosMock, type, toExpect }) => { - describe(`if api fetch responds with ${type}`, () => { - beforeEach(() => { - mock = new MockAdapter(axios); - mock.onGet().reply(axiosMock.code, axiosMock.res); - }); - it(`should update drawer correctly`, async () => { - expect(await getRenderedMarkdown('/any/path')).toStrictEqual(toExpect); - }); - }); - }); - - describe('splitDocument', () => { - it(`should update tables correctly`, () => { - expect(splitDocument(MOCK_TABLE_DATA_BEFORE)).toStrictEqual(MOCK_HTML_DATA_AFTER); - }); - }); -}); diff --git a/spec/helpers/groups/group_members_helper_spec.rb b/spec/helpers/groups/group_members_helper_spec.rb index fa8d4991d99..4d1280533dd 100644 --- a/spec/helpers/groups/group_members_helper_spec.rb +++ b/spec/helpers/groups/group_members_helper_spec.rb @@ -26,6 +26,7 @@ RSpec.describe Groups::GroupMembersHelper do allow(helper).to receive(:group_group_member_path).with(shared_group, ':id').and_return('/groups/foo-bar/-/group_members/:id') allow(helper).to receive(:group_group_link_path).with(shared_group, ':id').and_return('/groups/foo-bar/-/group_links/:id') allow(helper).to receive(:can?).with(current_user, :admin_group_member, shared_group).and_return(true) + allow(helper).to receive(:can?).with(current_user, :admin_member_access_request, shared_group).and_return(true) end subject do @@ -53,7 +54,8 @@ RSpec.describe Groups::GroupMembersHelper do it 'returns expected json' do expected = { source_id: shared_group.id, - can_manage_members: true + can_manage_members: true, + can_manage_access_requests: true } expect(subject).to include(expected) @@ -99,6 +101,7 @@ RSpec.describe Groups::GroupMembersHelper do allow(helper).to receive(:group_group_member_path).with(sub_shared_group, ':id').and_return('/groups/foo-bar/-/group_members/:id') allow(helper).to receive(:group_group_link_path).with(sub_shared_group, ':id').and_return('/groups/foo-bar/-/group_links/:id') allow(helper).to receive(:can?).with(current_user, :admin_group_member, sub_shared_group).and_return(true) + allow(helper).to receive(:can?).with(current_user, :admin_member_access_request, sub_shared_group).and_return(true) allow(helper).to receive(:can?).with(current_user, :export_group_memberships, sub_shared_group).and_return(true) end diff --git a/spec/helpers/projects/project_members_helper_spec.rb b/spec/helpers/projects/project_members_helper_spec.rb index 844c33de635..f3201ce0e14 100644 --- a/spec/helpers/projects/project_members_helper_spec.rb +++ b/spec/helpers/projects/project_members_helper_spec.rb @@ -41,7 +41,8 @@ RSpec.describe Projects::ProjectMembersHelper do it 'returns expected json' do expected = { source_id: project.id, - can_manage_members: true + can_manage_members: true, + can_manage_access_requests: true }.as_json expect(subject).to include(expected) diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 3ca41e613fa..8c802628559 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -299,7 +299,7 @@ RSpec.describe WebHook do end describe '#executable?' do - let(:web_hook) { create(:project_hook, project: project) } + let_it_be_with_reload(:web_hook) { create(:project_hook, project: project) } where(:recent_failures, :not_until, :executable) do [ @@ -757,4 +757,14 @@ RSpec.describe WebHook do expect { described_class.new.update_last_failure }.not_to raise_error end end + + describe '#masked_token' do + it { expect(hook.masked_token).to be_nil } + + context 'with a token' do + let(:hook) { build(:project_hook, :token, project: project) } + + it { expect(hook.masked_token).to eq described_class::SECRET_MASK } + end + end end diff --git a/spec/policies/group_member_policy_spec.rb b/spec/policies/group_member_policy_spec.rb index 27ce683861c..24ffd9e5e39 100644 --- a/spec/policies/group_member_policy_spec.rb +++ b/spec/policies/group_member_policy_spec.rb @@ -83,6 +83,31 @@ RSpec.describe GroupMemberPolicy do specify { expect_allowed(:read_group) } end + context 'for access requests' do + let_it_be(:group) { create(:group, :public) } + let_it_be(:user) { create(:user) } + + let(:current_user) { user } + + context 'for own access request' do + let(:membership) { create(:group_member, :access_request, group: group, user: user) } + + specify { expect_allowed(:withdraw_member_access_request) } + end + + context "for another user's access request" do + let(:membership) { create(:group_member, :access_request, group: group, user: create(:user)) } + + specify { expect_disallowed(:withdraw_member_access_request) } + end + + context 'for own, valid membership' do + let(:membership) { create(:group_member, :developer, group: group, user: user) } + + specify { expect_disallowed(:withdraw_member_access_request) } + end + end + context 'with bot user' do let(:current_user) { create(:user, :project_bot) } diff --git a/spec/policies/project_member_policy_spec.rb b/spec/policies/project_member_policy_spec.rb index b19ab71fcb5..d7c155b39f5 100644 --- a/spec/policies/project_member_policy_spec.rb +++ b/spec/policies/project_member_policy_spec.rb @@ -4,13 +4,14 @@ require 'spec_helper' RSpec.describe ProjectMemberPolicy do let(:project) { create(:project) } - let(:maintainer_user) { create(:user) } + let(:maintainer) { create(:user) } let(:member) { create(:project_member, project: project, user: member_user) } + let(:current_user) { maintainer } - subject { described_class.new(maintainer_user, member) } + subject { described_class.new(current_user, member) } before do - create(:project_member, :maintainer, project: project, user: maintainer_user) + create(:project_member, :maintainer, project: project, user: maintainer) end context 'with regular member' do @@ -40,4 +41,29 @@ RSpec.describe ProjectMemberPolicy do it { is_expected.not_to be_allowed(:update_project_member) } it { is_expected.not_to be_allowed(:destroy_project_member) } end + + context 'for access requests' do + let_it_be(:project) { create(:project, :public) } + let_it_be(:user) { create(:user) } + + let(:current_user) { user } + + context 'for own access request' do + let(:member) { create(:project_member, :access_request, project: project, user: user) } + + specify { expect_allowed(:withdraw_member_access_request) } + end + + context "for another user's access request" do + let(:member) { create(:project_member, :access_request, project: project, user: create(:user)) } + + specify { expect_disallowed(:withdraw_member_access_request) } + end + + context 'for own, valid membership' do + let(:member) { create(:project_member, :developer, project: project, user: user) } + + specify { expect_disallowed(:withdraw_member_access_request) } + end + end end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 8559c02be57..d0f009f1321 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -77,7 +77,7 @@ RSpec.describe Members::DestroyService do end end - shared_examples 'a service destroying an access requester' do + shared_examples 'a service destroying an access request of another user' do it_behaves_like 'a service destroying a member' it 'calls Member#after_decline_request' do @@ -85,12 +85,16 @@ RSpec.describe Members::DestroyService do described_class.new(current_user).execute(member, **opts) end + end + + shared_examples 'a service destroying an access request of self' do + it_behaves_like 'a service destroying a member' context 'when current user is the member' do it 'does not call Member#after_decline_request' do expect_any_instance_of(NotificationService).not_to receive(:decline_access_request).with(member) - described_class.new(member_user).execute(member, **opts) + described_class.new(current_user).execute(member, **opts) end end end @@ -277,11 +281,24 @@ RSpec.describe Members::DestroyService do group.add_owner(current_user) end - it_behaves_like 'a service destroying an access requester' do + it_behaves_like 'a service destroying an access request of another user' do + let(:member) { group_project.requesters.find_by(user_id: member_user.id) } + end + + it_behaves_like 'a service destroying an access request of another user' do + let(:member) { group.requesters.find_by(user_id: member_user.id) } + end + end + + context 'on withdrawing their own access request' do + let(:opts) { { skip_subresources: true } } + let(:current_user) { member_user } + + it_behaves_like 'a service destroying an access request of self' do let(:member) { group_project.requesters.find_by(user_id: member_user.id) } end - it_behaves_like 'a service destroying an access requester' do + it_behaves_like 'a service destroying an access request of self' do let(:member) { group.requesters.find_by(user_id: member_user.id) } end end diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb index bb1b794c2b6..a6226fe903b 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -76,6 +76,7 @@ RSpec.shared_context 'GroupPolicy context' do register_group_runners read_billing edit_billing + admin_member_access_request ] end diff --git a/spec/support/shared_contexts/policies/project_policy_shared_context.rb b/spec/support/shared_contexts/policies/project_policy_shared_context.rb index fc7255a4a20..9f99a66e2e1 100644 --- a/spec/support/shared_contexts/policies/project_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/project_policy_shared_context.rb @@ -68,7 +68,7 @@ RSpec.shared_context 'ProjectPolicy context' do admin_project admin_project_member admin_snippet admin_terraform_state admin_wiki create_deploy_token destroy_deploy_token push_to_delete_protected_branch read_deploy_token update_snippet - destroy_upload + destroy_upload admin_member_access_request ] end diff --git a/storybook/config/preview.js b/storybook/config/preview.js index 70bd9873833..6f3b8190742 100644 --- a/storybook/config/preview.js +++ b/storybook/config/preview.js @@ -3,10 +3,6 @@ import Vue from 'vue'; import { createMockServer } from 'test_helpers/mock_server'; import translateMixin from '~/vue_shared/translate'; -// fixing toJSON error -// https://github.com/storybookjs/storybook/issues/14933 -Vue.prototype.toJSON = () => {}; - const stylesheetsRequireCtx = require.context( '../../app/assets/stylesheets', true, |