From 1602277c2aaa28dd558f79bfe57394bffa5f69c7 Mon Sep 17 00:00:00 2001 From: manojmj Date: Wed, 19 Feb 2020 11:16:25 +0530 Subject: [PATCH 1/7] Introduce default branch protection at the group level This change introduces default branch protection at the group level so that group owners can set their own preference for default branch protection, and this will override the instance level default branch protection --- app/controllers/groups_controller.rb | 3 +- app/models/namespace.rb | 4 ++ app/models/project.rb | 6 +++ app/models/protected_branch.rb | 10 ++-- .../protect_default_branch_service.rb | 2 +- .../_visibility_and_access.html.haml | 5 +- .../groups/settings/_permissions.html.haml | 1 + .../_default_branch_protection.html.haml | 3 ++ ...push-to-projects-they-create-in-groups.yml | 5 ++ ...default_branch_protection_to_namespaces.rb | 9 ++++ db/schema.rb | 1 + lib/api/entities/group.rb | 1 + lib/api/helpers/groups_helpers.rb | 1 + lib/gitlab/access/branch_protection.rb | 4 ++ spec/controllers/groups_controller_spec.rb | 7 +++ .../gitlab/access/branch_protection_spec.rb | 17 +++++++ spec/lib/gitlab/user_access_spec.rb | 47 +++++++++---------- spec/models/namespace_spec.rb | 37 +++++++++++++++ spec/models/project_spec.rb | 29 ++++++++++-- spec/models/protected_branch_spec.rb | 33 +++++-------- spec/requests/api/groups_spec.rb | 4 +- spec/services/git/branch_push_service_spec.rb | 8 ++-- .../protect_default_branch_service_spec.rb | 14 +++--- 23 files changed, 178 insertions(+), 73 deletions(-) create mode 100644 app/views/shared/_default_branch_protection.html.haml create mode 100644 changelogs/unreleased/7583-developer-cannot-push-to-projects-they-create-in-groups.yml create mode 100644 db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 80c7a803392f55..7175eefcde7e37 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -195,7 +195,8 @@ def group_params_attributes :require_two_factor_authentication, :two_factor_grace_period, :project_creation_level, - :subgroup_creation_level + :subgroup_creation_level, + :default_branch_protection ] end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 99212d09b8ee95..f06e9da3b2aff4 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -139,6 +139,10 @@ def reset_ci_minutes!(namespace_id) end end + def default_branch_protection + super || Gitlab::CurrentSettings.default_branch_protection + end + def visibility_level_field :visibility_level end diff --git a/app/models/project.rb b/app/models/project.rb index f72e777c00419f..fdf7452d143320 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2359,6 +2359,12 @@ def deploy_token_revoke_url_for(token) Gitlab::Routing.url_helpers.revoke_project_deploy_token_path(self, token) end + def default_branch_protected? + branch_protection = Gitlab::Access::BranchProtection.new(self.namespace.default_branch_protection) + + branch_protection.fully_protected? || branch_protection.developer_can_merge? + end + private def closest_namespace_setting(name) diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 94c3b83564f54d..594c822c18f68a 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -11,7 +11,8 @@ class ProtectedBranch < ApplicationRecord def self.protected_ref_accessible_to?(ref, user, project:, action:, protected_refs: nil) # Maintainers, owners and admins are allowed to create the default branch - if default_branch_protected? && project.empty_repo? + + if project.empty_repo? && project.default_branch_protected? return true if user.admin? || project.team.max_member_access(user.id) > Gitlab::Access::DEVELOPER end @@ -20,7 +21,7 @@ def self.protected_ref_accessible_to?(ref, user, project:, action:, protected_re # Check if branch name is marked as protected in the system def self.protected?(project, ref_name) - return true if project.empty_repo? && default_branch_protected? + return true if project.empty_repo? && project.default_branch_protected? self.matching(ref_name, protected_refs: protected_refs(project)).present? end @@ -33,11 +34,6 @@ def self.any_protected?(project, ref_names) end end - def self.default_branch_protected? - Gitlab::CurrentSettings.default_branch_protection == Gitlab::Access::PROTECTION_FULL || - Gitlab::CurrentSettings.default_branch_protection == Gitlab::Access::PROTECTION_DEV_CAN_MERGE - end - def self.protected_refs(project) project.protected_branches end diff --git a/app/services/projects/protect_default_branch_service.rb b/app/services/projects/protect_default_branch_service.rb index 245490791bf605..1d3fb523448720 100644 --- a/app/services/projects/protect_default_branch_service.rb +++ b/app/services/projects/protect_default_branch_service.rb @@ -11,7 +11,7 @@ def initialize(project) @project = project @default_branch_protection = Gitlab::Access::BranchProtection - .new(Gitlab::CurrentSettings.default_branch_protection) + .new(project.namespace.default_branch_protection) end def execute diff --git a/app/views/admin/application_settings/_visibility_and_access.html.haml b/app/views/admin/application_settings/_visibility_and_access.html.haml index ae90ffd9efc90b..a4acbe6c885583 100644 --- a/app/views/admin/application_settings/_visibility_and_access.html.haml +++ b/app/views/admin/application_settings/_visibility_and_access.html.haml @@ -2,9 +2,8 @@ = form_errors(@application_setting) %fieldset - .form-group - = f.label :default_branch_protection, class: 'label-bold' - = f.select :default_branch_protection, options_for_select(Gitlab::Access.protection_options, @application_setting.default_branch_protection), {}, class: 'form-control' + = render 'shared/default_branch_protection', f: f, selected_level: @application_setting.default_branch_protection + .form-group = f.label s_('ProjectCreationLevel|Default project creation protection'), class: 'label-bold' = f.select :default_project_creation, options_for_select(Gitlab::Access.project_creation_options, @application_setting.default_project_creation), {}, class: 'form-control' diff --git a/app/views/groups/settings/_permissions.html.haml b/app/views/groups/settings/_permissions.html.haml index 618cfe57be4819..016a9c8e054086 100644 --- a/app/views/groups/settings/_permissions.html.haml +++ b/app/views/groups/settings/_permissions.html.haml @@ -33,6 +33,7 @@ = render_if_exists 'groups/settings/ip_restriction', f: f, group: @group = render_if_exists 'groups/settings/allowed_email_domain', f: f, group: @group = render 'groups/settings/lfs', f: f + = render 'shared/default_branch_protection', f: f, selected_level: @group.default_branch_protection = render 'groups/settings/project_creation_level', f: f, group: @group = render 'groups/settings/subgroup_creation_level', f: f, group: @group = render 'groups/settings/two_factor_auth', f: f diff --git a/app/views/shared/_default_branch_protection.html.haml b/app/views/shared/_default_branch_protection.html.haml new file mode 100644 index 00000000000000..d7ae21debd8681 --- /dev/null +++ b/app/views/shared/_default_branch_protection.html.haml @@ -0,0 +1,3 @@ +.form-group + = f.label :default_branch_protection, class: 'label-bold' + = f.select :default_branch_protection, options_for_select(Gitlab::Access.protection_options, selected_level), {}, class: 'form-control' diff --git a/changelogs/unreleased/7583-developer-cannot-push-to-projects-they-create-in-groups.yml b/changelogs/unreleased/7583-developer-cannot-push-to-projects-they-create-in-groups.yml new file mode 100644 index 00000000000000..2055abc45516af --- /dev/null +++ b/changelogs/unreleased/7583-developer-cannot-push-to-projects-they-create-in-groups.yml @@ -0,0 +1,5 @@ +--- +title: Introduce default branch protection at the group level +merge_request: 24426 +author: +type: added diff --git a/db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb b/db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb new file mode 100644 index 00000000000000..8a75c7d94b494a --- /dev/null +++ b/db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddDefaultBranchProtectionToNamespaces < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column :namespaces, :default_branch_protection, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 149582f94bf46b..103faa02009913 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2762,6 +2762,7 @@ t.integer "max_pages_size" t.integer "max_artifacts_size" t.boolean "mentions_disabled" + t.integer "default_branch_protection" t.index ["created_at"], name: "index_namespaces_on_created_at" t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)" t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id" diff --git a/lib/api/entities/group.rb b/lib/api/entities/group.rb index ae5ee4784edf6f..10e10e52d9f3d8 100644 --- a/lib/api/entities/group.rb +++ b/lib/api/entities/group.rb @@ -13,6 +13,7 @@ class Group < BasicGroupDetails expose :emails_disabled expose :mentions_disabled expose :lfs_enabled?, as: :lfs_enabled + expose :default_branch_protection expose :avatar_url do |group, options| group.avatar_url(only_path: false) end diff --git a/lib/api/helpers/groups_helpers.rb b/lib/api/helpers/groups_helpers.rb index e0fea4c7c9609d..25701ff683dee8 100644 --- a/lib/api/helpers/groups_helpers.rb +++ b/lib/api/helpers/groups_helpers.rb @@ -21,6 +21,7 @@ module GroupsHelpers optional :mentions_disabled, type: Boolean, desc: 'Disable a group from getting mentioned' optional :lfs_enabled, type: Boolean, desc: 'Enable/disable LFS for the projects in this group' optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' + optional :default_branch_protection, type: Integer, values: ::Gitlab::Access.protection_values, desc: 'Determine if developers can push to master' end params :optional_params_ee do diff --git a/lib/gitlab/access/branch_protection.rb b/lib/gitlab/access/branch_protection.rb index f039e5c011f924..339a99eb068685 100644 --- a/lib/gitlab/access/branch_protection.rb +++ b/lib/gitlab/access/branch_protection.rb @@ -37,6 +37,10 @@ def developer_can_push? def developer_can_merge? level == PROTECTION_DEV_CAN_MERGE end + + def fully_protected? + level == PROTECTION_FULL + end end end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 1c58c2b5c97260..11c70d3aeca058 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -411,6 +411,13 @@ expect(group.reload.project_creation_level).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS) end + it 'updates the default_branch_protection successfully' do + post :update, params: { id: group.to_param, group: { default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE } } + + expect(response).to have_gitlab_http_status(:found) + expect(group.reload.default_branch_protection).to eq(::Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + end + context 'when a project inside the group has container repositories' do before do stub_container_registry_config(enabled: true) diff --git a/spec/lib/gitlab/access/branch_protection_spec.rb b/spec/lib/gitlab/access/branch_protection_spec.rb index 7f2979e8e28d42..e4b763357c43b6 100644 --- a/spec/lib/gitlab/access/branch_protection_spec.rb +++ b/spec/lib/gitlab/access/branch_protection_spec.rb @@ -51,4 +51,21 @@ end end end + + describe '#fully_protected?' do + using RSpec::Parameterized::TableSyntax + + where(:level, :result) do + Gitlab::Access::PROTECTION_NONE | false + Gitlab::Access::PROTECTION_DEV_CAN_PUSH | false + Gitlab::Access::PROTECTION_DEV_CAN_MERGE | false + Gitlab::Access::PROTECTION_FULL | true + end + + with_them do + it do + expect(described_class.new(level).fully_protected?).to eq(result) + end + end + end end diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index 2f4ab2e71dbb5d..8d13f3776771a7 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -46,32 +46,27 @@ expect(project_access.can_push_to_branch?('master')).to be_truthy end - it 'returns false if user is developer and project is fully protected' do - empty_project.add_developer(user) - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL) - - expect(project_access.can_push_to_branch?('master')).to be_falsey - end - - it 'returns false if user is developer and it is not allowed to push new commits but can merge into branch' do - empty_project.add_developer(user) - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) - - expect(project_access.can_push_to_branch?('master')).to be_falsey - end - - it 'returns true if user is developer and project is unprotected' do - empty_project.add_developer(user) - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) - - expect(project_access.can_push_to_branch?('master')).to be_truthy - end - - it 'returns true if user is developer and project grants developers permission' do - empty_project.add_developer(user) - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) - - expect(project_access.can_push_to_branch?('master')).to be_truthy + context 'when the user is a developer' do + using RSpec::Parameterized::TableSyntax + + before do + empty_project.add_developer(user) + end + + where(:default_branch_protection_level, :result) do + Gitlab::Access::PROTECTION_NONE | true + Gitlab::Access::PROTECTION_DEV_CAN_PUSH | true + Gitlab::Access::PROTECTION_DEV_CAN_MERGE | false + Gitlab::Access::PROTECTION_FULL | false + end + + with_them do + it do + expect(empty_project.namespace).to receive(:default_branch_protection).and_return(default_branch_protection_level).at_least(:once) + + expect(project_access.can_push_to_branch?('master')).to eq(result) + end + end end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 54e54366c5a7cb..e1dfe675b8e2e8 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -531,6 +531,43 @@ def project_rugged(project) end end + describe "#default_branch_protection" do + let(:namespace) { create(:namespace) } + let(:group) { create(:group) } + + before do + stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + end + + context 'for a namespace' do + # Unlike a group, the settings of a namespace cannot be altered + # via the UI or the API, so the value of `default_branch_protection` + # for a namespace will always be `nil` in the database. + + it 'returns the instance level setting' do + expect(namespace.default_branch_protection).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + end + end + + context 'for a group' do + context 'that has not altered the default value' do + it 'returns the instance level setting' do + expect(group.default_branch_protection).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + end + end + + context 'that has altered the default value' do + before do + group.update!(default_branch_protection: Gitlab::Access::PROTECTION_FULL) + end + + it 'returns the group level setting' do + expect(group.default_branch_protection).to eq(Gitlab::Access::PROTECTION_FULL) + end + end + end + end + describe '#self_and_hierarchy' do let!(:group) { create(:group, path: 'git_lab') } let!(:nested_group) { create(:group, parent: group) } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2b4a832634f0d0..4885d4b63ff4a6 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1623,6 +1623,29 @@ end end + describe '#default_branch_protected?' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:project) { create(:project) } + + subject { project.default_branch_protected? } + + where(:default_branch_protection_level, :result) do + Gitlab::Access::PROTECTION_NONE | false + Gitlab::Access::PROTECTION_DEV_CAN_PUSH | false + Gitlab::Access::PROTECTION_DEV_CAN_MERGE | true + Gitlab::Access::PROTECTION_FULL | true + end + + with_them do + before do + expect(project.namespace).to receive(:default_branch_protection).and_return(default_branch_protection_level) + end + + it { is_expected.to eq(result) } + end + end + describe '#pages_url' do let(:group) { create(:group, name: group_name) } let(:project) { create(:project, namespace: group, name: project_name) } @@ -4576,7 +4599,7 @@ def enable_lfs end it 'does not protect when branch protection is disabled' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) + expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_NONE) project.after_import @@ -4584,7 +4607,7 @@ def enable_lfs end it "gives developer access to push when branch protection is set to 'developers can push'" do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) + expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH) project.after_import @@ -4594,7 +4617,7 @@ def enable_lfs end it "gives developer access to merge when branch protection is set to 'developers can merge'" do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) project.after_import diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 7f8a60dafa8cce..305fc07b9589a4 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -164,31 +164,24 @@ end end - context "new project" do - let(:project) { create(:project) } - - it 'returns false when default_protected_branch is unprotected' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) - - expect(described_class.protected?(project, 'master')).to be false - end - - it 'returns false when default_protected_branch lets developers push' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) + context 'new project' do + using RSpec::Parameterized::TableSyntax - expect(described_class.protected?(project, 'master')).to be false - end - - it 'returns true when default_branch_protection does not let developers push but let developer merge branches' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + let(:project) { create(:project) } - expect(described_class.protected?(project, 'master')).to be true + where(:default_branch_protection_level, :result) do + Gitlab::Access::PROTECTION_NONE | false + Gitlab::Access::PROTECTION_DEV_CAN_PUSH | false + Gitlab::Access::PROTECTION_DEV_CAN_MERGE | true + Gitlab::Access::PROTECTION_FULL | true end - it 'returns true when default_branch_protection is in full protection' do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_FULL) + with_them do + it do + expect(project.namespace).to receive(:default_branch_protection).and_return(default_branch_protection_level) - expect(described_class.protected?(project, 'master')).to be true + expect(described_class.protected?(project, 'master')).to eq(result) + end end end end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index e849924684095a..fb564bb398b70c 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -545,7 +545,8 @@ def response_project_ids(json_response, key) name: new_group_name, request_access_enabled: true, project_creation_level: "noone", - subgroup_creation_level: "maintainer" + subgroup_creation_level: "maintainer", + default_branch_protection: ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS } expect(response).to have_gitlab_http_status(:ok) @@ -566,6 +567,7 @@ def response_project_ids(json_response, key) expect(json_response['projects'].length).to eq(2) expect(json_response['shared_projects']).to be_an Array expect(json_response['shared_projects'].length).to eq(0) + expect(json_response['default_branch_protection']).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS) end it 'returns 404 for a non existing group' do diff --git a/spec/services/git/branch_push_service_spec.rb b/spec/services/git/branch_push_service_spec.rb index 8b4f45010ed777..d7357cf4d0b2cd 100644 --- a/spec/services/git/branch_push_service_spec.rb +++ b/spec/services/git/branch_push_service_spec.rb @@ -186,7 +186,7 @@ end it "when pushing a branch for the first time with default branch protection disabled" do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE) + expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_NONE) expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") @@ -195,7 +195,7 @@ end it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) + expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH) expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") @@ -208,7 +208,7 @@ end it "when pushing a branch for the first time with an existing branch permission configured" do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH) + expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH) create(:protected_branch, :no_one_can_push, :developers_can_merge, project: project, name: 'master') expect(project).to receive(:execute_hooks) @@ -223,7 +223,7 @@ end it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do - stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) + expect(project.namespace).to receive(:default_branch_protection).and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") diff --git a/spec/services/projects/protect_default_branch_service_spec.rb b/spec/services/projects/protect_default_branch_service_spec.rb index c145b2c06c633c..c0b819ab17b505 100644 --- a/spec/services/projects/protect_default_branch_service_spec.rb +++ b/spec/services/projects/protect_default_branch_service_spec.rb @@ -4,7 +4,7 @@ describe Projects::ProtectDefaultBranchService do let(:service) { described_class.new(project) } - let(:project) { instance_spy(Project) } + let(:project) { create(:project) } describe '#execute' do before do @@ -147,7 +147,7 @@ describe '#protect_branch?' do context 'when default branch protection is disabled' do it 'returns false' do - allow(Gitlab::CurrentSettings) + allow(project.namespace) .to receive(:default_branch_protection) .and_return(Gitlab::Access::PROTECTION_NONE) @@ -157,7 +157,7 @@ context 'when default branch protection is enabled' do before do - allow(Gitlab::CurrentSettings) + allow(project.namespace) .to receive(:default_branch_protection) .and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) @@ -199,7 +199,7 @@ describe '#push_access_level' do context 'when developers can push' do it 'returns the DEVELOPER access level' do - allow(Gitlab::CurrentSettings) + allow(project.namespace) .to receive(:default_branch_protection) .and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH) @@ -209,7 +209,7 @@ context 'when developers can not push' do it 'returns the MAINTAINER access level' do - allow(Gitlab::CurrentSettings) + allow(project.namespace) .to receive(:default_branch_protection) .and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) @@ -221,7 +221,7 @@ describe '#merge_access_level' do context 'when developers can merge' do it 'returns the DEVELOPER access level' do - allow(Gitlab::CurrentSettings) + allow(project.namespace) .to receive(:default_branch_protection) .and_return(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) @@ -231,7 +231,7 @@ context 'when developers can not merge' do it 'returns the MAINTAINER access level' do - allow(Gitlab::CurrentSettings) + allow(project.namespace) .to receive(:default_branch_protection) .and_return(Gitlab::Access::PROTECTION_DEV_CAN_PUSH) -- GitLab From fdfd165b5990e772cbbca27ea9e68a17aaea7d0d Mon Sep 17 00:00:00 2001 From: manojmj Date: Wed, 26 Feb 2020 10:11:52 +0530 Subject: [PATCH 2/7] Apply limit to migration --- ...207062728_add_default_branch_protection_to_namespaces.rb | 6 +++++- db/schema.rb | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb b/db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb index 8a75c7d94b494a..c21cac307f96e0 100644 --- a/db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb +++ b/db/migrate/20200207062728_add_default_branch_protection_to_namespaces.rb @@ -1,9 +1,13 @@ # frozen_string_literal: true class AddDefaultBranchProtectionToNamespaces < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + DOWNTIME = false def change - add_column :namespaces, :default_branch_protection, :integer + with_lock_retries do + add_column :namespaces, :default_branch_protection, :integer, limit: 2 + end end end diff --git a/db/schema.rb b/db/schema.rb index 103faa02009913..fa251f65741074 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2762,7 +2762,7 @@ t.integer "max_pages_size" t.integer "max_artifacts_size" t.boolean "mentions_disabled" - t.integer "default_branch_protection" + t.integer "default_branch_protection", limit: 2 t.index ["created_at"], name: "index_namespaces_on_created_at" t.index ["custom_project_templates_group_id", "type"], name: "index_namespaces_on_custom_project_templates_group_id_and_type", where: "(custom_project_templates_group_id IS NOT NULL)" t.index ["file_template_project_id"], name: "index_namespaces_on_file_template_project_id" -- GitLab From 0e0f0736bb9ac337ebb994be9d901b2d90c8bd19 Mon Sep 17 00:00:00 2001 From: manojmj Date: Thu, 27 Feb 2020 11:16:50 +0530 Subject: [PATCH 3/7] Address code review comments --- spec/models/namespace_spec.rb | 7 +++-- spec/models/protected_branch_spec.rb | 39 +++++++++++++++++++++------- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index e1dfe675b8e2e8..26a2e9bd342ed3 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -557,12 +557,11 @@ def project_rugged(project) end context 'that has altered the default value' do - before do - group.update!(default_branch_protection: Gitlab::Access::PROTECTION_FULL) - end + let(:default_branch_protection) { Gitlab::Access::PROTECTION_FULL } + let(:group) { create(:group, default_branch_protection: default_branch_protection) } it 'returns the group level setting' do - expect(group.default_branch_protection).to eq(Gitlab::Access::PROTECTION_FULL) + expect(group.default_branch_protection).to eq(default_branch_protection) end end end diff --git a/spec/models/protected_branch_spec.rb b/spec/models/protected_branch_spec.rb index 305fc07b9589a4..30fce1cd5c4cdf 100644 --- a/spec/models/protected_branch_spec.rb +++ b/spec/models/protected_branch_spec.rb @@ -169,18 +169,39 @@ let(:project) { create(:project) } - where(:default_branch_protection_level, :result) do - Gitlab::Access::PROTECTION_NONE | false - Gitlab::Access::PROTECTION_DEV_CAN_PUSH | false - Gitlab::Access::PROTECTION_DEV_CAN_MERGE | true - Gitlab::Access::PROTECTION_FULL | true + context 'when the group has set their own default_branch_protection level' do + where(:default_branch_protection_level, :result) do + Gitlab::Access::PROTECTION_NONE | false + Gitlab::Access::PROTECTION_DEV_CAN_PUSH | false + Gitlab::Access::PROTECTION_DEV_CAN_MERGE | true + Gitlab::Access::PROTECTION_FULL | true + end + + with_them do + it 'protects the default branch based on the default branch protection setting of the group' do + expect(project.namespace).to receive(:default_branch_protection).and_return(default_branch_protection_level) + + expect(described_class.protected?(project, 'master')).to eq(result) + end + end end - with_them do - it do - expect(project.namespace).to receive(:default_branch_protection).and_return(default_branch_protection_level) + context 'when the group has not set their own default_branch_protection level' do + where(:default_branch_protection_level, :result) do + Gitlab::Access::PROTECTION_NONE | false + Gitlab::Access::PROTECTION_DEV_CAN_PUSH | false + Gitlab::Access::PROTECTION_DEV_CAN_MERGE | true + Gitlab::Access::PROTECTION_FULL | true + end + + with_them do + before do + stub_application_setting(default_branch_protection: default_branch_protection_level) + end - expect(described_class.protected?(project, 'master')).to eq(result) + it 'protects the default branch based on the instance level default branch protection setting' do + expect(described_class.protected?(project, 'master')).to eq(result) + end end end end -- GitLab From 501ce6a1b0a4feee59cd858ccf2725cd4f8d2dea Mon Sep 17 00:00:00 2001 From: manojmj Date: Thu, 27 Feb 2020 12:55:56 +0530 Subject: [PATCH 4/7] Add documentation for feature --- doc/api/groups.md | 5 +++++ .../settings/visibility_and_access_controls.md | 2 ++ doc/user/group/index.md | 15 +++++++++++++++ 3 files changed, 22 insertions(+) diff --git a/doc/api/groups.md b/doc/api/groups.md index 3a488ca65469d1..c34a0dada1e701 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -42,6 +42,7 @@ GET /groups "emails_disabled": null, "mentions_disabled": null, "lfs_enabled": true, + "default_branch_protection": 2, "avatar_url": "http://localhost:3000/uploads/group/avatar/1/foo.jpg", "web_url": "http://localhost:3000/groups/foo-bar", "request_access_enabled": false, @@ -76,6 +77,7 @@ GET /groups?statistics=true "emails_disabled": null, "mentions_disabled": null, "lfs_enabled": true, + "default_branch_protection": 2, "avatar_url": "http://localhost:3000/uploads/group/avatar/1/foo.jpg", "web_url": "http://localhost:3000/groups/foo-bar", "request_access_enabled": false, @@ -148,6 +150,7 @@ GET /groups/:id/subgroups "emails_disabled": null, "mentions_disabled": null, "lfs_enabled": true, + "default_branch_protection": 2, "avatar_url": "http://gitlab.example.com/uploads/group/avatar/1/foo.jpg", "web_url": "http://gitlab.example.com/groups/foo-bar", "request_access_enabled": false, @@ -493,6 +496,7 @@ Parameters: | `lfs_enabled` | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group. | | `request_access_enabled` | boolean | no | Allow users to request member access. | | `parent_id` | integer | no | The parent group ID for creating nested group. | +| `default_branch_protection` | integer | no | Determine if developers can push to master branch of projects in the group. Can take: `0` _(not protected, both developers and maintainers can push new commits, force push, or delete the branch)_, `1` _(partially protected, developers and maintainers can push new commits, but cannot force push or delete the branch)_ or `2` _(fully protected, developers cannot push new commits, but maintainers can; no-one can force push or delete the branch)_ as a parameter. Default to the global level default branch protection setting. | | `shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Pipeline minutes quota for this group. | | `extra_shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Extra pipeline minutes quota for this group. | @@ -542,6 +546,7 @@ PUT /groups/:id | `mentions_disabled` | boolean | no | Disable the capability of a group from getting mentioned | | `lfs_enabled` (optional) | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group. | | `request_access_enabled` | boolean | no | Allow users to request member access. | +| `default_branch_protection` | integer | no | Determine if developers can push to master branch of projects in the group. Can take: `0` _(not protected, both developers and maintainers can push new commits, force push, or delete the branch)_, `1` _(partially protected, developers and maintainers can push new commits, but cannot force push or delete the branch)_ or `2` _(fully protected, developers cannot push new commits, but maintainers can; no-one can force push or delete the branch)_ as a parameter. | | `file_template_project_id` | integer | no | **(PREMIUM)** The ID of a project to load custom file templates from. | | `shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Pipeline minutes quota for this group. | | `extra_shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Extra pipeline minutes quota for this group. | diff --git a/doc/user/admin_area/settings/visibility_and_access_controls.md b/doc/user/admin_area/settings/visibility_and_access_controls.md index 8f64e9207b5224..704dd89ede2553 100644 --- a/doc/user/admin_area/settings/visibility_and_access_controls.md +++ b/doc/user/admin_area/settings/visibility_and_access_controls.md @@ -26,6 +26,8 @@ To change the default branch protection: For more details, see [Protected branches](../../project/protected_branches.md). +To change this setting for a specific group, see [Default branch protection for groups](../../group/index.md#changing-the-default-branch-protection-of-a-group) + ## Default project creation protection Project creation protection specifies which roles can create projects. diff --git a/doc/user/group/index.md b/doc/user/group/index.md index 6b76e070c41810..34e7be9fef1bce 100644 --- a/doc/user/group/index.md +++ b/doc/user/group/index.md @@ -181,6 +181,21 @@ of a group: 1. Give a different member **Owner** permissions. 1. Have the new owner sign in and remove **Owner** permissions from you. +## Changing the default branch protection of a group + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/7583) in GitLab 12.9. + +By default, every group inherits the default branch protection set at the global level. + +To change this setting for a specific group: + +1. Go to the group's **{settings}** **Settings > General** page. +1. Expand the **Permissions, LFS, 2FA** section. +1. Select the desired option in the **Default branch protection** dropdown list. +1. Click **Save changes**. + +To change this setting globally, see [Default branch protection](../admin_area/settings/visibility_and_access_controls.md#default-branch-protection). + ## Add projects to a group There are two different ways to add a new project to a group: -- GitLab From d67c941b4674958e1a110e4a3257d06d654c9c2b Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 28 Feb 2020 09:53:43 +0530 Subject: [PATCH 5/7] Change some docs --- doc/api/groups.md | 15 +++++++++++++-- doc/user/group/index.md | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/doc/api/groups.md b/doc/api/groups.md index c34a0dada1e701..21fe0713fe24cb 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -496,10 +496,21 @@ Parameters: | `lfs_enabled` | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group. | | `request_access_enabled` | boolean | no | Allow users to request member access. | | `parent_id` | integer | no | The parent group ID for creating nested group. | -| `default_branch_protection` | integer | no | Determine if developers can push to master branch of projects in the group. Can take: `0` _(not protected, both developers and maintainers can push new commits, force push, or delete the branch)_, `1` _(partially protected, developers and maintainers can push new commits, but cannot force push or delete the branch)_ or `2` _(fully protected, developers cannot push new commits, but maintainers can; no-one can force push or delete the branch)_ as a parameter. Default to the global level default branch protection setting. | +| `default_branch_protection` | integer | no | See [Options for `default_branch_protection`](#options-for-default_branch_protection). Default to the global level default branch protection setting. | | `shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Pipeline minutes quota for this group. | | `extra_shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Extra pipeline minutes quota for this group. | +### 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: + +| Value | Description | +|-------|-------------------------------------------------------------------------------------------------------------| +| `0` | No protection. Developers and maintainers can:
- Push new commits
- Force push changes
- Delete the branch | +| `1` | Partial protection. Developers and maintainers can:
- Push new commits | +| `2` | Full protection. Only maintainers can:
- Push new commits | + ## Transfer project to group Transfer a project to the Group namespace. Available only to instance administrators, although an [alternative API endpoint](projects.md#transfer-a-project-to-a-new-namespace) is available which does not require instance administrator access. Transferring projects may fail when tagged packages exist in the project's repository. @@ -546,7 +557,7 @@ PUT /groups/:id | `mentions_disabled` | boolean | no | Disable the capability of a group from getting mentioned | | `lfs_enabled` (optional) | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group. | | `request_access_enabled` | boolean | no | Allow users to request member access. | -| `default_branch_protection` | integer | no | Determine if developers can push to master branch of projects in the group. Can take: `0` _(not protected, both developers and maintainers can push new commits, force push, or delete the branch)_, `1` _(partially protected, developers and maintainers can push new commits, but cannot force push or delete the branch)_ or `2` _(fully protected, developers cannot push new commits, but maintainers can; no-one can force push or delete the branch)_ as a parameter. | +| `default_branch_protection` | integer | no | See [Options for `default_branch_protection`](#options-for-default_branch_protection). | | `file_template_project_id` | integer | no | **(PREMIUM)** The ID of a project to load custom file templates from. | | `shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Pipeline minutes quota for this group. | | `extra_shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Extra pipeline minutes quota for this group. | diff --git a/doc/user/group/index.md b/doc/user/group/index.md index 34e7be9fef1bce..cca82f6a4fb3ce 100644 --- a/doc/user/group/index.md +++ b/doc/user/group/index.md @@ -185,7 +185,7 @@ of a group: > [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/7583) in GitLab 12.9. -By default, every group inherits the default branch protection set at the global level. +By default, every group inherits the branch protection set at the global level. To change this setting for a specific group: -- GitLab From b8680b25f3c0c23f855a5a826c5c7452aa6c74e6 Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 28 Feb 2020 10:04:56 +0530 Subject: [PATCH 6/7] Add spec changes --- spec/models/namespace_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 26a2e9bd342ed3..276fbc2cb54c8a 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -533,7 +533,8 @@ def project_rugged(project) describe "#default_branch_protection" do let(:namespace) { create(:namespace) } - let(:group) { create(:group) } + let(:default_branch_protection) { nil } + let(:group) { create(:group, default_branch_protection: default_branch_protection) } before do stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_MERGE) @@ -541,8 +542,7 @@ def project_rugged(project) context 'for a namespace' do # Unlike a group, the settings of a namespace cannot be altered - # via the UI or the API, so the value of `default_branch_protection` - # for a namespace will always be `nil` in the database. + # via the UI or the API. it 'returns the instance level setting' do expect(namespace.default_branch_protection).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) @@ -558,7 +558,6 @@ def project_rugged(project) context 'that has altered the default value' do let(:default_branch_protection) { Gitlab::Access::PROTECTION_FULL } - let(:group) { create(:group, default_branch_protection: default_branch_protection) } it 'returns the group level setting' do expect(group.default_branch_protection).to eq(default_branch_protection) -- GitLab From 0bd096924c4280c591e94d8b25b708d20bdceaaa Mon Sep 17 00:00:00 2001 From: manojmj Date: Fri, 28 Feb 2020 11:46:18 +0530 Subject: [PATCH 7/7] Fix some documentation --- doc/api/groups.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/doc/api/groups.md b/doc/api/groups.md index 21fe0713fe24cb..e48cb78b2cc904 100644 --- a/doc/api/groups.md +++ b/doc/api/groups.md @@ -501,10 +501,9 @@ Parameters: | `extra_shared_runners_minutes_limit` | integer | no | **(STARTER ONLY)** Extra pipeline minutes quota for this group. | ### 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: - + +The `default_branch_protection` attribute determines whether developers and maintainers can push to the applicable master branch, as described in the following table: + | Value | Description | |-------|-------------------------------------------------------------------------------------------------------------| | `0` | No protection. Developers and maintainers can:
- Push new commits
- Force push changes
- Delete the branch | -- GitLab