diff options
56 files changed, 1089 insertions, 341 deletions
diff --git a/app/assets/javascripts/google_cloud/components/service_accounts_list.vue b/app/assets/javascripts/google_cloud/components/service_accounts_list.vue index b70b25a5dc3..4db84746482 100644 --- a/app/assets/javascripts/google_cloud/components/service_accounts_list.vue +++ b/app/assets/javascripts/google_cloud/components/service_accounts_list.vue @@ -1,9 +1,9 @@ <script> -import { GlButton, GlEmptyState, GlTable } from '@gitlab/ui'; +import { GlAlert, GlButton, GlEmptyState, GlLink, GlSprintf, GlTable } from '@gitlab/ui'; import { __ } from '~/locale'; export default { - components: { GlButton, GlEmptyState, GlTable }, + components: { GlAlert, GlButton, GlEmptyState, GlLink, GlSprintf, GlTable }, props: { list: { type: Array, @@ -28,6 +28,22 @@ export default { ], }; }, + i18n: { + createServiceAccount: __('Create service account'), + found: __('✔'), + notFound: __('Not found'), + noServiceAccountsTitle: __('No service accounts'), + noServiceAccountsDescription: __( + 'Service Accounts keys authorize GitLab to deploy your Google Cloud project', + ), + serviceAccountsTitle: __('Service accounts'), + serviceAccountsDescription: __( + 'Service Accounts keys authorize GitLab to deploy your Google Cloud project', + ), + secretManagersDescription: __( + 'Enhance security by storing service account keys in secret managers - learn more about %{docLinkStart}secret management with GitLab%{docLinkEnd}', + ), + }, }; </script> @@ -35,31 +51,39 @@ export default { <div> <gl-empty-state v-if="list.length === 0" - :title="__('No service accounts')" - :description=" - __('Service Accounts keys authorize GitLab to deploy your Google Cloud project') - " + :title="$options.i18n.noServiceAccountsTitle" + :description="$options.i18n.noServiceAccountsDescription" :primary-button-link="createUrl" - :primary-button-text="__('Create service account')" + :primary-button-text="$options.i18n.createServiceAccount" :svg-path="emptyIllustrationUrl" /> <div v-else> - <h2 class="gl-font-size-h2">{{ __('Service Accounts') }}</h2> - <p>{{ __('Service Accounts keys authorize GitLab to deploy your Google Cloud project') }}</p> + <h2 class="gl-font-size-h2">{{ $options.i18n.serviceAccountsTitle }}</h2> + <p>{{ $options.i18n.serviceAccountsDescription }}</p> <gl-table :items="list" :fields="tableFields"> <template #cell(service_account_exists)="{ value }"> - {{ value ? '✔' : __('Not found') }} + {{ value ? $options.i18n.found : $options.i18n.notFound }} </template> <template #cell(service_account_key_exists)="{ value }"> - {{ value ? '✔' : __('Not found') }} + {{ value ? $options.i18n.found : $options.i18n.notFound }} </template> </gl-table> <gl-button :href="createUrl" category="primary" variant="info"> - {{ __('Create service account') }} + {{ $options.i18n.createServiceAccount }} </gl-button> + + <gl-alert class="gl-mt-5" :dismissible="false" variant="tip"> + <gl-sprintf :message="$options.i18n.secretManagersDescription"> + <template #docLink="{ content }"> + <gl-link href="https://docs.gitlab.com/ee/ci/secrets/"> + {{ content }} + </gl-link> + </template> + </gl-sprintf> + </gl-alert> </div> </div> </template> diff --git a/app/helpers/system_note_helper.rb b/app/helpers/system_note_helper.rb index f2e1d158c2d..b45ce10a2f6 100644 --- a/app/helpers/system_note_helper.rb +++ b/app/helpers/system_note_helper.rb @@ -42,7 +42,8 @@ module SystemNoteHelper 'cloned' => 'documents', 'issue_type' => 'pencil-square', 'attention_requested' => 'user', - 'attention_request_removed' => 'user' + 'attention_request_removed' => 'user', + 'contact' => 'users' }.freeze def system_note_icon_name(note) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 1283d095c6b..a1311b8555f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -40,6 +40,10 @@ module Ci # https://gitlab.com/gitlab-org/gitlab/-/issues/259010 attr_accessor :merged_yaml + # This is used to retain access to the method defined by `Ci::HasRef` + # before being overridden in this class. + alias_method :jobs_git_ref, :git_ref + belongs_to :project, inverse_of: :all_pipelines belongs_to :user belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline' diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 203e14f1227..8a167034629 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -25,7 +25,7 @@ class Discussion :to_ability_name, :editable?, :resolved_by_id, - :system_note_with_references_visible_for?, + :system_note_visible_for?, :resource_parent, :save, to: :first_note diff --git a/app/models/note.rb b/app/models/note.rb index a143c21c0f9..3f3fa968393 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -27,10 +27,14 @@ class Note < ApplicationRecord redact_field :note - TYPES_RESTRICTED_BY_ABILITY = { + TYPES_RESTRICTED_BY_PROJECT_ABILITY = { branch: :download_code }.freeze + TYPES_RESTRICTED_BY_GROUP_ABILITY = { + contact: :read_crm_contact + }.freeze + # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes. # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102 alias_attribute :last_edited_by, :updated_by @@ -119,7 +123,7 @@ class Note < ApplicationRecord scope :inc_author, -> { includes(:author) } scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) } scope :inc_relations_for_view, -> do - includes(:project, { author: :status }, :updated_by, :resolved_by, :award_emoji, + includes({ project: :group }, { author: :status }, :updated_by, :resolved_by, :award_emoji, { system_note_metadata: :description_version }, :note_diff_file, :diff_note_positions, :suggestions) end @@ -565,10 +569,10 @@ class Note < ApplicationRecord noteable.user_mentions.where(note: self) end - def system_note_with_references_visible_for?(user) + def system_note_visible_for?(user) return true unless system? - (!system_note_with_references? || all_referenced_mentionables_allowed?(user)) && system_note_viewable_by?(user) + system_note_viewable_by?(user) && all_referenced_mentionables_allowed?(user) end def parent_user @@ -617,10 +621,17 @@ class Note < ApplicationRecord def system_note_viewable_by?(user) return true unless system_note_metadata - restriction = TYPES_RESTRICTED_BY_ABILITY[system_note_metadata.action.to_sym] - return Ability.allowed?(user, restriction, project) if restriction + system_note_viewable_by_project_ability?(user) && system_note_viewable_by_group_ability?(user) + end - true + def system_note_viewable_by_project_ability?(user) + project_restriction = TYPES_RESTRICTED_BY_PROJECT_ABILITY[system_note_metadata.action.to_sym] + !project_restriction || Ability.allowed?(user, project_restriction, project) + end + + def system_note_viewable_by_group_ability?(user) + group_restriction = TYPES_RESTRICTED_BY_GROUP_ABILITY[system_note_metadata.action.to_sym] + !group_restriction || Ability.allowed?(user, group_restriction, project&.group) end def keep_around_commit @@ -646,6 +657,8 @@ class Note < ApplicationRecord end def all_referenced_mentionables_allowed?(user) + return true unless system_note_with_references? + if user_visible_reference_count.present? && total_reference_count.present? # if they are not equal, then there are private/confidential references as well user_visible_reference_count > 0 && user_visible_reference_count == total_reference_count diff --git a/app/models/project.rb b/app/models/project.rb index e86d704f3b6..1530d129838 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2187,14 +2187,6 @@ class Project < ApplicationRecord end end - def ci_instance_variables_for(ref:) - if protected_for?(ref) - Ci::InstanceVariable.all_cached - else - Ci::InstanceVariable.unprotected_cached - end - end - def protected_for?(ref) raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref) diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index a3c9db90b5d..0be56d8b4a4 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -24,7 +24,7 @@ class SystemNoteMetadata < ApplicationRecord opened closed merged duplicate locked unlocked outdated reviewer tag due_date pinned_embed cherry_pick health_status approved unapproved status alert_issue_added relate unrelate new_alert_added severity - attention_requested attention_request_removed + attention_requested attention_request_removed contact ].freeze validates :note, presence: true, unless: :importing? diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb index d9ea7c38f11..e85f18f2d37 100644 --- a/app/policies/note_policy.rb +++ b/app/policies/note_policy.rb @@ -16,7 +16,7 @@ class NotePolicy < BasePolicy condition(:for_design) { @subject.for_design? } - condition(:is_visible) { @subject.system_note_with_references_visible_for?(@user) } + condition(:is_visible) { @subject.system_note_visible_for?(@user) } condition(:confidential, scope: :subject) { @subject.confidential? } diff --git a/app/services/issues/set_crm_contacts_service.rb b/app/services/issues/set_crm_contacts_service.rb index ea279c97c03..2edc944435b 100644 --- a/app/services/issues/set_crm_contacts_service.rb +++ b/app/services/issues/set_crm_contacts_service.rb @@ -16,6 +16,9 @@ module Issues determine_changes if params[:replace_ids].present? return error_too_many if too_many? + @added_count = 0 + @removed_count = 0 + add if params[:add_ids].present? remove if params[:remove_ids].present? @@ -25,6 +28,7 @@ module Issues if issue.valid? GraphqlTriggers.issue_crm_contacts_updated(issue) issue.touch + create_system_note ServiceResponse.success(payload: issue) else # The default error isn't very helpful: "Issue customer relations contacts is invalid" @@ -36,7 +40,7 @@ module Issues private - attr_accessor :issue, :errors, :existing_ids + attr_accessor :issue, :errors, :existing_ids, :added_count, :removed_count def determine_changes params[:add_ids] = params[:replace_ids] - existing_ids @@ -63,7 +67,9 @@ module Issues contact_ids.uniq.each do |contact_id| issue_contact = issue.issue_customer_relations_contacts.create(contact_id: contact_id) - unless issue_contact.persisted? + if issue_contact.persisted? + @added_count += 1 + else # The validation ensures that the id exists and the user has permission errors << "#{contact_id}: The resource that you are attempting to access does not exist or you don't have permission to perform this action" end @@ -81,7 +87,7 @@ module Issues def remove_by_id(contact_ids) contact_ids &= existing_ids - issue.issue_customer_relations_contacts + @removed_count += issue.issue_customer_relations_contacts .where(contact_id: contact_ids) # rubocop: disable CodeReuse/ActiveRecord .delete_all end @@ -129,6 +135,11 @@ module Issues params[:add_emails] && params[:add_emails].length > MAX_ADDITIONAL_CONTACTS end + def create_system_note + SystemNoteService.change_issuable_contacts( + issue, issue.project, current_user, added_count, removed_count) + end + def error_no_permissions ServiceResponse.error(message: _('You have insufficient permissions to set customer relations contacts for this issue')) end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 7a7aea3fa4c..1f1edad7a69 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -45,6 +45,10 @@ module SystemNoteService ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_reviewers(old_reviewers) end + def change_issuable_contacts(issuable, project, author, added_count, removed_count) + ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_contacts(added_count, removed_count) + end + def relate_issue(noteable, noteable_ref, user) ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).relate_issue(noteable_ref) end diff --git a/app/services/system_notes/issuables_service.rb b/app/services/system_notes/issuables_service.rb index d33dcd65589..09f36bb6501 100644 --- a/app/services/system_notes/issuables_service.rb +++ b/app/services/system_notes/issuables_service.rb @@ -111,6 +111,35 @@ module SystemNotes create_note(NoteSummary.new(noteable, project, author, body, action: 'reviewer')) end + # Called when the contacts of an issuable are changed or removed + # We intend to reference the contacts but for security we are just + # going to state how many were added/removed for now. See discussion: + # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77816#note_806114273 + # + # added_count - number of contacts added, or 0 + # removed_count - number of contacts removed, or 0 + # + # Example Note text: + # + # "added 2 contacts" + # + # "added 3 contacts and removed one contact" + # + # Returns the created Note object + def change_issuable_contacts(added_count, removed_count) + text_parts = [] + + Gitlab::I18n.with_default_locale do + text_parts << "added #{added_count} #{'contact'.pluralize(added_count)}" if added_count > 0 + text_parts << "removed #{removed_count} #{'contact'.pluralize(removed_count)}" if removed_count > 0 + end + + return if text_parts.empty? + + body = text_parts.join(' and ') + create_note(NoteSummary.new(noteable, project, author, body, action: 'contact')) + end + # Called when the title of a Noteable is changed # # old_title - Previous String title diff --git a/config/feature_flags/development/track_application_boot_time.yml b/config/feature_flags/ops/elastic_migration_worker.yml index 2a2b5eba866..7be2f181edb 100644 --- a/config/feature_flags/development/track_application_boot_time.yml +++ b/config/feature_flags/ops/elastic_migration_worker.yml @@ -1,8 +1,8 @@ --- -name: track_application_boot_time -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79139 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/351769 +name: elastic_migration_worker +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/80310 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/352424 milestone: '14.8' -type: development -group: group::memory +type: ops +group: group::global search default_enabled: true diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index f8d2c68f97c..db8c796f960 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -4310,6 +4310,28 @@ Input type: `SecurityPolicyProjectUnassignInput` | <a id="mutationsecuritypolicyprojectunassignclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | <a id="mutationsecuritypolicyprojectunassignerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +### `Mutation.securityTrainingUpdate` + +Input type: `SecurityTrainingUpdateInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationsecuritytrainingupdateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationsecuritytrainingupdateisenabled"></a>`isEnabled` | [`Boolean!`](#boolean) | Sets the training provider as enabled for the project. | +| <a id="mutationsecuritytrainingupdateisprimary"></a>`isPrimary` | [`Boolean`](#boolean) | Sets the training provider as primary for the project. | +| <a id="mutationsecuritytrainingupdateprojectpath"></a>`projectPath` | [`ID!`](#id) | Full path of the project. | +| <a id="mutationsecuritytrainingupdateproviderid"></a>`providerId` | [`SecurityTrainingProviderID!`](#securitytrainingproviderid) | ID of the provider. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationsecuritytrainingupdateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationsecuritytrainingupdateerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| <a id="mutationsecuritytrainingupdatetraining"></a>`training` | [`ProjectSecurityTraining`](#projectsecuritytraining) | Represents the training entity subject to mutation. | + ### `Mutation.terraformStateDelete` Input type: `TerraformStateDeleteInput` @@ -18644,6 +18666,12 @@ A `ReleasesLinkID` is a global ID. It is encoded as a string. An example `ReleasesLinkID` is: `"gid://gitlab/Releases::Link/1"`. +### `SecurityTrainingProviderID` + +A `SecurityTrainingProviderID` is a global ID. It is encoded as a string. + +An example `SecurityTrainingProviderID` is: `"gid://gitlab/Security::TrainingProvider/1"`. + ### `SnippetID` A `SnippetID` is a global ID. It is encoded as a string. diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index e4f7e9126dc..a54ea33aca8 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -1458,14 +1458,6 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://git Accept and merge changes submitted with MR using this API. -If a merge request is unable to be accepted (such as Draft, Closed, Pipeline Pending Completion, or Failed while requiring Success) - you receive a `405` and the error message 'Method Not Allowed' - -If it has some conflicts and can not be merged - you receive a `406` and the error message 'Branch cannot be merged' - -If the `sha` parameter is passed and does not match the HEAD of the source - you receive a `409` and the error message 'SHA does not match HEAD of source branch' - -If you don't have permissions to accept this merge request - you receive a `401` - ```plaintext PUT /projects/:id/merge_requests/:merge_request_iid/merge ``` @@ -1631,7 +1623,16 @@ the `approvals_before_merge` parameter: } ``` -For important notes on response data, read [Single merge request response notes](#single-merge-request-response-notes). +This API returns specific HTTP status codes on failure: + +| HTTP Status | Message | Reason | +| :---: | ------- | ------ | +| `401` | `Unauthorized` | This user does not have permission to accept this merge request. | +| `405` | `Method Not Allowed` | The merge request cannot be accepted because it is `Draft`, `Closed`, `Pipeline Pending Completion`, or `Failed`. `Success` is required. | +| `406` | `Branch cannot be merged` | The branch has conflicts and cannot be merged. | +| `409` | `SHA does not match HEAD of source branch` | The provided `sha` parameter does not match the HEAD of the source. | + +For additional important notes on response data, read [Single merge request response notes](#single-merge-request-response-notes). ## Merge to default merge ref path diff --git a/doc/development/code_review.md b/doc/development/code_review.md index bdfda0252a2..3664ca7642a 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -673,7 +673,7 @@ Properties of customer critical merge requests: - It is required that the reviewer(s) and maintainer(s) involved with a customer critical merge request are engaged as soon as this decision is made. - It is required to prioritize work for those involved on a customer critical merge request so that they have the time available necessary to focus on it. - It is required to adhere to GitLab [values](https://about.gitlab.com/handbook/values/) and processes when working on customer critical merge requests, taking particular note of family and friends first/work second, definition of done, iteration, and release when it's ready. -- Customer critical merge requests are required to not reduce security, introduce data-loss risk, reduce availability, nor break existing functionality per the process for [prioritizing technical decisions](https://about.gitlab.com/handbook/engineering/#prioritizing-technical-decisions.md). +- Customer critical merge requests are required to not reduce security, introduce data-loss risk, reduce availability, nor break existing functionality per the process for [prioritizing technical decisions](https://about.gitlab.com/handbook/engineering/principles/#prioritizing-technical-decisions). - On customer critical requests, it is _recommended_ that those involved _consider_ coordinating synchronously (Zoom, Slack) in addition to asynchronously (merge requests comments) if they believe this may reduce the elapsed time to merge even though this _may_ sacrifice [efficiency](https://about.gitlab.com/company/culture/all-remote/asynchronous/#evaluating-efficiency.md). - After a customer critical merge request is merged, a retrospective must be completed with the intention of reducing the frequency of future customer critical merge requests. diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md index ce582ddce53..ad8403d242c 100644 --- a/doc/development/contributing/issue_workflow.md +++ b/doc/development/contributing/issue_workflow.md @@ -57,6 +57,10 @@ Most issues will have labels for at least one of the following: - Priority: `~"priority::1"`, `~"priority::2"`, `~"priority::3"`, `~"priority::4"` - Severity: ~`"severity::1"`, `~"severity::2"`, `~"severity::3"`, `~"severity::4"` +Please add `~"breaking change"` label if the issue can be considered as a [breaking change](index.md#breaking-changes). + +Please add `~security` label if the issue is related to application security. + All labels, their meaning and priority are defined on the [labels page](https://gitlab.com/gitlab-org/gitlab/-/labels). diff --git a/doc/development/dangerbot.md b/doc/development/dangerbot.md index 374e4e5de68..8da1f5700e5 100644 --- a/doc/development/dangerbot.md +++ b/doc/development/dangerbot.md @@ -66,7 +66,7 @@ continue to apply. However, there are a few things that deserve special emphasis Danger is a powerful tool and flexible tool, but not always the most appropriate way to solve a given problem or workflow. -First, be aware of the GitLab [commitment to dogfooding](https://about.gitlab.com/handbook/engineering/#dogfooding). +First, be aware of the GitLab [commitment to dogfooding](https://about.gitlab.com/handbook/engineering/principles/#dogfooding). The code we write for Danger is GitLab-specific, and it **may not** be most appropriate place to implement functionality that addresses a need we encounter. Our users, customers, and even our own satellite projects, such as [Gitaly](https://gitlab.com/gitlab-org/gitaly), diff --git a/doc/development/deprecation_guidelines/index.md b/doc/development/deprecation_guidelines/index.md index 27c29a1ed7c..08e29e373f6 100644 --- a/doc/development/deprecation_guidelines/index.md +++ b/doc/development/deprecation_guidelines/index.md @@ -6,8 +6,8 @@ info: To determine the technical writer assigned to the Stage/Group associated w # Deprecation guidelines -This page includes information about how and when to remove or make breaking -changes to GitLab features. +This page includes information about how and when to remove or make [breaking +changes](../contributing/index.md#breaking-changes) to GitLab features. ## Terminology diff --git a/doc/development/fe_guide/design_anti_patterns.md b/doc/development/fe_guide/design_anti_patterns.md index 9e602b1ea04..76825d6ff18 100644 --- a/doc/development/fe_guide/design_anti_patterns.md +++ b/doc/development/fe_guide/design_anti_patterns.md @@ -9,7 +9,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w Anti-patterns may seem like good approaches at first, but it has been shown that they bring more ills than benefits. These should generally be avoided. -Throughout the GitLab codebase, there may be historic uses of these anti-patterns. Please [use discretion](https://about.gitlab.com/handbook/engineering/#balance-refactoring-and-velocity) +Throughout the GitLab codebase, there may be historic uses of these anti-patterns. Please [use discretion](https://about.gitlab.com/handbook/engineering/principles/#balance-refactoring-and-velocity) when figuring out whether or not to refactor, when touching code that uses one of these legacy patterns. NOTE: diff --git a/doc/development/pipelines.md b/doc/development/pipelines.md index df9acd48f2b..7011b3c6ef1 100644 --- a/doc/development/pipelines.md +++ b/doc/development/pipelines.md @@ -12,7 +12,7 @@ which itself includes files under [`.gitlab/ci/`](https://gitlab.com/gitlab-org/gitlab/-/tree/master/.gitlab/ci) for easier maintenance. -We're striving to [dogfood](https://about.gitlab.com/handbook/engineering/#dogfooding) +We're striving to [dogfood](https://about.gitlab.com/handbook/engineering/principles/#dogfooding) GitLab [CI/CD features and best-practices](../ci/yaml/index.md) as much as possible. diff --git a/doc/development/ruby_upgrade.md b/doc/development/ruby_upgrade.md index 2102a256645..a208a93e300 100644 --- a/doc/development/ruby_upgrade.md +++ b/doc/development/ruby_upgrade.md @@ -272,4 +272,4 @@ and merged back independently. - **Give yourself enough time to fix problems ahead of a milestone release.** GitLab moves fast. As a Ruby upgrade requires many MRs to be sent and reviewed, make sure all changes are merged at least a week before the 22nd. This gives us extra time to act if something breaks. If in doubt, it is better to -postpone the upgrade to the following month, as we [prioritize availability over velocity](https://about.gitlab.com/handbook/engineering/#prioritizing-technical-decisions). +postpone the upgrade to the following month, as we [prioritize availability over velocity](https://about.gitlab.com/handbook/engineering/principles/#prioritizing-technical-decisions). diff --git a/doc/development/testing_guide/end_to_end/rspec_metadata_tests.md b/doc/development/testing_guide/end_to_end/rspec_metadata_tests.md index bb214976926..07f7e9582ee 100644 --- a/doc/development/testing_guide/end_to_end/rspec_metadata_tests.md +++ b/doc/development/testing_guide/end_to_end/rspec_metadata_tests.md @@ -20,6 +20,7 @@ This is a partial list of the [RSpec metadata](https://relishapp.com/rspec/rspec | `:github` | The test requires a GitHub personal access token. | | `:group_saml` | The test requires a GitLab instance that has SAML SSO enabled at the group level. Interacts with an external SAML identity provider. Paired with the `:orchestrated` tag. | | `:instance_saml` | The test requires a GitLab instance that has SAML SSO enabled at the instance level. Interacts with an external SAML identity provider. Paired with the `:orchestrated` tag. | +| `:integrations` | This aims to test the available [integrations](../../../user/project/integrations/overview.md#integrations-listing). The test requires Docker to be installed in the run context. It will provision the containers and can be run against a local instance or using the `gitlab-qa` scenario `Test::Integration::Integrations` | | `:service_ping_disabled` | The test interacts with the GitLab configuration service ping at the instance level to turn admin setting service ping checkbox on or off. This tag will have the test run only in the `service_ping_disabled` job and must be paired with the `:orchestrated` and `:requires_admin` tags. | | `:jira` | The test requires a Jira Server. [GitLab-QA](https://gitlab.com/gitlab-org/gitlab-qa) provisions the Jira Server in a Docker container when the `Test::Integration::Jira` test scenario is run. | `:kubernetes` | The test includes a GitLab instance that is configured to be run behind an SSH tunnel, allowing a TLS-accessible GitLab. This test also includes provisioning of at least one Kubernetes cluster to test against. _This tag is often be paired with `:orchestrated`._ | diff --git a/doc/user/project/members/index.md b/doc/user/project/members/index.md index 9b2c61a5b4c..a5c02f30106 100644 --- a/doc/user/project/members/index.md +++ b/doc/user/project/members/index.md @@ -215,9 +215,6 @@ On self-managed GitLab, by default this feature is available. To hide the feature, ask an administrator to [disable the feature flag](#enable-or-disable-modal-window). On GitLab.com, this feature is available. -In GitLab 13.11, you can optionally replace the form to add a member with a modal window. -To add a member after enabling this feature: - 1. On the top bar, select **Menu > Projects** and find your project. 1. On the left sidebar, select **Project information > Members**. 1. Select **Invite members**. diff --git a/doc/user/project/merge_requests/approvals/rules.md b/doc/user/project/merge_requests/approvals/rules.md index d94bcccd855..fa447ea35af 100644 --- a/doc/user/project/merge_requests/approvals/rules.md +++ b/doc/user/project/merge_requests/approvals/rules.md @@ -127,8 +127,8 @@ users were not explicitly listed in the approval rules. ### Group approvers You can add a group of users as approvers, but those users count as approvers only if -they have direct membership to the group. In the future, group approvers may be -restricted to only groups [with share access to the project](https://gitlab.com/gitlab-org/gitlab/-/issues/2048). +they have direct membership to the group. Group approvers are +restricted to only groups [with share access to the project](../../members/share_project_with_groups.md). A user's membership in an approvers group affects their individual ability to approve in these ways: diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb index 0709a8c2036..f73e4b621ab 100644 --- a/lib/api/discussions.rb +++ b/lib/api/discussions.rb @@ -252,7 +252,7 @@ module API .fresh # Without RendersActions#prepare_notes_for_rendering, - # Note#system_note_with_references_visible_for? will attempt to render + # Note#system_note_visible_for? will attempt to render # Markdown references mentioned in the note to see whether they # should be redacted. For notes that reference a commit, this # would also incur a Gitaly call to verify the commit exists. diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index 4c98941e032..2b190d89fa4 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -158,7 +158,7 @@ module Gitlab # See more detail in the docs: https://docs.gitlab.com/ee/ci/variables/#cicd-variable-precedence variables.concat(project.predefined_variables) variables.concat(pipeline.predefined_variables) if pipeline - variables.concat(project.ci_instance_variables_for(ref: source_ref_path)) + variables.concat(secret_variables(project: project, pipeline: pipeline)) variables.concat(project.group.ci_variables_for(source_ref_path, project)) if project.group variables.concat(project.ci_variables_for(ref: source_ref_path)) variables.concat(pipeline.variables) if pipeline @@ -166,6 +166,14 @@ module Gitlab end end + def secret_variables(project:, pipeline:) + if pipeline + pipeline.variables_builder.secret_instance_variables + else + Gitlab::Ci::Variables::Builder::Instance.new.secret_variables + end + end + def track_and_raise_for_dev_exception(error) Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error, @context.sentry_payload) end diff --git a/lib/gitlab/ci/variables/builder.rb b/lib/gitlab/ci/variables/builder.rb index e5b8058a1f6..90b84803cff 100644 --- a/lib/gitlab/ci/variables/builder.rb +++ b/lib/gitlab/ci/variables/builder.rb @@ -8,6 +8,7 @@ module Gitlab def initialize(pipeline) @pipeline = pipeline + @instance_variables_builder = Builder::Instance.new end def scoped_variables(job, environment:, dependencies:) @@ -21,7 +22,7 @@ module Gitlab variables.concat(job.yaml_variables) variables.concat(user_variables(job.user)) variables.concat(job.dependency_variables) if dependencies - variables.concat(secret_instance_variables(ref: job.git_ref)) + variables.concat(secret_instance_variables) variables.concat(secret_group_variables(environment: environment, ref: job.git_ref)) variables.concat(secret_project_variables(environment: environment, ref: job.git_ref)) variables.concat(job.trigger_request.user_variables) if job.trigger_request @@ -62,8 +63,11 @@ module Gitlab end end - def secret_instance_variables(ref:) - project.ci_instance_variables_for(ref: ref) + def secret_instance_variables + strong_memoize(:secret_instance_variables) do + instance_variables_builder + .secret_variables(protected_ref: protected_ref?) + end end def secret_group_variables(environment:, ref:) @@ -79,6 +83,7 @@ module Gitlab private attr_reader :pipeline + attr_reader :instance_variables_builder delegate :project, to: :pipeline def predefined_variables(job) @@ -104,6 +109,12 @@ module Gitlab parallel = parallel.dig(:total) if parallel.is_a?(Hash) parallel || 1 end + + def protected_ref? + strong_memoize(:protected_ref) do + project.protected_for?(pipeline.jobs_git_ref) + end + end end end end diff --git a/lib/gitlab/ci/variables/builder/instance.rb b/lib/gitlab/ci/variables/builder/instance.rb new file mode 100644 index 00000000000..db4d54b9692 --- /dev/null +++ b/lib/gitlab/ci/variables/builder/instance.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Variables + class Builder + class Instance + include Gitlab::Utils::StrongMemoize + + def secret_variables(protected_ref: false) + variables = if protected_ref + ::Ci::InstanceVariable.all_cached + else + ::Ci::InstanceVariable.unprotected_cached + end + + Gitlab::Ci::Variables::Collection.new(variables) + end + end + end + end + end +end diff --git a/lib/gitlab/metrics/boot_time_tracker.rb b/lib/gitlab/metrics/boot_time_tracker.rb index d0eb66f293b..3e7026b8dea 100644 --- a/lib/gitlab/metrics/boot_time_tracker.rb +++ b/lib/gitlab/metrics/boot_time_tracker.rb @@ -15,7 +15,7 @@ module Gitlab return if @startup_time runtime = Gitlab::Runtime.safe_identify - return unless SUPPORTED_RUNTIMES.include?(runtime) && Feature.enabled?(:track_application_boot_time, default_enabled: :yaml) + return unless SUPPORTED_RUNTIMES.include?(runtime) @startup_time = Gitlab::Metrics::System.process_runtime_elapsed_seconds diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 76f157581cd..c1795606199 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -6492,6 +6492,9 @@ msgstr "" msgid "Can create groups:" msgstr "" +msgid "Can not delete primary training" +msgstr "" + msgid "Can't apply as the source branch was deleted." msgstr "" @@ -13619,6 +13622,9 @@ msgstr "" msgid "Enforce two-factor authentication for all user sign-ins." msgstr "" +msgid "Enhance security by storing service account keys in secret managers - learn more about %{docLinkStart}secret management with GitLab%{docLinkEnd}" +msgstr "" + msgid "Ensure connectivity is available from the GitLab server to the Prometheus server" msgstr "" @@ -33032,9 +33038,6 @@ msgstr "" msgid "Service Account Key" msgstr "" -msgid "Service Accounts" -msgstr "" - msgid "Service Accounts keys authorize GitLab to deploy your Google Cloud project" msgstr "" @@ -33047,6 +33050,9 @@ msgstr "" msgid "Service account generated successfully" msgstr "" +msgid "Service accounts" +msgstr "" + msgid "Service ping is disabled in your configuration file, and cannot be enabled through this form." msgstr "" @@ -44133,3 +44139,6 @@ msgstr "" msgid "{project}" msgstr "" + +msgid "✔" +msgstr "" diff --git a/qa/qa/resource/project_web_hook.rb b/qa/qa/resource/project_web_hook.rb new file mode 100644 index 00000000000..8b806c42030 --- /dev/null +++ b/qa/qa/resource/project_web_hook.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module QA + module Resource + class ProjectWebHook < Base + EVENT_TRIGGERS = %i[ + issues + job + merge_requests + note + pipeline + push + tag_push + wiki_page + confidential_issues + confidential_note + ].freeze + + attr_accessor :url, :enable_ssl, :id + + attribute :project do + Project.fabricate_via_api! do |resource| + resource.name = 'project-with-webhooks' + end + end + + EVENT_TRIGGERS.each do |trigger| + attribute "#{trigger}_events".to_sym do + false + end + end + + def initialize + @id = nil + @enable_ssl = false + @url = nil + end + + def resource_web_url(resource) + "/project/#{project.name}/~/hooks/##{resource[:id]}/edit" + end + + def api_get_path + "/projects/#{project.id}/hooks" + end + + def api_post_path + api_get_path + end + + def api_post_body + body = { + id: project.id, + url: url, + enable_ssl_verification: enable_ssl + } + EVENT_TRIGGERS.each_with_object(body) do |trigger, memo| + attr = "#{trigger}_events" + memo[attr.to_sym] = send(attr) + memo + end + end + end + end +end diff --git a/qa/qa/runtime/env.rb b/qa/qa/runtime/env.rb index 3e442814734..c50e02a2aaa 100644 --- a/qa/qa/runtime/env.rb +++ b/qa/qa/runtime/env.rb @@ -295,6 +295,14 @@ module QA ENV['JIRA_HOSTNAME'] end + # this is set by the integrations job + # which will allow bidirectional communication + # between the app and the specs container + # should the specs container spin up a server + def qa_hostname + ENV['QA_HOSTNAME'] + end + def cache_namespace_name? enabled?(ENV['CACHE_NAMESPACE_NAME'], default: true) end diff --git a/qa/qa/service/docker_run/smocker.rb b/qa/qa/service/docker_run/smocker.rb new file mode 100644 index 00000000000..83ab58887da --- /dev/null +++ b/qa/qa/service/docker_run/smocker.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module QA + module Service + module DockerRun + class Smocker < Base + def initialize + @image = 'thiht/smocker:0.17.1' + @name = 'smocker-server' + @public_port = '8080' + @admin_port = '8081' + super + @network_cache = network + end + + def host_name + return '127.0.0.1' unless QA::Runtime::Env.running_in_ci? || QA::Runtime::Env.qa_hostname + + "#{@name}.#{@network_cache}" + end + + def base_url + "http://#{host_name}:#{@public_port}" + end + + def admin_url + "http://#{host_name}:#{@admin_port}" + end + + def wait_for_running + Support::Waiter.wait_until(raise_on_failure: false, reload_page: false) do + running? + end + end + + def register! + command = <<~CMD.tr("\n", ' ') + docker run -d --rm + --network #{@network_cache} + --hostname #{host_name} + --name #{@name} + --publish #{@public_port}:8080 + --publish #{@admin_port}:8081 + #{@image} + CMD + + unless QA::Runtime::Env.running_in_ci? || QA::Runtime::Env.qa_hostname + command.gsub!("--network #{@network_cache} ", '') + end + + shell command + end + end + end + end +end diff --git a/qa/qa/specs/features/api/3_create/integrations/webhook_events_spec.rb b/qa/qa/specs/features/api/3_create/integrations/webhook_events_spec.rb new file mode 100644 index 00000000000..7a277d754c9 --- /dev/null +++ b/qa/qa/specs/features/api/3_create/integrations/webhook_events_spec.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +module QA + RSpec.describe 'Create' do + describe 'WebHooks integration', :requires_admin, :integrations, :orchestrated do + before(:context) do + toggle_local_requests(true) + end + + after(:context) do + Vendor::Smocker::SmockerApi.teardown! + end + + let(:session) { SecureRandom.hex(5) } + + it 'sends a push event', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/348945' do + setup_webhook(push: true) do |webhook, smocker| + Resource::Repository::ProjectPush.fabricate! do |project_push| + project_push.project = webhook.project + end + + wait_until do + !smocker.history(session).empty? + end + + events = smocker.history(session).map(&:as_hook_event) + aggregate_failures do + expect(events.size).to be(1), "Should have 1 event: \n#{events.map(&:raw).join("\n")}" + expect(events[0].project_name).to eql(webhook.project.name) + expect(events[0].push?).to be(true), "Not push event: \n#{events[0].raw}" + end + end + end + + it 'sends a merge request event', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/349720' do + setup_webhook(merge_requests: true) do |webhook, smocker| + Resource::MergeRequest.fabricate_via_api! do |merge_request| + merge_request.project = webhook.project + end + + wait_until do + !smocker.history(session).empty? + end + + events = smocker.history(session).map(&:as_hook_event) + aggregate_failures do + expect(events.size).to be(1), "Should have 1 event: \n#{events.map(&:raw).join("\n")}" + expect(events[0].project_name).to eql(webhook.project.name) + expect(events[0].mr?).to be(true), "Not MR event: \n#{events[0].raw}" + end + end + end + + it 'sends a wiki page event', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/349722' do + setup_webhook(wiki_page: true) do |webhook, smocker| + Resource::Wiki::ProjectPage.fabricate_via_api! do |page| + page.project = webhook.project + end + + wait_until do + !smocker.history(session).empty? + end + + events = smocker.history(session).map(&:as_hook_event) + aggregate_failures do + expect(events.size).to be(1), "Should have 1 event: \n#{events.map(&:raw).join("\n")}" + expect(events[0].project_name).to eql(webhook.project.name) + expect(events[0].wiki?).to be(true), "Not wiki event: \n#{events[0].raw}" + end + end + end + + it 'sends an issues and note event', 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/349723' do + setup_webhook(issues: true, note: true) do |webhook, smocker| + issue = Resource::Issue.fabricate_via_api! do |issue_init| + issue_init.project = webhook.project + end + + Resource::ProjectIssueNote.fabricate_via_api! do |note| + note.project = issue.project + note.issue = issue + end + + wait_until do + smocker.history(session).size > 1 + end + + events = smocker.history(session).map(&:as_hook_event) + aggregate_failures do + issue_event = events.find(&:issue?) + note_event = events.find(&:note?) + + expect(events.size).to be(2), "Should have 2 events: \n#{events.map(&:raw).join("\n")}" + expect(issue_event).not_to be(nil), "Not issue event: \n#{events[0].raw}" + expect(note_event).not_to be(nil), "Not note event: \n#{events[1].raw}" + end + end + end + + private + + def setup_webhook(**event_args) + Vendor::Smocker::SmockerApi.init(wait: 10) do |smocker| + smocker.register(session: session) + + webhook = Resource::ProjectWebHook.fabricate_via_api! do |hook| + hook.url = smocker.url + + event_args.each do |event, bool| + hook.send("#{event}_events=", bool) + end + end + + yield(webhook, smocker) + + smocker.reset + end + end + + def toggle_local_requests(on) + Runtime::ApplicationSettings.set_application_settings(allow_local_requests_from_web_hooks_and_services: on) + end + + def wait_until(timeout = 120, &block) + Support::Waiter.wait_until(max_duration: timeout, reload_page: false, raise_on_failure: false, &block) + end + end + end +end diff --git a/qa/qa/vendor/smocker/event_payload.rb b/qa/qa/vendor/smocker/event_payload.rb new file mode 100644 index 00000000000..4bf154b76c2 --- /dev/null +++ b/qa/qa/vendor/smocker/event_payload.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module QA + module Vendor + module Smocker + class EventPayload + def initialize(hook_data) + @hook_data = hook_data + end + + def raw + @hook_data + end + + def event + raw[:object_kind]&.to_sym + end + + def project_name + raw.dig(:project, :name) + end + + def mr? + event == :merge_request + end + + def issue? + event == :issue + end + + def note? + event == :note + end + + def push? + event == :push + end + + def tag? + event == :tag + end + + def wiki? + event == :wiki_page + end + end + end + end +end diff --git a/qa/qa/vendor/smocker/history_response.rb b/qa/qa/vendor/smocker/history_response.rb new file mode 100644 index 00000000000..53d5759ef8b --- /dev/null +++ b/qa/qa/vendor/smocker/history_response.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require_relative './event_payload' +require 'time' + +module QA + module Vendor + module Smocker + class HistoryResponse + attr_reader :payload + + def initialize(payload) + @payload = payload + end + + # Smocker context including call counter + def context + payload[:context] + end + + # Smocker request data + def request + payload[:request] + end + + # @return [EventPayload] the request body as a webhook event + def as_hook_event + body = request&.dig(:body) + EventPayload.new body if body + end + + # @return [Time] Time request was recieved + def received + date = request&.dig(:date) + Time.parse date if date + end + + # Find time elapsed since <target> + # + # @param target [Time] target time + # @return [Integer] seconds elapsed since <target> + def elapsed(target) + (received.to_f - target.to_f).round if received + end + + def response + payload[:response] + end + end + end + end +end diff --git a/qa/qa/vendor/smocker/smocker_api.rb b/qa/qa/vendor/smocker/smocker_api.rb new file mode 100644 index 00000000000..3f595b58886 --- /dev/null +++ b/qa/qa/vendor/smocker/smocker_api.rb @@ -0,0 +1,140 @@ +# frozen_string_literal: true + +module QA + module Vendor + module Smocker + class SmockerApi + include Scenario::Actable + include Support::API + + DEFAULT_MOCK = <<~YAML + - request: + method: POST + path: /default + response: + headers: + Content-Type: application/json + body: '{}' + YAML + + # @param wait [Integer] seconds to wait for server + # @yieldparam [SmockerApi] the api object ready for interaction + def self.init(**wait_args) + if @container.nil? + @container = Service::DockerRun::Smocker.new + @container.register! + @container.wait_for_running + end + + yield new(@container, **wait_args) + end + + def self.teardown! + @container&.remove! + end + + def initialize(container, **wait_args) + @container = container + wait_for_ready(**wait_args) + end + + # @return [String] Base url of mock endpoint + def base_url + @container.base_url + end + + # @return [String] Url of admin endpoint + def admin_url + @container.admin_url + end + + # @param endpoint [String] path for mock endpoint + # @return [String] url for mock endpoint + def url(endpoint = 'default') + "#{base_url}/#{endpoint}" + end + + # Waits for the smocker server to be ready + # + # @param wait [Integer] wait duration for smocker readiness + def wait_for_ready(wait: 10) + Support::Waiter.wait_until(max_duration: wait, reload_page: false, raise_on_failure: true) do + ready? + end + end + + # Is smocker server ready for interaction? + # + # @return [Boolean] + def ready? + QA::Runtime::Logger.debug 'Checking Smocker readiness' + get("#{admin_url}/version") + true + # rescuing StandardError because RestClient::ExceptionWithResponse isn't propagating + rescue StandardError => e + QA::Runtime::Logger.debug "Smocker not ready yet \n #{e}" + false + end + + # Clears mocks and history + # + # @param force [Boolean] remove locked mocks? + # @return [Boolean] reset was successful? + def reset(force: true) + response = post("#{admin_url}/reset?force=#{force}", {}.to_json) + parse_body(response)['message'] == 'Reset successful' + end + + # Fetches an active session id from a name + # + # @param name [String] the name of the session + # @return [String] the unique session id + def get_session_id(name) + sessions = parse_body get("#{admin_url}/sessions/summary") + current = sessions.find do |session| + session[:name] == name + end + current&.dig(:id) + end + + # Registers a mock to Smocker + # If a session name is provided, the mock will register to that session + # https://smocker.dev/technical-documentation/mock-definition.html + # + # @param yaml [String] the yaml representing the mock + # @param session [String] the session name for the mock + def register(yaml = DEFAULT_MOCK, session: nil) + query_params = build_params(session: session) + url = "#{admin_url}/mocks?#{query_params}" + headers = { 'Content-Type' => 'application/x-yaml' } + response = post(url, yaml, headers: headers) + parse_body(response) + end + + # Fetches call history for a mock + # + # @param session_name [String] the session name for the mock + # @return [Array<HistoryResponse>] + def history(session_name = nil) + query_params = session_name ? build_params(session: get_session_id(session_name)) : '' + response = get("#{admin_url}/history?#{query_params}") + body = parse_body(response) + + raise body[:message] unless body.is_a?(Array) + + body.map do |entry| + HistoryResponse.new(entry) + end + end + + private + + def build_params(**args) + args.each_with_object([]) do |(k, v), memo| + memo << "#{k}=#{v}" if v + end.join("&") + end + end + end + end +end diff --git a/spec/frontend/behaviors/copy_as_gfm_spec.js b/spec/frontend/behaviors/copy_as_gfm_spec.js index 557b609f5f9..c96db09cc76 100644 --- a/spec/frontend/behaviors/copy_as_gfm_spec.js +++ b/spec/frontend/behaviors/copy_as_gfm_spec.js @@ -1,3 +1,4 @@ +import waitForPromises from 'helpers/wait_for_promises'; import initCopyAsGFM, { CopyAsGFM } from '~/behaviors/markdown/copy_as_gfm'; describe('CopyAsGFM', () => { @@ -81,49 +82,40 @@ describe('CopyAsGFM', () => { stopPropagation() {}, }; CopyAsGFM.copyAsGFM(e, CopyAsGFM.transformGFMSelection); - return clipboardData; + + return waitForPromises(); }; - beforeAll((done) => { + beforeAll(() => { initCopyAsGFM(); // Fake call to nodeToGfm so the import of lazy bundle happened - CopyAsGFM.nodeToGFM(document.createElement('div')) - .then(() => { - done(); - }) - .catch(done.fail); + return CopyAsGFM.nodeToGFM(document.createElement('div')); }); beforeEach(() => jest.spyOn(clipboardData, 'setData')); describe('list handling', () => { - it('uses correct gfm for unordered lists', (done) => { + it('uses correct gfm for unordered lists', async () => { const selection = stubSelection('<li>List Item1</li><li>List Item2</li>\n', 'UL'); window.getSelection = jest.fn(() => selection); - simulateCopy(); + await simulateCopy(); - setImmediate(() => { - const expectedGFM = '* List Item1\n* List Item2'; + const expectedGFM = '* List Item1\n* List Item2'; - expect(clipboardData.setData).toHaveBeenCalledWith('text/x-gfm', expectedGFM); - done(); - }); + expect(clipboardData.setData).toHaveBeenCalledWith('text/x-gfm', expectedGFM); }); - it('uses correct gfm for ordered lists', (done) => { + it('uses correct gfm for ordered lists', async () => { const selection = stubSelection('<li>List Item1</li><li>List Item2</li>\n', 'OL'); window.getSelection = jest.fn(() => selection); - simulateCopy(); + await simulateCopy(); - setImmediate(() => { - const expectedGFM = '1. List Item1\n1. List Item2'; + const expectedGFM = '1. List Item1\n1. List Item2'; - expect(clipboardData.setData).toHaveBeenCalledWith('text/x-gfm', expectedGFM); - done(); - }); + expect(clipboardData.setData).toHaveBeenCalledWith('text/x-gfm', expectedGFM); }); }); }); @@ -131,10 +123,9 @@ describe('CopyAsGFM', () => { describe('CopyAsGFM.quoted', () => { const sampleGFM = '* List 1\n* List 2\n\n`Some code`'; - it('adds quote char `> ` to each line', (done) => { + it('adds quote char `> ` to each line', () => { const expectedQuotedGFM = '> * List 1\n> * List 2\n> \n> `Some code`'; expect(CopyAsGFM.quoted(sampleGFM)).toEqual(expectedQuotedGFM); - done(); }); }); }); diff --git a/spec/frontend/behaviors/shortcuts/shortcuts_issuable_spec.js b/spec/frontend/behaviors/shortcuts/shortcuts_issuable_spec.js index bb3b16b4c7a..e1811247124 100644 --- a/spec/frontend/behaviors/shortcuts/shortcuts_issuable_spec.js +++ b/spec/frontend/behaviors/shortcuts/shortcuts_issuable_spec.js @@ -1,5 +1,6 @@ import $ from 'jquery'; import Mousetrap from 'mousetrap'; +import waitForPromises from 'helpers/wait_for_promises'; import initCopyAsGFM, { CopyAsGFM } from '~/behaviors/markdown/copy_as_gfm'; import ShortcutsIssuable from '~/behaviors/shortcuts/shortcuts_issuable'; import { getSelectedFragment } from '~/lib/utils/common_utils'; @@ -13,15 +14,11 @@ describe('ShortcutsIssuable', () => { const snippetShowFixtureName = 'snippets/show.html'; const mrShowFixtureName = 'merge_requests/merge_request_of_current_user.html'; - beforeAll((done) => { + beforeAll(() => { initCopyAsGFM(); // Fake call to nodeToGfm so the import of lazy bundle happened - CopyAsGFM.nodeToGFM(document.createElement('div')) - .then(() => { - done(); - }) - .catch(done.fail); + return CopyAsGFM.nodeToGFM(document.createElement('div')); }); describe('replyWithSelectedText', () => { @@ -79,22 +76,18 @@ describe('ShortcutsIssuable', () => { stubSelection('<p>Selected text.</p>'); }); - it('leaves existing input intact', (done) => { + it('leaves existing input intact', async () => { $(FORM_SELECTOR).val('This text was already here.'); expect($(FORM_SELECTOR).val()).toBe('This text was already here.'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe( - 'This text was already here.\n\n> Selected text.\n\n', - ); - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe('This text was already here.\n\n> Selected text.\n\n'); }); - it('triggers `input`', (done) => { + it('triggers `input`', async () => { let triggered = false; $(FORM_SELECTOR).on('input', () => { triggered = true; @@ -102,48 +95,40 @@ describe('ShortcutsIssuable', () => { ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(triggered).toBe(true); - done(); - }); + await waitForPromises(); + expect(triggered).toBe(true); }); - it('triggers `focus`', (done) => { + it('triggers `focus`', async () => { const spy = jest.spyOn(document.querySelector(FORM_SELECTOR), 'focus'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(spy).toHaveBeenCalled(); - done(); - }); + await waitForPromises(); + expect(spy).toHaveBeenCalled(); }); }); describe('with a one-line selection', () => { - it('quotes the selection', (done) => { + it('quotes the selection', async () => { stubSelection('<p>This text has been selected.</p>'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe('> This text has been selected.\n\n'); - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe('> This text has been selected.\n\n'); }); }); describe('with a multi-line selection', () => { - it('quotes the selected lines as a group', (done) => { + it('quotes the selected lines as a group', async () => { stubSelection( '<p>Selected line one.</p>\n<p>Selected line two.</p>\n<p>Selected line three.</p>', ); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe( - '> Selected line one.\n>\n> Selected line two.\n>\n> Selected line three.\n\n', - ); - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe( + '> Selected line one.\n>\n> Selected line two.\n>\n> Selected line three.\n\n', + ); }); }); @@ -152,23 +137,19 @@ describe('ShortcutsIssuable', () => { stubSelection('<p>Selected text.</p>', true); }); - it('does not add anything to the input', (done) => { + it('does not add anything to the input', async () => { ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe(''); - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe(''); }); - it('triggers `focus`', (done) => { + it('triggers `focus`', async () => { const spy = jest.spyOn(document.querySelector(FORM_SELECTOR), 'focus'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(spy).toHaveBeenCalled(); - done(); - }); + await waitForPromises(); + expect(spy).toHaveBeenCalled(); }); }); @@ -177,26 +158,22 @@ describe('ShortcutsIssuable', () => { stubSelection('<div class="md">Selected text.</div><p>Invalid selected text.</p>', true); }); - it('only adds the valid part to the input', (done) => { + it('only adds the valid part to the input', async () => { ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe('> Selected text.\n\n'); - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe('> Selected text.\n\n'); }); - it('triggers `focus`', (done) => { + it('triggers `focus`', async () => { const spy = jest.spyOn(document.querySelector(FORM_SELECTOR), 'focus'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(spy).toHaveBeenCalled(); - done(); - }); + await waitForPromises(); + expect(spy).toHaveBeenCalled(); }); - it('triggers `input`', (done) => { + it('triggers `input`', async () => { let triggered = false; $(FORM_SELECTOR).on('input', () => { triggered = true; @@ -204,10 +181,8 @@ describe('ShortcutsIssuable', () => { ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(triggered).toBe(true); - done(); - }); + await waitForPromises(); + expect(triggered).toBe(true); }); }); @@ -231,26 +206,22 @@ describe('ShortcutsIssuable', () => { }); }); - it('adds the quoted selection to the input', (done) => { + it('adds the quoted selection to the input', async () => { ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe('> *Selected text.*\n\n'); - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe('> *Selected text.*\n\n'); }); - it('triggers `focus`', (done) => { + it('triggers `focus`', async () => { const spy = jest.spyOn(document.querySelector(FORM_SELECTOR), 'focus'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(spy).toHaveBeenCalled(); - done(); - }); + await waitForPromises(); + expect(spy).toHaveBeenCalled(); }); - it('triggers `input`', (done) => { + it('triggers `input`', async () => { let triggered = false; $(FORM_SELECTOR).on('input', () => { triggered = true; @@ -258,10 +229,8 @@ describe('ShortcutsIssuable', () => { ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(triggered).toBe(true); - done(); - }); + await waitForPromises(); + expect(triggered).toBe(true); }); }); @@ -285,36 +254,29 @@ describe('ShortcutsIssuable', () => { }); }); - it('does not add anything to the input', (done) => { + it('does not add anything to the input', async () => { ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe(''); - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe(''); }); - it('triggers `focus`', (done) => { + it('triggers `focus`', async () => { const spy = jest.spyOn(document.querySelector(FORM_SELECTOR), 'focus'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect(spy).toHaveBeenCalled(); - done(); - }); + await waitForPromises(); + expect(spy).toHaveBeenCalled(); }); }); describe('with a valid selection with no text content', () => { - it('returns the proper markdown', (done) => { + it('returns the proper markdown', async () => { stubSelection('<img src="https://gitlab.com/logo.png" alt="logo" />'); ShortcutsIssuable.replyWithSelectedText(true); - setImmediate(() => { - expect($(FORM_SELECTOR).val()).toBe('> \n\n'); - - done(); - }); + await waitForPromises(); + expect($(FORM_SELECTOR).val()).toBe('> \n\n'); }); }); }); diff --git a/spec/frontend/blob/viewer/index_spec.js b/spec/frontend/blob/viewer/index_spec.js index 9e9f866d40c..fe55a537b89 100644 --- a/spec/frontend/blob/viewer/index_spec.js +++ b/spec/frontend/blob/viewer/index_spec.js @@ -41,34 +41,30 @@ describe('Blob viewer', () => { window.location.hash = ''; }); - it('loads source file after switching views', (done) => { + it('loads source file after switching views', async () => { document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]').click(); - setImmediate(() => { - expect( - document - .querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]') - .classList.contains('hidden'), - ).toBeFalsy(); + await axios.waitForAll(); - done(); - }); + expect( + document + .querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]') + .classList.contains('hidden'), + ).toBeFalsy(); }); - it('loads source file when line number is in hash', (done) => { + it('loads source file when line number is in hash', async () => { window.location.hash = '#L1'; new BlobViewer(); - setImmediate(() => { - expect( - document - .querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]') - .classList.contains('hidden'), - ).toBeFalsy(); + await axios.waitForAll(); - done(); - }); + expect( + document + .querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]') + .classList.contains('hidden'), + ).toBeFalsy(); }); it('doesnt reload file if already loaded', () => { @@ -123,24 +119,20 @@ describe('Blob viewer', () => { expect(copyButton.blur).not.toHaveBeenCalled(); }); - it('enables after switching to simple view', (done) => { + it('enables after switching to simple view', async () => { document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]').click(); - setImmediate(() => { - expect(copyButton.classList.contains('disabled')).toBeFalsy(); + await axios.waitForAll(); - done(); - }); + expect(copyButton.classList.contains('disabled')).toBeFalsy(); }); - it('updates tooltip after switching to simple view', (done) => { + it('updates tooltip after switching to simple view', async () => { document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]').click(); - setImmediate(() => { - expect(copyButtonTooltip.getAttribute('title')).toBe('Copy file contents'); + await axios.waitForAll(); - done(); - }); + expect(copyButtonTooltip.getAttribute('title')).toBe('Copy file contents'); }); }); diff --git a/spec/frontend/confirm_modal_spec.js b/spec/frontend/confirm_modal_spec.js index e5e85910945..53991349ee5 100644 --- a/spec/frontend/confirm_modal_spec.js +++ b/spec/frontend/confirm_modal_spec.js @@ -1,4 +1,3 @@ -import { nextTick } from 'vue'; import { TEST_HOST } from 'helpers/test_constants'; import initConfirmModal from '~/confirm_modal'; @@ -50,7 +49,6 @@ describe('ConfirmModal', () => { const findModal = () => document.querySelector('.gl-modal'); const findModalOkButton = (modal, variant) => modal.querySelector(`.modal-footer .btn-${variant}`); - const findModalCancelButton = (modal) => modal.querySelector('.modal-footer .btn-secondary'); const modalIsHidden = () => findModal() === null; const serializeModal = (modal, buttonIndex) => { @@ -90,19 +88,6 @@ describe('ConfirmModal', () => { expect(findModal()).not.toBe(null); expect(modalIsHidden()).toBe(false); }); - - describe('Cancel Button', () => { - beforeEach(async () => { - findModalCancelButton(findModal()).click(); - await nextTick(); - }); - - it('closes the modal', () => { - setImmediate(() => { - expect(modalIsHidden()).toBe(true); - }); - }); - }); }); }); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index b5003a54917..c53789b4800 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -544,10 +544,8 @@ describe('DiffsStoreActions', () => { [{ type: types.SET_DIFF_VIEW_TYPE, payload: INLINE_DIFF_VIEW_TYPE }], [], () => { - setImmediate(() => { - expect(Cookies.get('diff_view')).toEqual(INLINE_DIFF_VIEW_TYPE); - done(); - }); + expect(Cookies.get('diff_view')).toEqual(INLINE_DIFF_VIEW_TYPE); + done(); }, ); }); @@ -562,10 +560,8 @@ describe('DiffsStoreActions', () => { [{ type: types.SET_DIFF_VIEW_TYPE, payload: PARALLEL_DIFF_VIEW_TYPE }], [], () => { - setImmediate(() => { - expect(Cookies.get(DIFF_VIEW_COOKIE_NAME)).toEqual(PARALLEL_DIFF_VIEW_TYPE); - done(); - }); + expect(Cookies.get(DIFF_VIEW_COOKIE_NAME)).toEqual(PARALLEL_DIFF_VIEW_TYPE); + done(); }, ); }); diff --git a/spec/frontend/google_cloud/components/service_accounts_list_spec.js b/spec/frontend/google_cloud/components/service_accounts_list_spec.js index cdb3f74051c..f7051c8a53d 100644 --- a/spec/frontend/google_cloud/components/service_accounts_list_spec.js +++ b/spec/frontend/google_cloud/components/service_accounts_list_spec.js @@ -1,5 +1,5 @@ import { mount } from '@vue/test-utils'; -import { GlButton, GlEmptyState, GlTable } from '@gitlab/ui'; +import { GlAlert, GlButton, GlEmptyState, GlTable } from '@gitlab/ui'; import ServiceAccountsList from '~/google_cloud/components/service_accounts_list.vue'; describe('ServiceAccounts component', () => { @@ -28,7 +28,7 @@ describe('ServiceAccounts component', () => { it('shows the link to create new service accounts', () => { const button = findButtonInEmptyState(); expect(button.exists()).toBe(true); - expect(button.text()).toBe('Create service account'); + expect(button.text()).toBe(ServiceAccountsList.i18n.createServiceAccount); expect(button.attributes('href')).toBe('#create-url'); }); }); @@ -41,6 +41,7 @@ describe('ServiceAccounts component', () => { const findTable = () => wrapper.findComponent(GlTable); const findRows = () => findTable().findAll('tr'); const findButton = () => wrapper.findComponent(GlButton); + const findSecretManagerTip = () => wrapper.findComponent(GlAlert); beforeEach(() => { const propsData = { @@ -52,13 +53,11 @@ describe('ServiceAccounts component', () => { }); it('shows the title', () => { - expect(findTitle().text()).toBe('Service Accounts'); + expect(findTitle().text()).toBe(ServiceAccountsList.i18n.serviceAccountsTitle); }); it('shows the description', () => { - expect(findDescription().text()).toBe( - 'Service Accounts keys authorize GitLab to deploy your Google Cloud project', - ); + expect(findDescription().text()).toBe(ServiceAccountsList.i18n.serviceAccountsDescription); }); it('shows the table', () => { @@ -72,8 +71,14 @@ describe('ServiceAccounts component', () => { it('shows the link to create new service accounts', () => { const button = findButton(); expect(button.exists()).toBe(true); - expect(button.text()).toBe('Create service account'); + expect(button.text()).toBe(ServiceAccountsList.i18n.createServiceAccount); expect(button.attributes('href')).toBe('#create-url'); }); + + it('must contain secret managers tip', () => { + const tip = findSecretManagerTip(); + const expectedText = ServiceAccountsList.i18n.secretManagersDescription.substr(0, 48); + expect(tip.text()).toContain(expectedText); + }); }); }); diff --git a/spec/frontend/pages/projects/learn_gitlab/components/learn_gitlab_section_link_spec.js b/spec/frontend/pages/projects/learn_gitlab/components/learn_gitlab_section_link_spec.js index 8da3d747b54..395e5d87ac1 100644 --- a/spec/frontend/pages/projects/learn_gitlab/components/learn_gitlab_section_link_spec.js +++ b/spec/frontend/pages/projects/learn_gitlab/components/learn_gitlab_section_link_spec.js @@ -33,6 +33,8 @@ describe('Learn GitLab Section Link', () => { const openInviteMembesrModalLink = () => wrapper.find('[data-testid="invite-for-help-continuous-onboarding-experiment-link"]'); + const findUncompletedLink = () => wrapper.find('[data-testid="uncompleted-learn-gitlab-link"]'); + it('renders no icon when not completed', () => { createWrapper(undefined, { completed: false }); @@ -57,12 +59,30 @@ describe('Learn GitLab Section Link', () => { expect(wrapper.find('[data-testid="trial-only"]').exists()).toBe(true); }); - it('renders doc links with blank target', () => { - createWrapper('securityScanEnabled', docLinkProps); - const linkElement = wrapper.find('[data-testid="uncompleted-learn-gitlab-link"]'); + describe('doc links', () => { + beforeEach(() => { + createWrapper('securityScanEnabled', docLinkProps); + }); + + it('renders links with blank target', () => { + const linkElement = findUncompletedLink(); + + expect(linkElement.exists()).toBe(true); + expect(linkElement.attributes('target')).toEqual('_blank'); + }); + + it('tracks the click', () => { + const trackingSpy = mockTracking('_category_', wrapper.element, jest.spyOn); + + findUncompletedLink().trigger('click'); - expect(linkElement.exists()).toBe(true); - expect(linkElement.attributes('target')).toEqual('_blank'); + expect(trackingSpy).toHaveBeenCalledWith('_category_', 'click_link', { + label: 'Run a Security scan using CI/CD', + property: 'Growth::Conversion::Experiment::LearnGitLab', + }); + + unmockTracking(); + }); }); describe('rendering a link to open the invite_members modal instead of a regular link', () => { diff --git a/spec/lib/gitlab/ci/config_spec.rb b/spec/lib/gitlab/ci/config_spec.rb index 1b3e8a2ce4a..05ff1f3618b 100644 --- a/spec/lib/gitlab/ci/config_spec.rb +++ b/spec/lib/gitlab/ci/config_spec.rb @@ -462,7 +462,7 @@ RSpec.describe Gitlab::Ci::Config do expect(project.repository).to receive(:blob_data_at) .with('eeff1122', local_location) - described_class.new(gitlab_ci_yml, project: project, sha: 'eeff1122', user: user) + described_class.new(gitlab_ci_yml, project: project, sha: 'eeff1122', user: user, pipeline: pipeline) end end @@ -470,7 +470,7 @@ RSpec.describe Gitlab::Ci::Config do it 'is using latest SHA on the default branch' do expect(project.repository).to receive(:root_ref_sha) - described_class.new(gitlab_ci_yml, project: project, sha: nil, user: user) + described_class.new(gitlab_ci_yml, project: project, sha: nil, user: user, pipeline: pipeline) end end end diff --git a/spec/lib/gitlab/ci/variables/builder/instance_spec.rb b/spec/lib/gitlab/ci/variables/builder/instance_spec.rb new file mode 100644 index 00000000000..7abda2bd615 --- /dev/null +++ b/spec/lib/gitlab/ci/variables/builder/instance_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Ci::Variables::Builder::Instance do + let_it_be(:variable) { create(:ci_instance_variable, protected: false) } + let_it_be(:protected_variable) { create(:ci_instance_variable, protected: true) } + + let(:builder) { described_class.new } + + describe '#secret_variables' do + let(:variable_item) { item(variable) } + let(:protected_variable_item) { item(protected_variable) } + + subject do + builder.secret_variables(protected_ref: protected_ref) + end + + context 'when the ref is protected' do + let(:protected_ref) { true } + + it 'contains all the variables' do + is_expected.to contain_exactly(variable_item, protected_variable_item) + end + end + + context 'when the ref is not protected' do + let(:protected_ref) { false } + + it 'contains only unprotected variables' do + is_expected.to contain_exactly(variable_item) + end + end + end + + def item(variable) + Gitlab::Ci::Variables::Collection::Item.fabricate(variable) + end +end diff --git a/spec/lib/gitlab/ci/variables/builder_spec.rb b/spec/lib/gitlab/ci/variables/builder_spec.rb index 6f9d4e8a70d..d2370b9d43c 100644 --- a/spec/lib/gitlab/ci/variables/builder_spec.rb +++ b/spec/lib/gitlab/ci/variables/builder_spec.rb @@ -7,7 +7,7 @@ RSpec.describe Gitlab::Ci::Variables::Builder do let_it_be(:project) { create(:project, :repository, namespace: group) } let_it_be_with_reload(:pipeline) { create(:ci_pipeline, project: project) } let_it_be(:user) { create(:user) } - let_it_be(:job) do + let_it_be_with_reload(:job) do create(:ci_build, pipeline: pipeline, user: user, @@ -276,27 +276,30 @@ RSpec.describe Gitlab::Ci::Variables::Builder do create(:protected_branch, :developers_can_merge, name: job.ref, project: project) end - it { is_expected.to include(variable) } + it { is_expected.to contain_exactly(protected_variable_item, unprotected_variable_item) } end context 'when ref is not protected' do - it { is_expected.not_to include(variable) } + it { is_expected.to contain_exactly(unprotected_variable_item) } end end context 'when ref is tag' do - let_it_be(:job) { create(:ci_build, ref: 'v1.1.0', tag: true, pipeline: pipeline) } + before do + job.update!(ref: 'v1.1.0', tag: true) + pipeline.update!(ref: 'v1.1.0', tag: true) + end context 'when ref is protected' do before do create(:protected_tag, project: project, name: 'v*') end - it { is_expected.to include(variable) } + it { is_expected.to contain_exactly(protected_variable_item, unprotected_variable_item) } end context 'when ref is not protected' do - it { is_expected.not_to include(variable) } + it { is_expected.to contain_exactly(unprotected_variable_item) } end end @@ -311,20 +314,24 @@ RSpec.describe Gitlab::Ci::Variables::Builder do end it 'does not return protected variables as it is not supported for merge request pipelines' do - is_expected.not_to include(variable) + is_expected.to contain_exactly(unprotected_variable_item) end end context 'when ref is not protected' do - it { is_expected.not_to include(variable) } + it { is_expected.to contain_exactly(unprotected_variable_item) } end end end describe '#secret_instance_variables' do - subject { builder.secret_instance_variables(ref: job.git_ref) } + subject { builder.secret_instance_variables } + + let_it_be(:protected_variable) { create(:ci_instance_variable, protected: true) } + let_it_be(:unprotected_variable) { create(:ci_instance_variable, protected: false) } - let_it_be(:variable) { create(:ci_instance_variable, protected: true) } + let(:protected_variable_item) { Gitlab::Ci::Variables::Collection::Item.fabricate(protected_variable) } + let(:unprotected_variable_item) { Gitlab::Ci::Variables::Collection::Item.fabricate(unprotected_variable) } include_examples "secret CI variables" end @@ -332,7 +339,11 @@ RSpec.describe Gitlab::Ci::Variables::Builder do describe '#secret_group_variables' do subject { builder.secret_group_variables(ref: job.git_ref, environment: job.expanded_environment_name) } - let_it_be(:variable) { create(:ci_group_variable, protected: true, group: group) } + let_it_be(:protected_variable) { create(:ci_group_variable, protected: true, group: group) } + let_it_be(:unprotected_variable) { create(:ci_group_variable, protected: false, group: group) } + + let(:protected_variable_item) { protected_variable } + let(:unprotected_variable_item) { unprotected_variable } include_examples "secret CI variables" end @@ -340,7 +351,11 @@ RSpec.describe Gitlab::Ci::Variables::Builder do describe '#secret_project_variables' do subject { builder.secret_project_variables(ref: job.git_ref, environment: job.expanded_environment_name) } - let_it_be(:variable) { create(:ci_variable, protected: true, project: project) } + let_it_be(:protected_variable) { create(:ci_variable, protected: true, project: project) } + let_it_be(:unprotected_variable) { create(:ci_variable, protected: false, project: project) } + + let(:protected_variable_item) { protected_variable } + let(:unprotected_variable_item) { unprotected_variable } include_examples "secret CI variables" end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index cf8c26e46ff..ce13f405459 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -606,6 +606,7 @@ project: - ci_project_mirror - sync_events - secure_files +- security_trainings award_emoji: - awardable - user diff --git a/spec/lib/gitlab/metrics/boot_time_tracker_spec.rb b/spec/lib/gitlab/metrics/boot_time_tracker_spec.rb index b6497766a20..8a17fa8dd2e 100644 --- a/spec/lib/gitlab/metrics/boot_time_tracker_spec.rb +++ b/spec/lib/gitlab/metrics/boot_time_tracker_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -require 'spec_helper' +require 'fast_spec_helper' RSpec.describe Gitlab::Metrics::BootTimeTracker do let(:logger) { double('logger') } @@ -74,20 +74,6 @@ RSpec.describe Gitlab::Metrics::BootTimeTracker do tracker.track_boot_time!(logger: logger) end end - - # TODO: When https://gitlab.com/gitlab-org/gitlab/-/issues/351769 is closed, - # revert to using fast_spec_helper again. - context 'when feature flag is off' do - it 'does nothing' do - stub_feature_flags(track_application_boot_time: false) - - expect(Gitlab::Metrics::System).not_to receive(:process_runtime_elapsed_seconds) - expect(logger).not_to receive(:info) - expect(gauge).not_to receive(:set) - - tracker.track_boot_time!(logger: logger) - end - end end describe '#startup_time' do diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index fd0df9ed992..c29cc04e0e9 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -4738,4 +4738,41 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do let!(:model) { create(:ci_pipeline, project: parent) } end end + + describe '#jobs_git_ref' do + subject { pipeline.jobs_git_ref } + + context 'when tag is true' do + let(:pipeline) { build(:ci_pipeline, tag: true) } + + it 'returns a tag ref' do + is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX) + end + end + + context 'when tag is false' do + let(:pipeline) { build(:ci_pipeline, tag: false) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + + context 'when tag is nil' do + let(:pipeline) { build(:ci_pipeline, tag: nil) } + + it 'returns a branch ref' do + is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX) + end + end + + context 'when it is triggered by a merge request' do + let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) } + let(:pipeline) { merge_request.pipelines_for_merge_request.first } + + it 'returns nil' do + is_expected.to be_nil + end + end + end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index f7d32dc1a7b..34ce0031bd2 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -445,7 +445,7 @@ RSpec.describe Note do end end - describe "#system_note_with_references_visible_for?" do + describe "#system_note_visible_for?" do let(:project) { create(:project, :public) } let(:user) { create(:user) } let(:guest) { create(:project_member, :guest, project: project, user: create(:user)).user } @@ -469,22 +469,25 @@ RSpec.describe Note do end it 'returns visible but not readable for non-member user' do - expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy + expect(note.system_note_visible_for?(non_member)).to be_truthy expect(note.readable_by?(non_member)).to be_falsy end it 'returns visible but not readable for a nil user' do - expect(note.system_note_with_references_visible_for?(nil)).to be_truthy + expect(note.system_note_visible_for?(nil)).to be_truthy expect(note.readable_by?(nil)).to be_falsy end end end describe "#system_note_viewable_by?(user)" do - let_it_be(:note) { create(:note) } + let_it_be(:group) { create(:group, :private) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:note) { create(:note, project: project) } let_it_be(:user) { create(:user) } - let!(:metadata) { create(:system_note_metadata, note: note, action: "branch") } + let(:action) { "commit" } + let!(:metadata) { create(:system_note_metadata, note: note, action: action) } context "when system_note_metadata is not present" do it "returns true" do @@ -494,32 +497,50 @@ RSpec.describe Note do end end - context "system_note_metadata isn't of type 'branch'" do - before do - metadata.action = "not_a_branch" - end - + context "system_note_metadata isn't of type 'branch' or 'contact'" do it "returns true" do expect(note.send(:system_note_viewable_by?, user)).to be_truthy end end - context "user doesn't have :download_code ability" do - it "returns false" do - expect(note.send(:system_note_viewable_by?, user)).to be_falsey + context "system_note_metadata is of type 'branch'" do + let(:action) { "branch" } + + context "user doesn't have :download_code ability" do + it "returns false" do + expect(note.send(:system_note_viewable_by?, user)).to be_falsey + end + end + + context "user has the :download_code ability" do + it "returns true" do + expect(Ability).to receive(:allowed?).with(user, :download_code, note.project).and_return(true) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end end end - context "user has the :download_code ability" do - it "returns true" do - expect(Ability).to receive(:allowed?).with(user, :download_code, note.project).and_return(true) + context "system_note_metadata is of type 'contact'" do + let(:action) { "contact" } - expect(note.send(:system_note_viewable_by?, user)).to be_truthy + context "user doesn't have :read_crm_contact ability" do + it "returns false" do + expect(note.send(:system_note_viewable_by?, user)).to be_falsey + end + end + + context "user has the :read_crm_contact ability" do + it "returns true" do + expect(Ability).to receive(:allowed?).with(user, :read_crm_contact, note.project.group).and_return(true) + + expect(note.send(:system_note_viewable_by?, user)).to be_truthy + end end end end - describe "system_note_with_references_visible_for?" do + describe "system_note_visible_for?" do let_it_be(:private_user) { create(:user) } let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } } let_it_be(:private_issue) { create(:issue, project: private_project) } @@ -529,11 +550,11 @@ RSpec.describe Note do shared_examples "checks references" do it "returns false" do - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy + expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy end it "returns true" do - expect(note.system_note_with_references_visible_for?(private_user)).to be_truthy + expect(note.system_note_visible_for?(private_user)).to be_truthy end it "returns true if user visible reference count set" do @@ -541,7 +562,7 @@ RSpec.describe Note do note.total_reference_count = 1 expect(note).not_to receive(:reference_mentionables) - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy + expect(note.system_note_visible_for?(ext_issue.author)).to be_truthy end it "returns false if user visible reference count set but does not match total reference count" do @@ -549,14 +570,14 @@ RSpec.describe Note do note.total_reference_count = 2 expect(note).not_to receive(:reference_mentionables) - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy + expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy end it "returns false if ref count is 0" do note.user_visible_reference_count = 0 expect(note).not_to receive(:reference_mentionables) - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy + expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy end end @@ -622,11 +643,11 @@ RSpec.describe Note do end it "returns true for other users" do - expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy + expect(note.system_note_visible_for?(ext_issue.author)).to be_truthy end it "returns true for anonymous users" do - expect(note.system_note_with_references_visible_for?(nil)).to be_truthy + expect(note.system_note_visible_for?(nil)).to be_truthy end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index fbbd1e3b2a2..ad63bf8b760 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3871,45 +3871,6 @@ RSpec.describe Project, factory_default: :keep do end end - describe '#ci_instance_variables_for' do - let(:project) { build_stubbed(:project) } - - let!(:instance_variable) do - create(:ci_instance_variable, value: 'secret') - end - - let!(:protected_instance_variable) do - create(:ci_instance_variable, :protected, value: 'protected') - end - - subject { project.ci_instance_variables_for(ref: 'ref') } - - before do - stub_application_setting( - default_branch_protection: Gitlab::Access::PROTECTION_NONE) - end - - context 'when the ref is not protected' do - before do - allow(project).to receive(:protected_for?).with('ref').and_return(false) - end - - it 'contains only the CI variables' do - is_expected.to contain_exactly(instance_variable) - end - end - - context 'when the ref is protected' do - before do - allow(project).to receive(:protected_for?).with('ref').and_return(true) - end - - it 'contains all the variables' do - is_expected.to contain_exactly(instance_variable, protected_instance_variable) - end - end - end - describe '#any_lfs_file_locks?', :request_store do let_it_be(:project) { create(:project) } diff --git a/spec/services/issues/set_crm_contacts_service_spec.rb b/spec/services/issues/set_crm_contacts_service_spec.rb index 2fc6bec1163..64011a7a003 100644 --- a/spec/services/issues/set_crm_contacts_service_spec.rb +++ b/spec/services/issues/set_crm_contacts_service_spec.rb @@ -32,6 +32,16 @@ RSpec.describe Issues::SetCrmContactsService do end end + shared_examples 'adds system note' do |added_count, removed_count| + it 'calls SystemNoteService.change_issuable_contacts with correct counts' do + expect(SystemNoteService) + .to receive(:change_issuable_contacts) + .with(issue, project, user, added_count, removed_count) + + set_crm_contacts + end + end + context 'when the user has no permission' do let(:params) { { replace_ids: [contacts[1].id, contacts[2].id] } } @@ -81,6 +91,7 @@ RSpec.describe Issues::SetCrmContactsService do let(:expected_contacts) { [contacts[1], contacts[2]] } it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 1 end context 'add' do @@ -89,6 +100,7 @@ RSpec.describe Issues::SetCrmContactsService do let(:expected_contacts) { [issue_contact_1, issue_contact_2, added_contact] } it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 0 end context 'add by email' do @@ -99,12 +111,14 @@ RSpec.describe Issues::SetCrmContactsService do let(:params) { { add_emails: [contacts[3].email] } } it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 0 end context 'with autocomplete prefix emails in params' do let(:params) { { add_emails: ["[\"contact:\"#{contacts[3].email}\"]"] } } it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 1, 0 end end @@ -113,6 +127,7 @@ RSpec.describe Issues::SetCrmContactsService do let(:expected_contacts) { [contacts[1]] } it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 0, 1 end context 'remove by email' do @@ -122,12 +137,14 @@ RSpec.describe Issues::SetCrmContactsService do let(:params) { { remove_emails: [contacts[0].email] } } it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 0, 1 end context 'with autocomplete prefix and suffix email in params' do let(:params) { { remove_emails: ["[contact:#{contacts[0].email}]"] } } it_behaves_like 'setting contacts' + it_behaves_like 'adds system note', 0, 1 end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 44acd34ae6a..a719487a219 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -77,6 +77,19 @@ RSpec.describe SystemNoteService do end end + describe '.change_issuable_contacts' do + let(:added_count) { 5 } + let(:removed_count) { 3 } + + it 'calls IssuableService' do + expect_next_instance_of(::SystemNotes::IssuablesService) do |service| + expect(service).to receive(:change_issuable_contacts).with(added_count, removed_count) + end + + described_class.change_issuable_contacts(noteable, project, author, added_count, removed_count) + end + end + describe '.close_after_error_tracking_resolve' do it 'calls IssuableService' do expect_next_instance_of(::SystemNotes::IssuablesService) do |service| diff --git a/spec/services/system_notes/issuables_service_spec.rb b/spec/services/system_notes/issuables_service_spec.rb index 7e53e66303b..e1c97026418 100644 --- a/spec/services/system_notes/issuables_service_spec.rb +++ b/spec/services/system_notes/issuables_service_spec.rb @@ -188,6 +188,54 @@ RSpec.describe ::SystemNotes::IssuablesService do end end + describe '#change_issuable_contacts' do + subject { service.change_issuable_contacts(1, 1) } + + let_it_be(:noteable) { create(:issue, project: project) } + + it_behaves_like 'a system note' do + let(:action) { 'contact' } + end + + def build_note(added_count, removed_count) + service.change_issuable_contacts(added_count, removed_count).note + end + + it 'builds a correct phrase when one contact is added' do + expect(build_note(1, 0)).to eq "added 1 contact" + end + + it 'builds a correct phrase when one contact is removed' do + expect(build_note(0, 1)).to eq "removed 1 contact" + end + + it 'builds a correct phrase when one contact is added and one contact is removed' do + expect(build_note(1, 1)).to( + eq("added 1 contact and removed 1 contact") + ) + end + + it 'builds a correct phrase when three contacts are added and one removed' do + expect(build_note(3, 1)).to( + eq("added 3 contacts and removed 1 contact") + ) + end + + it 'builds a correct phrase when three contacts are removed and one added' do + expect(build_note(1, 3)).to( + eq("added 1 contact and removed 3 contacts") + ) + end + + it 'builds a correct phrase when the locale is different' do + Gitlab::I18n.with_locale('pt-BR') do + expect(build_note(1, 3)).to( + eq("added 1 contact and removed 3 contacts") + ) + end + end + end + describe '#change_status' do subject { service.change_status(status, source) } diff --git a/spec/support/shared_examples/models/note_access_check_shared_examples.rb b/spec/support/shared_examples/models/note_access_check_shared_examples.rb index 44edafe9091..0c9992b832f 100644 --- a/spec/support/shared_examples/models/note_access_check_shared_examples.rb +++ b/spec/support/shared_examples/models/note_access_check_shared_examples.rb @@ -3,7 +3,7 @@ RSpec.shared_examples 'users with note access' do it 'returns true' do users.each do |user| - expect(note.system_note_with_references_visible_for?(user)).to be_truthy + expect(note.system_note_visible_for?(user)).to be_truthy expect(note.readable_by?(user)).to be_truthy end end @@ -12,7 +12,7 @@ end RSpec.shared_examples 'users without note access' do it 'returns false' do users.each do |user| - expect(note.system_note_with_references_visible_for?(user)).to be_falsy + expect(note.system_note_visible_for?(user)).to be_falsy expect(note.readable_by?(user)).to be_falsy end end |