FIX: Handle staged users as unregistered users for external auth (#12567)

FIX: Handle staged users as unregistered users for external auth (#12567)

For ‘local logins’, the UX for staged users is designed to be identical to unregistered users. However, staged users logging in via external auth were being automatically unstaged, and skipping the registration/invite flow. In the past this made sense because the registration/invite flows didn’t work perfectly with external auth. Now, both registration and invites work well with external auth, so it’s best to leave the ‘unstage’ logic to those endpoints.

This problem was particularly noticeable when using the ‘bulk invite’ feature to invite users with pre-configured User Fields. In that situation, staged user accounts are used to preserve the user field data.

diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb
index 32b9bc3..2b70296 100644
--- a/app/controllers/users/omniauth_callbacks_controller.rb
+++ b/app/controllers/users/omniauth_callbacks_controller.rb
@@ -35,6 +35,7 @@ class Users::OmniauthCallbacksController < ApplicationController
     else
       DiscourseEvent.trigger(:before_auth, authenticator, auth)
       @auth_result = authenticator.after_authenticate(auth)
+      @auth_result.user = nil if @auth_result&.user&.staged # Treat staged users the same as unregistered users
       DiscourseEvent.trigger(:after_auth, authenticator, @auth_result)
     end
 
@@ -137,10 +138,8 @@ class Users::OmniauthCallbacksController < ApplicationController
       return
     end
 
-    # automatically activate/unstage any account if a provider marked the email valid
+    # automatically activate any account if a provider marked the email valid
     if @auth_result.email_valid && @auth_result.email == user.email
-      user.unstage!
-
       if !user.active || !user.email_confirmed?
         user.update!(password: SecureRandom.hex)
 
diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb
index 33359a2..bd74215 100644
--- a/spec/requests/omniauth_callbacks_controller_spec.rb
+++ b/spec/requests/omniauth_callbacks_controller_spec.rb
@@ -356,7 +356,7 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(user.email_confirmed?).to eq(true)
       end
 
-      it "should unstage staged user" do
+      it "should treat a staged user the same as an unregistered user" do
         user.update!(staged: true, registration_ip_address: nil)
 
         user.reload
@@ -367,13 +367,32 @@ RSpec.describe Users::OmniauthCallbacksController do
           get "/auth/google_oauth2/callback.json"
         end
 
-        expect(events.map { |event| event[:event_name] }).to include(:user_logged_in, :user_first_logged_in)
+        expect(events.map { |event| event[:event_name] }).to include(:before_auth, :after_auth)
 
         expect(response.status).to eq(302)
 
+        data = JSON.parse(cookies[:authentication_data])
+        expect(data["username"]).to eq("Somenickname")
+
+        user.reload
+        expect(user.staged).to eq(true)
+        expect(user.registration_ip_address).to eq(nil)
+
+        # Now register
+        UsersController.any_instance.stubs(:honeypot_value).returns(nil)
+        UsersController.any_instance.stubs(:challenge_value).returns(nil)
+        post "/u.json", params: {
+          name: "My new name",
+          username: "mynewusername",
+          email: user.email
+        }
+
+        expect(response.status).to eq(200)
+
         user.reload
         expect(user.staged).to eq(false)
         expect(user.registration_ip_address).to be_present
+        expect(user.name).to eq("My new name")
       end
 
       it "should activate user with matching email" do

GitHub sha: 198c960b

1 Like

This commit appears in #12567 which was approved by udan11. It was merged by davidtaylorhq.