Refactor TagCheck logic

This cleans refactors the logic in TagCheck to improve readability and
code structure.

.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'

.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'

.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'

.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'

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