kotovalexarian-likes-gitlab
/
gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

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.
This commit is contained in:
Kamil Trzcinski 2016-02-01 23:58:04 +01:00
parent 7df149bb63
commit 055afab5c7
20 changed files with 78 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/ci/application_controller.rb
View File

@ -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

12
app/controllers/projects/artifacts_controller.rb
View File

@ -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

9
app/controllers/projects/builds_controller.rb
View File

@ -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

12
app/controllers/projects/commit_controller.rb
View File

@ -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

2
app/controllers/projects/runner_projects_controller.rb
View File

@ -1,5 +1,5 @@
class Projects::RunnerProjectsController < Projects::ApplicationController
before_action :authorize_admin_project!
before_action :authorize_admin_build!
layout 'project_settings'

2
app/controllers/projects/runners_controller.rb
View File

@ -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'

2
app/controllers/projects/triggers_controller.rb
View File

@ -1,5 +1,5 @@
class Projects::TriggersController < Projects::ApplicationController
before_action :authorize_admin_project!
before_action :authorize_admin_build!
layout 'project_settings'

2
app/controllers/projects/variables_controller.rb
View File

@ -1,5 +1,5 @@
class Projects::VariablesController < Projects::ApplicationController
before_action :authorize_admin_project!
before_action :authorize_admin_build!
layout 'project_settings'

2
app/helpers/projects_helper.rb
View File

@ -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

34
app/models/ability.rb
View File

@ -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

6
app/views/admin/builds/_build.html.haml
View File

@ -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

2
app/views/projects/builds/index.html.haml
View File

@ -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),

5
app/views/projects/builds/show.html.haml
View File

@ -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

2
app/views/projects/commit/_builds.html.haml
View File

@ -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

6
app/views/projects/commit_statuses/_commit_status.html.haml
View File

@ -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

29
lib/api/builds.rb
View File

@ -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

2
lib/api/commit_statuses.rb
View File

@ -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

8
lib/api/triggers.rb
View File

@ -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

2
lib/api/variables.rb
View File

@ -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

8
spec/requests/api/builds_spec.rb
View File

@ -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)