Commit 6b006eba authored by Imre Farkas's avatar Imre Farkas

Merge branch '7583-developer-cannot-push-to-projects-they-create-in-groups' into 'master'

Resolve "Developer cannot push to projects they create in groups" by allowing to set "Default branch protection" at the group level

Closes #7583

See merge request gitlab-org/gitlab!24426
parents 796cd50d f4463659
......@@ -195,7 +195,8 @@ class GroupsController < Groups::ApplicationController
:require_two_factor_authentication,
:two_factor_grace_period,
:project_creation_level,
:subgroup_creation_level
:subgroup_creation_level,
:default_branch_protection
]
end
......
......@@ -139,6 +139,10 @@ class Namespace < ApplicationRecord
end
end
def default_branch_protection
super || Gitlab::CurrentSettings.default_branch_protection
end
def visibility_level_field
:visibility_level
end
......
......@@ -2359,6 +2359,12 @@ class Project < ApplicationRecord
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)
......
......@@ -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 @@ class ProtectedBranch < ApplicationRecord
# 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 @@ class ProtectedBranch < ApplicationRecord
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
......
......@@ -11,7 +11,7 @@ module Projects
@project = project
@default_branch_protection = Gitlab::Access::BranchProtection
.new(Gitlab::CurrentSettings.default_branch_protection)
.new(project.namespace.default_branch_protection)
end
def execute
......
......@@ -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'
......
......@@ -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
......
.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'
---
title: Introduce default branch protection at the group level
merge_request: 24426
author:
type: added
# frozen_string_literal: true
class AddDefaultBranchProtectionToNamespaces < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
with_lock_retries do
add_column :namespaces, :default_branch_protection, :integer, limit: 2
end
end
end
......@@ -2762,6 +2762,7 @@ ActiveRecord::Schema.define(version: 2020_02_26_162723) do
t.integer "max_pages_size"
t.integer "max_artifacts_size"
t.boolean "mentions_disabled"
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"
......
......@@ -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,9 +496,20 @@ 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 | 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: <br>- Push new commits<br>- Force push changes<br>- Delete the branch |
| `1` | Partial protection. Developers and maintainers can: <br>- Push new commits |
| `2` | Full protection. Only maintainers can: <br>- 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.
......@@ -542,6 +556,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 | 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. |
......
......@@ -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.
......
......@@ -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 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:
......
......@@ -13,6 +13,7 @@ module API
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
......
......@@ -21,6 +21,7 @@ module API
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
......
......@@ -37,6 +37,10 @@ module Gitlab
def developer_can_merge?
level == PROTECTION_DEV_CAN_MERGE
end
def fully_protected?
level == PROTECTION_FULL
end
end
end
end
......@@ -411,6 +411,13 @@ describe GroupsController do
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)
......
......@@ -51,4 +51,21 @@ describe Gitlab::Access::BranchProtection do
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
......@@ -46,32 +46,27 @@ describe Gitlab::UserAccess do
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
......
......@@ -531,6 +531,41 @@ describe Namespace do
end
end
describe "#default_branch_protection" do
let(:namespace) { create(:namespace) }
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)
end
context 'for a namespace' do
# Unlike a group, the settings of a namespace cannot be altered
# 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)
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
let(:default_branch_protection) { Gitlab::Access::PROTECTION_FULL }
it 'returns the group level setting' do
expect(group.default_branch_protection).to eq(default_branch_protection)
end
end
end
end
describe '#self_and_hierarchy' do
let!(:group) { create(:group, path: 'git_lab') }
let!(:nested_group) { create(:group, parent: group) }
......
......@@ -1623,6 +1623,29 @@ describe Project do
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 @@ describe Project do
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 @@ describe Project do
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 @@ describe Project do
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
......
......@@ -164,31 +164,45 @@ describe ProtectedBranch do
end
end
context "new project" do
let(:project) { create(:project) }
context 'new project' do
using RSpec::Parameterized::TableSyntax
it 'returns false when default_protected_branch is unprotected' do
stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_NONE)
let(:project) { create(:project) }
expect(described_class.protected?(project, 'master')).to be false
end
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
it 'returns false when default_protected_branch lets developers push' do
stub_application_setting(default_branch_protection: Gitlab::Access::PROTECTION_DEV_CAN_PUSH)
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 be false
expect(described_class.protected?(project, 'master')).to eq(result)
end
end
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)
expect(described_class.protected?(project, 'master')).to be true
end
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
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
before do
stub_application_setting(default_branch_protection: default_branch_protection_level)
end
expect(described_class.protected?(project, 'master')).to be true
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
end
......
......@@ -545,7 +545,8 @@ describe API::Groups do
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 @@ describe API::Groups do
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
......
......@@ -186,7 +186,7 @@ describe Git::BranchPushService, services: true do
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 @@ describe Git::BranchPushService, services: true do
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 @@ describe Git::BranchPushService, services: true do
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 @@ describe Git::BranchPushService, services: true do
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")
......
......@@ -4,7 +4,7 @@ require 'spec_helper'
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 Projects::ProtectDefaultBranchService do
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 @@ describe Projects::ProtectDefaultBranchService do
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 Projects::ProtectDefaultBranchService do
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 @@ describe Projects::ProtectDefaultBranchService do
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 Projects::ProtectDefaultBranchService do
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 @@ describe Projects::ProtectDefaultBranchService do
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)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment