From: Tom Hughes Date: Thu, 7 Dec 2023 18:49:49 +0000 (+0000) Subject: Use rails tokens for signup confirmations X-Git-Tag: live~703^2 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/4dff06a6293971c3e17f8508859a1d80717a23f6 Use rails tokens for signup confirmations --- diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 3d369d558..6c917e218 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -66,7 +66,7 @@ Metrics/BlockNesting: # Offense count: 26 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 310 + Max: 313 # Offense count: 59 # Configuration parameters: AllowedMethods, AllowedPatterns. diff --git a/app/controllers/concerns/session_methods.rb b/app/controllers/concerns/session_methods.rb index fca851eeb..cebe932fc 100644 --- a/app/controllers/concerns/session_methods.rb +++ b/app/controllers/concerns/session_methods.rb @@ -62,9 +62,10 @@ module SessionMethods ## # def unconfirmed_login(user) - session[:token] = user.tokens.create.token + session[:pending_user] = user.id - redirect_to :controller => "confirmations", :action => "confirm", :display_name => user.display_name + redirect_to :controller => "confirmations", :action => "confirm", + :display_name => user.display_name, :referer => session[:referer] session.delete(:remember_me) session.delete(:referer) diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb index 604e6b5b3..48b8dabf2 100644 --- a/app/controllers/confirmations_controller.rb +++ b/app/controllers/confirmations_controller.rb @@ -15,41 +15,37 @@ class ConfirmationsController < ApplicationController def confirm if request.post? - token = UserToken.find_by(:token => params[:confirm_string]) - if token&.user&.active? - flash[:error] = t(".already active") - redirect_to login_path - elsif !token || token.expired? + token = params[:confirm_string] + + user = User.find_by_token_for(:new_user, token) || + UserToken.unexpired.find_by(:token => token)&.user + + if !user flash[:error] = t(".unknown token") redirect_to :action => "confirm" - elsif !token.user.visible? - render_unknown_user token.user.display_name + elsif user.active? + flash[:error] = t(".already active") + redirect_to login_path + elsif !user.visible? + render_unknown_user user.display_name else - user = token.user user.activate user.email_valid = true flash[:notice] = gravatar_status_message(user) if gravatar_enable(user) user.save! - referer = safe_referer(token.referer) if token.referer - token.destroy - - if session[:token] - token = UserToken.find_by(:token => session[:token]) - session.delete(:token) - else - token = nil - end + referer = safe_referer(params[:referer]) if params[:referer] + UserToken.delete_by(:token => token) - if token.nil? || token.user != user - flash[:notice] = t(".success") - redirect_to login_path(:referer => referer) - else - token.destroy + pending_user = session.delete(:pending_user) + if user.id == pending_user session[:user] = user.id session[:fingerprint] = user.fingerprint redirect_to referer || welcome_path + else + flash[:notice] = t(".success") + redirect_to login_path(:referer => referer) end end else @@ -61,12 +57,11 @@ class ConfirmationsController < ApplicationController def confirm_resend user = User.visible.find_by(:display_name => params[:display_name]) - token = UserToken.find_by(:token => session[:token]) - if user.nil? || token.nil? || token.user != user + if user.nil? || user.id != session[:pending_user] flash[:error] = t ".failure", :name => params[:display_name] else - UserMailer.signup_confirm(user, user.tokens.create).deliver_later + UserMailer.signup_confirm(user, user.generate_token_for(:new_user)).deliver_later flash[:notice] = { :partial => "confirmations/resend_success_flash", :locals => { :email => user.email, :sender => Settings.email_from } } end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 3c2084a5b..e57ffc06a 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -27,12 +27,7 @@ class SessionsController < ApplicationController @title = t ".title" if request.post? - if session[:token] - token = UserToken.find_by(:token => session[:token]) - token&.destroy - session.delete(:token) - end - + session.delete(:pending_user) session.delete(:user) session_expires_automatically diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ab13f93be..429fa47a4 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -203,8 +203,8 @@ class UsersController < ApplicationController session[:referer] = referer successful_login(current_user) else - session[:token] = current_user.tokens.create.token - UserMailer.signup_confirm(current_user, current_user.tokens.create(:referer => referer)).deliver_later + session[:pending_user] = current_user.id + UserMailer.signup_confirm(current_user, current_user.generate_token_for(:new_user), referer).deliver_later redirect_to :controller => :confirmations, :action => :confirm, :display_name => current_user.display_name end else diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 4e15a296a..92c64b4d6 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -10,11 +10,12 @@ class UserMailer < ApplicationMailer before_action :set_shared_template_vars before_action :attach_project_logo - def signup_confirm(user, token) + def signup_confirm(user, token, referer = nil) with_recipient_locale user do @url = url_for(:controller => "confirmations", :action => "confirm", :display_name => user.display_name, - :confirm_string => token.token) + :confirm_string => token, + :referer => referer) mail :to => user.email, :subject => t(".subject") diff --git a/app/models/user.rb b/app/models/user.rb index 958a03a98..6fa0f330e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -124,6 +124,10 @@ class User < ApplicationRecord before_save :update_tile after_save :spam_check + generates_token_for :new_user, :expires_in => 1.week do + fingerprint + end + generates_token_for :new_email, :expires_in => 1.week do fingerprint end diff --git a/test/controllers/confirmations_controller_test.rb b/test/controllers/confirmations_controller_test.rb index 11d2bfd2c..82580dc68 100644 --- a/test/controllers/confirmations_controller_test.rb +++ b/test/controllers/confirmations_controller_test.rb @@ -39,7 +39,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest user = build(:user, :pending) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) get user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } assert_response :success @@ -51,7 +51,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) # Get the confirmation page get user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } @@ -73,7 +73,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path @@ -87,7 +87,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } assert_redirected_to welcome_path @@ -98,7 +98,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path session_for(create(:user)) @@ -113,11 +113,11 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to login_path(:referer => new_diary_entry_path) assert_match(/Confirmed your account/, flash[:notice]) end @@ -127,9 +127,9 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to new_diary_entry_path end @@ -138,12 +138,12 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) post logout_path session_for(create(:user)) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to login_path(:referer => new_diary_entry_path) assert_match(/Confirmed your account/, flash[:notice]) end @@ -153,9 +153,11 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:expiry => 1.day.ago).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + travel 2.weeks do + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + end assert_redirected_to :action => "confirm" assert_match(/confirmation code has expired/, flash[:error]) end @@ -165,15 +167,15 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to new_diary_entry_path post logout_path - confirm_string = User.find_by(:email => user.email).tokens.create(:referer => new_diary_entry_path).token - post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string } + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) + post user_confirm_path, :params => { :display_name => user.display_name, :confirm_string => confirm_string, :referer => new_diary_entry_path } assert_redirected_to login_path assert_match(/already been confirmed/, flash[:error]) end @@ -183,7 +185,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest stub_gravatar_request(user.email) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - confirm_string = User.find_by(:email => user.email).tokens.create.token + confirm_string = User.find_by(:email => user.email).generate_token_for(:new_user) User.find_by(:display_name => user.display_name).hide! diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 4234bee70..71d6de184 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -88,10 +88,7 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest user = build(:user, :pending) post user_new_path, :params => { :user => user.attributes } post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - - assert_difference "User.find_by(:email => user.email).tokens.count", -1 do - post logout_path - end + post logout_path assert_response :redirect assert_redirected_to root_path end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 402129d32..a530a6f85 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -312,15 +312,13 @@ class UsersControllerTest < ActionDispatch::IntegrationTest assert_difference "User.count", 1 do assert_difference "ActionMailer::Base.deliveries.size", 1 do - perform_enqueued_jobs do - post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } - end + post user_save_path, :params => { :read_ct => 1, :read_tou => 1 } + assert_enqueued_with :job => ActionMailer::MailDeliveryJob, + :args => proc { |args| args[3][:args][2] == welcome_path(:editor => "id", :zoom => 1, :lat => 2, :lon => 3) } + perform_enqueued_jobs end end - assert_equal welcome_path(:editor => "id", :zoom => 1, :lat => 2, :lon => 3), - User.find_by(:email => user.email).tokens.order("id DESC").first.referer - ActionMailer::Base.deliveries.clear end diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb index 253f298a5..35f98d17d 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -209,11 +209,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -222,11 +222,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -362,11 +362,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_openid/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -375,11 +375,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -516,11 +516,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_google/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -529,11 +529,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -668,11 +668,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_facebook/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -681,11 +681,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -820,11 +820,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_microsoft/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -833,11 +833,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -974,11 +974,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_github/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -987,11 +987,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success @@ -1128,11 +1128,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_equal register_email.to.first, new_email # Check that the confirm account url is correct - confirm_regex = Regexp.new("/user/redirect_tester_wikipedia/confirm\\?confirm_string=([a-zA-Z0-9_-]*)") + confirm_regex = Regexp.new("confirm_string=([a-zA-Z0-9%_-]*)") email_text_parts(register_email).each do |part| assert_match confirm_regex, part.body.to_s end - confirm_string = email_text_parts(register_email).first.body.match(confirm_regex)[1] + confirm_string = CGI.unescape(email_text_parts(register_email).first.body.match(confirm_regex)[1]) # Check the page assert_response :success @@ -1141,11 +1141,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest ActionMailer::Base.deliveries.clear # Go to the confirmation page - get "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + get "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :success assert_template "confirmations/confirm" - post "/user/#{display_name}/confirm", :params => { :confirm_string => confirm_string } + post "/user/#{display_name}/confirm", :params => { :referer => "/welcome", :confirm_string => confirm_string } assert_response :redirect follow_redirect! assert_response :success