Merge remote-tracking branch 'origin/master' into 21170-cycle-analytics

13 files changed, 141 insertions, 72 deletions

diff --git a/app/services/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb
index 6072123b851..98da6563947 100644
--- a/app/services/auth/container_registry_authentication_service.rb
+++ b/app/services/auth/container_registry_authentication_service.rb
@@ -4,7 +4,9 @@ module Auth
 
     AUDIENCE = 'container_registry'
 
-    def execute
+    def execute(authentication_abilities:)
+      @authentication_abilities = authentication_abilities || []
+
       return error('not found', 404) unless registry.enabled
 
       unless current_user || project
@@ -74,9 +76,9 @@ module Auth
 
       case requested_action
       when 'pull'
-        requested_project == project || can?(current_user, :read_container_image, requested_project)
+        requested_project.public? || build_can_pull?(requested_project) || user_can_pull?(requested_project)
       when 'push'
-        requested_project == project || can?(current_user, :create_container_image, requested_project)
+        build_can_push?(requested_project) || user_can_push?(requested_project)
       else
         false
       end
@@ -85,5 +87,29 @@ module Auth
     def registry
       Gitlab.config.registry
     end
+
+    def build_can_pull?(requested_project)
+      # Build can:
+      # 1. pull from its own project (for ex. a build)
+      # 2. read images from dependent projects if creator of build is a team member
+      @authentication_abilities.include?(:build_read_container_image) &&
+        (requested_project == project || can?(current_user, :build_read_container_image, requested_project))
+    end
+
+    def user_can_pull?(requested_project)
+      @authentication_abilities.include?(:read_container_image) &&
+        can?(current_user, :read_container_image, requested_project)
+    end
+
+    def build_can_push?(requested_project)
+      # Build can push only to the project from which it originates
+      @authentication_abilities.include?(:build_create_container_image) &&
+        requested_project == project
+    end
+
+    def user_can_push?(requested_project)
+      @authentication_abilities.include?(:create_container_image) &&
+        can?(current_user, :create_container_image, requested_project)
+    end
   end
 end
diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb
index de48a50774e..36c93dddadb 100644
--- a/app/services/ci/process_pipeline_service.rb
+++ b/app/services/ci/process_pipeline_service.rb
@@ -31,13 +31,13 @@ module Ci
       current_status = status_for_prior_stages(index)
 
       created_builds_in_stage(index).select do |build|
-        process_build(build, current_status)
+        if HasStatus::COMPLETED_STATUSES.include?(current_status)
+          process_build(build, current_status)
+        end
       end
     end
 
     def process_build(build, current_status)
-      return false unless HasStatus::COMPLETED_STATUSES.include?(current_status)
-
       if valid_statuses_for_when(build.when).include?(current_status)
         build.enqueue
         true
diff --git a/app/services/commits/change_service.rb b/app/services/commits/change_service.rb
index ed73d8cb8c2..1c82599c579 100644
--- a/app/services/commits/change_service.rb
+++ b/app/services/commits/change_service.rb
@@ -16,11 +16,29 @@ module Commits
       error(ex.message)
     end
 
+    private
+
     def commit
       raise NotImplementedError
     end
 
-    private
+    def commit_change(action)
+      raise NotImplementedError unless repository.respond_to?(action)
+
+      into = @create_merge_request ? @commit.public_send("#{action}_branch_name") : @target_branch
+      tree_id = repository.public_send("check_#{action}_content", @commit, @target_branch)
+
+      if tree_id
+        create_target_branch(into) if @create_merge_request
+
+        repository.public_send(action, current_user, @commit, into, tree_id)
+        success
+      else
+        error_msg = "Sorry, we cannot #{action.to_s.dasherize} this #{@commit.change_type_title} automatically.
+                     It may have already been #{action.to_s.dasherize}, or a more recent commit may have updated some of its content."
+        raise ChangeError, error_msg
+      end
+    end
 
     def check_push_permissions
       allowed = ::Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(@target_branch)
diff --git a/app/services/commits/cherry_pick_service.rb b/app/services/commits/cherry_pick_service.rb
index f9a4efa7182..605cca36f9c 100644
--- a/app/services/commits/cherry_pick_service.rb
+++ b/app/services/commits/cherry_pick_service.rb
@@ -1,19 +1,7 @@
 module Commits
   class CherryPickService < ChangeService
     def commit
