Fix Oauth forgetting data on registration

Author: microstudi

decidim-core/app/commands/decidim/create_omniauth_registration.rb

@@ -29,6 +29,7 @@ def call
           return broadcast(:ok, @user)
         end
         return broadcast(:invalid) if form.invalid?
+        return broadcast(:add_tos_errors) if requires_tos_acceptance?
 
         transaction do
           create_or_find_user
@@ -37,8 +38,6 @@ def call
         trigger_omniauth_event
 
         broadcast(:ok, @user)
-      rescue NeedTosAcceptance
-        broadcast(:add_tos_errors, @user)
       rescue ActiveRecord::RecordInvalid => e
         broadcast(:error, e.record)
       end
@@ -75,14 +74,19 @@ def create_or_find_user
         attach_avatar(form.avatar_url) if form.avatar_url.present?
         @user.tos_agreement = form.tos_agreement
         @user.accepted_tos_version = Time.current
-        raise NeedTosAcceptance if @user.tos_agreement.blank?
 
         @user.skip_confirmation! if verified_email
         @user.save!
         @user.after_confirmation if verified_email
       end
     end
 
+    def requires_tos_acceptance?
+      return false if form.tos_agreement.present?
+
+      verified_email.blank? || User.find_by(email: verified_email, organization:).blank?
+    end
+
     def attach_avatar(avatar_url)
       url = URI.parse(avatar_url)
       filename = File.basename(url.path)
@@ -148,9 +152,6 @@ def trigger_omniauth_event(event_name = "decidim.user.omniauth_registration")
     end
   end
 
-  class NeedTosAcceptance < StandardError
-  end
-
   class InvalidOauthSignature < StandardError
   end
 end

decidim-core/app/controllers/decidim/devise/omniauth_registrations_controller.rb

@@ -14,13 +14,15 @@ def new
       end
 
       def create
-        form_params = user_params_from_oauth_hash || params[:user]
+        return handle_missing_oauth_data! if pending_oauth_data.blank?
 
-        @form = form(OmniauthRegistrationForm).from_params(form_params)
-        @form.email ||= verified_email
+        form_params = form_params_from_pending_oauth
 
+        @form = form(OmniauthRegistrationForm).from_params(form_params)
         CreateOmniauthRegistration.call(@form, verified_email) do
           on(:ok) do |user|
+            clear_pending_oauth_data!
+
             if user.active_for_authentication?
               sign_in_and_redirect user, event: :authentication
               set_flash_message :notice, :success, kind: @form.provider.capitalize
@@ -39,7 +41,6 @@ def create
 
           on(:add_tos_errors) do
             set_flash_message :alert, :add_tos_errors if @form.valid_tos?
-            session[:verified_email] = verified_email
             render :new_tos_fields
           end
 
@@ -61,28 +62,90 @@ def action_missing(action_name)
 
       private
 
+      def form_params_from_pending_oauth
+        submitted_params = params.fetch(:user, {}).permit(:name, :nickname, :email, :tos_agreement, :newsletter).to_h.symbolize_keys
+        oauth_params = pending_oauth_form_params
+
+        oauth_params.merge(
+          submitted_params
+        )
+      end
+
       def oauth_data
         @oauth_data ||= oauth_hash.slice(:provider, :uid, :info)
       end
 
