Merge branch 'feature/allow-artifacts-for-reporters' into 'master'

Allow access to artifacts for users with reporter role

This is originally introduced by @ajohnsn in this merge request: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2259

I've added and refactored specs, original commit by @ajohnsn has been cherry picked here.

See merge request !2448

13 files changed, 88 insertions, 30 deletions

diff --git a/CHANGELOG b/CHANGELOG
index f9442382f20..418dd16e5b6 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -55,6 +55,7 @@ v 8.4.0 (unreleased)
   - Allow broadcast messages to be edited
   - Autosize Markdown textareas
   - Import GitHub wiki into GitLab
+  - Add reporters ability to download and browse build artifacts (Andrew Johnson)
 
 v 8.3.4
   - Use gitlab-workhorse 0.5.4 (fixes API routing bug)
diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index dff0732bdfe..f159a6d6dc6 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -8,7 +8,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
     end
 
     unless artifacts_file.exists?
-      return not_found!
+      return render_404
     end
 
     send_file artifacts_file.path, disposition: 'attachment'
diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb
index 0e965966ffa..92d9699fe84 100644
--- a/app/controllers/projects/builds_controller.rb
+++ b/app/controllers/projects/builds_controller.rb
@@ -42,7 +42,7 @@ class Projects::BuildsController < Projects::ApplicationController
 
   def retry
     unless @build.retryable?
-      return page_404
+      return render_404
     end
 
     build = Ci::Build.retry(@build)
@@ -72,7 +72,7 @@ class Projects::BuildsController < Projects::ApplicationController
 
   def authorize_manage_builds!
     unless can?(current_user, :manage_builds, project)
-      return page_404
+      return render_404
     end
   end
 end
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 0aaba3792bf..870f6795219 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -79,7 +79,7 @@ class Projects::CommitController < Projects::ApplicationController
 
   def authorize_manage_builds!
     unless can?(current_user, :manage_builds, project)
-      return page_404
+      return render_404
     end
   end
 end
diff --git a/app/models/ability.rb b/app/models/ability.rb
index 5375148a654..ab59a3506a2 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -160,6 +160,7 @@ class Ability
       @project_report_rules ||= project_guest_rules + [
         :create_commit_status,
         :read_commit_statuses,
+        :read_build_artifacts,
         :download_code,
         :fork_project,
         :create_project_snippet,
@@ -175,7 +176,6 @@ class Ability
         :create_merge_request,
         :create_wiki,
         :manage_builds,
-        :read_build_artifacts,
         :push_code
       ]
     end
diff --git a/features/project/builds.feature b/features/project/builds/artifacts.feature
index c00b0a7ae07..7a7dbb71b18 100644
--- a/features/project/builds.feature
+++ b/features/project/builds/artifacts.feature
@@ -1,14 +1,9 @@
-Feature: Project Builds
+Feature: Project Builds Artifacts
   Background:
     Given I sign in as a user
     And I own a project
-    And CI is enabled
-    And I have recent build for my project
-
-  Scenario: I browse build summary page
-    When I visit recent build summary page
-    Then I see summary for build
-    And I see build trace
+    And project has CI enabled
+    And project has a recent build
 
   Scenario: I download build artifacts
     Given recent build has artifacts available
diff --git a/features/project/builds/permissions.feature b/features/project/builds/permissions.feature
new file mode 100644
index 00000000000..1193bcd74f6
--- /dev/null
+++ b/features/project/builds/permissions.feature
@@ -0,0 +1,18 @@
+Feature: Project Builds Permissions
+  Background:
+    Given I sign in as a user
+    And project exists in some group namespace
+    And project has CI enabled
+    And project has a recent 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
+    When I access artifacts download page
+    Then page status code should be 404
+
+  Scenario: I try to download build artifacts as reporter
+    Given I am member of a project with a reporter role
+    And recent build has artifacts available
+    When I access artifacts download page
+    Then download of build artifacts archive starts
diff --git a/features/project/builds/summary.feature b/features/project/builds/summary.feature
new file mode 100644
index 00000000000..e90ea592aab
--- /dev/null
+++ b/features/project/builds/summary.feature
@@ -0,0 +1,11 @@
+Feature: Project Builds Summary
+  Background:
+    Given I sign in as a user
+    And I own a project
+    And project has CI enabled
+    And project has a recent build
+
+  Scenario: I browse build summary page
+    When I visit recent build summary page
+    Then I see summary for build
+    And I see build trace
diff --git a/features/steps/project/builds.rb b/features/steps/project/builds/artifacts.rb
index 28395281077..f2c87da4717 100644
--- a/features/steps/project/builds.rb
+++ b/features/steps/project/builds/artifacts.rb
@@ -1,26 +1,13 @@
-class Spinach::Features::ProjectBuilds < Spinach::FeatureSteps
+class Spinach::Features::ProjectBuildsArtifacts < Spinach::FeatureSteps
   include SharedAuthentication
   include SharedProject
   include SharedBuilds
   include RepoHelpers
 
