Commit d78ab154 authored by Timothy Andrew's avatar Timothy Andrew Committed by Alfredo Sumaran

Implement model-level changes to allow branch protection for specific users.

- A protected branch now `has_many` access levels (as opposed to
  `has_one`, which CE will continue to have). Each access level
  represents either a user or a role that is allowed to access the
  protected branch.

- Update `git_access_spec` to test cases where a specific user is given
  access to a protected branch.

- Remove the explicit `push_check` for protected branches. This is
  because a non-developer user (such as a reporter) should be able to
  push code if they are added to a protected branch specifically.

- Fix specs (git_access_spec) that were implicitly depending on a
  master push access level being created (previously, a protected
  branch's default state was "masters can push + masters can merge").
  Since the current default is "none", the dependency needs to be
  explicitly declared.

- Update `git_push_service` so default branch protection (for new repos)
  works as expected.
parent 7096af19
module ProtectedBranchAccess
extend ActiveSupport::Concern
def type
if self.user.present?
:user
else
:role
end
end
def humanize
return self.user.name if self.user.present?
self.class.human_access_levels[self.access_level]
end
end
......@@ -8,8 +8,8 @@ class ProtectedBranch < ActiveRecord::Base
has_many :merge_access_levels, dependent: :destroy
has_many :push_access_levels, dependent: :destroy
validates_length_of :merge_access_levels, is: 1, message: "are restricted to a single instance per protected branch."
validates_length_of :push_access_levels, is: 1, message: "are restricted to a single instance per protected branch."
validates_length_of :merge_access_levels, minimum: 0
validates_length_of :push_access_levels, minimum: 0
accepts_nested_attributes_for :push_access_levels
accepts_nested_attributes_for :merge_access_levels
......@@ -46,6 +46,24 @@ class ProtectedBranch < ActiveRecord::Base
self.name && self.name.include?('*')
end
# Returns a hash were keys are types of push access levels (user, role), and
# values are the number of access levels of the particular type.
def push_access_level_frequencies
push_access_levels.reduce(Hash.new(0)) do |frequencies, access_level|
frequencies[access_level.type] = frequencies[access_level.type] + 1
frequencies
end
end
# Returns a hash were keys are types of merge access levels (user, role), and
# values are the number of access levels of the particular type.
def merge_access_level_frequencies
merge_access_levels.reduce(Hash.new(0)) do |frequencies, access_level|
frequencies[access_level.type] = frequencies[access_level.type] + 1
frequencies
end
end
protected
def exact_match?(branch_name)
......
......@@ -2,6 +2,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
include ProtectedBranchAccess
belongs_to :protected_branch
belongs_to :user
delegate :project, to: :protected_branch
validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
......@@ -16,6 +17,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
def check_access(user)
return true if user.is_admin?
return user.id == self.user_id if self.user.present?
project.team.max_member_access(user.id) >= access_level
end
......
......@@ -2,6 +2,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
include ProtectedBranchAccess
belongs_to :protected_branch
belongs_to :user
delegate :project, to: :protected_branch
validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
......@@ -19,6 +20,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
def check_access(user)
return false if access_level == Gitlab::Access::NO_ACCESS
return true if user.is_admin?
return user.id == self.user_id if self.user.present?
project.team.max_member_access(user.id) >= access_level
end
......
......@@ -93,6 +93,10 @@ class User < ActiveRecord::Base
has_many :award_emoji, dependent: :destroy
has_many :path_locks, dependent: :destroy
# Protected Branch Access
has_many :protected_branch_merge_access_levels, dependent: :destroy, class_name: ProtectedBranch::MergeAccessLevel
has_many :protected_branch_push_access_levels, dependent: :destroy, class_name: ProtectedBranch::PushAccessLevel
#
# Validations
#
......
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddColumnUserIdToProtectedBranchesAccessLevels < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def change
add_reference :protected_branch_merge_access_levels, :user, foreign_key: true, index: true
add_reference :protected_branch_push_access_levels, :user, foreign_key: true, index: true
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AllowNullsForProtectedBranchAccessLevels < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def change
change_column :protected_branch_merge_access_levels, :access_level, :integer, null: true
change_column :protected_branch_push_access_levels, :access_level, :integer, null: true
end
end
......@@ -953,18 +953,22 @@ ActiveRecord::Schema.define(version: 20160810153405) do
t.integer "access_level", default: 40, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "user_id"
end
add_index "protected_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree
add_index "protected_branch_merge_access_levels", ["user_id"], name: "index_protected_branch_merge_access_levels_on_user_id", using: :btree
create_table "protected_branch_push_access_levels", force: :cascade do |t|
t.integer "protected_branch_id", null: false
t.integer "access_level", default: 40, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "user_id"
end
add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
add_index "protected_branch_push_access_levels", ["user_id"], name: "index_protected_branch_push_access_levels_on_user_id", using: :btree
create_table "protected_branches", force: :cascade do |t|
t.integer "project_id", null: false
......@@ -1278,7 +1282,9 @@ ActiveRecord::Schema.define(version: 20160810153405) do
add_foreign_key "path_locks", "users"
add_foreign_key "personal_access_tokens", "users"
add_foreign_key "protected_branch_merge_access_levels", "protected_branches"
add_foreign_key "protected_branch_merge_access_levels", "users"
add_foreign_key "protected_branch_push_access_levels", "protected_branches"
add_foreign_key "remote_mirrors", "projects"
add_foreign_key "protected_branch_push_access_levels", "users"
add_foreign_key "u2f_registrations", "users"
end
......@@ -8,22 +8,38 @@ FactoryGirl.define do
protected_branch.merge_access_levels.new(access_level: Gitlab::Access::MASTER)
end
transient do
authorize_user_to_push nil
authorize_user_to_merge nil
end
trait :developers_can_push do
after(:create) do |protected_branch|
protected_branch.push_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER)
protected_branch.push_access_levels.create!(access_level: Gitlab::Access::DEVELOPER)
end
end
trait :developers_can_merge do
after(:create) do |protected_branch|
protected_branch.merge_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER)
protected_branch.merge_access_levels.create!(access_level: Gitlab::Access::DEVELOPER)
end
end
trait :no_one_can_push do
after(:create) do |protected_branch|
protected_branch.push_access_levels.first.update!(access_level: Gitlab::Access::NO_ACCESS)
protected_branch.push_access_levels.create!(access_level: Gitlab::Access::NO_ACCESS)
end
end
trait :masters_can_push do
after(:create) do |protected_branch|
protected_branch.push_access_levels.create!(access_level: Gitlab::Access::MASTER)
end
end
after(:create) do |protected_branch, evaluator|
protected_branch.push_access_levels.create!(user: evaluator.authorize_user_to_push) if evaluator.authorize_user_to_push
protected_branch.merge_access_levels.create!(user: evaluator.authorize_user_to_merge) if evaluator.authorize_user_to_merge
end
end
end
......@@ -245,19 +245,19 @@ describe Gitlab::GitAccess, lib: true do
[['feature', 'exact'], ['feat*', 'wildcard']].each do |protected_branch_name, protected_branch_type|
context do
before { create(:protected_branch, name: protected_branch_name, project: project) }
before { create(:protected_branch, :masters_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix)
end
context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
before { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
before { create(:protected_branch, :masters_can_push, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
before { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
before { create(:protected_branch, :masters_can_push, :developers_can_merge, name: protected_branch_name, project: project) }
context "when a merge request exists for the given source/target branch" do
context "when the merge request is in progress" do
......@@ -284,11 +284,51 @@ describe Gitlab::GitAccess, lib: true do
end
context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
before { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
before { create(:protected_branch, :masters_can_push, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
end
context "when a specific user is allowed to push into the #{protected_branch_type} protected branch" do
let(:user) { create(:user) }
before do
create(:protected_branch, authorize_user_to_push: user, name: protected_branch_name, project: project)
end
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
guest: { push_protected_branch: false, merge_into_protected_branch: false },
reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
end
context "when a specific user is allowed to merge into the #{protected_branch_type} protected branch" do
let(:user) { create(:user) }
before do
create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
create(:protected_branch, authorize_user_to_merge: user, name: protected_branch_name, project: project)
end
run_permission_checks(permissions_matrix.deep_merge(admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: true },
guest: { push_protected_branch: false, merge_into_protected_branch: false },
reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
end
context "when a specific user is allowed to push & merge into the #{protected_branch_type} protected branch" do
let(:user) { create(:user) }
before do
create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
create(:protected_branch, authorize_user_to_push: user, authorize_user_to_merge: user, name: protected_branch_name, project: project)
end
run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true },
guest: { push_protected_branch: false, merge_into_protected_branch: false },
reporter: { push_protected_branch: false, merge_into_protected_branch: false }))
end
context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
before { create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
......
......@@ -68,10 +68,10 @@ describe Gitlab::UserAccess, lib: true do
describe 'push to protected branch' do
let(:branch) { create :protected_branch, project: project }
it 'returns true if user is a master' do
it 'returns false if user is a master' do
project.team << [user, :master]
expect(access.can_push_to_branch?(branch.name)).to be_truthy
expect(access.can_push_to_branch?(branch.name)).to be_falsy
end
it 'returns false if user is a developer' do
......
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