From a03d4fa2f72b721c5f0f3fc266040ad8d7e38720 Mon Sep 17 00:00:00 2001 From: Alishan Ladhani Date: Fri, 22 Jul 2022 13:44:21 -0400 Subject: [PATCH] Split environments visibility setting from operations --- app/helpers/projects_helper.rb | 3 +- .../project_features_compatibility.rb | 4 + app/models/project.rb | 2 +- app/models/project_feature.rb | 1 + app/policies/project_policy.rb | 12 +- .../import_export/project/import_export.yml | 2 + rubocop/cop/gitlab/feature_available_usage.rb | 1 + spec/factories/projects.rb | 1 + spec/helpers/projects_helper_spec.rb | 3 +- .../import_export/safe_model_attributes.yml | 1 + .../project_features_compatibility_spec.rb | 2 +- spec/models/project_spec.rb | 1 + spec/policies/project_policy_spec.rb | 125 +++++++++++++++--- 13 files changed, 133 insertions(+), 25 deletions(-) diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index e586dc1f3591fb..c7b5bd300ee537 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -639,7 +639,8 @@ def project_permissions_settings(project) warnAboutPotentiallyUnwantedCharacters: project.warn_about_potentially_unwanted_characters?, enforceAuthChecksOnUploads: project.enforce_auth_checks_on_uploads?, securityAndComplianceAccessLevel: project.security_and_compliance_access_level, - containerRegistryAccessLevel: feature.container_registry_access_level + containerRegistryAccessLevel: feature.container_registry_access_level, + environmentsAccessLevel: feature.environments_access_level } end diff --git a/app/models/concerns/project_features_compatibility.rb b/app/models/concerns/project_features_compatibility.rb index 900e8f7d39b0a5..65b1cf023ec9aa 100644 --- a/app/models/concerns/project_features_compatibility.rb +++ b/app/models/concerns/project_features_compatibility.rb @@ -94,6 +94,10 @@ def container_registry_access_level=(value) write_feature_attribute_string(:container_registry_access_level, value) end + def environments_access_level=(value) + write_feature_attribute_string(:environments_access_level, value) + end + # TODO: Remove this method after we drop support for project create/edit APIs to set the # container_registry_enabled attribute. They can instead set the container_registry_access_level # attribute. diff --git a/app/models/project.rb b/app/models/project.rb index 8e59a3599dd8cf..cfa726ede41cf3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -446,7 +446,7 @@ def self.integration_association_name(name) :repository_access_level, :package_registry_access_level, :pages_access_level, :metrics_dashboard_access_level, :analytics_access_level, :operations_access_level, :security_and_compliance_access_level, - :container_registry_access_level, + :container_registry_access_level, :environments_access_level, to: :project_feature, allow_nil: true delegate :show_default_award_emojis, :show_default_award_emojis=, diff --git a/app/models/project_feature.rb b/app/models/project_feature.rb index 0a30e125c83c9d..30d871a27562ce 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -21,6 +21,7 @@ class ProjectFeature < ApplicationRecord security_and_compliance container_registry package_registry + environments ].freeze EXPORTABLE_FEATURES = (FEATURES - [:security_and_compliance, :pages]).freeze diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 9d121715d2cf7a..a36aa5721f280f 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -209,6 +209,7 @@ class ProjectPolicy < BasePolicy analytics operations security_and_compliance + environments ] features.each do |f| @@ -366,7 +367,11 @@ class ProjectPolicy < BasePolicy prevent(:metrics_dashboard) end - rule { operations_disabled }.policy do + condition(:split_operations_visibility_permissions) do + ::Feature.enabled?(:split_operations_visibility_permissions, @subject) + end + + rule { ~split_operations_visibility_permissions & operations_disabled }.policy do prevent(*create_read_update_admin_destroy(:feature_flag)) prevent(*create_read_update_admin_destroy(:environment)) prevent(*create_read_update_admin_destroy(:sentry_issue)) @@ -379,6 +384,11 @@ class ProjectPolicy < BasePolicy prevent(:read_prometheus) end + rule { split_operations_visibility_permissions & environments_disabled }.policy do + prevent(*create_read_update_admin_destroy(:environment)) + prevent(*create_read_update_admin_destroy(:deployment)) + end + rule { can?(:metrics_dashboard) }.policy do enable :read_prometheus enable :read_deployment diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index 50ff6146174fb3..1fdc374aa39e30 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -284,6 +284,7 @@ included_attributes: - :security_and_compliance_access_level - :container_registry_access_level - :package_registry_access_level + - :environments_access_level prometheus_metrics: - :created_at - :updated_at @@ -686,6 +687,7 @@ included_attributes: - :security_and_compliance_access_level - :container_registry_access_level - :package_registry_access_level + - :environments_access_level - :allow_merge_on_skipped_pipeline - :auto_devops_deploy_strategy - :auto_devops_enabled diff --git a/rubocop/cop/gitlab/feature_available_usage.rb b/rubocop/cop/gitlab/feature_available_usage.rb index b50bdd8ca43de2..fafe6c35b5ff5b 100644 --- a/rubocop/cop/gitlab/feature_available_usage.rb +++ b/rubocop/cop/gitlab/feature_available_usage.rb @@ -23,6 +23,7 @@ class FeatureAvailableUsage < RuboCop::Cop::Cop operations security_and_compliance container_registry + environments ].freeze EE_FEATURES = %i[requirements].freeze ALL_FEATURES = (FEATURES + EE_FEATURES).freeze diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 5c8f5d1a603169..107fa28a639e12 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -37,6 +37,7 @@ operations_access_level { ProjectFeature::ENABLED } container_registry_access_level { ProjectFeature::ENABLED } security_and_compliance_access_level { ProjectFeature::PRIVATE } + environments_access_level { ProjectFeature::ENABLED } # we can't assign the delegated `#ci_cd_settings` attributes directly, as the # `#ci_cd_settings` relation needs to be created first diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index b443ebff55246d..200e93f8dff07c 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -966,7 +966,8 @@ def license_name operationsAccessLevel: project.project_feature.operations_access_level, showDefaultAwardEmojis: project.show_default_award_emojis?, securityAndComplianceAccessLevel: project.security_and_compliance_access_level, - containerRegistryAccessLevel: project.project_feature.container_registry_access_level + containerRegistryAccessLevel: project.project_feature.container_registry_access_level, + environmentsAccessLevel: project.project_feature.environments_access_level ) end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index bd60bb53d49e05..3ff830b825b7f9 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -584,6 +584,7 @@ ProjectFeature: - security_and_compliance_access_level - container_registry_access_level - package_registry_access_level +- environments_access_level - created_at - updated_at ProtectedBranch::MergeAccessLevel: diff --git a/spec/models/concerns/project_features_compatibility_spec.rb b/spec/models/concerns/project_features_compatibility_spec.rb index f2dc8464e86a7f..6730b7a02f358d 100644 --- a/spec/models/concerns/project_features_compatibility_spec.rb +++ b/spec/models/concerns/project_features_compatibility_spec.rb @@ -5,7 +5,7 @@ RSpec.describe ProjectFeaturesCompatibility do let(:project) { create(:project) } let(:features_enabled) { %w(issues wiki builds merge_requests snippets security_and_compliance) } - let(:features) { features_enabled + %w(repository pages operations container_registry package_registry) } + let(:features) { features_enabled + %w(repository pages operations container_registry package_registry environments) } # We had issues_enabled, snippets_enabled, builds_enabled, merge_requests_enabled and issues_enabled fields on projects table # All those fields got moved to a new table called project_feature and are now integers instead of booleans diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b434b2c0332380..fb6fc01ed88136 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -831,6 +831,7 @@ it { is_expected.to delegate_method(:last_pipeline).to(:commit).allow_nil } it { is_expected.to delegate_method(:container_registry_enabled?).to(:project_feature) } it { is_expected.to delegate_method(:container_registry_access_level).to(:project_feature) } + it { is_expected.to delegate_method(:environments_access_level).to(:project_feature) } describe 'read project settings' do %i( diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index c041c72a0be143..01a0a96be30bfc 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -1930,6 +1930,10 @@ def set_access_level(access_level) describe 'operations feature' do using RSpec::Parameterized::TableSyntax + before do + stub_feature_flags(split_operations_visibility_permissions: false) + end + let(:guest_operations_permissions) { [:read_environment, :read_deployment] } let(:developer_operations_permissions) do @@ -2002,38 +2006,95 @@ def set_access_level(access_level) end end - def project_subject(project_type) - case project_type - when :public - public_project - when :internal - internal_project + def permissions_abilities(role) + case role + when :maintainer + maintainer_operations_permissions + when :developer + developer_operations_permissions else - private_project + guest_operations_permissions end end + end + end - def user_subject(role) - case role - when :maintainer - maintainer - when :developer - developer - when :guest - guest - when :anonymous - anonymous + describe 'environments feature' do + using RSpec::Parameterized::TableSyntax + + let(:guest_environments_permissions) { [:read_environment, :read_deployment] } + + let(:developer_environments_permissions) do + guest_environments_permissions + [ + :create_environment, :create_deployment, :update_environment, :update_deployment, :destroy_environment + ] + end + + let(:maintainer_environments_permissions) do + developer_environments_permissions + [:admin_environment, :admin_deployment] + end + + where(:project_visibility, :access_level, :role, :allowed) do + :public | ProjectFeature::ENABLED | :maintainer | true + :public | ProjectFeature::ENABLED | :developer | true + :public | ProjectFeature::ENABLED | :guest | true + :public | ProjectFeature::ENABLED | :anonymous | true + :public | ProjectFeature::PRIVATE | :maintainer | true + :public | ProjectFeature::PRIVATE | :developer | true + :public | ProjectFeature::PRIVATE | :guest | true + :public | ProjectFeature::PRIVATE | :anonymous | false + :public | ProjectFeature::DISABLED | :maintainer | false + :public | ProjectFeature::DISABLED | :developer | false + :public | ProjectFeature::DISABLED | :guest | false + :public | ProjectFeature::DISABLED | :anonymous | false + :internal | ProjectFeature::ENABLED | :maintainer | true + :internal | ProjectFeature::ENABLED | :developer | true + :internal | ProjectFeature::ENABLED | :guest | true + :internal | ProjectFeature::ENABLED | :anonymous | false + :internal | ProjectFeature::PRIVATE | :maintainer | true + :internal | ProjectFeature::PRIVATE | :developer | true + :internal | ProjectFeature::PRIVATE | :guest | true + :internal | ProjectFeature::PRIVATE | :anonymous | false + :internal | ProjectFeature::DISABLED | :maintainer | false + :internal | ProjectFeature::DISABLED | :developer | false + :internal | ProjectFeature::DISABLED | :guest | false + :internal | ProjectFeature::DISABLED | :anonymous | false + :private | ProjectFeature::ENABLED | :maintainer | true + :private | ProjectFeature::ENABLED | :developer | true + :private | ProjectFeature::ENABLED | :guest | false + :private | ProjectFeature::ENABLED | :anonymous | false + :private | ProjectFeature::PRIVATE | :maintainer | true + :private | ProjectFeature::PRIVATE | :developer | true + :private | ProjectFeature::PRIVATE | :guest | false + :private | ProjectFeature::PRIVATE | :anonymous | false + :private | ProjectFeature::DISABLED | :maintainer | false + :private | ProjectFeature::DISABLED | :developer | false + :private | ProjectFeature::DISABLED | :guest | false + :private | ProjectFeature::DISABLED | :anonymous | false + end + + with_them do + let(:current_user) { user_subject(role) } + let(:project) { project_subject(project_visibility) } + + it 'allows/disallows the abilities based on the environments feature access level' do + project.project_feature.update!(environments_access_level: access_level) + + if allowed + expect_allowed(*permissions_abilities(role)) + else + expect_disallowed(*permissions_abilities(role)) end end def permissions_abilities(role) case role when :maintainer - maintainer_operations_permissions + maintainer_environments_permissions when :developer - developer_operations_permissions + developer_environments_permissions else - guest_operations_permissions + guest_environments_permissions end end end @@ -2481,4 +2542,28 @@ def permissions_abilities(role) end end end + + def project_subject(project_type) + case project_type + when :public + public_project + when :internal + internal_project + else + private_project + end + end + + def user_subject(role) + case role + when :maintainer + maintainer + when :developer + developer + when :guest + guest + when :anonymous + anonymous + end + end end -- GitLab