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.

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
 

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"

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
 

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

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"]

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