From 1e0697db8b97129ee6e88e9806f9b0a0d49dc4fa Mon Sep 17 00:00:00 2001
From: Joe Woodward <jwoodward@gitlab.com>
Date: Sat, 2 Dec 2023 10:15:49 +0000
Subject: [PATCH] Refactor TagCheck logic
This cleans refactors the logic in TagCheck to improve readability and
code structure.
---
.../layout/line_continuation_spacing.yml | 1 -
...e_end_string_concatenation_indentation.yml | 1 -
.rubocop_todo/style/if_unless_modifier.yml | 1 -
.../style/inline_disable_annotation.yml | 1 -
lib/gitlab/checks/tag_check.rb | 108 ++++++++++--------
5 files changed, 62 insertions(+), 50 deletions(-)
diff --git a/.rubocop_todo/layout/line_continuation_spacing.yml b/.rubocop_todo/layout/line_continuation_spacing.yml
index c406fd1cae9fa..46e997246ea06 100644
--- a/.rubocop_todo/layout/line_continuation_spacing.yml
+++ b/.rubocop_todo/layout/line_continuation_spacing.yml
@@ -107,7 +107,6 @@ Layout/LineContinuationSpacing:
- 'lib/api/metrics/dashboard/annotations.rb'
- 'lib/gitlab/auth/user_access_denied_reason.rb'
- 'lib/gitlab/background_migration/populate_operation_visibility_permissions_from_operations.rb'
- - 'lib/gitlab/checks/tag_check.rb'
- 'lib/gitlab/ci/parsers/security/validators/schema_validator.rb'
- 'lib/gitlab/database/background_migration/batched_migration_runner.rb'
- 'lib/gitlab/database/load_balancing/sidekiq_server_middleware.rb'
diff --git a/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml b/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml
index e38219963fdfa..68d0f57006e21 100644
--- a/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml
+++ b/.rubocop_todo/layout/line_end_string_concatenation_indentation.yml
@@ -155,7 +155,6 @@ Layout/LineEndStringConcatenationIndentation:
- 'lib/gitlab/auth.rb'
- 'lib/gitlab/background_migration/populate_operation_visibility_permissions_from_operations.rb'
- 'lib/gitlab/changelog/config.rb'
- - 'lib/gitlab/checks/tag_check.rb'
- 'lib/gitlab/ci/parsers/security/validators/schema_validator.rb'
- 'lib/gitlab/ci/pipeline/chain/populate.rb'
- 'lib/gitlab/ci/pipeline/seed/build.rb'
diff --git a/.rubocop_todo/style/if_unless_modifier.yml b/.rubocop_todo/style/if_unless_modifier.yml
index bf767c1701e88..6c52c2d9f8e7e 100644
--- a/.rubocop_todo/style/if_unless_modifier.yml
+++ b/.rubocop_todo/style/if_unless_modifier.yml
@@ -730,7 +730,6 @@ Style/IfUnlessModifier:
- 'lib/gitlab/checks/matching_merge_request.rb'
- 'lib/gitlab/checks/push_check.rb'
- 'lib/gitlab/checks/push_file_count_check.rb'
- - 'lib/gitlab/checks/tag_check.rb'
- 'lib/gitlab/ci/ansi2html.rb'
- 'lib/gitlab/ci/ansi2json/converter.rb'
- 'lib/gitlab/ci/ansi2json/style.rb'
diff --git a/.rubocop_todo/style/inline_disable_annotation.yml b/.rubocop_todo/style/inline_disable_annotation.yml
index f5a4378575608..d86a6772ee2f9 100644
--- a/.rubocop_todo/style/inline_disable_annotation.yml
+++ b/.rubocop_todo/style/inline_disable_annotation.yml
@@ -2411,7 +2411,6 @@ Style/InlineDisableAnnotation:
- 'lib/gitlab/checks/branch_check.rb'
- 'lib/gitlab/checks/diff_check.rb'
- 'lib/gitlab/checks/matching_merge_request.rb'
- - 'lib/gitlab/checks/tag_check.rb'
- 'lib/gitlab/ci/ansi2html.rb'
- 'lib/gitlab/ci/ansi2json/parser.rb'
- 'lib/gitlab/ci/badge/pipeline/status.rb'
diff --git a/lib/gitlab/checks/tag_check.rb b/lib/gitlab/checks/tag_check.rb
index d5addab74b8fc..5684b89725666 100644
--- a/lib/gitlab/checks/tag_check.rb
+++ b/lib/gitlab/checks/tag_check.rb
@@ -6,8 +6,8 @@ class TagCheck < BaseSingleChecker
ERROR_MESSAGES = {
change_existing_tags: 'You are not allowed to change existing tags on this project.',
update_protected_tag: 'Protected tags cannot be updated.',
- delete_protected_tag: 'You are not allowed to delete protected tags from this project. '\
- 'Only a project maintainer or owner can delete a protected tag.',
+ delete_protected_tag: 'You are not allowed to delete protected tags from this project. ' \
+ 'Only a project maintainer or owner can delete a protected tag.',
delete_protected_tag_non_web: 'You can only delete protected tags using the web interface.',
create_protected_tag: 'You are not allowed to create this tag as it is protected.',
default_branch_collision: 'You cannot use default branch name to create a tag',
@@ -24,69 +24,85 @@ class TagCheck < BaseSingleChecker
def validate!
return unless tag_name
- logger.log_timed(LOG_MESSAGES[:tag_checks]) do
- if tag_exists? && user_access.cannot_do_action?(:admin_tag)
- raise GitAccess::ForbiddenError, ERROR_MESSAGES[:change_existing_tags]
- end
- end
-
- default_branch_collision_check
+ logger.log_timed(LOG_MESSAGES[:tag_checks]) { tag_checks }
+ logger.log_timed(LOG_MESSAGES[:default_branch_collision_check]) { default_branch_collision_check }
prohibited_tag_checks
- protected_tag_checks
+ logger.log_timed(LOG_MESSAGES[:protected_tag_checks]) { protected_tag_checks }
end
private
+ def tag_checks
+ return unless tag_exists? && user_access.cannot_do_action?(:admin_tag)
+
+ raise GitAccess::ForbiddenError, ERROR_MESSAGES[:change_existing_tags]
+ end
+
+ def default_branch_collision_check
+ return unless creation? && tag_name == project.default_branch
+
+ raise GitAccess::ForbiddenError, ERROR_MESSAGES[:default_branch_collision]
+ end
+
def prohibited_tag_checks
return if deletion?
- unless Gitlab::GitRefValidator.validate(tag_name)
- raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name]
- end
+ # Incorrectly encoded tags names may raise during other checks so we
+ # need to validate the encoding first
+ validate_encoding!
+ validate_valid_tag_name!
+ validate_tag_name_not_fully_qualified!
+ end
- if tag_name.start_with?("refs/tags/") # rubocop: disable Style/GuardClause
- raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name]
- end
+ def protected_tag_checks
+ return unless ProtectedTag.protected?(project, tag_name)
- # rubocop: disable Style/GuardClause
- # rubocop: disable Style/SoleNestedConditional
- if Feature.enabled?(:prohibited_tag_name_encoding_check, project)
- unless Gitlab::EncodingHelper.force_encode_utf8(tag_name).valid_encoding?
- raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name_encoding]
- end
- end
- # rubocop: enable Style/SoleNestedConditional
- # rubocop: enable Style/GuardClause
+ validate_protected_tag_update!
+ validate_protected_tag_deletion!
+ validate_protected_tag_creation!
end
- def protected_tag_checks
- logger.log_timed(LOG_MESSAGES[__method__]) do
- return unless ProtectedTag.protected?(project, tag_name) # rubocop:disable Cop/AvoidReturnFromBlocks
+ def validate_encoding!
+ return unless Feature.enabled?(:prohibited_tag_name_encoding_check, project)
+ return if Gitlab::EncodingHelper.force_encode_utf8(tag_name).valid_encoding?
- raise(GitAccess::ForbiddenError, ERROR_MESSAGES[:update_protected_tag]) if update?
+ raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name_encoding]
+ end
+
+ def validate_valid_tag_name!
+ return if Gitlab::GitRefValidator.validate(tag_name)
- if deletion?
- unless user_access.user.can?(:maintainer_access, project)
- raise(GitAccess::ForbiddenError, ERROR_MESSAGES[:delete_protected_tag])
- end
+ raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name]
+ end
- unless updated_from_web?
- raise GitAccess::ForbiddenError, ERROR_MESSAGES[:delete_protected_tag_non_web]
- end
- end
+ def validate_tag_name_not_fully_qualified!
+ return unless tag_name.start_with?("refs/tags/")
- unless user_access.can_create_tag?(tag_name)
- raise GitAccess::ForbiddenError, ERROR_MESSAGES[:create_protected_tag]
- end
- end
+ raise GitAccess::ForbiddenError, ERROR_MESSAGES[:prohibited_tag_name]
end
- def default_branch_collision_check
- logger.log_timed(LOG_MESSAGES[:default_branch_collision_check]) do
- if creation? && tag_name == project.default_branch
- raise GitAccess::ForbiddenError, ERROR_MESSAGES[:default_branch_collision]
- end
+ def validate_protected_tag_update!
+ return unless update?
+
+ raise(GitAccess::ForbiddenError, ERROR_MESSAGES[:update_protected_tag])
+ end
+
+ def validate_protected_tag_deletion!
+ return unless deletion?
+
+ unless user_access.user.can?(:maintainer_access, project)
+ raise(GitAccess::ForbiddenError, ERROR_MESSAGES[:delete_protected_tag])
end
+
+ return if updated_from_web?
+
+ raise GitAccess::ForbiddenError, ERROR_MESSAGES[:delete_protected_tag_non_web]
+ end
+
+ def validate_protected_tag_creation!
+ return if user_access.can_create_tag?(tag_name)
+
+ raise GitAccess::ForbiddenError, ERROR_MESSAGES[:create_protected_tag]
end
end
end
--
GitLab