From ca82725e5ef417b22ce7f3736b5dbd17f5924578 Mon Sep 17 00:00:00 2001 From: Jonas Chromik Date: Tue, 22 Mar 2016 13:48:51 +0100 Subject: [PATCH] move cron jobs feature flag to permissions --- lib/travis/api/v3/access_control/generic.rb | 4 +-- lib/travis/api/v3/permissions/cron.rb | 6 +++- lib/travis/api/v3/permissions/repository.rb | 2 +- lib/travis/api/v3/queries/crons.rb | 6 ++-- lib/travis/api/v3/services/cron/create.rb | 1 - lib/travis/api/v3/services/cron/delete.rb | 1 - lib/travis/api/v3/services/cron/find.rb | 1 - lib/travis/api/v3/services/cron/for_branch.rb | 1 - .../api/v3/services/crons/for_repository.rb | 5 ++-- lib/travis/api/v3/services/crons/start.rb | 1 - spec/v3/services/cron/create_spec.rb | 28 +++++++++++++------ spec/v3/services/cron/delete_spec.rb | 21 ++++++++------ spec/v3/services/cron/find_spec.rb | 23 ++++++++------- spec/v3/services/cron/for_branch_spec.rb | 18 ++++++------ spec/v3/services/crons/for_repository_spec.rb | 17 +++++------ .../repositories/for_current_user_spec.rb | 2 +- spec/v3/services/repository/find_spec.rb | 8 +++--- 17 files changed, 83 insertions(+), 62 deletions(-) diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index c232f879..0344b35b 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -60,11 +60,11 @@ module Travis::API::V3 end def cron_visible?(cron) - visible? cron.branch.repository + Travis::Features.owner_active?(:cron, cron.branch.repository.owner) and visible? cron.branch.repository end def cron_writable?(cron) - writable? cron.branch.repository + Travis::Features.owner_active?(:cron, cron.branch.repository.owner) and writable? cron.branch.repository end def job_visible?(job) diff --git a/lib/travis/api/v3/permissions/cron.rb b/lib/travis/api/v3/permissions/cron.rb index 815dd17c..c94b5a70 100644 --- a/lib/travis/api/v3/permissions/cron.rb +++ b/lib/travis/api/v3/permissions/cron.rb @@ -3,7 +3,11 @@ require 'travis/api/v3/permissions/generic' module Travis::API::V3 class Permissions::Cron < Permissions::Generic def delete? - write? + write? and Travis::Features.owner_active?(:cron, object.branch.repository.owner) + end + + def start? + Travis::Features.owner_active?(:cron, object.branch.repository.owner) end end end diff --git a/lib/travis/api/v3/permissions/repository.rb b/lib/travis/api/v3/permissions/repository.rb index dcdddbfa..81fb91a1 100644 --- a/lib/travis/api/v3/permissions/repository.rb +++ b/lib/travis/api/v3/permissions/repository.rb @@ -23,7 +23,7 @@ module Travis::API::V3 end def create_cron? - write? + Travis::Features.owner_active?(:cron, object.owner) and write? end end end diff --git a/lib/travis/api/v3/queries/crons.rb b/lib/travis/api/v3/queries/crons.rb index c69ec907..9a9249fc 100644 --- a/lib/travis/api/v3/queries/crons.rb +++ b/lib/travis/api/v3/queries/crons.rb @@ -10,7 +10,7 @@ module Travis::API::V3 Models::Cron.all.each do |cron| if cron.next_enqueuing <= Time.now - start(cron.branch) + start(cron) started.push cron end end @@ -18,8 +18,10 @@ module Travis::API::V3 started end - def start(branch) + def start(cron) + branch = cron.branch raise ServerError, 'repository does not have a github_id'.freeze unless branch.repository.github_id + access_control.permissions(cron).start! user_id = branch.repository.users.detect { |u| u.github_oauth_token }.id diff --git a/lib/travis/api/v3/services/cron/create.rb b/lib/travis/api/v3/services/cron/create.rb index 494f263e..cc8fc819 100644 --- a/lib/travis/api/v3/services/cron/create.rb +++ b/lib/travis/api/v3/services/cron/create.rb @@ -5,7 +5,6 @@ module Travis::API::V3 def run! raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise InsufficientAccess unless Travis::Features.feature_active?(:cron) raise NotFound unless repository = find(:repository) raise NotFound unless branch = find(:branch, repository) raise Error.new('Invalid value for interval. Interval must be "daily", "weekly" or "monthly"!', status: 422) unless ["daily", "weekly", "monthly"].include?(params["interval"]) diff --git a/lib/travis/api/v3/services/cron/delete.rb b/lib/travis/api/v3/services/cron/delete.rb index 256f97c7..c5d9287d 100644 --- a/lib/travis/api/v3/services/cron/delete.rb +++ b/lib/travis/api/v3/services/cron/delete.rb @@ -4,7 +4,6 @@ module Travis::API::V3 def run! raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise InsufficientAccess unless Travis::Features.feature_active?(:cron) cron = find access_control.permissions(cron).delete! cron.destroy diff --git a/lib/travis/api/v3/services/cron/find.rb b/lib/travis/api/v3/services/cron/find.rb index 33a337d6..6a191d7a 100644 --- a/lib/travis/api/v3/services/cron/find.rb +++ b/lib/travis/api/v3/services/cron/find.rb @@ -3,7 +3,6 @@ module Travis::API::V3 #params :id def run! - raise InsufficientAccess unless Travis::Features.feature_active?(:cron) find end end diff --git a/lib/travis/api/v3/services/cron/for_branch.rb b/lib/travis/api/v3/services/cron/for_branch.rb index d2396e9b..0290e112 100644 --- a/lib/travis/api/v3/services/cron/for_branch.rb +++ b/lib/travis/api/v3/services/cron/for_branch.rb @@ -2,7 +2,6 @@ module Travis::API::V3 class Services::Cron::ForBranch < Service def run! - raise InsufficientAccess unless Travis::Features.feature_active?(:cron) query.find_for_branch(find(:branch, find(:repository))) end end diff --git a/lib/travis/api/v3/services/crons/for_repository.rb b/lib/travis/api/v3/services/crons/for_repository.rb index 2b746fc1..97fd4a81 100644 --- a/lib/travis/api/v3/services/crons/for_repository.rb +++ b/lib/travis/api/v3/services/crons/for_repository.rb @@ -3,8 +3,9 @@ module Travis::API::V3 paginate def run! - raise InsufficientAccess unless Travis::Features.feature_active?(:cron) - query.find(find(:repository)) + repo = find(:repository) + raise InsufficientAccess unless Travis::Features.owner_active?(:cron, repo.owner) + query.find(repo) end end end diff --git a/lib/travis/api/v3/services/crons/start.rb b/lib/travis/api/v3/services/crons/start.rb index 0bfa3a16..104d43f5 100644 --- a/lib/travis/api/v3/services/crons/start.rb +++ b/lib/travis/api/v3/services/crons/start.rb @@ -2,7 +2,6 @@ module Travis::API::V3 class Services::Crons::Start < Service def run! - raise InsufficientAccess unless Travis::Features.feature_active?(:cron) query.start_all() end diff --git a/spec/v3/services/cron/create_spec.rb b/spec/v3/services/cron/create_spec.rb index f007f106..467f6bf1 100644 --- a/spec/v3/services/cron/create_spec.rb +++ b/spec/v3/services/cron/create_spec.rb @@ -12,17 +12,26 @@ describe Travis::API::V3::Services::Cron::Create do let(:parsed_body) { JSON.load(body) } before do - Travis::Features.enable_for_all(:cron) + Travis::Features.activate_owner(:cron, repo.owner) end - describe "no Feature enabled" do - before { Travis::Features.disable_for_all(:cron) } + describe "creating a cron job with feature flag disabled" do + before { Travis::Features.deactivate_owner(:cron, repo.owner) } before { post("/v3/repo/#{repo.id}/branch/#{branch.name}/cron", options, headers)} - example { expect(parsed_body).to be == { - "@type"=> "error", - "error_type"=> "insufficient_access", - "error_message"=> "forbidden" - }} + example { expect(parsed_body).to be == { + "@type" => "error", + "error_type" => "insufficient_access", + "error_message" => "operation requires create_cron access to repository", + "resource_type" => "repository", + "permission" => "create_cron", + "repository" => { + "@type" => "repository", + "@href" => "/repo/#{repo.id}", # should be /v3/repo/#{repo.id} + "@representation" => "minimal", + "id" => repo.id, + "name" => "minimal", + "slug" => "svenfuchs/minimal" } + }} end describe "creating a cron job" do @@ -37,7 +46,8 @@ describe Travis::API::V3::Services::Cron::Create do "@representation" => "standard", "@permissions" => { "read" => true, - "delete" => true }, + "delete" => true, + "start" => true }, "id" => current_cron.id, "repository" => { "@type" => "repository", diff --git a/spec/v3/services/cron/delete_spec.rb b/spec/v3/services/cron/delete_spec.rb index ee920566..601d7bb9 100644 --- a/spec/v3/services/cron/delete_spec.rb +++ b/spec/v3/services/cron/delete_spec.rb @@ -9,17 +9,19 @@ describe Travis::API::V3::Services::Cron::Delete do let(:parsed_body) { JSON.load(body) } before do - Travis::Features.enable_for_all(:cron) + Travis::Features.activate_owner(:cron, repo.owner) end - describe "no Feature enabled" do - before { Travis::Features.disable_for_all(:cron) } + describe "deleting cron jobs with feature disabled" do + before { Travis::API::V3::Models::Permission.create(repository: repo, user: repo.owner, push: true) } + before { Travis::Features.deactivate_owner(:cron, repo.owner) } before { delete("/v3/cron/#{cron.id}", {}, headers)} - example { expect(parsed_body).to be == { - "@type"=> "error", - "error_type"=> "insufficient_access", - "error_message"=> "forbidden" - }} + example { expect(parsed_body).to be == { + "@type" => "error", + "error_type" => "not_found", + "error_message" => "cron not found (or insufficient access)", + "resource_type" => "cron" + }} end describe "deleting a cron job by id" do @@ -33,7 +35,8 @@ describe Travis::API::V3::Services::Cron::Delete do "@representation" => "standard", "@permissions" => { "read" => true, - "delete" => true }, + "delete" => true, + "start" => true }, "id" => cron.id, "repository" => { "@type" => "repository", diff --git a/spec/v3/services/cron/find_spec.rb b/spec/v3/services/cron/find_spec.rb index 4f165e2b..8ae7f2c6 100644 --- a/spec/v3/services/cron/find_spec.rb +++ b/spec/v3/services/cron/find_spec.rb @@ -7,17 +7,18 @@ describe Travis::API::V3::Services::Cron::Find do let(:parsed_body) { JSON.load(body) } before do - Travis::Features.enable_for_all(:cron) + Travis::Features.activate_owner(:cron, repo.owner) end - describe "no Feature enabled" do - before { Travis::Features.disable_for_all(:cron) } + describe "find cron job with feature disabled" do + before { Travis::Features.deactivate_owner(:cron, repo.owner) } before { get("/v3/cron/#{cron.id}") } - example { expect(parsed_body).to be == { - "@type"=> "error", - "error_type"=> "insufficient_access", - "error_message"=> "forbidden" - }} + example { expect(parsed_body).to be == { + "@type" => "error", + "error_type" => "not_found", + "error_message" => "cron not found (or insufficient access)", + "resource_type" => "cron" + }} end describe "fetching a cron job by id" do @@ -29,7 +30,8 @@ describe Travis::API::V3::Services::Cron::Find do "@representation" => "standard", "@permissions" => { "read" => true, - "delete" => false }, + "delete" => false, + "start" => true }, "id" => cron.id, "repository" => { "@type" => "repository", @@ -87,7 +89,8 @@ describe Travis::API::V3::Services::Cron::Find do "@representation" => "standard", "@permissions" => { "read" => true, - "delete" => false }, + "delete" => false, + "start" => true }, "id" => cron.id, "repository" => { "@type" => "repository", diff --git a/spec/v3/services/cron/for_branch_spec.rb b/spec/v3/services/cron/for_branch_spec.rb index ad445b5a..b698ebe1 100644 --- a/spec/v3/services/cron/for_branch_spec.rb +++ b/spec/v3/services/cron/for_branch_spec.rb @@ -7,17 +7,18 @@ describe Travis::API::V3::Services::Cron::ForBranch do let(:parsed_body) { JSON.load(body) } before do - Travis::Features.enable_for_all(:cron) + Travis::Features.activate_owner(:cron, repo.owner) end - describe "no Feature enabled" do - before { Travis::Features.disable_for_all(:cron) } + describe "find cron job for branch with feature disabled" do + before { Travis::Features.deactivate_owner(:cron, repo.owner) } before { get("/v3/repo/#{repo.id}/branch/#{branch.name}/cron") } example { expect(parsed_body).to be == { - "@type"=> "error", - "error_type"=> "insufficient_access", - "error_message"=> "forbidden" - }} + "@type" => "error", + "error_type" => "not_found", + "error_message" => "cron not found (or insufficient access)", + "resource_type" => "cron" + }} end describe "fetching all crons by repo id" do @@ -30,7 +31,8 @@ describe Travis::API::V3::Services::Cron::ForBranch do "@representation" => "standard", "@permissions" => { "read" => true, - "delete" => false }, + "delete" => false, + "start" => true }, "id" => cron.id, "repository" => { "@type" => "repository", diff --git a/spec/v3/services/crons/for_repository_spec.rb b/spec/v3/services/crons/for_repository_spec.rb index 2d005f24..05e37102 100644 --- a/spec/v3/services/crons/for_repository_spec.rb +++ b/spec/v3/services/crons/for_repository_spec.rb @@ -7,17 +7,17 @@ describe Travis::API::V3::Services::Crons::ForRepository do let(:parsed_body) { JSON.load(body) } before do - Travis::Features.enable_for_all(:cron) + Travis::Features.activate_owner(:cron, repo.owner) end - describe "no Feature enabled" do - before { Travis::Features.disable_for_all(:cron) } + describe "fetching all crons by repo id with feature disabled" do + before { Travis::Features.deactivate_owner(:cron, repo.owner) } before { get("/v3/repo/#{repo.id}/crons") } example { expect(parsed_body).to be == { - "@type"=> "error", - "error_type"=> "insufficient_access", - "error_message"=> "forbidden" - }} + "@type" => "error", + "error_type" => "insufficient_access", + "error_message" => "forbidden" + }} end describe "fetching all crons by repo id" do @@ -51,7 +51,8 @@ describe Travis::API::V3::Services::Crons::ForRepository do "@representation" => "standard", "@permissions" => { "read" => true, - "delete" => false }, + "delete" => false, + "start" => true }, "id" => cron.id, "repository" => { "@type" => "repository", diff --git a/spec/v3/services/repositories/for_current_user_spec.rb b/spec/v3/services/repositories/for_current_user_spec.rb index b27d7ea9..d414783a 100644 --- a/spec/v3/services/repositories/for_current_user_spec.rb +++ b/spec/v3/services/repositories/for_current_user_spec.rb @@ -45,7 +45,7 @@ describe Travis::API::V3::Services::Repositories::ForCurrentUser do "star" => true, "unstar" => true, "create_request" => true, - "create_cron" => true}, + "create_cron" => false}, "id" => repo.id, "name" => "minimal", "slug" => "svenfuchs/minimal", diff --git a/spec/v3/services/repository/find_spec.rb b/spec/v3/services/repository/find_spec.rb index 55c88ed0..b3c03556 100644 --- a/spec/v3/services/repository/find_spec.rb +++ b/spec/v3/services/repository/find_spec.rb @@ -152,7 +152,7 @@ describe Travis::API::V3::Services::Repository::Find do }} end - describe "private repository, authenticated as internal application with full access" do + describe "private repository without cron feature, authenticated as internal application with full access" do let(:app_name) { 'travis-example' } let(:app_secret) { '12345678' } let(:sign_opts) { "a=#{app_name}" } @@ -178,7 +178,7 @@ describe Travis::API::V3::Services::Repository::Find do "star" => true, "unstar" => true, "create_request" => true, - "create_cron" => true}, + "create_cron" => false}, "id" => repo.id, "name" => "minimal", "slug" => "svenfuchs/minimal", @@ -221,7 +221,7 @@ describe Travis::API::V3::Services::Repository::Find do }} end - describe "private repository, authenticated as internal application with full access, scoped to the right org" do + describe "private repository without cron feature, authenticated as internal application with full access, scoped to the right org" do let(:app_name) { 'travis-example' } let(:app_secret) { '12345678' } let(:sign_opts) { "a=#{app_name}:s=#{repo.owner_name}" } @@ -247,7 +247,7 @@ describe Travis::API::V3::Services::Repository::Find do "star" => true, "unstar" => true, "create_request" => true, - "create_cron" => true}, + "create_cron" => false}, "id" => repo.id, "name" => "minimal", "slug" => "svenfuchs/minimal",