From 534e1b1b186b7dee3c90b2ee2e6342e7f4716087 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 28 Nov 2018 15:44:16 +0000 Subject: [PATCH] DEV: Introduce Auth::ManagedAuthenticator A generic implementation of Auth::Authenticator which stores data in the new UserAssociatedAccount model. This should help significantly reduce the duplicated logic across different auth providers. --- app/models/user_associated_account.rb | 24 +++ ...8115009_create_user_associated_accounts.rb | 18 +++ lib/auth.rb | 1 + lib/auth/managed_authenticator.rb | 105 +++++++++++++ .../auth/managed_authenticator_spec.rb | 148 ++++++++++++++++++ 5 files changed, 296 insertions(+) create mode 100644 app/models/user_associated_account.rb create mode 100644 db/migrate/20181108115009_create_user_associated_accounts.rb create mode 100644 lib/auth/managed_authenticator.rb create mode 100644 spec/components/auth/managed_authenticator_spec.rb diff --git a/app/models/user_associated_account.rb b/app/models/user_associated_account.rb new file mode 100644 index 00000000000..f23c4e069ae --- /dev/null +++ b/app/models/user_associated_account.rb @@ -0,0 +1,24 @@ +class UserAssociatedAccount < ActiveRecord::Base + belongs_to :user +end + +# == Schema Information +# +# Table name: user_associated_accounts +# +# id :bigint(8) not null, primary key +# provider_name :string not null +# provider_uid :string not null +# user_id :integer not null +# last_used :datetime not null +# info :jsonb not null +# credentials :jsonb not null +# extra :jsonb not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# associated_accounts_provider_uid (provider_name,provider_uid) UNIQUE +# associated_accounts_provider_user (provider_name,user_id) UNIQUE +# diff --git a/db/migrate/20181108115009_create_user_associated_accounts.rb b/db/migrate/20181108115009_create_user_associated_accounts.rb new file mode 100644 index 00000000000..f9994f77ff3 --- /dev/null +++ b/db/migrate/20181108115009_create_user_associated_accounts.rb @@ -0,0 +1,18 @@ +class CreateUserAssociatedAccounts < ActiveRecord::Migration[5.2] + def change + create_table :user_associated_accounts do |t| + t.string :provider_name, null: false + t.string :provider_uid, null: false + t.integer :user_id, null: false + t.datetime :last_used, null: false, default: -> { "CURRENT_TIMESTAMP" } + t.jsonb :info, null: false, default: {} + t.jsonb :credentials, null: false, default: {} + t.jsonb :extra, null: false, default: {} + + t.timestamps + end + + add_index :user_associated_accounts, [:provider_name, :provider_uid], unique: true, name: 'associated_accounts_provider_uid' + add_index :user_associated_accounts, [:provider_name, :user_id], unique: true, name: 'associated_accounts_provider_user' + end +end diff --git a/lib/auth.rb b/lib/auth.rb index 7d0c47db990..0e2ace56036 100644 --- a/lib/auth.rb +++ b/lib/auth.rb @@ -3,6 +3,7 @@ module Auth; end require_dependency 'auth/auth_provider' require_dependency 'auth/result' require_dependency 'auth/authenticator' +require_dependency 'auth/managed_authenticator' require_dependency 'auth/facebook_authenticator' require_dependency 'auth/open_id_authenticator' require_dependency 'auth/github_authenticator' diff --git a/lib/auth/managed_authenticator.rb b/lib/auth/managed_authenticator.rb new file mode 100644 index 00000000000..590efaacfe4 --- /dev/null +++ b/lib/auth/managed_authenticator.rb @@ -0,0 +1,105 @@ +class Auth::ManagedAuthenticator < Auth::Authenticator + def description_for_user(user) + info = UserAssociatedAccount.find_by(provider_name: name, user_id: user.id)&.info + return "" if info.nil? + info["email"] || info["nickname"] || info["name"] || "" + end + + # These three methods are designed to be overriden by child classes + def match_by_email + true + end + + def can_revoke? + true + end + + def can_connect_existing_user? + true + end + + def revoke(user, skip_remote: false) + association = UserAssociatedAccount.find_by(provider_name: name, user_id: user.id) + raise Discourse::NotFound if association.nil? + association.destroy! + true + 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 + + # 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 + end + + # Matching an account by email + if match_by_email && association.nil? && (user = User.find_by_email(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)) + 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[:info][:image]) + end + + result.email_valid = true if result.email + + 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["info"]["image"]) + end + + def retrieve_avatar(user, url) + return unless user && url + return if user.user_avatar.try(:custom_upload_id).present? + Jobs.enqueue(:download_avatar_from_url, url: url, user_id: user.id, override_gravatar: false) + end +end diff --git a/spec/components/auth/managed_authenticator_spec.rb b/spec/components/auth/managed_authenticator_spec.rb new file mode 100644 index 00000000000..300a4cc0f52 --- /dev/null +++ b/spec/components/auth/managed_authenticator_spec.rb @@ -0,0 +1,148 @@ +require 'rails_helper' + +describe Auth::ManagedAuthenticator do + let(:authenticator) { + Class.new(described_class) do + def name; "myauth" end + end.new + } + + let(:hash) { + { + provider: "myauth", + uid: "1234", + info: { + name: "Best Display Name", + email: "awesome@example.com", + nickname: "IAmGroot" + }, + credentials: { + token: "supersecrettoken" + }, + extra: { + raw_info: { + randominfo: "some info" + } + } + } + } + + describe 'after_authenticate' do + it 'can match account from an existing association' do + user = Fabricate(:user) + associated = UserAssociatedAccount.create!(user: user, provider_name: 'myauth', provider_uid: "1234") + result = authenticator.after_authenticate(hash) + + expect(result.user.id).to eq(user.id) + associated.reload + expect(associated.info["name"]).to eq("Best Display Name") + expect(associated.info["email"]).to eq("awesome@example.com") + expect(associated.credentials["token"]).to eq("supersecrettoken") + expect(associated.extra["raw_info"]["randominfo"]).to eq("some info") + end + + describe 'connecting to another user account' do + let(:user1) { Fabricate(:user) } + let(:user2) { Fabricate(:user) } + before { UserAssociatedAccount.create!(user: user1, provider_name: 'myauth', provider_uid: "1234") } + + it 'works by default' do + result = authenticator.after_authenticate(hash, existing_account: user2) + expect(result.user.id).to eq(user2.id) + expect(UserAssociatedAccount.exists?(user_id: user1.id)).to eq(false) + expect(UserAssociatedAccount.exists?(user_id: user2.id)).to eq(true) + end + + it 'does not work when disabled' do + authenticator = Class.new(described_class) do + def name; "myauth" end + def can_connect_existing_user?; false end + end.new + result = authenticator.after_authenticate(hash, existing_account: user2) + expect(result.user.id).to eq(user1.id) + expect(UserAssociatedAccount.exists?(user_id: user1.id)).to eq(true) + expect(UserAssociatedAccount.exists?(user_id: user2.id)).to eq(false) + end + end + + describe 'match by email' do + it 'works normally' do + user = Fabricate(:user) + result = authenticator.after_authenticate(hash.deep_merge(info: { email: user.email })) + expect(result.user.id).to eq(user.id) + expect(UserAssociatedAccount.find_by(provider_name: 'myauth', provider_uid: "1234").user_id).to eq(user.id) + end + + it 'works if there is already an association with the target account' do + user = Fabricate(:user, email: "awesome@example.com") + result = authenticator.after_authenticate(hash) + expect(result.user.id).to eq(user.id) + end + + it 'does not match if match_by_email is false' do + authenticator = Class.new(described_class) do + def name; "myauth" end + def match_by_email; false end + end.new + user = Fabricate(:user, email: "awesome@example.com") + result = authenticator.after_authenticate(hash) + expect(result.user).to eq(nil) + end + end + + context 'when no matching user' do + it 'returns the correct information' do + result = authenticator.after_authenticate(hash) + expect(result.user).to eq(nil) + expect(result.username).to eq("IAmGroot") + expect(result.email).to eq("awesome@example.com") + end + + it 'works if there is already an association with the target account' do + user = Fabricate(:user, email: "awesome@example.com") + result = authenticator.after_authenticate(hash) + expect(result.user.id).to eq(user.id) + end + end + end + + describe 'description_for_user' do + let(:user) { Fabricate(:user) } + + it 'returns empty string if no entry for user' do + expect(authenticator.description_for_user(user)).to eq("") + end + + it 'returns correct information' do + association = UserAssociatedAccount.create!(user: user, provider_name: 'myauth', provider_uid: "1234", info: { nickname: "somenickname", email: "test@domain.tld", name: "bestname" }) + expect(authenticator.description_for_user(user)).to eq('test@domain.tld') + association.update(info: { nickname: "somenickname", name: "bestname" }) + expect(authenticator.description_for_user(user)).to eq('somenickname') + association.update(info: { nickname: "bestname" }) + expect(authenticator.description_for_user(user)).to eq('bestname') + end + end + + describe 'revoke' do + let(:user) { Fabricate(:user) } + + it 'raises exception if no entry for user' do + expect { authenticator.revoke(user) }.to raise_error(Discourse::NotFound) + end + + context "with valid record" do + before do + UserAssociatedAccount.create!(user: user, provider_name: 'myauth', provider_uid: "1234", info: { name: "somename" }) + end + + it 'revokes correctly' do + expect(authenticator.description_for_user(user)).to eq("somename") + expect(authenticator.can_revoke?).to eq(true) + expect(authenticator.revoke(user)).to eq(true) + + expect(authenticator.description_for_user(user)).to eq("") + end + end + end + +end