From 3189dab622dead97d73651af99f2cf23ed6e188f Mon Sep 17 00:00:00 2001 From: Jeff Wong Date: Sat, 21 Mar 2020 13:24:49 -0700 Subject: [PATCH] FIX: correctly remove authentication_data cookie on oauth login flow 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 881fb3e9eca..4bcd8c8f5fd 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 6c1f3b3074a..0b037d521f8 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 f9334291142..4d551942f6b 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 204c3f1572c..e604a89aba4 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)