FIX: Use database to persist metadata during social registration (#6750)

FIX: Use database to persist metadata during social registration (#6750)

Previously was using the cookie_store, which is limited to 4kb. This caused issues for providers sending large volumes of metadata about a user.

From 9db829134c6b006c4953015eb7fbca1d0678f2c7 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Mon, 10 Dec 2018 15:10:06 +0000
Subject: [PATCH] FIX: Use database to persist metadata during social
 registration (#6750)

Previously was using the cookie_store, which is limited to 4kb. This caused issues for providers sending large volumes of metadata about a user.

diff --git a/app/jobs/scheduled/clean_up_associated_accounts.rb b/app/jobs/scheduled/clean_up_associated_accounts.rb
new file mode 100644
index 0000000..2d589cb
--- /dev/null
+++ b/app/jobs/scheduled/clean_up_associated_accounts.rb
@@ -0,0 +1,12 @@
+module Jobs
+
+  class CleanUpAssociatedAccounts < Jobs::Scheduled
+    every 1.day
+
+    def execute(args)
+      UserAssociatedAccount.cleanup!
+    end
+
+  end
+
+end
diff --git a/app/models/user_associated_account.rb b/app/models/user_associated_account.rb
index f23c4e0..ef85326 100644
--- a/app/models/user_associated_account.rb
+++ b/app/models/user_associated_account.rb
@@ -1,5 +1,11 @@
 class UserAssociatedAccount < ActiveRecord::Base
   belongs_to :user
+
+  def self.cleanup!
+    # This happens when a user starts the registration flow, but doesn't complete it
+    # Keeping the rows doesn't cause any technical issue, but we shouldn't store PII unless it's attached to a user
+    self.where("user_id IS NULL AND updated_at < ?", 1.day.ago).destroy_all
+  end
 end
 
 # == Schema Information
@@ -9,7 +15,7 @@ end
 #  id            :bigint(8)        not null, primary key
 #  provider_name :string           not null
 #  provider_uid  :string           not null
-#  user_id       :integer          not null
+#  user_id       :integer
 #  last_used     :datetime         not null
 #  info          :jsonb            not null
 #  credentials   :jsonb            not null
diff --git a/db/migrate/20181210122522_remove_not_null_user_associated_account_user.rb b/db/migrate/20181210122522_remove_not_null_user_associated_account_user.rb
new file mode 100644
index 0000000..b961341
--- /dev/null
+++ b/db/migrate/20181210122522_remove_not_null_user_associated_account_user.rb
@@ -0,0 +1,10 @@
+class RemoveNotNullUserAssociatedAccountUser < ActiveRecord::Migration[5.2]
+  def change
+    begin
+      Migration::SafeMigrate.disable!
+      change_column_null :user_associated_accounts, :user_id, true
+    ensure
+      Migration::SafeMigrate.enable!
+    end
+  end
+end
diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb
index 666c5a1..b7218b9 100644
--- a/lib/auth/managed_authenticator.rb
+++ b/lib/auth/managed_authenticator.rb
@@ -26,77 +26,56 @@ class Auth::ManagedAuthenticator < Auth::Authenticator
   end
 
   def after_authenticate(auth_token, existing_account: nil)
-    result = Auth::Result.new
-
-    # Store all the metadata for later, in case the `after_create_account` hook is used
-    result.extra_data = {
-      provider: auth_token[:provider],
-      uid: auth_token[:uid],
-      info: auth_token[:info],
-      extra: auth_token[:extra],
-      credentials: auth_token[:credentials]
-    }
-
-    # Build the Auth::Result object
-    info = auth_token[:info]
-    result.email = email = info[:email]
-    result.name = name = "#{info[:first_name]} #{info[:last_name]}"
-    result.username = info[:nickname]
-
     # Try and find an association for this account
-    association = UserAssociatedAccount.find_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid])
-    result.user = association&.user
+    association = UserAssociatedAccount.find_or_initialize_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid])
 
     # Reconnecting to existing account
-    if can_connect_existing_user? && existing_account && (association.nil? || existing_account.id != association.user_id)
-      association.destroy! if association
-      association = nil
-      result.user = existing_account
+    if can_connect_existing_user? && existing_account && (association.user.nil? || existing_account.id != association.user_id)
+      association.user = existing_account
     end
 
     # Matching an account by email
