Merge pull request #96 from nov/feature/better_error_handling

handle JWKS fetch failures
nhosoya · Oct 25, 2022 · 5a43139 · 5a43139
2 parents ad98e1b + c64bbf9
commit 5a43139
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 22 deletions.
diff --git a/lib/omniauth/strategies/apple.rb b/lib/omniauth/strategies/apple.rb
@@ -74,27 +74,38 @@ def stored_nonce
       def id_info
         @id_info ||= if request.params&.key?('id_token') || access_token&.params&.key?('id_token')
                        id_token = request.params['id_token'] || access_token.params['id_token']
-                       jwt_options = {
-                         verify_iss: true,
-                         iss: 'https://appleid.apple.com',
-                         verify_iat: true,
-                         verify_aud: true,
-                         aud: [options.client_id].concat(options.authorized_client_ids),
-                         algorithms: ['RS256'],
-                         jwks: fetch_jwks
-                       }
-                       payload, _header = ::JWT.decode(id_token, nil, true, jwt_options)
-                       verify_nonce!(payload)
-                       payload
+                       if (verification_key = fetch_jwks)
+                         jwt_options = {
+                           verify_iss: true,
+                           iss: 'https://appleid.apple.com',
+                           verify_iat: true,
+                           verify_aud: true,
+                           aud: [options.client_id].concat(options.authorized_client_ids),
+                           algorithms: ['RS256'],
+                           jwks: verification_key
+                         }
+                         payload, _header = ::JWT.decode(id_token, nil, true, jwt_options)
+                         verify_nonce!(payload)
+                         payload
+                       else
+                         {}
+                       end
                      end
       end
 
       def fetch_jwks
-        http = Net::HTTP.new('appleid.apple.com', 443)
-        http.use_ssl = true
-        request = Net::HTTP::Get.new('/auth/keys', 'User-Agent' => 'ruby/omniauth-apple')
-        response = http.request(request)
-        JSON.parse(response.body, symbolize_names: true)
+        conn = Faraday.new(headers: {user_agent: 'ruby/omniauth-apple'}) do |c|
+          c.response :json, parser_options: { symbolize_names: true }
+          c.adapter Faraday.default_adapter
+        end
+        res = conn.get 'https://appleid.apple.com/auth/keys'
+        if res.success?
+          res.body
+        else
+          fail!(:jwks_fetching_failed, CallbackError.new(:jwks_fetching_failed, 'HTTP Error when fetching JWKs'))
+        end
+      rescue Faraday::Error => e
+        fail!(:jwks_fetching_failed, e)
       end
 
       def verify_nonce!(payload)

diff --git a/spec/omniauth/apple/vesion_spec.rb b/spec/omniauth/apple/vesion_spec.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 require 'spec_helper'
-require_relative '../../../lib/omniauth/apple/version'
 
 describe OmniAuth::Apple do
   it 'has VERSION' do

diff --git a/spec/omniauth/strategies/apple_spec.rb b/spec/omniauth/strategies/apple_spec.rb
@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 require 'spec_helper'
-require 'json'
-require 'omniauth-apple'
 
 describe OmniAuth::Strategies::Apple do
   let(:request) { double('Request', params: {}, cookies: {}, env: {}) }
@@ -61,7 +59,12 @@
 
   before do
     OmniAuth.config.test_mode = true
-    stub_request(:get, 'https://appleid.apple.com/auth/keys').to_return(body: auth_keys.to_json)
+    stub_request(:get, 'https://appleid.apple.com/auth/keys').to_return(
+      body: auth_keys.to_json,
+      headers: {
+       'Content-Type': 'application/json'
+      }
+    )
   end
 
   after do
@@ -333,7 +336,49 @@
         end
       end
     end
+  end
+
+  describe 'network errors' do
+    before do
+      subject.authorize_params # initializes session / populates 'nonce', 'state', etc
+      id_token_payload['nonce'] = subject.session['omniauth.nonce']
+      request.params.merge!('id_token' => id_token)
+    end
 
- end
+    context 'when JWKS fetching failed' do
+      before do
+        stub_request(:get, 'https://appleid.apple.com/auth/keys').to_return(
+          status: 502,
+          body: "<html><head><title>502 Bad Gateway..."
+        )
+      end
 
+      it do
+        expect(subject).to receive(:fail!).with(
+          :jwks_fetching_failed,
+          instance_of(OmniAuth::Strategies::OAuth2::CallbackError)
+        )
+        subject.info
+      end
+    end
+
+    context 'when JWKS format is invalid' do
+      before do
+        stub_request(:get, 'https://appleid.apple.com/auth/keys').to_return(
+          body: 'invalid',
+          headers: {
+            'Content-Type': 'application/json'
+          }
+        )
+      end
+
+      it do
+        expect(subject).to receive(:fail!).with(
+          :jwks_fetching_failed,
+          instance_of(Faraday::ParsingError)
+        )
+        subject.info
+      end
+    end
+  end
 end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -5,5 +5,7 @@
 require 'simplecov'
 SimpleCov.start('test_frameworks')
 
+require 'omniauth-apple'
+
 require 'webmock/rspec'
 WebMock.disable_net_connect!