Security for online editor. Replace dev_access?, master_access? with can? method usage

7 files changed, 56 insertions, 18 deletions

diff --git a/app/controllers/tree_controller.rb b/app/controllers/tree_controller.rb
index c9098cca678..e507fb51975 100644
--- a/app/controllers/tree_controller.rb
+++ b/app/controllers/tree_controller.rb
@@ -48,5 +48,13 @@ class TreeController < ProjectResourceController
     unless @tree.is_blob? && @tree.text?
       redirect_to project_tree_path(@project, @id), notice: "You can only edit text files"
     end
+
+    allowed = if project.protected_branch? @ref
+                can?(current_user, :push_code_to_protected_branches, project)
+              else
+                can?(current_user, :push_code, project)
+              end
+
+    return access_denied! unless allowed
   end
 end
diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb
index c681dc607a0..4fe87a25554 100644
--- a/app/helpers/tree_helper.rb
+++ b/app/helpers/tree_helper.rb
@@ -59,4 +59,12 @@ module TreeHelper
   def tree_join(*args)
     File.join(*args)
   end
+
+  def allowed_tree_edit?
+    if @project.protected_branch? @ref
+      can?(current_user, :push_code_to_protected_branches, @project)
+    else
+      can?(current_user, :push_code, @project)
+    end
+  end
 end
diff --git a/app/models/ability.rb b/app/models/ability.rb
index d3180b76e7c..c3a212f473d 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -35,10 +35,15 @@ class Ability
       ] if project.report_access_for?(user)
 
       rules << [
-        :write_wiki
+        :write_wiki,
+        :push_code
       ] if project.dev_access_for?(user)
 
       rules << [
+        :push_code_to_protected_branches
+      ] if project.master_access_for?(user)
+
+      rules << [
         :modify_issue,
         :modify_snippet,
         :modify_merge_request,
diff --git a/app/roles/authority.rb b/app/roles/authority.rb
index dbdd98397b9..e0796d5f120 100644
--- a/app/roles/authority.rb
+++ b/app/roles/authority.rb
@@ -53,6 +53,6 @@ module Authority
   end
 
   def master_access_for?(user)
-    !users_projects.where(user_id: user.id, project_access: [UsersProject::MASTER]).empty? || owner_id == user.id
+    !users_projects.where(user_id: user.id, project_access: [UsersProject::MASTER]).empty?
   end
 end
diff --git a/app/roles/repository.rb b/app/roles/repository.rb
index 88fd90d061f..882ec31033c 100644
--- a/app/roles/repository.rb
+++ b/app/roles/repository.rb
@@ -181,4 +181,9 @@ module Repository
   def http_url_to_repo
     http_url = [Gitlab.config.url, "/", path, ".git"].join('')
   end
+
+  # Check if current branch name is marked as protected in the system
+  def protected_branch? branch_name
+    protected_branches.map(&:name).include?(branch_name)
+  end
 end
diff --git a/app/views/tree/_blob_actions.html.haml b/app/views/tree/_blob_actions.html.haml
index d51f7205ef6..21334ea1f16 100644
--- a/app/views/tree/_blob_actions.html.haml
+++ b/app/views/tree/_blob_actions.html.haml
@@ -1,7 +1,7 @@
 .btn-group.tree-btn-group
   -# only show edit link for text files
   - if @tree.text?
-    = link_to "edit", edit_project_tree_path(@project, @id), class: "btn very_small"
+    = link_to "edit", edit_project_tree_path(@project, @id), class: "btn very_small", disabled: !allowed_tree_edit?
   = link_to "raw", project_blob_path(@project, @id), class: "btn very_small", target: "_blank"
   -# only show normal/blame view links for text files
   - if @tree.text?
diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb
index 43a75cc377b..05329baf0b1 100644
--- a/lib/gitlab/backend/grack_auth.rb
+++ b/lib/gitlab/backend/grack_auth.rb
@@ -1,10 +1,11 @@
 module Grack
   class Auth < Rack::Auth::Basic
+    attr_accessor :user, :project
 
     def valid?
       # Authentication with username and password
       email, password = @auth.credentials
-      user = User.find_by_email(email)
+      self.user = User.find_by_email(email)
       return false unless user.try(:valid_password?, password)
 
       # Set GL_USER env variable
@@ -18,28 +19,39 @@ module Grack
 
       # Find project by PATH_INFO from env
       if m = /^\/([\w-]+).git/.match(@request.path_info).to_a
-        return false unless project = Project.find_by_path(m.last)
+        self.project = Project.find_by_path(m.last)
+        return false unless project
       end
 
       # Git upload and receive
       if @request.get?
-        true
+        validate_get_request
       elsif @request.post?
-        if @request.path_info.end_with?('git-upload-pack')
-          return project.dev_access_for?(user)
-        elsif @request.path_info.end_with?('git-receive-pack')
-          if project.protected_branches.map(&:name).include?(current_ref)
-            project.master_access_for?(user)
-          else
-            project.dev_access_for?(user)
-          end
-        else
-          false
-        end
+        validate_post_request
       else
         false
       end
-    end# valid?
+    end
+
+    def validate_get_request
+      true
+    end
+
+    def validate_post_request
+      if @request.path_info.end_with?('git-upload-pack')
+        can?(user, :push_code, project)
+      elsif @request.path_info.end_with?('git-receive-pack')
+        action = if project.protected_branch?(current_ref)
+                   :push_code_to_protected_branches
+                 else
+                   :push_code
+                 end
+
+        can?(user, action, project)
+      else
+        false
+      end
+    end
 
     def current_ref
       if @env["HTTP_CONTENT_ENCODING"] =~ /gzip/