Rename allow_guest_to_access_builds to public_builds

12 files changed, 202 insertions, 96 deletions

diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 153e7caaae3..14ca7426c2f 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -227,7 +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,
-      :allow_guest_to_access_builds,
+      :public_builds,
     )
   end
 
diff --git a/app/models/ability.rb b/app/models/ability.rb
index e1767ed8dd1..a6862f83158 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -64,7 +64,7 @@ class Ability
         ]
 
         # Allow to read builds by anonymous user if guests are allowed
-        rules << :read_build if project.allow_guest_to_access_builds?
+        rules << :read_build if project.public_builds?
 
         rules - project_disabled_features_rules(project)
       else
@@ -132,9 +132,9 @@ class Ability
           rules.push(*public_project_rules)
         end
 
-        # Allow to read builds if guests are allowed
-        if team.guest?(user) || project.public? || project.internal?
-          rules << :read_build if project.allow_guest_to_access_builds?
+        # Allow to read builds for internal projects
+        if project.public? || project.internal?
+          rules << :read_build if project.public_builds?
         end
 
         if project.owner == user || user.admin?
@@ -172,7 +172,6 @@ class Ability
         :read_project_member,
         :read_merge_request,
         :read_note,
-        :read_commit_status,
         :create_project,
         :create_issue,
         :create_note
@@ -187,6 +186,7 @@ class Ability
         :update_issue,
         :admin_issue,
         :admin_label,
+        :read_commit_status,
         :read_build,
       ]
     end
diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml
index fd61ce6a99a..fdcb6987471 100644
--- a/app/views/projects/edit.html.haml
+++ b/app/views/projects/edit.html.haml
@@ -162,10 +162,10 @@
               .form-group
                 .col-sm-offset-2.col-sm-10
                   .checkbox
-                    = f.label :allow_guest_to_access_builds do
-                      = f.check_box :allow_guest_to_access_builds
-                      %strong Guests can see builds
-                    .help-block Allow guests and anonymous users to access builds including build trace and artifacts
+                    = 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
diff --git a/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb
index 69ce8d08bba..793984343b4 100644
--- a/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb
+++ b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb
@@ -1,5 +1,5 @@
 class AddAllowGuestToAccessBuildsProject < ActiveRecord::Migration
   def change
-    add_column :projects, :allow_guest_to_access_builds, :boolean, default: true, null: false
+    add_column :projects, :public_builds, :boolean, default: true, null: false
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index a04e812ae22..4669a777103 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -650,35 +650,35 @@ ActiveRecord::Schema.define(version: 20160202164642) do
     t.datetime "created_at"
     t.datetime "updated_at"
     t.integer  "creator_id"
-    t.boolean  "issues_enabled",               default: true,     null: false
-    t.boolean  "wall_enabled",                 default: true,     null: false
-    t.boolean  "merge_requests_enabled",       default: true,     null: false
-    t.boolean  "wiki_enabled",                 default: true,     null: false
+    t.boolean  "issues_enabled",         default: true,     null: false
+    t.boolean  "wall_enabled",           default: true,     null: false
+    t.boolean  "merge_requests_enabled", default: true,     null: false
+    t.boolean  "wiki_enabled",           default: true,     null: false
     t.integer  "namespace_id"
-    t.string   "issues_tracker",               default: "gitlab", null: false
+    t.string   "issues_tracker",         default: "gitlab", null: false
     t.string   "issues_tracker_id"
-    t.boolean  "snippets_enabled",             default: true,     null: false
+    t.boolean  "snippets_enabled",       default: true,     null: false
     t.datetime "last_activity_at"
     t.string   "import_url"
-    t.integer  "visibility_level",             default: 0,        null: false
-    t.boolean  "archived",                     default: false,    null: false
+    t.integer  "visibility_level",       default: 0,        null: false
+    t.boolean  "archived",               default: false,    null: false
     t.string   "avatar"
     t.string   "import_status"
-    t.float    "repository_size",              default: 0.0
-    t.integer  "star_count",                   default: 0,        null: false
+    t.float    "repository_size",        default: 0.0
+    t.integer  "star_count",             default: 0,        null: false
     t.string   "import_type"
     t.string   "import_source"
-    t.integer  "commit_count",                 default: 0
+    t.integer  "commit_count",           default: 0
     t.text     "import_error"
     t.integer  "ci_id"
-    t.boolean  "builds_enabled",               default: true,     null: false
-    t.boolean  "shared_runners_enabled",       default: true,     null: false
+    t.boolean  "builds_enabled",         default: true,     null: false
+    t.boolean  "shared_runners_enabled", default: true,     null: false
     t.string   "runners_token"
     t.string   "build_coverage_regex"
