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.

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

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

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

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])
+    association = UserAssociatedAccount.find_or_initialize_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid])
-    result.user = association&.user
 
     # Reconnecting to existing account
-    if can_connect_existing_user? && existing_account && (association.nil? || existing_account.id != association.user_id)
+    if can_connect_existing_user? && existing_account && (association.user.nil? || existing_account.id != association.user_id)
-      association.destroy! if association
+      association.user = existing_account
-      association = nil
-      result.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
+      association.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))
     end
 
     # Update all the metadata in the association:
-    if association
+    association.info = auth_token[:info] || {}
-      association.update!(
+    association.credentials = auth_token[:credentials] || {}
-        info: auth_token[:info] || {},
+    association.extra = auth_token[:extra] || {}
-        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
 
+    # 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]
+    auth_token = auth[:extra_data]
-    create_association!(data.merge(user: user))
+    association = UserAssociatedAccount.find_or_initialize_by(provider_name: auth_token[:provider], provider_uid: auth_token[:uid])
-    retrieve_avatar(user, data.dig(:info, :image))
+    association.user = user
-    retrieve_profile(user, data[:info])
+    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]
+    bio = info["description"]
-    location = info[:location]
+    location = info["location"]
 
     if bio || location
       profile = user.user_profile

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_authenticate(hash)
+        result = nil
+        expect {
+          result = authenticator.after_authenticate(hash)
+        }.to change { UserAssociatedAccount.count }.by(1)
         expect(result.user).to eq(nil)
         expect(result.username).to eq("IAmGroot")
         expect(result.email).to eq("awesome@example.com")
+        expect(UserAssociatedAccount.last.user).to eq(nil)
+        expect(UserAssociatedAccount.last.info["nickname"]).to eq("IAmGroot")
       end
 
       it 'works if there is already an association with the target account' do
@@ -170,25 +182,34 @@ describe Auth::ManagedAuthenticator do
 
     describe "avatar on create" do
       let(:user) { Fabricate(:user) }
+      let!(:association) { UserAssociatedAccount.create!(provider_name: 'myauth', provider_uid: "1234") }
+
       it "doesn't schedule with no image" do
-        expect { result = authenticator.after_create_account(user, extra_data: hash) }
+        expect { result = authenticator.after_create_account(user, extra_data: create_hash) }
           .to change { Jobs::DownloadAvatarFromUrl.jobs.count }.by(0)
       end
 
       it "schedules with image" do
-        expect { result = authenticator.after_create_account(user, extra_data: hash.deep_merge(info: { image: "https://some.domain/image.jpg" })) }
+        association.info["image"] = "https://some.domain/image.jpg"
+        association.save!
+        expect { result = authenticator.after_create_account(user, extra_data: create_hash) }
           .to change { Jobs::DownloadAvatarFromUrl.jobs.count }.by(1)
       end
     end
 
     describe "profile on create" do
       let(:user) { Fabricate(:user) }
+      let!(:association) { UserAssociatedAccount.create!(provider_name: 'myauth', provider_uid: "1234") }
+
       it "doesn't explode without profile" do
-        authenticator.after_create_account(user, extra_data: hash)
+        authenticator.after_create_account(user, extra_data: create_hash)
       end
 
       it "works with profile" do
-        authenticator.after_create_account(user, extra_data: hash.deep_merge(info: { location: "DiscourseVille", description: "Online forum expert" }))
+        association.info["location"] = "DiscourseVille"
+        association.info["description"] = "Online forum expert"
+        association.save!
+        authenticator.after_create_account(user, extra_data: create_hash)
         expect(user.user_profile.bio_raw).to eq("Online forum expert")
         expect(user.user_profile.location).to eq("DiscourseVille")
       end

spec/jobs/clean_up_associated_accounts_spec.rb

@@ -0,0 +1,17 @@
+require 'rails_helper'
+
+describe Jobs::CleanUpAssociatedAccounts do
+  subject { Jobs::CleanUpAssociatedAccounts.new.execute({}) }
+
+  it "deletes the correct records" do
+    freeze_time
+
+    last_week = UserAssociatedAccount.create(provider_name: "twitter", provider_uid: "1", updated_at: 7.days.ago)
+    today = UserAssociatedAccount.create(provider_name: "twitter", provider_uid: "12", updated_at: 12.hours.ago)
+    connected = UserAssociatedAccount.create(provider_name: "twitter", provider_uid: "123", user: Fabricate(:user), updated_at: 12.hours.ago)
+
+    expect { subject }.to change { UserAssociatedAccount.count }.by(-1)
+    expect(UserAssociatedAccount.all).to contain_exactly(today, connected)
+  end
+
+end