Skip to content

Commit 3cf301a

Browse files
committed
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.
1 parent 9ec7283 commit 3cf301a

File tree

2 files changed

+14
-6
lines changed

2 files changed

+14
-6
lines changed

lib/omniauth/microsoft_graph/domain_verifier.rb

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# frozen_string_literal: true
2+
23
require 'jwt' # for token signature validation
3-
require 'omniauth' # to inherit from OmniAuth::Error
4+
require 'omniauth-oauth2' # to use CallbackError
45
require 'oauth2' # to rescue OAuth2::Error
56

67
module OmniAuth
@@ -11,8 +12,6 @@ module MicrosoftGraph
1112
OIDC_CONFIG_URL = 'https://login.microsoftonline.com/organizations/v2.0/.well-known/openid-configuration'
1213
COMMON_JWKS_URL = 'https://login.microsoftonline.com/common/discovery/v2.0/keys'
1314

14-
class DomainVerificationError < OmniAuth::Error; end
15-
1615
class DomainVerifier
1716
def self.verify!(auth_hash, access_token, options)
1817
new(auth_hash, access_token, options).verify!
@@ -41,7 +40,13 @@ def verify!
4140
skip_verification == true ||
4241
(skip_verification.is_a?(Array) && skip_verification.include?(email_domain)) ||
4342
domain_verified_jwt_claim
44-
raise DomainVerificationError, verification_error_message
43+
44+
# Use CallbackError to ensure the error is properly caught by the callback_phase
45+
# rescue clause and converted to an OmniAuth failure instead of bubbling up as a 500 error.
46+
raise OmniAuth::Strategies::OAuth2::CallbackError.new(
47+
:domain_verification_failed,
48+
verification_error_message
49+
)
4550
end
4651

4752
private

spec/omniauth/microsoft_graph/domain_verifier_spec.rb

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,11 @@
105105
context 'when all verification strategies fail' do
106106
before { allow(access_token).to receive(:get).and_raise(::OAuth2::Error.new('whoops')) }
107107

108-
it 'raises a DomainVerificationError' do
109-
expect { result }.to raise_error OmniAuth::MicrosoftGraph::DomainVerificationError
108+
it 'raises a CallbackError with domain_verification_failed' do
109+
expect { result }.to raise_error(OmniAuth::Strategies::OAuth2::CallbackError) do |error|
110+
expect(error.error).to eq(:domain_verification_failed)
111+
expect(error.error_reason).to include('not a verified domain')
112+
end
110113
end
111114
end
112115
end

0 commit comments

Comments
 (0)