Commit 8a521e08 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'validate-accidental-override-in-presenters' into 'master'

Validate accidental override by presenters

See merge request gitlab-org/gitlab!69823
parents b90a8bd1 c15d7aba
......@@ -66,14 +66,15 @@ we gain the following benefits:
### Presenter definition
Every presenter should inherit from `Gitlab::View::Presenter::Simple`, which
provides a `.presents` the method which allows you to define an accessor for the
If you need a presenter class that has only necessary interfaces for the view-related context,
inherit from `Gitlab::View::Presenter::Simple`.
It provides a `.presents` the method which allows you to define an accessor for the
presented object. It also includes common helpers like `Gitlab::Routing` and
`Gitlab::Allowable`.
```ruby
class LabelPresenter < Gitlab::View::Presenter::Simple
presents :label
presents ::Label, as: :label
def text_color
label.color.to_s
......@@ -85,13 +86,14 @@ class LabelPresenter < Gitlab::View::Presenter::Simple
end
```
In some cases, it can be more practical to transparently delegate all missing
method calls to the presented object, in these cases, you can make your
presenter inherit from `Gitlab::View::Presenter::Delegated`:
If you need a presenter class that delegates missing method calls to the presented object,
inherit from `Gitlab::View::Presenter::Delegated`.
This is more like an "extension" in the sense that the produced object is going to have
all of interfaces of the presented object **AND** all of the interfaces in the presenter class:
```ruby
class LabelPresenter < Gitlab::View::Presenter::Delegated
presents :label
presents ::Label, as: :label
def text_color
# color is delegated to label
......@@ -152,3 +154,82 @@ You can also present the model in the view:
%div{ class: label.text_color }
= render partial: label, label: label
```
### Validate accidental overrides
We use presenters in many places, such as Controller, Haml, GraphQL/Rest API,
it's very handy to extend the core/backend logic of Active Record models,
however, there is a risk that it accidentally overrides important logic.
For example, [this production incident](https://gitlab.com/gitlab-com/gl-infra/production/-/issues/5498)
was caused by [including `ActionView::Helpers::UrlHelper` in a presenter](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69537/diffs#4b581cff00ef3cc9780efd23682af383de302e7d_3_3).
The `tag` accesor in `Ci::Build` was accidentally overridden by `ActionView::Helpers::TagHelper#tag`,
and as a conseuqence, a wrong `tag` value was persited into database.
Starting from GitLab 14.4, we validate the presenters (specifically all of the subclasses of `Gitlab::View::Presenter::Delegated`)
that they do not accidentally override core/backend logic. In such case, a pipeline in merge requests fails with an error message,
here is an example:
```plaintext
We've detected that a presetner is overriding a specific method(s) on a subject model.
There is a risk that it accidentally modifies the backend/core logic that leads to production incident.
Please follow https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides
to resolve this error with caution.
Here are the conflict details.
- Ci::PipelinePresenter#tag is overriding Ci::Pipeline#tag. delegator_location: /devkitkat/services/rails/cache/ruby/2.7.0/gems/actionview-6.1.3.2/lib/action_view/helpers/tag_helper.rb:271 original_location: /devkitkat/services/rails/cache/ruby/2.7.0/gems/activemodel-6.1.3.2/lib/active_model/attribute_methods.rb:254
```
Here are the potential solutions:
- If the conflict happens on an instance method in the presenter:
- If you intend to override the core/backend logic, define `delegator_override <method-name>` on top of the conflicted method.
This explicitly adds the method to an allowlist.
- If you do NOT intend to override the core/backend logic, rename the method name in the presenter.
- If the conflict happens on an included module in the presenter, remove the module from the presenter and find a workaround.
### How to use the `Gitlab::Utils::DelegatorOverride` validator
If a presenter class inhertis from `Gitlab::View::Presenter::Delegated`,
you should define what object class is presented:
```ruby
class WebHookLogPresenter < Gitlab::View::Presenter::Delegated
presents ::WebHookLog, as: :web_hook_log # This defines that the presenter presents `WebHookLog` Active Record model.
```
These presenters are validated not to accidentaly override the methods in the presented object.
You can run the validation locally with:
```shell
bundle exec rake lint:static_verification
```
To add a method to an allowlist, use `delegator_override`. For example:
```ruby
class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated
presents ::Vulnerability, as: :vulnerability
delegator_override :description # This adds the `description` method to an allowlist that the override is intentional.
def description
vulnerability.description || finding.description
end
```
To add methods of a module to an allowlist, use `delegator_override_with`. For example:
```ruby
module Ci
class PipelinePresenter < Gitlab::View::Presenter::Delegated
include Gitlab::Utils::StrongMemoize
include ActionView::Helpers::UrlHelper
delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate
delegator_override_with ActionView::Helpers::TagHelper # TODO: Remove `ActionView::Helpers::UrlHelper` inclusion as it overrides `Ci::Pipeline#tag`
```
Keep in mind that if you use `delegator_override_with`,
there is a high chance that you're doing **something wrong**.
Read the [Validate Accidental Overrides](#validate-accidental-overrides) for more information.
......@@ -5,6 +5,9 @@ module AlertManagement
include IncidentManagement::Settings
include ActionView::Helpers::UrlHelper
presents ::AlertManagement::Alert
delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate
MARKDOWN_LINE_BREAK = " \n"
HORIZONTAL_LINE = "\n\n---\n\n"
......@@ -29,6 +32,7 @@ module AlertManagement
started_at&.strftime('%d %B %Y, %-l:%M%p (%Z)')
end
delegator_override :details_url
def details_url
details_project_alert_management_url(project, alert.iid)
end
......
# frozen_string_literal: true
class AwardEmojiPresenter < Gitlab::View::Presenter::Delegated
presents :award_emoji
presents ::AwardEmoji, as: :award_emoji
def description
as_emoji['description']
......
......@@ -7,7 +7,7 @@ class BlobPresenter < Gitlab::View::Presenter::Delegated
include TreeHelper
include ChecksCollaboration
presents :blob
presents ::Blob, as: :blob
def highlight(to: nil, plain: nil)
load_all_blob_data
......
......@@ -6,6 +6,8 @@ module Blobs
include ActiveModel::AttributeAssignment
include Gitlab::Utils::StrongMemoize
presents ::Blob
attribute :full, :boolean, default: false
attribute :since, :integer, default: 1
attribute :to, :integer, default: 1
......
# frozen_string_literal: true
class BoardPresenter < Gitlab::View::Presenter::Delegated
presents :board
presents ::Board, as: :board
end
......@@ -2,6 +2,9 @@
module Ci
class BridgePresenter < ProcessablePresenter
presents ::Ci::Bridge
delegator_override :detailed_status
def detailed_status
@detailed_status ||= subject.detailed_status(user)
end
......
......@@ -9,8 +9,9 @@ module Ci
job_timeout_source: 'job'
}.freeze
presents :metadata
presents ::Ci::BuildMetadata, as: :metadata
delegator_override :timeout_source
def timeout_source
return unless metadata.timeout_source?
......
......@@ -2,6 +2,8 @@
module Ci
class BuildPresenter < ProcessablePresenter
presents ::Ci::Build
def erased_by_user?
# Build can be erased through API, therefore it does not have
# `erased_by` user assigned in that case.
......
......@@ -2,7 +2,7 @@
module Ci
class GroupVariablePresenter < Gitlab::View::Presenter::Delegated
presents :variable
presents ::Ci::GroupVariable, as: :variable
def placeholder
'GROUP_VARIABLE'
......
......@@ -2,7 +2,7 @@
module Ci
class LegacyStagePresenter < Gitlab::View::Presenter::Delegated
presents :legacy_stage
presents ::Ci::LegacyStage, as: :legacy_stage
def latest_ordered_statuses
preload_statuses(legacy_stage.statuses.latest_ordered)
......
......@@ -5,6 +5,9 @@ module Ci
include Gitlab::Utils::StrongMemoize
include ActionView::Helpers::UrlHelper
delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate
delegator_override_with ActionView::Helpers::TagHelper # TODO: Remove `ActionView::Helpers::UrlHelper` inclusion as it overrides `Ci::Pipeline#tag`
# We use a class method here instead of a constant, allowing EE to redefine
# the returned `Hash` more easily.
def self.failure_reasons
......@@ -20,8 +23,9 @@ module Ci
user_blocked: 'The user who created this pipeline is blocked.' }
end
presents :pipeline
presents ::Ci::Pipeline, as: :pipeline
delegator_override :failed_builds
def failed_builds
return [] unless can?(current_user, :read_build, pipeline)
......@@ -30,6 +34,7 @@ module Ci
end
end
delegator_override :failure_reason
def failure_reason
return unless pipeline.failure_reason?
......
......@@ -2,5 +2,6 @@
module Ci
class ProcessablePresenter < CommitStatusPresenter
presents ::Ci::Processable
end
end
......@@ -2,11 +2,14 @@
module Ci
class RunnerPresenter < Gitlab::View::Presenter::Delegated
presents :runner
presents ::Ci::Runner, as: :runner
delegator_override :locked?
def locked?
read_attribute(:locked) && project_type?
end
delegator_override :locked
alias_method :locked, :locked?
end
end
......@@ -2,7 +2,7 @@
module Ci
class StagePresenter < Gitlab::View::Presenter::Delegated
presents :stage
presents ::Ci::Stage, as: :stage
PRELOADED_RELATIONS = [:pipeline, :metadata, :tags, :job_artifacts_archive, :downstream_pipeline].freeze
......
......@@ -2,12 +2,13 @@
module Ci
class TriggerPresenter < Gitlab::View::Presenter::Delegated
presents :trigger
presents ::Ci::Trigger, as: :trigger
def has_token_exposed?
can?(current_user, :admin_trigger, trigger)
end
delegator_override :token
def token
if has_token_exposed?
trigger.token
......
......@@ -2,7 +2,7 @@
module Ci
class VariablePresenter < Gitlab::View::Presenter::Delegated
presents :variable
presents ::Ci::Variable, as: :variable
def placeholder
'PROJECT_VARIABLE'
......
# frozen_string_literal: true
class ClusterablePresenter < Gitlab::View::Presenter::Delegated
presents :clusterable
presents ::Project, ::Group, ::Clusters::Instance, as: :clusterable
def self.fabricate(clusterable, **attributes)
presenter_class = "#{clusterable.class.name}ClusterablePresenter".constantize
......
......@@ -7,7 +7,9 @@ module Clusters
include ActionView::Helpers::UrlHelper
include IconsHelper
presents :cluster
delegator_override_with ::Gitlab::Utils::StrongMemoize # TODO: Remove `::Gitlab::Utils::StrongMemoize` inclusion as it's duplicate
presents ::Clusters::Cluster, as: :cluster
# We do not want to show the group path for clusters belonging to the
# clusterable, only for the ancestor clusters.
......
......@@ -2,7 +2,7 @@
module Clusters
class IntegrationPresenter < Gitlab::View::Presenter::Delegated
presents :integration
presents ::Clusters::Integrations::Prometheus, ::Clusters::Integrations::ElasticStack, as: :integration
def application_type
integration.class.name.demodulize.underscore
......
......@@ -3,7 +3,7 @@
class CommitPresenter < Gitlab::View::Presenter::Delegated
include GlobalID::Identification
presents :commit
presents ::Commit, as: :commit
def status_for(ref)
return unless can?(current_user, :read_commit_status, commit.project)
......
......@@ -33,7 +33,7 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated
private_constant :CALLOUT_FAILURE_MESSAGES
presents :build
presents ::CommitStatus, as: :build
def self.callout_failure_messages
CALLOUT_FAILURE_MESSAGES
......
......@@ -2,6 +2,8 @@
module DevOpsReport
class MetricPresenter < Gitlab::View::Presenter::Simple
presents ::DevOpsReport::Metric
delegate :created_at, to: :subject
def cards
......
......@@ -3,7 +3,7 @@
class EnvironmentPresenter < Gitlab::View::Presenter::Delegated
include ActionView::Helpers::UrlHelper
presents :environment
presents ::Environment, as: :environment
def path
project_environment_path(project, self)
......
# frozen_string_literal: true
class EventPresenter < Gitlab::View::Presenter::Delegated
presents :event
presents ::Event, as: :event
def initialize(subject, **attributes)
super
......@@ -10,6 +10,7 @@ class EventPresenter < Gitlab::View::Presenter::Delegated
end
# Caching `visible_to_user?` method in the presenter beause it might be called multiple times.
delegator_override :visible_to_user?
def visible_to_user?(user = nil)
@visible_to_user_cache.fetch(user&.id) { super(user) }
end
......
......@@ -12,7 +12,7 @@ module Gitlab
include TreeHelper
include IconsHelper
presents :blame
presents nil, as: :blame
CommitData = Struct.new(
:author_avatar,
......
......@@ -4,6 +4,8 @@ class GroupClusterablePresenter < ClusterablePresenter
extend ::Gitlab::Utils::Override
include ActionView::Helpers::UrlHelper
presents ::Group
override :cluster_status_cluster_path
def cluster_status_cluster_path(cluster, params = {})
cluster_status_group_cluster_path(clusterable, cluster, params)
......
# frozen_string_literal: true
class GroupMemberPresenter < MemberPresenter
presents ::GroupMember
private
def admin_member_permission
......
......@@ -4,6 +4,8 @@ class InstanceClusterablePresenter < ClusterablePresenter
extend ::Gitlab::Utils::Override
include ActionView::Helpers::UrlHelper
presents ::Clusters::Instance
def self.fabricate(clusterable, **attributes)
attributes_with_presenter_class = attributes.merge(presenter_class: InstanceClusterablePresenter)
......
# frozen_string_literal: true
class InvitationPresenter < Gitlab::View::Presenter::Delegated
presents :invitation
presents nil, as: :invitation
end
# frozen_string_literal: true
class IssuePresenter < Gitlab::View::Presenter::Delegated
presents :issue
presents ::Issue, as: :issue
def issue_path
url_builder.build(issue, only_path: true)
end
delegator_override :subscribed?
def subscribed?
issue.subscribed?(current_user, issue.project)
end
......
# frozen_string_literal: true
class LabelPresenter < Gitlab::View::Presenter::Delegated
presents :label
presents ::Label, as: :label
delegate :name, :full_name, to: :label_subject, prefix: :subject
delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `Label#subject`.
def edit_path
case label
when GroupLabel then edit_group_label_path(label.group, label)
......
# frozen_string_literal: true
class MemberPresenter < Gitlab::View::Presenter::Delegated
presents :member
presents ::Member, as: :member
def access_level_roles
member.class.access_level_roles
......
......@@ -3,7 +3,7 @@
class MembersPresenter < Gitlab::View::Presenter::Delegated
include Enumerable
presents :members
presents nil, as: :members
def to_ary
to_a
......
......@@ -8,9 +8,11 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
include ChecksCollaboration
include Gitlab::Utils::StrongMemoize
delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate
APPROVALS_WIDGET_BASE_TYPE = 'base'
presents :merge_request
presents ::MergeRequest, as: :merge_request
def ci_status
if pipeline
......@@ -183,6 +185,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
.can_push_to_branch?(source_branch)
end
delegator_override :can_remove_source_branch?
def can_remove_source_branch?
source_branch_exists? && merge_request.can_remove_source_branch?(current_user)
end
......@@ -202,6 +205,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end
end
delegator_override :subscribed?
def subscribed?
merge_request.subscribed?(current_user, merge_request.target_project)
end
......
# frozen_string_literal: true
class MilestonePresenter < Gitlab::View::Presenter::Delegated
presents :milestone
presents ::Milestone, as: :milestone
def milestone_path
url_builder.build(milestone, only_path: true)
......
# frozen_string_literal: true
class PagesDomainPresenter < Gitlab::View::Presenter::Delegated
presents :pages_domain
presents ::PagesDomain, as: :pages_domain
delegator_override :subject # TODO: Fix `Gitlab::View::Presenter::Delegated#subject` not to override `PagesDomain#subject`.
def needs_verification?
Gitlab::CurrentSettings.pages_domain_verification_enabled? && unverified?
......
......@@ -4,6 +4,8 @@ class ProjectClusterablePresenter < ClusterablePresenter
extend ::Gitlab::Utils::Override
include ActionView::Helpers::UrlHelper
presents ::Project
override :cluster_status_cluster_path
def cluster_status_cluster_path(cluster, params = {})
cluster_status_project_cluster_path(clusterable, cluster, params)
......
# frozen_string_literal: true
class ProjectHookPresenter < Gitlab::View::Presenter::Delegated
presents :project_hook
presents ::ProjectHook, as: :project_hook
def logs_details_path(log)
project_hook_hook_log_path(project, self, log)
......
# frozen_string_literal: true
class ProjectMemberPresenter < MemberPresenter
presents ::ProjectMember
private
def admin_member_permission
......
......@@ -12,7 +12,10 @@ class ProjectPresenter < Gitlab::View::Presenter::Delegated
include Gitlab::Utils::StrongMemoize
include Gitlab::Experiment::Dsl
presents :project
delegator_override_with GitlabRoutingHelper # TODO: Remove `GitlabRoutingHelper` inclusion as it's duplicate
delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate
presents ::Project, as: :project
AnchorData = Struct.new(:is_link, :label, :link, :class_modifier, :icon, :itemprop, :data)
MAX_TOPICS_TO_SHOW = 3
......
......@@ -5,16 +5,24 @@ module Projects
class ProjectExportPresenter < Gitlab::View::Presenter::Delegated
include ActiveModel::Serializers::JSON
presents :project
presents ::Project, as: :project
# TODO: Remove `ActiveModel::Serializers::JSON` inclusion as it's duplicate
delegator_override_with ActiveModel::Serializers::JSON
delegator_override_with ActiveModel::Naming
delegator_override :include_root_in_json, :include_root_in_json?
delegator_override :project_members
def project_members
super + converted_group_members
end
delegator_override :description
def description
self.respond_to?(:override_description) ? override_description : super
end
delegator_override :protected_branches
def protected_branches
project.exported_protected_branches
end
......
......@@ -5,7 +5,7 @@ module Projects
class DeployKeysPresenter < Gitlab::View::Presenter::Simple
include Gitlab::Utils::StrongMemoize
presents :project
presents ::Project, as: :project
delegate :size, to: :enabled_keys, prefix: true
delegate :size, to: :available_project_keys, prefix: true
delegate :size, to: :available_public_keys, prefix: true
......
......@@ -3,7 +3,7 @@
class PrometheusAlertPresenter < Gitlab::View::Presenter::Delegated
include ActionView::Helpers::UrlHelper
presents :prometheus_alert
presents ::PrometheusAlert, as: :prometheus_alert
def humanized_text
operator_text =
......
......@@ -3,8 +3,10 @@
class ReleasePresenter < Gitlab::View::Presenter::Delegated
include ActionView::Helpers::UrlHelper
presents :release
presents ::Release, as: :release
# TODO: Remove `delegate` as it's redundant due to SimpleDelegator.
delegator_override :tag, :project
delegate :project, :tag, to: :release
def commit_path
......@@ -51,6 +53,7 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
edit_project_release_url(project, release)
end
delegator_override :assets_count
def assets_count
if can_download_code?
release.assets_count
......@@ -59,6 +62,7 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
end
end
delegator_override :name
def name
can_download_code? ? release.name : "Release-#{release.id}"
end
......
......@@ -4,7 +4,7 @@ module Releases
class EvidencePresenter < Gitlab::View::Presenter::Delegated
include ActionView::Helpers::UrlHelper
presents :evidence
presents ::Releases::Evidence, as: :evidence
def filepath
release = evidence.release
......
......@@ -3,7 +3,7 @@
class SearchServicePresenter < Gitlab::View::Presenter::Delegated
include RendersCommits
presents :search_service
presents ::SearchService, as: :search_service
SCOPE_PRELOAD_METHOD = {
projects: :with_web_entity_associations,
......@@ -18,6 +18,7 @@ class SearchServicePresenter < Gitlab::View::Presenter::Delegated
SORT_ENABLED_SCOPES = %w(issues merge_requests epics).freeze
delegator_override :search_objects
def search_objects
@search_objects ||= begin
objects = search_service.search_objects(SCOPE_PRELOAD_METHOD[scope.to_sym])
......
# frozen_string_literal: true
class SentryErrorPresenter < Gitlab::View::Presenter::Delegated
presents :error
presents nil, as: :error
FrequencyStruct = Struct.new(:time, :count, keyword_init: true)
......
# frozen_string_literal: true
class ServiceHookPresenter < Gitlab::View::Presenter::Delegated
presents :service_hook
presents ::ServiceHook, as: :service_hook
def logs_details_path(log)
project_service_hook_log_path(integration.project, integration, log)
......
......@@ -3,6 +3,8 @@
class SnippetBlobPresenter < BlobPresenter
include GitlabRoutingHelper
presents ::SnippetBlob
def rich_data
return unless blob.rich_viewer
......
# frozen_string_literal: true
class SnippetPresenter < Gitlab::View::Presenter::Delegated
presents :snippet
presents ::Snippet, as: :snippet
def raw_url
url_builder.build(snippet, raw: true)
end
delegator_override :ssh_url_to_repo
def ssh_url_to_repo
snippet.ssh_url_to_repo if snippet.repository_exists?
end
delegator_override :http_url_to_repo
def http_url_to_repo
snippet.http_url_to_repo if snippet.repository_exists?
end
......@@ -31,6 +33,7 @@ class SnippetPresenter < Gitlab::View::Presenter::Delegated
snippet.submittable_as_spam_by?(current_user)
end
delegator_override :blob
def blob
return snippet.blob if snippet.empty_repo?
......
......@@ -4,7 +4,7 @@ module Terraform
class ModulesPresenter < Gitlab::View::Presenter::Simple
attr_accessor :packages, :system
presents :modules
presents nil, as: :modules
def initialize(packages, system)
@packages = packages
......
# frozen_string_literal: true
class TodoPresenter < Gitlab::View::Presenter::Delegated
presents :todo
presents ::Todo, as: :todo
end
# frozen_string_literal: true
class TreeEntryPresenter < Gitlab::View::Presenter::Delegated
presents :tree
presents nil, as: :tree
def web_url
Gitlab::Routing.url_helpers.project_tree_url(tree.repository.project, File.join(tree.commit_id, tree.path))
......
# frozen_string_literal: true
class UserPresenter < Gitlab::View::Presenter::Delegated
presents :user
presents ::User, as: :user
def group_memberships
should_be_private? ? GroupMember.none : user.group_members
......
# frozen_string_literal: true
class WebHookLogPresenter < Gitlab::View::Presenter::Delegated
presents :web_hook_log
presents ::WebHookLog, as: :web_hook_log
def details_path
web_hook.present.logs_details_path(self)
......
......@@ -183,6 +183,8 @@ queries they produce.
Everything in `app/presenters`, used for exposing complex data to a Rails view,
without having to create many instance variables.
See [the documentation](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md) for more information.
### Serializers
Everything in `app/serializers`, used for presenting the response to a request,
......
# frozen_string_literal: true
class AuditEventPresenter < Gitlab::View::Presenter::Simple
presents :audit_event
presents ::AuditEvent, as: :audit_event
def author_name
audit_event.author_name
......
......@@ -2,6 +2,6 @@
module Boards
class EpicBoardPresenter < Gitlab::View::Presenter::Delegated
presents :epic_board
presents ::Boards::EpicBoard, as: :epic_board
end
end
......@@ -5,7 +5,7 @@ module Dast
REDACTED_PASSWORD = '••••••••'
REDACTED_REQUEST_HEADERS = '••••••••'
presents :site_profile
presents ::DastSiteProfile, as: :site_profile
def password
return unless site_profile.secret_variables.any? { |variable| variable.key == ::Dast::SiteProfileSecretVariable::PASSWORD }
......
......@@ -4,7 +4,9 @@ module EE
module Ci
module BuildPresenter
extend ActiveSupport::Concern
extend ::Gitlab::Utils::DelegatorOverride
delegator_override :retryable?
def retryable?
!merge_train_pipeline? && super
end
......
......@@ -4,6 +4,7 @@ module EE
module Ci
module PipelinePresenter
extend ActiveSupport::Concern
extend ::Gitlab::Utils::DelegatorOverride
def expose_security_dashboard?
return false unless can?(current_user, :read_security_resource, pipeline.project)
......@@ -18,6 +19,7 @@ module EE
end
end
delegator_override :retryable?
def retryable?
!merge_train_pipeline? && super
end
......
......@@ -2,6 +2,9 @@
module EE
module LabelPresenter
extend ::Gitlab::Utils::DelegatorOverride
delegator_override :scoped_label?
def scoped_label?
label.scoped_label? && context_subject && context_subject.feature_available?(:scoped_labels)
end
......
......@@ -4,6 +4,9 @@ module EE
module MergeRequestPresenter
include ::VisibleApprovable
extend ::Gitlab::Utils::Override
extend ::Gitlab::Utils::DelegatorOverride
delegator_override_with ::VisibleApprovable # TODO: Remove `::VisibleApprovable` inclusion as it's duplicate
APPROVALS_WIDGET_FULL_TYPE = 'full'
......@@ -33,6 +36,7 @@ module EE
help_page_path('ci/pipelines/merge_trains.md', anchor: 'immediately-merge-a-merge-request-with-a-merge-train')
end
delegator_override :target_project
def target_project
merge_request.target_project.present(current_user: current_user)
end
......@@ -41,6 +45,7 @@ module EE
@code_owner_rules ||= merge_request.approval_rules.code_owner.with_users.to_a
end
delegator_override :approver_groups
def approver_groups
::ApproverGroup.filtered_approver_groups(merge_request.approver_groups, current_user)
end
......@@ -54,6 +59,7 @@ module EE
expose_mr_approval_path? ? APPROVALS_WIDGET_FULL_TYPE : super
end
delegator_override :missing_security_scan_types
def missing_security_scan_types
merge_request.missing_security_scan_types if expose_missing_security_scan_types?
end
......
......@@ -3,6 +3,7 @@
module EE
module ProjectPresenter
extend ::Gitlab::Utils::Override
extend ::Gitlab::Utils::DelegatorOverride
override :statistics_buttons
def statistics_buttons(show_auto_devops_callout:)
......@@ -13,6 +14,7 @@ module EE
[sast_anchor_data.presence].compact
end
delegator_override :approver_groups
def approver_groups
::ApproverGroup.filtered_approver_groups(project.approver_groups, current_user)
end
......
# frozen_string_literal: true
class EpicIssuePresenter < Gitlab::View::Presenter::Delegated
presents :issue
delegator_override :issue
presents ::EpicIssue, as: :issue
def group_epic_issue_path(current_user)
return unless can_admin_issue_link?(current_user)
......
......@@ -4,7 +4,7 @@ class EpicPresenter < Gitlab::View::Presenter::Delegated
include GitlabRoutingHelper
include EntityDateHelper
presents :epic
presents ::Epic, as: :epic
def show_data(base_data: {}, author_icon: nil)
{
......@@ -35,6 +35,7 @@ class EpicPresenter < Gitlab::View::Presenter::Delegated
end
end
delegator_override :subscribed?
def subscribed?
epic.subscribed?(current_user)
end
......
# frozen_string_literal: true
class IterationPresenter < Gitlab::View::Presenter::Delegated
presents :iteration
presents ::Iteration, as: :iteration
def iteration_path
url_builder.build(iteration, only_path: true)
......
......@@ -12,7 +12,7 @@ class MergeRequestApproverPresenter < Gitlab::View::Presenter::Simple
include ActionView::RecordIdentifier
include Gitlab::Utils::StrongMemoize
presents :merge_request
presents ::MergeRequest, as: :merge_request
attr_reader :skip_user
......
......@@ -7,7 +7,9 @@ module Projects
include AutoDevopsHelper
include LatestPipelineInformation
presents :project
delegator_override_with Gitlab::Utils::StrongMemoize # TODO: Remove `Gitlab::Utils::StrongMemoize` inclusion as it's duplicate
presents ::Project, as: :project
def to_h
{
......
......@@ -4,8 +4,9 @@ module Security
class ScanPresenter < Gitlab::View::Presenter::Delegated
ERROR_MESSAGE_FORMAT = '[%<type>s] %<message>s'
presents :scan
presents ::Security::Scan, as: :scan
delegator_override :errors
def errors
info['errors'].to_a.map { |error| format(ERROR_MESSAGE_FORMAT, error.symbolize_keys) }
end
......
......@@ -3,7 +3,7 @@
class SubscriptionPresenter < Gitlab::View::Presenter::Delegated
GRACE_PERIOD_EXTENSION_DAYS = 14.days
presents :subscription
presents ::Subscription, as: :subscription
def block_changes?
will_block_changes? && (block_changes_at < Date.today)
......
......@@ -2,11 +2,14 @@
module Subscriptions
class NewPlanPresenter < Gitlab::View::Presenter::Delegated
presents ::Plan
NEW_PLAN_TITLES = {
silver: 'Premium (Formerly Silver)',
gold: 'Ultimate (Formerly Gold)'
}.freeze
delegator_override :title
def title
NEW_PLAN_TITLES.fetch(plan_key, super)
end
......
......@@ -2,7 +2,7 @@
module Vulnerabilities
class FindingPresenter < Gitlab::View::Presenter::Delegated
presents :finding
presents ::Vulnerabilities::Finding, as: :finding
def title
name
......
# frozen_string_literal: true
class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated
presents :vulnerability
presents ::Vulnerability, as: :vulnerability
delegator_override :links
def links
vulnerability.links.map(&:with_indifferent_access)
end
delegator_override :remediations
def remediations
vulnerability.remediations.to_a.compact.map(&:with_indifferent_access)
end
......@@ -31,6 +33,7 @@ class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated
path_with_line_number(project_raw_path(vulnerability.project, File.join(pipeline_branch, file)))
end
delegator_override :blob_path
def blob_path
return unless file
......@@ -52,6 +55,7 @@ class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated
)
end
delegator_override :description
def description
vulnerability.description || finding.description
end
......
# frozen_string_literal: true
module Gitlab
module Utils
# This module is to validate that delegator classes (`SimpleDelegator`) do not
# accidentally override important logic on the fabricated object.
module DelegatorOverride
def delegator_target(target_class)
return unless ENV['STATIC_VERIFICATION']
unless self < ::SimpleDelegator
raise ArgumentError, "'#{self}' is not a subclass of 'SimpleDelegator' class."
end
DelegatorOverride.validator(self).add_target(target_class)
end
def delegator_override(*names)
return unless ENV['STATIC_VERIFICATION']
raise TypeError unless names.all? { |n| n.is_a?(Symbol) }
DelegatorOverride.validator(self).add_allowlist(names)
end
def delegator_override_with(mod)
return unless ENV['STATIC_VERIFICATION']
raise TypeError unless mod.is_a?(Module)
DelegatorOverride.validator(self).add_allowlist(mod.instance_methods)
end
def self.validator(delegator_class)
validators[delegator_class] ||= Validator.new(delegator_class)
end
def self.validators
@validators ||= {}
end
def self.verify!
validators.each_value do |validator|
validator.expand_on_ancestors(validators)
validator.validate_overrides!
end
end
end
end
end
# frozen_string_literal: true
module Gitlab
module Utils
module DelegatorOverride
class Error
attr_accessor :method_name, :target_class, :target_location, :delegator_class, :delegator_location
def initialize(method_name, target_class, target_location, delegator_class, delegator_location)
@method_name = method_name
@target_class = target_class
@target_location = target_location
@delegator_class = delegator_class
@delegator_location = delegator_location
end
def to_s
"#{delegator_class}##{method_name} is overriding #{target_class}##{method_name}. delegator_location: #{delegator_location} target_location: #{target_location}"
end
end
end
end
end
# frozen_string_literal: true
module Gitlab
module Utils
module DelegatorOverride
class Validator
UnexpectedDelegatorOverrideError = Class.new(StandardError)
attr_reader :delegator_class, :target_classes
OVERRIDE_ERROR_MESSAGE = <<~EOS
We've detected that the delegator is overriding a specific method(s) on the target class.
Please make sure if it's intentional and handle this error accordingly.
See https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides for more information.
EOS
def initialize(delegator_class)
@delegator_class = delegator_class
@target_classes = []
end
def add_allowlist(names)
allowed_method_names.concat(names)
end
def allowed_method_names
@allowed_method_names ||= []
end
def add_target(target_class)
@target_classes << target_class if target_class
end
# This will make sure allowlist we put into ancestors are all included
def expand_on_ancestors(validators)
delegator_class.ancestors.each do |ancestor|
next if delegator_class == ancestor # ancestor includes itself
validator_ancestor = validators[ancestor]
next unless validator_ancestor
add_allowlist(validator_ancestor.allowed_method_names)
end
end
def validate_overrides!
return if target_classes.empty?
errors = []
# Workaround to fully load the instance methods in the target class.
# See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69823#note_678887402
target_classes.map(&:new) rescue nil
(delegator_class.instance_methods - allowlist).each do |method_name|
target_classes.each do |target_class|
next unless target_class.instance_methods.include?(method_name)
errors << generate_error(method_name, target_class, delegator_class)
end
end
return if errors.empty?
details = errors.map { |error| "- #{error}" }.join("\n")
raise UnexpectedDelegatorOverrideError,
<<~TEXT
#{OVERRIDE_ERROR_MESSAGE}
Here are the conflict details.
#{details}
TEXT
end
private
def generate_error(method_name, target_class, delegator_class)
target_location = extract_location(target_class, method_name)
delegator_location = extract_location(delegator_class, method_name)
Error.new(method_name, target_class, target_location, delegator_class, delegator_location)
end
def extract_location(klass, method_name)
klass.instance_method(method_name).source_location&.join(':') || 'unknown'
end
def allowlist
[].tap do |allowed|
allowed.concat(allowed_method_names)
allowed.concat(Object.instance_methods)
allowed.concat(::Delegator.instance_methods)
end
end
end
end
end
end
......@@ -47,8 +47,18 @@ module Gitlab
true
end
def presents(name)
define_method(name) { subject }
def presents(*target_classes, as: nil)
if target_classes.any? { |k| k.is_a?(Symbol) }
raise ArgumentError, "Unsupported target class type: #{target_classes}."
end
if self < ::Gitlab::View::Presenter::Delegated
target_classes.each { |k| delegator_target(k) }
elsif self < ::Gitlab::View::Presenter::Simple
# no-op
end
define_method(as) { subject } if as
end
end
end
......
......@@ -4,7 +4,18 @@ module Gitlab
module View
module Presenter
class Delegated < SimpleDelegator
extend ::Gitlab::Utils::DelegatorOverride
# TODO: Stop including auxiliary methods/modules in `Presenter::Base` as
# it overrides many methods in the Active Record models.
# See https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/presenters/README.md#validate-accidental-overrides
# for more information.
include Gitlab::View::Presenter::Base
delegator_override_with Gitlab::Routing.url_helpers
delegator_override :can?
delegator_override :declarative_policy_delegate
delegator_override :present
delegator_override :web_url
def initialize(subject, **attributes)
@subject = subject
......
......@@ -12,6 +12,7 @@ unless Rails.env.production?
dev:load
] do
Gitlab::Utils::Override.verify!
Gitlab::Utils::DelegatorOverride.verify!
end
desc "GitLab | Lint | Lint JavaScript files using ESLint"
......
# frozen_string_literal: true
require 'fast_spec_helper'
RSpec.describe Gitlab::Utils::DelegatorOverride::Error do
let(:error) { described_class.new('foo', 'Target', '/path/to/target', 'Delegator', '/path/to/delegator') }
describe '#to_s' do
subject { error.to_s }
it { is_expected.to eq("Delegator#foo is overriding Target#foo. delegator_location: /path/to/delegator target_location: /path/to/target") }
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
RSpec.describe Gitlab::Utils::DelegatorOverride::Validator do
let(:delegator_class) do
Class.new(::SimpleDelegator) do
extend(::Gitlab::Utils::DelegatorOverride)
def foo
end
end.prepend(ee_delegator_extension)
end
let(:ee_delegator_extension) do
Module.new do
extend(::Gitlab::Utils::DelegatorOverride)
def bar
end
end
end
let(:target_class) do
Class.new do
def foo
end
def bar
end
end
end
let(:validator) { described_class.new(delegator_class) }
describe '#add_allowlist' do
it 'adds a method name to the allowlist' do
validator.add_allowlist([:foo])
expect(validator.allowed_method_names).to contain_exactly(:foo)
end
end
describe '#add_target' do
it 'adds the target class' do
validator.add_target(target_class)
expect(validator.target_classes).to contain_exactly(target_class)
end
end
describe '#expand_on_ancestors' do
it 'adds the allowlist in the ancestors' do
ancestor_validator = described_class.new(ee_delegator_extension)
ancestor_validator.add_allowlist([:bar])
validator.expand_on_ancestors( { ee_delegator_extension => ancestor_validator })
expect(validator.allowed_method_names).to contain_exactly(:bar)
end
end
describe '#validate_overrides!' do
before do
validator.add_target(target_class)
end
it 'does not raise an error when the overrides are allowed' do
validator.add_allowlist([:foo])
ancestor_validator = described_class.new(ee_delegator_extension)
ancestor_validator.add_allowlist([:bar])
validator.expand_on_ancestors( { ee_delegator_extension => ancestor_validator })
expect { validator.validate_overrides! }.not_to raise_error
end
it 'raises an error when there is an override' do
expect { validator.validate_overrides! }
.to raise_error(described_class::UnexpectedDelegatorOverrideError)
end
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
RSpec.describe Gitlab::Utils::DelegatorOverride do
let(:delegator_class) do
Class.new(::SimpleDelegator) do
extend(::Gitlab::Utils::DelegatorOverride)
def foo
end
end
end
let(:target_class) do
Class.new do
def foo
end
def bar
end
end
end
let(:dummy_module) do
Module.new do
def foobar
end
end
end
before do
stub_env('STATIC_VERIFICATION', 'true')
end
describe '.delegator_target' do
subject { delegator_class.delegator_target(target_class) }
it 'sets the delegator target to the validator' do
expect(described_class.validator(delegator_class))
.to receive(:add_target).with(target_class)
subject
end
context 'when the class does not inherit SimpleDelegator' do
let(:delegator_class) do
Class.new do
extend(::Gitlab::Utils::DelegatorOverride)
end
end
it 'raises an error' do
expect { subject }.to raise_error(ArgumentError, /not a subclass of 'SimpleDelegator' class/)
end
end
end
describe '.delegator_override' do
subject { delegator_class.delegator_override(:foo) }
it 'adds the method name to the allowlist' do
expect(described_class.validator(delegator_class))
.to receive(:add_allowlist).with([:foo])
subject
end
end
describe '.delegator_override_with' do
subject { delegator_class.delegator_override_with(dummy_module) }
it 'adds the method names of the module to the allowlist' do
expect(described_class.validator(delegator_class))
.to receive(:add_allowlist).with([:foobar])
subject
end
end
describe '.verify!' do
subject { described_class.verify! }
it 'does not raise an error when an override is in allowlist' do
delegator_class.delegator_target(target_class)
delegator_class.delegator_override(:foo)
expect { subject }.not_to raise_error
end
it 'raises an error when there is an override' do
delegator_class.delegator_target(target_class)
expect { subject }.to raise_error(Gitlab::Utils::DelegatorOverride::Validator::UnexpectedDelegatorOverrideError)
end
end
end
......@@ -18,11 +18,43 @@ RSpec.describe Gitlab::View::Presenter::Base do
describe '.presents' do
it 'exposes #subject with the given keyword' do
presenter_class.presents(:foo)
presenter_class.presents(Object, as: :foo)
presenter = presenter_class.new(project)
expect(presenter.foo).to eq(project)
end
it 'raises an error when symbol is passed' do
expect { presenter_class.presents(:foo) }.to raise_error(ArgumentError)
end
context 'when the presenter class inherits Presenter::Delegated' do
let(:presenter_class) do
Class.new(::Gitlab::View::Presenter::Delegated) do
include(::Gitlab::View::Presenter::Base)
end
end
it 'sets the delegator target' do
expect(presenter_class).to receive(:delegator_target).with(Object)
presenter_class.presents(Object, as: :foo)
end
end
context 'when the presenter class inherits Presenter::Simple' do
let(:presenter_class) do
Class.new(::Gitlab::View::Presenter::Simple) do
include(::Gitlab::View::Presenter::Base)
end
end
it 'does not set the delegator target' do
expect(presenter_class).not_to receive(:delegator_target).with(Object)
presenter_class.presents(Object, as: :foo)
end
end
end
describe '#can?' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment