Make the CI permission model simpler

This MR simplifies CI permission model:
- read_build: allows to read a list of builds, artifacts and trace
- update_build: allows to cancel and retry builds
- create_build: allows to create builds from gitlab-ci.yml (not yet implemented)
- admin_build: allows to manage triggers, runners and variables
- read_commit_status: allows to read a list of commit statuses (including the overall of builds)
- create_commit_status: allows to create a new commit status using API

Remove all extra methods to manage permission.
Made all controllers to use explicitly the new permissions.

15 files changed, 48 insertions, 52 deletions

diff --git a/app/controllers/ci/application_controller.rb b/app/controllers/ci/application_controller.rb
index c420b59c3a2..59c77653509 100644
--- a/app/controllers/ci/application_controller.rb
+++ b/app/controllers/ci/application_controller.rb
@@ -13,7 +13,7 @@ module Ci
     end
 
     def authorize_manage_builds!
-      unless can?(current_user, :manage_builds, project)
+      unless can?(current_user, :update_build, project)
         return page_404
       end
     end
diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index f159a6d6dc6..cfea1266516 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -1,6 +1,6 @@
 class Projects::ArtifactsController < Projects::ApplicationController
   layout 'project'
-  before_action :authorize_read_build_artifacts!
+  before_action :authorize_read_build!
 
   def download
     unless artifacts_file.file_storage?
@@ -43,14 +43,4 @@ class Projects::ArtifactsController < Projects::ApplicationController
   def artifacts_file
     @artifacts_file ||= build.artifacts_file
   end
-
-  def authorize_read_build_artifacts!
-    unless can?(current_user, :read_build_artifacts, @project)
-      if current_user.nil?
-        return authenticate_user!
-      else
-        return render_404
-      end
-    end
-  end
 end
diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb
index 92d9699fe84..9e89296e71d 100644
--- a/app/controllers/projects/builds_controller.rb
+++ b/app/controllers/projects/builds_controller.rb
@@ -1,7 +1,8 @@
 class Projects::BuildsController < Projects::ApplicationController
   before_action :build, except: [:index, :cancel_all]
 
-  before_action :authorize_manage_builds!, except: [:index, :show, :status]
+  before_action :authorize_read_build!, except: [:cancel, :cancel_all, :retry]
+  before_action :authorize_update_build!, except: [:index, :show, :status]
 
   layout "project"
 
@@ -69,10 +70,4 @@ class Projects::BuildsController < Projects::ApplicationController
   def build_path(build)
     namespace_project_build_path(build.project.namespace, build.project, build)
   end
-
-  def authorize_manage_builds!
-    unless can?(current_user, :manage_builds, project)
-      return render_404
-    end
-  end
 end
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index f5a169e5aa9..2bf367d2a25 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -4,10 +4,10 @@
 class Projects::CommitController < Projects::ApplicationController
   # Authorize
   before_action :require_non_empty_project
-  before_action :authorize_download_code!, except: [:cancel_builds]
-  before_action :authorize_manage_builds!, only: [:cancel_builds]
+  before_action :authorize_download_code!, except: [:cancel_builds, :retry_builds]
+  before_action :authorize_update_build!, only: [:cancel_builds, :retry_builds]
+  before_action :authorize_read_commit_status!, only: [:builds]
   before_action :commit
-  before_action :authorize_manage_builds!, only: [:cancel_builds, :retry_builds]
   before_action :define_show_vars, only: [:show, :builds]
 
   def show
@@ -77,10 +77,4 @@ class Projects::CommitController < Projects::ApplicationController
 
     @statuses = ci_commit.statuses if ci_commit
   end
-
-  def authorize_manage_builds!
-    unless can?(current_user, :manage_builds, project)
-      return render_404
-    end
-  end
 end
diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb
index e2785caa2fb..bedeb4a295c 100644
--- a/app/controllers/projects/runner_projects_controller.rb
+++ b/app/controllers/projects/runner_projects_controller.rb
@@ -1,5 +1,5 @@
 class Projects::RunnerProjectsController < Projects::ApplicationController
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_build!
 
   layout 'project_settings'
 
diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb
index 4993b2648a5..0dd2d6a99be 100644
--- a/app/controllers/projects/runners_controller.rb
+++ b/app/controllers/projects/runners_controller.rb
@@ -1,6 +1,6 @@
 class Projects::RunnersController < Projects::ApplicationController
+  before_action :authorize_admin_build!
   before_action :set_runner, only: [:edit, :update, :destroy, :pause, :resume, :show]
-  before_action :authorize_admin_project!
 
   layout 'project_settings'
 
diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb
index 30adfad1daa..92359745cec 100644
--- a/app/controllers/projects/triggers_controller.rb
+++ b/app/controllers/projects/triggers_controller.rb
@@ -1,5 +1,5 @@
 class Projects::TriggersController < Projects::ApplicationController
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_build!
 
   layout 'project_settings'
 
diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb
index 10efafea9db..00234654578 100644
--- a/app/controllers/projects/variables_controller.rb
+++ b/app/controllers/projects/variables_controller.rb
@@ -1,5 +1,5 @@
 class Projects::VariablesController < Projects::ApplicationController
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_build!
 
   layout 'project_settings'
 
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 8c8b355028c..4543eff0d63 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -126,7 +126,7 @@ module ProjectsHelper
       nav_tabs << :merge_requests
     end
 
-    if project.builds_enabled? && can?(current_user, :read_build, project)
+    if can?(current_user, :read_build, project)
       nav_tabs << :builds
     end
 
diff --git a/app/models/ability.rb b/app/models/ability.rb
index ab59a3506a2..e58e7a40273 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -52,10 +52,15 @@ class Ability
           :read_project_member,
           :read_merge_request,
           :read_note,
+          :read_commit_status,
           :read_build,
           :download_code
         ]
 
+        if project.restrict_builds?
+          rules -= :read_build
+        end
+
         rules - project_disabled_features_rules(project)
       else
         []
@@ -113,6 +118,10 @@ class Ability
 
         if project.public? || project.internal?
           rules.push(*public_project_rules)
+
+          if team.guest?(user) && project.restrict_builds?
+            rules -= named_abilities('build')
+          end
         end
 
         if project.owner == user || user.admin?
@@ -134,7 +143,9 @@ class Ability
     def public_project_rules
       @public_project_rules ||= project_guest_rules + [
         :download_code,
-        :fork_project
+        :fork_project,
+        :read_commit_status,
+        :read_build,
       ]
     end
 
@@ -149,7 +160,7 @@ class Ability
         :read_project_member,
         :read_merge_request,
         :read_note,
-        :read_build,
+        :read_commit_status,
         :create_project,
         :create_issue,
         :create_note
@@ -158,24 +169,25 @@ class Ability
 
     def project_report_rules
       @project_report_rules ||= project_guest_rules + [
-        :create_commit_status,
-        :read_commit_statuses,
-        :read_build_artifacts,
         :download_code,
         :fork_project,
         :create_project_snippet,
         :update_issue,
         :admin_issue,
-        :admin_label
+        :admin_label,
+        :read_build,
       ]
     end
 
     def project_dev_rules
       @project_dev_rules ||= project_report_rules + [
         :admin_merge_request,
+        :create_commit_status,
+        :update_commit_status,
+        :create_build,
+        :update_build,
         :create_merge_request,
         :create_wiki,
-        :manage_builds,
         :push_code
       ]
     end
@@ -201,7 +213,9 @@ class Ability
         :admin_merge_request,
         :admin_note,
         :admin_wiki,
-        :admin_project
+        :admin_project,
+        :admin_commit_status,
+        :admin_build
       ]
     end
 
@@ -240,6 +254,10 @@ class Ability
         rules += named_abilities('wiki')
       end
 
+      unless project.builds_enabled
+        rules += named_abilities('build')
+      end
+
       rules
     end
 
diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml
index c395bd908c3..bd7fb0b36f4 100644
--- a/app/views/admin/builds/_build.html.haml
+++ b/app/views/admin/builds/_build.html.haml
@@ -4,7 +4,7 @@
     = ci_status_with_icon(build.status)
 
   %td.build-link
-    - if build.target_url
+    - if can?(current_user, :read_build, project) && build.target_url
       = link_to build.target_url do
         %strong Build ##{build.id}
     - else
@@ -60,10 +60,10 @@
 
   %td
     .pull-right
