Merge branch 'ci-permissions' into 'master'

Limit guest access to builds

Closes https://dev.gitlab.org/gitlab/gitlabhq/issues/2646

See merge request !1942

app/controllers/ci/application_controller.rb

@@ -3,52 +3,5 @@ module Ci
     def self.railtie_helpers_paths
       "app/helpers/ci"
     end
-
-    private
-
-    def authorize_access_project!
-      unless can?(current_user, :read_project, project)
-        return page_404
-      end
-    end
-
-    def authorize_manage_builds!
-      unless can?(current_user, :manage_builds, project)
-        return page_404
-      end
-    end
-
-    def authenticate_admin!
-      return render_404 unless current_user.is_admin?
-    end
-
-    def authorize_manage_project!
-      unless can?(current_user, :admin_project, project)
-        return page_404
-      end
-    end
-
-    def page_404
-      render file: "#{Rails.root}/public/404.html", status: 404, layout: false
-    end
-
-    def default_headers
-      headers['X-Frame-Options'] = 'DENY'
-      headers['X-XSS-Protection'] = '1; mode=block'
-    end
-
-    # JSON for infinite scroll via Pager object
-    def pager_json(partial, count)
-      html = render_to_string(
-        partial,
-        layout: false,
-        formats: [:html]
-      )
-
-      render json: {
-        html: html,
-        count: count
-      }
-    end
   end
 end

app/controllers/ci/projects_controller.rb

 module Ci
   class ProjectsController < Ci::ApplicationController
-    before_action :project, except: [:index]
-    before_action :authenticate_user!, except: [:index, :build, :badge]
-    before_action :authorize_access_project!, except: [:index, :badge]
+    before_action :project
+    before_action :authorize_read_project!, except: [:badge]
     before_action :no_cache, only: [:badge]
     protect_from_forgery
 

app/controllers/projects/artifacts_controller.rb

 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

app/controllers/projects/builds_controller.rb

 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

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

app/controllers/projects/runner_projects_controller.rb

 class Projects::RunnerProjectsController < Projects::ApplicationController
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_build!
 
   layout 'project_settings'
 

app/controllers/projects/runners_controller.rb

 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'
 

app/controllers/projects/triggers_controller.rb

 class Projects::TriggersController < Projects::ApplicationController
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_build!
 
   layout 'project_settings'
 

app/controllers/projects/variables_controller.rb

 class Projects::VariablesController < Projects::ApplicationController
-  before_action :authorize_admin_project!
+  before_action :authorize_admin_build!
 
   layout 'project_settings'
 

app/controllers/projects_controller.rb

@@ -227,6 +227,7 @@ class ProjectsController < ApplicationController
       :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch,
       :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar,
       :builds_enabled, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex,
+      :public_builds,
     )
   end
 

app/helpers/projects_helper.rb

@@ -141,7 +141,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
 

app/models/ability.rb

@@ -5,17 +5,18 @@ class Ability
       return [] unless user.is_a?(User)
       return [] if user.blocked?
 
-      case subject.class.name
-      when "Project" then project_abilities(user, subject)
-      when "Issue" then issue_abilities(user, subject)
-      when "Note" then note_abilities(user, subject)
-      when "ProjectSnippet" then project_snippet_abilities(user, subject)
-      when "PersonalSnippet" then personal_snippet_abilities(user, subject)
-      when "MergeRequest" then merge_request_abilities(user, subject)
-      when "Group" then group_abilities(user, subject)
-      when "Namespace" then namespace_abilities(user, subject)
-      when "GroupMember" then group_member_abilities(user, subject)
-      when "ProjectMember" then project_member_abilities(user, subject)
+      case subject
+      when CommitStatus then commit_status_abilities(user, subject)
+      when Project then project_abilities(user, subject)
+      when Issue then issue_abilities(user, subject)
+      when Note then note_abilities(user, subject)
+      when ProjectSnippet then project_snippet_abilities(user, subject)
+      when PersonalSnippet then personal_snippet_abilities(user, subject)
+      when MergeRequest then merge_request_abilities(user, subject)
+      when Group then group_abilities(user, subject)
+      when Namespace then namespace_abilities(user, subject)
+      when GroupMember then group_member_abilities(user, subject)
+      when ProjectMember then project_member_abilities(user, subject)
       else []
       end.concat(global_abilities(user))
     end
@@ -25,6 +26,8 @@ class Ability
       case true
       when subject.is_a?(PersonalSnippet)
         anonymous_personal_snippet_abilities(subject)
+      when subject.is_a?(CommitStatus)
+        anonymous_commit_status_abilities(subject)
       when subject.is_a?(Project) || subject.respond_to?(:project)
         anonymous_project_abilities(subject)
       when subject.is_a?(Group) || subject.respond_to?(:group)
@@ -52,16 +55,26 @@ class Ability
           :read_project_member,
           :read_merge_request,
           :read_note,
-          :read_build,
+          :read_commit_status,
           :download_code
         ]
 
+        # Allow to read builds by anonymous user if guests are allowed
+        rules << :read_build if project.public_builds?
+
         rules - project_disabled_features_rules(project)
       else
         []
       end
     end
 
