Skip to content
Permalink
Browse files

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.
  • Loading branch information...
davidtaylorhq committed Dec 10, 2018
1 parent e06a898 commit 9db829134c6b006c4953015eb7fbca1d0678f2c7
@@ -0,0 +1,12 @@
module Jobs

class CleanUpAssociatedAccounts < Jobs::Scheduled
every 1.day

def execute(args)
UserAssociatedAccount.cleanup!
end

end

end
@@ -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

This comment has been minimized.

Copy link
@tgxworld

tgxworld Dec 10, 2018

Member

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.

This comment has been minimized.

Copy link
@davidtaylorhq

davidtaylorhq Dec 11, 2018

Author Member

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.

end
end

# == Schema Information
@@ -9,7 +15,7 @@ class UserAssociatedAccount < ActiveRecord::Base
# 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
@@ -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
@@ -26,77 +26,56 @@ def revoke(user, skip_remote: false)
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 @@ def retrieve_avatar(user, url)
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
@@ -29,6 +29,13 @@ def name
}
}

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 @@ def match_by_email

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)

This comment has been minimized.

Copy link
@tgxworld

tgxworld Dec 10, 2018

Member

O the assertion can be done within the block:

expect do
  expect(authenticator.after_authenticate(hash)).to eq(nil)
end.to change { ... }
}.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 @@ def match_by_email

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
@@ -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)

This comment has been minimized.

Copy link
@tgxworld

tgxworld Dec 10, 2018

Member

create! 😛

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

2 comments on commit 9db8291

@discoursebot

This comment has been minimized.

Copy link

replied Dec 10, 2018

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

https://meta.discourse.org/t/openid-connect-authentication-plugin/103632/6

@davidtaylorhq

This comment has been minimized.

Copy link
Member Author

replied Dec 11, 2018

Followed up in 3fedb2a

Thanks for the comments @tgxworld

Please sign in to comment.
You can’t perform that action at this time.