From 3170c06077e111a734c0e7e120d20d052f9d3681 Mon Sep 17 00:00:00 2001
From: Fernando Valverde <fernando@fdo.cr>
Date: Fri, 9 Dec 2022 09:58:04 -0600
Subject: [PATCH] Fix connect account bug from settings (#18824)

* Fix connect account bug from settings

* Adds a controller concern test for after_sign_in_path_for

* Styling tweaks

* Merge uri query_values and only ignore i=i param

* remove unused stored_location_for and rename path var

* Reintroduce stored_location_for :p

* Reorder path origin priority
---
 app/controllers/application_controller.rb     |  5 +--
 .../users/_additional_authentication.html.erb |  2 +-
 .../concerns/omniauth_redirect_spec.rb        | 36 +++++++++++++++++++
 3 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100644 spec/controllers/concerns/omniauth_redirect_spec.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index e9d4fc149..6119d66df 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -168,12 +168,13 @@ class ApplicationController < ActionController::Base
   # the user to after a successful log in
   def after_sign_in_path_for(resource)
     if current_user.saw_onboarding
-      path = stored_location_for(resource) || request.env["omniauth.origin"] || root_path(signin: "true")
+      path = request.env["omniauth.origin"] || stored_location_for(resource) || root_path(signin: "true")
       signin_param = { "signin" => "true" } # the "signin" param is used by the service worker
 
       uri = Addressable::URI.parse(path)
       uri.query_values = if uri.query_values
-                           uri.query_values.merge(signin_param)
+                           # Ignore i=i (internal navigation) param
+                           uri.query_values.except("i").merge(signin_param)
                          else
                            signin_param
                          end
diff --git a/app/views/users/_additional_authentication.html.erb b/app/views/users/_additional_authentication.html.erb
index 5332e3ad4..e4b2192d3 100644
--- a/app/views/users/_additional_authentication.html.erb
+++ b/app/views/users/_additional_authentication.html.erb
@@ -8,7 +8,7 @@
       -->
       <% next if provider.provider_name == :apple %>
       <% unless @user.authenticated_through?(provider.provider_name) %>
-        <%= form_with url: provider.sign_in_path(state: "profile"), class: "flex w-100", local: true do |f| %>
+        <%= form_with url: provider.sign_in_path(state: "profile", origin: URL.url("/settings")), class: "flex w-100", local: true do |f| %>
           <%= f.button type: :submit, class: "crayons-btn crayons-btn--icon-left crayons-btn--brand-#{provider.provider_name} m-1" do %>
             <%= crayons_icon_tag(provider.provider_name, title: provider.official_name) %>
             <%= t("views.settings.account.connect", provider: provider.official_name) %>
diff --git a/spec/controllers/concerns/omniauth_redirect_spec.rb b/spec/controllers/concerns/omniauth_redirect_spec.rb
new file mode 100644
index 000000000..28670440e
--- /dev/null
+++ b/spec/controllers/concerns/omniauth_redirect_spec.rb
@@ -0,0 +1,36 @@
+require "rails_helper"
+
+# rubocop:disable Style/OpenStructUse
+# rubocop:disable Performance/OpenStruct
+RSpec.describe "Omniauth redirect", type: :request do
+  let!(:user) { create(:user) }
+  let!(:controller) { ApplicationController.new }
+  let!(:mock_warden) { Warden::Proxy.new({}, Warden::Manager.new(nil)) }
+
+  before do
+    allow(controller).to receive(:current_user).and_return(user)
+    allow(controller).to receive(:stored_location_for).and_return(nil)
+    allow(controller).to receive(:root_path).and_return("/new")
+  end
+
+  it "avoids i=i param in after_sign_in_path_for" do
+    mock_request = OpenStruct.new(env: { "warden" => mock_warden })
+    allow(controller).to receive(:request).and_return(mock_request)
+
+    path = controller.after_sign_in_path_for(user)
+    expect(path).not_to include("i=i")
+    expect(path).to end_with("/new?signin=true")
+  end
+
+  it "respects the origin param passed through the OAuth flow" do
+    mock_env = { "omniauth.origin" => "/settings", "warden" => mock_warden }
+    mock_request = OpenStruct.new(env: mock_env)
+    allow(controller).to receive(:request).and_return(mock_request)
+
+    path = controller.after_sign_in_path_for(user)
+    expect(path).not_to include("i=i")
+    expect(path).to end_with("/settings?signin=true")
+  end
+end
+# rubocop:enable Style/OpenStructUse
+# rubocop:enable Performance/OpenStruct