From e434cb154c5ccecd7fc0ce672e4de79bd9e7e06f Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Sun, 22 Feb 2015 12:20:04 +0000 Subject: [PATCH] Convert OpenID authentication to generic third party authentication --- app/assets/javascripts/user.js | 118 ++++++++++++------ app/assets/stylesheets/common.scss | 4 +- app/controllers/user_controller.rb | 97 ++++++++------ app/helpers/user_helper.rb | 12 +- app/models/user.rb | 4 +- app/views/user/account.html.erb | 12 +- app/views/user/login.html.erb | 36 +++--- app/views/user/new.html.erb | 51 +++----- config/locales/en.yml | 18 +-- .../20150222101847_rename_openid_url.rb | 11 ++ db/structure.sql | 48 +++++-- test/controllers/user_controller_test.rb | 3 +- test/fixtures/users.yml | 3 +- test/integration/user_creation_test.rb | 10 +- 14 files changed, 253 insertions(+), 174 deletions(-) create mode 100644 db/migrate/20150222101847_rename_openid_url.rb diff --git a/app/assets/javascripts/user.js b/app/assets/javascripts/user.js index 22c2ad829..e8ad3324d 100644 --- a/app/assets/javascripts/user.js +++ b/app/assets/javascripts/user.js @@ -1,59 +1,95 @@ //= require leaflet.locate $(document).ready(function () { - var map = L.map("map", { - attributionControl: false, - zoomControl: false - }).addLayer(new L.OSM.Mapnik()); + if ($("#map").length) { + var map = L.map("map", { + attributionControl: false, + zoomControl: false + }).addLayer(new L.OSM.Mapnik()); - var position = $('html').attr('dir') === 'rtl' ? 'topleft' : 'topright'; + var position = $('html').attr('dir') === 'rtl' ? 'topleft' : 'topright'; - L.OSM.zoom({position: position}) - .addTo(map); + L.OSM.zoom({position: position}) + .addTo(map); - L.control.locate({ - position: position, - strings: { - title: I18n.t('javascripts.map.locate.title'), - popup: I18n.t('javascripts.map.locate.popup') + L.control.locate({ + position: position, + strings: { + title: I18n.t('javascripts.map.locate.title'), + popup: I18n.t('javascripts.map.locate.popup') + } + }).addTo(map); + + if (OSM.home) { + map.setView([OSM.home.lat, OSM.home.lon], 12); + } else { + map.setView([0, 0], 0); } - }).addTo(map); - if (OSM.home) { - map.setView([OSM.home.lat, OSM.home.lon], 12); - } else { - map.setView([0, 0], 0); + if ($("#map").hasClass("set_location")) { + var marker = L.marker([0, 0], {icon: OSM.getUserIcon()}); + + if (OSM.home) { + marker.setLatLng([OSM.home.lat, OSM.home.lon]); + marker.addTo(map); + } + + map.on("click", function (e) { + if ($('#updatehome').is(':checked')) { + var zoom = map.getZoom(), + precision = OSM.zoomPrecision(zoom), + location = e.latlng.wrap(); + + $('#homerow').removeClass(); + $('#home_lat').val(location.lat.toFixed(precision)); + $('#home_lon').val(location.lng.toFixed(precision)); + + marker.setLatLng(e.latlng); + marker.addTo(map); + } + }); + } else { + $("[data-user]").each(function () { + var user = $(this).data('user'); + if (user.lon && user.lat) { + L.marker([user.lat, user.lon], {icon: OSM.getUserIcon(user.icon)}).addTo(map) + .bindPopup(user.description); + } + }); + } } - if ($("#map").hasClass("set_location")) { - var marker = L.marker([0, 0], {icon: OSM.getUserIcon()}); + function updateAuthUID() { + var provider = $("select#user_auth_provider").val(); - if (OSM.home) { - marker.setLatLng([OSM.home.lat, OSM.home.lon]); - marker.addTo(map); + if (provider === "openid") { + $("input#user_auth_uid").show().prop("disabled", false); + } else { + $("input#user_auth_uid").hide().prop("disabled", true); } + } - map.on("click", function (e) { - if ($('#updatehome').is(':checked')) { - var zoom = map.getZoom(), - precision = OSM.zoomPrecision(zoom), - location = e.latlng.wrap(); + updateAuthUID(); - $('#homerow').removeClass(); - $('#home_lat').val(location.lat.toFixed(precision)); - $('#home_lon').val(location.lng.toFixed(precision)); + $("select#user_auth_provider").on("change", updateAuthUID); - marker.setLatLng(e.latlng); - marker.addTo(map); - } - }); + function enableAuth() { + $("#auth_prompt").hide(); + $("#auth_field").show(); + $("#user_auth_uid").prop("disabled", false); + } + + function disableAuth() { + $("#auth_prompt").show(); + $("#auth_field").hide(); + $("#user_auth_uid").prop("disabled", true); + } + + $("#auth_enable").click(enableAuth); + + if ($("select#user_auth_provider").val() === "") { + disableAuth(); } else { - $("[data-user]").each(function () { - var user = $(this).data('user'); - if (user.lon && user.lat) { - L.marker([user.lat, user.lon], {icon: OSM.getUserIcon(user.icon)}).addTo(map) - .bindPopup(user.description); - } - }); + enableAuth(); } }); diff --git a/app/assets/stylesheets/common.scss b/app/assets/stylesheets/common.scss index cbc721b3f..b4ce35d1d 100644 --- a/app/assets/stylesheets/common.scss +++ b/app/assets/stylesheets/common.scss @@ -1681,11 +1681,11 @@ tr.turn:hover { /* Rules for the log in page */ -#login_openid_buttons { +#login_auth_buttons { margin-bottom: 0; } -#login_openid_buttons li { +#login_auth_buttons li { float: left; padding: $lineheight/4 $lineheight/2; } diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index b1d6c86f7..a62e58b0e 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -80,7 +80,11 @@ class UserController < ApplicationController @user.languages = http_accept_language.user_preferred_languages @user.terms_agreed = Time.now.getutc @user.terms_seen = true - @user.openid_url = nil if @user.openid_url && @user.openid_url.empty? + + if @user.auth_uid.nil? || @user.auth_uid.empty? + @user.auth_provider = nil + @user.auth_uid = nil + end if @user.save flash[:piwik_goal] = PIWIK["goals"]["signup"] if defined?(PIWIK) @@ -119,17 +123,13 @@ class UserController < ApplicationController @tokens = @user.oauth_tokens.authorized if params[:user] && params[:user][:display_name] && params[:user][:description] - if params[:user][:openid_url] && - params[:user][:openid_url].length > 0 && - params[:user][:openid_url] != @user.openid_url - # If the OpenID has changed, we want to check that it is a - # valid OpenID and one the user has control over before saving - # it as a password equivalent for the user. - session[:new_user_settings] = params - openid_url = openid_expand_url(params[:user][:openid_url]) - redirect_to auth_path(:provider => "openid", :openid_url => openid_url, :origin => request.path) - else + if params[:user][:auth_provider].blank? || + (params[:user][:auth_provider] == @user.auth_provider && + params[:user][:auth_uid] == @user.auth_uid) update_user(@user, params) + else + session[:new_user_settings] = params + redirect_to auth_url(params[:user][:auth_provider], params[:user][:auth_uid]) end end end @@ -206,13 +206,14 @@ class UserController < ApplicationController else redirect_to :controller => "site", :action => "index" end - elsif params.key?(:openid) + elsif params.key?(:auth_provider) && params.key?(:auth_uid) @user = User.new(:email => params[:email], :email_confirmation => params[:email], :display_name => params[:nickname], - :openid_url => params[:openid]) + :auth_provider => params[:auth_provider], + :auth_uid => params[:auth_uid]) - flash.now[:notice] = t "user.new.openid association" + flash.now[:notice] = t "user.new.auth association" else check_signup_allowed end @@ -226,9 +227,9 @@ class UserController < ApplicationController @user.status = "pending" - if @user.openid_url.present? && @user.pass_crypt.empty? - # We are creating an account with OpenID and no password - # was specified so create a random one + if @user.auth_provider.present? && @user.auth_uid.present? && @user.pass_crypt.empty? + # We are creating an account with external authentication and + # no password was specified so create a random one @user.pass_crypt = SecureRandom.base64(16) @user.pass_crypt_confirmation = @user.pass_crypt end @@ -236,11 +237,10 @@ class UserController < ApplicationController if @user.invalid? # Something is wrong with a new user, so rerender the form render :action => "new" - elsif @user.openid_url.present? - # Verify OpenID before moving on + elsif @user.auth_provider.present? && @user.auth_uid.present? + # Verify external authenticator before moving on session[:new_user] = @user - openid_url = openid_expand_url(@user.openid_url) - redirect_to auth_path(:provider => "openid", :openid_url => openid_url, :origin => request.path) + redirect_to auth_url(@user.auth_provider, @user.auth_uid) else # Save the user record session[:new_user] = @user @@ -255,8 +255,7 @@ class UserController < ApplicationController if params[:openid_url].present? session[:remember_me] ||= params[:remember_me_openid] - openid_url = openid_expand_url(params[:openid_url]) - redirect_to auth_path(:provider => "openid", :openid_url => openid_url, :origin => request.path) + redirect_to auth_url("openid", params[:openid_url]) else session[:remember_me] ||= params[:remember_me] password_authentication(params[:username], params[:password]) @@ -482,11 +481,20 @@ class UserController < ApplicationController def auth_success auth_info = env["omniauth.auth"] - openid_url = auth_info[:uid] + provider = auth_info[:provider] + uid = auth_info[:uid] name = auth_info[:info][:name] email = auth_info[:info][:email] - if user = User.find_by_openid_url(openid_url) + case provider + when "openid" + email_verified = uid.match(%r{https://www.google.com/accounts/o8/id?(.*)}) || + uid.match(%r{https://me.yahoo.com/(.*)}) + else + email_verified = false + end + + if user = User.find_by_auth_provider_and_auth_uid(provider, uid) case user.status when "pending" then unconfirmed_login(user) @@ -498,21 +506,24 @@ class UserController < ApplicationController failed_login t("user.login.auth failure") end elsif settings = session.delete(:new_user_settings) - @user.openid_url = openid_url + @user.auth_provider = provider + @user.auth_uid = uid update_user(@user, settings) redirect_to :action => "account", :display_name => @user.display_name elsif session[:new_user] - session[:new_user].openid_url = openid_url + session[:new_user].auth_provider = provider + session[:new_user].auth_uid = uid - if email == session[:new_user].email && openid_email_verified(email) + if email_verified && email == session[:new_user].email session[:new_user].status = "active" end redirect_to :action => "terms" else - redirect_to :action => "new", :nickname => name, :email => email, :openid => openid_url + redirect_to :action => "new", :nickname => name, :email => email, + :auth_provider => provider, :auth_uid => uid end end @@ -539,6 +550,16 @@ class UserController < ApplicationController end end + ## + # return the URL to use for authentication + def auth_url(provider, uid) + if provider == "openid" + auth_path(:provider => "openid", :openid_url => openid_expand_url(uid), :origin => request.path) + else + auth_path(:provider => provider, :origin => request.path) + end + end + ## # special case some common OpenID providers by applying heuristics to # try and come up with the correct URL based on what the user entered @@ -556,14 +577,6 @@ class UserController < ApplicationController end end - ## - # check if we trust an OpenID provider to return a verified - # email, so that we can skpi verifying it ourselves - def openid_email_verified(openid_url) - openid_url.match(%r{https://www.google.com/accounts/o8/id?(.*)}) || - openid_url.match(%r{https://me.yahoo.com/(.*)}) - end - ## # process a successful login def successful_login(user) @@ -649,7 +662,11 @@ class UserController < ApplicationController user.preferred_editor = params[:user][:preferred_editor] end - user.openid_url = nil if params[:user][:openid_url].blank? + if params[:user][:auth_provider].nil? || params[:user][:auth_provider].blank? || + params[:user][:auth_uid].nil? || params[:user][:auth_uid].blank? + user.auth_provider = nil + user.auth_uid = nil + end if user.save set_locale @@ -728,7 +745,9 @@ class UserController < ApplicationController ## # return permitted user parameters def user_params - params.require(:user).permit(:email, :email_confirmation, :display_name, :openid_url, :pass_crypt, :pass_crypt_confirmation) + params.require(:user).permit(:email, :email_confirmation, :display_name, + :auth_provider, :auth_uid, + :pass_crypt, :pass_crypt_confirmation) end ## diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb index d48837166..dd4c343d8 100644 --- a/app/helpers/user_helper.rb +++ b/app/helpers/user_helper.rb @@ -42,18 +42,18 @@ module UserHelper end end - # OpenID support + # External authentication support def openid_logo image_tag "openid_small.png", :alt => t("user.login.openid_logo_alt"), :class => "openid_logo" end - def openid_button(name, url) + def auth_button(name, provider, options) link_to( - image_tag("#{name}.png", :alt => t("user.login.openid_providers.#{name}.alt")), - auth_path(:provider => "openid", :openid_url => url), - :class => "openid_button", - :title => t("user.login.openid_providers.#{name}.title") + image_tag("#{name}.png", :alt => t("user.login.auth_providers.#{name}.alt")), + auth_path(options.merge(:provider => provider)), + :class => "auth_button", + :title => t("user.login.auth_providers.#{name}.title") ) end diff --git a/app/models/user.rb b/app/models/user.rb index e6a11b6bd..ac293a0bd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,7 +39,6 @@ class User < ActiveRecord::Base validates_confirmation_of :pass_crypt # , :message => ' must match the confirmation password' validates_uniqueness_of :display_name, :allow_nil => true, :case_sensitive => false, :if => proc { |u| u.display_name_changed? } validates_uniqueness_of :email, :case_sensitive => false, :if => proc { |u| u.email_changed? } - validates_uniqueness_of :openid_url, :allow_nil => true validates_length_of :pass_crypt, :within => 8..255 validates_length_of :display_name, :within => 3..255, :allow_nil => true validates_email_format_of :email, :if => proc { |u| u.email_changed? } @@ -199,7 +198,8 @@ class User < ActiveRecord::Base self.image = nil self.email_valid = false self.new_email = nil - self.openid_url = nil + self.auth_provider = nil + self.auth_uid = nil self.status = "deleted" save end diff --git a/app/views/user/account.html.erb b/app/views/user/account.html.erb index 1cd05aba6..93ee79cb6 100644 --- a/app/views/user/account.html.erb +++ b/app/views/user/account.html.erb @@ -1,3 +1,7 @@ +<% content_for :head do %> + <%= javascript_include_tag "user" %> +<% end %> + <% content_for :heading do %>

<%= t 'user.account.my settings' %>