DEV: Improve robustness of associate_accounts_controller

This handles a few edge cases which are extremely rare (due to the UI layout), but still technically possible:

- Ensure users are authenticated before attempting association.

- Add a message and logic for when a user already has an association for a given auth provider.

app/assets/javascripts/discourse/app/routes/associate-account.js

@@ -3,9 +3,14 @@ import { ajax } from "discourse/lib/ajax";
 import { next } from "@ember/runloop";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import showModal from "discourse/lib/show-modal";
+import cookie from "discourse/lib/cookie";
 
 export default DiscourseRoute.extend({
-  beforeModel() {
+  beforeModel(transition) {
+    if (!this.currentUser) {
+      cookie("destination_url", transition.intent.url);
+      return this.replaceWith("login");
+    }
     const params = this.paramsFor("associate-account");
     this.replaceWith(`preferences.account`, this.currentUser).then(() =>
       next(() =>

app/assets/javascripts/discourse/app/templates/modal/associate-account-confirm.hbs

@@ -10,6 +10,15 @@
     </div>
   {{/if}}
 
+  {{#if model.existing_account_description}}
+    <p>
+      {{i18n "user.associated_accounts.confirm_description.disconnect"
+          provider=(i18n (concat "login." model.provider_name ".name"))
+          account_description=model.existing_account_description}}
+    </p>
+  {{/if}}
+
+  <p>
     {{#if model.account_description}}
       {{i18n "user.associated_accounts.confirm_description.account_specific"
           provider=(i18n (concat "login." model.provider_name ".name"))
@@ -18,6 +27,7 @@
       {{i18n "user.associated_accounts.confirm_description.generic"
           provider=(i18n (concat "login." model.provider_name ".name"))}}
     {{/if}}
+  </p>
 {{/d-modal-body}}
 
 <div class="modal-footer">

app/controllers/users/associate_accounts_controller.rb

@@ -3,42 +3,52 @@
 class Users::AssociateAccountsController < ApplicationController
   SECURE_SESSION_PREFIX ||= "omniauth_reconnect"
 
+  before_action :ensure_logged_in
+
   def connect_info
-    auth = get_auth_hash
-
-    provider_name = auth.provider
-    authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name }
-    raise Discourse::InvalidAccess.new(I18n.t('authenticator_not_found')) if authenticator.nil?
-
-    account_description = authenticator.description_for_auth_hash(auth)
-
-    render json: { token: params[:token], provider_name: provider_name, account_description: account_description }
+    account_description = authenticator.description_for_auth_hash(auth_hash)
+    existing_account_description = authenticator.description_for_user(current_user).presence
+    render json: {
+      token: params[:token],
+      provider_name: auth_hash.provider,
+      account_description: account_description,
+      existing_account_description: existing_account_description
+    }
   end
 
   def connect
-    auth = get_auth_hash
-    secure_session[self.class.key(params[:token])] = nil
+    if authenticator.description_for_user(current_user).present? && authenticator.can_revoke?
+      authenticator.revoke(current_user)
+    end
 
-    provider_name = auth.provider
-    authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name }
-    raise Discourse::InvalidAccess.new(I18n.t('authenticator_not_found')) if authenticator.nil?
-
-    DiscourseEvent.trigger(:before_auth, authenticator, auth, session, cookies, request)
+    DiscourseEvent.trigger(:before_auth, authenticator, auth_hash, session, cookies, request)
     auth_result = authenticator.after_authenticate(auth, existing_account: current_user)
     DiscourseEvent.trigger(:after_auth, authenticator, auth_result, session, cookies, request)
 
+    secure_session[self.class.key(params[:token])] = nil
+
     render json: success_json
   end
 
   private
 
-  def get_auth_hash
+  def auth_hash
+    @auth_hash ||= begin
       token = params[:token]
       json = secure_session[self.class.key(token)]
       raise Discourse::NotFound if json.nil?
 
       OmniAuth::AuthHash.new(JSON.parse(json))
     end
+  end
+
+  def authenticator
+    provider_name = auth_hash.provider
+    authenticator = Discourse.enabled_authenticators.find { |a| a.name == provider_name }
+    raise Discourse::InvalidAccess.new(I18n.t('authenticator_not_found')) if authenticator.nil?
+    raise Discourse::InvalidAccess.new(I18n.t('authenticator_no_connect')) if !authenticator.can_connect_existing_user?
+    authenticator
+  end
 
   def self.key(token)
     "#{SECURE_SESSION_PREFIX}_#{token}"

config/locales/client.en.yml

@@ -1359,6 +1359,7 @@ en:
         not_connected: "(not connected)"
         confirm_modal_title: "Connect %{provider} Account"
         confirm_description:
+          disconnect: "Your existing %{provider} account '%{account_description}' will be disconnected."
           account_specific: "Your %{provider} account '%{account_description}' will be used for authentication."
           generic: "Your %{provider} account will be used for authentication."
 

config/locales/server.en.yml

@@ -286,6 +286,7 @@ en:
   not_found: "The requested URL or resource could not be found."
   invalid_access: "You are not permitted to view the requested resource."
   authenticator_not_found: "Authentication method does not exist, or has been disabled."
+  authenticator_no_connect: "This authentication provider does not allow connection to an existing forum account."
   invalid_api_credentials: "You are not permitted to view the requested resource. The API username or key is invalid."
   provider_not_enabled: "You are not permitted to view the requested resource. The authentication provider is not enabled."
   provider_not_found: "You are not permitted to view the requested resource. The authentication provider does not exist."

spec/requests/associate_accounts_controller_spec.rb

@@ -96,11 +96,19 @@ RSpec.describe Users::AssociateAccountsController do
     end
 
     it "returns the correct response for non-existent tokens" do
+      sign_in(user)
+
       get "/associate/12345678901234567890123456789012.json"
       expect(response.status).to eq(404)
 
       get "/associate/shorttoken.json"
       expect(response.status).to eq(404)
     end
+
+    it "requires login" do
+      # XHR should 403
+      get "/associate/#{SecureRandom.hex}.json"
+      expect(response.status).to eq(403)
+    end
   end
 end