FIX: Improved 'discovery' error handling, with tests

Author: davidtaylorhq

.travis.yml

@@ -0,0 +1,12 @@
+# We want to use the KVM-based system, so require sudo
+sudo: required
+services:
+ - docker
+
+before_install:
+ - git clone https://github.com/discourse/discourse-plugin-ci
+
+install: true # Prevent travis doing bundle install
+
+script:
+ - discourse-plugin-ci/script.sh

config/locales/server.en.yml

@@ -5,5 +5,4 @@ en:
  openid_connect_client_secret: "OpenID Connect client secret"
  openid_connect_authorize_scope: "The scopes sent to the authorize endpoint. This must include 'openid'."
  openid_connect_token_scope: "The scopes sent when requesting the token endpoint. The official specification does not require this."
- openid_connect_use_userinfo: "Contact the userinfo endpoint for user metadata. If left blank, the 'id_token' will be used instead."
  openid_connect_error_redirects: "If the callback error_reason contains the first parameter, the user will be redirected to the URL in the second parameter"

config/settings.yml

@@ -9,8 +9,6 @@ plugins:
  default: "openid"
  openid_connect_token_scope:
  default: ""
- openid_connect_use_userinfo:
- default: true
  openid_connect_error_redirects:
  default: ''
  type: list

lib/omniauth_open_id_connect.rb

@@ -1,30 +1,47 @@
 require 'omniauth-oauth2'
 
 module ::OmniAuth
+ module OpenIDConnect
+ class DiscoveryError < Error; end
+ end
+
  module Strategies
  class OpenIDConnect < OmniAuth::Strategies::OAuth2
  option :scope, "openid"
  option :discovery, true
+ 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 :authorize_options, [:p]
- option :token_options, [:p]
+ option :passthrough_authorize_options, [:p]
+ option :passthrough_token_options, [:p]
 
  option :client_options,
- site: 'https://op.com/',
- authorize_url: 'authorize',
- token_url: 'token',
- userinfo_endpoint: 'userinfo',
+ discovery_document: nil,
+ site: nil,
+ authorize_url: nil,
+ token_url: nil,
+ userinfo_endpoint: nil,
  auth_scheme: :basic_auth
 
  def discover!
  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
- options[:client_options][:authorize_url] = discovery_document["authorization_endpoint"].to_s
- options[:client_options][:token_url] = discovery_document["token_endpoint"].to_s
- options[:client_options][:userinfo_endpoint] = discovery_document["userinfo_endpoint"].to_s
- options[:client_options][:site] = discovery_document["issuer"].to_s
+
+ {
+ authorize_url: "authorization_endpoint",
+ token_url: "token_endpoint",
+ site: "issuer"
+ }.each do |internal_key, external_key|
+ val = discovery_document[external_key].to_s
+ raise ::OmniAuth::OpenIDConnect::DiscoveryError.new("missing discovery parameter #{external_key}") if val.nil? || val.empty?
+ options[:client_options][internal_key] = val
+ end
+
+ userinfo_endpoint = options[:client_options][:userinfo_endpoint] = discovery_document["userinfo_endpoint"].to_s
+ if userinfo_endpoint.nil? || userinfo_endpoint.empty?
+ options.use_userinfo = false
+ end
  end
 
  def request_phase
@@ -34,14 +51,14 @@ def request_phase
 
  def authorize_params
  super.tap do |params|
- options[:authorize_options].each do |k|
+ options[:passthrough_authorize_options].each do |k|
  params[k] = request.params[k.to_s] unless [nil, ''].include?(request.params[k.to_s])
  end
 
  params[:scope] = options[:scope]
  session['omniauth.nonce'] = params[:nonce] = SecureRandom.hex(32)
 
- options[:token_options].each do |k|
+ options[:passthrough_token_options].each do |k|
  session["omniauth.param.#{k}"] = request.params[k.to_s] unless [nil, ''].include?(request.params[k.to_s])
  end
  end
@@ -95,8 +112,15 @@ def callback_phase
  if request.params["error"] && request.params["error_description"] && response = options.error_handler.call(request.params["error"], request.params["error_description"])
  return redirect(response)
  end
- discover! if options[:discovery]
+
+ begin
+ discover! if options[:discovery]
+ rescue ::OmniAuth::OpenIDConnect::DiscoveryError => e
+ fail!(:openid_connect_discovery_error, e)
+ end
+
  oauth2_callback_phase = super
+
  return oauth2_callback_phase if env['omniauth.error']
 
  if id_token_info["nonce"].empty? || id_token_info["nonce"] != session.delete("omniauth.nonce")
@@ -113,7 +137,7 @@ def callback_url
 
  def token_params
  params = {}
- options[:token_options].each do |k|
+ options[:passthrough_token_options].each do |k|
  val = session.delete("omniauth.param.#{k}")
  params[k] = val unless [nil, ''].include?(val)
  end

plugin.rb

@@ -83,7 +83,6 @@ def register_middleware(omniauth)
  setup: lambda { |env|
  opts = env['omniauth.strategy'].options
  opts.deep_merge!(
- use_userinfo: SiteSetting.openid_connect_use_userinfo,
  client_id: SiteSetting.openid_connect_client_id,
  client_secret: SiteSetting.openid_connect_client_secret,
  client_options: {

spec/lib/omniauth_open_id_connect_spec.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+require_relative '../../lib/omniauth_open_id_connect'
+
+require 'webmock/rspec'
+WebMock.disable_net_connect!
+
+describe OmniAuth::Strategies::OpenIDConnect do
+ # let(:request) { double('Request', params: {}, cookies: {}, env: {}) }
+ let(:app) do
+ lambda do
+ [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)
+ 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|
+ # allow(strategy).to receive(:request) do
+ # request
+ # end
+ end
+ 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)
+
+ 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)
+
+ subject.discover!
+ expect(subject.options.use_userinfo).to eq(false)
+ end
+
+ context 'with valid document' 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)
+ end
+
+ it "discovers correctly" do
+ subject.discover!
+ expect(subject.options.client_options.site).to eq("https://id.example.com/")
+ expect(subject.options.client_options.authorize_url).to eq("https://id.example.com/authorize")
+ expect(subject.options.client_options.token_url).to eq("https://id.example.com/token")
+ expect(subject.options.client_options.userinfo_endpoint).to eq("https://id.example.com/userinfo")
+ end
+ end
+
+end