From 0189ee97ed34b74cf0f500d678d4435b17ab6a85 Mon Sep 17 00:00:00 2001 From: randx Date: Sun, 21 Oct 2012 12:12:14 +0300 Subject: [PATCH] Security for online editor. Replace dev_access?, master_access? with can? method usage --- app/controllers/tree_controller.rb | 8 +++++ app/helpers/tree_helper.rb | 8 +++++ app/models/ability.rb | 7 ++++- app/roles/authority.rb | 2 +- app/roles/repository.rb | 5 +++ app/views/tree/_blob_actions.html.haml | 2 +- lib/gitlab/backend/grack_auth.rb | 42 +++++++++++++++++--------- 7 files changed, 56 insertions(+), 18 deletions(-) diff --git a/app/controllers/tree_controller.rb b/app/controllers/tree_controller.rb index c9098cca..e507fb51 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 c681dc60..4fe87a25 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 d3180b76..c3a212f4 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -35,9 +35,14 @@ 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, diff --git a/app/roles/authority.rb b/app/roles/authority.rb index dbdd9839..e0796d5f 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 88fd90d0..882ec310 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 d51f7205..21334ea1 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 43a75cc3..05329baf 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/