fixed and tested Basecamp OAuth2 implementation

oa-oauth/lib/omniauth/strategies/basecamp.rb

@@ -12,49 +12,67 @@ module OmniAuth
     #    use OmniAuth::Strategies::Basecamp, 'app_id', 'app_secret'
     class Basecamp < OAuth2
       
-      BASECAMP_SUBDOMAIN_PARAMETER = 'subdomain'
+      BASECAMP_SUBDOMAIN_PARAMETER = 'basecamp_subdomain'
       
-      def initialize(app, app_id, app_secret, options = {})
-        super(app, :campfire, app_id, app_secret, options)
+      def initialize(app, client_id, client_secret, options = {})
+        super(app, :basecamp, client_id, client_secret, options)
       end
       
       protected
       
+      def client
+        ::OAuth2::Client.new(@client.id, @client.secret, :site => basecamp_url)
+      end
+      
       def request_phase
-        if env['REQUEST_METHOD'] == 'GET'
-          ask_for_basecamp_subdomain
+        if subdomain
+          super
         else
-          super(options.merge(:site => basecamp_url))
+          ask_for_basecamp_subdomain
         end
       end
       
+      def callback_phase
+        if subdomain
+          super
+        else
+          ask_for_basecamp_subdomain
+        end
+      end
+      
+      def subdomain
+        ((request.session[:oauth] ||= {})[:basecamp] ||= {})[:subdomain] ||= request.params[BASECAMP_SUBDOMAIN_PARAMETER]
+      end
+      
       def user_data
-        @data ||= MultiJson.decode(@access_token.get('/users/me.xml'))
+        @data ||= Nokogiri::XML.parse(@access_token.get('/users/me.xml'))
       end
       
       def ask_for_basecamp_subdomain
-        OmniAuth::Form.build(title) do
-          text_field 'Basecamp Subdomain', BASECAMP_SUBDOMAIN_PARAMETER
+        OmniAuth::Form.build('Basecamp Subdomain Required') do
+          text_field 'Basecamp Subdomain', ::OmniAuth::Strategies::Basecamp::BASECAMP_SUBDOMAIN_PARAMETER
         end.to_response
       end
       
-      def campfire_url
-        subdomain = request.params[BASECAMP_SUBDOMAIN_PARAMETER]
-        'http://#{subdomain}.basecamphq.com'
+      def basecamp_url
+        "https://#{subdomain}.basecamphq.com"
       end
       
       def auth_hash
-        doc = Nokogiri::XML.parse(@response.body)
+        doc = user_data
         OmniAuth::Utils.deep_merge(super, {
           'uid' => doc.xpath('person/id').text,
           'user_info' => user_info(doc),
           'credentials' => {
             'token' => doc.xpath('person/token').text
-          }
+          },
+          'extra' => {
+            'access_token' => @access_token
+          },
         })
       end
       
