From b7b68aee36b19700aa95a3ea613bede6a841f7ee Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Tue, 30 Oct 2012 18:53:58 +0000 Subject: [PATCH] Simplify handling of verified emails in OpenID signup Also make sure that all paths that lead to OpenID based signup will notice a verified email properly. --- app/controllers/user_controller.rb | 105 +++++++++++++----------- app/views/user/terms.html.erb | 8 -- config/locales/en.yml | 1 + test/functional/user_controller_test.rb | 40 +++++++-- test/integration/user_creation_test.rb | 18 +++- 5 files changed, 106 insertions(+), 66 deletions(-) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 3fd136554..321b61918 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -32,12 +32,14 @@ class UserController < ApplicationController # the open_id_authentication function @user = session.delete(:new_user) - openid_verify(nil, @user) do |user| + openid_verify(nil, @user) do |user,verified_email| + user.status = "active" if user.email == verified_email end if @user.openid_url.nil? or @user.invalid? render :action => 'new' else + session[:new_user] = @user render :action => 'terms' end elsif params[:user] and Acl.no_account_creation(request.remote_ip, params[:user][:email].split("@").last) @@ -56,6 +58,8 @@ class UserController < ApplicationController end if @user + @user.status = "pending" + if @user.invalid? if @user.new_record? # Something is wrong with a new user, so rerender the form @@ -72,6 +76,9 @@ class UserController < ApplicationController # Verify OpenID before moving on session[:new_user] = @user openid_verify(params[:user][:openid_url], @user) + elsif @user.new_record? + # Save the user record + session[:new_user] = @user end else # Not logged in, so redirect to the login page @@ -114,42 +121,40 @@ class UserController < ApplicationController else redirect_to :action => :account, :display_name => @user.display_name end - elsif Acl.no_account_creation(request.remote_ip, params[:user][:email].split("@").last) - render :action => 'blocked' else - @user = User.new(params[:user]) - - @user.status = "pending" - @user.data_public = true - @user.description = "" if @user.description.nil? - @user.creation_ip = request.remote_ip - @user.languages = request.user_preferred_languages - @user.terms_agreed = Time.now.getutc - @user.terms_seen = true - @user.openid_url = nil if @user.openid_url and @user.openid_url.empty? - - if (session[:openid_verified]) - openid_verified = session.delete(:openid_verified) - if (openid_verified[:identity_url]) and (openid_verified[:identity_url] == @user.openid_url) and (openid_verified[:email]) and (openid_verified[:email] == @user.email) - # if we have an email from an OpenID provider that we trust to have verified the email for us, then activate the account directly - # without doing our own email verification. - @user.status = "active" - end - end + @user = session.delete(:new_user) + + if Acl.no_account_creation(request.remote_ip, @user.email.split("@").last) + render :action => 'blocked' + else + @user.data_public = true + @user.description = "" if @user.description.nil? + @user.creation_ip = request.remote_ip + @user.languages = request.user_preferred_languages + @user.terms_agreed = Time.now.getutc + @user.terms_seen = true + @user.openid_url = nil if @user.openid_url and @user.openid_url.empty? + + if @user.save + flash[:piwik_goal] = PIWIK_SIGNUP_GOAL if defined?(PIWIK_SIGNUP_GOAL) + + if @user.status == "active" + flash[:notice] = t 'user.new.flash welcome', :email => @user.email - if @user.save - flash[:piwik_goal] = PIWIK_SIGNUP_GOAL if defined?(PIWIK_SIGNUP_GOAL) - flash[:notice] = t 'user.new.flash create success message', :email => @user.email - if @user.status == "active" - Notifier.signup_confirm(@user, nil).deliver - successful_login(@user) + Notifier.signup_confirm(@user, nil).deliver + + successful_login(@user) + else + flash[:notice] = t 'user.new.flash create success message', :email => @user.email + session[:token] = @user.tokens.create.token + + Notifier.signup_confirm(@user, @user.tokens.create(:referer => session.delete(:referer))).deliver + + redirect_to :action => 'login', :referer => params[:referer] + end else - Notifier.signup_confirm(@user, @user.tokens.create(:referer => session.delete(:referer))).deliver - session[:token] = @user.tokens.create.token - redirect_to :action => 'login', :referer => params[:referer] + render :action => 'new', :referer => params[:referer] end - else - render :action => 'new', :referer => params[:referer] end end end @@ -566,8 +571,6 @@ private nickname = sreg["nickname"] || ax["http://axschema.org/namePerson/friendly"].first email = sreg["email"] || ax["http://axschema.org/contact/email"].first - # Check if the openID is from a "trusted" OpenID provider and thus provides a verified email address - session[:openid_verified] = openid_email_verified(identity_url, email) redirect_to :controller => 'user', :action => 'new', :nickname => nickname, :email => email, :openid => identity_url end elsif result.missing? @@ -585,8 +588,18 @@ private def openid_verify(openid_url, user) user.openid_url = openid_url - authenticate_with_open_id(openid_expand_url(openid_url), :method => :get) do |result, identity_url| + authenticate_with_open_id(openid_expand_url(openid_url), :method => :get, :required => [:email, "http://axschema.org/contact/email"]) do |result, identity_url, sreg, ax| if result.successful? + # Do we trust the emails this provider returns? + if openid_email_verified(identity_url) + # Guard against not getting any extension data + sreg = Hash.new if sreg.nil? + ax = Hash.new if ax.nil? + + # Get the verified email + verified_email = sreg["email"] || ax["http://axschema.org/contact/email"].first + end + # We need to use the openid url passed back from the OpenID provider # rather than the one supplied by the user, as these can be different. # @@ -594,7 +607,7 @@ private # than a user specific url. Only once it comes back from the provider # provider do we know the unique address for the user. user.openid_url = identity_url - yield user + yield user, verified_email elsif result.missing? flash.now[:error] = t 'user.login.openid missing provider' elsif result.invalid? @@ -622,18 +635,12 @@ private end end - def openid_email_verified(openid_url, email) - # OpenID providers Google and Yahoo are guaranteed to return (if at all) an email address that has been verified by - # them already. So we can trust the email addresses to be valid and own by the user without having to verify them our - # selves. - # Store the email in the session to compare agains the user set email address during account creation. - openid_verified = Hash.new - openid_verified[:identity_url] = openid_url - if openid_url.match(/https:\/\/www.google.com\/accounts\/o8\/id?(.*)/) or openid_url.match(/https:\/\/me.yahoo.com\/(.*)/) - openid_verified[:email] = email - end - return openid_verified - + ## + # 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(/https:\/\/www.google.com\/accounts\/o8\/id?(.*)/) or + openid_url.match(/https:\/\/me.yahoo.com\/(.*)/) end ## diff --git a/app/views/user/terms.html.erb b/app/views/user/terms.html.erb index 849005274..f006fde77 100644 --- a/app/views/user/terms.html.erb +++ b/app/views/user/terms.html.erb @@ -33,14 +33,6 @@

