From: Tom Hughes <tom@compton.nu>
Date: Wed, 16 Nov 2011 21:06:47 +0000 (+0000)
Subject: Make OAuth work again
X-Git-Tag: live~6612
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/7b89dc63496de0b6131a93c57d283ebb4490804b

Make OAuth work again
---

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index e57638767..44307d482 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -366,6 +366,11 @@ private
     return [user, pass] 
   end 
 
+  # used by oauth plugin to get the current user
+  def current_user
+    @user
+  end
+
   # used by oauth plugin to set the current user
   def current_user=(user)
     @user=user
diff --git a/app/controllers/oauth_controller.rb b/app/controllers/oauth_controller.rb
index 5c84be0cf..3c56a4bf4 100644
--- a/app/controllers/oauth_controller.rb
+++ b/app/controllers/oauth_controller.rb
@@ -1,83 +1,33 @@
+require 'oauth/controllers/provider_controller'
+
 class OauthController < ApplicationController
+  include OAuth::Controllers::ProviderController
+
   layout 'slim'
 
-  before_filter :authorize_web, :only => [:oauthorize, :revoke]
-  before_filter :set_locale, :only => [:oauthorize, :revoke]
-  before_filter :require_user, :only => [:oauthorize]
-  before_filter :verify_oauth_consumer_signature, :only => [:request_token]
-  before_filter :verify_oauth_request_token, :only => [:access_token]
-  # Uncomment the following if you are using restful_open_id_authentication
-  # skip_before_filter :verify_authenticity_token
+  def login_required
+    authorize_web
+    set_locale
+    require_user
+  end
 
-  def request_token
-    @token = current_client_application.create_request_token
+  def user_authorizes_token?
+    any_auth = false
 
-    if @token
-      logger.info "request token params: #{params.inspect}"
-      # request tokens indicate what permissions the client *wants*, not
-      # necessarily the same as those which the user allows.
-      current_client_application.permissions.each do |pref|
+    @token.client_application.permissions.each do |pref|
+      if params[pref]
         @token.write_attribute(pref, true)
+        any_auth ||= true
+      else
+        @token.write_attribute(pref, false)
       end
-      @token.save!
-
-      render :text => @token.to_query
-    else
-      render :nothing => true, :status => 401
     end
-  end
 
-  def access_token
-    @token = current_token && current_token.exchange!
-    if @token
-      render :text => @token.to_query
-    else
-      render :nothing => true, :status => 401
-    end
-  end
-
-  def oauthorize
-    @token = RequestToken.find_by_token params[:oauth_token]
-    unless @token.nil? or @token.invalidated? 
-      if request.post?
-        any_auth = false
-        @token.client_application.permissions.each do |pref|
-          if params[pref]
-            @token.write_attribute(pref, true)
-            any_auth ||= true
-          else
-            @token.write_attribute(pref, false)
-          end
-        end
-
-        if any_auth
-          @token.authorize!(@user)
-          if @token.oauth10?
-            redirect_url = params[:oauth_callback] || @token.client_application.callback_url
-          else
-            redirect_url = @token.oob? ? @token.client_application.callback_url : @token.callback_url
-          end
-          if redirect_url and not redirect_url.empty?
-            if @token.oauth10?
-              redirect_to "#{redirect_url}?oauth_token=#{@token.token}"
-            else
-              redirect_to "#{redirect_url}?oauth_token=#{@token.token}&oauth_verifier=#{@token.verifier}"
-            end
-          else
-            render :action => "authorize_success"
-          end
-        else
-          @token.invalidate!
-          render :action => "authorize_failure"
-        end
-      end
-    else
-      render :action => "authorize_failure"
-    end
+    any_auth
   end
 
   def revoke
-    @token = @user.oauth_tokens.find_by_token params[:token]
+    @token = current_user.oauth_tokens.find_by_token params[:token]
     if @token
       @token.invalidate!
       flash[:notice] = t('oauth.revoke.flash', :application => @token.client_application.name)
diff --git a/app/models/access_token.rb b/app/models/access_token.rb
index 1f172c5f0..a1888343a 100644
--- a/app/models/access_token.rb
+++ b/app/models/access_token.rb
@@ -4,7 +4,7 @@ class AccessToken < OauthToken
 
   scope :valid, where(:invalidated_at => nil)
 
-  validates_presence_of :user
+  validates_presence_of :user, :secret
 
   before_create :set_authorized_at
   
diff --git a/app/models/client_application.rb b/app/models/client_application.rb
index 04f1c0c99..55f34e6e2 100644
--- a/app/models/client_application.rb
+++ b/app/models/client_application.rb
@@ -4,6 +4,8 @@ class ClientApplication < ActiveRecord::Base
   belongs_to :user
   has_many :tokens, :class_name => "OauthToken"
   has_many :access_tokens
+  has_many :oauth2_verifiers
+  has_many :oauth_tokens
 
   validates_presence_of :name, :url, :key, :secret
   validates_uniqueness_of :key