-    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  "allow_guest_to_access_builds", default: true,     null: false
+    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
diff --git a/doc/api/projects.md b/doc/api/projects.md
index 873927f0e74..9e9486cd87a 100644
--- a/doc/api/projects.md
+++ b/doc/api/projects.md
@@ -82,7 +82,7 @@ Parameters:
     "forks_count": 0,
     "star_count": 0,
     "runners_token": "b8547b1dc37721d05889db52fa2f02",
-    "allow_guest_to_access_builds": true
+    "public_builds": true
   },
   {
     "id": 6,
@@ -140,7 +140,7 @@ Parameters:
     "forks_count": 0,
     "star_count": 0,
     "runners_token": "b8547b1dc37721d05889db52fa2f02",
-    "allow_guest_to_access_builds": true
+    "public_builds": true
   }
 ]
 ```
@@ -427,7 +427,7 @@ Parameters:
 - `public` (optional) - if `true` same as setting visibility_level = 20
 - `visibility_level` (optional)
 - `import_url` (optional)
-- `allow_guest_to_access_builds` (optional)
+- `public_builds` (optional)
 
 ### Create project for user
 
@@ -450,7 +450,7 @@ Parameters:
 - `public` (optional) - if `true` same as setting visibility_level = 20
 - `visibility_level` (optional)
 - `import_url` (optional)
-- `allow_guest_to_access_builds` (optional)
+- `public_builds` (optional)
 
 ### Edit project
 
@@ -474,7 +474,7 @@ Parameters:
 - `snippets_enabled` (optional)
 - `public` (optional) - if `true` same as setting visibility_level = 20
 - `visibility_level` (optional)
-- `allow_guest_to_access_builds` (optional)
+- `public_builds` (optional)
 
 On success, method returns 200 with the updated project. If parameters are
 invalid, 400 is returned.
diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb
index d9c75d12238..1200eb319d7 100644
--- a/features/steps/shared/project.rb
+++ b/features/steps/shared/project.rb
@@ -240,6 +240,14 @@ module SharedProject
     end
   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)
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 3a68b2c7801..1aa6570bd06 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -72,7 +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 :allow_guest_to_access_builds
+      expose :public_builds
     end
 
     class ProjectMember < UserBasic
@@ -384,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|
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index f68598e991b..6067c8b4a5e 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -99,7 +99,7 @@ module API
       #   public (optional) - if true same as setting visibility_level = 20
       #   visibility_level (optional) - 0 by default
       #   import_url (optional)
-      #   allow_guest_to_access_builds (optional)
+      #   public_builds (optional)
       # Example Request
       #   POST /projects
       post do
@@ -117,7 +117,7 @@ module API
                                      :public,
                                      :visibility_level,
                                      :import_url,
-                                     :allow_guest_to_access_builds]
+                                     :public_builds]
         attrs = map_public_to_visibility_level(attrs)
         @project = ::Projects::CreateService.new(current_user, attrs).execute
         if @project.saved?
@@ -147,7 +147,7 @@ module API
       #   public (optional) - if true same as setting visibility_level = 20
       #   visibility_level (optional)
       #   import_url (optional)
-      #   allow_guest_to_access_builds (optional)
+      #   public_builds (optional)
       # Example Request
       #   POST /projects/user/:user_id
       post "user/:user_id" do
@@ -165,7 +165,7 @@ module API
                                      :public,
                                      :visibility_level,
                                      :import_url,
-                                     :allow_guest_to_access_builds]
+                                     :public_builds]
         attrs = map_public_to_visibility_level(attrs)
         @project = ::Projects::CreateService.new(user, attrs).execute
         if @project.saved?
@@ -209,7 +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
-      #   allow_guest_to_access_builds (optional)
+      #   public_builds (optional)
       # Example Request
       #   PUT /projects/:id
       put ':id' do
@@ -225,7 +225,7 @@ module API
                                      :shared_runners_enabled,
                                      :public,
                                      :visibility_level,
-                                     :allow_guest_to_access_builds]
+                                     :public_builds]
         attrs = map_public_to_visibility_level(attrs)
         authorize_admin_project
         authorize! :rename_project, user_project if attrs[:name].present?
diff --git a/spec/features/builds_spec.rb b/spec/features/builds_spec.rb
index d37bd103714..22e38151bd8 100644
--- a/spec/features/builds_spec.rb
+++ b/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
diff --git a/spec/features/commits_spec.rb b/spec/features/commits_spec.rb
index 5a62da10619..dacaa96d760 100644
--- a/spec/features/commits_spec.rb
+++ b/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
diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb
index 655d2c8b7d9..b98476f854e 100644
--- a/spec/features/security/project/public_access_spec.rb
+++ b/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