From 87674f52bbd66a2b645d514811db0821a97670fb Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Mon, 28 Oct 2013 17:37:50 +0100 Subject: [PATCH] Redirect to web client on insufficient oauth scopes --- lib/travis/api/app/endpoint/authorization.rb | 32 +++++++++-- .../authorization/user_manager_spec.rb | 4 +- spec/unit/endpoint/authorization_spec.rb | 56 +++++++++++++++++++ 3 files changed, 84 insertions(+), 8 deletions(-) diff --git a/lib/travis/api/app/endpoint/authorization.rb b/lib/travis/api/app/endpoint/authorization.rb index 80a05363..ce48fd49 100644 --- a/lib/travis/api/app/endpoint/authorization.rb +++ b/lib/travis/api/app/endpoint/authorization.rb @@ -202,6 +202,14 @@ class Travis::Api::App class UserManager < Struct.new(:data, :token, :drop_token) include User::Renaming + attr_accessor :user + + def initialize(*) + super + + @user = ::User.find_by_github_id(data['id']) + end + def info(attributes = {}) info = data.to_hash.slice('name', 'login', 'gravatar_id') info.merge! attributes.stringify_keys @@ -209,9 +217,12 @@ class Travis::Api::App info end + def user_exists? + user + end + def fetch retried ||= false - user = ::User.find_by_github_id(data['id']) info = drop_token ? self.info : self.info(github_oauth_token: token) ActiveRecord::Base.transaction do @@ -219,7 +230,7 @@ class Travis::Api::App rename_repos_owner(user.login, info['login']) user.update_attributes info else - user = ::User.create! info + self.user = ::User.create! info end nullify_logins(user.github_id, user.login) @@ -235,11 +246,20 @@ class Travis::Api::App end def user_for_github_token(token, drop_token = false) - data = GH.with(token: token.to_s, client_id: nil) { GH['user'] } - scopes = parse_scopes data.headers['x-oauth-scopes'] - halt 403, 'insufficient access: %p' unless acceptable? scopes + data = GH.with(token: token.to_s, client_id: nil) { GH['user'] } + scopes = parse_scopes data.headers['x-oauth-scopes'] + manager = UserManager.new(data, token, drop_token) - user = UserManager.new(data, token, drop_token).fetch + unless acceptable? scopes + # TODO: we should probably only redirect if this is a web + # oauth request, are there any other possibilities to + # consider? + url = Travis.config.oauth2.insufficient_access_redirect_url + url += "#existing-user" if manager.user_exists? + redirect to(url) + end + + user = manager.fetch halt 403, 'not a Travis user' if user.nil? user end diff --git a/spec/unit/endpoint/authorization/user_manager_spec.rb b/spec/unit/endpoint/authorization/user_manager_spec.rb index d55c68fb..914cca27 100644 --- a/spec/unit/endpoint/authorization/user_manager_spec.rb +++ b/spec/unit/endpoint/authorization/user_manager_spec.rb @@ -30,11 +30,11 @@ describe Travis::Api::App::Endpoint::Authorization::UserManager do } it 'drops the token when drop_token is set to true' do - manager = described_class.new(data, 'abc123', true) - user = stub('user', login: 'drogus', github_id: 456) User.expects(:find_by_github_id).with(456).returns(user) + manager = described_class.new(data, 'abc123', true) + attributes = { login: 'drogus', github_id: 456 }.stringify_keys user.expects(:update_attributes).with(attributes) diff --git a/spec/unit/endpoint/authorization_spec.rb b/spec/unit/endpoint/authorization_spec.rb index 12f8529c..e8425082 100644 --- a/spec/unit/endpoint/authorization_spec.rb +++ b/spec/unit/endpoint/authorization_spec.rb @@ -23,6 +23,62 @@ describe Travis::Api::App::Endpoint::Authorization do pending "not yet implemented" end + describe "GET /auth/handshake" do + describe 'with insufficient oauth permissions' do + before do + # TODO: this is a first try of writing tests for authorization, it does not look pretty + # but I wanted to have something to start with, later on I would like to extract + # this flow to a class, which would be easier to test. + @original_config = Travis.config.oauth2 + Travis.config.oauth2 = { + authorization_server: 'https://foobar.com', + access_token_path: '/access_token_path', + client_id: 'client-id', + client_secret: 'client-secret', + scope: 'public_repo,user:email,new_scope', + insufficient_access_redirect_url: 'https://travis-ci.org/insufficient_access' + } + Travis.redis.sadd('github:states', 'github-state') + + response = mock('response') + response.expects(:body).returns('access_token=foobarbaz-token') + Faraday.expects(:post).with('https://foobar.com/access_token_path', + client_id: 'client-id', + client_secret: 'client-secret', + scope: 'public_repo,user:email,new_scope', + redirect_uri: 'http://example.org/auth/handshake', + state: 'github-state', + code: 'oauth-code').returns(response) + + data = { 'id' => 111 } + data.expects(:headers).returns('x-oauth-scopes' => 'public_repo,user:email') + GH.expects(:with).with(token: 'foobarbaz-token', client_id: nil).returns(data) + end + + after do + Travis.config.oauth2 = @original_config + Travis.redis.srem('github:states', 'github-state') + # this is cached after first run, so if we change scopes, it will stay + # like that for the rest of the run + User::Oauth.instance_variable_set("@wanted_scopes", nil) + end + + it 'redirects to insufficient access page' do + response = get '/auth/handshake?state=github-state&code=oauth-code' + response.should redirect_to('https://travis-ci.org/insufficient_access') + end + + it 'redirects to insufficient access page for existing user' do + user = mock('user') + User.expects(:find_by_github_id).with(111).returns(user) + expect { + response = get '/auth/handshake?state=github-state&code=oauth-code' + response.should redirect_to('https://travis-ci.org/insufficient_access#existing-user') + }.to_not change { User.count } + end + end + end + describe 'POST /auth/github' do before do data = { 'id' => user.github_id, 'name' => user.name, 'login' => user.login, 'gravatar_id' => user.gravatar_id }