Merge branch 'jej/fix-protected-branch-validations' into 'master'

Fix ProtectedBranch access level validations

See merge request gitlab-org/gitlab-ee!3542

app/controllers/projects/settings/repository_controller.rb

@@ -25,14 +25,14 @@ module Projects
 
       def access_levels_options
         {
-          create_access_levels: levels_for_dropdown(ProtectedTag::CreateAccessLevel),
-          push_access_levels: levels_for_dropdown(ProtectedBranch::PushAccessLevel),
-          merge_access_levels: levels_for_dropdown(ProtectedBranch::MergeAccessLevel)
+          create_access_levels: levels_for_dropdown,
+          push_access_levels: levels_for_dropdown,
+          merge_access_levels: levels_for_dropdown
         }
       end
 
-      def levels_for_dropdown(access_level_type)
-        roles = access_level_type.human_access_levels.map do |id, text|
+      def levels_for_dropdown
+        roles = ProtectedRefAccess::HUMAN_ACCESS_LEVELS.map do |id, text|
           { id: id, text: text, before_divider: true }
         end
         { roles: roles }

app/models/concerns/protected_branch_access.rb

 module ProtectedBranchAccess
   extend ActiveSupport::Concern
 
-  ALLOWED_ACCESS_LEVELS ||= [
-    Gitlab::Access::MASTER,
-    Gitlab::Access::DEVELOPER,
-    Gitlab::Access::NO_ACCESS
-  ].freeze
-
   included do
     include ProtectedRefAccess
     include EE::ProtectedRefAccess
@@ -15,18 +9,6 @@ module ProtectedBranchAccess
 
     delegate :project, to: :protected_branch
 
-    validates :access_level, presence: true, inclusion: {
-      in: ALLOWED_ACCESS_LEVELS
-    }
-
-    def self.human_access_levels
-      {
-        Gitlab::Access::MASTER => "Masters",
-        Gitlab::Access::DEVELOPER => "Developers + Masters",
-        Gitlab::Access::NO_ACCESS => "No one"
-      }.with_indifferent_access
-    end
-
     def check_access(user)
       return false if access_level == Gitlab::Access::NO_ACCESS
 

app/models/concerns/protected_ref_access.rb

 module ProtectedRefAccess
   extend ActiveSupport::Concern
 
+  ALLOWED_ACCESS_LEVELS = [
+    Gitlab::Access::MASTER,
+    Gitlab::Access::DEVELOPER,
+    Gitlab::Access::NO_ACCESS
+  ].freeze
+
+  HUMAN_ACCESS_LEVELS = {
+    Gitlab::Access::MASTER => "Masters".freeze,
+    Gitlab::Access::DEVELOPER => "Developers + Masters".freeze,
+    Gitlab::Access::NO_ACCESS => "No one".freeze
+  }.freeze
+
   included do
     scope :master, -> { where(access_level: Gitlab::Access::MASTER) }
     scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) }
+
+    validates :access_level, presence: true, if: :role?, inclusion: {
+      in: ALLOWED_ACCESS_LEVELS
+    }
   end
 
   def humanize
-    self.class.human_access_levels[self.access_level]
+    HUMAN_ACCESS_LEVELS[self.access_level]
+  end
+
+  # CE access levels are always role-based,
+  # where as EE allows groups and users too
+  def role?
+    true
   end
 
   def check_access(user)

app/models/protected_tag/create_access_level.rb

 class ProtectedTag::CreateAccessLevel < ActiveRecord::Base
   include ProtectedTagAccess
 
-  validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
-                                                             Gitlab::Access::DEVELOPER,
-                                                             Gitlab::Access::NO_ACCESS] }
-
-  def self.human_access_levels
-    {
-      Gitlab::Access::MASTER => "Masters",
-      Gitlab::Access::DEVELOPER => "Developers + Masters",
-      Gitlab::Access::NO_ACCESS => "No one"
-    }.with_indifferent_access
-  end
-
   def check_access(user)
     return false if access_level == Gitlab::Access::NO_ACCESS
 

doc/api/protected_branches.md

@@ -4,7 +4,7 @@
 
 **Valid access levels**
 