-      # Private: Create form params from omniauth hash
-      # Since we are using trusted omniauth data we are generating a valid signature.
-      def user_params_from_oauth_hash
-        return nil if oauth_data.empty?
+      # Private: Create trusted form params from oauth data stored server-side.
+      # Since we are using trusted oauth data we are generating a valid signature.
+      def pending_oauth_form_params
+        return {} if pending_oauth_data.blank?
 
         {
+          provider: pending_oauth_data[:provider],
+          uid: pending_oauth_data[:uid],
+          email: pending_oauth_data[:verified_email],
+          name: pending_oauth_data[:name],
+          nickname: pending_oauth_data[:nickname],
+          oauth_signature: OmniauthRegistrationForm.create_signature(pending_oauth_data[:provider], pending_oauth_data[:uid]),
+          avatar_url: pending_oauth_data[:avatar_url],
+          raw_data: pending_oauth_data[:raw_data],
+          pending_oauth_token:
+        }
+      end
+
+      def verified_email
+        @verified_email ||= pending_oauth_data&.dig(:verified_email)
+      end
+
+      def pending_oauth_data
+        @pending_oauth_data ||= if oauth_hash.present?
+                                  store_pending_oauth_data!
+                                else
+                                  restore_pending_oauth_data
+                                end
+      end
+
+      def pending_oauth_token
+        @pending_oauth_token ||= params.dig(:user, :pending_oauth_token).presence || session.dig(:omniauth_registration, :token)
+      end
+
+      def store_pending_oauth_data!
+        data = {
           provider: oauth_data[:provider],
           uid: oauth_data[:uid],
-          name: oauth_data[:info][:name],
-          nickname: oauth_data[:info][:nickname],
-          oauth_signature: OmniauthRegistrationForm.create_signature(oauth_data[:provider], oauth_data[:uid]),
-          avatar_url: oauth_data[:info][:image],
+          name: oauth_data.dig(:info, :name),
+          nickname: oauth_data.dig(:info, :nickname),
+          avatar_url: oauth_data.dig(:info, :image),
+          verified_email: oauth_data.dig(:info, :email).presence,
           raw_data: oauth_hash
         }
+
+        session[:omniauth_registration] = {
+          token: SecureRandom.hex(16),
+          data:
+        }
+
+        @pending_oauth_token = session[:omniauth_registration][:token]
+        data
       end
 
-      def verified_email
-        @verified_email ||= oauth_data.dig(:info, :email).presence || session[:verified_email]
+      def restore_pending_oauth_data
+        token = params.dig(:user, :pending_oauth_token)
+        return {} if token.blank?
+
+        state = session[:omniauth_registration]&.with_indifferent_access
+        return {} if state.blank?
+
+        stored_token = state[:token].to_s
+        return {} unless stored_token.bytesize == token.to_s.bytesize
+        return {} unless ActiveSupport::SecurityUtils.secure_compare(stored_token, token.to_s)
+
+        @pending_oauth_token = stored_token
+        state[:data] || {}
+      end
+
+      def clear_pending_oauth_data!
+        session.delete(:omniauth_registration)
       end
 
       def oauth_hash
@@ -91,6 +154,11 @@ def oauth_hash
 
         raw_hash.deep_symbolize_keys
       end
+
+      def handle_missing_oauth_data!
+        flash[:alert] = t("devise.failure.timeout")
+        redirect_back_or_to(decidim.root_path)
+      end
     end
   end
 end

decidim-core/app/forms/decidim/omniauth_registration_form.rb

@@ -15,6 +15,7 @@ class OmniauthRegistrationForm < Form
     attribute :oauth_signature, String
     attribute :avatar_url, String
     attribute :raw_data, Hash
+    attribute :pending_oauth_token, String
 
     validates :email, presence: true
     validates :name, presence: true

decidim-core/app/views/decidim/devise/omniauth_registrations/new.html.erb

@@ -24,9 +24,7 @@
 
       <%= render partial: "decidim/devise/shared/tos_fields", locals: { form: f, user: :user } %>
 
-      <%= f.hidden_field :uid %>
-      <%= f.hidden_field :provider %>
-      <%= f.hidden_field :oauth_signature %>
+      <%= f.hidden_field :pending_oauth_token %>
     </div>
 
     <div class="form__wrapper-block">

decidim-core/app/views/decidim/devise/omniauth_registrations/new_tos_fields.html.erb

@@ -12,9 +12,7 @@
 
       <%= render partial: "decidim/devise/shared/tos_fields", locals: { form: f, user: :user } %>
 
-      <%= f.hidden_field :uid %>
-      <%= f.hidden_field :provider %>
-      <%= f.hidden_field :oauth_signature %>
+      <%= f.hidden_field :pending_oauth_token %>
     </div>
 
     <div class="form__wrapper-block">

decidim-core/spec/commands/decidim/create_omniauth_registration_spec.rb

@@ -107,6 +107,38 @@ module Comments
           end
         end
 