-  step 'I see summary for 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 click artifacts download button' do
     page.within('.artifacts') { click_link 'Download' }
   end
 
-  step 'download of build artifacts archive starts' do
-    expect(page.response_headers['Content-Type']).to eq 'application/zip'
-    expect(page.response_headers['Content-Transfer-Encoding']).to eq 'binary'
-  end
-
   step 'I click artifacts browse button' do
     page.within('.artifacts') { click_link 'Browse' }
   end
diff --git a/features/steps/project/builds/permissions.rb b/features/steps/project/builds/permissions.rb
new file mode 100644
index 00000000000..6e9d6504fd5
--- /dev/null
+++ b/features/steps/project/builds/permissions.rb
@@ -0,0 +1,7 @@
+class Spinach::Features::ProjectBuildsPermissions < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedProject
+  include SharedBuilds
+  include SharedPaths
+  include RepoHelpers
+end
diff --git a/features/steps/project/builds/summary.rb b/features/steps/project/builds/summary.rb
new file mode 100644
index 00000000000..2439d48fbef
--- /dev/null
+++ b/features/steps/project/builds/summary.rb
@@ -0,0 +1,14 @@
+class Spinach::Features::ProjectBuildsSummary < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedProject
+  include SharedBuilds
+  include RepoHelpers
+
+  step 'I see summary for 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
+end
diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb
index a83d74e5946..f88b01af84e 100644
--- a/features/steps/shared/builds.rb
+++ b/features/steps/shared/builds.rb
@@ -1,11 +1,11 @@
 module SharedBuilds
   include Spinach::DSL
 
-  step 'CI is enabled' do
+  step 'project has CI enabled' do
     @project.enable_ci
   end
 
-  step 'I have recent build for my project' do
+  step 'project has a recent build' do
     ci_commit = create :ci_commit, project: @project, sha: sample_commit.id
     @build = create :ci_build, commit: ci_commit
   end
@@ -25,4 +25,13 @@ module SharedBuilds
     gzip = fixture_file_upload(metadata, 'application/x-gzip')
     @build.update_attributes(artifacts_metadata: gzip)
   end
+
+  step 'download of build artifacts archive starts' do
+    expect(page.response_headers['Content-Type']).to eq 'application/zip'
+    expect(page.response_headers['Content-Transfer-Encoding']).to eq 'binary'
+  end
+
+  step 'I access artifacts download page' do
+    visit download_namespace_project_build_artifacts_path(@project.namespace, @project, @build)
+  end
 end
diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb
index d3501b5f5cb..d9c75d12238 100644
--- a/features/steps/shared/project.rb
+++ b/features/steps/shared/project.rb
@@ -7,6 +7,11 @@ module SharedProject
     @project.team << [@user, :master]
   end
 
+  step "project exists in some group namespace" do
+    @group = create(:group, name: 'some group')
+    @project = create(:project, namespace: @group)
+  end
+
   # Create a specific project called "Shop"
   step 'I own project "Shop"' do
     @project = Project.find_by(name: "Shop")
@@ -98,6 +103,18 @@ module SharedProject
   end
 
   # ----------------------------------------
+  # Project permissions
+  # ----------------------------------------
+
+  step 'I am member of a project with a guest role' do
+    @project.team << [@user, Gitlab::Access::GUEST]
+  end
+
+  step 'I am member of a project with a reporter role' do
+    @project.team << [@user, Gitlab::Access::REPORTER]
+  end
+
+  # ----------------------------------------
   # Visibility of archived project
   # ----------------------------------------
 
@@ -229,5 +246,4 @@ module SharedProject
     project ||= create(:empty_project, visibility, name: project_name, namespace: user.namespace)
     project.team << [user, :master]
   end
-
 end