@@ -27,15 +29,10 @@ class ClientApplication < ActiveRecord::Base
   def self.verify_request(request, options = {}, &block)
     begin
       signature = OAuth::Signature.build(request, options, &block)
-      logger.info "Signature Base String: #{signature.signature_base_string}"
-      logger.info "Consumer: #{signature.send :consumer_key}"
-      logger.info "Token: #{signature.send :token}"
       return false unless OauthNonce.remember(signature.request.nonce, signature.request.timestamp)
       value = signature.verify
-      logger.info "Signature verification returned: #{value.to_s}"
       value
     rescue OAuth::Signature::UnknownSignatureMethod => e
-      logger.info "ERROR"+e.to_s
       false
     end
   end
@@ -52,8 +49,12 @@ class ClientApplication < ActiveRecord::Base
     @oauth_client ||= OAuth::Consumer.new(key, secret)
   end
     
-  def create_request_token
-    RequestToken.create :client_application => self, :callback_url => self.token_callback_url
+  def create_request_token(params={})
+    params = { :client_application => self, :callback_url => self.token_callback_url }
+    permissions.each do |p|
+      params[p] = true
+    end
+    RequestToken.create(params)
   end
 
   def access_token_for_user(user)
@@ -84,8 +85,7 @@ protected
                  :allow_write_api, :allow_read_gpx, :allow_write_gpx ]
 
   def generate_keys
-    oauth_client = oauth_server.generate_consumer_credentials
-    self.key = oauth_client.key
-    self.secret = oauth_client.secret
+    self.key = OAuth::Helper.generate_key(40)[0,40]
+    self.secret = OAuth::Helper.generate_key(40)[0,40]
   end
 end
diff --git a/app/models/oauth2_token.rb b/app/models/oauth2_token.rb
new file mode 100644
index 000000000..9c28d886e
--- /dev/null
+++ b/app/models/oauth2_token.rb
@@ -0,0 +1,21 @@
+class Oauth2Token < AccessToken
+  attr_accessor :state
+
+  def as_json(options={})
+    d = {:access_token=>token, :token_type => 'bearer'}
+    d[:expires_in] = expires_in if expires_at
+    d
+  end
+
+  def to_query
+    q = "access_token=#{token}&token_type=bearer"
+    q << "&state=#{URI.escape(state)}" if @state
+    q << "&expires_in=#{expires_in}" if expires_at
+    q << "&scope=#{URI.escape(scope)}" if scope
+    q
+  end
+
+  def expires_in
+    expires_at.to_i - Time.now.to_i
+  end
+end
diff --git a/app/models/oauth2_verifier.rb b/app/models/oauth2_verifier.rb
new file mode 100644
index 000000000..94856d0d8
--- /dev/null
+++ b/app/models/oauth2_verifier.rb
@@ -0,0 +1,34 @@
+class Oauth2Verifier < OauthToken
+  validates_presence_of :user
+  attr_accessor :state
+
+  def exchange!(params={})
+    OauthToken.transaction do
+      token = Oauth2Token.create! :user=>user,:client_application=>client_application, :scope => scope
+      invalidate!
+      token
+    end
+  end
+
+  def code
+    token
+  end
+
+  def redirect_url
+    callback_url
+  end
+
+  def to_query
+    q = "code=#{token}"
+    q << "&state=#{URI.escape(state)}" if @state
+    q
+  end
+
+  protected
+
+  def generate_keys
+    self.token = OAuth::Helper.generate_key(20)[0,20]
+    self.expires_at = 10.minutes.from_now
+    self.authorized_at = Time.now
+  end
+end
diff --git a/app/models/oauth_token.rb b/app/models/oauth_token.rb
index 376ad7644..b38fe0ec0 100644
--- a/app/models/oauth_token.rb
+++ b/app/models/oauth_token.rb
@@ -5,20 +5,10 @@ class OauthToken < ActiveRecord::Base
   scope :authorized, where("authorized_at IS NOT NULL and invalidated_at IS NULL")
 
   validates_uniqueness_of :token
-  validates_presence_of :client_application, :token, :secret
+  validates_presence_of :client_application, :token
 
   before_validation :generate_keys, :on => :create
   
-  def self.find_token(token_key)
-    token = OauthToken.find_by_token(token_key, :include => :client_application)
-    if token && token.authorized?
-      logger.info "Loaded #{token.token} which was authorized by (user_id=#{token.user_id}) on the #{token.authorized_at}"
-      token
-    else
-      nil
-    end
-  end
-  
   def invalidated?
     invalidated_at != nil
   end
@@ -38,8 +28,7 @@ class OauthToken < ActiveRecord::Base
 protected
   
   def generate_keys
-    @oauth_token = client_application.oauth_server.generate_credentials
-    self.token = @oauth_token[0]
-    self.secret = @oauth_token[1]
+    self.token = OAuth::Helper.generate_key(40)[0,40]
+    self.secret = OAuth::Helper.generate_key(40)[0,40]
   end
 end
