FIX: Gracefully handle errors while fetching the discovery document (#4)

Previously an error loading the discovery document would raise an exception. Now, it will display an error to the user, and log the error for site admins to view at `/logs`. Specs are updated and improved accordingly.

This moves the discovery document fetching out of OmniAuth and into Discourse. This makes it available for the upcoming rp-initiated-logout support.

config/locales/server.en.yml

@@ -11,3 +11,6 @@ en:
     openid_connect_verbose_logging: "Log detailed openid-connect authentication information to `/logs`. Keep this disabled during normal use."
     openid_connect_authorize_parameters: "URL parameters which will be included in the redirect from /auth/oidc to the IDP's authorize endpoint"
     openid_connect_overrides_email: "On every login, override the user's email using the openid-connect value"
+  login:
+    omniauth_error:
+      openid_connect_discovery_error: Unable to fetch configuration from identity provider. Please try again.

lib/omniauth_open_id_connect.rb

@@ -11,15 +11,14 @@ module ::OmniAuth
     class OpenIDConnect < OmniAuth::Strategies::OAuth2
       option :scope, "openid"
       option :discovery, true
+      option :discovery_document, nil
       option :use_userinfo, true
-      option :cache, lambda { |key, &blk| blk.call } # Default no-op cache
       option :error_handler, lambda { |error, message| nil } # Default no-op handler
       option :verbose_logger, lambda { |message| nil } # Default no-op handler
       option :passthrough_authorize_options, [:p]
       option :passthrough_token_options, [:p]
 
       option :client_options,
-        discovery_document: nil,
         site: nil,
         authorize_url: nil,
         token_url: nil,
@@ -31,11 +30,8 @@ module ::OmniAuth
       end
 
       def discover!
-        verbose_log("Fetching discovery document from #{options[:client_options][:discovery_document]}")
-        discovery_document = options.cache.call("openid_discovery_#{options[:client_options][:discovery_document]}") do
-          client.request(:get, options[:client_options][:discovery_document], parse: :json).parsed
-        end
-        verbose_log("Discovery document loaded\n\n#{discovery_document.to_yaml}")
+        discovery_document = options[:discovery_document]
+        raise ::OmniAuth::OpenIDConnect::DiscoveryError.new("Discovery document is missing") if discovery_document.nil?
 
         discovery_params = {
           authorize_url: "authorization_endpoint",
@@ -44,7 +40,7 @@ module ::OmniAuth
         }
 
         discovery_params.each do |internal_key, external_key|
-          val = discovery_document[external_key].to_s
+          val = discovery_document[external_key]
           raise ::OmniAuth::OpenIDConnect::DiscoveryError.new("missing discovery parameter #{external_key}") if val.nil? || val.empty?
           options[:client_options][internal_key] = val
         end
@@ -57,7 +53,7 @@ module ::OmniAuth
         begin
           discover! if options[:discovery]
         rescue ::OmniAuth::OpenIDConnect::DiscoveryError => e
-          fail!(:openid_connect_discovery_error, e)
+          return fail!(:openid_connect_discovery_error, e)
         end
 
         super

lib/openid_connect_authenticator.rb

@@ -31,11 +31,42 @@ class OpenIDConnectAuthenticator < Auth::ManagedAuthenticator
     SiteSetting.openid_connect_overrides_email
   end
 
+  def discovery_document
+    document_url = SiteSetting.openid_connect_discovery_document.presence
+    if !document_url
+      oidc_log("No discovery document URL specified", error: true)
+      return
+    end
+
+    from_cache = true
+    result = Discourse.cache.fetch("openid-connect-discovery-#{document_url}", expires_in: 10.minutes) do
+      from_cache = false
+      oidc_log("Fetching discovery document from #{document_url}")
+      connection = Faraday.new { |c| c.use Faraday::Response::RaiseError }
+      JSON.parse(connection.get(document_url).body)
+    rescue Faraday::Error, JSON::ParserError => e
+      oidc_log("Fetching discovery document raised error #{e.class} #{e.message}", error: true)
+      nil
+    end
+
+    oidc_log("Discovery document loaded from cache") if from_cache
+    oidc_log("Discovery document is\n\n#{result.to_yaml}")
+
+    result
+  end
+
+  def oidc_log(message, error: false)
+    if error
+      Rails.logger.error("OIDC Log: #{message}")
+    elsif SiteSetting.openid_connect_verbose_logging
+      Rails.logger.warn("OIDC Log: #{message}")
+    end
+  end
+
   def register_middleware(omniauth)
 
     omniauth.provider :openid_connect,
       name: :oidc,
-      cache: lambda { |key, &blk| Rails.cache.fetch(key, expires_in: 10.minutes, &blk) },
       error_handler: lambda { |error, message|
         handlers = SiteSetting.openid_connect_error_redirects.split("\n")
         handlers.each do |row|
@@ -44,10 +75,7 @@ class OpenIDConnectAuthenticator < Auth::ManagedAuthenticator
         end
         nil
       },
-      verbose_logger: lambda { |message|
-        return unless SiteSetting.openid_connect_verbose_logging
-        Rails.logger.warn("OIDC Log: #{message}")
-      },
+      verbose_logger: lambda { |message| oidc_log(message) },
       setup: lambda { |env|
         opts = env['omniauth.strategy'].options
 
@@ -57,9 +85,7 @@ class OpenIDConnectAuthenticator < Auth::ManagedAuthenticator
         opts.deep_merge!(
           client_id: SiteSetting.openid_connect_client_id,
           client_secret: SiteSetting.openid_connect_client_secret,
-          client_options: {
-            discovery_document: SiteSetting.openid_connect_discovery_document,
-          },
+          discovery_document: discovery_document,
           scope: SiteSetting.openid_connect_authorize_scope,
           token_params: token_params,
           passthrough_authorize_options: SiteSetting.openid_connect_authorize_parameters.split("|")

plugin.rb

@@ -6,6 +6,8 @@
 # authors: David Taylor
 # url: https://github.com/discourse/discourse-openid-connect
 
+enabled_site_setting :openid_connect_enabled
+
 require_relative "lib/openid_connect_faraday_formatter"
 require_relative "lib/omniauth_open_id_connect"
 require_relative "lib/openid_connect_authenticator"

spec/lib/omniauth_open_id_connect_spec.rb

@@ -4,70 +4,47 @@ require_relative '../../lib/omniauth_open_id_connect'
 require 'rails_helper'
 
 describe OmniAuth::Strategies::OpenIDConnect do
-  # let(:request) { double('Request', params: {}, cookies: {}, env: {}) }
   let(:app) do
     lambda do |*args|
       [200, {}, ['Hello.']]
     end
   end
 
-  before do
-    stub_request(:get, "https://id.example.com/.well-known/openid-configuration").
-      to_return(status: 200, body: {
-          "issuer": "https://id.example.com/",
-          "authorization_endpoint": "https://id.example.com/authorize",
-          "token_endpoint": "https://id.example.com/token",
-          "userinfo_endpoint": "https://id.example.com/userinfo",
-        }.to_json)
+  let(:discovery_document) do
+    {
+      "issuer" => "https://id.example.com/",
+      "authorization_endpoint" => "https://id.example.com/authorize",
+      "token_endpoint" => "https://id.example.com/token",
+      "userinfo_endpoint" => "https://id.example.com/userinfo",
+    }
   end
 
   subject do
-    OmniAuth::Strategies::OpenIDConnect.new(app, 'appid', 'secret',
-      client_options: {
-        discovery_document: "https://id.example.com/.well-known/openid-configuration"
-      }
-
-    ).tap do |strategy|
-    end
+    OmniAuth::Strategies::OpenIDConnect.new(app, 'appid', 'secret', discovery_document: discovery_document)
   end
 
   before { OmniAuth.config.test_mode = true }
 
   after { OmniAuth.config.test_mode = false }
 
-  it "throws error for on invalid discovery document" do
-    stub_request(:get, "https://id.example.com/.well-known/openid-configuration").
-      to_return(status: 200, body: {
-        "issuer": "https://id.example.com/",
-        "token_endpoint": "https://id.example.com/token",
-        "userinfo_endpoint": "https://id.example.com/userinfo",
-      }.to_json)
+  it "throws error for missing discovery document" do
+    strategy = OmniAuth::Strategies::OpenIDConnect.new(app, 'appid', 'secret', discovery_document: nil)
+    expect { strategy.discover! }.to raise_error(::OmniAuth::OpenIDConnect::DiscoveryError)
+  end
 
+  it "throws error for invalid discovery document" do
+    discovery_document.delete("authorization_endpoint")
     expect { subject.discover! }.to raise_error(::OmniAuth::OpenIDConnect::DiscoveryError)
   end
 
   it "disables userinfo if not included in discovery document" do
-    stub_request(:get, "https://id.example.com/.well-known/openid-configuration").
-      to_return(status: 200, body: {
-        "issuer": "https://id.example.com/",
-        "authorization_endpoint": "https://id.example.com/authorize",
-        "token_endpoint": "https://id.example.com/token",
-      }.to_json)
-
+    discovery_document.delete("userinfo_endpoint")
     subject.discover!
     expect(subject.options.use_userinfo).to eq(false)
   end
 
-  context 'with valid discovery document' do
+  context 'with valid discovery document loaded' do
     before do
-      stub_request(:get, "https://id.example.com/.well-known/openid-configuration").
-        to_return(status: 200, body: {
-          "issuer": "https://id.example.com/",
-          "authorization_endpoint": "https://id.example.com/authorize",
-          "token_endpoint": "https://id.example.com/token",
-          "userinfo_endpoint": "https://id.example.com/userinfo",
-        }.to_json)
-
       subject.stubs(:request).returns(mock('object'))
       subject.request.stubs(:params).returns("p" => "someallowedvalue", "somethingelse" => "notallowed")
 

spec/lib/openid_connect_authenticator_spec.rb

@@ -59,4 +59,57 @@ describe OpenIDConnectAuthenticator do
     end
   end
 
+  describe "discovery document fetching" do
+    let(:document_url) { SiteSetting.openid_connect_discovery_document = "https://id.example.com/.well-known/openid-configuration" }
+    let(:document) do
+      {
+        "issuer": "https://id.example.com/",
+        "authorization_endpoint": "https://id.example.com/authorize",
+        "token_endpoint": "https://id.example.com/token",
+        "userinfo_endpoint": "https://id.example.com/userinfo",
+      }.to_json
+    end
+    after { Discourse.cache.delete("openid-connect-discovery-#{document_url}") }
+
+    it "loads the document correctly" do
+      stub_request(:get, document_url).to_return(body: document)
+      expect(authenticator.discovery_document.keys).to contain_exactly(
+        "issuer",
+        "authorization_endpoint",
+        "token_endpoint",
+        "userinfo_endpoint"
+      )
+    end
+
+    it "handles a non-200 response" do
+      stub_request(:get, document_url).to_return(status: 404)
+      expect(authenticator.discovery_document).to eq(nil)
+    end
+
+    it "handles a network error" do
+      stub_request(:get, document_url).to_timeout
+      expect(authenticator.discovery_document).to eq(nil)
+    end
+
+    it "handles invalid json" do
+      stub_request(:get, document_url).to_return(body: "this is not the json you're looking for")
+      expect(authenticator.discovery_document).to eq(nil)
+    end
+
+    it "caches a success response" do
+      stub = stub_request(:get, document_url).to_return(body: document)
+      expect(authenticator.discovery_document).not_to eq(nil)
+      expect(authenticator.discovery_document).not_to eq(nil)
+      expect(stub).to have_been_requested.once
+    end
+
+    it "caches a failed response" do
+      stub = stub_request(:get, document_url).to_return(status: 404)
+      expect(authenticator.discovery_document).to eq(nil)
+      expect(authenticator.discovery_document).to eq(nil)
+      expect(stub).to have_been_requested.once
+    end
+
+  end
+
 end