DEV: Only run omniauth strategies for enabled authenticators (#24094)

Previously, we would build the stack of omniauth authenticators once on boot. That meant that all strategies had to be included, even if they were disabled. We then used the `before_request_phase` to ensure disabled strategies could not be used. This works well, but it means that omniauth is often doing unnecessary work running logic in disabled strategies.

This commit refactors things so that we build the stack of strategies on each request. That means we only need to include the enabled strategies in the stack - disabled strategies are totally ignored. Building the stack on-demand like this does add some overhead to auth requests, but on the majority of sites that will be significantly outweighed by the fact we're now skipping logic for disabled authenticators.

As well as the slight performance improvement, this new approach means that:

- Broken (i.e. exception-raising) strategies cannot cause issues on a site if they're disabled

- `other_phase` of disabled strategies will never appear in the backtrace of other authentication errors
This commit is contained in:
David Taylor 2023-10-25 13:52:33 +01:00 committed by GitHub
parent 0e37ceeeb9
commit 5c38e55dc9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 55 additions and 17 deletions

View File

@ -13,27 +13,12 @@ class Middleware::OmniauthBypassMiddleware
Discourse.plugins.each(&:notify_before_auth)
# if you need to test this and are having ssl issues see:
# http://stackoverflow.com/questions/6756460/openssl-error-using-omniauth-specified-ssl-path-but-didnt-work
# OpenSSL::SSL::VERIFY_PEER = OpenSSL::SSL::VERIFY_NONE if Rails.env.development?
@omniauth =
OmniAuth::Builder.new(app) do
Discourse.authenticators.each { |authenticator| authenticator.register_middleware(self) }
end
@omniauth.before_request_phase do |env|
OmniAuth.config.before_request_phase do |env|
request = ActionDispatch::Request.new(env)
# Check for CSRF token in POST requests
CSRFTokenVerifier.new.call(env) if request.request_method.downcase.to_sym != :get
# Check whether the authenticator is enabled
if !Discourse.enabled_authenticators.any? { |a|
a.name.to_sym == env["omniauth.strategy"].name.to_sym
}
raise AuthenticatorDisabled
end
# If the user is trying to reconnect to an existing account, store in session
request.session[:auth_reconnect] = !!request.params["reconnect"]
@ -50,7 +35,14 @@ class Middleware::OmniauthBypassMiddleware
!SiteSetting.enable_local_logins && Discourse.enabled_authenticators.length == 1
OmniAuth.config.allowed_request_methods = only_one_provider ? %i[get post] : [:post]
@omniauth.call(env)
omniauth =
OmniAuth::Builder.new(@app) do
Discourse.enabled_authenticators.each do |authenticator|
authenticator.register_middleware(self)
end
end
omniauth.call(env)
rescue AuthenticatorDisabled => e
# Authenticator is disabled, pretend it doesn't exist and pass request to app
@app.call(env)

View File

@ -1102,4 +1102,50 @@ RSpec.describe Users::OmniauthCallbacksController do
Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:google_oauth2]
end
end
describe "with a fake provider" do
class FakeAuthenticator < Auth::ManagedAuthenticator
class Strategy
include OmniAuth::Strategy
def other_phase
[418, {}, "I am a teapot"]
end
end
def name
"fake"
end
def enabled?
false
end
def register_middleware(omniauth)
omniauth.provider Strategy, name: :fake
end
end
before do
DiscoursePluginRegistry.register_auth_provider(
Auth::AuthProvider.new(authenticator: FakeAuthenticator.new),
)
OmniAuth.config.test_mode = false
end
it "does not run 'other_phase' for disabled auth methods" do
get "/auth/fake/blah"
expect(response.status).to eq(404)
end
it "does not leak 'other_phase' for disabled auth methods onto other methods" do
get "/auth/twitter/blah"
expect(response.status).to eq(404)
end
it "runs 'other_phase' for enabled auth methods" do
FakeAuthenticator.any_instance.stubs(:enabled?).returns(true)
get "/auth/fake/blah"
expect(response.status).to eq(418)
end
end
end