From 01356df26ff12dc41a99fac524f006f380579021 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Mon, 22 Apr 2013 17:10:51 +0200 Subject: [PATCH 1/7] Implement expiring access tokens --- lib/travis/api/app/access_token.rb | 15 ++++++++++++-- spec/unit/access_token_spec.rb | 32 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 spec/unit/access_token_spec.rb diff --git a/lib/travis/api/app/access_token.rb b/lib/travis/api/app/access_token.rb index a8e324b9..b3e62e5f 100644 --- a/lib/travis/api/app/access_token.rb +++ b/lib/travis/api/app/access_token.rb @@ -4,7 +4,7 @@ require 'securerandom' class Travis::Api::App class AccessToken DEFAULT_SCOPES = [:public, :private] - attr_reader :token, :scopes, :user_id, :app_id + attr_reader :token, :scopes, :user_id, :app_id, :expires_in def self.create(options = {}) new(options).tap(&:save) @@ -25,6 +25,12 @@ class Travis::Api::App raise ArgumentError, 'must supply either user_id or user' unless options.key?(:user) ^ options.key?(:user_id) raise ArgumentError, 'must supply app_id' unless options.key?(:app_id) + begin + @expires_in = Integer(options[:expires_in]) if options[:expires_in] + rescue ArgumentError + raise ArgumentError, 'expires_in must be of integer type' + end + @app_id = Integer(options[:app_id]) @scopes = Array(options[:scopes] || options[:scope] || DEFAULT_SCOPES).map(&:to_sym) @user = options[:user] @@ -37,6 +43,11 @@ class Travis::Api::App redis.del(key) redis.rpush(key, [user_id, app_id, *scopes].map(&:to_s)) redis.set(reuse_key, token) + + if expires_in + redis.expire(reuse_key, expires_in) + redis.expire(key, expires_in) + end end def user @@ -68,7 +79,7 @@ class Travis::Api::App private def reuse_token - redis.get(reuse_key) + redis.get(reuse_key) unless expires_in end def reuse_key diff --git a/spec/unit/access_token_spec.rb b/spec/unit/access_token_spec.rb new file mode 100644 index 00000000..846b0d9d --- /dev/null +++ b/spec/unit/access_token_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe Travis::Api::App::AccessToken do + it 'errors out on wrong type of :expires_in argument' do + expect { + described_class.new(app_id: 1, user_id: 2, expires_in: 'foo') + }.to raise_error(ArgumentError, 'expires_in must be of integer type') + end + + it 'allows to skip expires_in' do + expect { + described_class.new(app_id: 1, user_id: 2, expires_in: nil) + }.to_not raise_error(ArgumentError) + end + + it 'does not reuse token if expires_in is set' do + token = described_class.new(app_id: 1, user_id: 2).tap(&:save) + new_token = described_class.new(app_id: 1, user_id: 2, expires_in: 10) + + token.token.should_not == new_token.token + end + + it 'expires the token after given period of time' do + token = described_class.new(app_id: 1, user_id: 2, expires_in: 1).tap(&:save) + + described_class.find_by_token(token.token).should_not be_nil + + sleep 1.5 + + described_class.find_by_token(token.token).should be_nil + end +end From 1340fdb316f5a49183c076dfd988d98bdfce3060 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Wed, 24 Apr 2013 03:03:48 +0200 Subject: [PATCH 2/7] Allow to pass additional responders to respond_with --- lib/travis/api/app/helpers/respond_with.rb | 16 +++++++++--- lib/travis/api/app/responders/image.rb | 2 +- lib/travis/api/app/responders/json.rb | 2 +- lib/travis/api/app/responders/plain.rb | 2 +- lib/travis/api/app/responders/xml.rb | 2 +- spec/integration/responders_spec.rb | 30 ++++++++++++++++++++++ spec/unit/responders/json_spec.rb | 6 ++--- 7 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 spec/integration/responders_spec.rb diff --git a/lib/travis/api/app/helpers/respond_with.rb b/lib/travis/api/app/helpers/respond_with.rb index a5b19204..19f4c4fd 100644 --- a/lib/travis/api/app/helpers/respond_with.rb +++ b/lib/travis/api/app/helpers/respond_with.rb @@ -10,8 +10,8 @@ class Travis::Api::App def respond_with(resource, options = {}) result = respond(resource, options) - result = result ? result.to_json : 404 - halt result + result = result.to_json if result && response.content_type =~ /application\/json/ + halt result || 404 end def body(value = nil, options = {}, &block) @@ -24,10 +24,18 @@ class Travis::Api::App def respond(resource, options) resource = apply_service_responder(resource, options) - response = acceptable_formats.find do |accept| + response = nil + acceptable_formats.find do |accept| responders(resource, options).find do |const| responder = const.new(self, resource, options.dup.merge(accept: accept)) - responder.apply if responder.apply? + response = responder.apply if responder.apply? + end + end + + if responders = options[:responders] + responders.each do |klass| + responder = klass.new(self, response, options) + response = responder.apply if responder.apply? end end diff --git a/lib/travis/api/app/responders/image.rb b/lib/travis/api/app/responders/image.rb index d80d28e4..c6c6a17d 100644 --- a/lib/travis/api/app/responders/image.rb +++ b/lib/travis/api/app/responders/image.rb @@ -8,7 +8,7 @@ module Travis::Api::App::Responders headers['Pragma'] = "no-cache" headers['Expires'] = Time.now.utc.httpdate headers['Content-Disposition'] = %(inline; filename="#{File.basename(filename)}") - halt send_file(filename, type: :png, last_modified: last_modified) + send_file(filename, type: :png, last_modified: last_modified) end private diff --git a/lib/travis/api/app/responders/json.rb b/lib/travis/api/app/responders/json.rb index 15388f6b..012b2be8 100644 --- a/lib/travis/api/app/responders/json.rb +++ b/lib/travis/api/app/responders/json.rb @@ -10,7 +10,7 @@ class Travis::Api::App def apply super - halt result.to_json if result + result end private diff --git a/lib/travis/api/app/responders/plain.rb b/lib/travis/api/app/responders/plain.rb index 9d045478..63d3d223 100644 --- a/lib/travis/api/app/responders/plain.rb +++ b/lib/travis/api/app/responders/plain.rb @@ -21,7 +21,7 @@ module Travis::Api::App::Responders headers['Content-Disposition'] = %(#{disposition}; filename="#{filename}") - halt(params[:deansi] ? clear_ansi(resource.content) : resource.content) + params[:deansi] ? clear_ansi(resource.content) : resource.content end private diff --git a/lib/travis/api/app/responders/xml.rb b/lib/travis/api/app/responders/xml.rb index b69911b1..836d5f20 100644 --- a/lib/travis/api/app/responders/xml.rb +++ b/lib/travis/api/app/responders/xml.rb @@ -22,7 +22,7 @@ module Travis::Api::App::Responders def apply super - halt TEMPLATE % data + TEMPLATE % data end private diff --git a/spec/integration/responders_spec.rb b/spec/integration/responders_spec.rb new file mode 100644 index 00000000..8624b486 --- /dev/null +++ b/spec/integration/responders_spec.rb @@ -0,0 +1,30 @@ +require 'spec_helper' + +describe 'App' do + before do + FactoryGirl.create(:test, :number => '3.1', :queue => 'builds.common') + + responder = Class.new(Travis::Api::App::Responders::Base) do + def apply? + true + end + + def apply + resource[:extra] = 'moar!' + + resource + end + end + + add_endpoint '/foo' do + get '/hash' do + respond_with({ foo: 'bar' }, responders: [responder]) + end + end + end + + it '' do + response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json' + JSON.parse(response.body).should == { 'foo' => 'bar', 'extra' => 'moar!' } + end +end diff --git a/spec/unit/responders/json_spec.rb b/spec/unit/responders/json_spec.rb index 37146d58..5b43bbc1 100644 --- a/spec/unit/responders/json_spec.rb +++ b/spec/unit/responders/json_spec.rb @@ -23,8 +23,7 @@ module Travis::Api::App::Responders let(:resource) { { foo: 'bar' } } it 'returns resource converted to_json' do - json.expects(:halt).with({ foo: 'bar' }.to_json) - json.apply + json.apply.should == { foo: 'bar' } end end @@ -46,8 +45,7 @@ module Travis::Api::App::Responders end it 'returns proper data converted to json' do - json.expects(:halt).with({ foo: 'bar' }.to_json) - json.apply + json.apply.should == { foo: 'bar' } end end end From ee64af14d52cbbc5d558d1822912ad1ba3629ad2 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Tue, 7 May 2013 23:54:56 +0200 Subject: [PATCH 3/7] Allow to specify more than one scope for an endpoint --- lib/travis/api/app/extensions/scoping.rb | 35 +++++++++++++++--------- spec/integration/responders_spec.rb | 2 +- spec/integration/scopes_spec.rb | 32 ++++++++++++++++++++++ 3 files changed, 55 insertions(+), 14 deletions(-) create mode 100644 spec/integration/scopes_spec.rb diff --git a/lib/travis/api/app/extensions/scoping.rb b/lib/travis/api/app/extensions/scoping.rb index 626bdbb2..2e1624d3 100644 --- a/lib/travis/api/app/extensions/scoping.rb +++ b/lib/travis/api/app/extensions/scoping.rb @@ -18,23 +18,32 @@ class Travis::Api::App app.helpers(Helpers) end - def scope(name) + def scope(*names) condition do - name = settings.default_scope if name == :default + names = [settings.default_scope] if names == [:default] scopes = env['travis.access_token'].try(:scopes) || settings.anonymous_scopes - headers['X-OAuth-Scopes'] = scopes.map(&:to_s).join(',') - headers['X-Accepted-OAuth-Scopes'] = name.to_s - if scopes.include? name - env['travis.scope'] = name - headers['Vary'] = 'Accept' - headers['Vary'] << ', Authorization' unless public? - true - elsif env['travis.access_token'] - pass { halt 403, "insufficient access" } - else - pass { halt 401, "no access token supplied" } + result = names.any? do |name| + if scopes.include? name + headers['X-OAuth-Scopes'] = scopes.map(&:to_s).join(',') + headers['X-Accepted-OAuth-Scopes'] = name.to_s + + env['travis.scope'] = name + headers['Vary'] = 'Accept' + headers['Vary'] << ', Authorization' unless public? + true + end end + + if !result + if env['travis.access_token'] + pass { halt 403, "insufficient access" } + else + pass { halt 401, "no access token supplied" } + end + end + + result end end diff --git a/spec/integration/responders_spec.rb b/spec/integration/responders_spec.rb index 8624b486..2dc2cb78 100644 --- a/spec/integration/responders_spec.rb +++ b/spec/integration/responders_spec.rb @@ -23,7 +23,7 @@ describe 'App' do end end - it '' do + it 'runs responder when rendering the response with respond_with' do response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json' JSON.parse(response.body).should == { 'foo' => 'bar', 'extra' => 'moar!' } end diff --git a/spec/integration/scopes_spec.rb b/spec/integration/scopes_spec.rb new file mode 100644 index 00000000..d5b13576 --- /dev/null +++ b/spec/integration/scopes_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe 'App' do + before do + FactoryGirl.create(:test, :number => '3.1', :queue => 'builds.common') + + add_endpoint '/foo' do + get '/hash', scope: [:foo, :bar] do + respond_with foo: 'bar' + end + end + end + + it 'checks if token has one of the required scopes' do + token = Travis::Api::App::AccessToken.new(app_id: 1, user_id: 2, scopes: [:foo]).tap(&:save) + + response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response.should be_successful + response.headers['X-Accepted-OAuth-Scopes'].should == 'foo' + + token = Travis::Api::App::AccessToken.new(app_id: 1, user_id: 2, scopes: [:bar]).tap(&:save) + + response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response.should be_successful + response.headers['X-Accepted-OAuth-Scopes'].should == 'bar' + + token = Travis::Api::App::AccessToken.new(app_id: 1, user_id: 2, scopes: [:baz]).tap(&:save) + + response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response.status.should == 403 + end +end From 56d61ed4612998981241ad342dc0b605181c5305 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Wed, 8 May 2013 00:48:58 +0200 Subject: [PATCH 4/7] Allow to pass extra params for tokens --- lib/travis/api/app/access_token.rb | 18 +++++++++++++++--- spec/unit/access_token_spec.rb | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lib/travis/api/app/access_token.rb b/lib/travis/api/app/access_token.rb index b3e62e5f..99a70ad5 100644 --- a/lib/travis/api/app/access_token.rb +++ b/lib/travis/api/app/access_token.rb @@ -4,7 +4,7 @@ require 'securerandom' class Travis::Api::App class AccessToken DEFAULT_SCOPES = [:public, :private] - attr_reader :token, :scopes, :user_id, :app_id, :expires_in + attr_reader :token, :scopes, :user_id, :app_id, :expires_in, :extra def self.create(options = {}) new(options).tap(&:save) @@ -18,7 +18,8 @@ class Travis::Api::App def self.find_by_token(token) return token if token.is_a? self user_id, app_id, *scopes = redis.lrange(key(token), 0, -1) - new(token: token, scopes: scopes, user_id: user_id, app_id: app_id) if user_id + extra = decode_json(scopes.pop) if scopes.last && scopes.last =~ /^json:/ + new(token: token, scopes: scopes, user_id: user_id, app_id: app_id, extra: extra) if user_id end def initialize(options = {}) @@ -36,12 +37,15 @@ class Travis::Api::App @user = options[:user] @user_id = Integer(options[:user_id] || @user.id) @token = options[:token] || reuse_token || SecureRandom.urlsafe_base64(16) + @extra = options[:extra] end def save key = key(token) redis.del(key) - redis.rpush(key, [user_id, app_id, *scopes].map(&:to_s)) + data = [user_id, app_id, *scopes] + data << encode_json(extra) if extra + redis.rpush(key, data.map(&:to_s)) redis.set(reuse_key, token) if expires_in @@ -71,6 +75,14 @@ class Travis::Api::App def key(token) "t:#{token}" end + + def encode_json(hash) + 'json:' + Base64.encode64(hash.to_json) + end + + def decode_json(json) + JSON.parse(Base64.decode64(json.gsub(/^json:/, ''))) + end end include Helpers diff --git a/spec/unit/access_token_spec.rb b/spec/unit/access_token_spec.rb index 846b0d9d..cfdca57f 100644 --- a/spec/unit/access_token_spec.rb +++ b/spec/unit/access_token_spec.rb @@ -29,4 +29,21 @@ describe Travis::Api::App::AccessToken do described_class.find_by_token(token.token).should be_nil end + + it 'allows to save extra information' do + attrs = { + app_id: 1, + user_id: 3, + expires_in: 1, + extra: { + required_params: { job_id: '1' } + } + } + + token = described_class.new(attrs).tap(&:save) + token.extra.should == attrs[:extra] + + token = described_class.find_by_token(token.token) + token.extra.should == { 'required_params' => { 'job_id' => '1' } } + end end From 3b299cfec7a16a71c6788aed730b28f8f1e34c28 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Wed, 8 May 2013 01:14:20 +0200 Subject: [PATCH 5/7] Allow to pass required_params to token required_params will be matched with actual params to check if the token may be used for authorization. For example if { job_id: 44 } is passed as a required param, the token will be rejected for GET /jobs/33 --- lib/travis/api/app/extensions/scoping.rb | 12 +++++++++++- spec/integration/scopes_spec.rb | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/lib/travis/api/app/extensions/scoping.rb b/lib/travis/api/app/extensions/scoping.rb index 2e1624d3..d932a053 100644 --- a/lib/travis/api/app/extensions/scoping.rb +++ b/lib/travis/api/app/extensions/scoping.rb @@ -11,6 +11,16 @@ class Travis::Api::App def public? scope == :public end + + def required_params_match? + return true unless token = env['travis.access_token'] + + if token.extra && (required_params = token.extra['required_params']) + required_params.all? { |name, value| params[name] == value } + else + true + end + end end def self.registered(app) @@ -24,7 +34,7 @@ class Travis::Api::App scopes = env['travis.access_token'].try(:scopes) || settings.anonymous_scopes result = names.any? do |name| - if scopes.include? name + if scopes.include?(name) && required_params_match? headers['X-OAuth-Scopes'] = scopes.map(&:to_s).join(',') headers['X-Accepted-OAuth-Scopes'] = name.to_s diff --git a/spec/integration/scopes_spec.rb b/spec/integration/scopes_spec.rb index d5b13576..6230ee54 100644 --- a/spec/integration/scopes_spec.rb +++ b/spec/integration/scopes_spec.rb @@ -8,6 +8,10 @@ describe 'App' do get '/hash', scope: [:foo, :bar] do respond_with foo: 'bar' end + + get '/:job_id/log' do + respond_with job_id: params[:job_id] + end end end @@ -29,4 +33,17 @@ describe 'App' do response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" response.status.should == 403 end + + it 'checks if required_params match the from the request' do + extra = { + required_params: { job_id: '10' } + } + token = Travis::Api::App::AccessToken.new(app_id: 1, user_id: 2, extra: extra).tap(&:save) + + response = get '/foo/10/log', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response.should be_successful + + response = get '/foo/11/log', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response.status.should == 403 + end end From cdabec540d017711e05571089d1870aa4e7b6257 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Wed, 8 May 2013 13:21:22 +0200 Subject: [PATCH 6/7] Fix scopes and access token specs --- spec/integration/scopes_spec.rb | 10 +++++----- spec/unit/access_token_spec.rb | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/integration/scopes_spec.rb b/spec/integration/scopes_spec.rb index 6230ee54..f06fd4db 100644 --- a/spec/integration/scopes_spec.rb +++ b/spec/integration/scopes_spec.rb @@ -5,7 +5,7 @@ describe 'App' do FactoryGirl.create(:test, :number => '3.1', :queue => 'builds.common') add_endpoint '/foo' do - get '/hash', scope: [:foo, :bar] do + get '/:id/bar', scope: [:foo, :bar] do respond_with foo: 'bar' end @@ -18,20 +18,20 @@ describe 'App' do it 'checks if token has one of the required scopes' do token = Travis::Api::App::AccessToken.new(app_id: 1, user_id: 2, scopes: [:foo]).tap(&:save) - response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response = get '/foo/1/bar', {}, 'HTTP_ACCEPT' => 'application/json; version=2', 'HTTP_AUTHORIZATION' => "token #{token.token}" response.should be_successful response.headers['X-Accepted-OAuth-Scopes'].should == 'foo' token = Travis::Api::App::AccessToken.new(app_id: 1, user_id: 2, scopes: [:bar]).tap(&:save) - response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response = get '/foo/1/bar', {}, 'HTTP_ACCEPT' => 'application/json; version=2', 'HTTP_AUTHORIZATION' => "token #{token.token}" response.should be_successful response.headers['X-Accepted-OAuth-Scopes'].should == 'bar' token = Travis::Api::App::AccessToken.new(app_id: 1, user_id: 2, scopes: [:baz]).tap(&:save) - response = get '/foo/hash', {}, 'HTTP_ACCEPT' => 'application/json', 'HTTP_AUTHORIZATION' => "token #{token.token}" - response.status.should == 403 + response = get '/foo/1/bar', {}, 'HTTP_ACCEPT' => 'application/json; version=2', 'HTTP_AUTHORIZATION' => "token #{token.token}" + response.status.should == 404 end it 'checks if required_params match the from the request' do diff --git a/spec/unit/access_token_spec.rb b/spec/unit/access_token_spec.rb index cfdca57f..3cb8794b 100644 --- a/spec/unit/access_token_spec.rb +++ b/spec/unit/access_token_spec.rb @@ -25,7 +25,7 @@ describe Travis::Api::App::AccessToken do described_class.find_by_token(token.token).should_not be_nil - sleep 1.5 + sleep 2 described_class.find_by_token(token.token).should be_nil end From d276cd5b326a1b5f470d703676a5b3410b68db08 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Wed, 8 May 2013 13:21:46 +0200 Subject: [PATCH 7/7] Return Oauth headers even if none scope was matched If an endpoint specifies more than one scope and none of the scopes from access token matches, return oauth headers for the first of the scopes --- lib/travis/api/app/extensions/scoping.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/travis/api/app/extensions/scoping.rb b/lib/travis/api/app/extensions/scoping.rb index d932a053..5945804e 100644 --- a/lib/travis/api/app/extensions/scoping.rb +++ b/lib/travis/api/app/extensions/scoping.rb @@ -46,6 +46,9 @@ class Travis::Api::App end if !result + headers['X-OAuth-Scopes'] = scopes.map(&:to_s).join(',') + headers['X-Accepted-OAuth-Scopes'] = names.first.to_s + if env['travis.access_token'] pass { halt 403, "insufficient access" } else