From 98bf51ac74534108ae082139f68a1c345b33dd26 Mon Sep 17 00:00:00 2001 From: Paul Druziak Date: Thu, 16 Oct 2025 14:26:46 -0400 Subject: [PATCH 1/2] Fix domain verification error handling using CallbackError Changes: - Replace custom DomainVerificationError (inheriting from OmniAuth::Error) with OmniAuth::Strategies::OAuth2::CallbackError - This ensures the error is properly caught by omniauth-oauth2's callback_phase rescue clause instead of bubbling up as a 500 error - Update test to expect CallbackError with :domain_verification_failed symbol Rationale: The omniauth-oauth2 gem's callback_phase only rescues specific error types: rescue ::OAuth2::Error, CallbackError => e The previous DomainVerificationError inherited from OmniAuth::Error, which is not in the rescue clause, causing it to bubble up as an unhandled 500 error. By using CallbackError (the same pattern used by omniauth-google-oauth2 for hosted domain verification), the error is automatically caught and converted to a proper OmniAuth failure with fail!(:invalid_credentials, e). This provides a better user experience with proper error handling and redirects instead of 500 error pages. --- lib/omniauth/microsoft_graph/domain_verifier.rb | 13 +++++++++---- .../microsoft_graph/domain_verifier_spec.rb | 7 +++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/omniauth/microsoft_graph/domain_verifier.rb b/lib/omniauth/microsoft_graph/domain_verifier.rb index 4401a5e..d9455cb 100644 --- a/lib/omniauth/microsoft_graph/domain_verifier.rb +++ b/lib/omniauth/microsoft_graph/domain_verifier.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true + require 'jwt' # for token signature validation -require 'omniauth' # to inherit from OmniAuth::Error +require 'omniauth-oauth2' # to use CallbackError require 'oauth2' # to rescue OAuth2::Error module OmniAuth @@ -11,8 +12,6 @@ module MicrosoftGraph OIDC_CONFIG_URL = 'https://login.microsoftonline.com/organizations/v2.0/.well-known/openid-configuration' COMMON_JWKS_URL = 'https://login.microsoftonline.com/common/discovery/v2.0/keys' - class DomainVerificationError < OmniAuth::Error; end - class DomainVerifier def self.verify!(auth_hash, access_token, options) new(auth_hash, access_token, options).verify! @@ -41,7 +40,13 @@ def verify! skip_verification == true || (skip_verification.is_a?(Array) && skip_verification.include?(email_domain)) || domain_verified_jwt_claim - raise DomainVerificationError, verification_error_message + + # Use CallbackError to ensure the error is properly caught by the callback_phase + # rescue clause and converted to an OmniAuth failure instead of bubbling up as a 500 error. + raise OmniAuth::Strategies::OAuth2::CallbackError.new( + :domain_verification_failed, + verification_error_message + ) end private diff --git a/spec/omniauth/microsoft_graph/domain_verifier_spec.rb b/spec/omniauth/microsoft_graph/domain_verifier_spec.rb index 777695b..6d8891f 100644 --- a/spec/omniauth/microsoft_graph/domain_verifier_spec.rb +++ b/spec/omniauth/microsoft_graph/domain_verifier_spec.rb @@ -105,8 +105,11 @@ context 'when all verification strategies fail' do before { allow(access_token).to receive(:get).and_raise(::OAuth2::Error.new('whoops')) } - it 'raises a DomainVerificationError' do - expect { result }.to raise_error OmniAuth::MicrosoftGraph::DomainVerificationError + it 'raises a CallbackError with domain_verification_failed' do + expect { result }.to raise_error(OmniAuth::Strategies::OAuth2::CallbackError) do |error| + expect(error.error).to eq(:domain_verification_failed) + expect(error.error_reason).to include('not a verified domain') + end end end end From d902d69de6adcfd1d1e082fb3c24a7697cec746f Mon Sep 17 00:00:00 2001 From: Paul Druziak Date: Thu, 16 Oct 2025 14:41:14 -0400 Subject: [PATCH 2/2] Fix test to use CallbackError instead of DomainVerificationError Updated the test expectation to match the new error type used in the domain verifier. The test now properly expects CallbackError with :domain_verification_failed symbol." --- spec/omniauth/strategies/microsoft_graph_oauth2_spec.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spec/omniauth/strategies/microsoft_graph_oauth2_spec.rb b/spec/omniauth/strategies/microsoft_graph_oauth2_spec.rb index 01482d5..5f34d49 100644 --- a/spec/omniauth/strategies/microsoft_graph_oauth2_spec.rb +++ b/spec/omniauth/strategies/microsoft_graph_oauth2_spec.rb @@ -282,7 +282,12 @@ context 'when email verification fails' do let(:response_hash) { { mail: 'something@domain.invalid' } } - let(:error) { OmniAuth::MicrosoftGraph::DomainVerificationError.new } + let(:error) do + OmniAuth::Strategies::OAuth2::CallbackError.new( + :domain_verification_failed, + 'Domain verification failed' + ) + end before do allow(OmniAuth::MicrosoftGraph::DomainVerifier).to receive(:verify!).and_raise(error)