FIX: correctly remove authentication_data cookie on oauth login flow (#9238) (#9251)

Attempt 2, with more test.

Additionally correctly handle cookie path for authentication_data

There were two bugs that exposed an interesting case where two discourse
instances hosted across two subfolder installs in the same domain
with oauth may clash and cause strange redirection on first login:

Log in to example.com/forum1. authentication_data cookie is set with path /
On the first redirection, the current authentication_data cookie is not unset.
Log in to example.com/forum2. In this case, the authentication_data cookie
is already set from forum1 - the initial page load will incorrectly redirect
the user to the redirect URL from the already-stored cookie, to /forum1.

This removes this issue by:

Setting the cookie for the correct path, and not having it on root
Correctly removing the cookie on first login

app/controllers/users/omniauth_callbacks_controller.rb

@@ -74,7 +74,10 @@ class Users::OmniauthCallbacksController < ApplicationController
       @auth_result.authenticator_name = authenticator.name
       complete_response_data
       cookies['_bypass_cache'] = true
-      cookies[:authentication_data] = @auth_result.to_client_hash.to_json
+      cookies[:authentication_data] = {
+        value: @auth_result.to_client_hash.to_json,
+        path: Discourse.base_uri
+      }
       redirect_to @origin
     end
   end

app/views/layouts/application.html.erb

@@ -58,8 +58,8 @@
 
     <%= tag.meta id: 'data-discourse-setup', data: client_side_setup_data %>
 
-    <%- if !current_user && cookies[:authentication_data] %>
-      <meta id="data-authentication" data-authentication-data="<%= cookies.delete(:authentication_data) %>">
+    <%- if (data = cookies.delete(:authentication_data, path: Discourse.base_uri)) && !current_user  %>
+      <meta id="data-authentication" data-authentication-data="<%= data %>">
     <%- end %>
   </head>
 

spec/requests/application_controller_spec.rb

@@ -84,6 +84,14 @@ RSpec.describe ApplicationController do
         expect(response).to redirect_to("/login")
       end
     end
+
+    it 'contains authentication data when cookies exist' do
+      COOKIE_DATA = "someauthenticationdata"
+      cookies['authentication_data'] = COOKIE_DATA
+      get '/login'
+      expect(response.status).to eq(200)
+      expect(response.body).to include("data-authentication-data=\"#{COOKIE_DATA }\"")
+    end
   end
 
   describe '#redirect_to_second_factor_if_required' do

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -269,6 +269,30 @@ RSpec.describe Users::OmniauthCallbacksController do
         expect(user.email_confirmed?).to eq(true)
       end
 
+      it 'should return the authenticated response with the correct path for subfolders' do
+        set_subfolder "/forum"
+        events = DiscourseEvent.track_events do
+          get "/auth/google_oauth2/callback.json"
+        end
+
+        expect(response.headers["Set-Cookie"].match(/^authentication_data=.*; path=\/forum/)).not_to eq(nil)
+
+        expect(events.map { |event| event[:event_name] }).to include(:user_logged_in, :user_first_logged_in)
+
+        expect(response.status).to eq(302)
+
+        data = JSON.parse(response.cookies["authentication_data"])
+
+        expect(data["authenticated"]).to eq(true)
+        expect(data["awaiting_activation"]).to eq(false)
+        expect(data["awaiting_approval"]).to eq(false)
+        expect(data["not_allowed_from_ip_address"]).to eq(false)
+        expect(data["admin_not_allowed_from_ip_address"]).to eq(false)
+
+        user.reload
+        expect(user.email_confirmed?).to eq(true)
+      end
+
       it "should confirm email even when the tokens are expired" do
         user.email_tokens.update_all(confirmed: false, expired: true)