-    if match_by_email && association.nil? && result.user.nil? && (user = User.find_by_email(email))
+    if match_by_email && association.user.nil? && (user = User.find_by_email(auth_token.dig(:info, :email)))
       UserAssociatedAccount.where(user: user, provider_name: auth_token[:provider]).destroy_all # Destroy existing associations for the new user
-      result.user = user
-    end
-
-    # Add the association to the database if it doesn't already exist
-    if association.nil? && result.user
-      association = create_association!(result.extra_data.merge(user: result.user))
+      association.user = user
     end
 
     # Update all the metadata in the association:
-    if association
-      association.update!(
-        info: auth_token[:info] || {},
-        credentials: auth_token[:credentials] || {},
-        extra: auth_token[:extra] || {}
-      )
-      retrieve_avatar(result.user, auth_token.dig(:info, :image))
-      retrieve_profile(result.user, auth_token[:info])
-    end
+    association.info = auth_token[:info] || {}
+    association.credentials = auth_token[:credentials] || {}
+    association.extra = auth_token[:extra] || {}
 
+    # Save to the DB. Do this even if we don't have a user - it might be linked up later in after_create_account
+    association.save!
+
+    # Update avatar/profile
+    retrieve_avatar(association.user, association.info["image"])
+    retrieve_profile(association.user, association.info)
+
+    # Build the Auth::Result object
+    result = Auth::Result.new
+    info = auth_token[:info]
+    result.email = info[:email]
+    result.name = "#{info[:first_name]} #{info[:last_name]}"
+    result.username = info[:nickname]
     result.email_valid = true if result.email
+    result.extra_data = {
+      provider: auth_token[:provider],
+      uid: auth_token[:uid]
+    }
+    result.user = association.user
 
     result
   end
 
-  def create_association!(hash)
-    association = UserAssociatedAccount.create!(
-      user: hash[:user],
-      provider_name: hash[:provider],
-      provider_uid: hash[:uid],
-      info: hash[:info] || {},
-      credentials: hash[:credentials] || {},
-      extra: hash[:extra] || {}
-    )
-  end
-
   def after_create_account(user, auth)
-    data = auth[:extra_data]
-    create_association!(data.merge(user: user))
-    retrieve_avatar(user, data.dig(:info, :image))
-    retrieve_profile(user, data[:info])
+    auth_token = auth[:extra_data]
+    association = UserAssociatedAccount.find_or_initialize_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid])
+    association.user = user
+    association.save!
+
+    retrieve_avatar(user, association.info["image"])
+    retrieve_profile(user, association.info)
   end
 
   def retrieve_avatar(user, url)
@@ -108,8 +87,8 @@ class Auth::ManagedAuthenticator < Auth::Authenticator
   def retrieve_profile(user, info)
     return unless user
 
-    bio = info[:description]
-    location = info[:location]
+    bio = info["description"]
+    location = info["location"]
 
     if bio || location
       profile = user.user_profile
diff --git a/spec/components/auth/managed_authenticator_spec.rb b/spec/components/auth/managed_authenticator_spec.rb
index e02a45d..870536b 100644
--- a/spec/components/auth/managed_authenticator_spec.rb
+++ b/spec/components/auth/managed_authenticator_spec.rb
@@ -29,6 +29,13 @@ describe Auth::ManagedAuthenticator do
     }
   }
 
+  let(:create_hash) {
+    {
+      provider: "myauth",
+      uid: "1234"
+    }
+  }
+
   describe 'after_authenticate' do
     it 'can match account from an existing association' do
       user = Fabricate(:user)
@@ -110,10 +117,15 @@ describe Auth::ManagedAuthenticator do
 
     context 'when no matching user' do
       it 'returns the correct information' do
-        result = authenticator.after_auth

GitHub

This commit has been mentioned on Discourse Meta. There might be relevant details there:

Will there be alot of records to clean up? Note that destroy_all doesn’t load records in batches, it loads everything into memory at once.

2 Likes

O the assertion can be done within the block:

expect do
  expect(authenticator.after_authenticate(hash)).to eq(nil)
end.to change { ... }
2 Likes

create! :stuck_out_tongue:

2 Likes

Depends how many people get bored halfway through the signup flow. It will probably be fine, but I’ll switch to delete_all for safety - we can always switch back to destroy if we start using activerecord callbacks on this model.

1 Like

Followed up in DEV: Style and performance improvements · discourse/discourse@3fedb2a · GitHub

Thanks for the comments @tgxworld

4 Likes