diff options
60 files changed, 754 insertions, 417 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index 96ff65b3c49..4a6e90f5263 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -401,13 +401,9 @@ RSpec/RepeatedExample: - 'spec/frontend/fixtures/merge_requests.rb' - 'spec/graphql/gitlab_schema_spec.rb' - 'spec/helpers/users_helper_spec.rb' - - 'spec/lib/banzai/filter/autolink_filter_spec.rb' - 'spec/lib/gitlab/closing_issue_extractor_spec.rb' - 'spec/lib/gitlab/danger/changelog_spec.rb' - - 'spec/lib/gitlab/git/blob_spec.rb' - 'spec/lib/gitlab/import_export/project/relation_factory_spec.rb' - - 'spec/models/ability_spec.rb' - - 'spec/models/ci/build_spec.rb' - 'spec/models/concerns/issuable_spec.rb' - 'spec/models/project_services/chat_message/pipeline_message_spec.rb' - 'spec/routing/admin_routing_spec.rb' diff --git a/app/assets/javascripts/blob/blob_file_dropzone.js b/app/assets/javascripts/blob/blob_file_dropzone.js index 8acf0827c44..e8772b7240a 100644 --- a/app/assets/javascripts/blob/blob_file_dropzone.js +++ b/app/assets/javascripts/blob/blob_file_dropzone.js @@ -43,6 +43,10 @@ export default class BlobFileDropzone { previewsContainer: '.dropzone-previews', headers: csrf.headers, init() { + this.on('processing', function() { + this.options.url = form.attr('action'); + }); + this.on('addedfile', () => { toggleLoading(submitButton, submitButtonLoadingIcon, false); dropzoneMessage.addClass(HIDDEN_CLASS); diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index d1eed85fde6..664c58e8b7a 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -57,7 +57,7 @@ class Groups::GroupMembersController < Groups::ApplicationController def find_members filter_params = params.slice(:two_factor, :search).merge(sort: @sort) - GroupMembersFinder.new(@group, current_user).execute(include_relations: requested_relations, params: filter_params) + GroupMembersFinder.new(@group, current_user, params: filter_params).execute(include_relations: requested_relations) end def can_manage_members diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index a50d516f75e..f552c471eb2 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -186,7 +186,7 @@ class Projects::IssuesController < Projects::ApplicationController if result[:status] == :success render json: MergeRequestCreateSerializer.new.represent(result[:merge_request]) else - render json: result[:messsage], status: :unprocessable_entity + render json: result[:message], status: :unprocessable_entity end end diff --git a/app/finders/group_members_finder.rb b/app/finders/group_members_finder.rb index bee18017cf3..a56d4ebb368 100644 --- a/app/finders/group_members_finder.rb +++ b/app/finders/group_members_finder.rb @@ -10,13 +10,16 @@ class GroupMembersFinder < UnionFinder # created_after: datetime # created_before: datetime - def initialize(group, user = nil) + attr_reader :params + + def initialize(group, user = nil, params: {}) @group = group @user = user + @params = params end # rubocop: disable CodeReuse/ActiveRecord - def execute(include_relations: [:inherited, :direct], params: {}) + def execute(include_relations: [:inherited, :direct]) group_members = group.members relations = [] @params = params @@ -50,7 +53,7 @@ class GroupMembersFinder < UnionFinder private - attr_reader :user, :group, :params + attr_reader :user, :group def filter_members(members) members = members.search(params[:search]) if params[:search].present? diff --git a/app/serializers/diffs_entity.rb b/app/serializers/diffs_entity.rb index b709fc7228b..1d0b4183f96 100644 --- a/app/serializers/diffs_entity.rb +++ b/app/serializers/diffs_entity.rb @@ -16,12 +16,7 @@ class DiffsEntity < Grape::Entity end expose :commit do |diffs, options| - CommitEntity.represent options[:commit], options.merge( - type: :full, - commit_url_params: { merge_request_iid: merge_request&.iid }, - pipeline_ref: merge_request&.source_branch, - pipeline_project: merge_request&.source_project - ) + CommitEntity.represent(options[:commit], commit_options(options)) end expose :context_commits, using: API::Entities::Commit, if: -> (diffs, options) { merge_request&.project&.context_commits_enabled? } do |diffs| @@ -88,4 +83,31 @@ class DiffsEntity < Grape::Entity def merge_request options[:merge_request] end + + private + + def commit_ids + @commit_ids ||= merge_request.recent_commits.map(&:id) + end + + def commit_neighbors(commit_id) + index = commit_ids.index(commit_id) + + return [] unless index + + [(index > 0 ? commit_ids[index - 1] : nil), commit_ids[index + 1]] + end + + def commit_options(options) + prev_commit_id, next_commit_id = *commit_neighbors(options[:commit]&.id) + + options.merge( + type: :full, + commit_url_params: { merge_request_iid: merge_request&.iid }, + pipeline_ref: merge_request&.source_branch, + pipeline_project: merge_request&.source_project, + prev_commit_id: prev_commit_id, + next_commit_id: next_commit_id + ) + end end diff --git a/app/services/concerns/deploy_token_methods.rb b/app/services/concerns/deploy_token_methods.rb index c0208b16623..c875342a07c 100644 --- a/app/services/concerns/deploy_token_methods.rb +++ b/app/services/concerns/deploy_token_methods.rb @@ -8,4 +8,10 @@ module DeployTokenMethods deploy_token.username = params[:username].presence end end + + def destroy_deploy_token(entity, params) + deploy_token = entity.deploy_tokens.find_by_id!(params[:token_id]) + + deploy_token.destroy + end end diff --git a/app/services/groups/deploy_tokens/destroy_service.rb b/app/services/groups/deploy_tokens/destroy_service.rb new file mode 100644 index 00000000000..6dae22f29d2 --- /dev/null +++ b/app/services/groups/deploy_tokens/destroy_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Groups + module DeployTokens + class DestroyService < BaseService + include DeployTokenMethods + + def execute + destroy_deploy_token(@group, params) + end + end + end +end diff --git a/app/services/projects/deploy_tokens/destroy_service.rb b/app/services/projects/deploy_tokens/destroy_service.rb new file mode 100644 index 00000000000..e063f86a65c --- /dev/null +++ b/app/services/projects/deploy_tokens/destroy_service.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Projects + module DeployTokens + class DestroyService < BaseService + include DeployTokenMethods + + def execute + destroy_deploy_token(@project, params) + end + end + end +end diff --git a/app/services/resource_events/synthetic_milestone_notes_builder_service.rb b/app/services/resource_events/synthetic_milestone_notes_builder_service.rb index ad58417834e..cc6383d7083 100644 --- a/app/services/resource_events/synthetic_milestone_notes_builder_service.rb +++ b/app/services/resource_events/synthetic_milestone_notes_builder_service.rb @@ -10,8 +10,6 @@ module ResourceEvents private def synthetic_notes - return [] unless tracking_enabled? - milestone_change_events.map do |event| MilestoneNote.from_event(event, resource: resource, resource_parent: resource_parent) end @@ -23,9 +21,5 @@ module ResourceEvents events = resource.resource_milestone_events.includes(user: :status) # rubocop: disable CodeReuse/ActiveRecord since_fetch_at(events) end - - def tracking_enabled? - ::Feature.enabled?(:track_resource_milestone_change_events, resource.project) - end end end diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 8bbfb10ed6e..21e478f935b 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -21,6 +21,10 @@ module Gitlab finish: Stage::FinishImportWorker }.freeze + def find_import_state(project_id) + ProjectImportState.jid_by(project_id: project_id, status: :started) + end + private def next_stage_worker(next_stage) diff --git a/app/workers/gitlab/import/advance_stage.rb b/app/workers/gitlab/import/advance_stage.rb index 5c836413ae3..3f34437294e 100644 --- a/app/workers/gitlab/import/advance_stage.rb +++ b/app/workers/gitlab/import/advance_stage.rb @@ -48,7 +48,7 @@ module Gitlab end def find_import_state(project_id) - ProjectImportState.jid_by(project_id: project_id, status: :started) + raise NotImplementedError end private diff --git a/app/workers/gitlab/jira_import/advance_stage_worker.rb b/app/workers/gitlab/jira_import/advance_stage_worker.rb index 1b6fc54151e..c83a57bcd83 100644 --- a/app/workers/gitlab/jira_import/advance_stage_worker.rb +++ b/app/workers/gitlab/jira_import/advance_stage_worker.rb @@ -16,6 +16,10 @@ module Gitlab finish: Gitlab::JiraImport::Stage::FinishImportWorker }.freeze + def find_import_state(project_id) + ProjectImportState.jid_by(project_id: project_id, status: :started) + end + private def next_stage_worker(next_stage) diff --git a/changelogs/unreleased/212523-delete-deploy-tokens.yml b/changelogs/unreleased/212523-delete-deploy-tokens.yml new file mode 100644 index 00000000000..f16703bb594 --- /dev/null +++ b/changelogs/unreleased/212523-delete-deploy-tokens.yml @@ -0,0 +1,5 @@ +--- +title: Fix deploy token API to properly delete all associated deploy token records +merge_request: 28156 +author: +type: fixed diff --git a/changelogs/unreleased/212985-remove-ff-check-for-synthetic-milestone-notes-creation.yml b/changelogs/unreleased/212985-remove-ff-check-for-synthetic-milestone-notes-creation.yml new file mode 100644 index 00000000000..5395723e125 --- /dev/null +++ b/changelogs/unreleased/212985-remove-ff-check-for-synthetic-milestone-notes-creation.yml @@ -0,0 +1,5 @@ +--- +title: Fix missing synthetic milestone change notes for disabled milestone change event tracking feature flag +merge_request: 28440 +author: +type: fixed diff --git a/changelogs/unreleased/42639-remove-custom-metrics-license-check.yml b/changelogs/unreleased/42639-remove-custom-metrics-license-check.yml new file mode 100644 index 00000000000..018245a7384 --- /dev/null +++ b/changelogs/unreleased/42639-remove-custom-metrics-license-check.yml @@ -0,0 +1,5 @@ +--- +title: Move 'Additional Metrics' feature to GitLab Core +merge_request: 28527 +author: +type: changed diff --git a/changelogs/unreleased/merge-request-typo.yml b/changelogs/unreleased/merge-request-typo.yml new file mode 100644 index 00000000000..5815f5942bf --- /dev/null +++ b/changelogs/unreleased/merge-request-typo.yml @@ -0,0 +1,5 @@ +--- +title: Return error message for create_merge_request +merge_request: 28482 +author: +type: fixed diff --git a/changelogs/unreleased/ph-212642-userUploadsFile.yml b/changelogs/unreleased/ph-212642-userUploadsFile.yml new file mode 100644 index 00000000000..39053504d1a --- /dev/null +++ b/changelogs/unreleased/ph-212642-userUploadsFile.yml @@ -0,0 +1,5 @@ +--- +title: Fixed upload file creating a file in the wrong directory +merge_request: +author: +type: fixed diff --git a/changelogs/unreleased/refactor-ability-spec.yml b/changelogs/unreleased/refactor-ability-spec.yml new file mode 100644 index 00000000000..0578a6cd674 --- /dev/null +++ b/changelogs/unreleased/refactor-ability-spec.yml @@ -0,0 +1,5 @@ +--- +title: Remove duplicate specs in ability model +merge_request: 28644 +author: Rajendra Kadam +type: added diff --git a/changelogs/unreleased/refactor-build-spec.yml b/changelogs/unreleased/refactor-build-spec.yml new file mode 100644 index 00000000000..7b087933ed1 --- /dev/null +++ b/changelogs/unreleased/refactor-build-spec.yml @@ -0,0 +1,5 @@ +--- +title: Fix build duplicate spec +merge_request: 28633 +author: Rajendra Kadam +type: added diff --git a/changelogs/unreleased/separate-validator-files.yml b/changelogs/unreleased/separate-validator-files.yml new file mode 100644 index 00000000000..0003064c7cb --- /dev/null +++ b/changelogs/unreleased/separate-validator-files.yml @@ -0,0 +1,5 @@ +--- +title: Separate validators into own class files +merge_request: 28266 +author: Rajendra Kadam +type: added diff --git a/config/initializers/grape_validators.rb b/config/initializers/grape_validators.rb new file mode 100644 index 00000000000..9d2b6dc9bd1 --- /dev/null +++ b/config/initializers/grape_validators.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +Grape::Validations.register_validator(:absence, ::API::Validations::Validators::Absence) +Grape::Validations.register_validator(:file_path, ::API::Validations::Validators::FilePath) +Grape::Validations.register_validator(:git_ref, ::API::Validations::Validators::GitRef) +Grape::Validations.register_validator(:git_sha, ::API::Validations::Validators::GitSha) +Grape::Validations.register_validator(:integer_none_any, ::API::Validations::Validators::IntegerNoneAny) +Grape::Validations.register_validator(:array_none_any, ::API::Validations::Validators::ArrayNoneAny) +Grape::Validations.register_validator(:check_assignees_count, ::API::Validations::Validators::CheckAssigneesCount) diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 776ac252b76..46ea91fcdf3 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -246,6 +246,7 @@ Example changes: - `add_foreign_key` / `remove_foreign_key` - `add_column` / `remove_column` - `change_column_default` +- `create_table` / `drop_table` **Note:** `with_lock_retries` method **cannot** be used with `disable_ddl_transaction!`. diff --git a/lib/api/deploy_tokens.rb b/lib/api/deploy_tokens.rb index fb4c4265aef..d36b75f5bfd 100644 --- a/lib/api/deploy_tokens.rb +++ b/lib/api/deploy_tokens.rb @@ -85,12 +85,10 @@ module API delete ':id/deploy_tokens/:token_id' do authorize!(:destroy_deploy_token, user_project) - deploy_token = user_project.project_deploy_tokens - .find_by_deploy_token_id(params[:token_id]) - - not_found!('Deploy Token') unless deploy_token + ::Projects::DeployTokens::DestroyService.new( + user_project, current_user, token_id: params[:token_id] + ).execute - deploy_token.destroy no_content! end end @@ -144,13 +142,17 @@ module API desc 'Delete a group deploy token' do detail 'This feature was introduced in GitLab 12.9' end + params do + requires :token_id, type: Integer, desc: 'The deploy token ID' + end delete ':id/deploy_tokens/:token_id' do authorize!(:destroy_deploy_token, user_group) - deploy_token = user_group.group_deploy_tokens - .find_by_deploy_token_id!(params[:token_id]) + ::Groups::DeployTokens::DestroyService.new( + user_group, current_user, token_id: params[:token_id] + ).execute - destroy_conditionally!(deploy_token) + no_content! end end end diff --git a/lib/api/entities/commit_with_link.rb b/lib/api/entities/commit_with_link.rb index 31a9efed9bc..a135cc19480 100644 --- a/lib/api/entities/commit_with_link.rb +++ b/lib/api/entities/commit_with_link.rb @@ -32,6 +32,14 @@ module API render('projects/commit/_signature', signature: commit.signature) if commit.has_signature? end + expose :prev_commit_id, if: { type: :full } do |commit| + options[:prev_commit_id] + end + + expose :next_commit_id, if: { type: :full } do |commit| + options[:next_commit_id] + end + expose :pipeline_status_path, if: { type: :full } do |commit, options| pipeline_ref = options[:pipeline_ref] pipeline_project = options[:pipeline_project] || commit.project diff --git a/lib/api/environments.rb b/lib/api/environments.rb index e5db9cdedc8..28019ce7796 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -3,7 +3,6 @@ module API # Environments RESTfull API endpoints class Environments < Grape::API - include ::API::Helpers::CustomValidators include PaginationParams before { authenticate! } diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index be41788ac77..42b82aac1c4 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -179,8 +179,10 @@ module API end # rubocop: disable CodeReuse/ActiveRecord - def find_project_issue(iid) - IssuesFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid) + def find_project_issue(iid, project_id = nil) + project = project_id ? find_project!(project_id) : user_project + + ::IssuesFinder.new(current_user, project_id: project.id).find_by!(iid: iid) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/api/helpers/custom_validators.rb b/lib/api/helpers/custom_validators.rb deleted file mode 100644 index 76f5fe555b4..00000000000 --- a/lib/api/helpers/custom_validators.rb +++ /dev/null @@ -1,97 +0,0 @@ -# frozen_string_literal: true - -module API - module Helpers - module CustomValidators - class FilePath < Grape::Validations::Base - def validate_param!(attr_name, params) - path = params[attr_name] - - Gitlab::Utils.check_path_traversal!(path) - rescue StandardError - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], - message: "should be a valid file path" - end - end - - class GitSha < Grape::Validations::Base - def validate_param!(attr_name, params) - sha = params[attr_name] - - return if Commit::EXACT_COMMIT_SHA_PATTERN.match?(sha) - - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], - message: "should be a valid sha" - end - end - - class Absence < Grape::Validations::Base - def validate_param!(attr_name, params) - return if params.respond_to?(:key?) && !params.key?(attr_name) - - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:absence) - end - end - - class IntegerNoneAny < Grape::Validations::Base - def validate_param!(attr_name, params) - value = params[attr_name] - - return if value.is_a?(Integer) || - [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase) - - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], - message: "should be an integer, 'None' or 'Any'" - end - end - - class ArrayNoneAny < Grape::Validations::Base - def validate_param!(attr_name, params) - value = params[attr_name] - - return if value.is_a?(Array) || - [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase) - - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], - message: "should be an array, 'None' or 'Any'" - end - end - - class GitRef < Grape::Validations::Base - # There are few checks that a Git reference should pass through to be valid reference. - # The link contains some rules that have been added to this validator. - # https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html - # We have skipped some checks that are optional and can be skipped for exception. - # We also check for control characters, More info on ctrl chars - https://ruby-doc.org/core-2.7.0/Regexp.html#class-Regexp-label-Character+Classes - INVALID_CHARS = Regexp.union('..', '\\', '@', '@{', ' ', '~', '^', ':', '*', '?', '[', /[[:cntrl:]]/).freeze - GIT_REF_LENGTH = (1..1024).freeze - - def validate_param!(attr_name, params) - revision = params[attr_name] - - return unless invalid_character?(revision) - - raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], - message: 'should be a valid reference path' - end - - private - - def invalid_character?(revision) - revision.nil? || - revision.start_with?('-') || - revision.end_with?('.') || - GIT_REF_LENGTH.exclude?(revision.length) || - INVALID_CHARS.match?(revision) - end - end - end - end -end - -Grape::Validations.register_validator(:file_path, ::API::Helpers::CustomValidators::FilePath) -Grape::Validations.register_validator(:git_sha, ::API::Helpers::CustomValidators::GitSha) -Grape::Validations.register_validator(:absence, ::API::Helpers::CustomValidators::Absence) -Grape::Validations.register_validator(:integer_none_any, ::API::Helpers::CustomValidators::IntegerNoneAny) -Grape::Validations.register_validator(:array_none_any, ::API::Helpers::CustomValidators::ArrayNoneAny) -Grape::Validations.register_validator(:git_ref, ::API::Helpers::CustomValidators::GitRef) diff --git a/lib/api/helpers/merge_requests_helpers.rb b/lib/api/helpers/merge_requests_helpers.rb index e0753254002..73711a7e0ba 100644 --- a/lib/api/helpers/merge_requests_helpers.rb +++ b/lib/api/helpers/merge_requests_helpers.rb @@ -4,7 +4,6 @@ module API module Helpers module MergeRequestsHelpers extend Grape::API::Helpers - include ::API::Helpers::CustomValidators params :merge_requests_base_params do optional :state, diff --git a/lib/api/issues.rb b/lib/api/issues.rb index d34c205281a..a78202877fb 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -23,7 +23,7 @@ module API optional :assignee_id, types: [Integer, String], integer_none_any: true, desc: 'Return issues which are assigned to the user with the given ID' optional :assignee_username, type: Array[String], check_assignees_count: true, - coerce_with: Validations::CheckAssigneesCount.coerce, + coerce_with: Validations::Validators::CheckAssigneesCount.coerce, desc: 'Return issues which are assigned to the user with the given username' mutually_exclusive :assignee_id, :assignee_username end diff --git a/lib/api/validations/check_assignees_count.rb b/lib/api/validations/check_assignees_count.rb deleted file mode 100644 index 451b14c623c..00000000000 --- a/lib/api/validations/check_assignees_count.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -module API - module Validations - class CheckAssigneesCount < Grape::Validations::Base - def self.coerce - lambda do |value| - case value - when String, Array - Array.wrap(value) - else - [] - end - end - end - - def validate_param!(attr_name, params) - return if param_allowed?(attr_name, params) - - raise Grape::Exceptions::Validation, - params: [@scope.full_name(attr_name)], - message: "allows one value, but found #{params[attr_name].size}: #{params[attr_name].join(", ")}" - end - - private - - def param_allowed?(attr_name, params) - params[attr_name].size <= 1 - end - end - end -end - -API::Validations::CheckAssigneesCount.prepend_if_ee('EE::API::Validations::CheckAssigneesCount') diff --git a/lib/api/validations/validators/absence.rb b/lib/api/validations/validators/absence.rb new file mode 100644 index 00000000000..1f43f3ab126 --- /dev/null +++ b/lib/api/validations/validators/absence.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class Absence < Grape::Validations::Base + def validate_param!(attr_name, params) + return if params.respond_to?(:key?) && !params.key?(attr_name) + + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], message: message(:absence) + end + end + end + end +end diff --git a/lib/api/validations/validators/array_none_any.rb b/lib/api/validations/validators/array_none_any.rb new file mode 100644 index 00000000000..7efb8e6ccee --- /dev/null +++ b/lib/api/validations/validators/array_none_any.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class ArrayNoneAny < Grape::Validations::Base + def validate_param!(attr_name, params) + value = params[attr_name] + + return if value.is_a?(Array) || + [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase) + + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], + message: "should be an array, 'None' or 'Any'" + end + end + end + end +end diff --git a/lib/api/validations/validators/check_assignees_count.rb b/lib/api/validations/validators/check_assignees_count.rb new file mode 100644 index 00000000000..b614058e325 --- /dev/null +++ b/lib/api/validations/validators/check_assignees_count.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class CheckAssigneesCount < Grape::Validations::Base + def self.coerce + lambda do |value| + case value + when String, Array + Array.wrap(value) + else + [] + end + end + end + + def validate_param!(attr_name, params) + return if param_allowed?(attr_name, params) + + raise Grape::Exceptions::Validation, + params: [@scope.full_name(attr_name)], + message: "allows one value, but found #{params[attr_name].size}: #{params[attr_name].join(", ")}" + end + + private + + def param_allowed?(attr_name, params) + params[attr_name].size <= 1 + end + end + end + end +end + +API::Validations::Validators::CheckAssigneesCount.prepend_if_ee('EE::API::Validations::Validators::CheckAssigneesCount') diff --git a/lib/api/validations/validators/file_path.rb b/lib/api/validations/validators/file_path.rb new file mode 100644 index 00000000000..93a20e5bf7d --- /dev/null +++ b/lib/api/validations/validators/file_path.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class FilePath < Grape::Validations::Base + def validate_param!(attr_name, params) + path = params[attr_name] + + Gitlab::Utils.check_path_traversal!(path) + rescue StandardError + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], + message: "should be a valid file path" + end + end + end + end +end diff --git a/lib/api/validations/validators/git_ref.rb b/lib/api/validations/validators/git_ref.rb new file mode 100644 index 00000000000..1dda9d758a7 --- /dev/null +++ b/lib/api/validations/validators/git_ref.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class GitRef < Grape::Validations::Base + # There are few checks that a Git reference should pass through to be valid reference. + # The link contains some rules that have been added to this validator. + # https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html + # We have skipped some checks that are optional and can be skipped for exception. + # We also check for control characters, More info on ctrl chars - https://ruby-doc.org/core-2.7.0/Regexp.html#class-Regexp-label-Character+Classes + INVALID_CHARS = Regexp.union('..', '\\', '@', '@{', ' ', '~', '^', ':', '*', '?', '[', /[[:cntrl:]]/).freeze + GIT_REF_LENGTH = (1..1024).freeze + + def validate_param!(attr_name, params) + revision = params[attr_name] + + return unless invalid_character?(revision) + + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], + message: 'should be a valid reference path' + end + + private + + def invalid_character?(revision) + revision.nil? || + revision.start_with?('-') || + revision.end_with?('.') || + GIT_REF_LENGTH.exclude?(revision.length) || + INVALID_CHARS.match?(revision) + end + end + end + end +end diff --git a/lib/api/validations/validators/git_sha.rb b/lib/api/validations/validators/git_sha.rb new file mode 100644 index 00000000000..657307db1df --- /dev/null +++ b/lib/api/validations/validators/git_sha.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class GitSha < Grape::Validations::Base + def validate_param!(attr_name, params) + sha = params[attr_name] + + return if Commit::EXACT_COMMIT_SHA_PATTERN.match?(sha) + + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], + message: "should be a valid sha" + end + end + end + end +end diff --git a/lib/api/validations/validators/integer_none_any.rb b/lib/api/validations/validators/integer_none_any.rb new file mode 100644 index 00000000000..aa8c137a6ab --- /dev/null +++ b/lib/api/validations/validators/integer_none_any.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module API + module Validations + module Validators + class IntegerNoneAny < Grape::Validations::Base + def validate_param!(attr_name, params) + value = params[attr_name] + + return if value.is_a?(Integer) || + [IssuableFinder::Params::FILTER_NONE, IssuableFinder::Params::FILTER_ANY].include?(value.to_s.downcase) + + raise Grape::Exceptions::Validation, params: [@scope.full_name(attr_name)], + message: "should be an integer, 'None' or 'Any'" + end + end + end + end +end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 1fda9d63bbe..30228248baf 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1249,6 +1249,26 @@ describe Projects::IssuesController do expect(response).to have_gitlab_http_status(:not_found) end + context 'invalid branch name' do + it 'is unprocessable' do + post( + :create_merge_request, + params: { + target_project_id: nil, + branch_name: 'master', + ref: 'master', + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: issue.to_param + }, + format: :json + ) + + expect(response.body).to eq('Branch already exists') + expect(response).to have_gitlab_http_status(:unprocessable_entity) + end + end + context 'target_project_id is set' do let(:target_project) { fork_project(project, user, repository: true) } let(:target_project_id) { target_project.id } diff --git a/spec/features/projects/files/user_uploads_files_spec.rb b/spec/features/projects/files/user_uploads_files_spec.rb index 8a20c1387a3..ecf40969541 100644 --- a/spec/features/projects/files/user_uploads_files_spec.rb +++ b/spec/features/projects/files/user_uploads_files_spec.rb @@ -22,6 +22,31 @@ describe 'Projects > Files > User uploads files' do include_examples 'it uploads and commit a new text file' include_examples 'it uploads and commit a new image file' + + it 'uploads a file to a sub-directory', :js do + click_link 'files' + + page.within('.repo-breadcrumb') do + expect(page).to have_content('files') + end + + find('.add-to-tree').click + click_link('Upload file') + drop_in_dropzone(File.join(Rails.root, 'spec', 'fixtures', 'doc_sample.txt')) + + page.within('#modal-upload-blob') do + fill_in(:commit_message, with: 'New commit message') + end + + click_button('Upload file') + + expect(page).to have_content('New commit message') + + page.within('.repo-breadcrumb') do + expect(page).to have_content('files') + expect(page).to have_content('doc_sample.txt') + end + end end context 'when a user does not have write access' do diff --git a/spec/finders/group_members_finder_spec.rb b/spec/finders/group_members_finder_spec.rb index 34649097f70..d1d97f6f9f0 100644 --- a/spec/finders/group_members_finder_spec.rb +++ b/spec/finders/group_members_finder_spec.rb @@ -82,7 +82,7 @@ describe GroupMembersFinder, '#execute' do group.add_developer(user3) member = group.add_maintainer(user1) - result = described_class.new(group).execute(params: { search: user1.name }) + result = described_class.new(group, params: { search: user1.name }).execute expect(result.to_a).to match_array([member]) end @@ -92,7 +92,7 @@ describe GroupMembersFinder, '#execute' do group.add_developer(user3) group.add_maintainer(user1) - result = described_class.new(group).execute(include_relations: [:inherited], params: { search: user1.name }) + result = described_class.new(group, params: { search: user1.name }).execute(include_relations: [:inherited]) expect(result.to_a).to match_array([]) end @@ -103,7 +103,7 @@ describe GroupMembersFinder, '#execute' do nested_group.add_maintainer(create(:user, name: user1.name)) member = group.add_maintainer(user1) - result = described_class.new(nested_group).execute(include_relations: [:inherited], params: { search: member.user.name }) + result = described_class.new(nested_group, params: { search: member.user.name }).execute(include_relations: [:inherited]) expect(result.to_a).to contain_exactly(member) end @@ -113,7 +113,7 @@ describe GroupMembersFinder, '#execute' do group.add_maintainer(user1) member = group.add_maintainer(user5) - result = described_class.new(group, user2).execute(params: { two_factor: 'enabled' }) + result = described_class.new(group, user2, params: { two_factor: 'enabled' }).execute expect(result.to_a).to contain_exactly(member) end @@ -123,7 +123,7 @@ describe GroupMembersFinder, '#execute' do member2 = group.add_maintainer(user1) member_with_2fa = group.add_maintainer(user5) - result = described_class.new(group, user2).execute(params: { two_factor: 'disabled' }) + result = described_class.new(group, user2, params: { two_factor: 'disabled' }).execute expect(result.to_a).not_to include(member_with_2fa) expect(result.to_a).to match_array([member1, member2]) diff --git a/spec/frontend/diffs/components/commit_item_spec.js b/spec/frontend/diffs/components/commit_item_spec.js index 61bab77964e..6bb3a0dcf21 100644 --- a/spec/frontend/diffs/components/commit_item_spec.js +++ b/spec/frontend/diffs/components/commit_item_spec.js @@ -30,12 +30,12 @@ describe('diffs/components/commit_item', () => { const getCommitActionsElement = () => wrapper.find('.commit-actions'); const getCommitPipelineStatus = () => wrapper.find(CommitPipelineStatus); - const defaultProps = { - commit: getDiffWithCommit().commit, - }; - const mountComponent = (propsData = defaultProps) => { + const mountComponent = propsData => { wrapper = mount(Component, { - propsData, + propsData: { + commit, + ...propsData, + }, stubs: { CommitPipelineStatus: true, }, @@ -59,9 +59,7 @@ describe('diffs/components/commit_item', () => { expect(titleElement.text()).toBe(commit.title_html); }); - // https://gitlab.com/gitlab-org/gitlab/-/issues/209776 - // eslint-disable-next-line jest/no-disabled-tests - it.skip('renders commit description', () => { + it('renders commit description', () => { const descElement = getDescElement(); const descExpandElement = getDescExpandElement(); @@ -107,7 +105,7 @@ describe('diffs/components/commit_item', () => { describe('without commit description', () => { beforeEach(() => { - mountComponent({ defaultProps, commit: { ...defaultProps.commit, description_html: '' } }); + mountComponent({ commit: { ...commit, description_html: '' } }); }); it('hides description', () => { @@ -122,9 +120,8 @@ describe('diffs/components/commit_item', () => { describe('with no matching user', () => { beforeEach(() => { mountComponent({ - defaultProps, commit: { - ...defaultProps.commit, + ...commit, author: null, author_email: TEST_AUTHOR_EMAIL, author_name: TEST_AUTHOR_NAME, @@ -154,8 +151,7 @@ describe('diffs/components/commit_item', () => { describe('with signature', () => { beforeEach(() => { mountComponent({ - defaultProps, - commit: { ...defaultProps.commit, signature_html: TEST_SIGNATURE_HTML }, + commit: { ...commit, signature_html: TEST_SIGNATURE_HTML }, }); }); @@ -169,8 +165,7 @@ describe('diffs/components/commit_item', () => { describe('with pipeline status', () => { beforeEach(() => { mountComponent({ - defaultProps, - commit: { ...defaultProps.commit, pipeline_status_path: TEST_PIPELINE_STATUS_PATH }, + commit: { ...commit, pipeline_status_path: TEST_PIPELINE_STATUS_PATH }, }); }); diff --git a/spec/lib/api/helpers/custom_validators_spec.rb b/spec/lib/api/helpers/custom_validators_spec.rb deleted file mode 100644 index a4f2cd3452c..00000000000 --- a/spec/lib/api/helpers/custom_validators_spec.rb +++ /dev/null @@ -1,193 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe API::Helpers::CustomValidators do - let(:scope) do - Struct.new(:opts) do - def full_name(attr_name) - attr_name - end - end - end - - describe API::Helpers::CustomValidators::Absence do - subject do - described_class.new(['test'], {}, false, scope.new) - end - - context 'empty param' do - it 'does not raise a validation error' do - expect_no_validation_error({}) - end - end - - context 'invalid parameters' do - it 'raises a validation error' do - expect_validation_error('test' => 'some_value') - end - end - end - - describe API::Helpers::CustomValidators::GitSha do - let(:sha) { RepoHelpers.sample_commit.id } - let(:short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH] } - let(:too_short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH - 1] } - - subject do - described_class.new(['test'], {}, false, scope.new) - end - - context 'valid sha' do - it 'does not raise a validation error' do - expect_no_validation_error('test' => sha) - expect_no_validation_error('test' => short_sha) - end - end - - context 'empty params' do - it 'raises a validation error' do - expect_validation_error('test' => nil) - expect_validation_error('test' => '') - end - end - - context 'invalid sha' do - it 'raises a validation error' do - expect_validation_error('test' => "#{sha}2") # Sha length > 40 - expect_validation_error('test' => 'somestring') - expect_validation_error('test' => too_short_sha) # sha length < MIN_SHA_LENGTH (7) - end - end - end - - describe API::Helpers::CustomValidators::GitRef do - subject do - described_class.new(['test'], {}, false, scope.new) - end - - context 'valid revision param' do - it 'does not raise a validation error' do - expect_no_validation_error('test' => '4e963fe') - expect_no_validation_error('test' => 'foo/bar/baz') - expect_no_validation_error('test' => "heads/fu\303\237") - expect_no_validation_error('test' => 'a' * 1024) - end - end - - context "revision param contains invalid chars" do - it 'raises a validation error' do - expect_validation_error('test' => '-4e963fe') - expect_validation_error('test' => '4e963fe..ed4ef') - expect_validation_error('test' => '4e96\3fe') - expect_validation_error('test' => '4e96@3fe') - expect_validation_error('test' => '4e9@{63fe') - expect_validation_error('test' => '4e963 fe') - expect_validation_error('test' => '4e96~3fe') - expect_validation_error('test' => '^4e963fe') - expect_validation_error('test' => '4:e963fe') - expect_validation_error('test' => '4e963fe.') - expect_validation_error('test' => 'heads/foo..bar') - expect_validation_error('test' => 'foo/bar/.') - expect_validation_error('test' => 'heads/v@{ation') - expect_validation_error('test' => 'refs/heads/foo.') - expect_validation_error('test' => 'heads/foo\bar') - expect_validation_error('test' => 'heads/f[/bar') - expect_validation_error('test' => "heads/foo\t") - expect_validation_error('test' => "heads/foo\177") - expect_validation_error('test' => "#{'a' * 1025}") - expect_validation_error('test' => nil) - expect_validation_error('test' => '') - end - end - end - - describe API::Helpers::CustomValidators::FilePath do - subject do - described_class.new(['test'], {}, false, scope.new) - end - - context 'valid file path' do - it 'does not raise a validation error' do - expect_no_validation_error('test' => './foo') - expect_no_validation_error('test' => './bar.rb') - expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb') - expect_no_validation_error('test' => 'foo%2Fbar%2Fnew') - expect_no_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb') - end - end - - context 'invalid file path' do - it 'raise a validation error' do - expect_validation_error('test' => '../foo') - expect_validation_error('test' => '../') - expect_validation_error('test' => 'foo/../../bar') - expect_validation_error('test' => 'foo/../') - expect_validation_error('test' => 'foo/..') - expect_validation_error('test' => '../') - expect_validation_error('test' => '..\\') - expect_validation_error('test' => '..\/') - expect_validation_error('test' => '%2e%2e%2f') - expect_validation_error('test' => '/etc/passwd') - end - end - end - - describe API::Helpers::CustomValidators::IntegerNoneAny do - subject do - described_class.new(['test'], {}, false, scope.new) - end - - context 'valid parameters' do - it 'does not raise a validation error' do - expect_no_validation_error('test' => 2) - expect_no_validation_error('test' => 100) - expect_no_validation_error('test' => 'None') - expect_no_validation_error('test' => 'Any') - expect_no_validation_error('test' => 'none') - expect_no_validation_error('test' => 'any') - end - end - - context 'invalid parameters' do - it 'raises a validation error' do - expect_validation_error({ 'test' => 'some_other_string' }) - end - end - end - - describe API::Helpers::CustomValidators::ArrayNoneAny do - subject do - described_class.new(['test'], {}, false, scope.new) - end - - context 'valid parameters' do - it 'does not raise a validation error' do - expect_no_validation_error('test' => []) - expect_no_validation_error('test' => [1, 2, 3]) - expect_no_validation_error('test' => 'None') - expect_no_validation_error('test' => 'Any') - expect_no_validation_error('test' => 'none') - expect_no_validation_error('test' => 'any') - end - end - - context 'invalid parameters' do - it 'raises a validation error' do - expect_validation_error('test' => 'some_other_string') - end - end - end - - def expect_no_validation_error(params) - expect { validate_test_param!(params) }.not_to raise_error - end - - def expect_validation_error(params) - expect { validate_test_param!(params) }.to raise_error(Grape::Exceptions::Validation) - end - - def validate_test_param!(params) - subject.validate_param!('test', params) - end -end diff --git a/spec/lib/api/validations/validators/absence_spec.rb b/spec/lib/api/validations/validators/absence_spec.rb new file mode 100644 index 00000000000..31120979d4f --- /dev/null +++ b/spec/lib/api/validations/validators/absence_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Validations::Validators::Absence do + include ApiValidatorsHelpers + + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'empty param' do + it 'does not raise a validation error' do + expect_no_validation_error({}) + end + end + + context 'invalid parameters' do + it 'raises a validation error' do + expect_validation_error('test' => 'some_value') + end + end +end diff --git a/spec/lib/api/validations/validators/array_none_any_spec.rb b/spec/lib/api/validations/validators/array_none_any_spec.rb new file mode 100644 index 00000000000..03f1c63b117 --- /dev/null +++ b/spec/lib/api/validations/validators/array_none_any_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Validations::Validators::ArrayNoneAny do + include ApiValidatorsHelpers + + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'valid parameters' do + it 'does not raise a validation error' do + expect_no_validation_error('test' => []) + expect_no_validation_error('test' => [1, 2, 3]) + expect_no_validation_error('test' => 'None') + expect_no_validation_error('test' => 'Any') + expect_no_validation_error('test' => 'none') + expect_no_validation_error('test' => 'any') + end + end + + context 'invalid parameters' do + it 'raises a validation error' do + expect_validation_error('test' => 'some_other_string') + end + end +end diff --git a/spec/lib/api/validations/validators/file_path_spec.rb b/spec/lib/api/validations/validators/file_path_spec.rb new file mode 100644 index 00000000000..8679f102d23 --- /dev/null +++ b/spec/lib/api/validations/validators/file_path_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Validations::Validators::FilePath do + include ApiValidatorsHelpers + + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'valid file path' do + it 'does not raise a validation error' do + expect_no_validation_error('test' => './foo') + expect_no_validation_error('test' => './bar.rb') + expect_no_validation_error('test' => 'foo%2Fbar%2Fnew%2Ffile.rb') + expect_no_validation_error('test' => 'foo%2Fbar%2Fnew') + expect_no_validation_error('test' => 'foo%252Fbar%252Fnew%252Ffile.rb') + end + end + + context 'invalid file path' do + it 'raise a validation error' do + expect_validation_error('test' => '../foo') + expect_validation_error('test' => '../') + expect_validation_error('test' => 'foo/../../bar') + expect_validation_error('test' => 'foo/../') + expect_validation_error('test' => 'foo/..') + expect_validation_error('test' => '../') + expect_validation_error('test' => '..\\') + expect_validation_error('test' => '..\/') + expect_validation_error('test' => '%2e%2e%2f') + expect_validation_error('test' => '/etc/passwd') + end + end +end diff --git a/spec/lib/api/validations/validators/git_ref_spec.rb b/spec/lib/api/validations/validators/git_ref_spec.rb new file mode 100644 index 00000000000..84de6272fe1 --- /dev/null +++ b/spec/lib/api/validations/validators/git_ref_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Validations::Validators::GitRef do + include ApiValidatorsHelpers + + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'valid revision param' do + it 'does not raise a validation error' do + expect_no_validation_error('test' => '4e963fe') + expect_no_validation_error('test' => 'foo/bar/baz') + expect_no_validation_error('test' => "heads/fu\303\237") + expect_no_validation_error('test' => 'a' * 1024) + end + end + + context "revision param contains invalid chars" do + it 'raises a validation error' do + expect_validation_error('test' => '-4e963fe') + expect_validation_error('test' => '4e963fe..ed4ef') + expect_validation_error('test' => '4e96\3fe') + expect_validation_error('test' => '4e96@3fe') + expect_validation_error('test' => '4e9@{63fe') + expect_validation_error('test' => '4e963 fe') + expect_validation_error('test' => '4e96~3fe') + expect_validation_error('test' => '^4e963fe') + expect_validation_error('test' => '4:e963fe') + expect_validation_error('test' => '4e963fe.') + expect_validation_error('test' => 'heads/foo..bar') + expect_validation_error('test' => 'foo/bar/.') + expect_validation_error('test' => 'heads/v@{ation') + expect_validation_error('test' => 'refs/heads/foo.') + expect_validation_error('test' => 'heads/foo\bar') + expect_validation_error('test' => 'heads/f[/bar') + expect_validation_error('test' => "heads/foo\t") + expect_validation_error('test' => "heads/foo\177") + expect_validation_error('test' => "#{'a' * 1025}") + expect_validation_error('test' => nil) + expect_validation_error('test' => '') + end + end +end diff --git a/spec/lib/api/validations/validators/git_sha_spec.rb b/spec/lib/api/validations/validators/git_sha_spec.rb new file mode 100644 index 00000000000..39c2fe1dcf9 --- /dev/null +++ b/spec/lib/api/validations/validators/git_sha_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Validations::Validators::GitSha do + include ApiValidatorsHelpers + + let(:sha) { RepoHelpers.sample_commit.id } + let(:short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH] } + let(:too_short_sha) { sha[0, Gitlab::Git::Commit::MIN_SHA_LENGTH - 1] } + + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'valid sha' do + it 'does not raise a validation error' do + expect_no_validation_error('test' => sha) + expect_no_validation_error('test' => short_sha) + end + end + + context 'empty params' do + it 'raises a validation error' do + expect_validation_error('test' => nil) + expect_validation_error('test' => '') + end + end + + context 'invalid sha' do + it 'raises a validation error' do + expect_validation_error('test' => "#{sha}2") # Sha length > 40 + expect_validation_error('test' => 'somestring') + expect_validation_error('test' => too_short_sha) # sha length < MIN_SHA_LENGTH (7) + end + end +end diff --git a/spec/lib/api/validations/validators/integer_none_any_spec.rb b/spec/lib/api/validations/validators/integer_none_any_spec.rb new file mode 100644 index 00000000000..a42f69fd96e --- /dev/null +++ b/spec/lib/api/validations/validators/integer_none_any_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Validations::Validators::IntegerNoneAny do + include ApiValidatorsHelpers + + subject do + described_class.new(['test'], {}, false, scope.new) + end + + context 'valid parameters' do + it 'does not raise a validation error' do + expect_no_validation_error('test' => 2) + expect_no_validation_error('test' => 100) + expect_no_validation_error('test' => 'None') + expect_no_validation_error('test' => 'Any') + expect_no_validation_error('test' => 'none') + expect_no_validation_error('test' => 'any') + end + end + + context 'invalid parameters' do + it 'raises a validation error' do + expect_validation_error({ 'test' => 'some_other_string' }) + end + end +end diff --git a/spec/lib/banzai/filter/autolink_filter_spec.rb b/spec/lib/banzai/filter/autolink_filter_spec.rb index 8fba72a23f6..be6192f9ead 100644 --- a/spec/lib/banzai/filter/autolink_filter_spec.rb +++ b/spec/lib/banzai/filter/autolink_filter_spec.rb @@ -192,11 +192,6 @@ describe Banzai::Filter::AutolinkFilter do expect(doc.text).to eq "See <<<#{link}>>>" end - it 'accepts link_attr options' do - doc = filter("See #{link}", link_attr: { class: 'custom' }) - expect(doc.at_css('a')['class']).to eq 'custom' - end - it 'escapes RTLO and other characters' do # rendered text looks like "http://example.com/evilexe.mp3" evil_link = "#{link}evil\u202E3pm.exe" diff --git a/spec/lib/gitlab/git/blob_spec.rb b/spec/lib/gitlab/git/blob_spec.rb index 9c2f0e910b1..f25383ef416 100644 --- a/spec/lib/gitlab/git/blob_spec.rb +++ b/spec/lib/gitlab/git/blob_spec.rb @@ -409,37 +409,43 @@ describe Gitlab::Git::Blob, :seed_helper do end end - describe 'encoding' do + describe 'encoding', :aggregate_failures do context 'file with russian text' do let(:blob) { Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, "encoding/russian.rb") } - it { expect(blob.name).to eq("russian.rb") } - it { expect(blob.data.lines.first).to eq("Хороший файл") } - it { expect(blob.size).to eq(23) } - it { expect(blob.truncated?).to be_falsey } - # Run it twice since data is encoded after the first run - it { expect(blob.truncated?).to be_falsey } - it { expect(blob.mode).to eq("100755") } + it 'has the correct blob attributes' do + expect(blob.name).to eq("russian.rb") + expect(blob.data.lines.first).to eq("Хороший файл") + expect(blob.size).to eq(23) + expect(blob.truncated?).to be_falsey + # Run it twice since data is encoded after the first run + expect(blob.truncated?).to be_falsey + expect(blob.mode).to eq("100755") + end end context 'file with Japanese text' do let(:blob) { Gitlab::Git::Blob.find(repository, SeedRepo::Commit::ID, "encoding/テスト.txt") } - it { expect(blob.name).to eq("テスト.txt") } - it { expect(blob.data).to include("これはテスト") } - it { expect(blob.size).to eq(340) } - it { expect(blob.mode).to eq("100755") } - it { expect(blob.truncated?).to be_falsey } + it 'has the correct blob attributes' do + expect(blob.name).to eq("テスト.txt") + expect(blob.data).to include("これはテスト") + expect(blob.size).to eq(340) + expect(blob.mode).to eq("100755") + expect(blob.truncated?).to be_falsey + end end context 'file with ISO-8859 text' do let(:blob) { Gitlab::Git::Blob.find(repository, SeedRepo::LastCommit::ID, "encoding/iso8859.txt") } - it { expect(blob.name).to eq("iso8859.txt") } - it { expect(blob.loaded_size).to eq(4) } - it { expect(blob.size).to eq(4) } - it { expect(blob.mode).to eq("100644") } - it { expect(blob.truncated?).to be_falsey } + it 'has the correct blob attributes' do + expect(blob.name).to eq("iso8859.txt") + expect(blob.loaded_size).to eq(4) + expect(blob.size).to eq(4) + expect(blob.mode).to eq("100644") + expect(blob.truncated?).to be_falsey + end end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index d9d60e02a97..2bf971f553f 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -137,12 +137,6 @@ describe Ability do expect(users_for_snippet(snippet)).to match_array([author]) end - it 'internal snippet is readable by all registered users' do - snippet = create(:personal_snippet, :public, author: author) - - expect(users_for_snippet(snippet)).to match_array(users) - end - it 'public snippet is readable by all users' do snippet = create(:personal_snippet, :public, author: author) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 88c26c92417..673b9e5f076 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -3337,7 +3337,7 @@ describe Ci::Build do end it "doesn't save timeout" do - expect { run_job_without_exception }.not_to change { job.reload.ensure_metadata.timeout_source } + expect { run_job_without_exception }.not_to change { job.reload.ensure_metadata.timeout } end it "doesn't save timeout_source" do diff --git a/spec/requests/api/deploy_tokens_spec.rb b/spec/requests/api/deploy_tokens_spec.rb index a885e80fd55..5948c3d719f 100644 --- a/spec/requests/api/deploy_tokens_spec.rb +++ b/spec/requests/api/deploy_tokens_spec.rb @@ -175,8 +175,12 @@ describe API::DeployTokens do it { is_expected.to have_gitlab_http_status(:no_content) } - it 'deletes the deploy token' do - expect { subject }.to change { project.deploy_tokens.count }.by(-1) + it 'calls the deploy token destroy service' do + expect(::Projects::DeployTokens::DestroyService).to receive(:new) + .with(project, user, token_id: deploy_token.id) + .and_return(true) + + subject end context 'invalid request' do @@ -187,9 +191,13 @@ describe API::DeployTokens do end it 'returns bad_request with invalid token id' do - delete api("/projects/#{project.id}/deploy_tokens/123abc", user) + expect(::Projects::DeployTokens::DestroyService).to receive(:new) + .with(project, user, token_id: 999) + .and_raise(ActiveRecord::RecordNotFound) + + delete api("/projects/#{project.id}/deploy_tokens/999", user) - expect(response).to have_gitlab_http_status(:bad_request) + expect(response).to have_gitlab_http_status(:not_found) end end end @@ -307,10 +315,12 @@ describe API::DeployTokens do group.add_maintainer(user) end - it 'deletes the deploy token' do - expect { subject }.to change { group.deploy_tokens.count }.by(-1) + it 'calls the deploy token destroy service' do + expect(::Groups::DeployTokens::DestroyService).to receive(:new) + .with(group, user, token_id: group_deploy_token.id) + .and_return(true) - expect(group.deploy_tokens).to be_empty + subject end context 'invalid request' do @@ -321,7 +331,11 @@ describe API::DeployTokens do end it 'returns not found with invalid deploy token id' do - delete api("/groups/#{group.id}/deploy_tokens/bad_id", user) + expect(::Groups::DeployTokens::DestroyService).to receive(:new) + .with(group, user, token_id: 999) + .and_raise(ActiveRecord::RecordNotFound) + + delete api("/groups/#{group.id}/deploy_tokens/999", user) expect(response).to have_gitlab_http_status(:not_found) end diff --git a/spec/serializers/diffs_entity_spec.rb b/spec/serializers/diffs_entity_spec.rb index bb4ac5f9608..b42240037df 100644 --- a/spec/serializers/diffs_entity_spec.rb +++ b/spec/serializers/diffs_entity_spec.rb @@ -26,5 +26,47 @@ describe DiffsEntity do :merge_request_diffs, :definition_path_prefix ) end + + context "when a commit_id is passed" do + let(:commits) { merge_request.commits } + let(:entity) do + described_class.new( + merge_request_diffs.first.diffs, + request: request, + merge_request: merge_request, + merge_request_diffs: merge_request_diffs, + commit: commit + ) + end + + subject { entity.as_json } + + context "when the passed commit is not the first or last in the group" do + let(:commit) { commits.third } + + it 'includes commit references for previous and next' do + expect(subject[:commit][:prev_commit_id]).to eq(commits.second.id) + expect(subject[:commit][:next_commit_id]).to eq(commits.fourth.id) + end + end + + context "when the passed commit is the first in the group" do + let(:commit) { commits.first } + + it 'includes commit references for nil and next' do + expect(subject[:commit][:prev_commit_id]).to be_nil + expect(subject[:commit][:next_commit_id]).to eq(commits.second.id) + end + end + + context "when the passed commit is the last in the group" do + let(:commit) { commits.last } + + it 'includes commit references for previous and nil' do + expect(subject[:commit][:prev_commit_id]).to eq(commits[-2].id) + expect(subject[:commit][:next_commit_id]).to be_nil + end + end + end end end diff --git a/spec/services/groups/deploy_tokens/destroy_service_spec.rb b/spec/services/groups/deploy_tokens/destroy_service_spec.rb new file mode 100644 index 00000000000..d4ef5963558 --- /dev/null +++ b/spec/services/groups/deploy_tokens/destroy_service_spec.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Groups::DeployTokens::DestroyService do + it_behaves_like 'a deploy token deletion service' do + let_it_be(:entity) { create(:group) } + let_it_be(:deploy_token_class) { GroupDeployToken } + let_it_be(:deploy_token) { create(:deploy_token, :group, groups: [entity]) } + end +end diff --git a/spec/services/projects/deploy_tokens/destroy_service_spec.rb b/spec/services/projects/deploy_tokens/destroy_service_spec.rb new file mode 100644 index 00000000000..24407f46615 --- /dev/null +++ b/spec/services/projects/deploy_tokens/destroy_service_spec.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Projects::DeployTokens::DestroyService do + it_behaves_like 'a deploy token deletion service' do + let_it_be(:entity) { create(:project) } + let_it_be(:deploy_token_class) { ProjectDeployToken } + let_it_be(:deploy_token) { create(:deploy_token, projects: [entity]) } + end +end diff --git a/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb b/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb new file mode 100644 index 00000000000..e98b8bd00dc --- /dev/null +++ b/spec/services/resource_events/synthetic_milestone_notes_builder_service_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ResourceEvents::SyntheticMilestoneNotesBuilderService do + describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:issue) { create(:issue, author: user) } + + before do + create_list(:resource_milestone_event, 3, issue: issue) + + stub_feature_flags(track_resource_milestone_change_events: false) + end + + context 'when resource milestone events are disabled' do + # https://gitlab.com/gitlab-org/gitlab/-/issues/212985 + it 'still builds notes for existing resource milestone events' do + notes = described_class.new(issue, user).execute + + expect(notes.size).to eq(3) + end + end + end +end diff --git a/spec/support/helpers/api_validators_helpers.rb b/spec/support/helpers/api_validators_helpers.rb new file mode 100644 index 00000000000..dc05e5afd57 --- /dev/null +++ b/spec/support/helpers/api_validators_helpers.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module ApiValidatorsHelpers + def scope + Struct.new(:opts) do + def full_name(attr_name) + attr_name + end + end + end + + def expect_no_validation_error(params) + expect { validate_test_param!(params) }.not_to raise_error + end + + def expect_validation_error(params) + expect { validate_test_param!(params) }.to raise_error(Grape::Exceptions::Validation) + end + + def validate_test_param!(params) + subject.validate_param!('test', params) + end +end diff --git a/spec/support/services/deploy_token_shared_examples.rb b/spec/support/services/deploy_token_shared_examples.rb index 70efd1fcd0c..9d681970739 100644 --- a/spec/support/services/deploy_token_shared_examples.rb +++ b/spec/support/services/deploy_token_shared_examples.rb @@ -58,3 +58,25 @@ RSpec.shared_examples 'a deploy token creation service' do end end end + +RSpec.shared_examples 'a deploy token deletion service' do + let(:user) { create(:user) } + let(:deploy_token_params) { { token_id: deploy_token.id } } + + describe '#execute' do + subject { described_class.new(entity, user, deploy_token_params).execute } + + it "destroys a token record and it's associated DeployToken" do + expect { subject }.to change { deploy_token_class.count }.by(-1) + .and change { DeployToken.count }.by(-1) + end + + context 'invalid token id' do + let(:deploy_token_params) { { token_id: 9999 } } + + it 'raises an error' do + expect { subject }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end |