From 78a792b5b6c9ff8814618979e97fe8bcaeb1d6b3 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 21 Nov 2018 15:27:47 +0000 Subject: [PATCH] FIX: Improved 'discovery' error handling, with tests --- .travis.yml | 12 ++++ config/locales/server.en.yml | 1 - config/settings.yml | 2 - lib/omniauth_open_id_connect.rb | 52 ++++++++++---- plugin.rb | 1 - spec/lib/omniauth_open_id_connect_spec.rb | 86 +++++++++++++++++++++++ 6 files changed, 136 insertions(+), 18 deletions(-) create mode 100644 .travis.yml create mode 100644 spec/lib/omniauth_open_id_connect_spec.rb diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..88c7810 --- /dev/null +++ b/.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 \ No newline at end of file diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 2927213..53a3faa 100644 --- a/config/locales/server.en.yml +++ b/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" \ No newline at end of file diff --git a/config/settings.yml b/config/settings.yml index 33c00a7..4e0457f 100644 --- a/config/settings.yml +++ b/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 diff --git a/lib/omniauth_open_id_connect.rb b/lib/omniauth_open_id_connect.rb index 981b53e..09d6b50 100644 --- a/lib/omniauth_open_id_connect.rb +++ b/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 @@ module ::OmniAuth 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 @@ module ::OmniAuth 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 @@ module ::OmniAuth 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 diff --git a/plugin.rb b/plugin.rb index 39bd8a0..15818ee 100644 --- a/plugin.rb +++ b/plugin.rb @@ -83,7 +83,6 @@ class OpenIDConnectAuthenticator < Auth::ManagedAuthenticator 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: { diff --git a/spec/lib/omniauth_open_id_connect_spec.rb b/spec/lib/omniauth_open_id_connect_spec.rb new file mode 100644 index 0000000..915ba7a --- /dev/null +++ b/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