From 055afab5c7d33d061d339c270bd258ed847450f3 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Mon, 1 Feb 2016 23:58:04 +0100 Subject: Make the CI permission model simpler This MR simplifies CI permission model: - read_build: allows to read a list of builds, artifacts and trace - update_build: allows to cancel and retry builds - create_build: allows to create builds from gitlab-ci.yml (not yet implemented) - admin_build: allows to manage triggers, runners and variables - read_commit_status: allows to read a list of commit statuses (including the overall of builds) - create_commit_status: allows to create a new commit status using API Remove all extra methods to manage permission. Made all controllers to use explicitly the new permissions. --- app/controllers/ci/application_controller.rb | 2 +- app/controllers/projects/artifacts_controller.rb | 12 +------- app/controllers/projects/builds_controller.rb | 9 ++---- app/controllers/projects/commit_controller.rb | 12 ++------ .../projects/runner_projects_controller.rb | 2 +- app/controllers/projects/runners_controller.rb | 2 +- app/controllers/projects/triggers_controller.rb | 2 +- app/controllers/projects/variables_controller.rb | 2 +- app/helpers/projects_helper.rb | 2 +- app/models/ability.rb | 34 +++++++++++++++++----- app/views/admin/builds/_build.html.haml | 6 ++-- app/views/projects/builds/index.html.haml | 2 +- app/views/projects/builds/show.html.haml | 5 ++-- app/views/projects/commit/_builds.html.haml | 2 +- .../commit_statuses/_commit_status.html.haml | 6 ++-- lib/api/builds.rb | 29 ++++++++++++------ lib/api/commit_statuses.rb | 2 +- lib/api/triggers.rb | 8 ++--- lib/api/variables.rb | 2 +- spec/requests/api/builds_spec.rb | 8 ++--- 20 files changed, 78 insertions(+), 71 deletions(-) diff --git a/app/controllers/ci/application_controller.rb b/app/controllers/ci/application_controller.rb index c420b59c3a2..59c77653509 100644 --- a/app/controllers/ci/application_controller.rb +++ b/app/controllers/ci/application_controller.rb @@ -13,7 +13,7 @@ module Ci end def authorize_manage_builds! - unless can?(current_user, :manage_builds, project) + unless can?(current_user, :update_build, project) return page_404 end end 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/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 8c8b355028c..4543eff0d63 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -126,7 +126,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..e58e7a40273 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -52,10 +52,15 @@ class Ability :read_project_member, :read_merge_request, :read_note, + :read_commit_status, :read_build, :download_code ] + if project.restrict_builds? + rules -= :read_build + end + rules - project_disabled_features_rules(project) else [] @@ -113,6 +118,10 @@ class Ability if project.public? || project.internal? rules.push(*public_project_rules) + + if team.guest?(user) && project.restrict_builds? + rules -= named_abilities('build') + end end if project.owner == user || user.admin? @@ -134,7 +143,9 @@ class Ability def public_project_rules @public_project_rules ||= project_guest_rules + [ :download_code, - :fork_project + :fork_project, + :read_commit_status, + :read_build, ] end @@ -149,7 +160,7 @@ class Ability :read_project_member, :read_merge_request, :read_note, - :read_build, + :read_commit_status, :create_project, :create_issue, :create_note @@ -158,24 +169,25 @@ 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_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 +213,9 @@ class Ability :admin_merge_request, :admin_note, :admin_wiki, - :admin_project + :admin_project, + :admin_commit_status, + :admin_build ] end @@ -240,6 +254,10 @@ class Ability rules += named_abilities('wiki') end + unless project.builds_enabled + rules += named_abilities('build') + end + rules end diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index c395bd908c3..bd7fb0b36f4 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 current_user && 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 bbb6944a65a..2a2a4745bed 100644 --- a/app/views/projects/builds/index.html.haml +++ b/app/views/projects/builds/index.html.haml @@ -3,7 +3,7 @@ .project-issuable-filter .controls - - if can?(current_user, :manage_builds, @project) + - if can?(current_user, :update_build, @project) .pull-left.hidden-xs - if @all_builds.running_or_pending.any? = link_to 'Cancel running', cancel_all_namespace_project_builds_path(@project.namespace, @project), diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml index ba1fdc6f0e7..c43d6c3b427 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 current_user && 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..fba4405cb7d 100644 --- a/app/views/projects/commit_statuses/_commit_status.html.haml +++ b/app/views/projects/commit_statuses/_commit_status.html.haml @@ -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_build, commit_status.project) && 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_build, commit_status.project) && 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_build, commit_status.project) - if commit_status.active? - if commit_status.cancel_url = link_to commit_status.cancel_url, method: :post, title: 'Cancel' do 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/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/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) -- cgit v1.2.1 From 627909c2a4a938c6387afa459ef4dc815fe9fb5a Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 2 Feb 2016 17:59:37 +0100 Subject: Add CI setting: allow_guest_to_access_builds Add the `read_build` ability if user is anonymous or guest and allow_guest_to_access_builds is enabled. --- app/controllers/projects_controller.rb | 1 + app/models/ability.rb | 14 +++++---- app/views/projects/edit.html.haml | 8 +++-- ...642_add_allow_guest_to_access_builds_project.rb | 5 ++++ db/schema.rb | 35 +++++++++++----------- doc/permissions/permissions.md | 9 ++++++ 6 files changed, 47 insertions(+), 25 deletions(-) create mode 100644 db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 4df5095bd94..153e7caaae3 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, + :allow_guest_to_access_builds, ) end diff --git a/app/models/ability.rb b/app/models/ability.rb index e58e7a40273..313c6f049b7 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -53,12 +53,11 @@ class Ability :read_merge_request, :read_note, :read_commit_status, - :read_build, :download_code ] - if project.restrict_builds? - rules -= :read_build + if project.allow_guest_to_access_builds? + rules += :read_build end rules - project_disabled_features_rules(project) @@ -114,13 +113,17 @@ class Ability elsif team.guest?(user) rules.push(*project_guest_rules) + + if project.allow_guest_to_access_builds? + rules += :read_build + end end if project.public? || project.internal? rules.push(*public_project_rules) - if team.guest?(user) && project.restrict_builds? - rules -= named_abilities('build') + if project.allow_guest_to_access_builds? + rules += :read_build end end @@ -145,7 +148,6 @@ class Ability :download_code, :fork_project, :read_commit_status, - :read_build, ] end diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index 8a99aceef7f..e3165caad05 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -157,8 +157,12 @@ %li phpunit --coverage-text --colors=never (PHP) - %code ^\s*Lines:\s*\d+.\d+\% - - + .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 + Allow guest to access builds (including build logs and artifacts) %fieldset.features %legend Advanced settings 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..69ce8d08bba --- /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, :allow_guest_to_access_builds, :boolean, default: true, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 2ad2c23fba5..a04e812ae22 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: 20160128233227) do +ActiveRecord::Schema.define(version: 20160202164642) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -650,34 +650,35 @@ ActiveRecord::Schema.define(version: 20160128233227) 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 "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 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/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 -- cgit v1.2.1 From 8670411ae7acb93b5113634a3ae5e476ef6d2aee Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 3 Feb 2016 11:24:14 +0100 Subject: Clean Ci::ApplicationController from unused permission related code --- app/controllers/ci/application_controller.rb | 47 ---------------------------- app/controllers/ci/projects_controller.rb | 5 ++- 2 files changed, 2 insertions(+), 50 deletions(-) diff --git a/app/controllers/ci/application_controller.rb b/app/controllers/ci/application_controller.rb index 59c77653509..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, :update_build, 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 -- cgit v1.2.1 From e80c79e3ad0efe505541ebac0b149b750cd1cc60 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 3 Feb 2016 12:41:16 +0100 Subject: Fix build errors --- app/models/ability.rb | 6 +++--- app/views/projects/edit.html.haml | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 313c6f049b7..a9246dd3dd5 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -57,7 +57,7 @@ class Ability ] if project.allow_guest_to_access_builds? - rules += :read_build + rules << :read_build end rules - project_disabled_features_rules(project) @@ -115,7 +115,7 @@ class Ability rules.push(*project_guest_rules) if project.allow_guest_to_access_builds? - rules += :read_build + rules << :read_build end end @@ -123,7 +123,7 @@ class Ability rules.push(*public_project_rules) if project.allow_guest_to_access_builds? - rules += :read_build + rules << :read_build end end diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index e3165caad05..fd61ce6a99a 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 @@ -157,12 +158,15 @@ %li phpunit --coverage-text --colors=never (PHP) - %code ^\s*Lines:\s*\d+.\d+\% + .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 - Allow guest to access builds (including build logs and artifacts) + %strong Guests can see builds + .help-block Allow guests and anonymous users to access builds including build trace and artifacts + %fieldset.features %legend Advanced settings -- cgit v1.2.1 From c3d897a9a382b0b3354d29726add5af8c322beb4 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 3 Feb 2016 13:33:47 +0100 Subject: Properly handle commit status permissions (for a build) --- app/models/ability.rb | 22 ++++++++++++++++++++++ .../commit_statuses/_commit_status.html.haml | 8 ++++---- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index a9246dd3dd5..bf24749b173 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -5,6 +5,12 @@ class Ability return [] unless user.is_a?(User) return [] if user.blocked? + if subject.is_a?(CommitStatus) + rules = project_abilities(user, subject) + rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) + return rules + end + case subject.class.name when "Project" then project_abilities(user, subject) when "Issue" then issue_abilities(user, subject) @@ -25,6 +31,10 @@ class Ability case true when subject.is_a?(PersonalSnippet) anonymous_personal_snippet_abilities(subject) + when subject.is_a?(CommitStatus) + rules = anonymous_project_abilities(subject) + rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) + rules when subject.is_a?(Project) || subject.respond_to?(:project) anonymous_project_abilities(subject) when subject.is_a?(Group) || subject.respond_to?(:group) @@ -396,6 +406,18 @@ class Ability 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 + unless rules.include?(:read_build) + rules -= [:read_commit_status] + end + unless rules.include?(:update_build) + rules -= [:update_commit_status] + end + rules + end + def abilities @abilities ||= begin abilities = Six.new diff --git a/app/views/projects/commit_statuses/_commit_status.html.haml b/app/views/projects/commit_statuses/_commit_status.html.haml index fba4405cb7d..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 can?(current_user, :read_build, commit_status.project) && 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 can?(current_user, :read_build, 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 can?(current_user, :update_build, 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 -- cgit v1.2.1 From 5f7be11aa6d5d9edc31baa09b50ac21ee80533aa Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 11:09:42 +0100 Subject: Simplify abilities --- app/models/ability.rb | 39 +++++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index bf24749b173..e1767ed8dd1 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -5,10 +5,9 @@ class Ability return [] unless user.is_a?(User) return [] if user.blocked? + # We check with `is_a?`, because CommitStatus uses inheritance if subject.is_a?(CommitStatus) - rules = project_abilities(user, subject) - rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) - return rules + return commit_status_abilities(user, subject) end case subject.class.name @@ -32,9 +31,7 @@ class Ability when subject.is_a?(PersonalSnippet) anonymous_personal_snippet_abilities(subject) when subject.is_a?(CommitStatus) - rules = anonymous_project_abilities(subject) - rules = filter_build_abilities(rules) if subject.is_a?(Ci::Build) - rules + 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) @@ -66,9 +63,8 @@ class Ability :download_code ] - if project.allow_guest_to_access_builds? - rules << :read_build - end + # Allow to read builds by anonymous user if guests are allowed + rules << :read_build if project.allow_guest_to_access_builds? rules - project_disabled_features_rules(project) else @@ -76,6 +72,13 @@ class Ability 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 @@ -123,18 +126,15 @@ class Ability elsif team.guest?(user) rules.push(*project_guest_rules) - - if project.allow_guest_to_access_builds? - rules << :read_build - end end if project.public? || project.internal? rules.push(*public_project_rules) + end - if project.allow_guest_to_access_builds? - rules << :read_build - 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? end if project.owner == user || user.admin? @@ -406,6 +406,13 @@ 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 -- cgit v1.2.1 From 6a5a175d9fd1d72cdaab033eefc4191561e619cc Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 11:14:12 +0100 Subject: Expose allow_guest_to_access_builds in GitLab API --- doc/api/projects.md | 10 ++++++++-- lib/api/entities.rb | 1 + lib/api/projects.rb | 12 +++++++++--- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/doc/api/projects.md b/doc/api/projects.md index 3f372c955d2..873927f0e74 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", + "allow_guest_to_access_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", + "allow_guest_to_access_builds": true } ] ``` @@ -424,6 +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) ### 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) +- `allow_guest_to_access_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) +- `allow_guest_to_access_builds` (optional) On success, method returns 200 with the updated project. If parameters are invalid, 400 is returned. diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 82a75734de0..3a68b2c7801 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 :allow_guest_to_access_builds end class ProjectMember < UserBasic diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 1f991e600e3..f68598e991b 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) + # allow_guest_to_access_builds (optional) # Example Request # POST /projects post do @@ -115,7 +116,8 @@ module API :namespace_id, :public, :visibility_level, - :import_url] + :import_url, + :allow_guest_to_access_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) + # allow_guest_to_access_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, + :allow_guest_to_access_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 + # allow_guest_to_access_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, + :allow_guest_to_access_builds] attrs = map_public_to_visibility_level(attrs) authorize_admin_project authorize! :rename_project, user_project if attrs[:name].present? -- cgit v1.2.1 From b4c36130cc285ac25caef842040e44898eaf858d Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 12:57:46 +0100 Subject: 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 +- ...642_add_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 | 152 +++++++++++++-------- .../security/project/public_access_spec.rb | 54 ++++++++ 12 files changed, 202 insertions(+), 96 deletions(-) diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 153e7caaae3..14ca7426c2f 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -227,7 +227,7 @@ class ProjectsController < ApplicationController :issues_enabled, :merge_requests_enabled, :snippets_enabled, :issues_tracker_id, :default_branch, :wiki_enabled, :visibility_level, :import_url, :last_activity_at, :namespace_id, :avatar, :builds_enabled, :build_allow_git_fetch, :build_timeout_in_minutes, :build_coverage_regex, - :allow_guest_to_access_builds, + :public_builds, ) end diff --git a/app/models/ability.rb b/app/models/ability.rb index e1767ed8dd1..a6862f83158 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -64,7 +64,7 @@ class Ability ] # Allow to read builds by anonymous user if guests are allowed - rules << :read_build if project.allow_guest_to_access_builds? + rules << :read_build if project.public_builds? rules - project_disabled_features_rules(project) else @@ -132,9 +132,9 @@ class Ability rules.push(*public_project_rules) end - # Allow to read builds if guests are allowed - if team.guest?(user) || project.public? || project.internal? - rules << :read_build if project.allow_guest_to_access_builds? + # Allow to read builds for internal projects + if project.public? || project.internal? + rules << :read_build if project.public_builds? end if project.owner == user || user.admin? @@ -172,7 +172,6 @@ class Ability :read_project_member, :read_merge_request, :read_note, - :read_commit_status, :create_project, :create_issue, :create_note @@ -187,6 +186,7 @@ class Ability :update_issue, :admin_issue, :admin_label, + :read_commit_status, :read_build, ] end diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index fd61ce6a99a..fdcb6987471 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -162,10 +162,10 @@ .form-group .col-sm-offset-2.col-sm-10 .checkbox - = f.label :allow_guest_to_access_builds do - = f.check_box :allow_guest_to_access_builds - %strong Guests can see builds - .help-block Allow guests and anonymous users to access builds including build trace and artifacts + = f.label :public_builds do + = f.check_box :public_builds + %strong Public builds + .help-block Allow everyone to access builds for Public and Internal projects %fieldset.features %legend diff --git a/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb index 69ce8d08bba..793984343b4 100644 --- a/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb +++ b/db/migrate/20160202164642_add_allow_guest_to_access_builds_project.rb @@ -1,5 +1,5 @@ class AddAllowGuestToAccessBuildsProject < ActiveRecord::Migration def change - add_column :projects, :allow_guest_to_access_builds, :boolean, default: true, null: false + add_column :projects, :public_builds, :boolean, default: true, null: false end end diff --git a/db/schema.rb b/db/schema.rb index a04e812ae22..4669a777103 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -650,35 +650,35 @@ ActiveRecord::Schema.define(version: 20160202164642) do t.datetime "created_at" t.datetime "updated_at" t.integer "creator_id" - t.boolean "issues_enabled", default: true, null: false - t.boolean "wall_enabled", default: true, null: false - t.boolean "merge_requests_enabled", default: true, null: false - t.boolean "wiki_enabled", default: true, null: false + t.boolean "issues_enabled", default: true, null: false + t.boolean "wall_enabled", default: true, null: false + t.boolean "merge_requests_enabled", default: true, null: false + t.boolean "wiki_enabled", default: true, null: false t.integer "namespace_id" - t.string "issues_tracker", default: "gitlab", null: false + t.string "issues_tracker", default: "gitlab", null: false t.string "issues_tracker_id" - t.boolean "snippets_enabled", default: true, null: false + t.boolean "snippets_enabled", default: true, null: false t.datetime "last_activity_at" t.string "import_url" - t.integer "visibility_level", default: 0, null: false - t.boolean "archived", default: false, null: false + t.integer "visibility_level", default: 0, null: false + t.boolean "archived", default: false, null: false t.string "avatar" t.string "import_status" - t.float "repository_size", default: 0.0 - t.integer "star_count", default: 0, null: false + t.float "repository_size", default: 0.0 + t.integer "star_count", default: 0, null: false t.string "import_type" t.string "import_source" - t.integer "commit_count", default: 0 + t.integer "commit_count", default: 0 t.text "import_error" t.integer "ci_id" - t.boolean "builds_enabled", default: true, null: false - t.boolean "shared_runners_enabled", default: true, null: false + t.boolean "builds_enabled", default: true, null: false + t.boolean "shared_runners_enabled", default: true, null: false t.string "runners_token" t.string "build_coverage_regex" - t.boolean "build_allow_git_fetch", default: true, null: false - t.integer "build_timeout", default: 3600, null: false - t.boolean "pending_delete", default: false - t.boolean "allow_guest_to_access_builds", default: true, null: false + t.boolean "build_allow_git_fetch", default: true, null: false + t.integer "build_timeout", default: 3600, null: false + t.boolean "pending_delete", default: false + t.boolean "public_builds", default: true, null: false end add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree diff --git a/doc/api/projects.md b/doc/api/projects.md index 873927f0e74..9e9486cd87a 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -82,7 +82,7 @@ Parameters: "forks_count": 0, "star_count": 0, "runners_token": "b8547b1dc37721d05889db52fa2f02", - "allow_guest_to_access_builds": true + "public_builds": true }, { "id": 6, @@ -140,7 +140,7 @@ Parameters: "forks_count": 0, "star_count": 0, "runners_token": "b8547b1dc37721d05889db52fa2f02", - "allow_guest_to_access_builds": true + "public_builds": true } ] ``` @@ -427,7 +427,7 @@ Parameters: - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) - `import_url` (optional) -- `allow_guest_to_access_builds` (optional) +- `public_builds` (optional) ### Create project for user @@ -450,7 +450,7 @@ Parameters: - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) - `import_url` (optional) -- `allow_guest_to_access_builds` (optional) +- `public_builds` (optional) ### Edit project @@ -474,7 +474,7 @@ Parameters: - `snippets_enabled` (optional) - `public` (optional) - if `true` same as setting visibility_level = 20 - `visibility_level` (optional) -- `allow_guest_to_access_builds` (optional) +- `public_builds` (optional) On success, method returns 200 with the updated project. If parameters are invalid, 400 is returned. diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index d9c75d12238..1200eb319d7 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -240,6 +240,14 @@ module SharedProject end end + step 'public access for builds is enabled' do + @project.update(public_builds: true) + end + + step 'public access for builds is disabled' do + @project.update(public_builds: false) + end + def user_owns_project(user_name:, project_name:, visibility: :private) user = user_exists(user_name, username: user_name.gsub(/\s/, '').underscore) project = Project.find_by(name: project_name) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3a68b2c7801..1aa6570bd06 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -72,7 +72,7 @@ module API expose :star_count, :forks_count expose :open_issues_count, if: lambda { |project, options| project.issues_enabled? && project.default_issues_tracker? } expose :runners_token, if: lambda { |_project, options| options[:user_can_admin_project] } - expose :allow_guest_to_access_builds + expose :public_builds end class ProjectMember < UserBasic @@ -384,7 +384,7 @@ module API # for downloading of artifacts (see: https://gitlab.com/gitlab-org/gitlab-ce/issues/4255) expose :download_url do |repo_obj, options| if options[:user_can_download_artifacts] - repo_obj.download_url + repo_obj.artifacts_download_url end end expose :commit, with: RepoCommit do |repo_obj, _options| diff --git a/lib/api/projects.rb b/lib/api/projects.rb index f68598e991b..6067c8b4a5e 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -99,7 +99,7 @@ module API # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) - 0 by default # import_url (optional) - # allow_guest_to_access_builds (optional) + # public_builds (optional) # Example Request # POST /projects post do @@ -117,7 +117,7 @@ module API :public, :visibility_level, :import_url, - :allow_guest_to_access_builds] + :public_builds] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(current_user, attrs).execute if @project.saved? @@ -147,7 +147,7 @@ module API # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) # import_url (optional) - # allow_guest_to_access_builds (optional) + # public_builds (optional) # Example Request # POST /projects/user/:user_id post "user/:user_id" do @@ -165,7 +165,7 @@ module API :public, :visibility_level, :import_url, - :allow_guest_to_access_builds] + :public_builds] attrs = map_public_to_visibility_level(attrs) @project = ::Projects::CreateService.new(user, attrs).execute if @project.saved? @@ -209,7 +209,7 @@ module API # shared_runners_enabled (optional) # public (optional) - if true same as setting visibility_level = 20 # visibility_level (optional) - visibility level of a project - # allow_guest_to_access_builds (optional) + # public_builds (optional) # Example Request # PUT /projects/:id put ':id' do @@ -225,7 +225,7 @@ module API :shared_runners_enabled, :public, :visibility_level, - :allow_guest_to_access_builds] + :public_builds] attrs = map_public_to_visibility_level(attrs) authorize_admin_project authorize! :rename_project, user_project if attrs[:name].present? diff --git a/spec/features/builds_spec.rb b/spec/features/builds_spec.rb index d37bd103714..22e38151bd8 100644 --- a/spec/features/builds_spec.rb +++ b/spec/features/builds_spec.rb @@ -8,7 +8,7 @@ describe "Builds" do @commit = FactoryGirl.create :ci_commit @build = FactoryGirl.create :ci_build, commit: @commit @project = @commit.project - @project.team << [@user, :master] + @project.team << [@user, :developer] end describe "GET /:project/builds" do diff --git a/spec/features/commits_spec.rb b/spec/features/commits_spec.rb index 5a62da10619..dacaa96d760 100644 --- a/spec/features/commits_spec.rb +++ b/spec/features/commits_spec.rb @@ -8,7 +8,6 @@ describe 'Commits' do describe 'CI' do before do login_as :user - project.team << [@user, :master] stub_ci_commit_to_return_yaml_file end @@ -19,6 +18,10 @@ describe 'Commits' do context 'commit status is Generic Commit Status' do let!(:status) { FactoryGirl.create :generic_commit_status, commit: commit } + before do + project.team << [@user, :reporter] + end + describe 'Commit builds' do before do visit ci_status_path(commit) @@ -37,85 +40,126 @@ describe 'Commits' do context 'commit status is Ci Build' do let!(:build) { FactoryGirl.create :ci_build, commit: commit } + let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') } - describe 'Project commits' do + context 'when logged as developer' do before do - visit namespace_project_commits_path(project.namespace, project, :master) + project.team << [@user, :developer] end - it 'should show build status' do - page.within("//li[@id='commit-#{commit.short_sha}']") do - expect(page).to have_css(".ci-status-link") + describe 'Project commits' do + before do + visit namespace_project_commits_path(project.namespace, project, :master) end - end - end - describe 'Commit builds' do - before do - visit ci_status_path(commit) + it 'should show build status' do + page.within("//li[@id='commit-#{commit.short_sha}']") do + expect(page).to have_css(".ci-status-link") + end + end end - it { expect(page).to have_content commit.sha[0..7] } - it { expect(page).to have_content commit.git_commit_message } - it { expect(page).to have_content commit.git_author_name } - end - - context 'Download artifacts' do - let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') } - - before do - build.update_attributes(artifacts_file: artifacts_file) - end + describe 'Commit builds' do + before do + visit ci_status_path(commit) + end - it do - visit ci_status_path(commit) - click_on 'Download artifacts' - expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) + it { expect(page).to have_content commit.sha[0..7] } + it { expect(page).to have_content commit.git_commit_message } + it { expect(page).to have_content commit.git_author_name } end - end - describe 'Cancel all builds' do - it 'cancels commit' do - visit ci_status_path(commit) - click_on 'Cancel running' - expect(page).to have_content 'canceled' - end - end + context 'Download artifacts' do + before do + build.update_attributes(artifacts_file: artifacts_file) + end - describe 'Cancel build' do - it 'cancels build' do - visit ci_status_path(commit) - click_on 'Cancel' - expect(page).to have_content 'canceled' + it do + visit ci_status_path(commit) + click_on 'Download artifacts' + expect(page.response_headers['Content-Type']).to eq(artifacts_file.content_type) + end end - end - describe '.gitlab-ci.yml not found warning' do - context 'ci builds enabled' do - it "does not show warning" do + describe 'Cancel all builds' do + it 'cancels commit' do visit ci_status_path(commit) - expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' + click_on 'Cancel running' + expect(page).to have_content 'canceled' end + end - it 'shows warning' do - stub_ci_commit_yaml_file(nil) + describe 'Cancel build' do + it 'cancels build' do visit ci_status_path(commit) - expect(page).to have_content '.gitlab-ci.yml not found in this commit' + click_on 'Cancel' + expect(page).to have_content 'canceled' end end - context 'ci builds disabled' do - before do - stub_ci_builds_disabled - stub_ci_commit_yaml_file(nil) - visit ci_status_path(commit) + describe '.gitlab-ci.yml not found warning' do + context 'ci builds enabled' do + it "does not show warning" do + visit ci_status_path(commit) + expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' + end + + it 'shows warning' do + stub_ci_commit_yaml_file(nil) + visit ci_status_path(commit) + expect(page).to have_content '.gitlab-ci.yml not found in this commit' + end end - it 'does not show warning' do - expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' + context 'ci builds disabled' do + before do + stub_ci_builds_disabled + stub_ci_commit_yaml_file(nil) + visit ci_status_path(commit) + end + + it 'does not show warning' do + expect(page).not_to have_content '.gitlab-ci.yml not found in this commit' + end end end end + + context "when logged as reporter" do + before do + project.team << [@user, :reporter] + build.update_attributes(artifacts_file: artifacts_file) + visit ci_status_path(commit) + end + + it do + expect(page).to have_content commit.sha[0..7] + expect(page).to have_content commit.git_commit_message + expect(page).to have_content commit.git_author_name + expect(page).to have_link('Download artifacts') + expect(page).to_not have_link('Cancel running') + expect(page).to_not have_link('Retry failed') + end + end + + context 'when accessing internal project with disallowed access' do + before do + project.update( + visibility_level: Gitlab::VisibilityLevel::INTERNAL, + public_builds: false) + build.update_attributes(artifacts_file: artifacts_file) + visit ci_status_path(commit) + end + + it do + expect(page).to have_content commit.sha[0..7] + expect(page).to have_content commit.git_commit_message + expect(page).to have_content commit.git_author_name + expect(page).to_not have_link('Download artifacts') + expect(page).to_not have_link('Cancel running') + expect(page).to_not have_link('Retry failed') + end + end end end end diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb index 655d2c8b7d9..b98476f854e 100644 --- a/spec/features/security/project/public_access_spec.rb +++ b/spec/features/security/project/public_access_spec.rb @@ -96,6 +96,60 @@ describe "Public Project Access", feature: true do it { is_expected.to be_denied_for :visitor } end + describe "GET /:project_path/builds" do + subject { namespace_project_builds_path(project.namespace, project) } + + context "when allowed for public" do + before { project.update(public_builds: true) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_allowed_for guest } + it { is_expected.to be_allowed_for :user } + it { is_expected.to be_allowed_for :visitor } + end + + context "when disallowed for public" do + before { project.update(public_builds: false) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for :user } + it { is_expected.to be_denied_for :visitor } + end + end + + describe "GET /:project_path/builds/:id" do + let(:commit) { create(:ci_commit, project: project) } + let(:build) { create(:ci_build, commit: commit) } + subject { namespace_project_build_path(project.namespace, project, build.id) } + + context "when allowed for public" do + before { project.update(public_builds: true) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_allowed_for guest } + it { is_expected.to be_allowed_for :user } + it { is_expected.to be_allowed_for :visitor } + end + + context "when disallowed for public" do + before { project.update(public_builds: false) } + + it { is_expected.to be_allowed_for master } + it { is_expected.to be_allowed_for reporter } + it { is_expected.to be_allowed_for :admin } + it { is_expected.to be_denied_for guest } + it { is_expected.to be_denied_for :user } + it { is_expected.to be_denied_for :visitor } + end + end + describe "GET /:project_path/blob" do before do commit = project.repository.commit -- cgit v1.2.1 From d231b6b9182ce9f68f267af0a073136c898f6892 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 13:13:01 +0100 Subject: Add behaviour tests for build permissions --- features/project/builds/permissions.feature | 35 +++++++++++++++++++++++++++++ features/steps/project/builds/summary.rb | 8 ------- features/steps/shared/builds.rb | 17 ++++++++++++++ features/steps/shared/project.rb | 4 ++++ 4 files changed, 56 insertions(+), 8 deletions(-) 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 036bc0a499e..433253dd44b 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('.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 1200eb319d7..b13e82f276b 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -240,6 +240,10 @@ 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 -- cgit v1.2.1 From c64c106091ee5526e9413b9929205134c0ce114f Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 14:43:52 +0100 Subject: Update ability model after comments --- app/models/ability.rb | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index a6862f83158..00f5f3a93b3 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -5,22 +5,18 @@ class Ability return [] unless user.is_a?(User) return [] if user.blocked? - # We check with `is_a?`, because CommitStatus uses inheritance - if subject.is_a?(CommitStatus) - return commit_status_abilities(user, subject) - end - - 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 @@ -130,10 +126,8 @@ class Ability if project.public? || project.internal? rules.push(*public_project_rules) - end - # Allow to read builds for internal projects - if project.public? || project.internal? + # Allow to read builds for internal projects rules << :read_build if project.public_builds? end @@ -416,11 +410,8 @@ class Ability 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 - unless rules.include?(:read_build) - rules -= [:read_commit_status] - end - unless rules.include?(:update_build) - rules -= [:update_commit_status] + %w(read create update admin).each do |rule| + rules -= [:"#{rule}_commit_status"] unless rules.include?(:"#{rule}_build") end rules end -- cgit v1.2.1 From a7c441aa17ba64eb702b583ac9198cdace599d2e Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 4 Feb 2016 16:32:41 +0100 Subject: Use `delete` instead of assignment operator when filtering build abilities --- app/models/ability.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 00f5f3a93b3..a866eadeebb 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -411,7 +411,7 @@ class Ability # 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 -= [:"#{rule}_commit_status"] unless rules.include?(:"#{rule}_build") + rules.delete(:"#{rule}_commit_status") unless rules.include?(:"#{rule}_build") end rules end -- cgit v1.2.1 From 08064767615ad7b0e2599aed94c53117516e8377 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 5 Feb 2016 20:17:58 +0100 Subject: Remove current_user && when can? is used --- app/views/admin/builds/_build.html.haml | 2 +- app/views/projects/builds/show.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml index bd7fb0b36f4..34d955568f2 100644 --- a/app/views/admin/builds/_build.html.haml +++ b/app/views/admin/builds/_build.html.haml @@ -63,7 +63,7 @@ - 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, :update_build, 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/show.html.haml b/app/views/projects/builds/show.html.haml index c43d6c3b427..ca1441a20d8 100644 --- a/app/views/projects/builds/show.html.haml +++ b/app/views/projects/builds/show.html.haml @@ -101,7 +101,7 @@ .build-widget %h4.title Build ##{@build.id} - - if current_user && can?(current_user, :update_build, @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 -- cgit v1.2.1 From 311f407651e9ad1859bb0e9b6b9d6de79fde1a3d Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sat, 6 Feb 2016 15:01:37 +0100 Subject: Fix commit status tests --- spec/requests/api/commit_status_spec.rb | 48 ++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 18 deletions(-) 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 -- cgit v1.2.1