-      cherry_pick_into = @create_merge_request ? @commit.cherry_pick_branch_name : @target_branch
-      cherry_pick_tree_id = repository.check_cherry_pick_content(@commit, @target_branch)
-
-      if cherry_pick_tree_id
-        create_target_branch(cherry_pick_into) if @create_merge_request
-
-        repository.cherry_pick(current_user, @commit, cherry_pick_into, cherry_pick_tree_id)
-        success
-      else
-        error_msg = "Sorry, we cannot cherry-pick this #{@commit.change_type_title} automatically.
-                     It may have already been cherry-picked, or a more recent commit may have updated some of its content."
-        raise ChangeError, error_msg
-      end
+      commit_change(:cherry_pick)
     end
   end
 end
diff --git a/app/services/commits/revert_service.rb b/app/services/commits/revert_service.rb
index c7de9f6f35e..addd55cb32f 100644
--- a/app/services/commits/revert_service.rb
+++ b/app/services/commits/revert_service.rb
@@ -1,19 +1,7 @@
 module Commits
   class RevertService < ChangeService
     def commit
-      revert_into = @create_merge_request ? @commit.revert_branch_name : @target_branch
-      revert_tree_id = repository.check_revert_content(@commit, @target_branch)
-
-      if revert_tree_id
-        create_target_branch(revert_into) if @create_merge_request
-
-        repository.revert(current_user, @commit, revert_into, revert_tree_id)
-        success
-      else
-        error_msg = "Sorry, we cannot revert this #{@commit.change_type_title} automatically.
-                     It may have already been reverted, or a more recent commit may have updated some of its content."
-        raise ChangeError, error_msg
-      end
+      commit_change(:revert)
     end
   end
 end
diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb
index c126b2fad31..aad5cc8a15b 100644
--- a/app/services/create_deployment_service.rb
+++ b/app/services/create_deployment_service.rb
@@ -2,9 +2,7 @@ require_relative 'base_service'
 
 class CreateDeploymentService < BaseService
   def execute(deployable = nil)
