Commit 23f63f4e authored by Rémy Coutable's avatar Rémy Coutable

Introduce a new Gitlab/FeatureAvailableUsage cop

This new cop will ensure consistency in usage of the
`#feature_available?` method.
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 2115836f
......@@ -106,6 +106,7 @@ linters:
- Cop/LineBreakAfterGuardClauses
- Cop/LineBreakAroundConditionalBlock
- Cop/ProjectPathHelper
- Gitlab/FeatureAvailableUsage
- GitlabSecurity/PublicSend
- Layout/EmptyLineAfterGuardClause
- Layout/LeadingCommentSpace
......
......@@ -3580,3 +3580,239 @@ Gitlab/DelegatePredicateMethods:
- 'ee/app/models/ee/namespace.rb'
- 'ee/app/models/license.rb'
- 'lib/gitlab/ci/trace/stream.rb'
# Offense count: 298
Gitlab/FeatureAvailableUsage:
Exclude:
- 'app/controllers/projects/application_controller.rb'
- 'app/graphql/types/project_type.rb'
- 'app/helpers/events_helper.rb'
- 'app/helpers/labels_helper.rb'
- 'app/policies/project_policy.rb'
- 'app/views/groups/issues.html.haml'
- 'app/views/groups/merge_requests.html.haml'
- 'app/views/shared/boards/_switcher.html.haml'
- 'ee/app/controllers/concerns/description_diff_actions.rb'
- 'ee/app/controllers/concerns/ee/boards_actions.rb'
- 'ee/app/controllers/concerns/security_dashboards_permissions.rb'
- 'ee/app/controllers/ee/boards/lists_controller.rb'
- 'ee/app/controllers/ee/groups/application_controller.rb'
- 'ee/app/controllers/ee/groups/group_members_controller.rb'
- 'ee/app/controllers/ee/projects/autocomplete_sources_controller.rb'
- 'ee/app/controllers/ee/projects/issues_controller.rb'
- 'ee/app/controllers/ee/projects/security/configuration_controller.rb'
- 'ee/app/controllers/ee/projects/settings/ci_cd_controller.rb'
- 'ee/app/controllers/ee/projects/settings/operations_controller.rb'
- 'ee/app/controllers/ee/projects/settings/repository_controller.rb'
- 'ee/app/controllers/groups/analytics/application_controller.rb'
- 'ee/app/controllers/groups/audit_events_controller.rb'
- 'ee/app/controllers/groups/bulk_update_controller.rb'
- 'ee/app/controllers/groups/contribution_analytics_controller.rb'
- 'ee/app/controllers/groups/epics_controller.rb'
- 'ee/app/controllers/groups/hooks_controller.rb'
- 'ee/app/controllers/groups/issues_analytics_controller.rb'
- 'ee/app/controllers/groups/iterations_controller.rb'
- 'ee/app/controllers/projects/analytics/issues_analytics_controller.rb'
- 'ee/app/controllers/projects/audit_events_controller.rb'
- 'ee/app/controllers/projects/cluster_agents_controller.rb'
- 'ee/app/controllers/projects/iterations/inherited_controller.rb'
- 'ee/app/controllers/projects/iterations_controller.rb'
- 'ee/app/controllers/projects/path_locks_controller.rb'
- 'ee/app/controllers/projects/subscriptions_controller.rb'
- 'ee/app/finders/autocomplete/vulnerabilities_autocomplete_finder.rb'
- 'ee/app/finders/clusters/agents_finder.rb'
- 'ee/app/finders/ee/alert_management/alerts_finder.rb'
- 'ee/app/finders/ee/alert_management/http_integrations_finder.rb'
- 'ee/app/finders/ee/group_projects_finder.rb'
- 'ee/app/graphql/ee/types/group_type.rb'
- 'ee/app/graphql/mutations/dast/profiles/create.rb'
- 'ee/app/graphql/mutations/dast/profiles/run.rb'
- 'ee/app/graphql/mutations/dast/profiles/update.rb'
- 'ee/app/graphql/mutations/instance_security_dashboard/remove_project.rb'
- 'ee/app/graphql/resolvers/boards/epic_boards_resolver.rb'
- 'ee/app/graphql/resolvers/clusters/agent_tokens_resolver.rb'
- 'ee/app/graphql/resolvers/epics_resolver.rb'
- 'ee/app/helpers/ee/analytics/navbar_helper.rb'
- 'ee/app/helpers/ee/application_helper.rb'
- 'ee/app/helpers/ee/boards_helper.rb'
- 'ee/app/helpers/ee/clusters_helper.rb'
- 'ee/app/helpers/ee/dashboard_helper.rb'
- 'ee/app/helpers/ee/form_helper.rb'
- 'ee/app/helpers/ee/graph_helper.rb'
- 'ee/app/helpers/ee/groups_helper.rb'
- 'ee/app/helpers/ee/issues_helper.rb'
- 'ee/app/helpers/ee/lock_helper.rb'
- 'ee/app/helpers/ee/operations_helper.rb'
- 'ee/app/helpers/ee/projects/incidents_helper.rb'
- 'ee/app/helpers/ee/projects_helper.rb'
- 'ee/app/helpers/ee/releases_helper.rb'
- 'ee/app/helpers/ee/search_helper.rb'
- 'ee/app/helpers/ee/tree_helper.rb'
- 'ee/app/helpers/groups/security_features_helper.rb'
- 'ee/app/models/approval_state.rb'
- 'ee/app/models/concerns/approvable.rb'
- 'ee/app/models/concerns/ee/project_security_scanners_information.rb'
- 'ee/app/models/concerns/ee/protected_ref_access.rb'
- 'ee/app/models/concerns/has_timelogs_report.rb'
- 'ee/app/models/concerns/insights_feature.rb'
- 'ee/app/models/ee/board.rb'
- 'ee/app/models/ee/ci/build.rb'
- 'ee/app/models/ee/ci/build_dependencies.rb'
- 'ee/app/models/ee/ci/pipeline.rb'
- 'ee/app/models/ee/group.rb'
- 'ee/app/models/ee/group_member.rb'
- 'ee/app/models/ee/issue.rb'
- 'ee/app/models/ee/list.rb'
- 'ee/app/models/ee/merge_request.rb'
- 'ee/app/models/ee/milestone_release.rb'
- 'ee/app/models/ee/namespace.rb'
- 'ee/app/models/ee/namespace_setting.rb'
- 'ee/app/models/ee/project.rb'
- 'ee/app/models/ee/project_ci_cd_setting.rb'
- 'ee/app/models/namespace_statistics.rb'
- 'ee/app/models/project_security_setting.rb'
- 'ee/app/models/saml_provider.rb'
- 'ee/app/policies/compliance_management/framework_policy.rb'
- 'ee/app/policies/compliance_management/framework_policy.rb'
- 'ee/app/policies/ee/group_policy.rb'
- 'ee/app/policies/ee/namespace_policy.rb'
- 'ee/app/policies/ee/project_policy.rb'
- 'ee/app/policies/ee/protected_branch_policy.rb'
- 'ee/app/presenters/ee/label_presenter.rb'
- 'ee/app/presenters/epic_presenter.rb'
- 'ee/app/presenters/merge_request_approver_presenter.rb'
- 'ee/app/serializers/dashboard_operations_project_entity.rb'
- 'ee/app/serializers/ee/environment_entity.rb'
- 'ee/app/serializers/ee/evidences/release_entity.rb'
- 'ee/app/serializers/ee/note_entity.rb'
- 'ee/app/services/boards/epic_boards/update_service.rb'
- 'ee/app/services/ci/audit_variable_change_service.rb'
- 'ee/app/services/clusters/agent_tokens/create_service.rb'
- 'ee/app/services/clusters/agents/create_service.rb'
- 'ee/app/services/dashboard/projects/create_service.rb'
- 'ee/app/services/dashboard/projects/list_service.rb'
- 'ee/app/services/dast/profiles/create_service.rb'
- 'ee/app/services/dast/profiles/update_service.rb'
- 'ee/app/services/dast_on_demand_scans/create_service.rb'
- 'ee/app/services/dast_site_tokens/create_service.rb'
- 'ee/app/services/dast_site_validations/create_service.rb'
- 'ee/app/services/dast_site_validations/revoke_service.rb'
- 'ee/app/services/dast_site_validations/validate_service.rb'
- 'ee/app/services/ee/alert_management/http_integrations/create_service.rb'
- 'ee/app/services/ee/audit_event_service.rb'
- 'ee/app/services/ee/boards/issues/list_service.rb'
- 'ee/app/services/ee/boards/lists/create_service.rb'
- 'ee/app/services/ee/boards/update_service.rb'
- 'ee/app/services/ee/groups/create_service.rb'
- 'ee/app/services/ee/ide/schemas_config_service.rb'
- 'ee/app/services/ee/issuable_base_service.rb'
- 'ee/app/services/ee/issue_links/create_service.rb'
- 'ee/app/services/ee/issues/build_service.rb'
- 'ee/app/services/ee/lfs/lock_file_service.rb'
- 'ee/app/services/ee/lfs/unlock_file_service.rb'
- 'ee/app/services/ee/merge_requests/approval_service.rb'
- 'ee/app/services/ee/merge_requests/build_service.rb'
- 'ee/app/services/ee/merge_requests/merge_base_service.rb'
- 'ee/app/services/ee/merge_requests/refresh_service.rb'
- 'ee/app/services/ee/merge_requests/update_service.rb'
- 'ee/app/services/ee/projects/create_service.rb'
- 'ee/app/services/ee/protected_branches/create_service.rb'
- 'ee/app/services/ee/releases/create_evidence_service.rb'
- 'ee/app/services/ee/search/group_service.rb'
- 'ee/app/services/iterations/cadences/create_service.rb'
- 'ee/app/services/iterations/cadences/update_service.rb'
- 'ee/app/services/iterations/create_service.rb'
- 'ee/app/services/iterations/update_service.rb'
- 'ee/app/services/merge_requests/sync_report_approver_approval_rules.rb'
- 'ee/app/services/merge_requests/update_blocks_service.rb'
- 'ee/app/services/projects/mark_for_deletion_service.rb'
- 'ee/app/services/quality_management/test_cases/create_service.rb'
- 'ee/app/services/requirements_management/process_test_reports_service.rb'
- 'ee/app/services/security/store_scans_service.rb'
- 'ee/app/views/groups/_templates_setting.html.haml'
- 'ee/app/views/groups/contribution_analytics/show.html.haml'
- 'ee/app/views/groups/ee/_settings_nav.html.haml'
- 'ee/app/views/groups/epics/index.html.haml'
- 'ee/app/views/groups/epics/show.html.haml'
- 'ee/app/views/groups/epics/show.html.haml'
- 'ee/app/views/groups/hooks/index.html.haml'
- 'ee/app/views/groups/roadmap/show.html.haml'
- 'ee/app/views/groups/settings/_allowed_email_domain.html.haml'
- 'ee/app/views/groups/settings/_ip_restriction.html.haml'
- 'ee/app/views/layouts/nav/_test_cases_link.html.haml'
- 'ee/app/views/layouts/nav/sidebar/_project_iterations_link.html.haml'
- 'ee/app/views/projects/_merge_request_approvals_settings.html.haml'
- 'ee/app/views/projects/_merge_request_settings.html.haml'
- 'ee/app/views/projects/_merge_request_settings_description_text.html.haml'
- 'ee/app/views/projects/audit_events/index.html.haml'
- 'ee/app/views/projects/blob/_header_file_locks.html.haml'
- 'ee/app/views/projects/issues/_related_issues.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/merge_requests/show.html.haml'
- 'ee/app/views/projects/pipelines/_tabs_content.html.haml'
- 'ee/app/views/projects/protected_branches/ee/_code_owner_approval_form.html.haml'
- 'ee/app/views/projects/protected_branches/ee/_code_owner_approval_table.html.haml'
- 'ee/app/views/projects/protected_branches/ee/_code_owner_approval_table_head.html.haml'
- 'ee/app/views/projects/push_rules/_index.html.haml'
- 'ee/app/views/projects/settings/_default_issue_template.html.haml'
- 'ee/app/views/projects/settings/_marked_for_removal.html.haml'
- 'ee/app/views/projects/settings/_restore.html.haml'
- 'ee/app/views/projects/settings/ci_cd/_auto_rollback.html.haml'
- 'ee/app/views/projects/settings/ci_cd/_pipeline_subscriptions.html.haml'
- 'ee/app/views/projects/settings/operations/_status_page.html.haml'
- 'ee/app/views/projects/settings/repository/_protected_branches.html.haml'
- 'ee/app/views/projects/sidebar/_repository_locked_files.html.haml'
- 'ee/app/views/shared/issuable/_board_create_list_dropdown.html.haml'
- 'ee/app/views/shared/issuable/_board_create_list_dropdown.html.haml'
- 'ee/app/views/shared/issuable/_group_bulk_update_sidebar.html.haml'
- 'ee/app/views/shared/issuable/_iteration_select.html.haml'
- 'ee/app/views/shared/issuable/form/_default_templates.html.haml'
- 'ee/app/views/shared/labels/_create_label_help_text.html.haml'
- 'ee/app/views/shared/promotions/_promote_mr_features.html.haml'
- 'ee/app/views/shared/promotions/_promote_mr_features.html.haml'
- 'ee/app/views/shared/promotions/_promote_repository_features.html.haml'
- 'ee/app/views/shared/promotions/_promote_repository_features.html.haml'
- 'ee/app/views/shared/promotions/_promote_repository_features.html.haml'
- 'ee/app/views/shared/promotions/_promote_repository_features.html.haml'
- 'ee/app/views/shared/promotions/_promote_repository_features.html.haml'
- 'ee/app/views/shared/promotions/_promote_repository_features.html.haml'
- 'ee/app/workers/analytics/code_review_metrics_worker.rb'
- 'ee/app/workers/group_saml_group_sync_worker.rb'
- 'ee/lib/api/external_approval_rules.rb'
- 'ee/lib/api/helpers/epics_helpers.rb'
- 'ee/lib/api/ldap_group_links.rb'
- 'ee/lib/ee/api/entities/approval_state.rb'
- 'ee/lib/ee/api/entities/board.rb'
- 'ee/lib/ee/api/entities/group.rb'
- 'ee/lib/ee/api/entities/issue.rb'
- 'ee/lib/ee/api/entities/project.rb'
- 'ee/lib/ee/api/groups.rb'
- 'ee/lib/ee/api/helpers.rb'
- 'ee/lib/ee/api/internal/kubernetes.rb'
- 'ee/lib/ee/api/job_artifacts.rb'
- 'ee/lib/ee/api/projects.rb'
- 'ee/lib/ee/gitlab/alert_management/payload/generic.rb'
- 'ee/lib/ee/gitlab/checks/diff_check.rb'
- 'ee/lib/ee/gitlab/gon_helper.rb'
- 'ee/lib/ee/gitlab/tree_summary.rb'
- 'ee/lib/gitlab/alert_management.rb'
- 'ee/lib/gitlab/auth/group_saml/group_lookup.rb'
- 'ee/lib/gitlab/ci/pipeline/chain/config/content/compliance.rb'
- 'ee/lib/gitlab/code_owners.rb'
- 'ee/lib/gitlab/import_export/group/group_and_descendants_repo_restorer.rb'
- 'ee/lib/gitlab/incident_management.rb'
- 'ee/lib/gitlab/path_locks_finder.rb'
- 'ee/lib/incident_management/incident_sla.rb'
- 'ee/spec/models/ee/namespace_spec.rb'
- 'ee/spec/models/instance_security_dashboard_spec.rb'
- 'ee/spec/models/license_spec.rb'
- 'ee/spec/models/project_spec.rb'
- 'lib/api/helpers/related_resources_helpers.rb'
- 'spec/models/concerns/featurable_spec.rb'
......@@ -3,6 +3,7 @@
class ProjectFeature < ApplicationRecord
include Featurable
# When updating this array, make sure to update rubocop/cop/gitlab/feature_available_usage.rb as well.
FEATURES = %i[
issues
forking
......
......@@ -4,6 +4,7 @@ module EE
module ProjectFeature
extend ActiveSupport::Concern
# When updating this array, make sure to update rubocop/cop/gitlab/feature_available_usage.rb as well.
EE_FEATURES = %i(requirements).freeze
NOTES_PERMISSION_TRACKED_FIELDS = %w(
issues_access_level
......
# frozen_string_literal: true
module RuboCop
module Cop
module Gitlab
# Cop that checks for correct calling of #feature_available?
class FeatureAvailableUsage < RuboCop::Cop::Cop
OBSERVED_METHOD = :feature_available?
LICENSED_FEATURE_LITERAL_ARG_MSG = '`feature_available?` should not be called for features that can be licensed (`%s` given), use `licensed_feature_available?(feature)` instead.'
LICENSED_FEATURE_DYNAMIC_ARG_MSG = "`feature_available?` should not be called for features that can be licensed (`%s` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate."
NOT_ENOUGH_ARGS_MSG = '`feature_available?` should be called with two arguments: `feature` and `user`.'
FEATURES = %i[
issues
forking
merge_requests
wiki
snippets
builds
repository
pages
metrics_dashboard
analytics
operations
security_and_compliance
container_registry
].freeze
EE_FEATURES = %i[requirements].freeze
ALL_FEATURES = (FEATURES + EE_FEATURES).freeze
SPECIAL_CLASS = %w[License].freeze
def on_send(node)
return unless method_name(node) == OBSERVED_METHOD
return if caller_is_special_case?(node)
return if feature_name(node).nil?
return if ALL_FEATURES.include?(feature_name(node)) && args_count(node) == 2
if !ALL_FEATURES.include?(feature_name(node))
add_offense(node, location: :expression, message: licensed_feature_message(node))
elsif args_count(node) < 2
add_offense(node, location: :expression, message: NOT_ENOUGH_ARGS_MSG)
end
end
def licensed_feature_message(node)
message_template = dynamic_feature?(node) ? LICENSED_FEATURE_DYNAMIC_ARG_MSG : LICENSED_FEATURE_LITERAL_ARG_MSG
message_template % feature_arg_name(node)
end
private
def method_name(node)
node.children[1]
end
def class_caller(node)
node.children[0]&.const_name.to_s
end
def caller_is_special_case?(node)
SPECIAL_CLASS.include?(class_caller(node))
end
def feature_arg(node)
node.children[2]
end
def dynamic_feature?(node)
feature = feature_arg(node)
return false unless feature
!feature.literal?
end
def feature_name(node)
feature = feature_arg(node)
return unless feature
return feature.children.compact.join('.') if dynamic_feature?(node)
return feature.value if feature.respond_to?(:value)
feature.type
end
def feature_arg_name(node)
feature = feature_arg(node)
return unless feature
return feature.children.compact.join('.') if dynamic_feature?(node)
return feature.children[0].inspect if feature.literal?
feature.type
end
def args_count(node)
node.children[2..].size
end
end
end
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
require 'rubocop'
require 'rubocop/rspec/support'
require_relative '../../../../rubocop/cop/gitlab/feature_available_usage'
RSpec.describe RuboCop::Cop::Gitlab::FeatureAvailableUsage do
subject(:cop) { described_class.new }
context 'no arguments given' do
it 'does not flag the use of Gitlab::Sourcegraph.feature_available? with no arguments' do
expect_no_offenses('Gitlab::Sourcegraph.feature_available?')
expect_no_offenses('subject { described_class.feature_available? }')
end
end
context 'one argument given' do
it 'does not flag the use of License.feature_available?' do
expect_no_offenses('License.feature_available?(:push_rules)')
end
it 'flags the use with a dynamic feature as nil' do
expect_offense(<<~SOURCE)
feature_available?(nil)
^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`nil` given), use `licensed_feature_available?(feature)` instead.
SOURCE
expect_offense(<<~SOURCE)
project.feature_available?(nil)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`nil` given), use `licensed_feature_available?(feature)` instead.
SOURCE
end
it 'flags the use with an OSS project feature' do
expect_offense(<<~SOURCE)
project.feature_available?(:issues)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should be called with two arguments: `feature` and `user`.
SOURCE
expect_offense(<<~SOURCE)
feature_available?(:issues)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should be called with two arguments: `feature` and `user`.
SOURCE
end
it 'flags the use with a feature that is not a project feature' do
expect_offense(<<~SOURCE)
feature_available?(:foo)
^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
SOURCE
expect_offense(<<~SOURCE)
project.feature_available?(:foo)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
SOURCE
expect_offense(<<~SOURCE)
feature_available?(foo)
^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
SOURCE
expect_offense(<<~SOURCE)
foo = :feature
feature_available?(foo)
^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
SOURCE
end
end
context 'two arguments given' do
it 'does not flag the use with an OSS project feature' do
expect_no_offenses('feature_available?(:issues, user)')
expect_no_offenses('project.feature_available?(:issues, user)')
end
it 'does not flag the use with an EE project feature' do
expect_no_offenses('feature_available?(:requirements, user)')
expect_no_offenses('project.feature_available?(:requirements, user)')
end
it 'flags the use with a dynamic feature as a method call with two args' do
expect_offense(<<~SOURCE)
feature_available?(:foo, current_user)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
SOURCE
expect_offense(<<~SOURCE)
project.feature_available?(:foo, current_user)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
SOURCE
expect_offense(<<~SOURCE)
feature_available?(foo, current_user)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
SOURCE
expect_offense(<<~SOURCE)
project.feature_available?(foo, current_user)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
SOURCE
end
end
end
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