diff options
author | Douwe Maan <douwe@gitlab.com> | 2018-03-07 18:43:53 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2018-03-07 18:43:53 +0000 |
commit | e85f4697cce4f0040f235bd8f2fefce1daa9e8bf (patch) | |
tree | 29a5992d0bd311c535efc25d25acb31826d83d79 | |
parent | 67185097734bb88979df020123cf7327bc5d32c5 (diff) | |
parent | cfa9d1ed6b870dbc635148219b42d73c382eb90a (diff) | |
download | gitlab-ce-e85f4697cce4f0040f235bd8f2fefce1daa9e8bf.tar.gz |
Merge branch 'bvl-allow-maintainer-to-push' into 'master'
Allow maintainers to push forks of a project for branches that have open MRs
Closes #22292
See merge request gitlab-org/gitlab-ce!17395
58 files changed, 911 insertions, 118 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_maintainer_edit.vue b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_maintainer_edit.vue new file mode 100644 index 00000000000..7a69cce695e --- /dev/null +++ b/app/assets/javascripts/vue_merge_request_widget/components/mr_widget_maintainer_edit.vue @@ -0,0 +1,19 @@ +<script> + export default { + name: 'MRWidgetMaintainerEdit', + props: { + maintainerEditAllowed: { + type: Boolean, + default: false, + required: false, + }, + }, + }; +</script> +<template> + <section class="mr-info-list mr-maintainer-edit"> + <p v-if="maintainerEditAllowed"> + {{ s__("mrWidget|Allows edits from maintainers") }} + </p> + </section> +</template> diff --git a/app/assets/javascripts/vue_merge_request_widget/dependencies.js b/app/assets/javascripts/vue_merge_request_widget/dependencies.js index edb3baa39e4..a1bc28873df 100644 --- a/app/assets/javascripts/vue_merge_request_widget/dependencies.js +++ b/app/assets/javascripts/vue_merge_request_widget/dependencies.js @@ -15,6 +15,7 @@ export { default as WidgetHeader } from './components/mr_widget_header.vue'; export { default as WidgetMergeHelp } from './components/mr_widget_merge_help.vue'; export { default as WidgetPipeline } from './components/mr_widget_pipeline.vue'; export { default as WidgetDeployment } from './components/mr_widget_deployment'; +export { default as WidgetMaintainerEdit } from './components/mr_widget_maintainer_edit.vue'; export { default as WidgetRelatedLinks } from './components/mr_widget_related_links.vue'; export { default as MergedState } from './components/states/mr_widget_merged.vue'; export { default as FailedToMerge } from './components/states/mr_widget_failed_to_merge.vue'; diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 797f0f6ec0f..df3eb86f35c 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -6,6 +6,7 @@ import { WidgetMergeHelp, WidgetPipeline, WidgetDeployment, + WidgetMaintainerEdit, WidgetRelatedLinks, MergedState, ClosedState, @@ -211,6 +212,7 @@ export default { 'mr-widget-merge-help': WidgetMergeHelp, 'mr-widget-pipeline': WidgetPipeline, 'mr-widget-deployment': WidgetDeployment, + 'mr-widget-maintainer-edit': WidgetMaintainerEdit, 'mr-widget-related-links': WidgetRelatedLinks, 'mr-widget-merged': MergedState, 'mr-widget-closed': ClosedState, @@ -251,11 +253,12 @@ export default { :is="componentName" :mr="mr" :service="service" /> + <mr-widget-maintainer-edit + :maintainerEditAllowed="mr.maintainerEditAllowed" /> <mr-widget-related-links v-if="shouldRenderRelatedLinks" :state="mr.state" - :related-links="mr.relatedLinks" - /> + :related-links="mr.relatedLinks" /> </div> <div class="mr-widget-footer" diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 9a750ce42bd..5d07bcf1bb9 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -76,6 +76,7 @@ export default class MergeRequestStore { this.canBeMerged = data.can_be_merged || false; this.isMergeAllowed = data.mergeable || false; this.mergeOngoing = data.merge_ongoing; + this.maintainerEditAllowed = data.allow_maintainer_to_push; // Cherry-pick and Revert actions related this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false; diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index f887a11004f..f8708a2c0a3 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -453,7 +453,8 @@ } } -.mr-links { +.mr-links, +.mr-maintainer-edit { padding-left: $status-icon-size + $status-icon-margin; } diff --git a/app/controllers/concerns/creates_commit.rb b/app/controllers/concerns/creates_commit.rb index 6f4fdcdaa4f..b26a76d2b62 100644 --- a/app/controllers/concerns/creates_commit.rb +++ b/app/controllers/concerns/creates_commit.rb @@ -4,7 +4,7 @@ module CreatesCommit # rubocop:disable Gitlab/ModuleWithInstanceVariables def create_commit(service, success_path:, failure_path:, failure_view: nil, success_notice: nil) - if can?(current_user, :push_code, @project) + if user_access(@project).can_push_to_branch?(branch_name_or_ref) @project_to_commit_into = @project @branch_name ||= @ref else @@ -50,7 +50,7 @@ module CreatesCommit # rubocop:enable Gitlab/ModuleWithInstanceVariables def authorize_edit_tree! - return if can_collaborate_with_project? + return if can_collaborate_with_project?(project, ref: branch_name_or_ref) access_denied! end @@ -123,4 +123,8 @@ module CreatesCommit params[:create_merge_request].present? && (different_project? || @start_branch != @branch_name) # rubocop:disable Gitlab/ModuleWithInstanceVariables end + + def branch_name_or_ref + @branch_name || @ref # rubocop:disable Gitlab/ModuleWithInstanceVariables + end end diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb index 6025a40348b..6d9b42a2c04 100644 --- a/app/controllers/projects/application_controller.rb +++ b/app/controllers/projects/application_controller.rb @@ -6,7 +6,7 @@ class Projects::ApplicationController < ApplicationController before_action :repository layout 'project' - helper_method :repository, :can_collaborate_with_project? + helper_method :repository, :can_collaborate_with_project?, :user_access private @@ -31,11 +31,12 @@ class Projects::ApplicationController < ApplicationController @repository ||= project.repository end - def can_collaborate_with_project?(project = nil) + def can_collaborate_with_project?(project = nil, ref: nil) project ||= @project can?(current_user, :push_code, project) || - (current_user && current_user.already_forked?(project)) + (current_user && current_user.already_forked?(project)) || + user_access(project).can_push_to_branch?(ref) end def authorize_action!(action) @@ -90,4 +91,9 @@ class Projects::ApplicationController < ApplicationController def check_issues_available! return render_404 unless @project.feature_available?(:issues, current_user) end + + def user_access(project) + @user_access ||= {} + @user_access[project] ||= Gitlab::UserAccess.new(current_user, project: project) + end end diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 405726c017c..0c1c286a0a4 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -9,8 +9,12 @@ class Projects::BlobController < Projects::ApplicationController before_action :require_non_empty_project, except: [:new, :create] before_action :authorize_download_code! - before_action :authorize_edit_tree!, only: [:new, :create, :update, :destroy] + + # We need to assign the blob vars before `authorize_edit_tree!` so we can + # validate access to a specific ref. before_action :assign_blob_vars + before_action :authorize_edit_tree!, only: [:new, :create, :update, :destroy] + before_action :commit, except: [:new, :create] before_action :blob, except: [:new, :create] before_action :require_branch_head, only: [:edit, :update] @@ -46,7 +50,7 @@ class Projects::BlobController < Projects::ApplicationController end def edit - if can_collaborate_with_project? + if can_collaborate_with_project?(project, ref: @ref) blob.load_all_data! else redirect_to action: 'show' diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 793ae03fb88..67d4ea2ca8f 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -15,6 +15,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont def merge_request_params_attributes [ + :allow_maintainer_to_push, :assignee_id, :description, :force_remove_source_branch, diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index ce57422f45d..fb4fe1c40b7 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -125,6 +125,19 @@ module MergeRequestsHelper link_to(url[merge_request.project, merge_request], data: data_attrs, &block) end + def allow_maintainer_push_unavailable_reason(merge_request) + return if merge_request.can_allow_maintainer_to_push?(current_user) + + minimum_visibility = [merge_request.target_project.visibility_level, + merge_request.source_project.visibility_level].min + + if minimum_visibility < Gitlab::VisibilityLevel::INTERNAL + _('Not available for private projects') + elsif ProtectedBranch.protected?(merge_request.source_project, merge_request.source_branch) + _('Not available for protected branches') + end + end + def merge_params_ee(merge_request) {} end diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb index f6a6d9bebde..b64be89c181 100644 --- a/app/helpers/tree_helper.rb +++ b/app/helpers/tree_helper.rb @@ -49,15 +49,13 @@ module TreeHelper return false unless on_top_of_branch?(project, ref) - can_collaborate_with_project?(project) + can_collaborate_with_project?(project, ref: ref) end def tree_edit_branch(project = @project, ref = @ref) return unless can_edit_tree?(project, ref) - project = project.present(current_user: current_user) - - if project.can_current_user_push_to_branch?(ref) + if user_access(project).can_push_to_branch?(ref) ref else project = tree_edit_project(project) @@ -88,7 +86,16 @@ module TreeHelper end def commit_in_fork_help - "A new branch will be created in your fork and a new merge request will be started." + _("A new branch will be created in your fork and a new merge request will be started.") + end + + def commit_in_single_accessible_branch + branch_name = html_escape(selected_branch) + + message = _("Your changes can be committed to %{branch_name} because a merge "\ + "request is open.") % { branch_name: "<strong>#{branch_name}</strong>" } + + message.html_safe end def path_breadcrumbs(max_links = 6) @@ -125,4 +132,8 @@ module TreeHelper return tree.name end end + + def selected_branch + @branch_name || tree_edit_branch + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9a7e66a9cbb..c2bae379a94 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -865,7 +865,7 @@ class MergeRequest < ActiveRecord::Base def can_be_merged_by?(user) access = ::Gitlab::UserAccess.new(user, project: project) - access.can_push_to_branch?(target_branch) || access.can_merge_to_branch?(target_branch) + access.can_update_branch?(target_branch) end def can_be_merged_via_command_line_by?(user) @@ -1087,4 +1087,22 @@ class MergeRequest < ActiveRecord::Base project.merge_requests.merged.where(author_id: author_id).empty? end + + def allow_maintainer_to_push + maintainer_push_possible? && super + end + + alias_method :allow_maintainer_to_push?, :allow_maintainer_to_push + + def maintainer_push_possible? + source_project.present? && for_fork? && + target_project.visibility_level > Gitlab::VisibilityLevel::PRIVATE && + source_project.visibility_level > Gitlab::VisibilityLevel::PRIVATE && + !ProtectedBranch.protected?(source_project, source_branch) + end + + def can_allow_maintainer_to_push?(user) + maintainer_push_possible? && + Ability.allowed?(user, :push_code, source_project) + end end diff --git a/app/models/project.rb b/app/models/project.rb index 5cd1da43645..5f9d9785d64 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -150,6 +150,7 @@ class Project < ActiveRecord::Base # Merge Requests for target project should be removed with it has_many :merge_requests, foreign_key: 'target_project_id' + has_many :source_of_merge_requests, foreign_key: 'source_project_id', class_name: 'MergeRequest' has_many :issues has_many :labels, class_name: 'ProjectLabel' has_many :services @@ -1799,6 +1800,33 @@ class Project < ActiveRecord::Base Badge.where("id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end + def merge_requests_allowing_push_to_user(user) + return MergeRequest.none unless user + + developer_access_exists = user.project_authorizations + .where('access_level >= ? ', Gitlab::Access::DEVELOPER) + .where('project_authorizations.project_id = merge_requests.target_project_id') + .limit(1) + .select(1) + source_of_merge_requests.opened + .where(allow_maintainer_to_push: true) + .where('EXISTS (?)', developer_access_exists) + end + + def branch_allows_maintainer_push?(user, branch_name) + return false unless user + + cache_key = "user:#{user.id}:#{branch_name}:branch_allows_push" + + memoized_results = strong_memoize(:branch_allows_maintainer_push) do + Hash.new do |result, cache_key| + result[cache_key] = fetch_branch_allows_maintainer_push?(user, branch_name) + end + end + + memoized_results[cache_key] + end + private def storage @@ -1921,4 +1949,22 @@ class Project < ActiveRecord::Base raise ex end + + def fetch_branch_allows_maintainer_push?(user, branch_name) + check_access = -> do + merge_request = source_of_merge_requests.opened + .where(allow_maintainer_to_push: true) + .find_by(source_branch: branch_name) + + merge_request&.can_be_merged_by?(user) + end + + if RequestStore.active? + RequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_maintainer_push") do + check_access.call + end + else + check_access.call + end + end end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 3b0550b4dd6..57ab0c23dcd 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -61,6 +61,11 @@ class ProjectPolicy < BasePolicy desc "Project has request access enabled" condition(:request_access_enabled, scope: :subject) { project.request_access_enabled } + desc "Has merge requests allowing pushes to user" + condition(:has_merge_requests_allowing_pushes, scope: :subject) do + project.merge_requests_allowing_push_to_user(user).any? + end + features = %w[ merge_requests issues @@ -291,6 +296,15 @@ class ProjectPolicy < BasePolicy prevent :read_issue end + # These rules are included to allow maintainers of projects to push to certain + # to run pipelines for the branches they have access to. + rule { can?(:public_access) & has_merge_requests_allowing_pushes }.policy do + enable :create_build + enable :update_build + enable :create_pipeline + enable :update_pipeline + end + private def team_member? diff --git a/app/presenters/merge_request_presenter.rb b/app/presenters/merge_request_presenter.rb index 08ae49562c7..9f3f2637183 100644 --- a/app/presenters/merge_request_presenter.rb +++ b/app/presenters/merge_request_presenter.rb @@ -78,7 +78,7 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end def rebase_path - if !rebase_in_progress? && should_be_rebased? && user_can_push_to_source_branch? + if !rebase_in_progress? && should_be_rebased? && can_push_to_source_branch? rebase_project_merge_request_path(project, merge_request) end end @@ -160,7 +160,11 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end def can_push_to_source_branch? - source_branch_exists? && user_can_push_to_source_branch? + return false unless source_branch_exists? + + !!::Gitlab::UserAccess + .new(current_user, project: source_project) + .can_push_to_branch?(source_branch) end private @@ -191,17 +195,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated end.sort.to_sentence end - def user_can_push_to_source_branch? - return false unless source_branch_exists? - - ::Gitlab::UserAccess - .new(current_user, project: source_project) - .can_push_to_branch?(source_branch) - end - def user_can_collaborate_with_project? can?(current_user, :push_code, project) || - (current_user && current_user.already_forked?(project)) + (current_user && current_user.already_forked?(project)) || + can_push_to_source_branch? end def user_can_fork_project? diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index a3ebec0efc6..4a812e39ee1 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -11,6 +11,7 @@ class MergeRequestWidgetEntity < IssuableEntity expose :source_project_id expose :target_branch expose :target_project_id + expose :allow_maintainer_to_push expose :should_be_rebased?, as: :should_be_rebased expose :ff_only_enabled do |merge_request| @@ -29,6 +30,7 @@ class MergeRequestWidgetEntity < IssuableEntity expose :can_push_to_source_branch do |merge_request| presenter(merge_request).can_push_to_source_branch? end + expose :rebase_path do |merge_request| presenter(merge_request).rebase_path end @@ -136,8 +138,8 @@ class MergeRequestWidgetEntity < IssuableEntity end expose :new_blob_path do |merge_request| - if can?(current_user, :push_code, merge_request.project) - project_new_blob_path(merge_request.project, merge_request.source_branch) + if presenter(merge_request).can_push_to_source_branch? + project_new_blob_path(merge_request.source_project, merge_request.source_branch) end end diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index c8b112132b3..3b3d9239086 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -81,7 +81,7 @@ module Ci end def related_merge_requests - MergeRequest.opened.where(source_project: pipeline.project, source_branch: pipeline.ref) + pipeline.project.source_of_merge_requests.opened.where(source_branch: pipeline.ref) end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 23262b62615..231ab76fde4 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -35,6 +35,14 @@ module MergeRequests end end + def filter_params(merge_request) + super + + unless merge_request.can_allow_maintainer_to_push?(current_user) + params.delete(:allow_maintainer_to_push) + end + end + def merge_request_metrics_service(merge_request) MergeRequestMetricsService.new(merge_request.metrics) end diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 4b186d93772..a98bbdf74dd 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -6,6 +6,7 @@ module MergeRequests @params_issue_iid = params.delete(:issue_iid) self.merge_request = MergeRequest.new(params) + merge_request.author = current_user merge_request.compare_commits = [] merge_request.source_project = find_source_project merge_request.target_project = find_target_project diff --git a/app/views/projects/_commit_button.html.haml b/app/views/projects/_commit_button.html.haml index b55dc3dce5c..b387e38c1a6 100644 --- a/app/views/projects/_commit_button.html.haml +++ b/app/views/projects/_commit_button.html.haml @@ -3,6 +3,4 @@ = link_to 'Cancel', cancel_path, class: 'btn btn-cancel', data: {confirm: leave_edit_message} - - unless can?(current_user, :push_code, @project) - .inline.prepend-left-10 - = commit_in_fork_help + = render 'shared/projects/edit_information' diff --git a/app/views/projects/blob/_new_dir.html.haml b/app/views/projects/blob/_new_dir.html.haml index 5d48a35dc4c..48ff66900be 100644 --- a/app/views/projects/blob/_new_dir.html.haml +++ b/app/views/projects/blob/_new_dir.html.haml @@ -17,6 +17,4 @@ = submit_tag _("Create directory"), class: 'btn btn-create' = link_to "Cancel", '#', class: "btn btn-cancel", "data-dismiss" => "modal" - - unless can?(current_user, :push_code, @project) - .inline.prepend-left-10 - = commit_in_fork_help + = render 'shared/projects/edit_information' diff --git a/app/views/projects/blob/_upload.html.haml b/app/views/projects/blob/_upload.html.haml index f1324c61500..182d02376bf 100644 --- a/app/views/projects/blob/_upload.html.haml +++ b/app/views/projects/blob/_upload.html.haml @@ -24,6 +24,4 @@ = button_title = link_to _("Cancel"), '#', class: "btn btn-cancel", "data-dismiss" => "modal" - - unless can?(current_user, :push_code, @project) - .inline.prepend-left-10 - = commit_in_fork_help + = render 'shared/projects/edit_information' diff --git a/app/views/projects/commit/_change.html.haml b/app/views/projects/commit/_change.html.haml index 93407956f56..21e4664d4e4 100644 --- a/app/views/projects/commit/_change.html.haml +++ b/app/views/projects/commit/_change.html.haml @@ -35,6 +35,4 @@ = submit_tag label, class: 'btn btn-create' = link_to _("Cancel"), '#', class: "btn btn-cancel", "data-dismiss" => "modal" - - unless can?(current_user, :push_code, @project) - .inline.prepend-left-10 - = commit_in_fork_help + = render 'shared/projects/edit_information' diff --git a/app/views/shared/_new_commit_form.html.haml b/app/views/shared/_new_commit_form.html.haml index 0a4a24ae807..9221fd1e025 100644 --- a/app/views/shared/_new_commit_form.html.haml +++ b/app/views/shared/_new_commit_form.html.haml @@ -1,3 +1,6 @@ +- project = @project.present(current_user: current_user) +- branch_name = selected_branch + = render 'shared/commit_message_container', placeholder: placeholder - if @project.empty_repo? @@ -7,12 +10,14 @@ .form-group.branch = label_tag 'branch_name', _('Target Branch'), class: 'control-label' .col-sm-10 - = text_field_tag 'branch_name', @branch_name || tree_edit_branch, required: true, class: "form-control js-branch-name ref-name" + = text_field_tag 'branch_name', branch_name, required: true, class: "form-control js-branch-name ref-name" .js-create-merge-request-container = render 'shared/new_merge_request_checkbox' + - elsif project.can_current_user_push_to_branch?(branch_name) + = hidden_field_tag 'branch_name', branch_name - else - = hidden_field_tag 'branch_name', @branch_name || tree_edit_branch + = hidden_field_tag 'branch_name', branch_name = hidden_field_tag 'create_merge_request', 1 = hidden_field_tag 'original_branch', @ref, class: 'js-original-branch' diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 6dfabd7ba4c..4c8f03f1498 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -33,6 +33,8 @@ = render 'shared/issuable/form/merge_params', issuable: issuable += render 'shared/issuable/form/contribution', issuable: issuable, form: form + - if @merge_request_to_resolve_discussions_of .form-group .col-sm-10.col-sm-offset-2 diff --git a/app/views/shared/issuable/form/_contribution.html.haml b/app/views/shared/issuable/form/_contribution.html.haml new file mode 100644 index 00000000000..0f2d313a5cc --- /dev/null +++ b/app/views/shared/issuable/form/_contribution.html.haml @@ -0,0 +1,20 @@ +- issuable = local_assigns.fetch(:issuable) +- form = local_assigns.fetch(:form) + +- return unless issuable.is_a?(MergeRequest) +- return unless issuable.for_fork? +- return unless can?(current_user, :push_code, issuable.source_project) + +%hr + +.form-group + .control-label + = _('Contribution') + .col-sm-10 + .checkbox + = form.label :allow_maintainer_to_push do + = form.check_box :allow_maintainer_to_push, disabled: !issuable.can_allow_maintainer_to_push?(current_user) + = _('Allow edits from maintainers') + = link_to 'About this feature', help_page_path('user/project/merge_requests/maintainer_access') + .help-block + = allow_maintainer_push_unavailable_reason(issuable) diff --git a/app/views/shared/projects/_edit_information.html.haml b/app/views/shared/projects/_edit_information.html.haml new file mode 100644 index 00000000000..ec9dc8f62c2 --- /dev/null +++ b/app/views/shared/projects/_edit_information.html.haml @@ -0,0 +1,6 @@ +- unless can?(current_user, :push_code, @project) + .inline.prepend-left-10 + - if @project.branch_allows_maintainer_push?(current_user, selected_branch) + = commit_in_single_accessible_branch + - else + = commit_in_fork_help diff --git a/changelogs/unreleased/bvl-allow-maintainer-to-push.yml b/changelogs/unreleased/bvl-allow-maintainer-to-push.yml new file mode 100644 index 00000000000..a3fefc2889a --- /dev/null +++ b/changelogs/unreleased/bvl-allow-maintainer-to-push.yml @@ -0,0 +1,5 @@ +--- +title: Allow maintainers to push to forks of their projects when a merge request is open +merge_request: 17395 +author: +type: added diff --git a/db/migrate/20180221151752_add_allow_maintainer_to_push_to_merge_requests.rb b/db/migrate/20180221151752_add_allow_maintainer_to_push_to_merge_requests.rb new file mode 100644 index 00000000000..81acfbc3655 --- /dev/null +++ b/db/migrate/20180221151752_add_allow_maintainer_to_push_to_merge_requests.rb @@ -0,0 +1,18 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddAllowMaintainerToPushToMergeRequests < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column :merge_requests, :allow_maintainer_to_push, :boolean + end + + def down + remove_column :merge_requests, :allow_maintainer_to_push + end +end diff --git a/db/schema.rb b/db/schema.rb index 387b15f8f30..b93e6fa594b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1145,6 +1145,7 @@ ActiveRecord::Schema.define(version: 20180307012445) do t.boolean "discussion_locked" t.integer "latest_merge_request_diff_id" t.string "rebase_commit_sha" + t.boolean "allow_maintainer_to_push" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 25b0807eb18..b9a4f661777 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -529,18 +529,19 @@ Creates a new merge request. POST /projects/:id/merge_requests ``` -| Attribute | Type | Required | Description | -| --------- | ---- | -------- | ----------- | -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `source_branch` | string | yes | The source branch | -| `target_branch` | string | yes | The target branch | -| `title` | string | yes | Title of MR | -| `assignee_id` | integer | no | Assignee user ID | -| `description` | string | no | Description of MR | -| `target_project_id` | integer | no | The target project (numeric id) | -| `labels` | string | no | Labels for MR as a comma-separated list | -| `milestone_id` | integer | no | The ID of a milestone | -| `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `source_branch` | string | yes | The source branch | +| `target_branch` | string | yes | The target branch | +| `title` | string | yes | Title of MR | +| `assignee_id` | integer | no | Assignee user ID | +| `description` | string | no | Description of MR | +| `target_project_id` | integer | no | The target project (numeric id) | +| `labels` | string | no | Labels for MR as a comma-separated list | +| `milestone_id` | integer | no | The ID of a milestone | +| `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | +| `allow_maintainer_to_push` | boolean | no | Whether or not a maintainer of the target project can push to the source branch | ```json { @@ -548,7 +549,7 @@ POST /projects/:id/merge_requests "iid": 1, "target_branch": "master", "source_branch": "test1", - "project_id": 3, + "project_id": 4, "title": "test1", "state": "opened", "upvotes": 0, @@ -569,7 +570,7 @@ POST /projects/:id/merge_requests "state": "active", "created_at": "2012-04-29T08:46:00Z" }, - "source_project_id": 4, + "source_project_id": 3, "target_project_id": 4, "labels": [ ], "description": "fixed login page css paddings", @@ -596,6 +597,7 @@ POST /projects/:id/merge_requests "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, + "allow_maintainer_to_push": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, @@ -613,19 +615,20 @@ Updates an existing merge request. You can change the target branch, title, or e PUT /projects/:id/merge_requests/:merge_request_iid ``` -| Attribute | Type | Required | Description | -| --------- | ---- | -------- | ----------- | -| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | -| `merge_request_iid` | integer | yes | The ID of a merge request | -| `target_branch` | string | no | The target branch | -| `title` | string | no | Title of MR | -| `assignee_id` | integer | no | The ID of the user to assign the merge request to. Set to `0` or provide an empty value to unassign all assignees. | -| `milestone_id` | integer | no | The ID of a milestone to assign the merge request to. Set to `0` or provide an empty value to unassign a milestone.| -| `labels` | string | no | Comma-separated label names for a merge request. Set to an empty string to unassign all labels. | -| `description` | string | no | Description of MR | -| `state_event` | string | no | New state (close/reopen) | -| `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | -| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked. If the discussion is locked only project members can add, edit or resolve comments. | +| Attribute | Type | Required | Description | +| --------- | ---- | -------- | ----------- | +| `id` | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user | +| `merge_request_iid` | integer | yes | The ID of a merge request | +| `target_branch` | string | no | The target branch | +| `title` | string | no | Title of MR | +| `assignee_id` | integer | no | The ID of the user to assign the merge request to. Set to `0` or provide an empty value to unassign all assignees. | +| `milestone_id` | integer | no | The ID of a milestone to assign the merge request to. Set to `0` or provide an empty value to unassign a milestone.| +| `labels` | string | no | Comma-separated label names for a merge request. Set to an empty string to unassign all labels. | +| `description` | string | no | Description of MR | +| `state_event` | string | no | New state (close/reopen) | +| `remove_source_branch` | boolean | no | Flag indicating if a merge request should remove the source branch when merging | +| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked. If the discussion is locked only project members can add, edit or resolve comments. | +| `allow_maintainer_to_push` | boolean | no | Whether or not a maintainer of the target project can push to the source branch | Must include at least one non-required attribute from above. @@ -634,7 +637,7 @@ Must include at least one non-required attribute from above. "id": 1, "iid": 1, "target_branch": "master", - "project_id": 3, + "project_id": 4, "title": "test1", "state": "opened", "upvotes": 0, @@ -655,7 +658,7 @@ Must include at least one non-required attribute from above. "state": "active", "created_at": "2012-04-29T08:46:00Z" }, - "source_project_id": 4, + "source_project_id": 3, "target_project_id": 4, "labels": [ ], "description": "description1", @@ -682,6 +685,7 @@ Must include at least one non-required attribute from above. "force_remove_source_branch": false, "web_url": "http://example.com/example/example/merge_requests/1", "discussion_locked": false, + "allow_maintainer_to_push": false, "time_stats": { "time_estimate": 0, "total_time_spent": 0, diff --git a/doc/user/project/merge_requests/img/allow_maintainer_push.png b/doc/user/project/merge_requests/img/allow_maintainer_push.png Binary files differnew file mode 100644 index 00000000000..e73cc1ab5a5 --- /dev/null +++ b/doc/user/project/merge_requests/img/allow_maintainer_push.png diff --git a/doc/user/project/merge_requests/index.md b/doc/user/project/merge_requests/index.md index d3220598933..10d67729734 100644 --- a/doc/user/project/merge_requests/index.md +++ b/doc/user/project/merge_requests/index.md @@ -28,6 +28,7 @@ With GitLab merge requests, you can: - Enable [fast-forward merge requests](#fast-forward-merge-requests) - Enable [semi-linear history merge requests](#semi-linear-history-merge-requests) as another security layer to guarantee the pipeline is passing in the target branch - [Create new merge requests by email](#create-new-merge-requests-by-email) +- Allow maintainers of the target project to push directly to the fork by [allowing edits from maintainers](maintainer_access.md) With **[GitLab Enterprise Edition][ee]**, you can also: diff --git a/doc/user/project/merge_requests/maintainer_access.md b/doc/user/project/merge_requests/maintainer_access.md new file mode 100644 index 00000000000..1ade5db1587 --- /dev/null +++ b/doc/user/project/merge_requests/maintainer_access.md @@ -0,0 +1,13 @@ +# Allow maintainer pushes for merge requests accross forks + +This feature is available for merge requests across forked projects that are +publicly accessible. It makes it easier for maintainers of projects to collaborate +on merge requests across forks. + +When enabling this feature for a merge request, you give can give members with push access to the target project rights to edit files on the source branch of the merge request. + +The feature can only be enabled by users who already have push access to the source project. And only lasts while the merge request is open. + +Enable this functionality while creating a merge request: + +![Enable maintainer edits](./img/allow_maintainer_push.png) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 4555184095c..16147ee90c9 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -547,6 +547,7 @@ module API expose :discussion_locked expose :should_remove_source_branch?, as: :should_remove_source_branch expose :force_remove_source_branch?, as: :force_remove_source_branch + expose :allow_maintainer_to_push, if: -> (merge_request, _) { merge_request.for_fork? } expose :web_url do |merge_request, options| Gitlab::UrlBuilder.build(merge_request) diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index ead1bb7957b..3264a26f7d2 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -144,6 +144,7 @@ module API optional :milestone_id, type: Integer, desc: 'The ID of a milestone to assign the merge request' optional :labels, type: String, desc: 'Comma-separated list of label names' optional :remove_source_branch, type: Boolean, desc: 'Remove source branch when merging' + optional :allow_maintainer_to_push, type: Boolean, desc: 'Whether a maintainer of the target project can push to the source project' use :optional_params_ee end diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 3ce5f807989..51ba09aa129 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -47,7 +47,7 @@ module Gitlab protected def push_checks - if user_access.cannot_do_action?(:push_code) + unless can_push? raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:push_code] end end @@ -183,6 +183,11 @@ module Gitlab def commits @commits ||= project.repository.new_commits(newrev) end + + def can_push? + user_access.can_do_action?(:push_code) || + user_access.can_push_to_branch?(branch_name) + end end end end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 91b8bb2a83f..fd68b9f2b48 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -68,8 +68,10 @@ module Gitlab return true if project.user_can_push_to_empty_repo?(user) protected_branch_accessible_to?(ref, action: :push) + elsif user.can?(:push_code, project) + true else - user.can?(:push_code, project) + project.branch_allows_maintainer_push?(user, ref) end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8a2176a4d72..bb7aaa2fb2f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8,8 +8,8 @@ msgid "" msgstr "" "Project-Id-Version: gitlab 1.0.0\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2018-03-05 13:02-0600\n" -"PO-Revision-Date: 2018-03-05 13:02-0600\n" +"POT-Creation-Date: 2018-03-06 17:36+0100\n" +"PO-Revision-Date: 2018-03-06 17:36+0100\n" "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n" "Language-Team: LANGUAGE <LL@li.org>\n" "Language: \n" @@ -102,6 +102,9 @@ msgstr "" msgid "A collection of graphs regarding Continuous Integration" msgstr "" +msgid "A new branch will be created in your fork and a new merge request will be started." +msgstr "" + msgid "A project is where you house your files (repository), plan your work (issues), and publish your documentation (wiki), %{among_other_things_link}." msgstr "" @@ -207,6 +210,9 @@ msgstr "" msgid "All features are enabled for blank projects, from templates, or when importing, but you can disable them afterward in the project settings." msgstr "" +msgid "Allow edits from maintainers" +msgstr "" + msgid "Allows you to add and manage Kubernetes clusters." msgstr "" @@ -288,9 +294,6 @@ msgstr "" msgid "Are you sure you want to delete this pipeline schedule?" msgstr "" -msgid "Are you sure you want to discard your changes?" -msgstr "" - msgid "Are you sure you want to reset registration token?" msgstr "" @@ -392,9 +395,6 @@ msgstr[1] "" msgid "Branch <strong>%{branch_name}</strong> was created. To set up auto deploy, choose a GitLab CI Yaml template and commit your changes. %{link_to_autodeploy_doc}" msgstr "" -msgid "Branch has changed" -msgstr "" - msgid "Branch is already taken" msgstr "" @@ -410,6 +410,15 @@ msgstr "" msgid "Branches" msgstr "" +msgid "Branches|Active" +msgstr "" + +msgid "Branches|Active branches" +msgstr "" + +msgid "Branches|All" +msgstr "" + msgid "Branches|Cant find HEAD commit for this branch" msgstr "" @@ -455,12 +464,39 @@ msgstr "" msgid "Branches|Only a project master or owner can delete a protected branch" msgstr "" -msgid "Branches|Protected branches can be managed in %{project_settings_link}" +msgid "Branches|Overview" +msgstr "" + +msgid "Branches|Protected branches can be managed in %{project_settings_link}." +msgstr "" + +msgid "Branches|Show active branches" +msgstr "" + +msgid "Branches|Show all branches" +msgstr "" + +msgid "Branches|Show more active branches" +msgstr "" + +msgid "Branches|Show more stale branches" +msgstr "" + +msgid "Branches|Show overview of the branches" +msgstr "" + +msgid "Branches|Show stale branches" msgstr "" msgid "Branches|Sort by" msgstr "" +msgid "Branches|Stale" +msgstr "" + +msgid "Branches|Stale branches" +msgstr "" + msgid "Branches|The default branch cannot be deleted" msgstr "" @@ -512,9 +548,6 @@ msgstr "" msgid "Cancel" msgstr "" -msgid "Cancel edit" -msgstr "" - msgid "Cannot modify managed Kubernetes cluster" msgstr "" @@ -662,6 +695,9 @@ msgstr "" msgid "Clone repository" msgstr "" +msgid "Close" +msgstr "" + msgid "ClusterIntegration|%{appList} was successfully installed on your Kubernetes cluster" msgstr "" @@ -1077,6 +1113,9 @@ msgstr "" msgid "ContainerRegistry|With the Docker Container Registry integrated into GitLab, every project can have its own space to store its Docker images." msgstr "" +msgid "Contribution" +msgstr "" + msgid "Contribution guide" msgstr "" @@ -1128,9 +1167,6 @@ msgstr "" msgid "Create empty bare repository" msgstr "" -msgid "Create file" -msgstr "" - msgid "Create lists from labels. Issues with that label appear in that list." msgstr "" @@ -1140,15 +1176,6 @@ msgstr "" msgid "Create merge request and branch" msgstr "" -msgid "Create new branch" -msgstr "" - -msgid "Create new directory" -msgstr "" - -msgid "Create new file" -msgstr "" - msgid "Create new label" msgstr "" @@ -1238,9 +1265,6 @@ msgstr "" msgid "Directory name" msgstr "" -msgid "Discard changes" -msgstr "" - msgid "Dismiss Cycle Analytics introduction box" msgstr "" @@ -1421,9 +1445,6 @@ msgstr "" msgid "Fields on this page are now uneditable, you can configure" msgstr "" -msgid "File name" -msgstr "" - msgid "Files" msgstr "" @@ -1439,6 +1460,9 @@ msgstr "" msgid "Find file" msgstr "" +msgid "Finished" +msgstr "" + msgid "FirstPushedBy|First" msgstr "" @@ -1495,6 +1519,9 @@ msgstr "" msgid "Gitaly Servers" msgstr "" +msgid "Go back" +msgstr "" + msgid "Go to your fork" msgstr "" @@ -1701,6 +1728,15 @@ msgstr "" msgid "LFSStatus|Enabled" msgstr "" +msgid "Label" +msgstr "" + +msgid "LabelSelect|%{firstLabelName} +%{remainingLabelCount} more" +msgstr "" + +msgid "LabelSelect|%{labelsString}, and %{remainingLabelCount} more" +msgstr "" + msgid "Labels" msgstr "" @@ -1760,9 +1796,6 @@ msgstr "" msgid "Leave project" msgstr "" -msgid "Loading the GitLab IDE..." -msgstr "" - msgid "Lock" msgstr "" @@ -1945,6 +1978,12 @@ msgstr "" msgid "Not available" msgstr "" +msgid "Not available for private projects" +msgstr "" + +msgid "Not available for protected branches" +msgstr "" + msgid "Not confidential" msgstr "" @@ -2071,6 +2110,9 @@ msgstr "" msgid "Password" msgstr "" +msgid "Pending" +msgstr "" + msgid "Pipeline" msgstr "" @@ -2149,9 +2191,30 @@ msgstr "" msgid "Pipelines|Build with confidence" msgstr "" +msgid "Pipelines|CI Lint" +msgstr "" + +msgid "Pipelines|Clear Runner Caches" +msgstr "" + msgid "Pipelines|Get started with Pipelines" msgstr "" +msgid "Pipelines|Loading Pipelines" +msgstr "" + +msgid "Pipelines|Run Pipeline" +msgstr "" + +msgid "Pipelines|There are currently no %{scope} pipelines." +msgstr "" + +msgid "Pipelines|There are currently no pipelines." +msgstr "" + +msgid "Pipelines|This project is not currently set up to run pipelines." +msgstr "" + msgid "Pipeline|Retry pipeline" msgstr "" @@ -2404,6 +2467,9 @@ msgstr "" msgid "Public - The project can be accessed without any authentication." msgstr "" +msgid "Push access to this project is necessary in order to enable this option" +msgstr "" + msgid "Push events" msgstr "" @@ -2490,6 +2556,9 @@ msgstr "" msgid "Revert this merge request" msgstr "" +msgid "Running" +msgstr "" + msgid "SSH Keys" msgstr "" @@ -2511,6 +2580,9 @@ msgstr "" msgid "Scheduling Pipelines" msgstr "" +msgid "Search" +msgstr "" + msgid "Search branches and tags" msgstr "" @@ -3278,9 +3350,6 @@ msgstr "" msgid "We want to be sure it is you, please confirm you are not a robot." msgstr "" -msgid "Web IDE" -msgstr "" - msgid "Wiki" msgstr "" @@ -3395,13 +3464,13 @@ msgstr "" msgid "You are going to remove %{group_name}. Removed groups CANNOT be restored! Are you ABSOLUTELY sure?" msgstr "" -msgid "You are going to remove %{project_name_with_namespace}. Removed project CANNOT be restored! Are you ABSOLUTELY sure?" +msgid "You are going to remove %{project_full_name}. Removed project CANNOT be restored! Are you ABSOLUTELY sure?" msgstr "" msgid "You are going to remove the fork relationship to source project %{forked_from_project}. Are you ABSOLUTELY sure?" msgstr "" -msgid "You are going to transfer %{project_name_with_namespace} to another owner. Are you ABSOLUTELY sure?" +msgid "You are going to transfer %{project_full_name} to another owner. Are you ABSOLUTELY sure?" msgstr "" msgid "You are on a read-only GitLab instance." @@ -3467,6 +3536,9 @@ msgstr "" msgid "Your Kubernetes cluster information on this page is still editable, but you are advised to disable and reconfigure" msgstr "" +msgid "Your changes can be committed to %{branch_name} because a merge request is open." +msgstr "" + msgid "Your comment will not be visible to the public." msgstr "" @@ -3513,6 +3585,9 @@ msgstr[1] "" msgid "mrWidget| Please restore it or use a different %{missingBranchName} branch" msgstr "" +msgid "mrWidget|Allows edits from maintainers" +msgstr "" + msgid "mrWidget|Cancel automatic merge" msgstr "" diff --git a/spec/features/merge_request/maintainer_edits_fork_spec.rb b/spec/features/merge_request/maintainer_edits_fork_spec.rb new file mode 100644 index 00000000000..a3323da1b1f --- /dev/null +++ b/spec/features/merge_request/maintainer_edits_fork_spec.rb @@ -0,0 +1,44 @@ +require 'spec_helper' + +describe 'a maintainer edits files on a source-branch of an MR from a fork', :js do + include ProjectForksHelper + let(:user) { create(:user, username: 'the-maintainer') } + let(:target_project) { create(:project, :public, :repository) } + let(:author) { create(:user, username: 'mr-authoring-machine') } + let(:source_project) { fork_project(target_project, author, repository: true) } + + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: target_project, + source_branch: 'fix', + target_branch: 'master', + author: author, + allow_maintainer_to_push: true) + end + + before do + target_project.add_master(user) + sign_in(user) + + visit project_merge_request_path(target_project, merge_request) + click_link 'Changes' + wait_for_requests + first('.js-file-title').click_link 'Edit' + wait_for_requests + end + + it 'mentions commits will go to the source branch' do + expect(page).to have_content('Your changes can be committed to fix because a merge request is open.') + end + + it 'allows committing to the source branch' do + find('.ace_text-input', visible: false).send_keys('Updated the readme') + + click_button 'Commit changes' + wait_for_requests + + expect(page).to have_content('Your changes have been successfully committed') + expect(page).to have_content('Updated the readme') + end +end diff --git a/spec/features/merge_request/user_allows_a_maintainer_to_push_spec.rb b/spec/features/merge_request/user_allows_a_maintainer_to_push_spec.rb new file mode 100644 index 00000000000..eb41d7de8ed --- /dev/null +++ b/spec/features/merge_request/user_allows_a_maintainer_to_push_spec.rb @@ -0,0 +1,85 @@ +require 'spec_helper' + +describe 'create a merge request that allows maintainers to push', :js do + include ProjectForksHelper + let(:user) { create(:user) } + let(:target_project) { create(:project, :public, :repository) } + let(:source_project) { fork_project(target_project, user, repository: true, namespace: user.namespace) } + + def visit_new_merge_request + visit project_new_merge_request_path( + source_project, + merge_request: { + source_project_id: source_project.id, + target_project_id: target_project.id, + source_branch: 'fix', + target_branch: 'master' + }) + end + + before do + sign_in(user) + end + + it 'allows setting maintainer push possible' do + visit_new_merge_request + + check 'Allow edits from maintainers' + + click_button 'Submit merge request' + + wait_for_requests + + expect(page).to have_content('Allows edits from maintainers') + end + + it 'shows a message when one of the projects is private' do + source_project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + + visit_new_merge_request + + expect(page).to have_content('Not available for private projects') + end + + it 'shows a message when the source branch is protected' do + create(:protected_branch, project: source_project, name: 'fix') + + visit_new_merge_request + + expect(page).to have_content('Not available for protected branches') + end + + context 'when the merge request is being created within the same project' do + let(:source_project) { target_project } + + it 'hides the checkbox if the merge request is being created within the same project' do + target_project.add_developer(user) + + visit_new_merge_request + + expect(page).not_to have_content('Allows edits from maintainers') + end + end + + context 'when a maintainer tries to edit the option' do + let(:maintainer) { create(:user) } + let(:merge_request) do + create(:merge_request, + source_project: source_project, + target_project: target_project, + source_branch: 'fixes') + end + + before do + target_project.add_master(maintainer) + + sign_in(maintainer) + end + + it 'it hides the option from maintainers' do + visit edit_project_merge_request_path(target_project, merge_request) + + expect(page).not_to have_content('Allows edits from maintainers') + end + end +end diff --git a/spec/features/projects/user_creates_files_spec.rb b/spec/features/projects/user_creates_files_spec.rb index 7a935dd2477..8993533676b 100644 --- a/spec/features/projects/user_creates_files_spec.rb +++ b/spec/features/projects/user_creates_files_spec.rb @@ -133,13 +133,20 @@ describe 'User creates files' do before do project2.add_reporter(user) visit(project2_tree_path_root_ref) - end - it 'creates and commit new file in forked project', :js do find('.add-to-tree').click click_link('New file') + end + + it 'shows a message saying the file will be committed in a fork' do + message = "A new branch will be created in your fork and a new merge request will be started." + expect(page).to have_content(message) + end + + it 'creates and commit new file in forked project', :js do expect(page).to have_selector('.file-editor') + expect(page).to have_content find('#editor') execute_script("ace.edit('editor').setValue('*.rbca')") diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index f1199468d53..46031961cca 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json @@ -12,7 +12,8 @@ "rebase_in_progress": { "type": "boolean" }, "assignee_id": { "type": ["integer", "null"] }, "subscribed": { "type": ["boolean", "null"] }, - "participants": { "type": "array" } + "participants": { "type": "array" }, + "allow_maintainer_to_push": { "type": "boolean"} }, "additionalProperties": false } diff --git a/spec/fixtures/api/schemas/entities/merge_request_widget.json b/spec/fixtures/api/schemas/entities/merge_request_widget.json index cfbeec58a45..a622bf88b13 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_widget.json +++ b/spec/fixtures/api/schemas/entities/merge_request_widget.json @@ -30,6 +30,7 @@ "source_project_id": { "type": "integer" }, "target_branch": { "type": "string" }, "target_project_id": { "type": "integer" }, + "allow_maintainer_to_push": { "type": "boolean"}, "metrics": { "oneOf": [ { "type": "null" }, diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json index e86176e5316..0dc2eabec5d 100644 --- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json +++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json @@ -80,7 +80,8 @@ "total_time_spent": { "type": "integer" }, "human_time_estimate": { "type": ["string", "null"] }, "human_total_time_spent": { "type": ["string", "null"] } - } + }, + "allow_maintainer_to_push": { "type": ["boolean", "null"] } }, "required": [ "id", "iid", "project_id", "title", "description", diff --git a/spec/helpers/tree_helper_spec.rb b/spec/helpers/tree_helper_spec.rb index d3b1be599dd..ccac6e29447 100644 --- a/spec/helpers/tree_helper_spec.rb +++ b/spec/helpers/tree_helper_spec.rb @@ -62,4 +62,13 @@ describe TreeHelper do end end end + + describe '#commit_in_single_accessible_branch' do + it 'escapes HTML from the branch name' do + helper.instance_variable_set(:@branch_name, "<script>alert('escape me!');</script>") + escaped_branch_name = '<script>alert('escape me!');</script>' + + expect(helper.commit_in_single_accessible_branch).to include(escaped_branch_name) + end + end end diff --git a/spec/javascripts/vue_mr_widget/components/mr_widget_maintainer_edit_spec.js b/spec/javascripts/vue_mr_widget/components/mr_widget_maintainer_edit_spec.js new file mode 100644 index 00000000000..65b3f721281 --- /dev/null +++ b/spec/javascripts/vue_mr_widget/components/mr_widget_maintainer_edit_spec.js @@ -0,0 +1,23 @@ +import Vue from 'vue'; +import maintainerEditComponent from '~/vue_merge_request_widget/components/mr_widget_maintainer_edit.vue'; +import mountComponent from 'spec/helpers/vue_mount_component_helper'; + +describe('MRWidgetAuthor', () => { + let vm; + + beforeEach(() => { + const Component = Vue.extend(maintainerEditComponent); + + vm = mountComponent(Component, { + maintainerEditAllowed: true, + }); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('renders the message when maintainers are allowed to edit', () => { + expect(vm.$el.textContent.trim()).toEqual('Allows edits from maintainers'); + }); +}); diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index 18ba34b55a5..ebe151ac3b1 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -349,6 +349,7 @@ describe('mrWidgetOptions', () => { expect(comps['mr-widget-pipeline-blocked']).toBeDefined(); expect(comps['mr-widget-pipeline-failed']).toBeDefined(); expect(comps['mr-widget-merge-when-pipeline-succeeds']).toBeDefined(); + expect(comps['mr-widget-maintainer-edit']).toBeDefined(); }); }); diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index b49ddbfc780..48e9902027c 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -30,9 +30,10 @@ describe Gitlab::Checks::ChangeAccess do end end - context 'when the user is not allowed to push code' do + context 'when the user is not allowed to push to the repo' do it 'raises an error' do expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false) + expect(user_access).to receive(:can_push_to_branch?).with('master').and_return(false) expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.') end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index b20cc34dd5c..bece82e531a 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -278,6 +278,7 @@ project: - custom_attributes - lfs_file_locks - project_badges +- source_of_merge_requests award_emoji: - awardable - user diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index ddcbb7a0033..0b938892da5 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -168,6 +168,7 @@ MergeRequest: - last_edited_by_id - head_pipeline_id - discussion_locked +- allow_maintainer_to_push MergeRequestDiff: - id - state diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index 7280acb6c82..40c8286b1b9 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Gitlab::UserAccess do + include ProjectForksHelper + let(:access) { described_class.new(user, project: project) } let(:project) { create(:project, :repository) } let(:user) { create(:user) } @@ -118,6 +120,39 @@ describe Gitlab::UserAccess do end end + describe 'allowing pushes to maintainers of forked projects' do + let(:canonical_project) { create(:project, :public, :repository) } + let(:project) { fork_project(canonical_project, create(:user), repository: true) } + + before do + create( + :merge_request, + target_project: canonical_project, + source_project: project, + source_branch: 'awesome-feature', + allow_maintainer_to_push: true + ) + end + + it 'allows users that have push access to the canonical project to push to the MR branch' do + canonical_project.add_developer(user) + + expect(access.can_push_to_branch?('awesome-feature')).to be_truthy + end + + it 'does not allow the user to push to other branches' do + canonical_project.add_developer(user) + + expect(access.can_push_to_branch?('master')).to be_falsey + end + + it 'does not allow the user to push if he does not have push access to the canonical project' do + canonical_project.add_guest(user) + + expect(access.can_push_to_branch?('awesome-feature')).to be_falsey + end + end + describe 'merge to protected branch if allowed for developers' do before do @branch = create :protected_branch, :developers_can_merge, project: project diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 243eeddc7a8..7986aa31e16 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2084,4 +2084,82 @@ describe MergeRequest do it_behaves_like 'checking whether a rebase is in progress' end end + + describe '#allow_maintainer_to_push' do + let(:merge_request) do + build(:merge_request, source_branch: 'fixes', allow_maintainer_to_push: true) + end + + it 'is false when pushing by a maintainer is not possible' do + expect(merge_request).to receive(:maintainer_push_possible?) { false } + + expect(merge_request.allow_maintainer_to_push).to be_falsy + end + + it 'is true when pushing by a maintainer is possible' do + expect(merge_request).to receive(:maintainer_push_possible?) { true } + + expect(merge_request.allow_maintainer_to_push).to be_truthy + end + end + + describe '#maintainer_push_possible?' do + let(:merge_request) do + build(:merge_request, source_branch: 'fixes') + end + + before do + allow(ProtectedBranch).to receive(:protected?) { false } + end + + it 'does not allow maintainer to push if the source project is the same as the target' do + merge_request.target_project = merge_request.source_project = create(:project, :public) + + expect(merge_request.maintainer_push_possible?).to be_falsy + end + + it 'allows maintainer to push when both source and target are public' do + merge_request.target_project = build(:project, :public) + merge_request.source_project = build(:project, :public) + + expect(merge_request.maintainer_push_possible?).to be_truthy + end + + it 'is not available for protected branches' do + merge_request.target_project = build(:project, :public) + merge_request.source_project = build(:project, :public) + + expect(ProtectedBranch).to receive(:protected?) + .with(merge_request.source_project, 'fixes') + .and_return(true) + + expect(merge_request.maintainer_push_possible?).to be_falsy + end + end + + describe '#can_allow_maintainer_to_push?' do + let(:target_project) { create(:project, :public) } + let(:source_project) { fork_project(target_project) } + let(:merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'fixes', + target_project: target_project) + end + let(:user) { create(:user) } + + before do + allow(merge_request).to receive(:maintainer_push_possible?) { true } + end + + it 'is false if the user does not have push access to the source project' do + expect(merge_request.can_allow_maintainer_to_push?(user)).to be_falsy + end + + it 'is true when the user has push access to the source project' do + source_project.add_developer(user) + + expect(merge_request.can_allow_maintainer_to_push?(user)).to be_truthy + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b1c9e6754b9..e970cd7dfdb 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe Project do + include ProjectForksHelper + describe 'associations' do it { is_expected.to belong_to(:group) } it { is_expected.to belong_to(:namespace) } @@ -3378,4 +3380,103 @@ describe Project do end end end + + context 'with cross project merge requests' do + let(:user) { create(:user) } + let(:target_project) { create(:project, :repository) } + let(:project) { fork_project(target_project, nil, repository: true) } + let!(:merge_request) do + create( + :merge_request, + target_project: target_project, + target_branch: 'target-branch', + source_project: project, + source_branch: 'awesome-feature-1', + allow_maintainer_to_push: true + ) + end + + before do + target_project.add_developer(user) + end + + describe '#merge_requests_allowing_push_to_user' do + it 'returns open merge requests for which the user has developer access to the target project' do + expect(project.merge_requests_allowing_push_to_user(user)).to include(merge_request) + end + + it 'does not include closed merge requests' do + merge_request.close + + expect(project.merge_requests_allowing_push_to_user(user)).to be_empty + end + + it 'does not include merge requests for guest users' do + guest = create(:user) + target_project.add_guest(guest) + + expect(project.merge_requests_allowing_push_to_user(guest)).to be_empty + end + + it 'does not include the merge request for other users' do + other_user = create(:user) + + expect(project.merge_requests_allowing_push_to_user(other_user)).to be_empty + end + + it 'is empty when no user is passed' do + expect(project.merge_requests_allowing_push_to_user(nil)).to be_empty + end + end + + describe '#branch_allows_maintainer_push?' do + it 'allows access if the user can merge the merge request' do + expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1')) + .to be_truthy + end + + it 'does not allow guest users access' do + guest = create(:user) + target_project.add_guest(guest) + + expect(project.branch_allows_maintainer_push?(guest, 'awesome-feature-1')) + .to be_falsy + end + + it 'does not allow access to branches for which the merge request was closed' do + create(:merge_request, :closed, + target_project: target_project, + target_branch: 'target-branch', + source_project: project, + source_branch: 'rejected-feature-1', + allow_maintainer_to_push: true) + + expect(project.branch_allows_maintainer_push?(user, 'rejected-feature-1')) + .to be_falsy + end + + it 'does not allow access if the user cannot merge the merge request' do + create(:protected_branch, :masters_can_push, project: target_project, name: 'target-branch') + + expect(project.branch_allows_maintainer_push?(user, 'awesome-feature-1')) + .to be_falsy + end + + it 'caches the result' do + control = ActiveRecord::QueryRecorder.new { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') } + + expect { 3.times { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') } } + .not_to exceed_query_limit(control) + end + + context 'when the requeststore is active', :request_store do + it 'only queries per project across instances' do + control = ActiveRecord::QueryRecorder.new { project.branch_allows_maintainer_push?(user, 'awesome-feature-1') } + + expect { 2.times { described_class.find(project.id).branch_allows_maintainer_push?(user, 'awesome-feature-1') } } + .not_to exceed_query_limit(control).with_threshold(2) + end + end + end + end end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index 129344f105f..ea76e604153 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -308,4 +308,41 @@ describe ProjectPolicy do it_behaves_like 'project policies as master' it_behaves_like 'project policies as owner' it_behaves_like 'project policies as admin' + + context 'when a public project has merge requests allowing access' do + include ProjectForksHelper + let(:user) { create(:user) } + let(:target_project) { create(:project, :public) } + let(:project) { fork_project(target_project) } + let!(:merge_request) do + create( + :merge_request, + target_project: target_project, + source_project: project, + allow_maintainer_to_push: true + ) + end + let(:maintainer_abilities) do + %w(create_build update_build create_pipeline update_pipeline) + end + + subject { described_class.new(user, project) } + + it 'does not allow pushing code' do + expect_disallowed(*maintainer_abilities) + end + + it 'allows pushing if the user is a member with push access to the target project' do + target_project.add_developer(user) + + expect_allowed(*maintainer_abilities) + end + + it 'dissallows abilities to a maintainer if the merge request was closed' do + target_project.add_developer(user) + merge_request.close! + + expect_disallowed(*maintainer_abilities) + end + end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 484322752c0..3764aec0c71 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -616,6 +616,25 @@ describe API::MergeRequests do expect(json_response['changes_count']).to eq('5+') end end + + context 'for forked projects' do + let(:user2) { create(:user) } + let(:project) { create(:project, :public, :repository) } + let(:forked_project) { fork_project(project, user2, repository: true) } + let(:merge_request) do + create(:merge_request, + source_project: forked_project, + target_project: project, + source_branch: 'fixes', + allow_maintainer_to_push: true) + end + + it 'includes the `allow_maintainer_to_push` field' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user) + + expect(json_response['allow_maintainer_to_push']).to be_truthy + end + end end describe 'GET /projects/:id/merge_requests/:merge_request_iid/participants' do @@ -815,6 +834,7 @@ describe API::MergeRequests do context 'forked projects' do let!(:user2) { create(:user) } + let(:project) { create(:project, :public, :repository) } let!(:forked_project) { fork_project(project, user2, repository: true) } let!(:unrelated_project) { create(:project, namespace: create(:user).namespace, creator_id: user2.id) } @@ -872,6 +892,14 @@ describe API::MergeRequests do expect(response).to have_gitlab_http_status(400) end + it 'allows setting `allow_maintainer_to_push`' do + post api("/projects/#{forked_project.id}/merge_requests", user2), + title: 'Test merge_request', source_branch: "feature_conflict", target_branch: "master", + author: user2, target_project_id: project.id, allow_maintainer_to_push: true + expect(response).to have_gitlab_http_status(201) + expect(json_response['allow_maintainer_to_push']).to be_truthy + end + context 'when target_branch and target_project_id is specified' do let(:params) do { title: 'Test merge_request', diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index c31259239ee..5279ea6164e 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe MergeRequests::UpdateService, :mailer do + include ProjectForksHelper + let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:user2) { create(:user) } @@ -538,5 +540,40 @@ describe MergeRequests::UpdateService, :mailer do let(:open_issuable) { merge_request } let(:closed_issuable) { create(:closed_merge_request, source_project: project) } end + + context 'setting `allow_maintainer_to_push`' do + let(:target_project) { create(:project, :public) } + let(:source_project) { fork_project(target_project) } + let(:user) { create(:user) } + let(:merge_request) do + create(:merge_request, + source_project: source_project, + source_branch: 'fixes', + target_project: target_project) + end + + before do + allow(ProtectedBranch).to receive(:protected?).with(source_project, 'fixes') { false } + end + + it 'does not allow a maintainer of the target project to set `allow_maintainer_to_push`' do + target_project.add_developer(user) + + update_merge_request(allow_maintainer_to_push: true, title: 'Updated title') + + expect(merge_request.title).to eq('Updated title') + expect(merge_request.allow_maintainer_to_push).to be_falsy + end + + it 'is allowed by a user that can push to the source and can update the merge request' do + merge_request.update!(assignee: user) + source_project.add_developer(user) + + update_merge_request(allow_maintainer_to_push: true, title: 'Updated title') + + expect(merge_request.title).to eq('Updated title') + expect(merge_request.allow_maintainer_to_push).to be_truthy + end + end end end diff --git a/spec/views/projects/tree/show.html.haml_spec.rb b/spec/views/projects/tree/show.html.haml_spec.rb index 44b32df0395..3b098320ad7 100644 --- a/spec/views/projects/tree/show.html.haml_spec.rb +++ b/spec/views/projects/tree/show.html.haml_spec.rb @@ -13,6 +13,7 @@ describe 'projects/tree/show' do allow(view).to receive(:can?).and_return(true) allow(view).to receive(:can_collaborate_with_project?).and_return(true) + allow(view).to receive_message_chain('user_access.can_push_to_branch?').and_return(true) allow(view).to receive(:current_application_settings).and_return(Gitlab::CurrentSettings.current_application_settings) end |