-The access levels are defined in the `ProtectedBranchAccess::ALLOWED_ACCESS_LEVELS` constant. Currently, these levels are recognized:
+The access levels are defined in the `ProtectedRefAccess::ALLOWED_ACCESS_LEVELS` constant. Currently, these levels are recognized:
 ```
 0  => No access
 30 => Developer access

ee/app/models/concerns/ee/protected_ref_access.rb

@@ -41,6 +41,8 @@ module EE
 
     # Is this a role-based access level?
     def role?
+      raise NotImplementedError unless defined?(super)
+
       type == :role
     end
 

lib/api/protected_branches.rb

@@ -40,10 +40,10 @@ module API
       params do
         requires :name, type: String, desc: 'The name of the protected branch'
         optional :push_access_level, type: Integer, default: Gitlab::Access::MASTER,
-                                     values: ProtectedBranchAccess::ALLOWED_ACCESS_LEVELS,
+                                     values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS,
                                      desc: 'Access levels allowed to push (defaults: `40`, master access level)'
         optional :merge_access_level, type: Integer, default: Gitlab::Access::MASTER,
-                                      values: ProtectedBranchAccess::ALLOWED_ACCESS_LEVELS,
+                                      values: ProtectedRefAccess::ALLOWED_ACCESS_LEVELS,
                                       desc: 'Access levels allowed to merge (defaults: `40`, master access level)'
       end
       post ':id/protected_branches' do

spec/ee/spec/models/ee/protected_ref_access_spec.rb

@@ -7,10 +7,8 @@ describe EE::ProtectedRefAccess do
 
   included_in_classes.each do |included_in_class|
     context "in #{included_in_class}" do
-      let(:access_level) do
-        factory_name = included_in_class.name.underscore.tr('/', '_')
-        build(factory_name)
-      end
+      let(:factory_name) { included_in_class.name.underscore.tr('/', '_') }
+      let(:access_level) { build(factory_name) }
 
       it "#{included_in_class} includes {described_class}" do
         expect(included_in_class.included_modules).to include(described_class)
@@ -53,6 +51,24 @@ describe EE::ProtectedRefAccess do
           expect(access_level).to be_valid
         end
       end
+
+      it 'requires access_level if no user or group is specified' do
+        subject = build(factory_name, access_level: nil)
+
+        expect(subject).not_to be_valid
+      end
+
+      it "doesn't require access_level if user specified" do
+        subject = build(factory_name, access_level: nil, user: create(:user))
+
+        expect(subject).to be_valid
+      end
+
+      it "doesn't require access_level if group specified" do
+        subject = build(factory_name, access_level: nil, group: create(:group))
+
+        expect(subject).to be_valid
+      end
     end
   end
 end

spec/ee/support/protected_tags/access_control_spec.rb

 RSpec.shared_examples "protected tags > access control > EE" do
   let(:users) { create_list(:user, 5) }
   let(:groups) { create_list(:group, 5) }
-  let(:roles) { ProtectedTag::CreateAccessLevel.human_access_levels.except(0) }
+  let(:roles) { ProtectedRefAccess::HUMAN_ACCESS_LEVELS.except(0) }
 
   before do
     users.each { |user| project.team << [user, :developer] }
@@ -146,7 +146,7 @@ RSpec.shared_examples "protected tags > access control > EE" do
       visit project_protected_tags_path(project)
 
       set_protected_tag_name('v1.0')
-      set_allowed_to('create', ProtectedTag::CreateAccessLevel.human_access_levels.values) # Last item (No one) should deselect the other ones
+      set_allowed_to('create', ProtectedRefAccess::HUMAN_ACCESS_LEVELS.values) # Last item (No one) should deselect the other ones
 
       click_on "Protect"
 

spec/ee/support/shared_examples/features/protected_branches_access_control.rb

 shared_examples "protected branches > access control > EE" do
-  [['merge', ProtectedBranch::MergeAccessLevel], ['push', ProtectedBranch::PushAccessLevel]].each do |git_operation, access_level_class|
+  %w[merge push].each do |git_operation|
     # Need to set a default for the `git_operation` access level that _isn't_ being tested
     other_git_operation = git_operation == 'merge' ? 'push' : 'merge'
-    roles_except_noone = access_level_class.human_access_levels.except(0)
+    roles_except_noone = ProtectedRefAccess::HUMAN_ACCESS_LEVELS.except(0)
 
     let(:users) { create_list(:user, 5) }
     let(:groups) { create_list(:group, 5) }
@@ -124,7 +124,7 @@ shared_examples "protected branches > access control > EE" do
 
   context 'When updating a protected branch' do
     it 'discards other roles when choosing "No one"' do
-      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      roles = ProtectedRefAccess::HUMAN_ACCESS_LEVELS.except(0)
       visit project_protected_branches_path(project)
       set_protected_branch_name('fix')
       set_allowed_to('merge')
@@ -151,11 +151,11 @@ shared_examples "protected branches > access control > EE" do
 
   context 'When creating a protected branch' do
     it 'discards other roles when choosing "No one"' do
-      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      roles = ProtectedRefAccess::HUMAN_ACCESS_LEVELS.except(0)
       visit project_protected_branches_path(project)
       set_protected_branch_name('master')
       set_allowed_to('merge')
-      set_allowed_to('push', ProtectedBranch::PushAccessLevel.human_access_levels.values) # Last item (No one) should deselect the other ones
+      set_allowed_to('push', ProtectedRefAccess::HUMAN_ACCESS_LEVELS.values) # Last item (No one) should deselect the other ones
       click_on "Protect"
       wait_for_requests
 

spec/support/protected_tags/access_control_ce_shared_examples.rb

 RSpec.shared_examples "protected tags > access control > CE" do
-  ProtectedTag::CreateAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
+  ProtectedRefAccess::HUMAN_ACCESS_LEVELS.each do |(access_type_id, access_type_name)|
     it "allows creating protected tags that #{access_type_name} can create" do
       visit project_protected_tags_path(project)
 

spec/support/shared_examples/features/protected_branches_access_control_ce.rb

 shared_examples "protected branches > access control > CE" do
-  ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
+  ProtectedRefAccess::HUMAN_ACCESS_LEVELS.each do |(access_type_id, access_type_name)|
     it "allows creating protected branches that #{access_type_name} can push to" do
       visit project_protected_branches_path(project)
 
@@ -44,7 +44,7 @@ shared_examples "protected branches > access control > CE" do
     end
   end
 
-  ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
+  ProtectedRefAccess::HUMAN_ACCESS_LEVELS.each do |(access_type_id, access_type_name)|
     it "allows creating protected branches that #{access_type_name} can merge to" do
       visit project_protected_branches_path(project)