Fix updating github oauth token after signing in

lib/travis/api/app/endpoint/authorization.rb

@@ -180,18 +180,32 @@ class Travis::Api::App
           generate_token options.merge(user: user_for_github_token(token))
         end
 
-        def user_info(data, misc = {})
+        class UserManager < Struct.new(:data, :token)
-          info = data.to_hash.slice('name', 'login', 'github_oauth_token', 'gravatar_id')
+          def info(attributes = {})
-          info.merge! misc
+            info = data.to_hash.slice('name', 'login', 'gravatar_id')
-          info['github_id'] ||= data['id']
+            info.merge! attributes.stringify_keys
-          info
+            info['github_id'] ||= data['id']
+            info
+          end
+
+          def fetch
+            user   = ::User.find_by_github_id(data['id'])
+            info   = info(github_oauth_token: token)
+
+            if user
+              user.update_attributes info
+            else
+              user = ::User.create! info
+            end
+
+            user
+          end
         end
 
         def user_for_github_token(token)
           data   = GH.with(token: token.to_s) { GH['user'] }
           scopes = parse_scopes data.headers['x-oauth-scopes']
-          user   = ::User.find_by_github_id(data['id'])
+          user   = UserManager.new(data, token).fetch
-          user ||= ::User.create! user_info(data, github_oauth_token: token)
 
           halt 403, 'not a Travis user'   if user.nil?
           halt 403, 'insufficient access' unless acceptable? scopes
@@ -218,7 +232,6 @@ class Travis::Api::App
 
         def post_message(payload)
           content_type :html
-          p [:payload, payload]
           erb(:post_message, locals: payload)
         end
 

spec/unit/endpoint/authorization/user_manager_spec.rb

@@ -0,0 +1,54 @@
+require 'spec_helper'
+
+describe Travis::Api::App::Endpoint::Authorization::UserManager do
+  let(:manager) { described_class.new(data, 'abc123') }
+
+  describe '#info' do
+    let(:data) {
+      {
+        name: 'Piotr Sarnacki', login: 'drogus', gravatar_id: '123', id: 456, foo: 'bar'
+      }.stringify_keys
+    }
+
+    it 'gets data from github payload' do
+      manager.info.should == {
+        name: 'Piotr Sarnacki', login: 'drogus', gravatar_id: '123', github_id: 456
+      }.stringify_keys
+    end
+
+    it 'allows to overwrite existing keys' do
+      manager.info({login: 'piotr.sarnacki', bar: 'baz'}.stringify_keys).should == {
+        name: 'Piotr Sarnacki', login: 'piotr.sarnacki', gravatar_id: '123',
+        github_id: 456, bar: 'baz'
+      }.stringify_keys
+    end
+  end
+
+  describe '#fetch' do
+   let(:data) {
+      { login: 'drogus', id: 456 }.stringify_keys
+    }
+
+    context 'with existing user' do
+      it 'updates user data' do
+        user = mock('user')
+        User.expects(:find_by_github_id).with(456).returns(user)
+        attributes = { login: 'drogus', github_id: 456, github_oauth_token: 'abc123' }.stringify_keys
+        user.expects(:update_attributes).with(attributes)
+
+        manager.fetch.should == user
+      end
+    end
+
+    context 'without existing user' do
+      it 'creates new user' do
+        user = mock('user')
+        User.expects(:find_by_github_id).with(456).returns(nil)
+        attributes = { login: 'drogus', github_id: 456, github_oauth_token: 'abc123' }.stringify_keys
+        User.expects(:create!).with(attributes).returns(user)
+
+        manager.fetch.should == user
+      end
+    end
+  end
+end