diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 3f844e9ed06b4e7d5802c63212a2b0bcf0167520..a9391159541763fdcf327271e106367ea4502ba9 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 75622e7430c80ecfd59ca76bfcfac7f04140567c..b257d9896a2e79cc3a0af884b9d072ffbbc0856c 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 65b1cf023ec9aa483ee45db2706361c187e1839e..8f87e8b3f2249c577a5e7654acd8dd43213f06f3 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 23e10a00e5955547aab1814d320f19cecdb1610e..8259b9beebec961d879e334564d779a0f1d75c06 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 30d871a27562ce5152fdec71cdd7d4caa0176f46..82d98496b6b8bdf08446131a2ed2b977bf5c46a1 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 a36aa5721f280ff6bb9f59c8101effd765b09ec4..c2bea7436eb17c9f6279d33bc92f9a9433e92af3 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 4ead49e9c385fde7541636610e1cb6ad94a33019..b5925013f1522a5d1b1b100325ab5c0ef347b7cb 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 fafe6c35b5ff5bfc88baf0868b055f08d372c166..a153d3f7b2f5c93d1e212ff2f26390a080dc0539 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 34477a7bb688b83c4900f710dfce5afdb331b859..388d7c2266b4e4fe6748550e2880758f4ff2e73d 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 107fa28a639e12fcbdcd038c959184cf73c2627c..98dfecb2888e0ccad386e95abca279ea75485a51 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 200e93f8dff07c66eb4abf9f385ec97309ec8b7b..214086a382321165282677a66e99c8809aac4ad5 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 3ebed1096e321242fbdb7fc6c3c5f994d3286088..e3390496c4fed5c77620a5f8a94b2abbbf1bb86f 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 6730b7a02f358d61fe284b80a1d9a839edb22f4a..48c391451edf7c84df3fecc599f1ca45767979ca 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 afb321c0777220490cea78af89de14db33e97132..403cf31eb898b3b59d263a1ef7acc308a9689b14 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 01a0a96be30bfca3aaa4d8174664ed479bbc3bbc..c3817f671585f847fd64786e7b0dd3f90288a6ef 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