From 750802bf5618ca40cf296b576c49b28bcf07af72 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 12 Aug 2019 10:55:02 +0100 Subject: [PATCH] UX: Improve error handling for common OmniAuth exceptions (#7991) This displays more useful messages for the most common issues we see: - CSRF (when the user switches browser) - Invalid IAT (when the server clock is wrong) - OAuth::Unauthorized for OAuth1 providers, when the credentials are incorrect This commit also stops earlier for disabled authenticators. Now we stop at the request phase, rather than the callback phase. --- .../users/omniauth_callbacks_controller.rb | 3 +- config/locales/server.en.yml | 6 +++- lib/csrf_token_verifier.rb | 4 ++- lib/middleware/omniauth_bypass_middleware.rb | 28 ++++++++++++++++- .../omniauth_callbacks_controller_spec.rb | 30 +++++++++++++++++-- .../failure.html.erb_spec.rb | 2 +- 6 files changed, 65 insertions(+), 8 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index e9ed2d0f5e4..fb3578bacc1 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -91,7 +91,8 @@ class Users::OmniauthCallbacksController < ApplicationController end def failure - flash[:error] = I18n.t("login.omniauth_error") + error_key = params[:message].to_s.gsub(/[^\w-]/, "") || "generic" + flash[:error] = I18n.t("login.omniauth_error.#{error_key}", default: I18n.t("login.omniauth_error.generic")) render 'failure' end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5c4d88421de..a3c13c2d431 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2224,7 +2224,11 @@ en: errors: "%{errors}" not_available: "Not available. Try %{suggestion}?" something_already_taken: "Something went wrong, perhaps the username or email is already registered. Try the forgot password link." - omniauth_error: "Sorry, there was an error authorizing your account. Perhaps you did not approve authorization?" + omniauth_error: + generic: "Sorry, there was an error authorizing your account. Please try again." + csrf_detected: "Authorization timed out, or you have switched browsers. Please try again." + request_error: "An error occured when starting authorization. Please try again." + invalid_iat: "Unable to verify authorization token due to server clock differences. Please try again." omniauth_error_unknown: "Something went wrong processing your log in, please try again." omniauth_confirm_title: "Log in using %{provider}" omniauth_confirm_button: "Continue" diff --git a/lib/csrf_token_verifier.rb b/lib/csrf_token_verifier.rb index 1c1842419d9..875e65ededf 100644 --- a/lib/csrf_token_verifier.rb +++ b/lib/csrf_token_verifier.rb @@ -2,6 +2,8 @@ # Provides a way to check a CSRF token outside of a controller class CSRFTokenVerifier + class InvalidCSRFToken < StandardError; end + include ActiveSupport::Configurable include ActionController::RequestForgeryProtection @@ -17,7 +19,7 @@ class CSRFTokenVerifier @request = ActionDispatch::Request.new(env.dup) unless verified_request? - raise ActionController::InvalidAuthenticityToken + raise InvalidCSRFToken end end diff --git a/lib/middleware/omniauth_bypass_middleware.rb b/lib/middleware/omniauth_bypass_middleware.rb index 8f4fdf27805..c40c8924d12 100644 --- a/lib/middleware/omniauth_bypass_middleware.rb +++ b/lib/middleware/omniauth_bypass_middleware.rb @@ -5,6 +5,7 @@ require "csrf_token_verifier" # omniauth loves spending lots cycles in its magic middleware stack # this middleware bypasses omniauth middleware and only hits it when needed class Middleware::OmniauthBypassMiddleware + class AuthenticatorDisabled < StandardError; end def initialize(app, options = {}) @app = app @@ -24,6 +25,11 @@ class Middleware::OmniauthBypassMiddleware # Check for CSRF token CSRFTokenVerifier.new.call(env) + # Check whether the authenticator is enabled + if !Discourse.enabled_authenticators.any? { |a| a.name == env['omniauth.strategy'].name } + raise AuthenticatorDisabled + end + # If the user is trying to reconnect to an existing account, store in session request = ActionDispatch::Request.new(env) request.session[:auth_reconnect] = !!request.params["reconnect"] @@ -32,7 +38,27 @@ class Middleware::OmniauthBypassMiddleware def call(env) if env["PATH_INFO"].start_with?("/auth") - @omniauth.call(env) + begin + @omniauth.call(env) + rescue AuthenticatorDisabled => e + # Authenticator is disabled, pretend it doesn't exist and pass request to app + @app.call(env) + rescue OAuth::Unauthorized => e + # OAuth1 (i.e. Twitter) makes a web request during the setup phase + # If it fails, Omniauth does not handle the error. Handle it here + env["omniauth.error.type"] ||= "request_error" + Rails.logger.error "Authentication failure! request_error: #{e.class}, #{e.message}" + OmniAuth::FailureEndpoint.call(env) + rescue JWT::InvalidIatError => e + # Happens for openid-connect (including google) providers, when the server clock is wrong + env["omniauth.error.type"] ||= "invalid_iat" + Rails.logger.error "Authentication failure! invalid_iat: #{e.class}, #{e.message}" + OmniAuth::FailureEndpoint.call(env) + rescue CSRFTokenVerifier::InvalidCSRFToken => e + # Happens when CSRF token is missing from request + env["omniauth.error.type"] ||= "csrf_detected" + OmniAuth::FailureEndpoint.call(env) + end else @app.call(env) end diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 2f4df1be52a..63850f66d16 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -87,7 +87,7 @@ RSpec.describe Users::OmniauthCallbacksController do it "should display the failure message if needed" do get "/auth/failure" expect(response.status).to eq(200) - expect(response.body).to include(I18n.t("login.omniauth_error")) + expect(response.body).to include(I18n.t("login.omniauth_error.generic")) end describe "request" do @@ -98,6 +98,28 @@ RSpec.describe Users::OmniauthCallbacksController do expect(response.status).to eq(403) end + it "should error for disabled authenticators" do + SiteSetting.enable_google_oauth2_logins = false + post "/auth/google_oauth2" + expect(response.status).to eq(404) + get "/auth/google_oauth2" + expect(response.status).to eq(403) + end + + it "should handle common errors" do + OmniAuth::Strategies::GoogleOauth2.any_instance.stubs(:mock_request_call).raises( + OAuth::Unauthorized.new(mock().tap { |m| m.stubs(:code).returns(403); m.stubs(:message).returns("Message") }) + ) + post "/auth/google_oauth2" + expect(response.status).to eq(302) + expect(response.location).to include("/auth/failure?message=request_error") + + OmniAuth::Strategies::GoogleOauth2.any_instance.stubs(:mock_request_call).raises(JWT::InvalidIatError.new) + post "/auth/google_oauth2" + expect(response.status).to eq(302) + expect(response.location).to include("/auth/failure?message=invalid_iat") + end + it "should only start auth with a POST request" do post "/auth/google_oauth2" expect(response.status).to eq(302) @@ -111,10 +133,12 @@ RSpec.describe Users::OmniauthCallbacksController do it "should be CSRF protected" do post "/auth/google_oauth2" - expect(response.status).to eq(422) + expect(response.status).to eq(302) + expect(response.location).to include("/auth/failure?message=csrf_detected") post "/auth/google_oauth2", params: { authenticity_token: "faketoken" } - expect(response.status).to eq(422) + expect(response.status).to eq(302) + expect(response.location).to include("/auth/failure?message=csrf_detected") get "/session/csrf.json" token = JSON.parse(response.body)["csrf"] diff --git a/spec/views/omniauth_callbacks/failure.html.erb_spec.rb b/spec/views/omniauth_callbacks/failure.html.erb_spec.rb index a96dc160745..78592b2c6c9 100644 --- a/spec/views/omniauth_callbacks/failure.html.erb_spec.rb +++ b/spec/views/omniauth_callbacks/failure.html.erb_spec.rb @@ -8,7 +8,7 @@ describe "users/omniauth_callbacks/failure.html.erb" do flash[:error] = I18n.t("login.omniauth_error", strategy: 'test') render - expect(rendered.match(I18n.t("login.omniauth_error", strategy: 'test'))).not_to eq(nil) + expect(rendered.match(I18n.t("login.omniauth_error.generic", strategy: 'test'))).not_to eq(nil) end end