diff --git a/app/controllers/ci/application_controller.rb b/app/controllers/ci/application_controller.rb index c420b59c3a2..5bb7d499cdc 100644 --- a/app/controllers/ci/application_controller.rb +++ b/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 diff --git a/app/controllers/ci/projects_controller.rb b/app/controllers/ci/projects_controller.rb index 3004c2d27f0..711c2847d5e 100644 --- a/app/controllers/ci/projects_controller.rb +++ b/app/controllers/ci/projects_controller.rb @@ -1,8 +1,7 @@ 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 diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index f159a6d6dc6..cfea1266516 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -1,6 +1,6 @@ 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 diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index 92d9699fe84..9e89296e71d 100644 --- a/app/controllers/projects/builds_controller.rb +++ b/app/controllers/projects/builds_controller.rb @@ -1,7 +1,8 @@ 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 diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index f5a169e5aa9..2bf367d2a25 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/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 diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb index e2785caa2fb..bedeb4a295c 100644 --- a/app/controllers/projects/runner_projects_controller.rb +++ b/app/controllers/projects/runner_projects_controller.rb @@ -1,5 +1,5 @@ class Projects::RunnerProjectsController < Projects::ApplicationController - before_action :authorize_admin_project! + before_action :authorize_admin_build! layout 'project_settings' diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 4993b2648a5..0dd2d6a99be 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -1,6 +1,6 @@ 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' diff --git a/app/controllers/projects/triggers_controller.rb b/app/controllers/projects/triggers_controller.rb index 30adfad1daa..92359745cec 100644 --- a/app/controllers/projects/triggers_controller.rb +++ b/app/controllers/projects/triggers_controller.rb @@ -1,5 +1,5 @@ class Projects::TriggersController < Projects::ApplicationController - before_action :authorize_admin_project! + before_action :authorize_admin_build! layout 'project_settings' diff --git a/app/controllers/projects/variables_controller.rb b/app/controllers/projects/variables_controller.rb index 10efafea9db..00234654578 100644 --- a/app/controllers/projects/variables_controller.rb +++ b/app/controllers/projects/variables_controller.rb @@ -1,5 +1,5 @@ class Projects::VariablesController < Projects::ApplicationController - before_action :authorize_admin_project! + before_action :authorize_admin_build! layout 'project_settings' diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 4df5095bd94..14ca7426c2f 100644 --- a/app/controllers/projects_controller.rb +++ b/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 diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index dd49283089d..cc1c61e9ec0 100644 --- a/app/helpers/projects_helper.rb +++ b/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 diff --git a/app/models/ability.rb b/app/models/ability.rb index ab59a3506a2..a866eadeebb 100644 --- a/app/models/ability.rb +++ b/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 diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index c395bd908c3..34d955568f2 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/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 diff --git a/app/views/projects/builds/index.html.haml b/app/views/projects/builds/index.html.haml index 630d0286f25..5e3bd14565e 100644 --- a/app/views/projects/builds/index.html.haml +++ b/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 diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml index ba1fdc6f0e7..ca1441a20d8 100644 --- a/app/views/projects/builds/show.html.haml +++ b/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 diff --git a/app/views/projects/commit/_builds.html.haml b/app/views/projects/commit/_builds.html.haml index 329aaa0bb8b..befad27666c 100644 --- a/app/views/projects/commit/_builds.html.haml +++ b/app/views/projects/commit/_builds.html.haml @@ -1,6 +1,6 @@ .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 diff --git a/app/views/projects/commit_statuses/_commit_status.html.haml b/app/views/projects/commit_statuses/_commit_status.html.haml index 2e3c956ddc4..c02c5983ac8 100644 --- a/app/views/projects/commit_statuses/_commit_status.html.haml +++ b/app/views/projects/commit_statuses/_commit_status.html.haml @@ -1,6 +1,6 @@ %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 diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 8a99aceef7f..fdcb6987471 100644 --- a/app/views/projects/edit.html.haml +++ b/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 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 new file mode 100644 index 00000000000..793984343b4 --- /dev/null +++ b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb @@ -0,0 +1,5 @@ +class AddAllowGuestToAccessBuildsProject < ActiveRecord::Migration + def change + add_column :projects, :public_builds, :boolean, default: true, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index d4710346b82..a35be8f46de 100644 --- a/db/schema.rb +++ b/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 diff --git a/doc/api/projects.md b/doc/api/projects.md index 3f372c955d2..9e9486cd87a 100644 --- a/doc/api/projects.md +++ b/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. diff --git a/doc/permissions/permissions.md b/doc/permissions/permissions.md index 1be78ac1823..168e7d143ee 100644 --- a/doc/permissions/permissions.md +++ b/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 diff --git a/features/project/builds/permissions.feature b/features/project/builds/permissions.feature index 1193bcd74f6..3c7f72335d9 100644 --- a/features/project/builds/permissions.feature +++ b/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 diff --git a/features/steps/project/builds/summary.rb b/features/steps/project/builds/summary.rb index 4bc670fdfcb..4f94fc96354 100644 --- a/features/steps/project/builds/summary.rb +++ b/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') diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb index 92bf362879b..726e2e814ad 100644 --- a/features/steps/shared/builds.rb +++ b/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 diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index d9c75d12238..b13e82f276b 100644 --- a/features/steps/shared/project.rb +++ b/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) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index d293f988165..a8bd3842ce4 100644 --- a/lib/api/builds.rb +++ b/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 diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 1162271f5fc..9422d438d21 100644 --- a/lib/api/commit_statuses.rb +++ b/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 diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 82a75734de0..1aa6570bd06 100644 --- a/lib/api/entities.rb +++ b/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| diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 1f991e600e3..6067c8b4a5e 100644 --- a/lib/api/projects.rb +++ b/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? diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index 5e4964f446c..d1d07394e92 100644 --- a/lib/api/triggers.rb +++ b/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 diff --git a/lib/api/variables.rb b/lib/api/variables.rb index d9a055f6c92..f6495071a11 100644 --- a/lib/api/variables.rb +++ b/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 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,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 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