-      - if current_user && can?(current_user, :read_build_artifacts, project) && build.artifacts?
+      - if can?(current_user, :read_build, project) && build.artifacts?
         = link_to build.artifacts_download_url, title: 'Download artifacts' do
           %i.fa.fa-download
-      - if current_user && can?(current_user, :manage_builds, build.project)
+      - if current_user && can?(current_user, :update_build, build.project)
         - if build.active?
           - if build.cancel_url
             = link_to build.cancel_url, method: :post, title: 'Cancel' do
diff --git a/app/views/projects/builds/index.html.haml b/app/views/projects/builds/index.html.haml
index bbb6944a65a..2a2a4745bed 100644
--- a/app/views/projects/builds/index.html.haml
+++ b/app/views/projects/builds/index.html.haml
@@ -3,7 +3,7 @@
 
 .project-issuable-filter
   .controls
-    - if can?(current_user, :manage_builds, @project)
+    - if can?(current_user, :update_build, @project)
       .pull-left.hidden-xs
         - if @all_builds.running_or_pending.any?
           = link_to 'Cancel running', cancel_all_namespace_project_builds_path(@project.namespace, @project),
diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml
index ba1fdc6f0e7..c43d6c3b427 100644
--- a/app/views/projects/builds/show.html.haml
+++ b/app/views/projects/builds/show.html.haml
@@ -89,8 +89,7 @@
             Test coverage
           %h1 #{@build.coverage}%
 
-      - if current_user && can?(current_user, :read_build_artifacts, @project) && @build.artifacts?
-
+      - if can?(current_user, :read_build, @project) && @build.artifacts?
         .build-widget.artifacts
           %h4.title Build artifacts
           .center
@@ -102,7 +101,7 @@
       .build-widget
         %h4.title
           Build ##{@build.id}
-          - if current_user && can?(current_user, :manage_builds, @project)
+          - if current_user && can?(current_user, :update_build, @project)
             .pull-right
               - if @build.cancel_url
                 = link_to "Cancel", @build.cancel_url, class: 'btn btn-sm btn-danger', method: :post
diff --git a/app/views/projects/commit/_builds.html.haml b/app/views/projects/commit/_builds.html.haml
index 329aaa0bb8b..befad27666c 100644
--- a/app/views/projects/commit/_builds.html.haml
+++ b/app/views/projects/commit/_builds.html.haml
@@ -1,6 +1,6 @@
 .gray-content-block.middle-block
   .pull-right
-    - if can?(current_user, :manage_builds, @ci_commit.project)
+    - if can?(current_user, :update_build, @ci_commit.project)
       - if @ci_commit.builds.latest.failed.any?(&:retryable?)
         = link_to "Retry failed", retry_builds_namespace_project_commit_path(@ci_commit.project.namespace, @ci_commit.project, @ci_commit.sha), class: 'btn btn-grouped btn-primary', method: :post
 
diff --git a/app/views/projects/commit_statuses/_commit_status.html.haml b/app/views/projects/commit_statuses/_commit_status.html.haml
index 2e3c956ddc4..fba4405cb7d 100644
--- a/app/views/projects/commit_statuses/_commit_status.html.haml
+++ b/app/views/projects/commit_statuses/_commit_status.html.haml
@@ -8,7 +8,7 @@
       = ci_status_with_icon(commit_status.status)
 
   %td.commit_status-link
-    - if commit_status.target_url
+    - if can?(current_user, :read_build, commit_status.project) && commit_status.target_url
       = link_to commit_status.target_url do
         %strong ##{commit_status.id}
     - else
@@ -66,10 +66,10 @@
 
   %td
     .pull-right
-      - if current_user && can?(current_user, :read_build_artifacts, commit_status.project) && commit_status.artifacts_download_url
+      - if can?(current_user, :read_build, commit_status.project) && commit_status.artifacts_download_url
         = link_to commit_status.artifacts_download_url, title: 'Download artifacts' do
           %i.fa.fa-download
-      - if current_user && can?(current_user, :manage_builds, commit_status.project)
+      - if can?(current_user, :update_build, commit_status.project)
         - if commit_status.active?
           - if commit_status.cancel_url
             = link_to commit_status.cancel_url, method: :post, title: 'Cancel' do