From 6a2788234e11aa6aeb89034e575ef7879001202f Mon Sep 17 00:00:00 2001 From: Alishan Ladhani Date: Wed, 3 Aug 2022 12:09:53 -0400 Subject: [PATCH] Split feature flags visibility setting from operations --- app/controllers/projects_controller.rb | 17 +++- 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 | 7 ++ .../import_export/project/import_export.yml | 2 + rubocop/cop/gitlab/feature_available_usage.rb | 1 + spec/controllers/projects_controller_spec.rb | 81 ++++++++++++++---- spec/factories/projects.rb | 1 + spec/helpers/projects_helper_spec.rb | 3 +- .../import_export/safe_model_attributes.yml | 1 + .../project_features_compatibility_spec.rb | 4 +- spec/models/project_spec.rb | 1 + spec/policies/project_policy_spec.rb | 82 +++++++++++++++++++ 15 files changed, 189 insertions(+), 21 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 3f844e9ed06b4e..a9391159541763 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -46,6 +46,10 @@ class ProjectsController < Projects::ApplicationController push_frontend_feature_flag(:work_items_hierarchy, @project) end + before_action only: :edit do + push_frontend_feature_flag(:split_operations_visibility_permissions, @project) + end + layout :determine_layout feature_category :projects, [ @@ -416,10 +420,19 @@ def project_feature_attributes pages_access_level metrics_dashboard_access_level analytics_access_level - operations_access_level security_and_compliance_access_level container_registry_access_level - ] + ] + operations_feature_attributes + end + + def operations_feature_attributes + if Feature.enabled?(:split_operations_visibility_permissions, project) + %i[ + environments_access_level feature_flags_access_level + ] + else + %i[operations_access_level] + end end def project_setting_attributes diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 75622e7430c80e..b257d9896a2e79 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -640,7 +640,8 @@ def project_permissions_settings(project) enforceAuthChecksOnUploads: project.enforce_auth_checks_on_uploads?, securityAndComplianceAccessLevel: project.security_and_compliance_access_level, containerRegistryAccessLevel: feature.container_registry_access_level, - environmentsAccessLevel: feature.environments_access_level + environmentsAccessLevel: feature.environments_access_level, + featureFlagsAccessLevel: feature.feature_flags_access_level } end diff --git a/app/models/concerns/project_features_compatibility.rb b/app/models/concerns/project_features_compatibility.rb index 65b1cf023ec9aa..8f87e8b3f2249c 100644 --- a/app/models/concerns/project_features_compatibility.rb +++ b/app/models/concerns/project_features_compatibility.rb @@ -98,6 +98,10 @@ def environments_access_level=(value) write_feature_attribute_string(:environments_access_level, value) end + def feature_flags_access_level=(value) + write_feature_attribute_string(:feature_flags_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 23e10a00e59555..8259b9beebec96 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -449,7 +449,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, :environments_access_level, + :container_registry_access_level, :environments_access_level, :feature_flags_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 30d871a27562ce..82d98496b6b8bd 100644 --- a/app/models/project_feature.rb +++ b/app/models/project_feature.rb @@ -22,6 +22,7 @@ class ProjectFeature < ApplicationRecord container_registry package_registry environments + feature_flags ].freeze EXPORTABLE_FEATURES = (FEATURES - [:security_and_compliance, :pages]).freeze diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index a36aa5721f280f..c2bea7436eb17c 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -210,6 +210,7 @@ class ProjectPolicy < BasePolicy operations security_and_compliance environments + feature_flags ] features.each do |f| @@ -389,6 +390,12 @@ class ProjectPolicy < BasePolicy prevent(*create_read_update_admin_destroy(:deployment)) end + rule { split_operations_visibility_permissions & feature_flags_disabled }.policy do + prevent(*create_read_update_admin_destroy(:feature_flag)) + prevent(:admin_feature_flags_user_lists) + prevent(:admin_feature_flags_client) + 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 4ead49e9c385fd..b5925013f1522a 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -285,6 +285,7 @@ included_attributes: - :container_registry_access_level - :package_registry_access_level - :environments_access_level + - :feature_flags_access_level prometheus_metrics: - :created_at - :updated_at @@ -687,6 +688,7 @@ included_attributes: - :container_registry_access_level - :package_registry_access_level - :environments_access_level + - :feature_flags_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 fafe6c35b5ff5b..a153d3f7b2f5c9 100644 --- a/rubocop/cop/gitlab/feature_available_usage.rb +++ b/rubocop/cop/gitlab/feature_available_usage.rb @@ -24,6 +24,7 @@ class FeatureAvailableUsage < RuboCop::Cop::Cop security_and_compliance container_registry environments + feature_flags ].freeze EE_FEATURES = %i[requirements].freeze ALL_FEATURES = (FEATURES + EE_FEATURES).freeze diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 34477a7bb688b8..388d7c2266b4e4 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -878,30 +878,81 @@ def update_project(**parameters) end context 'with project feature attributes' do - using RSpec::Parameterized::TableSyntax + let(:initial_value) { ProjectFeature::PRIVATE } + let(:update_to) { ProjectFeature::ENABLED } - where(:feature, :initial_value, :update_to) do - :metrics_dashboard_access_level | ProjectFeature::PRIVATE | ProjectFeature::ENABLED - :container_registry_access_level | ProjectFeature::ENABLED | ProjectFeature::PRIVATE + before do + project.project_feature.update!(feature_access_level => initial_value) end - with_them do - it "updates the project_feature new" do - params = { - namespace_id: project.namespace, - id: project.path, - project: { - project_feature_attributes: { - "#{feature}": update_to - } + def update_project_feature + put :update, params: { + namespace_id: project.namespace, + id: project.path, + project: { + project_feature_attributes: { + feature_access_level.to_s => update_to } } + } + end - expect { put :update, params: params }.to change { - project.reload.project_feature.public_send(feature) + shared_examples 'feature update success' do + it 'updates access level successfully' do + expect { update_project_feature }.to change { + project.reload.project_feature.public_send(feature_access_level) }.from(initial_value).to(update_to) end end + + shared_examples 'feature update failure' do + it 'cannot update access level' do + expect { update_project_feature }.not_to change { + project.reload.project_feature.public_send(feature_access_level) + } + end + end + + where(:feature_access_level) do + %i[ + metrics_dashboard_access_level + container_registry_access_level + environments_access_level + feature_flags_access_level + ] + end + + with_them do + it_behaves_like 'feature update success' + end + + context 'for feature_access_level operations_access_level' do + let(:feature_access_level) { :operations_access_level } + + include_examples 'feature update failure' + end + + context 'with feature flag split_operations_visibility_permissions disabled' do + before do + stub_feature_flags(split_operations_visibility_permissions: false) + end + + context 'for feature_access_level operations_access_level' do + let(:feature_access_level) { :operations_access_level } + + include_examples 'feature update success' + end + + where(:feature_access_level) do + %i[ + environments_access_level feature_flags_access_level + ] + end + + with_them do + it_behaves_like 'feature update failure' + end + end end end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 107fa28a639e12..98dfecb2888e0c 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -38,6 +38,7 @@ container_registry_access_level { ProjectFeature::ENABLED } security_and_compliance_access_level { ProjectFeature::PRIVATE } environments_access_level { ProjectFeature::ENABLED } + feature_flags_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 200e93f8dff07c..214086a3823211 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -967,7 +967,8 @@ def license_name showDefaultAwardEmojis: project.show_default_award_emojis?, securityAndComplianceAccessLevel: project.security_and_compliance_access_level, containerRegistryAccessLevel: project.project_feature.container_registry_access_level, - environmentsAccessLevel: project.project_feature.environments_access_level + environmentsAccessLevel: project.project_feature.environments_access_level, + featureFlagsAccessLevel: project.project_feature.feature_flags_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 3ebed1096e3212..e3390496c4fed5 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: - container_registry_access_level - package_registry_access_level - environments_access_level +- feature_flags_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 6730b7a02f358d..48c391451edf7c 100644 --- a/spec/models/concerns/project_features_compatibility_spec.rb +++ b/spec/models/concerns/project_features_compatibility_spec.rb @@ -5,7 +5,9 @@ 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 environments) } + let(:features) do + features_enabled + %w(repository pages operations container_registry package_registry environments feature_flags) + end # 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 afb321c0777220..403cf31eb898b3 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -832,6 +832,7 @@ 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) } + it { is_expected.to delegate_method(:feature_flags_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 01a0a96be30bfc..c3817f671585f8 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -2100,6 +2100,77 @@ def permissions_abilities(role) end end + describe 'feature flags feature' do + using RSpec::Parameterized::TableSyntax + + let(:guest_permissions) { [] } + + let(:developer_permissions) do + guest_permissions + [ + :read_feature_flag, :create_feature_flag, :update_feature_flag, :destroy_feature_flag, :admin_feature_flag, + :admin_feature_flags_user_lists + ] + end + + let(:maintainer_permissions) do + developer_permissions + [:admin_feature_flags_client] + 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 feature flags access level' do + project.project_feature.update!(feature_flags_access_level: access_level) + + if allowed + expect_allowed(*permissions_abilities(role)) + else + expect_disallowed(*permissions_abilities(role)) + end + end + end + end + describe 'access_security_and_compliance' do context 'when the "Security & Compliance" is enabled' do before do @@ -2566,4 +2637,15 @@ def user_subject(role) anonymous end end + + def permissions_abilities(role) + case role + when :maintainer + maintainer_permissions + when :developer + developer_permissions + else + guest_permissions + end + end end -- GitLab