From 15623aa35a9fe77343efd7e67f04d33599a8ce3b Mon Sep 17 00:00:00 2001 From: Milan Cvetkovic Date: Mon, 27 May 2024 14:40:53 +0000 Subject: [PATCH] Social sign-in: avoid re-authorization in `users_controller#create` It does not add any additional guards against malicious users: Malicious user may attempt to invoke `POST /users/new` with bogus values for `auth_provider` and `auth_uid` resulting with a new account to which user would have a way to login, other than sending a password reset request. In some cases, re-authorization would introduce additional "Please login to your social account", or "Are you sure you want to be logged in" popup triggered by identity provider. This PR removes the re-authorization request from `POST /users/new` in authorization flow. --- app/controllers/users_controller.rb | 13 -- test/integration/user_creation_test.rb | 195 +++---------------------- 2 files changed, 18 insertions(+), 190 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 341679763..b7c156bd9 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -98,13 +98,8 @@ class UsersController < ApplicationController if current_user.invalid? # Something is wrong with a new user, so rerender the form render :action => "new" - elsif current_user.auth_provider.present? - # Verify external authenticator before moving on - session[:new_user] = current_user.slice("email", "display_name", "pass_crypt", "pass_crypt_confirmation") - redirect_to auth_url(current_user.auth_provider, current_user.auth_uid, params[:referer]), :status => :temporary_redirect else # Save the user record - session[:new_user] = current_user.slice("email", "display_name", "pass_crypt", "pass_crypt_confirmation") save_new_user params[:email_hmac], params[:referer] end end @@ -219,12 +214,6 @@ class UsersController < ApplicationController session[:user_errors] = current_user.errors.as_json redirect_to edit_account_path - elsif session[:new_user] - session[:new_user]["auth_provider"] = provider - session[:new_user]["auth_uid"] = uid - - email_hmac = UsersController.message_hmac(email) if email_verified && email - save_new_user email_hmac, referer else user = User.find_by(:auth_provider => provider, :auth_uid => uid) @@ -273,8 +262,6 @@ class UsersController < ApplicationController private def save_new_user(email_hmac, referer = nil) - new_user = session.delete(:new_user) - self.current_user = User.new(new_user) if check_signup_allowed(current_user.email) current_user.data_public = true current_user.description = "" if current_user.description.nil? diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb index 4611860d0..1b0933d32 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -45,47 +45,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_select "form > div > input.is-invalid#user_email" end - def test_user_create_association_bad_auth_provider - assert_difference("User.count", 0) do - assert_no_difference("ActionMailer::Base.deliveries.size") do - perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => "test@example.com", - :display_name => "new_tester", - :pass_crypt => "testtest", - :pass_crypt_confirmation => "testtest", - :auth_provider => "noprovider", - :auth_uid => "123454321", - :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "noprovider", :origin => "/user/new") - post response.location - end - end - end - assert_response :not_found - end - - def test_user_create_association_no_auth_uid - OmniAuth.config.mock_auth[:google] = :invalid_credentials - assert_difference("User.count", 0) do - assert_no_difference("ActionMailer::Base.deliveries.size") do - perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => "test@example.com", - :display_name => "new_tester", - :pass_crypt => "testtest", - :pass_crypt_confirmation => "testtest", - :auth_provider => "google", - :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "google", :origin => "/user/new") - post response.location - end - end - end - follow_redirect! - assert_redirected_to auth_failure_path(:strategy => "google", :message => "invalid_credentials", :origin => "/user/new") - end - def test_user_create_association_submit_duplicate_email dup_email = create(:user).email display_name = "new_tester" @@ -275,6 +234,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest def test_user_create_openid_success new_email = "newtester-openid@osm.org" display_name = "new_tester-openid" + openid_url = "http://localhost:1000/new.tester" auth_uid = "http://localhost:1123/new.tester" OmniAuth.config.add_mock(:openid, @@ -284,8 +244,8 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_difference("User.count") do assert_difference("ActionMailer::Base.deliveries.size", 1) do perform_enqueued_jobs do - post auth_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") - assert_redirected_to auth_success_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") + post auth_path(:provider => "openid", :openid_url => openid_url, :origin => "/user/new") + assert_redirected_to auth_success_path(:provider => "openid", :openid_url => openid_url, :origin => "/user/new") follow_redirect! assert_redirected_to :controller => :users, :action => "new", :nickname => display_name, :email => new_email, :auth_provider => "openid", :auth_uid => auth_uid @@ -294,11 +254,8 @@ class UserCreationTest < ActionDispatch::IntegrationTest :params => { :user => { :email => new_email, :display_name => display_name, :auth_provider => "openid", - :auth_uid => "http://localhost:1123/new.tester", + :auth_uid => auth_uid, :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") - post response.location - follow_redirect! end end end @@ -335,22 +292,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest def test_user_create_openid_failure OmniAuth.config.mock_auth[:openid] = :connection_failed - new_email = "newtester-openid2@osm.org" - display_name = "new_tester-openid2" assert_difference("User.count", 0) do assert_difference("ActionMailer::Base.deliveries.size", 0) do perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => new_email, - :email_confirmation => new_email, - :display_name => display_name, - :auth_provider => "openid", - :auth_uid => "http://localhost:1123/new.tester", - :pass_crypt => "", - :pass_crypt_confirmation => "" } } - assert_redirected_to auth_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") + post auth_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") follow_redirect! assert_redirected_to auth_failure_path(:strategy => "openid", :message => "connection_failed", :origin => "/user/new") follow_redirect! @@ -363,6 +308,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest end def test_user_create_openid_redirect + openid_url = "http://localhost:1000/new.tester" auth_uid = "http://localhost:1123/new.tester" new_email = "redirect_tester_openid@osm.org" display_name = "redirect_tester_openid" @@ -374,8 +320,8 @@ class UserCreationTest < ActionDispatch::IntegrationTest assert_difference("User.count") do assert_difference("ActionMailer::Base.deliveries.size", 1) do perform_enqueued_jobs do - post auth_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") - assert_redirected_to auth_success_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") + post auth_path(:provider => "openid", :openid_url => openid_url, :origin => "/user/new") + assert_redirected_to auth_success_path(:provider => "openid", :openid_url => openid_url, :origin => "/user/new") follow_redirect! assert_redirected_to :controller => :users, :action => "new", :nickname => display_name, :email => new_email, :auth_provider => "openid", :auth_uid => auth_uid @@ -386,11 +332,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_provider => "openid", :auth_uid => auth_uid, :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "openid", :openid_url => "http://localhost:1123/new.tester", :origin => "/user/new") - follow_redirect! - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name follow_redirect! end end @@ -454,10 +395,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_uid => auth_uid, :consider_pd => "1" }, :email_hmac => email_hmac } - assert_redirected_to auth_path(:provider => "google", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "google") - follow_redirect! assert_redirected_to welcome_path follow_redirect! end @@ -499,22 +436,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest def test_user_create_google_failure OmniAuth.config.mock_auth[:google] = :connection_failed - new_email = "newtester-google2@osm.org" - display_name = "new_tester-google2" assert_difference("User.count", 0) do assert_difference("ActionMailer::Base.deliveries.size", 0) do perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => new_email, - :email_confirmation => new_email, - :display_name => display_name, - :auth_provider => "google", - :auth_uid => "123454321", - :pass_crypt => "", - :pass_crypt_confirmation => "" } } - assert_redirected_to auth_path(:provider => "google", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "google") + post auth_path(:provider => "google", :origin => "/user/new") + assert_response :redirect follow_redirect! assert_redirected_to auth_failure_path(:strategy => "google", :message => "connection_failed", :origin => "/user/new") follow_redirect! @@ -555,11 +481,7 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_provider => "google", :auth_uid => auth_uid, :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "google", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "google") - follow_redirect! - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name + assert_response :redirect follow_redirect! end end @@ -622,10 +544,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_uid => auth_uid, :consider_pd => "1" }, :email_hmac => email_hmac } - assert_redirected_to auth_path(:provider => "facebook", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "facebook") - follow_redirect! assert_redirected_to welcome_path follow_redirect! end @@ -666,22 +584,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest def test_user_create_facebook_failure OmniAuth.config.mock_auth[:facebook] = :connection_failed - new_email = "newtester-facebook2@osm.org" - display_name = "new_tester-facebook2" assert_difference("User.count", 0) do assert_difference("ActionMailer::Base.deliveries.size", 0) do perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => new_email, - :email_confirmation => new_email, - :display_name => display_name, - :auth_provider => "facebook", - :auth_uid => "123454321", - :pass_crypt => "", - :pass_crypt_confirmation => "" } } - assert_redirected_to auth_path(:provider => "facebook", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "facebook") + post auth_path(:provider => "facebook", :origin => "/user/new") + assert_response :redirect follow_redirect! assert_redirected_to auth_failure_path(:strategy => "facebook", :message => "connection_failed", :origin => "/user/new") follow_redirect! @@ -723,11 +630,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_provider => "facebook", :auth_uid => auth_uid, :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "facebook", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "facebook") - follow_redirect! - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name assert_response :redirect follow_redirect! end @@ -790,10 +692,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_uid => auth_uid, :consider_pd => "1" }, :email_hmac => email_hmac } - assert_redirected_to auth_path(:provider => "microsoft", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "microsoft") - follow_redirect! assert_redirected_to welcome_path follow_redirect! end @@ -834,22 +732,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest def test_user_create_microsoft_failure OmniAuth.config.mock_auth[:microsoft] = :connection_failed - new_email = "newtester-microsoft2@osm.org" - display_name = "new_tester-microsoft2" assert_difference("User.count", 0) do assert_difference("ActionMailer::Base.deliveries.size", 0) do perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => new_email, - :email_confirmation => new_email, - :display_name => display_name, - :auth_provider => "microsoft", - :auth_uid => "123454321", - :pass_crypt => "", - :pass_crypt_confirmation => "" } } - assert_redirected_to auth_path(:provider => "microsoft", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "microsoft") + post auth_path(:provider => "microsoft", :origin => "/user/new") + assert_response :redirect follow_redirect! assert_redirected_to auth_failure_path(:strategy => "microsoft", :message => "connection_failed", :origin => "/user/new") follow_redirect! @@ -890,11 +777,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_provider => "microsoft", :auth_uid => auth_uid, :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "microsoft", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "microsoft") - follow_redirect! - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name assert_response :redirect follow_redirect! end @@ -962,10 +844,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :read_ct => 1, :read_tou => 1, :email_hmac => email_hmac } - assert_redirected_to auth_path(:provider => "github", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "github") - follow_redirect! assert_redirected_to welcome_path follow_redirect! end @@ -1007,22 +885,10 @@ class UserCreationTest < ActionDispatch::IntegrationTest def test_user_create_github_failure OmniAuth.config.mock_auth[:github] = :connection_failed - new_email = "newtester-github2@osm.org" - display_name = "new_tester-github2" assert_difference("User.count", 0) do assert_difference("ActionMailer::Base.deliveries.size", 0) do perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => new_email, - :email_confirmation => new_email, - :display_name => display_name, - :auth_provider => "github", - :auth_uid => "123454321", - :pass_crypt => "", - :pass_crypt_confirmation => "" } } - assert_redirected_to auth_path(:provider => "github", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "github") + post auth_path(:provider => "github", :origin => "/user/new") follow_redirect! assert_redirected_to auth_failure_path(:strategy => "github", :message => "connection_failed", :origin => "/user/new") follow_redirect! @@ -1062,11 +928,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_provider => "github", :auth_uid => auth_uid, :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "github", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "github") - follow_redirect! - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name assert_response :redirect follow_redirect! end @@ -1133,10 +994,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :read_ct => 1, :read_tou => 1, :email_hmac => email_hmac } - assert_redirected_to auth_path(:provider => "wikipedia", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "wikipedia", :origin => "/user/new") - follow_redirect! assert_redirected_to welcome_path follow_redirect! end @@ -1175,22 +1032,11 @@ class UserCreationTest < ActionDispatch::IntegrationTest def test_user_create_wikipedia_failure OmniAuth.config.mock_auth[:wikipedia] = :connection_failed - new_email = "newtester-wikipedia2@osm.org" - display_name = "new_tester-wikipedia2" assert_difference("User.count", 0) do assert_difference("ActionMailer::Base.deliveries.size", 0) do perform_enqueued_jobs do - post "/user/new", - :params => { :user => { :email => new_email, - :email_confirmation => new_email, - :display_name => display_name, - :auth_provider => "wikipedia", - :auth_uid => "123454321", - :pass_crypt => "", - :pass_crypt_confirmation => "" } } - assert_redirected_to auth_path(:provider => "wikipedia", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "wikipedia", :origin => "/user/new") + post auth_path(:provider => "wikipedia", :origin => "/user/new") + assert_response :redirect follow_redirect! assert_redirected_to auth_failure_path(:strategy => "wikipedia", :message => "connection_failed", :origin => "/user/new") follow_redirect! @@ -1232,11 +1078,6 @@ class UserCreationTest < ActionDispatch::IntegrationTest :auth_provider => "wikipedia", :auth_uid => auth_uid, :consider_pd => "1" } } - assert_redirected_to auth_path(:provider => "wikipedia", :origin => "/user/new") - post response.location - assert_redirected_to auth_success_path(:provider => "wikipedia", :origin => "/user/new") - follow_redirect! - assert_redirected_to :controller => :confirmations, :action => :confirm, :display_name => display_name assert_response :redirect follow_redirect! end -- 2.39.5