-      def user_info(hash)
+      def user_info(doc)
         hash = {
           'first_name' => doc.xpath('person/first-name').text,
           'last_name' => doc.xpath('person/last-name').text,

oa-oauth/lib/omniauth/strategies/oauth2.rb

@@ -14,15 +14,17 @@ module OmniAuth
         @client = ::OAuth2::Client.new(client_id, client_secret, options)
       end
       
-      attr_accessor :client_id, :client_secret, :options
+      protected
+      
+      attr_accessor :client
       
       def request_phase(options = {})
-        redirect @client.web_server.authorize_url({:redirect_uri => callback_url}.merge(options))
+        redirect client.web_server.authorize_url({:redirect_uri => callback_url}.merge(options))
       end
       
       def callback_phase
         verifier = request.params['code']
-        @access_token = @client.web_server.get_access_token(verifier, :redirect_uri => callback_url)
+        @access_token = client.web_server.get_access_token(verifier, :redirect_uri => callback_url)
         super
       rescue ::OAuth2::HTTPError => e
         fail!(:invalid_credentials)

oa-oauth/spec/fixtures/basecamp_200.xml

@@ -0,0 +1,24 @@
+<person>
+  <client-id type="integer">0</client-id>
+  <created-at type="datetime">2008-08-14T00:00:00Z</created-at>
+  <id type="integer">1827370</id>
+  <im-handle/>
+  <im-service>AOL</im-service>
+  <phone-number-fax/>
+  <phone-number-home/>
+  <phone-number-mobile/>
+  <phone-number-office/>
+  <phone-number-office-ext/>
+  <title/>
+  <token>5fc2ab4f6c2f9cdf12ed01b88e7554f8ad21bbfb</token>
+  <updated-at type="datetime">2010-05-24T11:59:34Z</updated-at>
+  <uuid>b11312ca-227d-36fd-e3b5-af2f419-a650</uuid>
+  <first-name>Sally</first-name>
+  <last-name>Fried</last-name>
+  <company-id type="integer">1042368</company-id>
+  <user-name/>
+  <email-address>sfried@example.org</email-address>
+  <avatar-url>
+    http://asset3.37img.com/75521bbf128b89b7ec2ab5fe98ad21b4f6ad21e/avatar.png?r=3
+  </avatar-url>
+</person>

oa-oauth/spec/omniauth/strategies/basecamp_spec.rb

@@ -1,7 +1,82 @@
 require File.dirname(__FILE__) + '/../../spec_helper'
 
 describe OmniAuth::Strategies::Basecamp do
-  it 'should exist' do
-    # do nothing
+  
+  def app
+    Rack::Builder.new {
+      use OmniAuth::Test::PhonySession
+      use OmniAuth::Strategies::Basecamp, 'abc', 'def'
+      run lambda { |env| [200, {'Content-Type' => 'text/plain'}, [Rack::Request.new(env).params.key?('auth').to_s]] }
+    }.to_app
+  end
+
+  def session
+    last_request.env['rack.session']
+  end
+  
+  describe '/auth/basecamp without a subdomain' do
+    before do
+      get '/auth/basecamp'
+    end
+    
+    it 'should respond with OK' do
+      last_response.should be_ok
+    end
+    
+    it 'should respond with HTML' do
+      last_response.content_type.should == 'text/html'
+    end
+    
+    it 'should render a subdomain input' do
+      last_response.body.should =~ %r{<input[^>]*subdomain}
+    end
+  end
+  
+  describe 'POST /auth/basecamp with a subdomain' do
+    before do
+      # the middleware doesn't actually care that it's a POST,
+      # but it makes the "redirect_to" calculation down below easier
+      # since the params are passed in the body rather than the URL.
+      post '/auth/basecamp', {OmniAuth::Strategies::Basecamp::BASECAMP_SUBDOMAIN_PARAMETER => 'flugle'}
+    end
+    
+    it 'should redirect to the proper authorize_url' do
+      last_response.should be_redirect
+      redirect_to = CGI.escape(last_request.url + '/callback')
+      last_response.headers['Location'].should == "https://flugle.basecamphq.com/oauth/authorize?client_id=abc&redirect_uri=#{redirect_to}&type=web_server"
+    end
+    
+    it 'should set the basecamp subdomain in the session' do
+      session[:oauth][:basecamp][:subdomain].should == 'flugle'
+    end
+    
+  end
+  
+  describe 'followed by GET /auth/basecamp/callback' do
+    before do
+      stub_request(:post, 'https://flugle.basecamphq.com/oauth/access_token').
+         to_return(:body => %q{{"access_token": "your_token"}})
+      stub_request(:get, 'https://flugle.basecamphq.com/users/me.xml?access_token=your_token').
+         to_return(:body => File.read(File.join(File.dirname(__FILE__), '..', '..', 'fixtures', 'basecamp_200.xml')))
+      get '/auth/basecamp/callback?code=plums', {}, {'rack.session' => {:oauth => {:basecamp => {:subdomain => 'flugle'}}}}
+    end
+  
+    it 'should set the provider to "basecamp"' do
+      last_request['auth']['provider'].should == 'basecamp'
+    end
+  
+    it 'should set the UID to "1827370"' do
+      last_request['auth']['uid'].should == '1827370'
+    end
+  
+    it 'should exchange the request token for an access token' do
+      token = last_request['auth']['extra']['access_token']
+      token.should be_kind_of(OAuth2::AccessToken)
+      token.token.should == 'your_token'
+    end
+  
+    it 'should call through to the master app' do
+      last_response.body.should == 'true'
+    end
   end
 end