From 5d73884e21d606b2644b738eab30cefa235e467f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 11 Mar 2021 15:05:01 +0100 Subject: [PATCH 1/7] Add feature flag to block gitlab.com top-level group creation via api Add new Feature-flag, add new policy to block top-level group creation on Gitlab.com --- ee/app/policies/ee/global_policy.rb | 7 ++ .../ops/top_level_group_creation_enabled.yml | 8 ++ ee/lib/ee/api/groups.rb | 6 ++ ee/spec/policies/global_policy_spec.rb | 44 +++++++++ ee/spec/requests/api/groups_spec.rb | 94 +++++++++++++++++++ lib/api/groups.rb | 6 +- 6 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 ee/config/feature_flags/ops/top_level_group_creation_enabled.yml diff --git a/ee/app/policies/ee/global_policy.rb b/ee/app/policies/ee/global_policy.rb index 97c9ef1d778b0b..1f1df8dedcf874 100644 --- a/ee/app/policies/ee/global_policy.rb +++ b/ee/app/policies/ee/global_policy.rb @@ -21,6 +21,10 @@ module GlobalPolicy ::License.feature_available?(:export_user_permissions) end + condition(:top_level_group_creation_disabled) do + ::Gitlab.com? && ::Feature.disabled?(:top_level_group_creation_enabled, type: :ops) + end + rule { ~anonymous & operations_dashboard_available }.enable :read_operations_dashboard rule { admin }.policy do @@ -46,6 +50,9 @@ module GlobalPolicy end rule { export_user_permissions_available & admin }.enable :export_user_permissions + + rule { can?(:create_group) }.enable :create_group_via_api + rule { top_level_group_creation_disabled }.prevent :create_group_via_api end end end diff --git a/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml b/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml new file mode 100644 index 00000000000000..1c1671c3e9c84d --- /dev/null +++ b/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml @@ -0,0 +1,8 @@ +--- +name: top_level_group_creation_enabled +introduced_by_url: +rollout_issue_url: +milestone: '13.10' +type: ops +group: group::access +default_enabled: false diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index af4d5ef022edc7..7e8b99ddd54f68 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -49,6 +49,12 @@ def update_group(group) super end + override :authorize_group_creation! + def authorize_group_creation! + authorize! :create_group + authorize! :create_group_via_api + end + def check_audit_events_available!(group) forbidden! unless group.feature_available?(:audit_events) end diff --git a/ee/spec/policies/global_policy_spec.rb b/ee/spec/policies/global_policy_spec.rb index cd7c31a75abbb4..eee4da5a734df4 100644 --- a/ee/spec/policies/global_policy_spec.rb +++ b/ee/spec/policies/global_policy_spec.rb @@ -266,4 +266,48 @@ it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) } end end + + describe 'create_group_via_api' do + let(:policy) { :create_group_via_api } + + context 'on .com' do + before do + allow(::Gitlab).to receive(:com?).and_return(true) + end + + context 'when feature is enabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: true) + end + + it { is_expected.to be_allowed(policy) } + end + + context 'when feature is disabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: false) + end + + it { is_expected.to be_disallowed(policy) } + end + end + + context 'on self-managed' do + context 'when feature is enabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: true) + end + + it { is_expected.to be_allowed(policy) } + end + + context 'when feature is disabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: false) + end + + it { is_expected.to be_allowed(policy) } + end + end + end end diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index 40a1b77a175eb8..8b255314accba0 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -414,6 +414,100 @@ end end end + + context 'when creating group on .com' do + before do + allow(::Gitlab).to receive(:com?).and_return(true) + end + + context 'when top_level_group_creation_enabled feature flag is disabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: false) + end + + it 'does not create a top-level group' do + group = attributes_for_group_api + + expect do + post api("/groups", admin), params: group + end.not_to change { Group.count } + + expect(response).to have_gitlab_http_status(:forbidden) + end + + it 'does creates a subgroup' do + parent = create(:group) + parent.add_owner(admin) + + expect do + post api("/groups", admin), params: { parent_id: parent.id, name: 'foo', path: 'foo' } + end.to change { Group.count }.by(1) + + expect(response).to have_gitlab_http_status(:created) + end + end + + context 'when top_level_group_creation_enabled feature flag is enabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: true) + end + + it 'creates a top-level group' do + group = attributes_for_group_api + + expect do + post api("/groups", admin), params: group + end.to change { Group.count } + + expect(response).to have_gitlab_http_status(:created) + end + end + end + + context 'when creating group on sefl-managed' do + context 'when top_level_group_creation_enabled feature flag is disabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: false) + end + + it 'creates a top-level group' do + group = attributes_for_group_api + + expect do + post api("/groups", admin), params: group + end.to change { Group.count } + + expect(response).to have_gitlab_http_status(:created) + end + + it 'creates a subgroup' do + parent = create(:group) + parent.add_owner(admin) + + expect do + post api("/groups", admin), params: { parent_id: parent.id, name: 'foo', path: 'foo' } + end.to change { Group.count }.by(1) + + expect(response).to have_gitlab_http_status(:created) + end + end + + context 'when top_level_group_creation_enabled feature flag is enabled' do + before do + stub_feature_flags(top_level_group_creation_enabled: true) + end + + it 'creates a top-level group' do + group = attributes_for_group_api + + expect do + post api("/groups", admin), params: group + end.to change { Group.count } + + expect(response).to have_gitlab_http_status(:created) + end + end + end end describe 'POST /groups/:id/ldap_sync' do diff --git a/lib/api/groups.rb b/lib/api/groups.rb index a8b1cdab0212d9..26fa00d618669b 100644 --- a/lib/api/groups.rb +++ b/lib/api/groups.rb @@ -137,6 +137,10 @@ def handle_similarity_order(group, projects) end end # rubocop: enable CodeReuse/ActiveRecord + + def authorize_group_creation! + authorize! :create_group + end end resource :groups do @@ -169,7 +173,7 @@ def handle_similarity_order(group, projects) if parent_group authorize! :create_subgroup, parent_group else - authorize! :create_group + authorize_group_creation! end group = create_group -- GitLab From f5af8e63d1fea7430f0d816942a45bcaa04a6959 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Thu, 11 Mar 2021 15:59:54 +0100 Subject: [PATCH 2/7] Add docs and changelog entry --- doc/api/groups.md | 3 +++ .../unreleased/323573-limit-creating-top-level-groups.yml | 5 +++++ 2 files changed, 8 insertions(+) create mode 100644 ee/changelogs/unreleased/323573-limit-creating-top-level-groups.yml diff --git a/doc/api/groups.md b/doc/api/groups.md index b3ab00b362e847..41596a8d15e717 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -767,6 +767,9 @@ Parameters: | `shared_runners_minutes_limit` | integer | no | **(PREMIUM SELF)** Pipeline minutes quota for this group (included in plan). Can be `nil` (default; inherit system default), `0` (unlimited) or `> 0` | | `extra_shared_runners_minutes_limit` | integer | no | **(PREMIUM SELF)** Extra pipeline minutes quota for this group (purchased in addition to the minutes included in the plan). | +NOTE: +On GitLab.com as of [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56360) it is not possible to create group without a parent via API. It is still possible via UI. + ### Options for `default_branch_protection` The `default_branch_protection` attribute determines whether developers and maintainers can push to the applicable master branch, as described in the following table: diff --git a/ee/changelogs/unreleased/323573-limit-creating-top-level-groups.yml b/ee/changelogs/unreleased/323573-limit-creating-top-level-groups.yml new file mode 100644 index 00000000000000..d41b9b2280d8ff --- /dev/null +++ b/ee/changelogs/unreleased/323573-limit-creating-top-level-groups.yml @@ -0,0 +1,5 @@ +--- +title: Add feature flag to block gitlab.com top-level group creation via api +merge_request: 56360 +author: +type: added -- GitLab From ecb641fae930b1f8c3f5ac94da6228f133b37954 Mon Sep 17 00:00:00 2001 From: Mike Jang Date: Thu, 11 Mar 2021 18:00:38 +0000 Subject: [PATCH 3/7] Apply 1 suggestion(s) to 1 file(s) --- doc/api/groups.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/api/groups.md b/doc/api/groups.md index 41596a8d15e717..51185e4d7bf90c 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -768,7 +768,9 @@ Parameters: | `extra_shared_runners_minutes_limit` | integer | no | **(PREMIUM SELF)** Extra pipeline minutes quota for this group (purchased in addition to the minutes included in the plan). | NOTE: -On GitLab.com as of [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56360) it is not possible to create group without a parent via API. It is still possible via UI. +On GitLab.com as of [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56360), +you can no longer create a group without a parent through our API. You can still perform this +function through the UI. ### Options for `default_branch_protection` -- GitLab From 96e264df24c3e726e477a79d637675964d515340 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 12 Mar 2021 12:09:10 +0100 Subject: [PATCH 4/7] Reframe feature flag use --- ee/app/policies/ee/global_policy.rb | 10 +++++++--- .../ops/top_level_group_creation_enabled.yml | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/ee/app/policies/ee/global_policy.rb b/ee/app/policies/ee/global_policy.rb index 1f1df8dedcf874..1e5cd81049cfd7 100644 --- a/ee/app/policies/ee/global_policy.rb +++ b/ee/app/policies/ee/global_policy.rb @@ -21,8 +21,12 @@ module GlobalPolicy ::License.feature_available?(:export_user_permissions) end - condition(:top_level_group_creation_disabled) do - ::Gitlab.com? && ::Feature.disabled?(:top_level_group_creation_enabled, type: :ops) + condition(:top_level_group_creation_enabled) do + if ::Gitlab.com? + ::Feature.enabled?(:top_level_group_creation_enabled, type: :ops, default_enabled: true) + else + true + end end rule { ~anonymous & operations_dashboard_available }.enable :read_operations_dashboard @@ -52,7 +56,7 @@ module GlobalPolicy rule { export_user_permissions_available & admin }.enable :export_user_permissions rule { can?(:create_group) }.enable :create_group_via_api - rule { top_level_group_creation_disabled }.prevent :create_group_via_api + rule { ~top_level_group_creation_enabled }.prevent :create_group_via_api end end end diff --git a/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml b/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml index 1c1671c3e9c84d..334a3bc9280f5e 100644 --- a/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml +++ b/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml @@ -5,4 +5,4 @@ rollout_issue_url: milestone: '13.10' type: ops group: group::access -default_enabled: false +default_enabled: true -- GitLab From 37c149f4d513f6167080a84d5c2cedadad586687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 12 Mar 2021 13:01:05 +0100 Subject: [PATCH 5/7] Remove note from docs --- doc/api/groups.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/doc/api/groups.md b/doc/api/groups.md index 51185e4d7bf90c..b3ab00b362e847 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -767,11 +767,6 @@ Parameters: | `shared_runners_minutes_limit` | integer | no | **(PREMIUM SELF)** Pipeline minutes quota for this group (included in plan). Can be `nil` (default; inherit system default), `0` (unlimited) or `> 0` | | `extra_shared_runners_minutes_limit` | integer | no | **(PREMIUM SELF)** Extra pipeline minutes quota for this group (purchased in addition to the minutes included in the plan). | -NOTE: -On GitLab.com as of [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56360), -you can no longer create a group without a parent through our API. You can still perform this -function through the UI. - ### Options for `default_branch_protection` The `default_branch_protection` attribute determines whether developers and maintainers can push to the applicable master branch, as described in the following table: -- GitLab From 67a137fe5cdd9bc0de8b4aa056f0687cc6afd8d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 12 Mar 2021 14:50:23 +0100 Subject: [PATCH 6/7] Fix typo and add mr url to feature flag file --- .../feature_flags/ops/top_level_group_creation_enabled.yml | 2 +- ee/spec/requests/api/groups_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml b/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml index 334a3bc9280f5e..37a05607ba8c57 100644 --- a/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml +++ b/ee/config/feature_flags/ops/top_level_group_creation_enabled.yml @@ -1,6 +1,6 @@ --- name: top_level_group_creation_enabled -introduced_by_url: +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56360 rollout_issue_url: milestone: '13.10' type: ops diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index 8b255314accba0..09c32c0319f2d3 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -464,7 +464,7 @@ end end - context 'when creating group on sefl-managed' do + context 'when creating group on self-managed' do context 'when top_level_group_creation_enabled feature flag is disabled' do before do stub_feature_flags(top_level_group_creation_enabled: false) -- GitLab From a4d8eef613e4f48d5c88c98a95bda79140db6c86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C5=82gorzata=20Ksionek?= Date: Fri, 12 Mar 2021 16:48:27 +0100 Subject: [PATCH 7/7] Add cr remarks --- ee/lib/ee/api/groups.rb | 1 - ee/spec/requests/api/groups_spec.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index 7e8b99ddd54f68..7c071ec238025e 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -51,7 +51,6 @@ def update_group(group) override :authorize_group_creation! def authorize_group_creation! - authorize! :create_group authorize! :create_group_via_api end diff --git a/ee/spec/requests/api/groups_spec.rb b/ee/spec/requests/api/groups_spec.rb index 09c32c0319f2d3..af46cbbce6b11b 100644 --- a/ee/spec/requests/api/groups_spec.rb +++ b/ee/spec/requests/api/groups_spec.rb @@ -435,7 +435,7 @@ expect(response).to have_gitlab_http_status(:forbidden) end - it 'does creates a subgroup' do + it 'creates a subgroup' do parent = create(:group) parent.add_owner(admin) -- GitLab