From beaeb0c4b2ba0d273d7707d2fbc79feb63083a81 Mon Sep 17 00:00:00 2001 From: Jeff Wong Date: Fri, 20 Mar 2020 11:03:38 -1000 Subject: [PATCH] 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 --- .../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..31c14c66ca7 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..7fa77c809a9 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 (data = cookies.delete(:authentication_data, path: Discourse.base_uri)) && !current_user %> + <%- end %> diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 4aaf547e33c..2c65450ae6f 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)