From 026dc4cb989aca8a2cdc03f6cb393d5308732bff Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Thu, 9 Jun 2016 16:21:55 +0200 Subject: [PATCH 01/21] Use travis-settings to manage JSON settings field Since we use repository.settings as a kind of dump for all sorts of settings, some user-facing and some not, this lets us leave the db as it is, but pretend to have separate models for each "kind" of setting. --- Gemfile | 1 + Gemfile.lock | 8 ++++++++ lib/travis/api/v3/models/admin_settings.rb | 5 +++++ lib/travis/api/v3/models/repository.rb | 10 +++++++++- lib/travis/api/v3/models/settings.rb | 13 ++----------- lib/travis/api/v3/models/user_settings.rb | 8 ++++++++ lib/travis/api/v3/services/requests/create.rb | 6 +----- 7 files changed, 34 insertions(+), 17 deletions(-) create mode 100644 lib/travis/api/v3/models/admin_settings.rb create mode 100644 lib/travis/api/v3/models/user_settings.rb diff --git a/Gemfile b/Gemfile index ab1dc3bf..ccf97b60 100644 --- a/Gemfile +++ b/Gemfile @@ -11,6 +11,7 @@ gem 'travis-settings', github: 'travis-ci/travis-settings' gem 'travis-sidekiqs', github: 'travis-ci/travis-sidekiqs' gem 'travis-yaml', github: 'travis-ci/travis-yaml' +gem 'travis-settings', github: 'travis-ci/travis-settings' gem 'mustermann', github: 'rkh/mustermann' gem 'sinatra' gem 'sinatra-contrib', require: nil #github: 'sinatra/sinatra-contrib', require: nil diff --git a/Gemfile.lock b/Gemfile.lock index 0dda637b..96104e61 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -53,6 +53,14 @@ GIT activemodel virtus +GIT + remote: git://github.com/travis-ci/travis-settings.git + revision: d510e63b6c6f059cccae141c265e7a0c7236d1fd + specs: + travis-settings (0.0.1) + activemodel + virtus + GIT remote: git://github.com/travis-ci/travis-sidekiqs.git revision: c5d4a4abc6c3737f9c43d3333efb94daa18b9fbb diff --git a/lib/travis/api/v3/models/admin_settings.rb b/lib/travis/api/v3/models/admin_settings.rb new file mode 100644 index 00000000..ee1d9d2a --- /dev/null +++ b/lib/travis/api/v3/models/admin_settings.rb @@ -0,0 +1,5 @@ +module Travis::API::V3 + class Models::AdminSettings < Travis::Settings::Model + attribute :api_builds_rate_limit, Integer + end +end diff --git a/lib/travis/api/v3/models/repository.rb b/lib/travis/api/v3/models/repository.rb index b462db88..1b14bd5f 100644 --- a/lib/travis/api/v3/models/repository.rb +++ b/lib/travis/api/v3/models/repository.rb @@ -65,7 +65,15 @@ module Travis::API::V3 end def settings - @settings ||= JSON.load(super) + @settings ||= JSON.load(super || '{}'.freeze) + end + + def user_settings + @user_settings ||= Models::UserSettings.new(settings) + end + + def admin_settings + @admin_settings ||= Models::AdminSettings.new(settings) end end end diff --git a/lib/travis/api/v3/models/settings.rb b/lib/travis/api/v3/models/settings.rb index 00949495..0d3e4593 100644 --- a/lib/travis/api/v3/models/settings.rb +++ b/lib/travis/api/v3/models/settings.rb @@ -1,12 +1,5 @@ module Travis::API::V3 class Models::Settings - DEFAULTS = { - 'builds_only_with_travis_yml' => false, - 'build_pushes' => true, - 'build_pull_requests' => true, - 'maximum_number_of_builds' => 0 - }.freeze - attr_reader :repository def initialize(repository) @@ -14,13 +7,11 @@ module Travis::API::V3 end def to_h - DEFAULTS.merge(repository.settings || {}) + repository.user_settings.to_hash end def update(settings = {}) - settings = to_h.merge(settings) - repository.settings.clear - settings.each { |k, v| repository.settings[k] = v } + repository.user_settings.update(settings) repository.save! end end diff --git a/lib/travis/api/v3/models/user_settings.rb b/lib/travis/api/v3/models/user_settings.rb new file mode 100644 index 00000000..42e6519e --- /dev/null +++ b/lib/travis/api/v3/models/user_settings.rb @@ -0,0 +1,8 @@ +module Travis::API::V3 + class Models::UserSettings < Travis::Settings::Model + attribute :builds_only_with_travis_yml, Boolean, default: false + attribute :build_pushes, Boolean, default: true + attribute :build_pull_requests, Boolean, default: true + attribute :maximum_number_of_builds, Integer, default: 0 + end +end diff --git a/lib/travis/api/v3/services/requests/create.rb b/lib/travis/api/v3/services/requests/create.rb index ea3827de..87c54d2d 100644 --- a/lib/travis/api/v3/services/requests/create.rb +++ b/lib/travis/api/v3/services/requests/create.rb @@ -23,11 +23,7 @@ module Travis::API::V3 end def limit(repository) - if repository.settings.nil? - Travis.config.requests_create_api_limit || LIMIT - else - repository.settings["api_builds_rate_limit"] || Travis.config.requests_create_api_limit || LIMIT - end + repository.admin_settings.api_builds_rate_limit || Travis.config.requests_create_api_limit || LIMIT end def remaining_requests(repository) From cae6da540fabb42fd83d6e03b86277bc5a86ada9 Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 14 Jun 2016 11:59:15 +0200 Subject: [PATCH 02/21] Add env vars endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds API V3 endpoints for querying, creating, modifying and deleting environment variables. It has no concept of encryption yet and should be considered a work in progress. We should also talk about the slightly off-track approach in the EnvVars::Create service – maybe there's a way to standardise the querying and rendering for post requests? :coffee: --- lib/travis/api/v3.rb | 1 + lib/travis/api/v3/access_control/generic.rb | 8 +++ lib/travis/api/v3/models/env_var.rb | 18 +++++ lib/travis/api/v3/models/env_vars.rb | 11 +++ lib/travis/api/v3/models/repository.rb | 6 ++ lib/travis/api/v3/queries/env_var.rb | 21 ++++++ lib/travis/api/v3/queries/env_vars.rb | 27 ++++++++ lib/travis/api/v3/renderer/env_var.rb | 5 ++ lib/travis/api/v3/renderer/env_vars.rb | 6 ++ lib/travis/api/v3/routes.rb | 13 ++++ lib/travis/api/v3/services.rb | 2 + lib/travis/api/v3/services/env_var/delete.rb | 12 ++++ lib/travis/api/v3/services/env_var/find.rb | 12 ++++ lib/travis/api/v3/services/env_var/update.rb | 12 ++++ lib/travis/api/v3/services/env_vars/create.rb | 13 ++++ .../v3/services/env_vars/for_repository.rb | 9 +++ spec/spec_helper.rb | 34 ++++++++++ spec/support/shared_examples.rb | 34 ++++++++++ spec/v3/services/env_var/delete_spec.rb | 33 +++++++++ spec/v3/services/env_var/find_spec.rb | 43 ++++++++++++ spec/v3/services/env_var/update_spec.rb | 50 ++++++++++++++ spec/v3/services/env_vars/create_spec.rb | 67 +++++++++++++++++++ .../services/env_vars/for_repository_spec.rb | 59 ++++++++++++++++ spec/v3/services/settings_spec.rb | 9 +-- 24 files changed, 497 insertions(+), 8 deletions(-) create mode 100644 lib/travis/api/v3/models/env_var.rb create mode 100644 lib/travis/api/v3/models/env_vars.rb create mode 100644 lib/travis/api/v3/queries/env_var.rb create mode 100644 lib/travis/api/v3/queries/env_vars.rb create mode 100644 lib/travis/api/v3/renderer/env_var.rb create mode 100644 lib/travis/api/v3/renderer/env_vars.rb create mode 100644 lib/travis/api/v3/services/env_var/delete.rb create mode 100644 lib/travis/api/v3/services/env_var/find.rb create mode 100644 lib/travis/api/v3/services/env_var/update.rb create mode 100644 lib/travis/api/v3/services/env_vars/create.rb create mode 100644 lib/travis/api/v3/services/env_vars/for_repository.rb create mode 100644 spec/support/shared_examples.rb create mode 100644 spec/v3/services/env_var/delete_spec.rb create mode 100644 spec/v3/services/env_var/find_spec.rb create mode 100644 spec/v3/services/env_var/update_spec.rb create mode 100644 spec/v3/services/env_vars/create_spec.rb create mode 100644 spec/v3/services/env_vars/for_repository_spec.rb diff --git a/lib/travis/api/v3.rb b/lib/travis/api/v3.rb index 610de9cd..f05aec5e 100644 --- a/lib/travis/api/v3.rb +++ b/lib/travis/api/v3.rb @@ -31,6 +31,7 @@ module Travis AlreadySyncing = ClientError .create('sync already in progress', status: 409) BuildAlreadyRunning = ClientError .create('build already running, cannot restart', status: 409) BuildNotCancelable = ClientError .create('build is not running, cannot cancel', status: 409) + DuplicateResource = ClientError .create('resource already exists', status: 409) EntityMissing = NotFound .create(type: 'not_found') InsufficientAccess = ClientError .create(status: 403) JobAlreadyRunning = ClientError .create('job already running, cannot restart', status: 409) diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index 92b68e1e..f3671da7 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -96,6 +96,14 @@ module Travis::API::V3 repository_visible?(settings.repository) end + def env_vars_visible?(env_vars) + repository_visible?(env_vars.repository) + end + + def env_var_visible?(env_var) + repository_visible?(env_var.repository) + end + def private_repository_visible?(repository) false end diff --git a/lib/travis/api/v3/models/env_var.rb b/lib/travis/api/v3/models/env_var.rb new file mode 100644 index 00000000..1d4c7f8f --- /dev/null +++ b/lib/travis/api/v3/models/env_var.rb @@ -0,0 +1,18 @@ +module Travis::API::V3 + class Models::EnvVar < Travis::Settings::Model + attribute :id, Integer + attribute :name, String + attribute :value, String + attribute :public, Boolean + attribute :repository_id, Integer + + def repository + @repository ||= Models::Repository.find(repository_id) + end + + validates_each :id, :name do |record, attr, value| + others = record.repository.env_vars.select { |ev| ev.id != record.id } + record.errors.add(:base, :duplicate_resource) if others.find { |ev| ev.send(attr) == record.send(attr) } + end + end +end diff --git a/lib/travis/api/v3/models/env_vars.rb b/lib/travis/api/v3/models/env_vars.rb new file mode 100644 index 00000000..1e4b7d08 --- /dev/null +++ b/lib/travis/api/v3/models/env_vars.rb @@ -0,0 +1,11 @@ +module Travis::API::V3 + class Models::EnvVars < Travis::Settings::Collection + model Models::EnvVar + + def repository + @repository ||= Models::Repository.find(additional_attributes[:repository_id]) + end + + undef :to_hash + end +end diff --git a/lib/travis/api/v3/models/repository.rb b/lib/travis/api/v3/models/repository.rb index 1b14bd5f..dfbd1eba 100644 --- a/lib/travis/api/v3/models/repository.rb +++ b/lib/travis/api/v3/models/repository.rb @@ -75,5 +75,11 @@ module Travis::API::V3 def admin_settings @admin_settings ||= Models::AdminSettings.new(settings) end + + def env_vars + @env_vars ||= Models::EnvVars.new.tap do |vars| + vars.load(settings.fetch('env_vars', []), repository_id: self.id) + end + end end end diff --git a/lib/travis/api/v3/queries/env_var.rb b/lib/travis/api/v3/queries/env_var.rb new file mode 100644 index 00000000..4ffaeab9 --- /dev/null +++ b/lib/travis/api/v3/queries/env_var.rb @@ -0,0 +1,21 @@ +module Travis::API::V3 + class Queries::EnvVar < Query + params :id, :name, :value, :public, prefix: :env_var + + def find(repository) + repository.env_vars.find(id) + end + + def update(repository) + if env_var = find(repository) + env_var.update(env_var_params) + repository.save! + env_var + end + end + + def delete(repository) + repository.env_vars.destroy(id) + end + end +end diff --git a/lib/travis/api/v3/queries/env_vars.rb b/lib/travis/api/v3/queries/env_vars.rb new file mode 100644 index 00000000..0544cbbe --- /dev/null +++ b/lib/travis/api/v3/queries/env_vars.rb @@ -0,0 +1,27 @@ +module Travis::API::V3 + class Queries::EnvVars < Query + params :id, :name, :value, :public, prefix: :env_var + + def find(repository) + repository.env_vars + end + + def create(repository) + env_var = repository.env_vars.create(env_var_params) + handle_errors(env_var) unless env_var.valid? + repository.save! + env_var + end + + private + + def handle_errors(env_var) + base = env_var.errors[:base] + case + when base.include?(:format) then raise WrongParams + when base.include?(:duplicate_resource) then raise DuplicateResource + else raise ServerError + end + end + end +end diff --git a/lib/travis/api/v3/renderer/env_var.rb b/lib/travis/api/v3/renderer/env_var.rb new file mode 100644 index 00000000..6d2f955a --- /dev/null +++ b/lib/travis/api/v3/renderer/env_var.rb @@ -0,0 +1,5 @@ +module Travis::API::V3 + class Renderer::EnvVar < Renderer::ModelRenderer + representation :standard, :id, :name, :value, :public + end +end diff --git a/lib/travis/api/v3/renderer/env_vars.rb b/lib/travis/api/v3/renderer/env_vars.rb new file mode 100644 index 00000000..9a47a84c --- /dev/null +++ b/lib/travis/api/v3/renderer/env_vars.rb @@ -0,0 +1,6 @@ +module Travis::API::V3 + class Renderer::EnvVars < Renderer::CollectionRenderer + type :env_vars + collection_key :env_vars + end +end diff --git a/lib/travis/api/v3/routes.rb b/lib/travis/api/v3/routes.rb index 585dc1d8..7767527e 100644 --- a/lib/travis/api/v3/routes.rb +++ b/lib/travis/api/v3/routes.rb @@ -123,6 +123,19 @@ module Travis::API::V3 get :find patch :update end + + resource :env_vars do + route '/env_vars' + get :for_repository + post :create + end + + resource :env_var do + route '/env_var/{env_var.id}' + get :find + patch :update + delete :delete + end end resource :user do diff --git a/lib/travis/api/v3/services.rb b/lib/travis/api/v3/services.rb index 32b30b7c..8a48c895 100644 --- a/lib/travis/api/v3/services.rb +++ b/lib/travis/api/v3/services.rb @@ -11,6 +11,8 @@ module Travis::API::V3 Builds = Module.new { extend Services } Cron = Module.new { extend Services } Crons = Module.new { extend Services } + EnvVar = Module.new { extend Services } + EnvVars = Module.new { extend Services } Job = Module.new { extend Services } Jobs = Module.new { extend Services } Lint = Module.new { extend Services } diff --git a/lib/travis/api/v3/services/env_var/delete.rb b/lib/travis/api/v3/services/env_var/delete.rb new file mode 100644 index 00000000..d11f20f0 --- /dev/null +++ b/lib/travis/api/v3/services/env_var/delete.rb @@ -0,0 +1,12 @@ +module Travis::API::V3 + class Services::EnvVar::Delete < Service + params :id, prefix: :repository + params :id, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + query.delete(repository) + end + end +end diff --git a/lib/travis/api/v3/services/env_var/find.rb b/lib/travis/api/v3/services/env_var/find.rb new file mode 100644 index 00000000..476c1b3a --- /dev/null +++ b/lib/travis/api/v3/services/env_var/find.rb @@ -0,0 +1,12 @@ +module Travis::API::V3 + class Services::EnvVar::Find < Service + params :id, prefix: :repository + params :id, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + query.find(repository) + end + end +end diff --git a/lib/travis/api/v3/services/env_var/update.rb b/lib/travis/api/v3/services/env_var/update.rb new file mode 100644 index 00000000..6caa751a --- /dev/null +++ b/lib/travis/api/v3/services/env_var/update.rb @@ -0,0 +1,12 @@ +module Travis::API::V3 + class Services::EnvVar::Update < Service + params :id, prefix: :repository + params :id, :name, :value, :public, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + query.update(repository) + end + end +end diff --git a/lib/travis/api/v3/services/env_vars/create.rb b/lib/travis/api/v3/services/env_vars/create.rb new file mode 100644 index 00000000..02839fc2 --- /dev/null +++ b/lib/travis/api/v3/services/env_vars/create.rb @@ -0,0 +1,13 @@ +module Travis::API::V3 + class Services::EnvVars::Create < Service + params :id, prefix: :repository + params :id, :name, :value, :public, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + env_var = query(:env_vars).create(repository) + result(:env_var, env_var, status: 201) + end + end +end diff --git a/lib/travis/api/v3/services/env_vars/for_repository.rb b/lib/travis/api/v3/services/env_vars/for_repository.rb new file mode 100644 index 00000000..1cb1b4cf --- /dev/null +++ b/lib/travis/api/v3/services/env_vars/for_repository.rb @@ -0,0 +1,9 @@ +module Travis::API::V3 + class Services::EnvVars::ForRepository < Service + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + find(:env_vars, repository) + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 748561f5..3d6a4f3a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -23,6 +23,40 @@ require 'support/payloads' require 'support/private_key' require 'support/s3' require 'support/test_helpers' +require 'support/shared_examples' + +Travis.logger = Logger.new(StringIO.new) +Travis::Api::App.setup +Travis.config.client_domain = "www.example.com" +Travis.config.endpoints.ssh_key = true + +module TestHelpers + include Sinatra::TestHelpers + + def custom_endpoints + @custom_endpoints ||= [] + end + + def add_settings_endpoint(name, options = {}) + if options[:singleton] + Travis::Api::App::SingletonSettingsEndpoint.subclass(name) + else + Travis::Api::App::SettingsEndpoint.subclass(name) + end + set_app Travis::Api::App.new + end + + def add_endpoint(prefix, &block) + endpoint = Sinatra.new(Travis::Api::App::Endpoint, &block) + endpoint.set(prefix: prefix) + set_app Travis::Api::App.new + custom_endpoints << endpoint + end + + def parsed_body + MultiJson.decode(body) + end +end RSpec.configure do |c| c.mock_framework = :mocha diff --git a/spec/support/shared_examples.rb b/spec/support/shared_examples.rb new file mode 100644 index 00000000..b1964bfb --- /dev/null +++ b/spec/support/shared_examples.rb @@ -0,0 +1,34 @@ +RSpec.shared_examples 'not authenticated' do + example { expect(last_response.status).to eq(403) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_type' => 'login_required', + 'error_message' => 'login required' + ) + end +end + +RSpec.shared_examples 'missing repo' do + example { expect(last_response.status).to eq(404) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_message' => 'repository not found (or insufficient access)', + 'error_type' => 'not_found', + 'resource_type' => 'repository' + ) + end +end + +RSpec.shared_examples 'missing env_var' do + example { expect(last_response.status).to eq 404 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_message' => 'env_var not found (or insufficient access)', + 'error_type' => 'not_found', + 'resource_type' => 'env_var' + ) + end +end diff --git a/spec/v3/services/env_var/delete_spec.rb b/spec/v3/services/env_var/delete_spec.rb new file mode 100644 index 00000000..9f8879c8 --- /dev/null +++ b/spec/v3/services/env_var/delete_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVar::Delete do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + + describe 'not authenticated' do + before { delete("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { delete("/v3/repo/999999999/env_var/foo", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, missing repo, missing env var' do + before { delete("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) } + include_examples 'missing env_var' + end + + describe 'authenticated, missing repo, existing env var' do + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + delete("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) + end + + example { expect(last_response.status).to eq 200 } + example { pending 'should we return an empty body here?' } + end +end diff --git a/spec/v3/services/env_var/find_spec.rb b/spec/v3/services/env_var/find_spec.rb new file mode 100644 index 00000000..0b9081b7 --- /dev/null +++ b/spec/v3/services/env_var/find_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVar::Find do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + + describe 'not authenticated' do + before { get("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { get("/v3/repo/999999999/env_var/foo", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, missing env var' do + before { get("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) } + include_examples 'missing env_var' + end + + describe 'authenticated, existing repo, existing env var' do + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + get("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) + end + + example { expect(last_response.status).to eq 200 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_var', + '@href' => "/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", + '@representation' => 'standard', + 'id' => env_var[:id], + 'name' => env_var[:name], + 'public' => env_var[:public], + 'value' => env_var[:value] + ) + end + end +end diff --git a/spec/v3/services/env_var/update_spec.rb b/spec/v3/services/env_var/update_spec.rb new file mode 100644 index 00000000..e23359c7 --- /dev/null +++ b/spec/v3/services/env_var/update_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVar::Update do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + let(:json_headers) { { 'CONTENT_TYPE' => 'application/json' } } + + describe 'not authenticated' do + before { patch("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { patch("/v3/repo/999999999/env_var/#{env_var[:id]}", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, missing env var' do + before { patch("/v3/repo/#{repo.id}/env_var/foo", {}, auth_headers) } + include_examples 'missing env_var' + end + + describe 'authenticated, existing repo, existing env var' do + let(:params) do + { + 'env_var.name' => 'QUX' + } + end + + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + patch("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", JSON.generate(params), auth_headers.merge(json_headers)) + end + + example { expect(last_response.status).to eq 200 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_var', + '@href' => '/v3/repo/1/env_var/abc', + '@representation' => 'standard', + 'id' => env_var[:id], + 'name' => params['env_var.name'], + 'value' => env_var[:value], + 'public' => env_var[:public] + ) + end + end +end diff --git a/spec/v3/services/env_vars/create_spec.rb b/spec/v3/services/env_vars/create_spec.rb new file mode 100644 index 00000000..52230df1 --- /dev/null +++ b/spec/v3/services/env_vars/create_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVars::Create do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + let(:json_headers) { { 'CONTENT_TYPE' => 'application/json' } } + + describe 'not authenticated' do + before { post("/v3/repo/#{repo.id}/env_vars", {}) } + include_examples 'not authenticated' + end + + describe 'authenticated, repo missing' do + before { post("/v3/repo/99999999/env_vars", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, env var already exists' do + let(:params) do + { + 'env_var.name' => 'FOO', + 'env_var.value' => 'bar', + 'env_var.public' => false + } + end + + before do + repo.update_attributes(settings: JSON.generate(env_vars: [{ id: 'abc', name: 'FOO', value: 'bar', public: false }])) + post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) + end + + example { expect(last_response.status).to eq 409 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_message' => 'resource already exists', + 'error_type' => 'duplicate_resource' + ) + end + end + + describe 'authenticated, existing repo, env var is new' do + let(:params) do + { + 'env_var.name' => 'FOO', + 'env_var.value' => 'bar', + 'env_var.public' => false + } + end + + before { post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) } + + example { expect(last_response.status).to eq 201 } + example do + response = JSON.load(body) + expect(response).to include( + '@type' => 'env_var', + '@representation' => 'standard', + 'name' => 'FOO', + 'value' => 'bar', + 'public' => false + ) + expect(response).to include('@href', 'id') + end + end +end diff --git a/spec/v3/services/env_vars/for_repository_spec.rb b/spec/v3/services/env_vars/for_repository_spec.rb new file mode 100644 index 00000000..60ade927 --- /dev/null +++ b/spec/v3/services/env_vars/for_repository_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVars::ForRepository do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + + describe 'not authenticated' do + before { get("/v3/repo/#{repo.id}/env_vars") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { get("/v3/repo/999999999/env_vars", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, no env vars' do + before do + get("/v3/repo/#{repo.id}/env_vars", {}, auth_headers) + end + example { expect(last_response.status).to eq(200) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_vars', + '@href' => "/v3/repo/#{repo.id}/env_vars", + '@representation' => 'standard', + 'env_vars' => [] + ) + end + end + + describe 'authenticated, existing repo, existing env vars' do + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + get("/v3/repo/#{repo.id}/env_vars", {}, auth_headers) + end + example { expect(last_response.status).to eq(200) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_vars', + '@href' => "/v3/repo/#{repo.id}/env_vars", + '@representation' => 'standard', + 'env_vars' => [ + { + '@type' => 'env_var', + '@href' => "/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", + '@representation' => 'standard', + 'id' => env_var[:id], + 'name' => env_var[:name], + 'value' => env_var[:value], + 'public' => env_var[:public] + } + ] + ) + end + end +end diff --git a/spec/v3/services/settings_spec.rb b/spec/v3/services/settings_spec.rb index b8189275..11b839b5 100644 --- a/spec/v3/services/settings_spec.rb +++ b/spec/v3/services/settings_spec.rb @@ -7,14 +7,7 @@ describe Travis::API::V3::Services::Settings, set_app: true do describe :Find do describe 'not authenticated' do before { get("/v3/repo/#{repo.id}/settings") } - example { expect(last_response.status).to eq(403) } - example do - expect(JSON.load(body)).to eq( - '@type' => 'error', - 'error_type' => 'login_required', - 'error_message' => 'login required' - ) - end + include_examples 'not authenticated' end describe 'authenticated, missing repo' do From 00fb01bd7a722873d78d1dceda6b8d7f9888a9fc Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 21 Jun 2016 12:23:34 +0200 Subject: [PATCH 03/21] Remove some duplication --- lib/travis/api/v3/access_control/generic.rb | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index f3671da7..30be8902 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -92,22 +92,15 @@ module Travis::API::V3 private_repository_visible?(repository) end - def settings_visible?(settings) - repository_visible?(settings.repository) - end - - def env_vars_visible?(env_vars) - repository_visible?(env_vars.repository) - end - - def env_var_visible?(env_var) - repository_visible?(env_var.repository) - end - def private_repository_visible?(repository) false end + def repository_attr_visible?(record) + repository_visible?(record.repository) + end + [:settings_visible?, :env_vars_visible?, :env_var_visible?].each { |m| alias_method m, :repository_attr_visible? } + def public_api? !Travis.config.private_api end From 81e93ca710cd72167b95af29899fbacbe7803a1f Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 21 Jun 2016 12:31:42 +0200 Subject: [PATCH 04/21] Better style in error handling --- lib/travis/api/v3/queries/env_vars.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/travis/api/v3/queries/env_vars.rb b/lib/travis/api/v3/queries/env_vars.rb index 0544cbbe..9c92d4d6 100644 --- a/lib/travis/api/v3/queries/env_vars.rb +++ b/lib/travis/api/v3/queries/env_vars.rb @@ -17,11 +17,9 @@ module Travis::API::V3 def handle_errors(env_var) base = env_var.errors[:base] - case - when base.include?(:format) then raise WrongParams - when base.include?(:duplicate_resource) then raise DuplicateResource - else raise ServerError - end + raise WrongParams if base.include?(:format) + raise DuplicateResource if base.include?(:duplicate_resource) + raise ServerError end end end From 88d900004232953f43039e0ac9584b6df5a0e147 Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 21 Jun 2016 13:17:44 +0200 Subject: [PATCH 05/21] Refactor repeated pattern into extracted method Seems like we'll be checking login status before finding a resource a lot, so here's a standard way to do it. --- lib/travis/api/v3/access_control/generic.rb | 4 ++++ lib/travis/api/v3/service.rb | 5 +++++ lib/travis/api/v3/services/env_var/delete.rb | 3 +-- lib/travis/api/v3/services/env_var/find.rb | 3 +-- lib/travis/api/v3/services/env_var/update.rb | 3 +-- lib/travis/api/v3/services/env_vars/create.rb | 3 +-- lib/travis/api/v3/services/env_vars/for_repository.rb | 3 +-- 7 files changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index 30be8902..48b4958f 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -30,6 +30,10 @@ module Travis::API::V3 false end + def full_access_or_logged_in? + full_access? || logged_in? + end + def visible_repositories(list) # naïve implementation, replaced with smart implementation in specific subclasses return list if full_access? diff --git a/lib/travis/api/v3/service.rb b/lib/travis/api/v3/service.rb index fa63ecf1..13b8f2aa 100644 --- a/lib/travis/api/v3/service.rb +++ b/lib/travis/api/v3/service.rb @@ -66,6 +66,11 @@ module Travis::API::V3 object end + def check_login_and_find(*args) + raise LoginRequired unless access_control.full_access_or_logged_in? + find(*args) + end + def not_found(actually_not_found = false, type = nil) type, actually_not_found = actually_not_found, false if actually_not_found.is_a? Symbol error = actually_not_found ? EntityMissing : NotFound diff --git a/lib/travis/api/v3/services/env_var/delete.rb b/lib/travis/api/v3/services/env_var/delete.rb index d11f20f0..3ea473ab 100644 --- a/lib/travis/api/v3/services/env_var/delete.rb +++ b/lib/travis/api/v3/services/env_var/delete.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) query.delete(repository) end end diff --git a/lib/travis/api/v3/services/env_var/find.rb b/lib/travis/api/v3/services/env_var/find.rb index 476c1b3a..17258043 100644 --- a/lib/travis/api/v3/services/env_var/find.rb +++ b/lib/travis/api/v3/services/env_var/find.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) query.find(repository) end end diff --git a/lib/travis/api/v3/services/env_var/update.rb b/lib/travis/api/v3/services/env_var/update.rb index 6caa751a..e83e9256 100644 --- a/lib/travis/api/v3/services/env_var/update.rb +++ b/lib/travis/api/v3/services/env_var/update.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, :name, :value, :public, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) query.update(repository) end end diff --git a/lib/travis/api/v3/services/env_vars/create.rb b/lib/travis/api/v3/services/env_vars/create.rb index 02839fc2..a2fd3ed1 100644 --- a/lib/travis/api/v3/services/env_vars/create.rb +++ b/lib/travis/api/v3/services/env_vars/create.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, :name, :value, :public, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) env_var = query(:env_vars).create(repository) result(:env_var, env_var, status: 201) end diff --git a/lib/travis/api/v3/services/env_vars/for_repository.rb b/lib/travis/api/v3/services/env_vars/for_repository.rb index 1cb1b4cf..60a88577 100644 --- a/lib/travis/api/v3/services/env_vars/for_repository.rb +++ b/lib/travis/api/v3/services/env_vars/for_repository.rb @@ -1,8 +1,7 @@ module Travis::API::V3 class Services::EnvVars::ForRepository < Service def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) find(:env_vars, repository) end end From 769ae71076874b38fb5a5d92e3143a985387fb87 Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Wed, 22 Jun 2016 16:50:11 +0200 Subject: [PATCH 06/21] Fix service specs --- spec/v3/services/env_var/delete_spec.rb | 2 +- spec/v3/services/env_var/find_spec.rb | 2 +- spec/v3/services/env_var/update_spec.rb | 2 +- spec/v3/services/env_vars/create_spec.rb | 2 +- spec/v3/services/env_vars/for_repository_spec.rb | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/v3/services/env_var/delete_spec.rb b/spec/v3/services/env_var/delete_spec.rb index 9f8879c8..870c69e9 100644 --- a/spec/v3/services/env_var/delete_spec.rb +++ b/spec/v3/services/env_var/delete_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVar::Delete do +describe Travis::API::V3::Services::EnvVar::Delete, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } diff --git a/spec/v3/services/env_var/find_spec.rb b/spec/v3/services/env_var/find_spec.rb index 0b9081b7..f80cfdc1 100644 --- a/spec/v3/services/env_var/find_spec.rb +++ b/spec/v3/services/env_var/find_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVar::Find do +describe Travis::API::V3::Services::EnvVar::Find, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } diff --git a/spec/v3/services/env_var/update_spec.rb b/spec/v3/services/env_var/update_spec.rb index e23359c7..bac9a065 100644 --- a/spec/v3/services/env_var/update_spec.rb +++ b/spec/v3/services/env_var/update_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVar::Update do +describe Travis::API::V3::Services::EnvVar::Update, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } diff --git a/spec/v3/services/env_vars/create_spec.rb b/spec/v3/services/env_vars/create_spec.rb index 52230df1..1755e593 100644 --- a/spec/v3/services/env_vars/create_spec.rb +++ b/spec/v3/services/env_vars/create_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVars::Create do +describe Travis::API::V3::Services::EnvVars::Create, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } diff --git a/spec/v3/services/env_vars/for_repository_spec.rb b/spec/v3/services/env_vars/for_repository_spec.rb index 60ade927..fd20de9d 100644 --- a/spec/v3/services/env_vars/for_repository_spec.rb +++ b/spec/v3/services/env_vars/for_repository_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVars::ForRepository do +describe Travis::API::V3::Services::EnvVars::ForRepository, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } From 0aeec3593513cd05e82cc58bcaa3c3b8d353d38c Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Wed, 22 Jun 2016 16:59:49 +0200 Subject: [PATCH 07/21] Fix duplicate lines from rebase --- spec/spec_helper.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3d6a4f3a..1023fb79 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -25,11 +25,6 @@ require 'support/s3' require 'support/test_helpers' require 'support/shared_examples' -Travis.logger = Logger.new(StringIO.new) -Travis::Api::App.setup -Travis.config.client_domain = "www.example.com" -Travis.config.endpoints.ssh_key = true - module TestHelpers include Sinatra::TestHelpers From 77dcdaa48257ebb90308f9d364134aa660926f20 Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Thu, 9 Jun 2016 16:21:55 +0200 Subject: [PATCH 08/21] Use travis-settings to manage JSON settings field Since we use repository.settings as a kind of dump for all sorts of settings, some user-facing and some not, this lets us leave the db as it is, but pretend to have separate models for each "kind" of setting. --- Gemfile | 1 + Gemfile.lock | 8 ++++++++ lib/travis/api/v3/models/admin_settings.rb | 5 +++++ lib/travis/api/v3/models/repository.rb | 10 +++++++++- lib/travis/api/v3/models/settings.rb | 13 ++----------- lib/travis/api/v3/models/user_settings.rb | 8 ++++++++ lib/travis/api/v3/services/requests/create.rb | 6 +----- 7 files changed, 34 insertions(+), 17 deletions(-) create mode 100644 lib/travis/api/v3/models/admin_settings.rb create mode 100644 lib/travis/api/v3/models/user_settings.rb diff --git a/Gemfile b/Gemfile index ab1dc3bf..ccf97b60 100644 --- a/Gemfile +++ b/Gemfile @@ -11,6 +11,7 @@ gem 'travis-settings', github: 'travis-ci/travis-settings' gem 'travis-sidekiqs', github: 'travis-ci/travis-sidekiqs' gem 'travis-yaml', github: 'travis-ci/travis-yaml' +gem 'travis-settings', github: 'travis-ci/travis-settings' gem 'mustermann', github: 'rkh/mustermann' gem 'sinatra' gem 'sinatra-contrib', require: nil #github: 'sinatra/sinatra-contrib', require: nil diff --git a/Gemfile.lock b/Gemfile.lock index 0dda637b..96104e61 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -53,6 +53,14 @@ GIT activemodel virtus +GIT + remote: git://github.com/travis-ci/travis-settings.git + revision: d510e63b6c6f059cccae141c265e7a0c7236d1fd + specs: + travis-settings (0.0.1) + activemodel + virtus + GIT remote: git://github.com/travis-ci/travis-sidekiqs.git revision: c5d4a4abc6c3737f9c43d3333efb94daa18b9fbb diff --git a/lib/travis/api/v3/models/admin_settings.rb b/lib/travis/api/v3/models/admin_settings.rb new file mode 100644 index 00000000..ee1d9d2a --- /dev/null +++ b/lib/travis/api/v3/models/admin_settings.rb @@ -0,0 +1,5 @@ +module Travis::API::V3 + class Models::AdminSettings < Travis::Settings::Model + attribute :api_builds_rate_limit, Integer + end +end diff --git a/lib/travis/api/v3/models/repository.rb b/lib/travis/api/v3/models/repository.rb index b462db88..1b14bd5f 100644 --- a/lib/travis/api/v3/models/repository.rb +++ b/lib/travis/api/v3/models/repository.rb @@ -65,7 +65,15 @@ module Travis::API::V3 end def settings - @settings ||= JSON.load(super) + @settings ||= JSON.load(super || '{}'.freeze) + end + + def user_settings + @user_settings ||= Models::UserSettings.new(settings) + end + + def admin_settings + @admin_settings ||= Models::AdminSettings.new(settings) end end end diff --git a/lib/travis/api/v3/models/settings.rb b/lib/travis/api/v3/models/settings.rb index 00949495..0d3e4593 100644 --- a/lib/travis/api/v3/models/settings.rb +++ b/lib/travis/api/v3/models/settings.rb @@ -1,12 +1,5 @@ module Travis::API::V3 class Models::Settings - DEFAULTS = { - 'builds_only_with_travis_yml' => false, - 'build_pushes' => true, - 'build_pull_requests' => true, - 'maximum_number_of_builds' => 0 - }.freeze - attr_reader :repository def initialize(repository) @@ -14,13 +7,11 @@ module Travis::API::V3 end def to_h - DEFAULTS.merge(repository.settings || {}) + repository.user_settings.to_hash end def update(settings = {}) - settings = to_h.merge(settings) - repository.settings.clear - settings.each { |k, v| repository.settings[k] = v } + repository.user_settings.update(settings) repository.save! end end diff --git a/lib/travis/api/v3/models/user_settings.rb b/lib/travis/api/v3/models/user_settings.rb new file mode 100644 index 00000000..42e6519e --- /dev/null +++ b/lib/travis/api/v3/models/user_settings.rb @@ -0,0 +1,8 @@ +module Travis::API::V3 + class Models::UserSettings < Travis::Settings::Model + attribute :builds_only_with_travis_yml, Boolean, default: false + attribute :build_pushes, Boolean, default: true + attribute :build_pull_requests, Boolean, default: true + attribute :maximum_number_of_builds, Integer, default: 0 + end +end diff --git a/lib/travis/api/v3/services/requests/create.rb b/lib/travis/api/v3/services/requests/create.rb index ea3827de..87c54d2d 100644 --- a/lib/travis/api/v3/services/requests/create.rb +++ b/lib/travis/api/v3/services/requests/create.rb @@ -23,11 +23,7 @@ module Travis::API::V3 end def limit(repository) - if repository.settings.nil? - Travis.config.requests_create_api_limit || LIMIT - else - repository.settings["api_builds_rate_limit"] || Travis.config.requests_create_api_limit || LIMIT - end + repository.admin_settings.api_builds_rate_limit || Travis.config.requests_create_api_limit || LIMIT end def remaining_requests(repository) From 871b91551565e561e8c42ec346554638e883cd6d Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 14 Jun 2016 11:59:15 +0200 Subject: [PATCH 09/21] Add env vars endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds API V3 endpoints for querying, creating, modifying and deleting environment variables. It has no concept of encryption yet and should be considered a work in progress. We should also talk about the slightly off-track approach in the EnvVars::Create service – maybe there's a way to standardise the querying and rendering for post requests? :coffee: --- lib/travis/api/v3.rb | 1 + lib/travis/api/v3/access_control/generic.rb | 8 +++ lib/travis/api/v3/models/env_var.rb | 18 +++++ lib/travis/api/v3/models/env_vars.rb | 11 +++ lib/travis/api/v3/models/repository.rb | 6 ++ lib/travis/api/v3/queries/env_var.rb | 21 ++++++ lib/travis/api/v3/queries/env_vars.rb | 27 ++++++++ lib/travis/api/v3/renderer/env_var.rb | 5 ++ lib/travis/api/v3/renderer/env_vars.rb | 6 ++ lib/travis/api/v3/routes.rb | 13 ++++ lib/travis/api/v3/services.rb | 2 + lib/travis/api/v3/services/env_var/delete.rb | 12 ++++ lib/travis/api/v3/services/env_var/find.rb | 12 ++++ lib/travis/api/v3/services/env_var/update.rb | 12 ++++ lib/travis/api/v3/services/env_vars/create.rb | 13 ++++ .../v3/services/env_vars/for_repository.rb | 9 +++ spec/spec_helper.rb | 34 ++++++++++ spec/support/shared_examples.rb | 34 ++++++++++ spec/v3/services/env_var/delete_spec.rb | 33 +++++++++ spec/v3/services/env_var/find_spec.rb | 43 ++++++++++++ spec/v3/services/env_var/update_spec.rb | 50 ++++++++++++++ spec/v3/services/env_vars/create_spec.rb | 67 +++++++++++++++++++ .../services/env_vars/for_repository_spec.rb | 59 ++++++++++++++++ spec/v3/services/settings_spec.rb | 9 +-- 24 files changed, 497 insertions(+), 8 deletions(-) create mode 100644 lib/travis/api/v3/models/env_var.rb create mode 100644 lib/travis/api/v3/models/env_vars.rb create mode 100644 lib/travis/api/v3/queries/env_var.rb create mode 100644 lib/travis/api/v3/queries/env_vars.rb create mode 100644 lib/travis/api/v3/renderer/env_var.rb create mode 100644 lib/travis/api/v3/renderer/env_vars.rb create mode 100644 lib/travis/api/v3/services/env_var/delete.rb create mode 100644 lib/travis/api/v3/services/env_var/find.rb create mode 100644 lib/travis/api/v3/services/env_var/update.rb create mode 100644 lib/travis/api/v3/services/env_vars/create.rb create mode 100644 lib/travis/api/v3/services/env_vars/for_repository.rb create mode 100644 spec/support/shared_examples.rb create mode 100644 spec/v3/services/env_var/delete_spec.rb create mode 100644 spec/v3/services/env_var/find_spec.rb create mode 100644 spec/v3/services/env_var/update_spec.rb create mode 100644 spec/v3/services/env_vars/create_spec.rb create mode 100644 spec/v3/services/env_vars/for_repository_spec.rb diff --git a/lib/travis/api/v3.rb b/lib/travis/api/v3.rb index 610de9cd..f05aec5e 100644 --- a/lib/travis/api/v3.rb +++ b/lib/travis/api/v3.rb @@ -31,6 +31,7 @@ module Travis AlreadySyncing = ClientError .create('sync already in progress', status: 409) BuildAlreadyRunning = ClientError .create('build already running, cannot restart', status: 409) BuildNotCancelable = ClientError .create('build is not running, cannot cancel', status: 409) + DuplicateResource = ClientError .create('resource already exists', status: 409) EntityMissing = NotFound .create(type: 'not_found') InsufficientAccess = ClientError .create(status: 403) JobAlreadyRunning = ClientError .create('job already running, cannot restart', status: 409) diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index 92b68e1e..f3671da7 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -96,6 +96,14 @@ module Travis::API::V3 repository_visible?(settings.repository) end + def env_vars_visible?(env_vars) + repository_visible?(env_vars.repository) + end + + def env_var_visible?(env_var) + repository_visible?(env_var.repository) + end + def private_repository_visible?(repository) false end diff --git a/lib/travis/api/v3/models/env_var.rb b/lib/travis/api/v3/models/env_var.rb new file mode 100644 index 00000000..1d4c7f8f --- /dev/null +++ b/lib/travis/api/v3/models/env_var.rb @@ -0,0 +1,18 @@ +module Travis::API::V3 + class Models::EnvVar < Travis::Settings::Model + attribute :id, Integer + attribute :name, String + attribute :value, String + attribute :public, Boolean + attribute :repository_id, Integer + + def repository + @repository ||= Models::Repository.find(repository_id) + end + + validates_each :id, :name do |record, attr, value| + others = record.repository.env_vars.select { |ev| ev.id != record.id } + record.errors.add(:base, :duplicate_resource) if others.find { |ev| ev.send(attr) == record.send(attr) } + end + end +end diff --git a/lib/travis/api/v3/models/env_vars.rb b/lib/travis/api/v3/models/env_vars.rb new file mode 100644 index 00000000..1e4b7d08 --- /dev/null +++ b/lib/travis/api/v3/models/env_vars.rb @@ -0,0 +1,11 @@ +module Travis::API::V3 + class Models::EnvVars < Travis::Settings::Collection + model Models::EnvVar + + def repository + @repository ||= Models::Repository.find(additional_attributes[:repository_id]) + end + + undef :to_hash + end +end diff --git a/lib/travis/api/v3/models/repository.rb b/lib/travis/api/v3/models/repository.rb index 1b14bd5f..dfbd1eba 100644 --- a/lib/travis/api/v3/models/repository.rb +++ b/lib/travis/api/v3/models/repository.rb @@ -75,5 +75,11 @@ module Travis::API::V3 def admin_settings @admin_settings ||= Models::AdminSettings.new(settings) end + + def env_vars + @env_vars ||= Models::EnvVars.new.tap do |vars| + vars.load(settings.fetch('env_vars', []), repository_id: self.id) + end + end end end diff --git a/lib/travis/api/v3/queries/env_var.rb b/lib/travis/api/v3/queries/env_var.rb new file mode 100644 index 00000000..4ffaeab9 --- /dev/null +++ b/lib/travis/api/v3/queries/env_var.rb @@ -0,0 +1,21 @@ +module Travis::API::V3 + class Queries::EnvVar < Query + params :id, :name, :value, :public, prefix: :env_var + + def find(repository) + repository.env_vars.find(id) + end + + def update(repository) + if env_var = find(repository) + env_var.update(env_var_params) + repository.save! + env_var + end + end + + def delete(repository) + repository.env_vars.destroy(id) + end + end +end diff --git a/lib/travis/api/v3/queries/env_vars.rb b/lib/travis/api/v3/queries/env_vars.rb new file mode 100644 index 00000000..0544cbbe --- /dev/null +++ b/lib/travis/api/v3/queries/env_vars.rb @@ -0,0 +1,27 @@ +module Travis::API::V3 + class Queries::EnvVars < Query + params :id, :name, :value, :public, prefix: :env_var + + def find(repository) + repository.env_vars + end + + def create(repository) + env_var = repository.env_vars.create(env_var_params) + handle_errors(env_var) unless env_var.valid? + repository.save! + env_var + end + + private + + def handle_errors(env_var) + base = env_var.errors[:base] + case + when base.include?(:format) then raise WrongParams + when base.include?(:duplicate_resource) then raise DuplicateResource + else raise ServerError + end + end + end +end diff --git a/lib/travis/api/v3/renderer/env_var.rb b/lib/travis/api/v3/renderer/env_var.rb new file mode 100644 index 00000000..6d2f955a --- /dev/null +++ b/lib/travis/api/v3/renderer/env_var.rb @@ -0,0 +1,5 @@ +module Travis::API::V3 + class Renderer::EnvVar < Renderer::ModelRenderer + representation :standard, :id, :name, :value, :public + end +end diff --git a/lib/travis/api/v3/renderer/env_vars.rb b/lib/travis/api/v3/renderer/env_vars.rb new file mode 100644 index 00000000..9a47a84c --- /dev/null +++ b/lib/travis/api/v3/renderer/env_vars.rb @@ -0,0 +1,6 @@ +module Travis::API::V3 + class Renderer::EnvVars < Renderer::CollectionRenderer + type :env_vars + collection_key :env_vars + end +end diff --git a/lib/travis/api/v3/routes.rb b/lib/travis/api/v3/routes.rb index 585dc1d8..7767527e 100644 --- a/lib/travis/api/v3/routes.rb +++ b/lib/travis/api/v3/routes.rb @@ -123,6 +123,19 @@ module Travis::API::V3 get :find patch :update end + + resource :env_vars do + route '/env_vars' + get :for_repository + post :create + end + + resource :env_var do + route '/env_var/{env_var.id}' + get :find + patch :update + delete :delete + end end resource :user do diff --git a/lib/travis/api/v3/services.rb b/lib/travis/api/v3/services.rb index 32b30b7c..8a48c895 100644 --- a/lib/travis/api/v3/services.rb +++ b/lib/travis/api/v3/services.rb @@ -11,6 +11,8 @@ module Travis::API::V3 Builds = Module.new { extend Services } Cron = Module.new { extend Services } Crons = Module.new { extend Services } + EnvVar = Module.new { extend Services } + EnvVars = Module.new { extend Services } Job = Module.new { extend Services } Jobs = Module.new { extend Services } Lint = Module.new { extend Services } diff --git a/lib/travis/api/v3/services/env_var/delete.rb b/lib/travis/api/v3/services/env_var/delete.rb new file mode 100644 index 00000000..d11f20f0 --- /dev/null +++ b/lib/travis/api/v3/services/env_var/delete.rb @@ -0,0 +1,12 @@ +module Travis::API::V3 + class Services::EnvVar::Delete < Service + params :id, prefix: :repository + params :id, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + query.delete(repository) + end + end +end diff --git a/lib/travis/api/v3/services/env_var/find.rb b/lib/travis/api/v3/services/env_var/find.rb new file mode 100644 index 00000000..476c1b3a --- /dev/null +++ b/lib/travis/api/v3/services/env_var/find.rb @@ -0,0 +1,12 @@ +module Travis::API::V3 + class Services::EnvVar::Find < Service + params :id, prefix: :repository + params :id, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + query.find(repository) + end + end +end diff --git a/lib/travis/api/v3/services/env_var/update.rb b/lib/travis/api/v3/services/env_var/update.rb new file mode 100644 index 00000000..6caa751a --- /dev/null +++ b/lib/travis/api/v3/services/env_var/update.rb @@ -0,0 +1,12 @@ +module Travis::API::V3 + class Services::EnvVar::Update < Service + params :id, prefix: :repository + params :id, :name, :value, :public, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + query.update(repository) + end + end +end diff --git a/lib/travis/api/v3/services/env_vars/create.rb b/lib/travis/api/v3/services/env_vars/create.rb new file mode 100644 index 00000000..02839fc2 --- /dev/null +++ b/lib/travis/api/v3/services/env_vars/create.rb @@ -0,0 +1,13 @@ +module Travis::API::V3 + class Services::EnvVars::Create < Service + params :id, prefix: :repository + params :id, :name, :value, :public, prefix: :env_var + + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + env_var = query(:env_vars).create(repository) + result(:env_var, env_var, status: 201) + end + end +end diff --git a/lib/travis/api/v3/services/env_vars/for_repository.rb b/lib/travis/api/v3/services/env_vars/for_repository.rb new file mode 100644 index 00000000..1cb1b4cf --- /dev/null +++ b/lib/travis/api/v3/services/env_vars/for_repository.rb @@ -0,0 +1,9 @@ +module Travis::API::V3 + class Services::EnvVars::ForRepository < Service + def run! + raise LoginRequired unless access_control.logged_in? or access_control.full_access? + raise NotFound unless repository = find(:repository) + find(:env_vars, repository) + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 748561f5..3d6a4f3a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -23,6 +23,40 @@ require 'support/payloads' require 'support/private_key' require 'support/s3' require 'support/test_helpers' +require 'support/shared_examples' + +Travis.logger = Logger.new(StringIO.new) +Travis::Api::App.setup +Travis.config.client_domain = "www.example.com" +Travis.config.endpoints.ssh_key = true + +module TestHelpers + include Sinatra::TestHelpers + + def custom_endpoints + @custom_endpoints ||= [] + end + + def add_settings_endpoint(name, options = {}) + if options[:singleton] + Travis::Api::App::SingletonSettingsEndpoint.subclass(name) + else + Travis::Api::App::SettingsEndpoint.subclass(name) + end + set_app Travis::Api::App.new + end + + def add_endpoint(prefix, &block) + endpoint = Sinatra.new(Travis::Api::App::Endpoint, &block) + endpoint.set(prefix: prefix) + set_app Travis::Api::App.new + custom_endpoints << endpoint + end + + def parsed_body + MultiJson.decode(body) + end +end RSpec.configure do |c| c.mock_framework = :mocha diff --git a/spec/support/shared_examples.rb b/spec/support/shared_examples.rb new file mode 100644 index 00000000..b1964bfb --- /dev/null +++ b/spec/support/shared_examples.rb @@ -0,0 +1,34 @@ +RSpec.shared_examples 'not authenticated' do + example { expect(last_response.status).to eq(403) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_type' => 'login_required', + 'error_message' => 'login required' + ) + end +end + +RSpec.shared_examples 'missing repo' do + example { expect(last_response.status).to eq(404) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_message' => 'repository not found (or insufficient access)', + 'error_type' => 'not_found', + 'resource_type' => 'repository' + ) + end +end + +RSpec.shared_examples 'missing env_var' do + example { expect(last_response.status).to eq 404 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_message' => 'env_var not found (or insufficient access)', + 'error_type' => 'not_found', + 'resource_type' => 'env_var' + ) + end +end diff --git a/spec/v3/services/env_var/delete_spec.rb b/spec/v3/services/env_var/delete_spec.rb new file mode 100644 index 00000000..9f8879c8 --- /dev/null +++ b/spec/v3/services/env_var/delete_spec.rb @@ -0,0 +1,33 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVar::Delete do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + + describe 'not authenticated' do + before { delete("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { delete("/v3/repo/999999999/env_var/foo", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, missing repo, missing env var' do + before { delete("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) } + include_examples 'missing env_var' + end + + describe 'authenticated, missing repo, existing env var' do + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + delete("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) + end + + example { expect(last_response.status).to eq 200 } + example { pending 'should we return an empty body here?' } + end +end diff --git a/spec/v3/services/env_var/find_spec.rb b/spec/v3/services/env_var/find_spec.rb new file mode 100644 index 00000000..0b9081b7 --- /dev/null +++ b/spec/v3/services/env_var/find_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVar::Find do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + + describe 'not authenticated' do + before { get("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { get("/v3/repo/999999999/env_var/foo", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, missing env var' do + before { get("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) } + include_examples 'missing env_var' + end + + describe 'authenticated, existing repo, existing env var' do + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + get("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", {}, auth_headers) + end + + example { expect(last_response.status).to eq 200 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_var', + '@href' => "/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", + '@representation' => 'standard', + 'id' => env_var[:id], + 'name' => env_var[:name], + 'public' => env_var[:public], + 'value' => env_var[:value] + ) + end + end +end diff --git a/spec/v3/services/env_var/update_spec.rb b/spec/v3/services/env_var/update_spec.rb new file mode 100644 index 00000000..e23359c7 --- /dev/null +++ b/spec/v3/services/env_var/update_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVar::Update do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + let(:json_headers) { { 'CONTENT_TYPE' => 'application/json' } } + + describe 'not authenticated' do + before { patch("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { patch("/v3/repo/999999999/env_var/#{env_var[:id]}", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, missing env var' do + before { patch("/v3/repo/#{repo.id}/env_var/foo", {}, auth_headers) } + include_examples 'missing env_var' + end + + describe 'authenticated, existing repo, existing env var' do + let(:params) do + { + 'env_var.name' => 'QUX' + } + end + + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + patch("/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", JSON.generate(params), auth_headers.merge(json_headers)) + end + + example { expect(last_response.status).to eq 200 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_var', + '@href' => '/v3/repo/1/env_var/abc', + '@representation' => 'standard', + 'id' => env_var[:id], + 'name' => params['env_var.name'], + 'value' => env_var[:value], + 'public' => env_var[:public] + ) + end + end +end diff --git a/spec/v3/services/env_vars/create_spec.rb b/spec/v3/services/env_vars/create_spec.rb new file mode 100644 index 00000000..52230df1 --- /dev/null +++ b/spec/v3/services/env_vars/create_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVars::Create do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + let(:json_headers) { { 'CONTENT_TYPE' => 'application/json' } } + + describe 'not authenticated' do + before { post("/v3/repo/#{repo.id}/env_vars", {}) } + include_examples 'not authenticated' + end + + describe 'authenticated, repo missing' do + before { post("/v3/repo/99999999/env_vars", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, env var already exists' do + let(:params) do + { + 'env_var.name' => 'FOO', + 'env_var.value' => 'bar', + 'env_var.public' => false + } + end + + before do + repo.update_attributes(settings: JSON.generate(env_vars: [{ id: 'abc', name: 'FOO', value: 'bar', public: false }])) + post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) + end + + example { expect(last_response.status).to eq 409 } + example do + expect(JSON.load(body)).to eq( + '@type' => 'error', + 'error_message' => 'resource already exists', + 'error_type' => 'duplicate_resource' + ) + end + end + + describe 'authenticated, existing repo, env var is new' do + let(:params) do + { + 'env_var.name' => 'FOO', + 'env_var.value' => 'bar', + 'env_var.public' => false + } + end + + before { post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) } + + example { expect(last_response.status).to eq 201 } + example do + response = JSON.load(body) + expect(response).to include( + '@type' => 'env_var', + '@representation' => 'standard', + 'name' => 'FOO', + 'value' => 'bar', + 'public' => false + ) + expect(response).to include('@href', 'id') + end + end +end diff --git a/spec/v3/services/env_vars/for_repository_spec.rb b/spec/v3/services/env_vars/for_repository_spec.rb new file mode 100644 index 00000000..60ade927 --- /dev/null +++ b/spec/v3/services/env_vars/for_repository_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe Travis::API::V3::Services::EnvVars::ForRepository do + let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } + let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } + let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } + + describe 'not authenticated' do + before { get("/v3/repo/#{repo.id}/env_vars") } + include_examples 'not authenticated' + end + + describe 'authenticated, missing repo' do + before { get("/v3/repo/999999999/env_vars", {}, auth_headers) } + include_examples 'missing repo' + end + + describe 'authenticated, existing repo, no env vars' do + before do + get("/v3/repo/#{repo.id}/env_vars", {}, auth_headers) + end + example { expect(last_response.status).to eq(200) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_vars', + '@href' => "/v3/repo/#{repo.id}/env_vars", + '@representation' => 'standard', + 'env_vars' => [] + ) + end + end + + describe 'authenticated, existing repo, existing env vars' do + before do + repo.update_attributes(settings: JSON.generate(env_vars: [env_var])) + get("/v3/repo/#{repo.id}/env_vars", {}, auth_headers) + end + example { expect(last_response.status).to eq(200) } + example do + expect(JSON.load(body)).to eq( + '@type' => 'env_vars', + '@href' => "/v3/repo/#{repo.id}/env_vars", + '@representation' => 'standard', + 'env_vars' => [ + { + '@type' => 'env_var', + '@href' => "/v3/repo/#{repo.id}/env_var/#{env_var[:id]}", + '@representation' => 'standard', + 'id' => env_var[:id], + 'name' => env_var[:name], + 'value' => env_var[:value], + 'public' => env_var[:public] + } + ] + ) + end + end +end diff --git a/spec/v3/services/settings_spec.rb b/spec/v3/services/settings_spec.rb index b8189275..11b839b5 100644 --- a/spec/v3/services/settings_spec.rb +++ b/spec/v3/services/settings_spec.rb @@ -7,14 +7,7 @@ describe Travis::API::V3::Services::Settings, set_app: true do describe :Find do describe 'not authenticated' do before { get("/v3/repo/#{repo.id}/settings") } - example { expect(last_response.status).to eq(403) } - example do - expect(JSON.load(body)).to eq( - '@type' => 'error', - 'error_type' => 'login_required', - 'error_message' => 'login required' - ) - end + include_examples 'not authenticated' end describe 'authenticated, missing repo' do From 91e9fcebf75c2a2e5304ab897f45f5dba3a6e34f Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 21 Jun 2016 12:23:34 +0200 Subject: [PATCH 10/21] Remove some duplication --- lib/travis/api/v3/access_control/generic.rb | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index f3671da7..30be8902 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -92,22 +92,15 @@ module Travis::API::V3 private_repository_visible?(repository) end - def settings_visible?(settings) - repository_visible?(settings.repository) - end - - def env_vars_visible?(env_vars) - repository_visible?(env_vars.repository) - end - - def env_var_visible?(env_var) - repository_visible?(env_var.repository) - end - def private_repository_visible?(repository) false end + def repository_attr_visible?(record) + repository_visible?(record.repository) + end + [:settings_visible?, :env_vars_visible?, :env_var_visible?].each { |m| alias_method m, :repository_attr_visible? } + def public_api? !Travis.config.private_api end From 1a07e199b51fd75e4303659d42c4222ac5718cce Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 21 Jun 2016 12:31:42 +0200 Subject: [PATCH 11/21] Better style in error handling --- lib/travis/api/v3/queries/env_vars.rb | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/travis/api/v3/queries/env_vars.rb b/lib/travis/api/v3/queries/env_vars.rb index 0544cbbe..9c92d4d6 100644 --- a/lib/travis/api/v3/queries/env_vars.rb +++ b/lib/travis/api/v3/queries/env_vars.rb @@ -17,11 +17,9 @@ module Travis::API::V3 def handle_errors(env_var) base = env_var.errors[:base] - case - when base.include?(:format) then raise WrongParams - when base.include?(:duplicate_resource) then raise DuplicateResource - else raise ServerError - end + raise WrongParams if base.include?(:format) + raise DuplicateResource if base.include?(:duplicate_resource) + raise ServerError end end end From f5bc526f250145fd3d22ee0e33b0b2ee693cde2a Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Tue, 21 Jun 2016 13:17:44 +0200 Subject: [PATCH 12/21] Refactor repeated pattern into extracted method Seems like we'll be checking login status before finding a resource a lot, so here's a standard way to do it. --- lib/travis/api/v3/access_control/generic.rb | 4 ++++ lib/travis/api/v3/service.rb | 5 +++++ lib/travis/api/v3/services/env_var/delete.rb | 3 +-- lib/travis/api/v3/services/env_var/find.rb | 3 +-- lib/travis/api/v3/services/env_var/update.rb | 3 +-- lib/travis/api/v3/services/env_vars/create.rb | 3 +-- lib/travis/api/v3/services/env_vars/for_repository.rb | 3 +-- 7 files changed, 14 insertions(+), 10 deletions(-) diff --git a/lib/travis/api/v3/access_control/generic.rb b/lib/travis/api/v3/access_control/generic.rb index 30be8902..48b4958f 100644 --- a/lib/travis/api/v3/access_control/generic.rb +++ b/lib/travis/api/v3/access_control/generic.rb @@ -30,6 +30,10 @@ module Travis::API::V3 false end + def full_access_or_logged_in? + full_access? || logged_in? + end + def visible_repositories(list) # naïve implementation, replaced with smart implementation in specific subclasses return list if full_access? diff --git a/lib/travis/api/v3/service.rb b/lib/travis/api/v3/service.rb index fa63ecf1..13b8f2aa 100644 --- a/lib/travis/api/v3/service.rb +++ b/lib/travis/api/v3/service.rb @@ -66,6 +66,11 @@ module Travis::API::V3 object end + def check_login_and_find(*args) + raise LoginRequired unless access_control.full_access_or_logged_in? + find(*args) + end + def not_found(actually_not_found = false, type = nil) type, actually_not_found = actually_not_found, false if actually_not_found.is_a? Symbol error = actually_not_found ? EntityMissing : NotFound diff --git a/lib/travis/api/v3/services/env_var/delete.rb b/lib/travis/api/v3/services/env_var/delete.rb index d11f20f0..3ea473ab 100644 --- a/lib/travis/api/v3/services/env_var/delete.rb +++ b/lib/travis/api/v3/services/env_var/delete.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) query.delete(repository) end end diff --git a/lib/travis/api/v3/services/env_var/find.rb b/lib/travis/api/v3/services/env_var/find.rb index 476c1b3a..17258043 100644 --- a/lib/travis/api/v3/services/env_var/find.rb +++ b/lib/travis/api/v3/services/env_var/find.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) query.find(repository) end end diff --git a/lib/travis/api/v3/services/env_var/update.rb b/lib/travis/api/v3/services/env_var/update.rb index 6caa751a..e83e9256 100644 --- a/lib/travis/api/v3/services/env_var/update.rb +++ b/lib/travis/api/v3/services/env_var/update.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, :name, :value, :public, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) query.update(repository) end end diff --git a/lib/travis/api/v3/services/env_vars/create.rb b/lib/travis/api/v3/services/env_vars/create.rb index 02839fc2..a2fd3ed1 100644 --- a/lib/travis/api/v3/services/env_vars/create.rb +++ b/lib/travis/api/v3/services/env_vars/create.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :id, :name, :value, :public, prefix: :env_var def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) env_var = query(:env_vars).create(repository) result(:env_var, env_var, status: 201) end diff --git a/lib/travis/api/v3/services/env_vars/for_repository.rb b/lib/travis/api/v3/services/env_vars/for_repository.rb index 1cb1b4cf..60a88577 100644 --- a/lib/travis/api/v3/services/env_vars/for_repository.rb +++ b/lib/travis/api/v3/services/env_vars/for_repository.rb @@ -1,8 +1,7 @@ module Travis::API::V3 class Services::EnvVars::ForRepository < Service def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) find(:env_vars, repository) end end From 4b14f17cc5181c31400cc2db0eda3be3e3699753 Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Wed, 22 Jun 2016 16:50:11 +0200 Subject: [PATCH 13/21] Fix service specs --- spec/v3/services/env_var/delete_spec.rb | 2 +- spec/v3/services/env_var/find_spec.rb | 2 +- spec/v3/services/env_var/update_spec.rb | 2 +- spec/v3/services/env_vars/create_spec.rb | 2 +- spec/v3/services/env_vars/for_repository_spec.rb | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/v3/services/env_var/delete_spec.rb b/spec/v3/services/env_var/delete_spec.rb index 9f8879c8..870c69e9 100644 --- a/spec/v3/services/env_var/delete_spec.rb +++ b/spec/v3/services/env_var/delete_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVar::Delete do +describe Travis::API::V3::Services::EnvVar::Delete, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } diff --git a/spec/v3/services/env_var/find_spec.rb b/spec/v3/services/env_var/find_spec.rb index 0b9081b7..f80cfdc1 100644 --- a/spec/v3/services/env_var/find_spec.rb +++ b/spec/v3/services/env_var/find_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVar::Find do +describe Travis::API::V3::Services::EnvVar::Find, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } diff --git a/spec/v3/services/env_var/update_spec.rb b/spec/v3/services/env_var/update_spec.rb index e23359c7..bac9a065 100644 --- a/spec/v3/services/env_var/update_spec.rb +++ b/spec/v3/services/env_var/update_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVar::Update do +describe Travis::API::V3::Services::EnvVar::Update, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } diff --git a/spec/v3/services/env_vars/create_spec.rb b/spec/v3/services/env_vars/create_spec.rb index 52230df1..1755e593 100644 --- a/spec/v3/services/env_vars/create_spec.rb +++ b/spec/v3/services/env_vars/create_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVars::Create do +describe Travis::API::V3::Services::EnvVars::Create, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } diff --git a/spec/v3/services/env_vars/for_repository_spec.rb b/spec/v3/services/env_vars/for_repository_spec.rb index 60ade927..fd20de9d 100644 --- a/spec/v3/services/env_vars/for_repository_spec.rb +++ b/spec/v3/services/env_vars/for_repository_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Travis::API::V3::Services::EnvVars::ForRepository do +describe Travis::API::V3::Services::EnvVars::ForRepository, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } From 829366a55467b68c8e18fc0034ad76fada2f37cc Mon Sep 17 00:00:00 2001 From: Joe Corcoran Date: Wed, 22 Jun 2016 16:59:49 +0200 Subject: [PATCH 14/21] Fix duplicate lines from rebase --- spec/spec_helper.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3d6a4f3a..1023fb79 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -25,11 +25,6 @@ require 'support/s3' require 'support/test_helpers' require 'support/shared_examples' -Travis.logger = Logger.new(StringIO.new) -Travis::Api::App.setup -Travis.config.client_domain = "www.example.com" -Travis.config.endpoints.ssh_key = true - module TestHelpers include Sinatra::TestHelpers From 1ec8dba7e417721508186beed44d008028012113 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Hendricksen?= Date: Tue, 5 Jul 2016 13:36:31 -0400 Subject: [PATCH 15/21] fix the tests --- Gemfile.lock | 8 -------- 1 file changed, 8 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 0238e071..15221eed 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -53,14 +53,6 @@ GIT activemodel virtus -GIT - remote: git://github.com/travis-ci/travis-settings.git - revision: d510e63b6c6f059cccae141c265e7a0c7236d1fd - specs: - travis-settings (0.0.1) - activemodel - virtus - GIT remote: git://github.com/travis-ci/travis-sidekiqs.git revision: c5d4a4abc6c3737f9c43d3333efb94daa18b9fbb From b94d9c8637bd64518bc2661adfaed53e32c43829 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Hendricksen?= Date: Thu, 7 Jul 2016 00:31:35 -0400 Subject: [PATCH 16/21] use the new method --- lib/travis/api/v3/service.rb | 2 +- lib/travis/api/v3/services/build/cancel.rb | 3 +-- lib/travis/api/v3/services/build/restart.rb | 3 +-- lib/travis/api/v3/services/cron/create.rb | 3 +-- lib/travis/api/v3/services/cron/delete.rb | 3 +-- lib/travis/api/v3/services/job/cancel.rb | 3 +-- lib/travis/api/v3/services/job/debug.rb | 3 +-- lib/travis/api/v3/services/job/restart.rb | 3 +-- lib/travis/api/v3/services/repository/disable.rb | 3 +-- lib/travis/api/v3/services/repository/star.rb | 3 +-- lib/travis/api/v3/services/repository/unstar.rb | 3 +-- lib/travis/api/v3/services/requests/create.rb | 3 +-- lib/travis/api/v3/services/settings/find.rb | 3 +-- lib/travis/api/v3/services/settings/update.rb | 3 +-- lib/travis/api/v3/services/user/sync.rb | 3 +-- 15 files changed, 15 insertions(+), 29 deletions(-) diff --git a/lib/travis/api/v3/service.rb b/lib/travis/api/v3/service.rb index 13b8f2aa..d0cb719f 100644 --- a/lib/travis/api/v3/service.rb +++ b/lib/travis/api/v3/service.rb @@ -68,7 +68,7 @@ module Travis::API::V3 def check_login_and_find(*args) raise LoginRequired unless access_control.full_access_or_logged_in? - find(*args) + find(*args) # should this raise NotFound if nil? Can it return nil? see above? end def not_found(actually_not_found = false, type = nil) diff --git a/lib/travis/api/v3/services/build/cancel.rb b/lib/travis/api/v3/services/build/cancel.rb index 168b3ccf..4e10a110 100644 --- a/lib/travis/api/v3/services/build/cancel.rb +++ b/lib/travis/api/v3/services/build/cancel.rb @@ -2,8 +2,7 @@ module Travis::API::V3 class Services::Build::Cancel < Service def run - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless build = find(:build) + build = check_login_and_find(:build) access_control.permissions(build).cancel! query.cancel(access_control.user) diff --git a/lib/travis/api/v3/services/build/restart.rb b/lib/travis/api/v3/services/build/restart.rb index acb49727..7ba3aff6 100644 --- a/lib/travis/api/v3/services/build/restart.rb +++ b/lib/travis/api/v3/services/build/restart.rb @@ -2,8 +2,7 @@ module Travis::API::V3 class Services::Build::Restart < Service def run - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless build = find(:build) + build = check_login_and_find(:build) access_control.permissions(build).restart! query.restart(access_control.user) diff --git a/lib/travis/api/v3/services/cron/create.rb b/lib/travis/api/v3/services/cron/create.rb index dcbfffb8..2fb366e1 100644 --- a/lib/travis/api/v3/services/cron/create.rb +++ b/lib/travis/api/v3/services/cron/create.rb @@ -4,8 +4,7 @@ module Travis::API::V3 params :interval, :disable_by_build def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) raise NotFound unless branch = find(:branch, repository) raise Error.new('Crons can only be set up for branches existing on GitHub!', status: 422) unless branch.exists_on_github 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 c5d9287d..59aa6d10 100644 --- a/lib/travis/api/v3/services/cron/delete.rb +++ b/lib/travis/api/v3/services/cron/delete.rb @@ -3,8 +3,7 @@ module Travis::API::V3 #params :id def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - cron = find + cron = check_login_and_find access_control.permissions(cron).delete! cron.destroy end diff --git a/lib/travis/api/v3/services/job/cancel.rb b/lib/travis/api/v3/services/job/cancel.rb index 0b565498..a6f143ec 100644 --- a/lib/travis/api/v3/services/job/cancel.rb +++ b/lib/travis/api/v3/services/job/cancel.rb @@ -2,8 +2,7 @@ module Travis::API::V3 class Services::Job::Cancel < Service def run - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless job = find(:job) + job = check_login_and_find(:job) access_control.permissions(job).cancel! query.cancel(access_control.user) diff --git a/lib/travis/api/v3/services/job/debug.rb b/lib/travis/api/v3/services/job/debug.rb index 63c1939c..a892b993 100644 --- a/lib/travis/api/v3/services/job/debug.rb +++ b/lib/travis/api/v3/services/job/debug.rb @@ -5,8 +5,7 @@ module Travis::API::V3 attr_reader :job def run - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless @job = find(:job) + @job = check_login_and_find(:job) raise WrongCredentials unless Travis.config.debug_tools_enabled or Travis::Features.active?(:debug_tools, job.repository) access_control.permissions(job).debug! diff --git a/lib/travis/api/v3/services/job/restart.rb b/lib/travis/api/v3/services/job/restart.rb index a10dc71b..26ef1499 100644 --- a/lib/travis/api/v3/services/job/restart.rb +++ b/lib/travis/api/v3/services/job/restart.rb @@ -2,8 +2,7 @@ module Travis::API::V3 class Services::Job::Restart < Service def run - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless job = find(:job) + job = check_login_and_find(:job) access_control.permissions(job).restart! query.restart(access_control.user) diff --git a/lib/travis/api/v3/services/repository/disable.rb b/lib/travis/api/v3/services/repository/disable.rb index c904351e..169af0d7 100644 --- a/lib/travis/api/v3/services/repository/disable.rb +++ b/lib/travis/api/v3/services/repository/disable.rb @@ -1,8 +1,7 @@ module Travis::API::V3 class Services::Repository::Disable < Service def run!(activate = false) - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) check_access(repository) admin = access_control.admin_for(repository) diff --git a/lib/travis/api/v3/services/repository/star.rb b/lib/travis/api/v3/services/repository/star.rb index 0b29c2bb..cd3f1a24 100644 --- a/lib/travis/api/v3/services/repository/star.rb +++ b/lib/travis/api/v3/services/repository/star.rb @@ -1,8 +1,7 @@ module Travis::API::V3 class Services::Repository::Star < Service def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) check_access(repository) current_user = access_control.user query.star(current_user) diff --git a/lib/travis/api/v3/services/repository/unstar.rb b/lib/travis/api/v3/services/repository/unstar.rb index 75874f9f..39e75856 100644 --- a/lib/travis/api/v3/services/repository/unstar.rb +++ b/lib/travis/api/v3/services/repository/unstar.rb @@ -1,8 +1,7 @@ module Travis::API::V3 class Services::Repository::Unstar < Service def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) check_access(repository) current_user = access_control.user query.unstar(current_user) diff --git a/lib/travis/api/v3/services/requests/create.rb b/lib/travis/api/v3/services/requests/create.rb index 87c54d2d..ec4c6a08 100644 --- a/lib/travis/api/v3/services/requests/create.rb +++ b/lib/travis/api/v3/services/requests/create.rb @@ -8,8 +8,7 @@ module Travis::API::V3 params "request", "user", :config, :message, :branch, :token def run - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) access_control.permissions(repository).create_request! user = find(:user) if access_control.full_access? and params_for? 'user'.freeze diff --git a/lib/travis/api/v3/services/settings/find.rb b/lib/travis/api/v3/services/settings/find.rb index 9ab54fec..bd7c08c9 100644 --- a/lib/travis/api/v3/services/settings/find.rb +++ b/lib/travis/api/v3/services/settings/find.rb @@ -1,8 +1,7 @@ module Travis::API::V3 class Services::Settings::Find < Service def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repo = find(:repository) + repository = check_login_and_find(:repository) find(:settings, repo) end end diff --git a/lib/travis/api/v3/services/settings/update.rb b/lib/travis/api/v3/services/settings/update.rb index c2780e20..924bc39f 100644 --- a/lib/travis/api/v3/services/settings/update.rb +++ b/lib/travis/api/v3/services/settings/update.rb @@ -3,8 +3,7 @@ module Travis::API::V3 params :builds_only_with_travis_yml, :build_pushes, :build_pull_requests, :maximum_number_of_builds, prefix: :settings def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless repository = find(:repository) + repository = check_login_and_find(:repository) query.update(repository) end end diff --git a/lib/travis/api/v3/services/user/sync.rb b/lib/travis/api/v3/services/user/sync.rb index 5ece301b..16cd3674 100644 --- a/lib/travis/api/v3/services/user/sync.rb +++ b/lib/travis/api/v3/services/user/sync.rb @@ -2,8 +2,7 @@ module Travis::API::V3 class Services::User::Sync < Service def run! - raise LoginRequired unless access_control.logged_in? or access_control.full_access? - raise NotFound unless user = find(:user) + user = check_login_and_find(:user) access_control.permissions(user).sync! query.sync(user) From f63bb1c6a2ec12b2326a546d7550c3c05d89eb81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Hendricksen?= Date: Thu, 7 Jul 2016 01:16:39 -0400 Subject: [PATCH 17/21] if this logic isn't needed we should take it out later --- lib/travis/api/v3/service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/travis/api/v3/service.rb b/lib/travis/api/v3/service.rb index d0cb719f..f3243189 100644 --- a/lib/travis/api/v3/service.rb +++ b/lib/travis/api/v3/service.rb @@ -68,7 +68,7 @@ module Travis::API::V3 def check_login_and_find(*args) raise LoginRequired unless access_control.full_access_or_logged_in? - find(*args) # should this raise NotFound if nil? Can it return nil? see above? + find(*args) or raise NotFound end def not_found(actually_not_found = false, type = nil) From 3fb7c35286ebc5b598b817633acf9d4717c95d30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Hendricksen?= Date: Thu, 7 Jul 2016 01:30:35 -0400 Subject: [PATCH 18/21] copy pasta fix --- lib/travis/api/v3/services/settings/find.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/travis/api/v3/services/settings/find.rb b/lib/travis/api/v3/services/settings/find.rb index bd7c08c9..3dff2eb8 100644 --- a/lib/travis/api/v3/services/settings/find.rb +++ b/lib/travis/api/v3/services/settings/find.rb @@ -1,7 +1,7 @@ module Travis::API::V3 class Services::Settings::Find < Service def run! - repository = check_login_and_find(:repository) + repo = check_login_and_find(:repository) find(:settings, repo) end end From 2783a69f89229d71f8fd0a7b16c59d0189a1e037 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Hendricksen?= Date: Tue, 12 Jul 2016 14:12:46 -0400 Subject: [PATCH 19/21] encrpted value and working tests --- lib/travis/api/v3/models/env_var.rb | 4 ++-- lib/travis/api/v3/renderer.rb | 3 ++- lib/travis/api/v3/renderer/model_renderer.rb | 1 + spec/v3/services/env_var/delete_spec.rb | 2 +- spec/v3/services/env_vars/create_spec.rb | 3 +-- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/travis/api/v3/models/env_var.rb b/lib/travis/api/v3/models/env_var.rb index 1d4c7f8f..27f2606f 100644 --- a/lib/travis/api/v3/models/env_var.rb +++ b/lib/travis/api/v3/models/env_var.rb @@ -2,7 +2,7 @@ module Travis::API::V3 class Models::EnvVar < Travis::Settings::Model attribute :id, Integer attribute :name, String - attribute :value, String + attribute :value, Travis::Settings::EncryptedValue attribute :public, Boolean attribute :repository_id, Integer @@ -11,7 +11,7 @@ module Travis::API::V3 end validates_each :id, :name do |record, attr, value| - others = record.repository.env_vars.select { |ev| ev.id != record.id } + others = record.repository.env_vars.select { |ev| ev.id != record.id } record.errors.add(:base, :duplicate_resource) if others.find { |ev| ev.send(attr) == record.send(attr) } end end diff --git a/lib/travis/api/v3/renderer.rb b/lib/travis/api/v3/renderer.rb index 637f937d..2710fbc5 100644 --- a/lib/travis/api/v3/renderer.rb +++ b/lib/travis/api/v3/renderer.rb @@ -10,7 +10,7 @@ module Travis::API::V3 extend self def clear(**args) - args.select { |key, value| !value.nil? } + args.compact end def href(type, string_args = nil, script_name: nil, **args) @@ -49,6 +49,7 @@ module Travis::API::V3 when Model then render_model(value, **options) when ActiveRecord::Relation then render_value(value.to_a, **options) when ActiveRecord::Associations::CollectionProxy then render_value(value.to_a, **options) + when Travis::Settings::EncryptedValue then value # Should this be value.decrypt ?? If so do we want to add if options[:included].first.public? so we ensure we only decrypt public values? else raise ArgumentError, 'cannot render %p (%p)' % [value.class, value] end end diff --git a/lib/travis/api/v3/renderer/model_renderer.rb b/lib/travis/api/v3/renderer/model_renderer.rb index dd5bdc0b..9df7506f 100644 --- a/lib/travis/api/v3/renderer/model_renderer.rb +++ b/lib/travis/api/v3/renderer/model_renderer.rb @@ -110,6 +110,7 @@ module Travis::API::V3 end fields.each do |field| + next if field == :value && !@model.public? value = Renderer.render_value(send(field), access_control: access_control, script_name: script_name, diff --git a/spec/v3/services/env_var/delete_spec.rb b/spec/v3/services/env_var/delete_spec.rb index 870c69e9..96975893 100644 --- a/spec/v3/services/env_var/delete_spec.rb +++ b/spec/v3/services/env_var/delete_spec.rb @@ -28,6 +28,6 @@ describe Travis::API::V3::Services::EnvVar::Delete, set_app: true do end example { expect(last_response.status).to eq 200 } - example { pending 'should we return an empty body here?' } + example { expect(JSON.parse(last_response.body)["id"]).to eq(env_var[:id]) } end end diff --git a/spec/v3/services/env_vars/create_spec.rb b/spec/v3/services/env_vars/create_spec.rb index 1755e593..7a8e4587 100644 --- a/spec/v3/services/env_vars/create_spec.rb +++ b/spec/v3/services/env_vars/create_spec.rb @@ -13,7 +13,7 @@ describe Travis::API::V3::Services::EnvVars::Create, set_app: true do describe 'authenticated, repo missing' do before { post("/v3/repo/99999999/env_vars", {}, auth_headers) } - include_examples 'missing repo' + include_examples 'missing repo' end describe 'authenticated, existing repo, env var already exists' do @@ -58,7 +58,6 @@ describe Travis::API::V3::Services::EnvVars::Create, set_app: true do '@type' => 'env_var', '@representation' => 'standard', 'name' => 'FOO', - 'value' => 'bar', 'public' => false ) expect(response).to include('@href', 'id') From 58cd17158c36932cd9a3ada553a22c83ea915d83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Hendricksen?= Date: Thu, 14 Jul 2016 17:24:49 -0400 Subject: [PATCH 20/21] decrypt the public values for return and fix the tests --- lib/travis/api/v3/renderer.rb | 2 +- spec/v3/services/env_var/delete_spec.rb | 2 +- spec/v3/services/env_var/find_spec.rb | 4 +- spec/v3/services/env_var/update_spec.rb | 4 +- spec/v3/services/env_vars/create_spec.rb | 63 +++++++++++++------ .../services/env_vars/for_repository_spec.rb | 8 +-- 6 files changed, 55 insertions(+), 28 deletions(-) diff --git a/lib/travis/api/v3/renderer.rb b/lib/travis/api/v3/renderer.rb index 2710fbc5..e31a7c77 100644 --- a/lib/travis/api/v3/renderer.rb +++ b/lib/travis/api/v3/renderer.rb @@ -49,7 +49,7 @@ module Travis::API::V3 when Model then render_model(value, **options) when ActiveRecord::Relation then render_value(value.to_a, **options) when ActiveRecord::Associations::CollectionProxy then render_value(value.to_a, **options) - when Travis::Settings::EncryptedValue then value # Should this be value.decrypt ?? If so do we want to add if options[:included].first.public? so we ensure we only decrypt public values? + when Travis::Settings::EncryptedValue then value.decrypt else raise ArgumentError, 'cannot render %p (%p)' % [value.class, value] end end diff --git a/spec/v3/services/env_var/delete_spec.rb b/spec/v3/services/env_var/delete_spec.rb index 96975893..305b01da 100644 --- a/spec/v3/services/env_var/delete_spec.rb +++ b/spec/v3/services/env_var/delete_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Travis::API::V3::Services::EnvVar::Delete, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } - let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:env_var) { { id: 'abc', name: 'FOO', value: Travis::Settings::EncryptedValue.new('bar'), public: true, repository_id: repo.id } } let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } describe 'not authenticated' do diff --git a/spec/v3/services/env_var/find_spec.rb b/spec/v3/services/env_var/find_spec.rb index f80cfdc1..1438ccca 100644 --- a/spec/v3/services/env_var/find_spec.rb +++ b/spec/v3/services/env_var/find_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Travis::API::V3::Services::EnvVar::Find, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } - let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:env_var) { { id: 'abc', name: 'FOO', value: Travis::Settings::EncryptedValue.new('bar'), public: true, repository_id: repo.id } } let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } describe 'not authenticated' do @@ -36,7 +36,7 @@ describe Travis::API::V3::Services::EnvVar::Find, set_app: true do 'id' => env_var[:id], 'name' => env_var[:name], 'public' => env_var[:public], - 'value' => env_var[:value] + 'value' => env_var[:value].decrypt ) end end diff --git a/spec/v3/services/env_var/update_spec.rb b/spec/v3/services/env_var/update_spec.rb index bac9a065..c4cab579 100644 --- a/spec/v3/services/env_var/update_spec.rb +++ b/spec/v3/services/env_var/update_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Travis::API::V3::Services::EnvVar::Update, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } - let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:env_var) { { id: 'abc', name: 'FOO', value: Travis::Settings::EncryptedValue.new('bar'), public: true, repository_id: repo.id } } let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } let(:json_headers) { { 'CONTENT_TYPE' => 'application/json' } } @@ -42,7 +42,7 @@ describe Travis::API::V3::Services::EnvVar::Update, set_app: true do '@representation' => 'standard', 'id' => env_var[:id], 'name' => params['env_var.name'], - 'value' => env_var[:value], + 'value' => env_var[:value].decrypt, 'public' => env_var[:public] ) end diff --git a/spec/v3/services/env_vars/create_spec.rb b/spec/v3/services/env_vars/create_spec.rb index 7a8e4587..48e82f21 100644 --- a/spec/v3/services/env_vars/create_spec.rb +++ b/spec/v3/services/env_vars/create_spec.rb @@ -26,7 +26,7 @@ describe Travis::API::V3::Services::EnvVars::Create, set_app: true do end before do - repo.update_attributes(settings: JSON.generate(env_vars: [{ id: 'abc', name: 'FOO', value: 'bar', public: false }])) + repo.update_attributes(settings: JSON.generate(env_vars: [{ id: 'abc', name: 'FOO', value: Travis::Settings::EncryptedValue.new('bar'), public: false }])) post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) end @@ -41,26 +41,53 @@ describe Travis::API::V3::Services::EnvVars::Create, set_app: true do end describe 'authenticated, existing repo, env var is new' do - let(:params) do - { - 'env_var.name' => 'FOO', - 'env_var.value' => 'bar', - 'env_var.public' => false - } + describe 'private' do + let(:params) do + { + 'env_var.name' => 'FOO', + 'env_var.value' => 'bar', + 'env_var.public' => false + } + end + + before { post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) } + + example { expect(last_response.status).to eq 201 } + example do + response = JSON.load(body) + expect(response).to include( + '@type' => 'env_var', + '@representation' => 'standard', + 'name' => 'FOO', + 'public' => false + ) + expect(response).to include('@href', 'id') + end end - before { post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) } + describe 'public' do + let(:params) do + { + 'env_var.name' => 'FOO', + 'env_var.value' => 'bar', + 'env_var.public' => true + } + end - example { expect(last_response.status).to eq 201 } - example do - response = JSON.load(body) - expect(response).to include( - '@type' => 'env_var', - '@representation' => 'standard', - 'name' => 'FOO', - 'public' => false - ) - expect(response).to include('@href', 'id') + before { post("/v3/repo/#{repo.id}/env_vars", JSON.generate(params), auth_headers.merge(json_headers)) } + + example { expect(last_response.status).to eq 201 } + example do + response = JSON.load(body) + expect(response).to include( + '@type' => 'env_var', + '@representation' => 'standard', + 'name' => 'FOO', + 'value' => 'bar', + 'public' => true + ) + expect(response).to include('@href', 'id') + end end end end diff --git a/spec/v3/services/env_vars/for_repository_spec.rb b/spec/v3/services/env_vars/for_repository_spec.rb index fd20de9d..eeb4e928 100644 --- a/spec/v3/services/env_vars/for_repository_spec.rb +++ b/spec/v3/services/env_vars/for_repository_spec.rb @@ -3,9 +3,9 @@ require 'spec_helper' describe Travis::API::V3::Services::EnvVars::ForRepository, set_app: true do let(:repo) { Travis::API::V3::Models::Repository.where(owner_name: 'svenfuchs', name: 'minimal').first_or_create } let(:token) { Travis::Api::App::AccessToken.create(user: repo.owner, app_id: 1) } - let(:env_var) { { id: 'abc', name: 'FOO', value: 'bar', public: true, repository_id: repo.id } } + let(:env_var) { { id: 'abc', name: 'FOO', value: Travis::Settings::EncryptedValue.new('bar'), public: true, repository_id: repo.id } } let(:auth_headers) { { 'HTTP_AUTHORIZATION' => "token #{token}" } } - + describe 'not authenticated' do before { get("/v3/repo/#{repo.id}/env_vars") } include_examples 'not authenticated' @@ -13,7 +13,7 @@ describe Travis::API::V3::Services::EnvVars::ForRepository, set_app: true do describe 'authenticated, missing repo' do before { get("/v3/repo/999999999/env_vars", {}, auth_headers) } - include_examples 'missing repo' + include_examples 'missing repo' end describe 'authenticated, existing repo, no env vars' do @@ -49,7 +49,7 @@ describe Travis::API::V3::Services::EnvVars::ForRepository, set_app: true do '@representation' => 'standard', 'id' => env_var[:id], 'name' => env_var[:name], - 'value' => env_var[:value], + 'value' => env_var[:value].decrypt, 'public' => env_var[:public] } ] From 48e4a2c589c6961b5655dd79b0bcf5f43d88f24a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Hendricksen?= Date: Thu, 14 Jul 2016 17:47:17 -0400 Subject: [PATCH 21/21] use travis settings for encrypted columns --- .../api/v3/extensions/encrypted_column.rb | 109 ------------------ lib/travis/api/v3/models/ssl_key.rb | 2 +- lib/travis/api/v3/models/token.rb | 2 +- lib/travis/api/v3/models/user.rb | 2 +- 4 files changed, 3 insertions(+), 112 deletions(-) delete mode 100644 lib/travis/api/v3/extensions/encrypted_column.rb diff --git a/lib/travis/api/v3/extensions/encrypted_column.rb b/lib/travis/api/v3/extensions/encrypted_column.rb deleted file mode 100644 index 6818b887..00000000 --- a/lib/travis/api/v3/extensions/encrypted_column.rb +++ /dev/null @@ -1,109 +0,0 @@ -require 'securerandom' -require 'base64' - -module Travis::API::V3 - module Extensions - class EncryptedColumn - attr_reader :disable, :options - alias disabled? disable - - def initialize(options = {}) - @options = options || {} - @disable = self.options[:disable] - @key = self.options[:key] - end - - def enabled? - !disabled? - end - - def load(data) - return nil unless data - - data = data.to_s - - decrypt?(data) ? decrypt(data) : data - end - - def dump(data) - encrypt?(data) ? encrypt(data.to_s) : data - end - - def key - @key || config.key - end - - def iv - SecureRandom.hex(8) - end - - def prefix - '--ENCR--' - end - - def decrypt?(data) - data.present? && (!use_prefix? || prefix_used?(data)) - end - - def encrypt?(data) - data.present? && enabled? - end - - def prefix_used?(data) - data[0..7] == prefix - end - - def decrypt(data) - data = data[8..-1] if prefix_used?(data) - - data = decode data - - iv = data[-16..-1] - data = data[0..-17] - - aes = create_aes :decrypt, key.to_s, iv - - result = aes.update(data) + aes.final - end - - def encrypt(data) - iv = self.iv - - aes = create_aes :encrypt, key.to_s, iv - - encrypted = aes.update(data) + aes.final - - encrypted = "#{encrypted}#{iv}" - encrypted = encode encrypted - encrypted = "#{prefix}#{encrypted}" if use_prefix? - encrypted - end - - def use_prefix? - options.has_key?(:use_prefix) ? options[:use_prefix] : Travis::Features.feature_inactive?(:db_encryption_prefix) - end - - def create_aes(mode = :encrypt, key, iv) - aes = OpenSSL::Cipher::AES.new(256, :CBC) - - aes.send(mode) - aes.key = key - aes.iv = iv - - aes - end - - def config - Travis.config.encryption - end - - def decode(str) - Base64.strict_decode64 str - end - - def encode(str) - Base64.strict_encode64 str - end - end - end -end diff --git a/lib/travis/api/v3/models/ssl_key.rb b/lib/travis/api/v3/models/ssl_key.rb index 75e1e8a2..bd9685c7 100644 --- a/lib/travis/api/v3/models/ssl_key.rb +++ b/lib/travis/api/v3/models/ssl_key.rb @@ -2,7 +2,7 @@ module Travis::API::V3 class Models::SSLKey < Model belongs_to :repository - serialize :private_key, Travis::API::V3::Extensions::EncryptedColumn.new + serialize :private_key, Travis::Settings::EncryptedColumn.new def encoded_public_key key = build_key.public_key diff --git a/lib/travis/api/v3/models/token.rb b/lib/travis/api/v3/models/token.rb index 90965d0b..4cd6bf35 100644 --- a/lib/travis/api/v3/models/token.rb +++ b/lib/travis/api/v3/models/token.rb @@ -2,7 +2,7 @@ module Travis::API::V3 class Models::Token < Model belongs_to :user validate :token, presence: true - serialize :token, Extensions::EncryptedColumn.new(disable: true) + serialize :token, Travis::Settings::EncryptedColumn.new(disable: true) before_validation :generate_token, on: :create protected diff --git a/lib/travis/api/v3/models/user.rb b/lib/travis/api/v3/models/user.rb index 510542c8..66ccf86d 100644 --- a/lib/travis/api/v3/models/user.rb +++ b/lib/travis/api/v3/models/user.rb @@ -9,7 +9,7 @@ module Travis::API::V3 has_many :stars has_one :subscription, as: :owner - serialize :github_oauth_token, Extensions::EncryptedColumn.new(disable: true) + serialize :github_oauth_token, Travis::Settings::EncryptedColumn.new(disable: true) def token tokens.first_or_create.token