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

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

config/settings.yml

@@ -46,3 +46,5 @@ login:
   oauth2_full_screen_login:
     default: false
     client: true
+  oauth2_allow_association_change:
+    default: false

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

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'])
+      if fetched_user_details = fetch_user_details(auth['credentials']['token'], auth['uid'])
-        user_details.merge!(fetched_user_details)
+        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]
+    super(auth, existing_account: existing_account)
-    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?
   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"
 

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)
-
+    end
-      # Check it doesn't interfere with the site setting
+    
+    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