CVE-2023-46241: Insufficient Information (NVD-CWE-noinfo)
value: `discourse-microsoft-auth` is a plugin that enables authentication via Microsoft. On sites with the `discourse-microsoft-auth` plugin enabled, an attack can potentially take control of a victim's Discourse account. Sites that have configured their application's account type to any options other than `Accounts in this organizational directory only (O365 only - Single tenant)` are vulnerable. This vulnerability has been patched in commit c40665f44509724b64938c85def9fb2e79f62ec8 of `discourse-microsoft-auth`. A `microsoft_auth:revoke` rake task has also been added which will deactivate and log out all users that have connected their accounts to Microsoft. User API keys as well as API keys created by those users will also be revoked. The rake task will also remove the connection records to Microsoft for those users. This will allow affected users to re-verify their account emails as well as reconnect their Discourse account to Microsoft for authentication. As a workaround, disable the `discourse-microsoft-auth` plugin by setting the `microsoft_auth_enabled` site setting to `false`. Run the `microsoft_auth:log_out_users` rake task to log out all users with associated Microsoft accounts.

CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-02-21T16:15Z
Repository: https://github.com/discourse/discourse-microsoft-auth
Language: Ruby
Files changed: config/locales/server.en.yml, config/settings.yml, lib/microsoft_auth_revoker.rb, lib/tasks/microsoft_auth.rake, plugin.rb, spec/integration/microsoft_auth_spec.rb, spec/lib/microsoft_auth_revoker_spec.rb
Commit date: 2024-02-21 12:48:32 +0800
Commit message: SECURITY: Emails from microsoft are not verified (#72)

Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Diff: diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 4fd40a4..8c8c150 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -1,5 +1,6 @@
 en:
   site_settings:
     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.'
diff --git a/config/settings.yml b/config/settings.yml
index 2a317ab..a5e596f 100644
--- a/config/settings.yml
+++ b/config/settings.yml
@@ -5,6 +5,8 @@ plugins:
   microsoft_auth_client_id:
     client: false
     default: ''
   microsoft_auth_client_secret:
     client: false
-    default: ''
\ No newline at end of file
+    default: ''
+  microsoft_auth_email_verified:
+    default: false
\ No newline at end of file
diff --git a/lib/microsoft_auth_revoker.rb b/lib/microsoft_auth_revoker.rb
new file mode 100644
index 0000000..d17ff69
--- /dev/null
+++ b/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
diff --git a/lib/tasks/microsoft_auth.rake b/lib/tasks/microsoft_auth.rake
new file mode 100644
index 0000000..0d959be
--- /dev/null
+++ b/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
diff --git a/plugin.rb b/plugin.rb
index 4679a59..6ecbe76 100644
--- a/plugin.rb
+++ b/plugin.rb
@@ -1,11 +1,11 @@
 # frozen_string_literal: true
 
 # 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
 
 require_relative "lib/omniauth-microsoft365"
 
@@ -32,14 +32,11 @@ class ::MicrosoftAuthenticator < ::Auth::ManagedAuthenticator
 
   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
 
 auth_provider authenticator: MicrosoftAuthenticator.new, icon: "fab-microsoft"
diff --git a/spec/integration/microsoft_auth_spec.rb b/spec/integration/microsoft_auth_spec.rb
index ea059e6..6eaca2a 100644
--- a/spec/integration/microsoft_auth_spec.rb
+++ b/spec/integration/microsoft_auth_spec.rb
@@ -62,13 +62,15 @@ describe "Microsoft OAuth2" do
         "Content-Type" => "application/x-www-form-urlencoded",
       },
     )
   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",
     )
 
@@ -77,10 +79,34 @@ describe "Microsoft OAuth2" do
     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(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
diff --git a/spec/lib/microsoft_auth_revoker_spec.rb b/spec/lib/microsoft_auth_revoker_spec.rb
new file mode 100644
index 0000000..3ab2a31
--- /dev/null
+++ b/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

Vulnerable code:     default: ''
# version: 1.0
  # 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.
    true
  # 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
Fixed code:     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.'
    default: ''
  microsoft_auth_email_verified:
    default: false
# 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
# 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
# version: 2.0
    SiteSetting.microsoft_auth_email_verified
  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




  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
# 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