<%= hidden_field_tag('referer', h(params[:referer])) unless params[:referer].nil? %> - <% if @user.new_record? %> - <%= hidden_field('user', 'email') %> - <%= hidden_field('user', 'email_confirmation') %> - <%= hidden_field('user', 'display_name') %> - <%= hidden_field('user', 'pass_crypt') %> - <%= hidden_field('user', 'pass_crypt_confirmation') %> - <%= hidden_field('user', 'openid_url') %> - <% end %>

<%= submit_tag(t('user.terms.decline'), :name => "decline", :id => "decline") %> <%= submit_tag(t('user.terms.agree'), :name => "agree", :id => "agree") %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 07b9a9d4e..9f6ea5ef5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1660,6 +1660,7 @@ en: continue: Continue + flash welcome: "Thanks for signing up. We've sent a welcome message to %{email} with some hints on getting started." flash create success message: "Thanks for signing up. We've sent a confirmation note to %{email} and as soon as you confirm your account you'll be able to get mapping.

If you use an antispam system which sends confirmation requests then please make sure you whitelist webmaster@openstreetmap.org as we are unable to reply to any confirmation requests." terms accepted: "Thanks for accepting the new contributor terms!" terms declined: "We are sorry that you have decided to not accept the new Contributor Terms. For more information, please see this wiki page." diff --git a/test/functional/user_controller_test.rb b/test/functional/user_controller_test.rb index 606924b35..17642b149 100644 --- a/test/functional/user_controller_test.rb +++ b/test/functional/user_controller_test.rb @@ -216,7 +216,13 @@ class UserControllerTest < ActionController::TestCase display_name = "new_tester" assert_difference('User.count') do assert_difference('ActionMailer::Base.deliveries.size') do - post :save, {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}} + session[:new_user] = User.new({ + :status => "pending", :display_name => display_name, + :email => new_email, :email_confirmation => new_email, + :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest" + }, :without_protection => true) + + post :save end end @@ -237,7 +243,13 @@ class UserControllerTest < ActionController::TestCase display_name = "new_tester" assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post :save, :user => { :email => email, :email_confirmation => email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"} + session[:new_user] = User.new({ + :status => "pending", :display_name => display_name, + :email => email, :email_confirmation => email, + :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest" + }, :without_protection => true) + + post :save end end assert_response :success @@ -251,7 +263,13 @@ class UserControllerTest < ActionController::TestCase display_name = "new_tester" assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post :save, :user => { :email => email, :email_confirmation => email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"} + session[:new_user] = User.new({ + :status => "pending", :display_name => display_name, + :email => email, :email_confirmation => email, + :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest" + }, :without_protection => true) + + post :save end end assert_response :success @@ -265,7 +283,13 @@ class UserControllerTest < ActionController::TestCase display_name = users(:public_user).display_name assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post :save, :user => { :email => email, :email_confirmation => email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"} + session[:new_user] = User.new({ + :status => "pending", :display_name => display_name, + :email => email, :email_confirmation => email, + :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest" + }, :without_protection => true) + + post :save end end assert_response :success @@ -279,7 +303,13 @@ class UserControllerTest < ActionController::TestCase display_name = users(:public_user).display_name.upcase assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post :save, :user => { :email => email, :email_confirmation => email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"} + session[:new_user] = User.new({ + :status => "pending", :display_name => display_name, + :email => email, :email_confirmation => email, + :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest" + }, :without_protection => true) + + post :save end end assert_response :success diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb index b5b3e91c7..7f3e921f9 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -21,7 +21,7 @@ class UserCreationTest < ActionController::IntegrationTest display_name = "#{localer.to_s}_new_tester" assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post '/user/save', + post '/user/terms', {:user => { :email => dup_email, :email_confirmation => dup_email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}}, {"HTTP_ACCEPT_LANGUAGE" => localer.to_s} end @@ -41,7 +41,7 @@ class UserCreationTest < ActionController::IntegrationTest email = "#{locale.to_s}_new_tester" assert_difference('User.count', 0) do assert_difference('ActionMailer::Base.deliveries.size', 0) do - post '/user/save', + post '/user/terms', {:user => {:email => email, :email_confirmation => email, :display_name => dup_display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}}, {"HTTP_ACCEPT_LANGUAGE" => locale.to_s} end @@ -58,10 +58,20 @@ class UserCreationTest < ActionController::IntegrationTest I18n.available_locales.each do |locale| new_email = "#{locale.to_s}newtester@osm.org" display_name = "#{locale.to_s}_new_tester" + + assert_difference('User.count', 0) do + assert_difference('ActionMailer::Base.deliveries.size', 0) do + post "/user/terms", + {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}} + end + end + + assert_response :success + assert_template 'user/terms' + assert_difference('User.count') do assert_difference('ActionMailer::Base.deliveries.size', 1) do - post_via_redirect "/user/save", - {:user => { :email => new_email, :email_confirmation => new_email, :display_name => display_name, :pass_crypt => "testtest", :pass_crypt_confirmation => "testtest"}}, + post_via_redirect "/user/save", {}, {"HTTP_ACCEPT_LANGUAGE" => "#{locale.to_s}"} end end -- 2.39.5