From 468ad8112245ac827174aa0dfc283b5f754332ac Mon Sep 17 00:00:00 2001 From: lukemelia Date: Tue, 13 Feb 2007 05:28:51 +0000 Subject: [PATCH] Improve OpenId authentication in cases of delegated OpenID (see http://simonwillison.net/2006/Dec/19/openid/ for background on this). git-svn-id: http://www.rousette.org.uk/svn/tracks-repos/trunk@437 a4c988fc-2ded-0310-b66e-134b36920a42 --- tracks/app/controllers/login_controller.rb | 18 +++++++++----- tracks/app/controllers/users_controller.rb | 16 +++++++------ tracks/config/routes.rb | 2 +- tracks/db/schema.rb | 28 ++++++++++++---------- 4 files changed, 37 insertions(+), 27 deletions(-) diff --git a/tracks/app/controllers/login_controller.rb b/tracks/app/controllers/login_controller.rb index 2fc038f3..caa29828 100644 --- a/tracks/app/controllers/login_controller.rb +++ b/tracks/app/controllers/login_controller.rb @@ -37,25 +37,31 @@ class LoginController < ApplicationController # Let the user know that the URL is unusable. case open_id_response.status when OpenID::SUCCESS + openid_url = params[:openid_url] # The URL was a valid identity URL. Now we just need to send a redirect # to the server using the redirect_url the library created for us. # redirect to the server - redirect_to open_id_response.redirect_url((request.protocol + request.host_with_port + "/"), url_for(:action => 'complete')) + redirect_to open_id_response.redirect_url((request.protocol + request.host_with_port + "/"), url_for(:action => 'complete', :openid_url => openid_url)) else - notify :warning, "Unable to find openid server for #{params[:openid_url]}" + notify :warning, "Unable to find openid server for #{openid_url}" redirect_to :action => 'login' end end def complete + openid_url = params[:openid_url] + if openid_url.blank? + notify :error, "expected an openid_url" + end + case open_id_response.status when OpenID::FAILURE # In the case of failure, if info is non-nil, it is the # URL that we were verifying. We include it in the error # message to help the user figure out what happened. if open_id_response.identity_url - msg = "Verification of #{open_id_response.identity_url} failed. " + msg = "Verification of #{openid_url}(#{open_id_response.identity_url}) failed. " else msg = "Verification failed. " end @@ -65,13 +71,13 @@ class LoginController < ApplicationController # Success means that the transaction completed without # error. If info is nil, it means that the user cancelled # the verification. - @user = User.find_by_open_id_url(open_id_response.identity_url) + @user = User.find_by_open_id_url(openid_url) unless (@user.nil?) - notify :notice, "You have successfully verified #{open_id_response.identity_url} as your identity." + notify :notice, "You have successfully verified #{openid_url} as your identity." session['user_id'] = @user.id redirect_back_or_home else - notify :warning, "You have successfully verified #{open_id_response.identity_url} as your identity, but you do not have a Tracks account. Please ask your administrator to sign you up." + notify :warning, "You have successfully verified #{openid_url} as your identity, but you do not have a Tracks account. Please ask your administrator to sign you up." end when OpenID::CANCEL diff --git a/tracks/app/controllers/users_controller.rb b/tracks/app/controllers/users_controller.rb index e02d265b..66febae3 100644 --- a/tracks/app/controllers/users_controller.rb +++ b/tracks/app/controllers/users_controller.rb @@ -156,11 +156,12 @@ class UsersController < ApplicationController when OpenID::SUCCESS # The URL was a valid identity URL. Now we just need to send a redirect # to the server using the redirect_url the library created for us. + openid_url = params[:openid_url] # redirect to the server - redirect_to open_id_response.redirect_url((request.protocol + request.host_with_port + "/"), url_for(:action => 'complete')) + redirect_to open_id_response.redirect_url((request.protocol + request.host_with_port + "/"), url_for(:action => 'complete', :openid_url => openid_url)) else - notify :warning, "Unable to find openid server for #{params[:openid_url]}" + notify :warning, "Unable to find openid server for #{openid_url}" redirect_to :action => 'change_auth_type' end return @@ -176,13 +177,14 @@ class UsersController < ApplicationController end def complete + openid_url = params[:openid_url] case open_id_response.status when OpenID::FAILURE # In the case of failure, if info is non-nil, it is the # URL that we were verifying. We include it in the error # message to help the user figure out what happened. if open_id_response.identity_url - msg = "Verification of #{open_id_response.identity_url} failed. " + msg = "Verification of #{openid_url}(#{open_id_response.identity_url}) failed. " else msg = "Verification failed. " end @@ -193,13 +195,13 @@ class UsersController < ApplicationController # error. If info is nil, it means that the user cancelled # the verification. @user.auth_type = 'open_id' - @user.open_id_url = open_id_response.identity_url + @user.open_id_url = openid_url if @user.save - notify :notice, "You have successfully verified #{open_id_response.identity_url} as your identity and set your authentication type to Open ID." + notify :notice, "You have successfully verified #{openid_url} as your identity and set your authentication type to Open ID." else - notify :warning, "You have successfully verified #{open_id_response.identity_url} as your identity but there was a problem saving your authentication preferences." + notify :warning, "You have successfully verified #{openid_url} as your identity but there was a problem saving your authentication preferences." end - redirect_to :action => 'preferences' + redirect_to :controller => 'preferences', :action => 'index' when OpenID::CANCEL notify :warning, "Verification cancelled." diff --git a/tracks/config/routes.rb b/tracks/config/routes.rb index a06a3b4d..b801104a 100644 --- a/tracks/config/routes.rb +++ b/tracks/config/routes.rb @@ -11,7 +11,7 @@ ActionController::Routing::Routes.draw do |map| map.resources :users, :member => {:change_password => :get, :update_password => :post, - :change_auth_type => :get, :update_auth_type => :post, + :change_auth_type => :get, :update_auth_type => :post, :complete => :get, :refresh_token => :post } map.with_options :controller => "users" do |users| users.signup 'signup', :action => "new" diff --git a/tracks/db/schema.rb b/tracks/db/schema.rb index a937bdbb..5afc62ac 100644 --- a/tracks/db/schema.rb +++ b/tracks/db/schema.rb @@ -5,14 +5,15 @@ ActiveRecord::Schema.define(:version => 27) do create_table "contexts", :force => true do |t| - t.column "name", :string, :default => "", :null => false - t.column "position", :integer, :default => 0, :null => false - t.column "hide", :boolean, :default => false - t.column "user_id", :integer, :default => 1 + t.column "name", :string, :default => "", :null => false + t.column "hide", :integer, :limit => 4, :default => 0, :null => false + t.column "position", :integer, :default => 0, :null => false + t.column "user_id", :integer, :default => 0, :null => false t.column "created_at", :datetime t.column "updated_at", :datetime end + add_index "contexts", ["user_id"], :name => "index_contexts_on_user_id" add_index "contexts", ["user_id", "name"], :name => "index_contexts_on_user_id_and_name" create_table "notes", :force => true do |t| @@ -63,13 +64,14 @@ ActiveRecord::Schema.define(:version => 27) do create_table "projects", :force => true do |t| t.column "name", :string, :default => "", :null => false t.column "position", :integer, :default => 0, :null => false - t.column "user_id", :integer, :default => 1 + t.column "user_id", :integer, :default => 0, :null => false t.column "description", :text t.column "state", :string, :limit => 20, :default => "active", :null => false t.column "created_at", :datetime t.column "updated_at", :datetime end + add_index "projects", ["user_id"], :name => "index_projects_on_user_id" add_index "projects", ["user_id", "name"], :name => "index_projects_on_user_id_and_name" create_table "sessions", :force => true do |t| @@ -98,16 +100,16 @@ ActiveRecord::Schema.define(:version => 27) do add_index "tags", ["name"], :name => "index_tags_on_name" create_table "todos", :force => true do |t| - t.column "context_id", :integer, :default => 0, :null => false - t.column "project_id", :integer - t.column "description", :string, :default => "", :null => false + t.column "context_id", :integer, :default => 0, :null => false + t.column "description", :string, :limit => 100, :default => "", :null => false t.column "notes", :text t.column "created_at", :datetime t.column "due", :date t.column "completed_at", :datetime - t.column "user_id", :integer, :default => 1 + t.column "project_id", :integer + t.column "user_id", :integer, :default => 0, :null => false t.column "show_from", :date - t.column "state", :string, :limit => 20, :default => "immediate", :null => false + t.column "state", :string, :limit => 20, :default => "immediate", :null => false end add_index "todos", ["user_id", "state"], :name => "index_todos_on_user_id_and_state" @@ -117,10 +119,10 @@ ActiveRecord::Schema.define(:version => 27) do add_index "todos", ["user_id", "context_id"], :name => "index_todos_on_user_id_and_context_id" create_table "users", :force => true do |t| - t.column "login", :string, :limit => 80, :default => "", :null => false - t.column "password", :string, :limit => 40, :default => "", :null => false + t.column "login", :string, :limit => 80 + t.column "password", :string, :limit => 40 t.column "word", :string - t.column "is_admin", :boolean, :default => false, :null => false + t.column "is_admin", :integer, :limit => 4, :default => 0, :null => false t.column "first_name", :string t.column "last_name", :string t.column "auth_type", :string, :default => "database", :null => false