+        context "when tos agreement is missing for a new user" do
+          let(:tos_agreement) { nil }
+
+          it "broadcasts add_tos_errors" do
+            expect { command.call }.to broadcast(:add_tos_errors)
+          end
+
+          it "does not create a new user" do
+            expect do
+              command.call
+            end.not_to change(User, :count)
+          end
+        end
+
+        context "when tos agreement is missing for an existing verified user" do
+          let(:tos_agreement) { nil }
+
+          before do
+            create(:user, email:, organization:)
+          end
+
+          it "broadcasts ok" do
+            expect { command.call }.to broadcast(:ok)
+          end
+
+          it "does not create a new user" do
+            expect do
+              command.call
+            end.not_to change(User, :count)
+          end
+        end
+
         context "when the form is valid" do
           it "broadcasts ok" do
             expect { command.call }.to broadcast(:ok)

decidim-core/spec/controllers/omniauth_registrations_controller_spec.rb

@@ -219,6 +219,118 @@ module Decidim
           end
         end
       end
+
+      context "when ToS is missing and user retries without omniauth.auth" do
+        let!(:user) { create(:user, organization:, email: "other@example.org") }
+        let(:raw_data) do
+          {
+            "provider" => provider,
+            "uid" => uid,
+            "info" => {
+              "name" => "Facebook User",
+              "nickname" => "facebook_user",
+              "email" => email
+            },
+            "extra" => {
+              "raw_info" => {
+                "id" => uid,
+                "name" => "Facebook User"
+              }
+            }
+          }
+        end
+
+        before do
+          request.env["omniauth.auth"] = raw_data
+        end
+
+        it "preserves oauth data in pending state and creates the account on retry" do
+          expect do
+            post :create, params: { locale: I18n.locale, user: { tos_agreement: nil } }
+          end.not_to change(User, :count)
+
+          expect(response).to render_template(:new_tos_fields)
+
+          token = session.dig(:omniauth_registration, :token)
+          expect(token).to be_present
+          expect(session.dig(:omniauth_registration, :data, :raw_data)).to eq(raw_data.deep_symbolize_keys)
+
+          request.env["omniauth.auth"] = nil
+          allow(ActiveSupport::Notifications).to receive(:publish).and_call_original
+
+          expect do
+            post :create, params: { locale: I18n.locale, user: { pending_oauth_token: token, tos_agreement: "1" } }
+          end.to change(User, :count).by(1)
+
+          expect(controller).to be_user_signed_in
+          expect(User.find_by(email:)).to be_present
+          expect(ActiveSupport::Notifications).to have_received(:publish).with(
+            "decidim.user.omniauth_registration",
+            hash_including(
+              provider:,
+              uid:,
+              email:,
+              raw_data: raw_data.deep_symbolize_keys
+            )
+          )
+        end
+      end
+
+      context "when oauth data is missing and there is no pending state" do
+        before do
+          request.env["omniauth.auth"] = nil
+        end
+
+        it "redirects to root with a timeout alert" do
+          expect do
+            post :create, params: { locale: I18n.locale }
+          end.not_to raise_error
+
+          expect(controller).not_to be_user_signed_in
+          expect(controller).to redirect_to(root_path)
+          expect(flash[:alert]).to eq(I18n.t("devise.failure.timeout"))
+        end
+      end
+
+      context "when pending oauth state session keys are strings" do
+        let(:pending_token) { SecureRandom.hex(16) }
+        let(:pending_raw_data) do
+          {
+            provider:,
+            uid:,
+            info: {
+              name: "Facebook User",
+              nickname: "facebook_user",
+              email:
+            }
+          }
+        end
+
+        before do
+          request.env["omniauth.auth"] = nil
+          session[:omniauth_registration] = {
+            "token" => pending_token,
+            "data" => {
+              "provider" => provider,
+              "uid" => uid,
+              "name" => "Facebook User",
+              "nickname" => "facebook_user",
+              "avatar_url" => nil,
+              "verified_email" => email,
+              "raw_data" => pending_raw_data
+            }
+          }
+        end
+
+        it "restores the pending state and signs in" do
+          expect do
+            post :create, params: { locale: I18n.locale, user: { pending_oauth_token: pending_token, tos_agreement: "1" } }
+          end.to change(Identity, :count).by(1)
+
+          expect(controller).to be_user_signed_in
+          expect(User.find_by(email:)).to be_present
+        end
+      end
     end
   end
 end