+    def anonymous_commit_status_abilities(subject)
+      rules = anonymous_project_abilities(subject.project)
+      # If subject is Ci::Build which inherits from CommitStatus filter the abilities
+      rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build)
+      rules
+    end
+
     def anonymous_group_abilities(subject)
       group = if subject.is_a?(Group)
                 subject
@@ -113,6 +126,9 @@ class Ability
 
         if project.public? || project.internal?
           rules.push(*public_project_rules)
+
+          # Allow to read builds for internal projects
+          rules << :read_build if project.public_builds?
         end
 
         if project.owner == user || user.admin?
@@ -134,7 +150,8 @@ class Ability
     def public_project_rules
       @public_project_rules ||= project_guest_rules + [
         :download_code,
-        :fork_project
+        :fork_project,
+        :read_commit_status,
       ]
     end
 
@@ -149,7 +166,6 @@ class Ability
         :read_project_member,
         :read_merge_request,
         :read_note,
-        :read_build,
         :create_project,
         :create_issue,
         :create_note
@@ -158,24 +174,26 @@ 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_commit_status,
+        :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 +219,9 @@ class Ability
         :admin_merge_request,
         :admin_note,
         :admin_wiki,
-        :admin_project
+        :admin_project,
+        :admin_commit_status,
+        :admin_build
       ]
     end
 
@@ -240,6 +260,10 @@ class Ability
         rules += named_abilities('wiki')
       end
 
+      unless project.builds_enabled
+        rules += named_abilities('build')
+      end
+
       rules
     end
 
@@ -376,6 +400,22 @@ class Ability
       rules
     end
 
+    def commit_status_abilities(user, subject)
+      rules = project_abilities(user, subject.project)
+      # If subject is Ci::Build which inherits from CommitStatus filter the abilities
+      rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build)
+      rules
+    end
+
+    def filter_build_abilities(rules)
+      # If we can't read build we should also not have that
+      # ability when looking at this in context of commit_status
+      %w(read create update admin).each do |rule|
+        rules.delete(:"#{rule}_commit_status") unless rules.include?(:"#{rule}_build")
+      end
+      rules
+    end
+
     def abilities
       @abilities ||= begin
         abilities = Six.new

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 can?(current_user, :update_build, build.project)
         - if build.active?
           - if build.cancel_url
             = link_to build.cancel_url, method: :post, title: 'Cancel' do

app/views/projects/builds/index.html.haml

@@ -22,7 +22,7 @@
           = number_with_delimiter(@all_builds.finished.count(:id))
 
   .nav-controls
-    - if can?(current_user, :manage_builds, @project)
+    - if can?(current_user, :update_build, @project)
       - if @all_builds.running_or_pending.any?
         = link_to 'Cancel running', cancel_all_namespace_project_builds_path(@project.namespace, @project),
           data: { confirm: 'Are you sure?' }, class: 'btn btn-danger', method: :post

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 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

app/views/projects/commit/_builds.html.haml

 .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
 

app/views/projects/commit_statuses/_commit_status.html.haml

 %tr.commit_status
   %td.status
-    - if commit_status.target_url
+    - if can?(current_user, :read_commit_status, commit_status) && commit_status.target_url
       = link_to commit_status.target_url, class: "ci-status ci-#{commit_status.status}" do
         = ci_icon_for_status(commit_status.status)
         = commit_status.status
@@ -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_commit_status, commit_status) && 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_commit_status, commit_status) && 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_commit_status, commit_status)
         - if commit_status.active?
           - if commit_status.cancel_url
             = link_to commit_status.cancel_url, method: :post, title: 'Cancel' do

app/views/projects/edit.html.haml

@@ -130,6 +130,7 @@
                     %strong git fetch
                     %br
                     %span.descr Faster
+
             .form-group
               = f.label :build_timeout_in_minutes, 'Timeout', class: 'control-label'
               .col-sm-10
@@ -158,6 +159,13 @@
                       phpunit --coverage-text --colors=never (PHP) -
                       %code ^\s*Lines:\s*\d+.\d+\%
 
+              .form-group
+                .col-sm-offset-2.col-sm-10
+                  .checkbox
+                    = f.label :public_builds do
+                      = f.check_box :public_builds
+                      %strong Public builds
+                    .help-block Allow everyone to access builds for Public and Internal projects
 
           %fieldset.features
             %legend

db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb

+class AddAllowGuestToAccessBuildsProject < ActiveRecord::Migration
+  def change
+    add_column :projects, :public_builds, :boolean, default: true, null: false
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160129135155) do
+ActiveRecord::Schema.define(version: 20160202164642) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -680,6 +680,7 @@ ActiveRecord::Schema.define(version: 20160129135155) do
     t.boolean  "build_allow_git_fetch",  default: true,     null: false
     t.integer  "build_timeout",          default: 3600,     null: false
     t.boolean  "pending_delete",         default: false
+    t.boolean  "public_builds",          default: true,     null: false
   end
 
   add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree

doc/api/projects.md