-    environment = project.environments.find_or_create_by(
-      name: params[:environment]
-    )
+    environment = find_or_create_environment
 
     deployment = project.deployments.create(
       environment: environment,
@@ -45,4 +43,38 @@ class CreateDeploymentService < BaseService
       where.not(id: current_deployment.id).
       first
   end
+
+  private
+
+  def find_or_create_environment
+    project.environments.find_or_create_by(name: expanded_name) do |environment|
+      environment.external_url = expanded_url
+    end
+  end
+
+  def expanded_name
+    ExpandVariables.expand(name, variables)
+  end
+
+  def expanded_url
+    return unless url
+
+    @expanded_url ||= ExpandVariables.expand(url, variables)
+  end
+
+  def name
+    params[:environment]
+  end
+
+  def url
+    options[:url]
+  end
+
+  def options
+    params[:options] || {}
+  end
+
+  def variables
+    params[:variables] || []
+  end
 end
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index 2aab67b4b9d..cde993221ff 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -87,7 +87,7 @@ class GitPushService < BaseService
     project.change_head(branch_name)
 
     # Set protection on the default branch if configured
-    if current_application_settings.default_branch_protection != PROTECTION_NONE
+    if current_application_settings.default_branch_protection != PROTECTION_NONE && !@project.protected_branch?(@project.default_branch)
 
       params = {
         name: @project.default_branch,
diff --git a/app/services/issuable/bulk_update_service.rb b/app/services/issuable/bulk_update_service.rb
new file mode 100644
index 00000000000..60891cbb255
--- /dev/null
+++ b/app/services/issuable/bulk_update_service.rb
@@ -0,0 +1,26 @@
+module Issuable
+  class BulkUpdateService < IssuableBaseService
+    def execute(type)
+      model_class = type.classify.constantize
+      update_class = type.classify.pluralize.constantize::UpdateService
+
+      ids = params.delete(:issuable_ids).split(",")
+      items = model_class.where(id: ids)
+
+      %i(state_event milestone_id assignee_id add_label_ids remove_label_ids subscription_event).each do |key|
+        params.delete(key) unless params[key].present?
+      end
+
+      items.each do |issuable|
+        next unless can?(current_user, :"update_#{type}", issuable)
+
+        update_class.new(issuable.project, current_user, params).execute(issuable)
+      end
+
+      {
+        count:    items.count,
+        success:  !items.count.zero?
+      }
+    end
+  end
+end
diff --git a/app/services/issues/bulk_update_service.rb b/app/services/issues/bulk_update_service.rb
deleted file mode 100644
index 7e19a73f71a..00000000000
--- a/app/services/issues/bulk_update_service.rb
+++ /dev/null
@@ -1,25 +0,0 @@
-module Issues
-  class BulkUpdateService < BaseService
-    def execute
-      issues_ids   = params.delete(:issues_ids).split(",")
-      issue_params = params
-
-      %i(state_event milestone_id assignee_id add_label_ids remove_label_ids subscription_event).each do |key|
-        issue_params.delete(key) unless issue_params[key].present?
-      end
-
-      issues = Issue.where(id: issues_ids)
-
-      issues.each do |issue|
-        next unless can?(current_user, :update_issue, issue)
-
-        Issues::UpdateService.new(issue.project, current_user, issue_params).execute(issue)
-      end
-
-      {
-        count:    issues.count,
-        success:  !issues.count.zero?
-      }
-    end
-  end
-end
diff --git a/app/services/milestones/create_service.rb b/app/services/milestones/create_service.rb
index 3b90399af64..b8e08c9f1eb 100644
--- a/app/services/milestones/create_service.rb
+++ b/app/services/milestones/create_service.rb
@@ -3,7 +3,7 @@ module Milestones
     def execute
       milestone = project.milestones.new(params)
 
-      if milestone.save!
+      if milestone.save
         event_service.open_milestone(milestone, current_user)
       end
 
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 8a53f65aec1..a08c6fcd94b 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -27,6 +27,8 @@ module Projects
       # Git data (e.g. a list of branch names).
       flush_caches(project, wiki_path)
 
+      Projects::UnlinkForkService.new(project, current_user).execute
+
       Project.transaction do
         project.destroy!
 
diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb
index 29b3981f49f..c3dfc8cfbe8 100644
--- a/app/services/projects/housekeeping_service.rb
+++ b/app/services/projects/housekeeping_service.rb
@@ -30,10 +30,8 @@ module Projects
     end
 
     def increment!
-      if Gitlab::ExclusiveLease.new("project_housekeeping:increment!:#{@project.id}", timeout: 60).try_obtain
-        Gitlab::Metrics.measure(:increment_pushes_since_gc) do
-          update_pushes_since_gc(@project.pushes_since_gc + 1)
-        end
+      Gitlab::Metrics.measure(:increment_pushes_since_gc) do
+        @project.increment_pushes_since_gc
       end
     end
 
@@ -43,14 +41,10 @@ module Projects
       GitGarbageCollectWorker.perform_async(@project.id)
     ensure
       Gitlab::Metrics.measure(:reset_pushes_since_gc) do
-        update_pushes_since_gc(0)
+        @project.reset_pushes_since_gc
       end
     end
 
-    def update_pushes_since_gc(new_value)
-      @project.update_column(:pushes_since_gc, new_value)
-    end
-
     def try_obtain_lease
       Gitlab::Metrics.measure(:obtain_housekeeping_lease) do
         lease = ::Gitlab::ExclusiveLease.new("project_housekeeping:#{@project.id}", timeout: LEASE_TIMEOUT)
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 2aab8c736d6..776530ac0a5 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -31,6 +31,14 @@ class TodoService
     mark_pending_todos_as_done(issue, current_user)
   end
 
+  # When we destroy an issue we should:
+  #
+  #  * refresh the todos count cache for the current user
+  #
+  def destroy_issue(issue, current_user)
+    destroy_issuable(issue, current_user)
+  end
+
   # When we reassign an issue we should:
   #
   #  * create a pending todo for new assignee if issue is assigned
@@ -64,6 +72,14 @@ class TodoService
     mark_pending_todos_as_done(merge_request, current_user)
   end
 
+  # When we destroy a merge request we should:
+  #
+  #  * refresh the todos count cache for the current user
+  #
+  def destroy_merge_request(merge_request, current_user)
+    destroy_issuable(merge_request, current_user)
+  end
+
   # When we reassign a merge request we should:
   #
   #  * creates a pending todo for new assignee if merge request is assigned
@@ -187,6 +203,10 @@ class TodoService
     create_mention_todos(issuable.project, issuable, author)
   end
 
+  def destroy_issuable(issuable, user)
+    user.update_todos_count_cache
+  end
+
   def toggling_tasks?(issuable)
     issuable.previous_changes.include?('description') &&
       issuable.tasks? && issuable.updated_tasks.any?