From 0e553f1fd12d0fd730259f87aa9fd91f25a3beff Mon Sep 17 00:00:00 2001 From: Jeff Wong Date: Thu, 19 Mar 2020 06:00:46 -1000 Subject: [PATCH] FIX: correctly remove authentication_data cookie on oauth login flow (#9238) 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 --- .../users/omniauth_callbacks_controller.rb | 5 +++- app/views/layouts/application.html.erb | 4 ++-- spec/requests/application_controller_spec.rb | 8 +++++++ .../omniauth_callbacks_controller_spec.rb | 24 +++++++++++++++++++ 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 36548585f3..5c73c7af78 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/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 diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 3340017d75..6981fdee4f 100644 --- a/app/views/layouts/application.html.erb +++ b/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] %> - + <%- if !current_user && (data = cookies.delete(:authentication_data, path: Discourse.base_uri("/"))) %> + <%- end %> diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 4aaf547e33..2c65450ae6 100644 --- a/spec/requests/application_controller_spec.rb +++ b/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 diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 204c3f1572..e604a89aba 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/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)