From b4c36130cc285ac25caef842040e44898eaf858d Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 12:57:46 +0100 Subject: [PATCH] Rename allow_guest_to_access_builds to public_builds --- app/controllers/projects_controller.rb | 2 +- app/models/ability.rb | 10 +- app/views/projects/edit.html.haml | 8 +- ...dd_allow_guest_to_access_builds_project.rb | 2 +- db/schema.rb | 34 ++-- doc/api/projects.md | 10 +- features/steps/shared/project.rb | 8 + lib/api/entities.rb | 4 +- lib/api/projects.rb | 12 +- spec/features/builds_spec.rb | 2 +- spec/features/commits_spec.rb | 162 +++++++++++------- .../security/project/public_access_spec.rb | 54 ++++++ 12 files changed, 207 insertions(+), 101 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,83 +40,124 @@ 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 + + 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 + + describe 'Commit builds' do + before do + visit ci_status_path(commit) + 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 + before do + build.update_attributes(artifacts_file: artifacts_file) + end + + 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 + + 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 + + describe 'Cancel build' do + it 'cancels build' do + visit ci_status_path(commit) + click_on 'Cancel' + expect(page).to have_content 'canceled' + end + end + + 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 + + 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 - describe 'Commit builds' do - before do - visit ci_status_path(commit) - 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') } - + 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 - visit ci_status_path(commit) - click_on 'Download artifacts' - expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) + 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 - describe 'Cancel all builds' do - it 'cancels commit' do + 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) - click_on 'Cancel running' - expect(page).to have_content 'canceled' - end - end - - describe 'Cancel build' do - it 'cancels build' do - visit ci_status_path(commit) - click_on 'Cancel' - expect(page).to have_content 'canceled' - end - end - - 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 - 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 + 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 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