From d6dd9d712ac24a095d0b0506731f9415b7c3b5f5 Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Fri, 24 Nov 2017 12:41:36 +0000
Subject: Fix ProtectedBranch access level validations

Before an access_level was required in EE even when an
it had been set for a user/group.
---
 app/models/concerns/protected_branch_access.rb  | 10 ----------
 app/models/concerns/protected_ref_access.rb     | 16 ++++++++++++++++
 app/models/protected_tag/create_access_level.rb |  4 ----
 doc/api/protected_branches.md                   |  2 +-
 lib/api/protected_branches.rb                   |  4 ++--
 5 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb
index fde1cc44afa..77307e92f22 100644
--- a/app/models/concerns/protected_branch_access.rb
+++ b/app/models/concerns/protected_branch_access.rb
@@ -1,12 +1,6 @@
 module ProtectedBranchAccess
   extend ActiveSupport::Concern
 
-  ALLOWED_ACCESS_LEVELS ||= [
-    Gitlab::Access::MASTER,
-    Gitlab::Access::DEVELOPER,
-    Gitlab::Access::NO_ACCESS
-  ].freeze
-
   included do
     include ProtectedRefAccess
 
@@ -14,10 +8,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",
diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb
index c4f158e569a..665c41c825e 100644
--- a/app/models/concerns/protected_ref_access.rb
+++ b/app/models/concerns/protected_ref_access.rb
@@ -1,15 +1,31 @@
 module ProtectedRefAccess
   extend ActiveSupport::Concern
 
+  ALLOWED_ACCESS_LEVELS = [
+    Gitlab::Access::MASTER,
+    Gitlab::Access::DEVELOPER,
+    Gitlab::Access::NO_ACCESS
+  ].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]
   end
 
+  # CE access levels are always role-based,
+  # where as EE allows groups and users too
+  def role?
+    true
+  end
+
   def check_access(user)
     return true if user.admin?
 
diff --git a/app/models/protected_tag/create_access_level.rb b/app/models/protected_tag/create_access_level.rb
index c7e1319719d..d1e81158351 100644
--- a/app/models/protected_tag/create_access_level.rb
+++ b/app/models/protected_tag/create_access_level.rb
@@ -1,10 +1,6 @@
 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",
diff --git a/doc/api/protected_branches.md b/doc/api/protected_branches.md
index 10faa95d7e8..81fe854060a 100644
--- a/doc/api/protected_branches.md
+++ b/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
diff --git a/lib/api/protected_branches.rb b/lib/api/protected_branches.rb
index 15fcb9e8e27..b5021e8a712 100644
--- a/lib/api/protected_branches.rb
+++ b/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
-- 
cgit v1.2.1