From 5ae9f35e814cdc160dcd4ad77d613a70c76a5228 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Sun, 28 Jul 2019 01:34:17 +1000 Subject: [PATCH] FEATURE: Migrate to ManagedAuthenticator (#21) This brings the plugin in-line with recent core improvements. Advantages include - Account-linking logic and storage is shared between all authentication providers - Optionally, users can be allowed to disconnect/reconnect their accounts - The 'last used' date of an association is recorded - Association metadata is recorded in the database for use in data explorer and other plugins Data migration will be performed automatically, and all existing functionality is maintained. --- config/locales/server.en.yml | 1 + config/settings.yml | 2 + ...724055909_move_to_managed_authenticator.rb | 20 +++++ plugin.rb | 85 ++++++++----------- spec/plugin_spec.rb | 45 ++++++---- 5 files changed, 85 insertions(+), 68 deletions(-) create mode 100644 db/migrate/20190724055909_move_to_managed_authenticator.rb diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 9e0cb29..855c20b 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -28,6 +28,7 @@ en: oauth2_scope: "When authorizing request this scope" oauth2_button_title: "The text for the OAuth2 button" oauth2_full_screen_login: "Use main browser window instead of popup for login" + oauth2_allow_association_change: Allow users to disconnect and reconnect their Discourse accounts from the OAuth2 provider errors: oauth2_fetch_user_details: "oauth2_callback_user_id_path must be present to disable oauth2_fetch_user_details" diff --git a/config/settings.yml b/config/settings.yml index 50470e7..70b4c8f 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -46,3 +46,5 @@ login: oauth2_full_screen_login: default: false client: true + oauth2_allow_association_change: + default: false diff --git a/db/migrate/20190724055909_move_to_managed_authenticator.rb b/db/migrate/20190724055909_move_to_managed_authenticator.rb new file mode 100644 index 0000000..8db23ca --- /dev/null +++ b/db/migrate/20190724055909_move_to_managed_authenticator.rb @@ -0,0 +1,20 @@ +class MoveToManagedAuthenticator < ActiveRecord::Migration[5.2] + def up + execute <<~SQL + INSERT INTO user_associated_accounts ( + provider_name, + provider_uid, + user_id, + created_at, + updated_at + ) SELECT + 'oauth2_basic', + replace(key, 'oauth2_basic_user_', ''), + (value::json->>'user_id')::integer, + CURRENT_TIMESTAMP, + CURRENT_TIMESTAMP + FROM plugin_store_rows + WHERE plugin_name = 'oauth2_basic' + SQL + end +end diff --git a/plugin.rb b/plugin.rb index 01ab810..2dd8476 100644 --- a/plugin.rb +++ b/plugin.rb @@ -46,10 +46,22 @@ class ::OmniAuth::Strategies::Oauth2Basic < ::OmniAuth::Strategies::OAuth2 end end -class OAuth2BasicAuthenticator < ::Auth::OAuth2Authenticator +class OAuth2BasicAuthenticator < Auth::ManagedAuthenticator + def name + 'oauth2_basic' + end + + def can_revoke? + SiteSetting.oauth2_allow_association_change + end + + def can_connect_existing_user? + SiteSetting.oauth2_allow_association_change + end + def register_middleware(omniauth) omniauth.provider :oauth2_basic, - name: 'oauth2_basic', + name: name, setup: lambda { |env| opts = env['omniauth.strategy'].options opts[:client_id] = SiteSetting.oauth2_client_id @@ -110,7 +122,7 @@ class OAuth2BasicAuthenticator < ::Auth::OAuth2Authenticator bearer_token = "Bearer #{token}" connection = Excon.new( user_json_url, - :headers => { 'Authorization' => bearer_token, 'Accept' => 'application/json' } + headers: { 'Authorization' => bearer_token, 'Accept' => 'application/json' } ) user_json_response = connection.request(method: user_json_method) @@ -136,62 +148,35 @@ class OAuth2BasicAuthenticator < ::Auth::OAuth2Authenticator end end - def after_authenticate(auth) + def primary_email_verified?(auth) + auth['info']['email_verified'] || + SiteSetting.oauth2_email_verified + end + + def always_update_user_email? + SiteSetting.oauth2_overrides_email + end + + def after_authenticate(auth, existing_account: nil) log("after_authenticate response: \n\ncreds: #{auth['credentials'].to_hash}\nuid: #{auth['uid']}\ninfo: #{auth['info'].to_hash}\nextra: #{auth['extra'].to_hash}") - result = Auth::Result.new - token = auth['credentials']['token'] - - user_details = {} - user_details[:user_id] = auth['uid'] if auth['uid'] - ['name', 'username', 'email', 'email_verified', 'avatar'].each do |key| - user_details[key.to_sym] = auth['info'][key] if auth['info'][key] - end - if SiteSetting.oauth2_fetch_user_details? - if fetched_user_details = fetch_user_details(token, auth['uid']) - user_details.merge!(fetched_user_details) + if fetched_user_details = fetch_user_details(auth['credentials']['token'], auth['uid']) + auth['uid'] = fetched_user_details[:user_id] if fetched_user_details[:user_id] + auth['info']['nickname'] = fetched_user_details[:username] if fetched_user_details[:username] + auth['info']['image'] = fetched_user_details[:avatar] if fetched_user_details[:avatar] + ['name', 'email', 'email_verified'].each do |property| + auth['info'][property] = fetched_user_details[property.to_sym] if fetched_user_details[property.to_sym] + end else + result = Auth::Result.new result.failed = true result.failed_reason = I18n.t("login.authenticator_error_fetch_user_details") return result end end - result.name = user_details[:name] - result.username = user_details[:username] - result.email = user_details[:email] - result.email_valid = result.email.present? && (user_details[:email_verified] || SiteSetting.oauth2_email_verified?) - avatar_url = user_details[:avatar] - - current_info = ::PluginStore.get("oauth2_basic", "oauth2_basic_user_#{user_details[:user_id]}") - if current_info - result.user = User.where(id: current_info[:user_id]).first - result.user&.update!(email: result.email) if SiteSetting.oauth2_overrides_email && result.email - elsif result.email_valid - result.user = User.find_by_email(result.email) - if result.user && user_details[:user_id] - ::PluginStore.set("oauth2_basic", "oauth2_basic_user_#{user_details[:user_id]}", user_id: result.user.id) - end - end - - download_avatar(result.user, avatar_url) - - result.extra_data = { oauth2_basic_user_id: user_details[:user_id], avatar_url: avatar_url } - result - end - - def after_create_account(user, auth) - ::PluginStore.set("oauth2_basic", "oauth2_basic_user_#{auth[:extra_data][:oauth2_basic_user_id]}", user_id: user.id) - download_avatar(user, auth[:extra_data][:avatar_url]) - end - - def download_avatar(user, avatar_url) - Jobs.enqueue(:download_avatar_from_url, - url: avatar_url, - user_id: user.id, - override_gravatar: SiteSetting.sso_overrides_avatar - ) if user && avatar_url.present? + super(auth, existing_account: existing_account) end def enabled? @@ -200,7 +185,7 @@ class OAuth2BasicAuthenticator < ::Auth::OAuth2Authenticator end auth_provider title_setting: "oauth2_button_title", - authenticator: OAuth2BasicAuthenticator.new('oauth2_basic'), + authenticator: OAuth2BasicAuthenticator.new, message: "OAuth2", full_screen_login_setting: "oauth2_full_screen_login" diff --git a/spec/plugin_spec.rb b/spec/plugin_spec.rb index 23a6cac..03faa79 100644 --- a/spec/plugin_spec.rb +++ b/spec/plugin_spec.rb @@ -30,12 +30,14 @@ require_relative '../plugin.rb' describe OAuth2BasicAuthenticator do context 'after_authenticate' do let(:user) { Fabricate(:user) } - let(:authenticator) { OAuth2BasicAuthenticator.new('oauth2_basic') } + let(:authenticator) { OAuth2BasicAuthenticator.new } let(:auth) do - { 'credentials' => { 'token': 'token' }, + OmniAuth::AuthHash.new({ 'provider' => 'oauth2_basic', + 'credentials' => { 'token': 'token' }, + 'uid' => '123456789', 'info' => { id: 'id' }, - 'extra' => {} } + 'extra' => {} }) end before(:each) do @@ -73,17 +75,19 @@ describe OAuth2BasicAuthenticator do it 'validates user email if provider has verified' do SiteSetting.oauth2_email_verified = false - - # Check it's working authenticator.stubs(:fetch_user_details).returns(email: user.email, email_verified: true) result = authenticator.after_authenticate(auth) expect(result.email_valid).to eq(true) - + end + + it 'doesnt validate user email if provider hasnt verified' do + SiteSetting.oauth2_email_verified = false authenticator.stubs(:fetch_user_details).returns(email: user.email, email_verified: nil) result = authenticator.after_authenticate(auth) expect(result.email_valid).to eq(false) - - # Check it doesn't interfere with the site setting + end + + it 'doesnt affect the site setting' do SiteSetting.oauth2_email_verified = true authenticator.stubs(:fetch_user_details).returns(email: user.email, email_verified: false) result = authenticator.after_authenticate(auth) @@ -135,7 +139,11 @@ describe OAuth2BasicAuthenticator do end context 'avatar downloading' do - before { SiteSetting.queue_jobs = true } + before do + SiteSetting.queue_jobs = true + SiteSetting.oauth2_fetch_user_details = true + SiteSetting.oauth2_email_verified = true + end let(:job_klass) { Jobs::DownloadAvatarFromUrl } @@ -186,7 +194,7 @@ describe OAuth2BasicAuthenticator do end it 'can walk json' do - authenticator = OAuth2BasicAuthenticator.new('oauth2_basic') + authenticator = OAuth2BasicAuthenticator.new json_string = '{"user":{"id":1234,"email":{"address":"test@example.com"}}}' SiteSetting.oauth2_json_email_path = 'user.email.address' result = authenticator.json_walk({}, JSON.parse(json_string), :email) @@ -195,7 +203,7 @@ describe OAuth2BasicAuthenticator do end it 'can walk json that contains an array' do - authenticator = OAuth2BasicAuthenticator.new('oauth2_basic') + authenticator = OAuth2BasicAuthenticator.new json_string = '{"email":"test@example.com","identities":[{"user_id":"123456789","provider":"auth0","isSocial":false}]}' SiteSetting.oauth2_json_user_id_path = 'identities.[].user_id' result = authenticator.json_walk({}, JSON.parse(json_string), :user_id) @@ -204,7 +212,7 @@ describe OAuth2BasicAuthenticator do end it 'can walk json and handle an empty array' do - authenticator = OAuth2BasicAuthenticator.new('oauth2_basic') + authenticator = OAuth2BasicAuthenticator.new json_string = '{"email":"test@example.com","identities":[]}' SiteSetting.oauth2_json_user_id_path = 'identities.[].user_id' result = authenticator.json_walk({}, JSON.parse(json_string), :user_id) @@ -213,7 +221,7 @@ describe OAuth2BasicAuthenticator do end it 'can walk json and download avatar' do - authenticator = OAuth2BasicAuthenticator.new('oauth2_basic') + authenticator = OAuth2BasicAuthenticator.new json_string = '{"user":{"avatar":"http://example.com/1.png"}}' SiteSetting.oauth2_json_avatar_path = 'user.avatar' result = authenticator.json_walk({}, JSON.parse(json_string), :avatar) @@ -224,10 +232,11 @@ describe OAuth2BasicAuthenticator do context 'token_callback' do let(:user) { Fabricate(:user) } let(:strategy) { OmniAuth::Strategies::Oauth2Basic.new({}) } - let(:authenticator) { OAuth2BasicAuthenticator.new('oauth2_basic') } + let(:authenticator) { OAuth2BasicAuthenticator.new } let(:auth) do - { + OmniAuth::AuthHash.new({ + 'provider' => 'oauth2_basic', 'credentials' => { 'token' => 'token' }, @@ -237,7 +246,7 @@ describe OAuth2BasicAuthenticator do "email" => 'sammy@digitalocean.com' }, 'extra' => {} - } + }) end let(:access_token) do @@ -273,7 +282,7 @@ describe OAuth2BasicAuthenticator do authenticator.stubs(:fetch_user_details).returns(email: 'sammy@digitalocean.com') result = authenticator.after_authenticate(auth) - expect(result.extra_data[:oauth2_basic_user_id]).to eq 'e028b1b918853eca7fba208a9d7e9d29a6e93c57' + expect(result.extra_data[:uid]).to eq 'e028b1b918853eca7fba208a9d7e9d29a6e93c57' expect(result.name).to eq 'Sammy the Shark' expect(result.email).to eq 'sammy@digitalocean.com' end @@ -282,7 +291,7 @@ describe OAuth2BasicAuthenticator do SiteSetting.oauth2_fetch_user_details = false result = authenticator.after_authenticate(auth) - expect(result.extra_data[:oauth2_basic_user_id]).to eq 'e028b1b918853eca7fba208a9d7e9d29a6e93c57' + expect(result.extra_data[:uid]).to eq 'e028b1b918853eca7fba208a9d7e9d29a6e93c57' expect(result.name).to eq 'Sammy the Shark' expect(result.email).to eq 'sammy@digitalocean.com' end