Merge branch 'ci-permissions' into 'master'

Limit guest access to builds

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

See merge request !1942

5 files changed, 187 insertions, 77 deletions

diff --git a/spec/features/builds_spec.rb b/spec/features/builds_spec.rb
index 5b6f3cb3f15..6da3a857b3f 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
diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb
index 8c9f5a382b7..6c07802db8b 100644
--- a/spec/requests/api/builds_spec.rb
+++ b/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)
 
diff --git a/spec/requests/api/commit_status_spec.rb b/spec/requests/api/commit_status_spec.rb
index 21482fc1070..89b554622ef 100644
--- a/spec/requests/api/commit_status_spec.rb
+++ b/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