@@ -80,7 +80,9 @@ Parameters:
     "avatar_url": "http://example.com/uploads/project/avatar/4/uploads/avatar.png",
     "shared_runners_enabled": true,
     "forks_count": 0,
-    "star_count": 0
+    "star_count": 0,
+    "runners_token": "b8547b1dc37721d05889db52fa2f02",
+    "public_builds": true
   },
   {
     "id": 6,
@@ -137,7 +139,8 @@ Parameters:
     "shared_runners_enabled": true,
     "forks_count": 0,
     "star_count": 0,
-    "runners_token": "b8547b1dc37721d05889db52fa2f02"
+    "runners_token": "b8547b1dc37721d05889db52fa2f02",
+    "public_builds": true
   }
 ]
 ```
@@ -424,6 +427,7 @@ Parameters:
 - `public` (optional) - if `true` same as setting visibility_level = 20
 - `visibility_level` (optional)
 - `import_url` (optional)
+- `public_builds` (optional)
 
 ### Create project for user
 
@@ -446,6 +450,7 @@ Parameters:
 - `public` (optional) - if `true` same as setting visibility_level = 20
 - `visibility_level` (optional)
 - `import_url` (optional)
+- `public_builds` (optional)
 
 ### Edit project
 
@@ -469,6 +474,7 @@ Parameters:
 - `snippets_enabled` (optional)
 - `public` (optional) - if `true` same as setting visibility_level = 20
 - `visibility_level` (optional)
+- `public_builds` (optional)
 
 On success, method returns 200 with the updated project. If parameters are
 invalid, 400 is returned.

doc/permissions/permissions.md

@@ -18,6 +18,9 @@ documentation](../workflow/add-user/add-user.md).
 |---------------------------------------|---------|------------|-------------|----------|--------|
 | Create new issue                      | ✓       | ✓          | ✓           | ✓        | ✓      |
 | Leave comments                        | ✓       | ✓          | ✓           | ✓        | ✓      |
+| See a list of builds                  | ✓ [^1]  | ✓          | ✓           | ✓        | ✓      |
+| See a build log                       | ✓ [^1]  | ✓          | ✓           | ✓        | ✓      |
+| Download and browse build artifacts   | ✓ [^1]  | ✓          | ✓           | ✓        | ✓      |
 | Pull project code                     |         | ✓          | ✓           | ✓        | ✓      |
 | Download project                      |         | ✓          | ✓           | ✓        | ✓      |
 | Create code snippets                  |         | ✓          | ✓           | ✓        | ✓      |
@@ -31,6 +34,7 @@ documentation](../workflow/add-user/add-user.md).
 | Remove non-protected branches         |         |            | ✓           | ✓        | ✓      |
 | Add tags                              |         |            | ✓           | ✓        | ✓      |
 | Write a wiki                          |         |            | ✓           | ✓        | ✓      |
+| Cancel and retry builds               |         |            | ✓           | ✓        | ✓      |
 | Create new milestones                 |         |            |             | ✓        | ✓      |
 | Add new team members                  |         |            |             | ✓        | ✓      |
 | Push to protected branches            |         |            |             | ✓        | ✓      |
@@ -40,12 +44,17 @@ documentation](../workflow/add-user/add-user.md).
 | Edit project                          |         |            |             | ✓        | ✓      |
 | Add deploy keys to project            |         |            |             | ✓        | ✓      |
 | Configure project hooks               |         |            |             | ✓        | ✓      |
+| Manage runners                        |         |            |             | ✓        | ✓      |
+| Manage build triggers                 |         |            |             | ✓        | ✓      |
+| Manage variables                      |         |            |             | ✓        | ✓      |
 | Switch visibility level               |         |            |             |          | ✓      |
 | Transfer project to another namespace |         |            |             |          | ✓      |
 | Remove project                        |         |            |             |          | ✓      |
 | Force push to protected branches      |         |            |             |          |        |
 | Remove protected branches             |         |            |             |          |        |
 
+[^1]: If **Allow guest to access builds** is enabled in CI settings
+
 ## Group
 
 In order for a group to appear as public and be browsable, it must contain at

features/project/builds/permissions.feature

@@ -5,6 +5,41 @@ Feature: Project Builds Permissions
     And project has CI enabled
     And project has a recent build
 
+  Scenario: I try to visit build details as guest
+    Given I am member of a project with a guest role
+    When I visit recent build details page
+    Then page status code should be 404
+
+  Scenario: I try to visit project builds page as guest
+    Given I am member of a project with a guest role
+    When I visit project builds page
+    Then page status code should be 404
+
+  Scenario: I try to visit build details of internal project without access to builds
+    Given The project is internal
+    And public access for builds is disabled
+    When I visit recent build details page
+    Then page status code should be 404
+
+  Scenario: I try to visit internal project builds page without access to builds
+    Given The project is internal
+    And public access for builds is disabled
+    When I visit project builds page
+    Then page status code should be 404
+
+  Scenario: I try to visit build details of internal project with access to builds
+    Given The project is internal
+    And public access for builds is enabled
+    When I visit recent build details page
+    Then I see details of a build
+    And I see build trace
+
+  Scenario: I try to visit internal project builds page with access to builds
+    Given The project is internal
+    And public access for builds is enabled
+    When I visit project builds page
+    Then I see the build
+
   Scenario: I try to download build artifacts as guest
     Given I am member of a project with a guest role
     And recent build has artifacts available

features/steps/project/builds/summary.rb

@@ -4,14 +4,6 @@ class Spinach::Features::ProjectBuildsSummary < Spinach::FeatureSteps
   include SharedBuilds
   include RepoHelpers
 
-  step 'I see details of a build' do
-    expect(page).to have_content "Build ##{@build.id}"
-  end
-
-  step 'I see build trace' do
-    expect(page).to have_css '#build-trace'
-  end
-
   step 'I see button to CI Lint' do
     page.within('.nav-controls') do
       ci_lint_tool_link = page.find_link('CI Lint')

features/steps/shared/builds.rb

@@ -38,4 +38,21 @@ module SharedBuilds
   step 'I access artifacts download page' do
     visit download_namespace_project_build_artifacts_path(@project.namespace, @project, @build)
   end
+
+  step 'I see details of a build' do
+    expect(page).to have_content "Build ##{@build.id}"
+  end
+
+  step 'I see build trace' do
+    expect(page).to have_css '#build-trace'
+  end
+
+  step 'I see the build' do
+    page.within('.commit_status') do
+      expect(page).to have_content "##{@build.id}"
+      expect(page).to have_content @build.sha[0..7]
+      expect(page).to have_content @build.ref
+      expect(page).to have_content @build.name
+    end
+  end
 end

features/steps/shared/project.rb

@@ -240,6 +240,18 @@ module SharedProject
     end
   end
 
+  step 'The project is internal' do
+    @project.update(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+  end
+
+  step 'public access for builds is enabled' do
+    @project.update(public_builds: true)
+  end
+
+  step 'public access for builds is disabled' do
+    @project.update(public_builds: false)
+  end
+
   def user_owns_project(user_name:, project_name:, visibility: :private)
     user = user_exists(user_name, username: user_name.gsub(/\s/, '').underscore)
     project = Project.find_by(name: project_name)

lib/api/builds.rb

@@ -13,11 +13,12 @@ module API
       # Example Request:
       #   GET /projects/:id/builds
       get ':id/builds' do
+
         builds = user_project.builds.order('id DESC')
         builds = filter_builds(builds, params[:scope])
 
         present paginate(builds), with: Entities::Build,
-                                  user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project)
+                                  user_can_download_artifacts: can?(current_user, :read_build, user_project)
       end
 
       # Get builds for a specific commit of a project
@@ -30,6 +31,8 @@ module API
       # Example Request:
       #   GET /projects/:id/repository/commits/:sha/builds
       get ':id/repository/commits/:sha/builds' do
+        authorize_read_builds!
+
         commit = user_project.ci_commits.find_by_sha(params[:sha])
         return not_found! unless commit
 
@@ -37,7 +40,7 @@ module API
         builds = filter_builds(builds, params[:scope])
 
         present paginate(builds), with: Entities::Build,
-                                  user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project)
+                                  user_can_download_artifacts: can?(current_user, :read_build, user_project)
       end
 
       # Get a specific build of a project
@@ -48,11 +51,13 @@ module API
       # Example Request:
       #   GET /projects/:id/builds/:build_id
       get ':id/builds/:build_id' do
+        authorize_read_builds!
+
         build = get_build(params[:build_id])
         return not_found!(build) unless build
 
         present build, with: Entities::Build,
-                       user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project)
+                       user_can_download_artifacts: can?(current_user, :read_build, user_project)
       end
 
       # Get a trace of a specific build of a project
@@ -67,6 +72,8 @@ module API
       #       is saved in the DB instead of file). But before that, we need to consider how to replace the value of
       #       `runners_token` with some mask (like `xxxxxx`) when sending trace file directly by workhorse.
       get ':id/builds/:build_id/trace' do
+        authorize_read_builds!
+
         build = get_build(params[:build_id])
         return not_found!(build) unless build
 
@@ -86,7 +93,7 @@ module API
       # example request:
       #   post /projects/:id/build/:build_id/cancel
       post ':id/builds/:build_id/cancel' do
-        authorize_manage_builds!
+        authorize_update_builds!
 
         build = get_build(params[:build_id])
         return not_found!(build) unless build
@@ -94,7 +101,7 @@ module API
         build.cancel
 
         present build, with: Entities::Build,
-                       user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project)
+                       user_can_download_artifacts: can?(current_user, :read_build, user_project)
       end
 
       # Retry a specific build of a project
@@ -105,7 +112,7 @@ module API
       # example request:
       #   post /projects/:id/build/:build_id/retry
       post ':id/builds/:build_id/retry' do
-        authorize_manage_builds!
+        authorize_update_builds!
 
         build = get_build(params[:build_id])
         return forbidden!('Build is not retryable') unless build && build.retryable?
@@ -113,7 +120,7 @@ module API
         build = Ci::Build.retry(build)
 
         present build, with: Entities::Build,
-                       user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project)
+                       user_can_download_artifacts: can?(current_user, :read_build, user_project)
       end
     end
 
@@ -141,8 +148,12 @@ module API
         builds.where(status: available_statuses && scope)
       end
 
-      def authorize_manage_builds!
-        authorize! :manage_builds, user_project
+      def authorize_read_builds!
+        authorize! :read_build, user_project
+      end
+
+      def authorize_update_builds!
+        authorize! :update_build, user_project
       end
     end
   end

lib/api/commit_statuses.rb

@@ -18,7 +18,7 @@ module API
       # Examples:
       #   GET /projects/:id/repository/commits/:sha/statuses
       get ':id/repository/commits/:sha/statuses' do
-        authorize! :read_commit_statuses, user_project
+        authorize! :read_commit_status, user_project
         sha = params[:sha]
         ci_commit = user_project.ci_commit(sha)
         not_found! 'Commit' unless ci_commit

lib/api/entities.rb

@@ -72,6 +72,7 @@ module API
       expose :star_count, :forks_count
       expose :open_issues_count, if: lambda { |project, options| project.issues_enabled? && project.default_issues_tracker? }
       expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] }
+      expose :public_builds
     end
 
     class ProjectMember < UserBasic
@@ -383,7 +384,7 @@ module API
       #       for downloading of artifacts (see: https://gitlab.com/gitlab-org/gitlab-ce/issues/4255)
       expose :download_url do |repo_obj, options|
         if options[:user_can_download_artifacts]
-          repo_obj.download_url
+          repo_obj.artifacts_download_url
         end
       end
       expose :commit, with: RepoCommit do |repo_obj, _options|

lib/api/projects.rb

@@ -99,6 +99,7 @@ module API
       #   public (optional) - if true same as setting visibility_level = 20
       #   visibility_level (optional) - 0 by default
       #   import_url (optional)
+      #   public_builds (optional)
       # Example Request
       #   POST /projects
       post do
@@ -115,7 +116,8 @@ module API
                                      :namespace_id,
                                      :public,
                                      :visibility_level,
-                                     :import_url]
+                                     :import_url,
+                                     :public_builds]
         attrs = map_public_to_visibility_level(attrs)
         @project = ::Projects::CreateService.new(current_user, attrs).execute
         if @project.saved?
@@ -145,6 +147,7 @@ module API
       #   public (optional) - if true same as setting visibility_level = 20
       #   visibility_level (optional)
       #   import_url (optional)
+      #   public_builds (optional)
       # Example Request
       #   POST /projects/user/:user_id
       post "user/:user_id" do
@@ -161,7 +164,8 @@ module API
                                      :shared_runners_enabled,
                                      :public,
                                      :visibility_level,
-                                     :import_url]
+                                     :import_url,
+                                     :public_builds]
         attrs = map_public_to_visibility_level(attrs)
         @project = ::Projects::CreateService.new(user, attrs).execute
         if @project.saved?
@@ -205,6 +209,7 @@ module API
       #   shared_runners_enabled (optional)
       #   public (optional) - if true same as setting visibility_level = 20
       #   visibility_level (optional) - visibility level of a project
+      #   public_builds (optional)
       # Example Request
       #   PUT /projects/:id
       put ':id' do
@@ -219,7 +224,8 @@ module API
                                      :snippets_enabled,
                                      :shared_runners_enabled,
                                      :public,
-                                     :visibility_level]
+                                     :visibility_level,
+                                     :public_builds]
         attrs = map_public_to_visibility_level(attrs)
         authorize_admin_project
         authorize! :rename_project, user_project if attrs[:name].present?

lib/api/triggers.rb

@@ -54,7 +54,7 @@ module API
       #   GET /projects/:id/triggers
       get ':id/triggers' do
         authenticate!
-        authorize_admin_project
+        authorize! :admin_build, user_project
 
         triggers = user_project.triggers.includes(:trigger_requests)
         triggers = paginate(triggers)
@@ -71,7 +71,7 @@ module API
       #   GET /projects/:id/triggers/:token
       get ':id/triggers/:token' do
         authenticate!
-        authorize_admin_project
+        authorize! :admin_build, user_project
 
         trigger = user_project.triggers.find_by(token: params[:token].to_s)
         return not_found!('Trigger') unless trigger
@@ -87,7 +87,7 @@ module API
       #   POST /projects/:id/triggers
       post ':id/triggers' do
         authenticate!
-        authorize_admin_project
+        authorize! :admin_build, user_project
 
         trigger = user_project.triggers.create
 
@@ -103,7 +103,7 @@ module API
       #   DELETE /projects/:id/triggers/:token
       delete ':id/triggers/:token' do
         authenticate!
-        authorize_admin_project
+        authorize! :admin_build, user_project
 
         trigger = user_project.triggers.find_by(token: params[:token].to_s)
         return not_found!('Trigger') unless trigger

lib/api/variables.rb

@@ -2,7 +2,7 @@ module API
   # Projects variables API
   class Variables < Grape::API
     before { authenticate! }
-    before { authorize_admin_project }
+    before { authorize! :admin_build, user_project }
 
     resource :projects do
       # Get project variables

spec/features/builds_spec.rb

@@ -8,7 +8,7 @@ describe "Builds" do
     @commit = FactoryGirl.create :ci_commit
     @build = FactoryGirl.create :ci_build, commit: @commit
     @project = @commit.project
-    @project.team << [@user, :master]
+    @project.team << [@user, :developer]
   end
 
   describe "GET /:project/builds" do

spec/features/commits_spec.rb

@@ -8,7 +8,6 @@ describe 'Commits' do
   describe 'CI' do
     before do
       login_as :user
-      project.team << [@user, :master]
       stub_ci_commit_to_return_yaml_file
     end
 
@@ -19,6 +18,10 @@ describe 'Commits' do
     context 'commit status is Generic Commit Status' do
       let!(:status) { FactoryGirl.create :generic_commit_status, commit: commit }
 
+      before do
+        project.team << [@user, :reporter]
+      end
+
       describe 'Commit builds' do
         before do
           visit ci_status_path(commit)
@@ -37,85 +40,126 @@ describe 'Commits' do
 
     context 'commit status is Ci Build' do
       let!(:build) { FactoryGirl.create :ci_build, commit: commit }
+      let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
 
-      describe 'Project commits' do
+      context 'when logged as developer' do
         before do
-          visit namespace_project_commits_path(project.namespace, project, :master)
+          project.team << [@user, :developer]
         end
 
-        it 'should show build status' do
-          page.within("//li[@id='commit-#{commit.short_sha}']") do
-            expect(page).to have_css(".ci-status-link")
+        describe 'Project commits' do
+          before do
+            visit namespace_project_commits_path(project.namespace, project, :master)
           end
-        end
-      end
 
-      describe 'Commit builds' do
-        before do
-          visit ci_status_path(commit)
+          it 'should show build status' do
+            page.within("//li[@id='commit-#{commit.short_sha}']") do
+              expect(page).to have_css(".ci-status-link")
+            end
+          end
         end
 
-        it { expect(page).to have_content commit.sha[0..7] }
-        it { expect(page).to have_content commit.git_commit_message }
-        it { expect(page).to have_content commit.git_author_name }
-      end
-
-      context 'Download artifacts' do
-        let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
-
-        before do
-          build.update_attributes(artifacts_file: artifacts_file)
-        end
+        describe 'Commit builds' do
+          before do
+            visit ci_status_path(commit)
+          end
 
-        it do
-          visit ci_status_path(commit)
-          click_on 'Download artifacts'
-          expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type)
+          it { expect(page).to have_content commit.sha[0..7] }
+          it { expect(page).to have_content commit.git_commit_message }
+          it { expect(page).to have_content commit.git_author_name }
         end
-      end
 
-      describe 'Cancel all builds' do
-        it 'cancels commit' do
-          visit ci_status_path(commit)
-          click_on 'Cancel running'
-          expect(page).to have_content 'canceled'
-        end
-      end
+        context 'Download artifacts' do
+          before do
+            build.update_attributes(artifacts_file: artifacts_file)
+          end
 
-      describe 'Cancel build' do
-        it 'cancels build' do
-          visit ci_status_path(commit)
-          click_on 'Cancel'
-          expect(page).to have_content 'canceled'
+          it do
+            visit ci_status_path(commit)
+            click_on 'Download artifacts'
+            expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type)
+          end
         end
-      end
 
-      describe '.gitlab-ci.yml not found warning' do
-        context 'ci builds enabled' do
-          it "does not show warning" do
+        describe 'Cancel all builds' do
+          it 'cancels commit' do
             visit ci_status_path(commit)
-            expect(page).not_to have_content '.gitlab-ci.yml not found in this commit'
+            click_on 'Cancel running'
+            expect(page).to have_content 'canceled'
           end
+        end
 
-          it 'shows warning' do
-            stub_ci_commit_yaml_file(nil)
+        describe 'Cancel build' do
+          it 'cancels build' do
             visit ci_status_path(commit)
-            expect(page).to have_content '.gitlab-ci.yml not found in this commit'
+            click_on 'Cancel'
+            expect(page).to have_content 'canceled'
           end
         end
 
-        context 'ci builds disabled' do
-          before do
-            stub_ci_builds_disabled
-            stub_ci_commit_yaml_file(nil)
-            visit ci_status_path(commit)
+        describe '.gitlab-ci.yml not found warning' do
+          context 'ci builds enabled' do
+            it "does not show warning" do
+              visit ci_status_path(commit)
+              expect(page).not_to have_content '.gitlab-ci.yml not found in this commit'
+            end
+
+            it 'shows warning' do
+              stub_ci_commit_yaml_file(nil)
+              visit ci_status_path(commit)
+              expect(page).to have_content '.gitlab-ci.yml not found in this commit'
+            end
           end
 
-          it 'does not show warning' do
-            expect(page).not_to have_content '.gitlab-ci.yml not found in this commit'
+          context 'ci builds disabled' do
+            before do
+              stub_ci_builds_disabled
+              stub_ci_commit_yaml_file(nil)
+              visit ci_status_path(commit)
+            end
+
+            it 'does not show warning' do
+              expect(page).not_to have_content '.gitlab-ci.yml not found in this commit'
+            end
           end
         end
       end
+
+      context "when logged as reporter" do
+        before do
+          project.team << [@user, :reporter]
+          build.update_attributes(artifacts_file: artifacts_file)
+          visit ci_status_path(commit)
+        end
+
+        it do
+          expect(page).to have_content commit.sha[0..7]
+          expect(page).to have_content commit.git_commit_message
+          expect(page).to have_content commit.git_author_name
+          expect(page).to have_link('Download artifacts')
+          expect(page).to_not have_link('Cancel running')
+          expect(page).to_not have_link('Retry failed')
+        end
+      end
+
+      context 'when accessing internal project with disallowed access' do
+        before do
+          project.update(
+            visibility_level: Gitlab::VisibilityLevel::INTERNAL,
+            public_builds: false)
+          build.update_attributes(artifacts_file: artifacts_file)
+          visit ci_status_path(commit)
+        end
+
+        it do
+          expect(page).to have_content commit.sha[0..7]
+          expect(page).to have_content commit.git_commit_message
+          expect(page).to have_content commit.git_author_name
+          expect(page).to_not have_link('Download artifacts')
+          expect(page).to_not have_link('Cancel running')
+          expect(page).to_not have_link('Retry failed')
+        end
+      end
     end
   end
 end

spec/features/security/project/public_access_spec.rb

@@ -96,6 +96,60 @@ describe "Public Project Access", feature: true  do
     it { is_expected.to be_denied_for :visitor }
   end
 
+  describe "GET /:project_path/builds" do
+    subject { namespace_project_builds_path(project.namespace, project) }
+
+    context "when allowed for public" do
+      before { project.update(public_builds: true) }
+
+      it { is_expected.to be_allowed_for master }
+      it { is_expected.to be_allowed_for reporter }
+      it { is_expected.to be_allowed_for :admin }
+      it { is_expected.to be_allowed_for guest }
+      it { is_expected.to be_allowed_for :user }
+      it { is_expected.to be_allowed_for :visitor }
+    end
+
+    context "when disallowed for public" do
+      before { project.update(public_builds: false) }
+
+      it { is_expected.to be_allowed_for master }
+      it { is_expected.to be_allowed_for reporter }
+      it { is_expected.to be_allowed_for :admin }
+      it { is_expected.to be_denied_for guest }
+      it { is_expected.to be_denied_for :user }
+      it { is_expected.to be_denied_for :visitor }
+    end
+  end
+
+  describe "GET /:project_path/builds/:id" do
+    let(:commit) { create(:ci_commit, project: project) }
+    let(:build) { create(:ci_build, commit: commit) }
+    subject { namespace_project_build_path(project.namespace, project, build.id) }
+
+    context "when allowed for public" do
+      before { project.update(public_builds: true) }
+
+      it { is_expected.to be_allowed_for master }
+      it { is_expected.to be_allowed_for reporter }
+      it { is_expected.to be_allowed_for :admin }
+      it { is_expected.to be_allowed_for guest }
+      it { is_expected.to be_allowed_for :user }
+      it { is_expected.to be_allowed_for :visitor }
+    end
+
+    context "when disallowed for public" do
+      before { project.update(public_builds: false) }
+
+      it { is_expected.to be_allowed_for master }
+      it { is_expected.to be_allowed_for reporter }
+      it { is_expected.to be_allowed_for :admin }
+      it { is_expected.to be_denied_for guest }
+      it { is_expected.to be_denied_for :user }
+      it { is_expected.to be_denied_for :visitor }
+    end
+  end
+
   describe "GET /:project_path/blob" do
     before do
       commit = project.repository.commit

spec/requests/api/builds_spec.rb

@@ -113,7 +113,7 @@ describe API::API, api: true  do
 
   describe 'POST /projects/:id/builds/:build_id/cancel' do
     context 'authorized user' do
-      context 'user with :manage_builds persmission' do
+      context 'user with :update_build persmission' do
         it 'should cancel running or pending build' do
           post api("/projects/#{project.id}/builds/#{build.id}/cancel", user)
 
@@ -122,7 +122,7 @@ describe API::API, api: true  do
         end
       end
 
-      context 'user without :manage_builds permission' do
+      context 'user without :update_build permission' do
         it 'should not cancel build' do
           post api("/projects/#{project.id}/builds/#{build.id}/cancel", user2)
 
@@ -142,7 +142,7 @@ describe API::API, api: true  do
 
   describe 'POST /projects/:id/builds/:build_id/retry' do
     context 'authorized user' do
-      context 'user with :manage_builds persmission' do
+      context 'user with :update_build persmission' do
         it 'should retry non-running build' do
           post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user)
 
@@ -152,7 +152,7 @@ describe API::API, api: true  do
         end
       end
 
-      context 'user without :manage_builds permission' do
+      context 'user without :update_build permission' do
         it 'should not retry build' do
           post api("/projects/#{project.id}/builds/#{build_canceled.id}/retry", user2)
 

spec/requests/api/commit_status_spec.rb

@@ -2,18 +2,17 @@ require 'spec_helper'
 
 describe API::CommitStatus, api: true do
   include ApiHelpers
-  let(:user) { create(:user) }
-  let(:user2) { create(:user) }
-  let!(:project) { create(:project, creator_id: user.id) }
-  let!(:reporter) { create(:project_member, user: user, project: project, access_level: ProjectMember::REPORTER) }
-  let!(:guest) { create(:project_member, user: user2, project: project, access_level: ProjectMember::GUEST) }
+  let!(:project) { create(:project) }
   let(:commit) { project.repository.commit }
   let!(:ci_commit) { project.ensure_ci_commit(commit.id) }
   let(:commit_status) { create(:commit_status, commit: ci_commit) }
+  let(:guest) { create_user(ProjectMember::GUEST) }
+  let(:reporter) { create_user(ProjectMember::REPORTER) }
+  let(:developer) { create_user(ProjectMember::DEVELOPER) }
 
   describe "GET /projects/:id/repository/commits/:sha/statuses" do
     it_behaves_like 'a paginated resources' do
-      let(:request) { get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", user) }
+      let(:request) { get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter) }
     end
 
     context "reporter user" do
@@ -29,7 +28,7 @@ describe API::CommitStatus, api: true do
       end
 
       it "should return latest commit statuses" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", user)
+        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter)
         expect(response.status).to eq(200)
 
         expect(json_response).to be_an Array
@@ -39,7 +38,7 @@ describe API::CommitStatus, api: true do
       end
 
       it "should return all commit statuses" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?all=1", user)
+        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?all=1", reporter)
         expect(response.status).to eq(200)
 
         expect(json_response).to be_an Array
@@ -47,7 +46,7 @@ describe API::CommitStatus, api: true do
       end
 
       it "should return latest commit statuses for specific ref" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?ref=develop", user)
+        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?ref=develop", reporter)
         expect(response.status).to eq(200)
 
         expect(json_response).to be_an Array
@@ -55,7 +54,7 @@ describe API::CommitStatus, api: true do
       end
 
       it "should return latest commit statuses for specific name" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?name=coverage", user)
+        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?name=coverage", reporter)
         expect(response.status).to eq(200)
 
         expect(json_response).to be_an Array
@@ -65,7 +64,7 @@ describe API::CommitStatus, api: true do
 
     context "guest user" do
       it "should not return project commits" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", user2)
+        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", guest)
         expect(response.status).to eq(403)
       end
     end
@@ -81,10 +80,10 @@ describe API::CommitStatus, api: true do
   describe 'POST /projects/:id/statuses/:sha' do
     let(:post_url) { "/projects/#{project.id}/statuses/#{commit.id}" }
 
-    context 'reporter user' do
+    context 'developer user' do
       context 'should create commit status' do
         it 'with only required parameters' do
-          post api(post_url, user), state: 'success'
+          post api(post_url, developer), state: 'success'
           expect(response.status).to eq(201)
           expect(json_response['sha']).to eq(commit.id)
           expect(json_response['status']).to eq('success')
@@ -95,7 +94,7 @@ describe API::CommitStatus, api: true do
         end
 
         it 'with all optional parameters' do
-          post api(post_url, user), state: 'success', context: 'coverage', ref: 'develop', target_url: 'url', description: 'test'
+          post api(post_url, developer), state: 'success', context: 'coverage', ref: 'develop', target_url: 'url', description: 'test'
           expect(response.status).to eq(201)
           expect(json_response['sha']).to eq(commit.id)
           expect(json_response['status']).to eq('success')
@@ -108,25 +107,32 @@ describe API::CommitStatus, api: true do
 
       context 'should not create commit status' do
         it 'with invalid state' do
-          post api(post_url, user), state: 'invalid'
+          post api(post_url, developer), state: 'invalid'
           expect(response.status).to eq(400)
         end
 
         it 'without state' do
-          post api(post_url, user)
+          post api(post_url, developer)
           expect(response.status).to eq(400)
         end
 
         it 'invalid commit' do
-          post api("/projects/#{project.id}/statuses/invalid_sha", user), state: 'running'
+          post api("/projects/#{project.id}/statuses/invalid_sha", developer), state: 'running'
           expect(response.status).to eq(404)
         end
       end
     end
 
+    context 'reporter user' do
+      it 'should not create commit status' do
+        post api(post_url, reporter)
+        expect(response.status).to eq(403)
+      end
+    end
+
     context 'guest user' do
       it 'should not create commit status' do
-        post api(post_url, user2)
+        post api(post_url, guest)
         expect(response.status).to eq(403)
       end
     end
@@ -138,4 +144,10 @@ describe API::CommitStatus, api: true do
       end
     end
   end
+
+  def create_user(access_level)
+    user = create(:user)
+    create(:project_member, user: user, project: project, access_level: access_level)
+    user
+  end
 end