From 2cae29f644848ea8ce85c37cfd5374a8130e278a Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Tue, 10 Aug 2021 14:09:30 +0100
Subject: [PATCH] DEV: Update associate_accounts_controller to use
 secure_session

This is much cleaner than using redis directly. It also opens the door to more complex association change flows which may happen during login.
---
 .../users/associate_accounts_controller.rb    | 10 +++--
 .../users/omniauth_callbacks_controller.rb    |  2 +-
 .../associate_accounts_controller_spec.rb     | 37 +++++++++++++------
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/app/controllers/users/associate_accounts_controller.rb b/app/controllers/users/associate_accounts_controller.rb
index 48cfe412e4f..2567a1b5dfd 100644
--- a/app/controllers/users/associate_accounts_controller.rb
+++ b/app/controllers/users/associate_accounts_controller.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class Users::AssociateAccountsController < ApplicationController
-  REDIS_PREFIX ||= "omniauth_reconnect"
+  SECURE_SESSION_PREFIX ||= "omniauth_reconnect"
 
   def connect_info
     auth = get_auth_hash
@@ -17,7 +17,7 @@ class Users::AssociateAccountsController < ApplicationController
 
   def connect
     auth = get_auth_hash
-    Discourse.redis.del "#{REDIS_PREFIX}_#{current_user&.id}_#{params[:token]}"
+    secure_session[self.class.key(params[:token])] = nil
 
     provider_name = auth.provider
     authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name }
@@ -34,9 +34,13 @@ class Users::AssociateAccountsController < ApplicationController
 
   def get_auth_hash
     token = params[:token]
-    json = Discourse.redis.get "#{REDIS_PREFIX}_#{current_user&.id}_#{token}"
+    json = secure_session[self.class.key(token)]
     raise Discourse::NotFound if json.nil?
 
     OmniAuth::AuthHash.new(JSON.parse(json))
   end
+
+  def self.key(token)
+    "#{SECURE_SESSION_PREFIX}_#{token}"
+  end
 end
diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb
index 190f87f0ad3..ff5d455857f 100644
--- a/app/controllers/users/omniauth_callbacks_controller.rb
+++ b/app/controllers/users/omniauth_callbacks_controller.rb
@@ -30,7 +30,7 @@ class Users::OmniauthCallbacksController < ApplicationController
     if session.delete(:auth_reconnect) && authenticator.can_connect_existing_user? && current_user
       # Save to redis, with a secret token, then redirect to confirmation screen
       token = SecureRandom.hex
-      Discourse.redis.setex "#{Users::AssociateAccountsController::REDIS_PREFIX}_#{current_user.id}_#{token}", 10.minutes, auth.to_json
+      secure_session.set "#{Users::AssociateAccountsController.key(token)}", auth.to_json, expires: 10.minutes
       return redirect_to "#{Discourse.base_path}/associate/#{token}"
     else
       DiscourseEvent.trigger(:before_auth, authenticator, auth, session, cookies, request)
diff --git a/spec/requests/associate_accounts_controller_spec.rb b/spec/requests/associate_accounts_controller_spec.rb
index 43a64ee26b0..0cf9eaee220 100644
--- a/spec/requests/associate_accounts_controller_spec.rb
+++ b/spec/requests/associate_accounts_controller_spec.rb
@@ -56,17 +56,6 @@ RSpec.describe Users::AssociateAccountsController do
       expect(data["provider_name"]).to eq("google_oauth2")
       expect(data["account_description"]).to eq("someemail@test.com")
 
-      # Request as different user, should not work
-      sign_in(user2)
-      get "#{uri.path}.json"
-      expect(response.status).to eq(404)
-
-      # Back to first user
-      sign_in(user)
-      get "#{uri.path}.json"
-      data = response.parsed_body
-      expect(data["provider_name"]).to eq("google_oauth2")
-
       # Make the connection
       events = DiscourseEvent.track_events { post "#{uri.path}.json" }
       expect(events.any? { |e| e[:event_name] == :before_auth }).to eq(true)
@@ -80,6 +69,32 @@ RSpec.describe Users::AssociateAccountsController do
       expect(response.status).to eq(404)
     end
 
+    it 'should only work within the current session' do
+      sign_in(user)
+
+      post "/auth/google_oauth2?reconnect=true"
+      expect(response.status).to eq(302)
+      expect(session[:auth_reconnect]).to eq(true)
+
+      OmniAuth.config.mock_auth[:google_oauth2].uid = "123456"
+      get "/auth/google_oauth2/callback.json"
+      expect(response.status).to eq(302)
+
+      expect(session[:current_user_id]).to eq(user.id) # Still logged in
+      expect(UserAssociatedAccount.count).to eq(0) # Reconnect has not yet happened
+
+      uri = URI.parse(response.redirect_url)
+      get "#{uri.path}.json"
+      data = response.parsed_body
+      expect(data["provider_name"]).to eq("google_oauth2")
+      expect(data["account_description"]).to eq("someemail@test.com")
+
+      cookies.delete "_forum_session"
+
+      get "#{uri.path}.json"
+      expect(response.status).to eq(404)
+    end
+
     it "returns the correct response for non-existent tokens" do
       get "/associate/12345678901234567890123456789012.json"
       expect(response.status).to eq(404)