diff --git a/app/models/request_token.rb b/app/models/request_token.rb
index 0044dde26..1ac502bc7 100644
--- a/app/models/request_token.rb
+++ b/app/models/request_token.rb
@@ -6,7 +6,7 @@ class RequestToken < OauthToken
     return false if authorized?
     self.user = user
     self.authorized_at = Time.now
-    self.verifier = OAuth::Helper.generate_key(16)[0,20] unless oauth10?
+    self.verifier = OAuth::Helper.generate_key(20)[0,20] unless oauth10?
     self.save
   end
 
@@ -36,7 +36,7 @@ class RequestToken < OauthToken
   end
 
   def oob?
-    self.callback_url=='oob'
+    callback_url.nil? || callback_url.downcase == 'oob'
   end
 
   def oauth10?
diff --git a/app/views/oauth/oauthorize.html.erb b/app/views/oauth/authorize.html.erb
similarity index 68%
rename from app/views/oauth/oauthorize.html.erb
rename to app/views/oauth/authorize.html.erb
index ffb403be1..eaf43f9d1 100644
--- a/app/views/oauth/oauthorize.html.erb
+++ b/app/views/oauth/authorize.html.erb
@@ -1,5 +1,5 @@
 <h1>Authorize access to your account</h1>
-<p><%= t('oauth.oauthorize.request_access', :app_name => link_to(@token.client_application.name, @token.client_application.url), :user => link_to(@user.display_name, :controller => :user, :action => :view, :display_name => @user.display_name)) %></p>
+<p><%= raw t('oauth.oauthorize.request_access', :app_name => link_to(@token.client_application.name, @token.client_application.url), :user => link_to(@user.display_name, :controller => :user, :action => :view, :display_name => @user.display_name)) %></p>
 <%= form_tag authorize_url do %>
   <%= hidden_field_tag "oauth_token", @token.token %>
   <%- if params[:oauth_callback] -%>
diff --git a/app/views/oauth/authorize_success.html.erb b/app/views/oauth/authorize_success.html.erb
index a25b98ec7..89512db31 100644
--- a/app/views/oauth/authorize_success.html.erb
+++ b/app/views/oauth/authorize_success.html.erb
@@ -1,5 +1,5 @@
 <h1>You have allowed this request</h1>
 
-<% if @token.oob? %>
+<% if @token.oob? and not @token.oauth10? %>
 <p>The verification code is <%= @token.verifier %></p>
 <% end %>
diff --git a/config/routes.rb b/config/routes.rb
index 471f67e73..1916ae74a 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -213,7 +213,8 @@ OpenStreetMap::Application.routes.draw do
     resources :oauth_clients
   end
   match '/oauth/revoke' => 'oauth#revoke'
-  match '/oauth/authorize' => 'oauth#oauthorize', :as => :authorize
+  match '/oauth/authorize' => 'oauth#authorize', :as => :authorize
+  match '/oauth/token' => 'oauth#token', :as => :token
   match '/oauth/request_token' => 'oauth#request_token', :as => :request_token
   match '/oauth/access_token' => 'oauth#access_token', :as => :access_token
   match '/oauth/test_request' => 'oauth#test_request', :as => :test_request
diff --git a/db/migrate/20111116184519_update_oauth.rb b/db/migrate/20111116184519_update_oauth.rb
new file mode 100644
index 000000000..59ab9c39b
--- /dev/null
+++ b/db/migrate/20111116184519_update_oauth.rb
@@ -0,0 +1,11 @@
+class UpdateOauth < ActiveRecord::Migration
+  def up
+    add_column :oauth_tokens, :scope, :string
+    add_column :oauth_tokens, :valid_to, :timestamp
+  end
+
+  def down
+    remove_column :oauth_tokens, :valid_to
+    remove_column :oauth_tokens, :scope
+  end
+end
diff --git a/test/unit/oauth_token_test.rb b/test/unit/oauth_token_test.rb
index 655e64eda..eb8219c57 100644
--- a/test/unit/oauth_token_test.rb
+++ b/test/unit/oauth_token_test.rb
@@ -23,15 +23,4 @@ class OauthTokenTest < ActiveSupport::TestCase
     assert_equal false, tok.authorized?, "Token should now be invalid."
   end
 
-  ##
-  # test that tokens can't be found unless they're authorised
-  def test_find_token
-    tok = client_applications(:oauth_web_app).create_request_token
-    assert_equal false, tok.authorized?, "Token should be created unauthorised."
-    assert_equal nil, OauthToken.find_token(tok.token), "Shouldn't be able to find unauthorised token"
-    tok.authorize!(users(:public_user))
-    assert_equal true, tok.authorized?, "Token should now be authorised."
-    assert_not_equal nil, OauthToken.find_token(tok.token), "Should be able to find authorised token"
-  end
-
 end