SECURITY: Emails from microsoft are not verified (#72)

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>

config/locales/server.en.yml

@@ -3,3 +3,4 @@ en:
     microsoft_auth_enabled: 'Allow users to authenticate using Microsoft?'
     microsoft_auth_client_id: 'Microsoft App ID/Client ID (NOT the Secret ID) - to setup visit <a href="https://apps.dev.microsoft.com/#/appList">https://apps.dev.microsoft.com/#/appList</a>)'
     microsoft_auth_client_secret: 'Microsoft Secret Password (use the Value, NOT the Secret ID)'
+    microsoft_auth_email_verified: 'Treat all Microsoft emails as verified by default. WARNING: Only enable this if you are sure that all your users have verified emails.'

config/settings.yml

@@ -7,4 +7,6 @@ plugins:
     default: ''
   microsoft_auth_client_secret:
     client: false
-    default: ''
+    default: ''
+  microsoft_auth_email_verified:
+    default: false

lib/microsoft_auth_revoker.rb

@@ -0,0 +1,49 @@
+# frozen_string_literal: true
+
+class MicrosoftAuthRevoker
+  def self.revoke
+    # Deactive users using microsoft as an authentication provider
+    DB.exec <<~SQL
+      UPDATE users SET active = false
+      WHERE users.id IN (#{microsoft_user_associated_accounts_sql})
+    SQL
+
+    # Log out all users using microsoft as an authentication provider
+    log_out_users
+
+    # Revoke user API keys for users using microsoft as an authentication provider
+    DB.exec <<~SQL
+    UPDATE user_api_keys
+    SET revoked_at = NOW()
+    WHERE user_id IN (#{microsoft_user_associated_accounts_sql})
+    SQL
+
+    # Revoke API keys that are created by users using microsoft as an authentication provider
+    DB.exec <<~SQL
+    UPDATE api_keys
+    SET revoked_at = NOW()
+    WHERE created_by_id IN (#{microsoft_user_associated_accounts_sql})
+    SQL
+
+    # Remove microsoft as an authentication provider for all users
+    DB.exec <<~SQL
+    DELETE FROM user_associated_accounts
+    WHERE provider_name = 'microsoft_office365'
+    SQL
+  end
+
+  def self.log_out_users
+    DB.exec <<~SQL
+        DELETE FROM user_auth_tokens
+        WHERE user_id IN (#{microsoft_user_associated_accounts_sql})
+        SQL
+  end
+
+  def self.microsoft_user_associated_accounts_sql
+    <<~SQL
+    SELECT user_id
+    FROM user_associated_accounts
+    WHERE provider_name = 'microsoft_office365'
+    SQL
+  end
+end

lib/tasks/microsoft_auth.rake

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+require_relative "../microsoft_auth_revoker"
+
+desc <<~DESC
+A rake task to remove microsoft as an authentication provider, log out, deactivate and remove all API keys for all
+users accounts that have used Microsoft as an authentication provider.
+DESC
+task "microsoft_auth:revoke" => :environment do
+  MicrosoftAuthRevoker.revoke
+end
+
+task "microsoft_auth:log_out_users" => :environment do
+  MicrosoftAuthRevoker.log_out_users
+end

plugin.rb

@@ -3,7 +3,7 @@
 # name: discourse-microsoft-auth
 # about: Enable Login via Microsoft Identity Platform (Office 365 / Microsoft 365 Accounts)
 # meta_topic_id: 51731
-# version: 1.0
+# version: 2.0
 # authors: Matthew Wilkin
 # url: https://github.com/discourse/discourse-microsoft-auth
 
@@ -34,11 +34,8 @@ def enabled?
     SiteSetting.microsoft_auth_enabled
   end
 
-  # Microsoft doesn't let users login with OAuth2 to websites unless the user
-  # has verified their email address so we can assume whatever email we get
-  # from MS is verified.
   def primary_email_verified?(auth_token)
-    true
+    SiteSetting.microsoft_auth_email_verified
   end
 end
 

spec/integration/microsoft_auth_spec.rb

@@ -64,9 +64,11 @@ def setup_ms_emails_stub(email:)
     )
   end
 
-  # microsoft doesn't allow oauth2 logins unless the user has verified their email
-  it "signs in the user whose email matches the email included in the API response from microsoft" do
+  it "signs in the user whose email matches the email included in the API response from microsoft when `microsoft_auth_email_verified` site setting is true" do
+    SiteSetting.microsoft_auth_email_verified = true
+
     post "/auth/microsoft_office365"
+
     expect(response.status).to eq(302)
     expect(response.location).to start_with(
       "https://login.microsoftonline.com/common/oauth2/v2.0/authorize",
@@ -79,8 +81,32 @@ def setup_ms_emails_stub(email:)
            state: session["omniauth.state"],
            code: temp_code,
          }
+
     expect(response.status).to eq(302)
     expect(response.location).to eq("http://test.localhost/")
     expect(session[:current_user_id]).to eq(user1.id)
   end
+
+  it "does not sign in the user whose email matches the email included in the API response from microsoft when `microsoft_auth_email_verified` site setting is false" do
+    SiteSetting.microsoft_auth_email_verified = false
+
+    post "/auth/microsoft_office365"
+
+    expect(response.status).to eq(302)
+    expect(response.location).to start_with(
+      "https://login.microsoftonline.com/common/oauth2/v2.0/authorize",
+    )
+
+    setup_ms_emails_stub(email: user1.email)
+
+    post "/auth/microsoft_office365/callback",
+         params: {
+           state: session["omniauth.state"],
+           code: temp_code,
+         }
+
+    expect(response.status).to eq(302)
+    expect(response.location).to eq("http://test.localhost/")
+    expect(session[:current_user_id]).to eq(nil)
+  end
 end

spec/lib/microsoft_auth_revoker_spec.rb

@@ -0,0 +1,90 @@
+# frozen_string_literal: true
+
+require_relative "../../lib/microsoft_auth_revoker"
+
+RSpec.describe MicrosoftAuthRevoker do
+  describe ".revoke" do
+    fab!(:user_1) { Fabricate(:user).tap { |user| UserAuthToken.generate!(user_id: user.id) } }
+    fab!(:user_2) { Fabricate(:user).tap { |user| UserAuthToken.generate!(user_id: user.id) } }
+    fab!(:user_3) { Fabricate(:user).tap { |user| UserAuthToken.generate!(user_id: user.id) } }
+
+    fab!(:microsoft_user_associated_account_for_user_1) do
+      UserAssociatedAccount.create!(
+        provider_name: "microsoft_office365",
+        user_id: user_1.id,
+        provider_uid: 100,
+        info: {
+          email: "someuser@somedomain.tld",
+        },
+      )
+    end
+
+    fab!(:microsoft_user_associated_account_for_user_2) do
+      UserAssociatedAccount.create!(
+        provider_name: "microsoft_office365",
+        user_id: user_2.id,
+        provider_uid: 200,
+        info: {
+          email: "someuser@somedomain.tld",
+        },
+      )
+    end
+
+    fab!(:facebook_user_associated_account_for_user_3) do
+      UserAssociatedAccount.create!(
+        provider_name: "facebook",
+        user_id: user_1.id,
+        provider_uid: 100,
+        info: {
+          email: "someuser@somedomain.tld",
+        },
+      )
+    end
+
+    fab!(:user_api_key_for_user_1) { Fabricate(:user_api_key, user: user_1) }
+    fab!(:user_api_key_for_user_2) { Fabricate(:user_api_key, user: user_2) }
+    fab!(:user_api_key_for_user_3) { Fabricate(:user_api_key, user: user_3) }
+    fab!(:api_key_for_user_1) { Fabricate(:api_key, created_by_id: user_1.id) }
+    fab!(:api_key_for_user_2) { Fabricate(:api_key, created_by_id: user_2.id) }
+    fab!(:api_key_for_user_3) { Fabricate(:api_key, created_by_id: user_3.id) }
+
+    it "should delete all microsoft provider `UserAssociatedAccount` records" do
+      expect do MicrosoftAuthRevoker.revoke end.to change { UserAssociatedAccount.count }.by(-2)
+      expect(UserAssociatedAccount.where(provider_name: "microsoft_office365").count).to eq(0)
+    end
+
+    it "should deactivate all users with microsoft provider `UserAssociatedAccount` records" do
+      expect do MicrosoftAuthRevoker.revoke end.to change { User.where(active: true).count }.by(-2)
+      expect(user_1.reload.active).to eq(false)
+      expect(user_2.reload.active).to eq(false)
+      expect(user_3.reload.active).to eq(true)
+    end
+
+    it "should delete all `UserAuthToken` records for users with microsoft provider `UserAssociatedAccount` records" do
+      expect do MicrosoftAuthRevoker.revoke end.to change { UserAuthToken.count }.by(-2)
+      expect(UserAuthToken.where(user_id: user_1.id).count).to eq(0)
+      expect(UserAuthToken.where(user_id: user_2.id).count).to eq(0)
+      expect(UserAuthToken.where(user_id: user_3.id).count).to eq(1)
+    end
+
+    it "should revoke all `UserApiKey` records for users with microsoft provider `UserAssociatedAccount` records" do
+      expect do MicrosoftAuthRevoker.revoke end.to change {
+        UserApiKey.where(revoked_at: nil).count
+      }.by(-2)
+
+      expect(user_api_key_for_user_1.reload.revoked_at).to be_present
+      expect(user_api_key_for_user_2.reload.revoked_at).to be_present
+      expect(user_api_key_for_user_3.reload.revoked_at).to be_nil
+    end
+
+    it "should revoke all `ApiKey` records created by users with microsoft provider `UserAssociatedAccount` records" do
+      expect do MicrosoftAuthRevoker.revoke end.to change {
+        ApiKey.where(revoked_at: nil).count
+      }.by(-2)
+
+      expect(api_key_for_user_1.reload.revoked_at).to be_present
+      expect(api_key_for_user_2.reload.revoked_at).to be_present
+      expect(api_key_for_user_3.reload.revoked_at).to be_nil
+    end
+  end
+end