From 0fd397bba1a36136c3737165c9057bc59dcbca77 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 21 Nov 2016 10:48:07 +0100 Subject: Added permissions per stage to cycle analytics endpoint --- .../projects/cycle_analytics_controller.rb | 5 +- app/models/cycle_analytics.rb | 9 +- .../unreleased/fix-cycle-analytics-permissions.yml | 4 + lib/gitlab/cycle_analytics/permissions.rb | 49 ++++++++ .../lib/gitlab/cycle_analytics/permissions_spec.rb | 127 +++++++++++++++++++++ spec/models/cycle_analytics/code_spec.rb | 2 +- spec/models/cycle_analytics/issue_spec.rb | 2 +- spec/models/cycle_analytics/plan_spec.rb | 2 +- spec/models/cycle_analytics/production_spec.rb | 2 +- spec/models/cycle_analytics/review_spec.rb | 2 +- spec/models/cycle_analytics/staging_spec.rb | 2 +- spec/models/cycle_analytics/summary_spec.rb | 2 +- spec/models/cycle_analytics/test_spec.rb | 2 +- 13 files changed, 199 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/fix-cycle-analytics-permissions.yml create mode 100644 lib/gitlab/cycle_analytics/permissions.rb create mode 100644 spec/lib/gitlab/cycle_analytics/permissions_spec.rb diff --git a/app/controllers/projects/cycle_analytics_controller.rb b/app/controllers/projects/cycle_analytics_controller.rb index 96eb75a0547..8567b074f11 100644 --- a/app/controllers/projects/cycle_analytics_controller.rb +++ b/app/controllers/projects/cycle_analytics_controller.rb @@ -6,7 +6,7 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController before_action :authorize_read_cycle_analytics! def show - @cycle_analytics = ::CycleAnalytics.new(@project, from: start_date(cycle_analytics_params)) + @cycle_analytics = ::CycleAnalytics.new(@project, from: start_date(cycle_analytics_params), user: current_user) respond_to do |format| format.html @@ -54,7 +54,8 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController { summary: summary, - stats: stats + stats: stats, + permissions: @cycle_analytics.permissions } end end diff --git a/app/models/cycle_analytics.rb b/app/models/cycle_analytics.rb index 314a1ce9b63..a10e96d53bf 100644 --- a/app/models/cycle_analytics.rb +++ b/app/models/cycle_analytics.rb @@ -1,7 +1,10 @@ class CycleAnalytics - def initialize(project, from:) + STAGES = %i[issue plan code test review staging production].freeze + + def initialize(project, from:, user:) @project = project @from = from + @user = user @fetcher = Gitlab::CycleAnalytics::MetricsFetcher.new(project: project, from: from, branch: nil) end @@ -9,6 +12,10 @@ class CycleAnalytics @summary ||= Summary.new(@project, from: @from) end + def permissions + Gitlab::CycleAnalytics::Permissions.get(user: @user, project: @project) + end + def issue @fetcher.calculate_metric(:issue, Issue.arel_table[:created_at], diff --git a/changelogs/unreleased/fix-cycle-analytics-permissions.yml b/changelogs/unreleased/fix-cycle-analytics-permissions.yml new file mode 100644 index 00000000000..ddcf78d705f --- /dev/null +++ b/changelogs/unreleased/fix-cycle-analytics-permissions.yml @@ -0,0 +1,4 @@ +--- +title: Added permissions per stage to cycle analytics endpoint +merge_request: +author: diff --git a/lib/gitlab/cycle_analytics/permissions.rb b/lib/gitlab/cycle_analytics/permissions.rb new file mode 100644 index 00000000000..121b723f7be --- /dev/null +++ b/lib/gitlab/cycle_analytics/permissions.rb @@ -0,0 +1,49 @@ +module Gitlab + module CycleAnalytics + class Permissions + STAGE_PERMISSIONS = { + read_build: [:test, :staging], + read_issue: [:issue, :production], + read_merge_request: [:code, :review] + }.freeze + + def self.get(*args) + new(*args).get + end + + def initialize(user:, project:) + @user = user + @project = project + @stage_permission_hash = {} + end + + def get + ::CycleAnalytics::STAGES.each do |stage| + @stage_permission_hash[stage] = authorized_stage?(stage) + end + + @stage_permission_hash + end + + private + + def authorized_stage?(stage) + return false unless authorize_project(:read_cycle_analytics) + + permissions_for_stage(stage).keys.each do |permission| + return false unless authorize_project(permission) + end + + true + end + + def permissions_for_stage(stage) + STAGE_PERMISSIONS.select { |_permission, stages| stages.include?(stage) } + end + + def authorize_project(permission) + Ability.allowed?(@user, permission, @project) + end + end + end +end diff --git a/spec/lib/gitlab/cycle_analytics/permissions_spec.rb b/spec/lib/gitlab/cycle_analytics/permissions_spec.rb new file mode 100644 index 00000000000..dc4f7dc69db --- /dev/null +++ b/spec/lib/gitlab/cycle_analytics/permissions_spec.rb @@ -0,0 +1,127 @@ +require 'spec_helper' + +describe Gitlab::CycleAnalytics::Permissions do + let(:project) { create(:empty_project) } + let(:user) { create(:user) } + + subject { described_class.get(user: user, project: project) } + + context 'user with no relation to the project' do + it 'has no permissions to issue stage' do + expect(subject[:issue]).to eq(false) + end + + it 'has no permissions to test stage' do + expect(subject[:test]).to eq(false) + end + + it 'has no permissions to staging stage' do + expect(subject[:staging]).to eq(false) + end + + it 'has no permissions to production stage' do + expect(subject[:production]).to eq(false) + end + + it 'has no permissions to code stage' do + expect(subject[:code]).to eq(false) + end + + it 'has no permissions to review stage' do + expect(subject[:review]).to eq(false) + end + + it 'has no permissions to plan stage' do + expect(subject[:plan]).to eq(false) + end + end + + context 'user is master' do + before do + project.team << [user, :master] + end + + it 'has permissions to issue stage' do + expect(subject[:issue]).to eq(true) + end + + it 'has permissions to test stage' do + expect(subject[:test]).to eq(true) + end + + it 'has permissions to staging stage' do + expect(subject[:staging]).to eq(true) + end + + it 'has permissions to production stage' do + expect(subject[:production]).to eq(true) + end + + it 'has permissions to code stage' do + expect(subject[:code]).to eq(true) + end + + it 'has permissions to review stage' do + expect(subject[:review]).to eq(true) + end + + it 'has permissions to plan stage' do + expect(subject[:plan]).to eq(true) + end + end + + context 'user has no build permissions' do + before do + project.team << [user, :guest] + end + + it 'has permissions to issue stage' do + expect(subject[:issue]).to eq(true) + end + + it 'has no permissions to test stage' do + expect(subject[:test]).to eq(false) + end + + it 'has no permissions to staging stage' do + expect(subject[:staging]).to eq(false) + end + end + + context 'user has no merge request permissions' do + before do + project.team << [user, :guest] + end + + it 'has permissions to issue stage' do + expect(subject[:issue]).to eq(true) + end + + it 'has no permissions to code stage' do + expect(subject[:code]).to eq(false) + end + + it 'has no permissions to review stage' do + expect(subject[:review]).to eq(false) + end + end + + context 'user has no issue permissions' do + before do + project.team << [user, :developer] + project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED) + end + + it 'has permissions to code stage' do + expect(subject[:code]).to eq(true) + end + + it 'has no permissions to issue stage' do + expect(subject[:issue]).to eq(false) + end + + it 'has no permissions to production stage' do + expect(subject[:production]).to eq(false) + end + end +end diff --git a/spec/models/cycle_analytics/code_spec.rb b/spec/models/cycle_analytics/code_spec.rb index 7691d690db0..7e5941c39a6 100644 --- a/spec/models/cycle_analytics/code_spec.rb +++ b/spec/models/cycle_analytics/code_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#code', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, from: from_date, user: user) } context 'with deployment' do generate_cycle_analytics_spec( diff --git a/spec/models/cycle_analytics/issue_spec.rb b/spec/models/cycle_analytics/issue_spec.rb index f649b44d367..ff5c95c08cb 100644 --- a/spec/models/cycle_analytics/issue_spec.rb +++ b/spec/models/cycle_analytics/issue_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#issue', models: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, from: from_date, user: user) } generate_cycle_analytics_spec( phase: :issue, diff --git a/spec/models/cycle_analytics/plan_spec.rb b/spec/models/cycle_analytics/plan_spec.rb index 2cdefbeef21..16db10e4a1e 100644 --- a/spec/models/cycle_analytics/plan_spec.rb +++ b/spec/models/cycle_analytics/plan_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#plan', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, from: from_date, user: user) } generate_cycle_analytics_spec( phase: :plan, diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb index 1f5e5cab92d..8d5460c1e5a 100644 --- a/spec/models/cycle_analytics/production_spec.rb +++ b/spec/models/cycle_analytics/production_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#production', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, from: from_date, user: user) } generate_cycle_analytics_spec( phase: :production, diff --git a/spec/models/cycle_analytics/review_spec.rb b/spec/models/cycle_analytics/review_spec.rb index 0ed080a42b1..9f3cb06c2df 100644 --- a/spec/models/cycle_analytics/review_spec.rb +++ b/spec/models/cycle_analytics/review_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#review', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, from: from_date, user: user) } generate_cycle_analytics_spec( phase: :review, diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb index af1c4477ddb..1feb6326c31 100644 --- a/spec/models/cycle_analytics/staging_spec.rb +++ b/spec/models/cycle_analytics/staging_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#staging', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, from: from_date, user: user) } generate_cycle_analytics_spec( phase: :staging, diff --git a/spec/models/cycle_analytics/summary_spec.rb b/spec/models/cycle_analytics/summary_spec.rb index 9d67bc82cba..dfbd9a553bc 100644 --- a/spec/models/cycle_analytics/summary_spec.rb +++ b/spec/models/cycle_analytics/summary_spec.rb @@ -4,7 +4,7 @@ describe CycleAnalytics::Summary, models: true do let(:project) { create(:project) } let(:from) { Time.now } let(:user) { create(:user, :admin) } - subject { described_class.new(project, from: from) } + subject { described_class.new(project, from: from, user: user) } describe "#new_issues" do it "finds the number of issues created after the 'from date'" do diff --git a/spec/models/cycle_analytics/test_spec.rb b/spec/models/cycle_analytics/test_spec.rb index 02ddfeed9c1..ff7f03ca5fb 100644 --- a/spec/models/cycle_analytics/test_spec.rb +++ b/spec/models/cycle_analytics/test_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#test', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date) } + subject { CycleAnalytics.new(project, from: from_date, user: user) } generate_cycle_analytics_spec( phase: :test, -- cgit v1.2.1 From d747c1c0f974034d71a25edc2c8e525d3657c774 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 21 Nov 2016 11:52:04 +0100 Subject: fix spec failure --- spec/models/cycle_analytics/summary_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/cycle_analytics/summary_spec.rb b/spec/models/cycle_analytics/summary_spec.rb index dfbd9a553bc..9d67bc82cba 100644 --- a/spec/models/cycle_analytics/summary_spec.rb +++ b/spec/models/cycle_analytics/summary_spec.rb @@ -4,7 +4,7 @@ describe CycleAnalytics::Summary, models: true do let(:project) { create(:project) } let(:from) { Time.now } let(:user) { create(:user, :admin) } - subject { described_class.new(project, from: from, user: user) } + subject { described_class.new(project, from: from) } describe "#new_issues" do it "finds the number of issues created after the 'from date'" do -- cgit v1.2.1 From 9b691688583ad46d5608320ec64873dd2eb9a647 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 21 Nov 2016 14:09:26 +0100 Subject: refactored a couple of things based on feedback --- .../projects/cycle_analytics_controller.rb | 4 ++-- app/models/cycle_analytics.rb | 7 +++---- lib/gitlab/cycle_analytics/permissions.rb | 19 +++++++------------ spec/models/cycle_analytics/code_spec.rb | 2 +- spec/models/cycle_analytics/issue_spec.rb | 2 +- spec/models/cycle_analytics/plan_spec.rb | 2 +- spec/models/cycle_analytics/production_spec.rb | 2 +- spec/models/cycle_analytics/review_spec.rb | 2 +- spec/models/cycle_analytics/staging_spec.rb | 2 +- spec/models/cycle_analytics/test_spec.rb | 2 +- 10 files changed, 19 insertions(+), 25 deletions(-) diff --git a/app/controllers/projects/cycle_analytics_controller.rb b/app/controllers/projects/cycle_analytics_controller.rb index 8567b074f11..00ecdcbd1b9 100644 --- a/app/controllers/projects/cycle_analytics_controller.rb +++ b/app/controllers/projects/cycle_analytics_controller.rb @@ -6,7 +6,7 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController before_action :authorize_read_cycle_analytics! def show - @cycle_analytics = ::CycleAnalytics.new(@project, from: start_date(cycle_analytics_params), user: current_user) + @cycle_analytics = ::CycleAnalytics.new(@project, from: start_date(cycle_analytics_params)) respond_to do |format| format.html @@ -55,7 +55,7 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController { summary: summary, stats: stats, - permissions: @cycle_analytics.permissions + permissions: @cycle_analytics.permissions(user: current_user) } end end diff --git a/app/models/cycle_analytics.rb b/app/models/cycle_analytics.rb index a10e96d53bf..cb8e088d21d 100644 --- a/app/models/cycle_analytics.rb +++ b/app/models/cycle_analytics.rb @@ -1,10 +1,9 @@ class CycleAnalytics STAGES = %i[issue plan code test review staging production].freeze - def initialize(project, from:, user:) + def initialize(project, from:) @project = project @from = from - @user = user @fetcher = Gitlab::CycleAnalytics::MetricsFetcher.new(project: project, from: from, branch: nil) end @@ -12,8 +11,8 @@ class CycleAnalytics @summary ||= Summary.new(@project, from: @from) end - def permissions - Gitlab::CycleAnalytics::Permissions.get(user: @user, project: @project) + def permissions(user:) + Gitlab::CycleAnalytics::Permissions.get(user: user, project: @project) end def issue diff --git a/lib/gitlab/cycle_analytics/permissions.rb b/lib/gitlab/cycle_analytics/permissions.rb index 121b723f7be..bef3b95ff1b 100644 --- a/lib/gitlab/cycle_analytics/permissions.rb +++ b/lib/gitlab/cycle_analytics/permissions.rb @@ -2,9 +2,12 @@ module Gitlab module CycleAnalytics class Permissions STAGE_PERMISSIONS = { - read_build: [:test, :staging], - read_issue: [:issue, :production], - read_merge_request: [:code, :review] + issue: :read_issue, + code: :read_merge_request, + test: :read_build, + review: :read_merge_request, + staging: :read_build, + production: :read_issue, }.freeze def self.get(*args) @@ -30,15 +33,7 @@ module Gitlab def authorized_stage?(stage) return false unless authorize_project(:read_cycle_analytics) - permissions_for_stage(stage).keys.each do |permission| - return false unless authorize_project(permission) - end - - true - end - - def permissions_for_stage(stage) - STAGE_PERMISSIONS.select { |_permission, stages| stages.include?(stage) } + STAGE_PERMISSIONS[stage] ? authorize_project(STAGE_PERMISSIONS[stage]) : true end def authorize_project(permission) diff --git a/spec/models/cycle_analytics/code_spec.rb b/spec/models/cycle_analytics/code_spec.rb index 7e5941c39a6..7691d690db0 100644 --- a/spec/models/cycle_analytics/code_spec.rb +++ b/spec/models/cycle_analytics/code_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#code', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date, user: user) } + subject { CycleAnalytics.new(project, from: from_date) } context 'with deployment' do generate_cycle_analytics_spec( diff --git a/spec/models/cycle_analytics/issue_spec.rb b/spec/models/cycle_analytics/issue_spec.rb index ff5c95c08cb..f649b44d367 100644 --- a/spec/models/cycle_analytics/issue_spec.rb +++ b/spec/models/cycle_analytics/issue_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#issue', models: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date, user: user) } + subject { CycleAnalytics.new(project, from: from_date) } generate_cycle_analytics_spec( phase: :issue, diff --git a/spec/models/cycle_analytics/plan_spec.rb b/spec/models/cycle_analytics/plan_spec.rb index 16db10e4a1e..2cdefbeef21 100644 --- a/spec/models/cycle_analytics/plan_spec.rb +++ b/spec/models/cycle_analytics/plan_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#plan', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date, user: user) } + subject { CycleAnalytics.new(project, from: from_date) } generate_cycle_analytics_spec( phase: :plan, diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb index 8d5460c1e5a..1f5e5cab92d 100644 --- a/spec/models/cycle_analytics/production_spec.rb +++ b/spec/models/cycle_analytics/production_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#production', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date, user: user) } + subject { CycleAnalytics.new(project, from: from_date) } generate_cycle_analytics_spec( phase: :production, diff --git a/spec/models/cycle_analytics/review_spec.rb b/spec/models/cycle_analytics/review_spec.rb index 9f3cb06c2df..0ed080a42b1 100644 --- a/spec/models/cycle_analytics/review_spec.rb +++ b/spec/models/cycle_analytics/review_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#review', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date, user: user) } + subject { CycleAnalytics.new(project, from: from_date) } generate_cycle_analytics_spec( phase: :review, diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb index 1feb6326c31..af1c4477ddb 100644 --- a/spec/models/cycle_analytics/staging_spec.rb +++ b/spec/models/cycle_analytics/staging_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#staging', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date, user: user) } + subject { CycleAnalytics.new(project, from: from_date) } generate_cycle_analytics_spec( phase: :staging, diff --git a/spec/models/cycle_analytics/test_spec.rb b/spec/models/cycle_analytics/test_spec.rb index ff7f03ca5fb..02ddfeed9c1 100644 --- a/spec/models/cycle_analytics/test_spec.rb +++ b/spec/models/cycle_analytics/test_spec.rb @@ -6,7 +6,7 @@ describe 'CycleAnalytics#test', feature: true do let(:project) { create(:project) } let(:from_date) { 10.days.ago } let(:user) { create(:user, :admin) } - subject { CycleAnalytics.new(project, from: from_date, user: user) } + subject { CycleAnalytics.new(project, from: from_date) } generate_cycle_analytics_spec( phase: :test, -- cgit v1.2.1