From 0ca28b39c2dca6d4c39fbe053ea7a02202d6f375 Mon Sep 17 00:00:00 2001 From: jcsawyer123 Date: Mon, 14 Sep 2026 23:07:17 +0100 Subject: [PATCH 1/4] [DEV-3974] Add account-switch interstitial and Core return (#23785) * Add provider-neutral account-switch interstitial Completes the cross-account guard from #23783: the Authenticator already raises AccountSwitchConfirmation when a signed-in user's incoming identity resolves to a different account, but nothing rescued it. Now the callback stages the OmniAuth payload encrypted in the session, renders a confirm/cancel interstitial, and only attaches the identity and swaps the session after explicit confirmation. The Authenticator re-verifies the staged target with expected_user so a target that changed email, became unconfirmed, was suspended, or whose identity was reassigned in the meantime is rejected without mutation. Nothing in this layer knows about a specific provider. Providers that must not hold OAuth credentials beyond the callback request declare `persist_credentials?` false and have them blanked before staging; MLH does so because access is brokered upstream. Co-Authored-By: Claude Fable 5.1 * Add temporary MLH Core return bridge Everything MLH-Core-specific now lives in two clearly marked places: - Authentication::MlhCoreBridge points the omniauth-mlh strategy at Core (MLH_OAUTH_BASE_URL / MLH_API_BASE_URL), narrows scopes to what Core issues, and disables credential persistence. It carries the removal checklist for the whole bridge. - Authentication::ExternalReturn returns the browser to Core after sign-in. The controller only sees an opaque capture/resolve interface: `capture` turns OmniAuth request params into a session-safe context that rides through the account-switch interstitial, and `resolve` turns it back into a redirect URL. The `continuation` wire format is known only to this class, so swapping in OIDC target_link_uri later touches nothing else. The bridge is inert unless FOREM_EXTERNAL_RETURN_ENABLED=true and an HTTPS FOREM_EXTERNAL_RETURN_URL is configured. The Core-matching seed users only load when the bridge is configured. The earlier after_sign_in_path_for hook is intentionally gone: the callback short-circuits to the return URL before Devise's redirect runs, so the hook only widened the bridge into every password sign-in for no benefit. Co-Authored-By: Claude Fable 5.1 * Pin bullseye-security apt source to snapshot.debian.org Container builds fail in the builder stage's apt install with 404s for bullseye-security packages. Bullseye LTS ended 2026-08-31 and the bullseye-security pool is being purged from deb.debian.org and security.debian.org: their CDN edges return 404 for .deb files the index still lists, and which edge you hit decides whether the build passes. archive.debian.org has not picked the suite up yet. snapshot.debian.org keeps every package permanently, so pin the security source to a snapshot from the last LTS day in the base stage so every derived stage builds again. This is a stopgap until the base image moves to a supported Debian release. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Ben Halpern Co-authored-by: Claude Fable 5.1 --- .env_sample | 9 + Containerfile | 10 ++ .../omniauth_callbacks_controller.rb | 124 +++++++++++++- .../packs/onboardingRedirectCheck.jsx | 2 + app/services/authentication/authenticator.rb | 11 +- .../authentication/external_return.rb | 70 ++++++++ .../authentication/mlh_core_bridge.rb | 45 +++++ app/services/authentication/providers/mlh.rb | 6 + .../authentication/providers/provider.rb | 8 + .../account_switch.html.erb | 18 ++ config/initializers/devise.rb | 3 + .../initializers/filter_parameter_logging.rb | 2 +- config/locales/controllers/en.yml | 5 + config/locales/controllers/fr.yml | 5 + config/locales/controllers/pt.yml | 5 + config/routes.rb | 4 + db/seeds.rb | 37 +++++ spec/initializers/mlh_omniauth_setup_spec.rb | 22 +++ .../account_switch_interstitial_spec.rb | 156 ++++++++++++++++++ spec/requests/mlh_oauth_callbacks_spec.rb | 120 ++++++++++++++ .../authentication/external_return_spec.rb | 82 +++++++++ .../authentication/providers/github_spec.rb | 6 + .../authentication/providers/mlh_spec.rb | 6 + spec/support/omniauth_session_helpers.rb | 35 ++++ 24 files changed, 783 insertions(+), 8 deletions(-) create mode 100644 app/services/authentication/external_return.rb create mode 100644 app/services/authentication/mlh_core_bridge.rb create mode 100644 app/views/omniauth_callbacks/account_switch.html.erb create mode 100644 spec/requests/account_switch_interstitial_spec.rb create mode 100644 spec/requests/mlh_oauth_callbacks_spec.rb create mode 100644 spec/services/authentication/external_return_spec.rb create mode 100644 spec/support/omniauth_session_helpers.rb diff --git a/.env_sample b/.env_sample index 7ba64c99ab7b5..eef1527342725 100644 --- a/.env_sample +++ b/.env_sample @@ -197,3 +197,12 @@ COVERAGE="false" # MLH API URL override (optional) MLH_API_BASE_URL= + +# TEMPORARY MLH Core bridge (see app/services/authentication/mlh_core_bridge.rb). +# Setting MLH_OAUTH_BASE_URL routes MLH sign-in through Core instead of my.mlh.io. +MLH_OAUTH_BASE_URL= +# External sign-in return; disabled unless explicitly set to true. To be replaced +# by OIDC third-party initiated login (target_link_uri). +FOREM_EXTERNAL_RETURN_ENABLED=false +# Full HTTPS endpoint, no query or fragment. Core integration: https://www.mlh.test/oauth/dev +FOREM_EXTERNAL_RETURN_URL= diff --git a/Containerfile b/Containerfile index c04a91d873a8f..e49c494c58996 100644 --- a/Containerfile +++ b/Containerfile @@ -3,6 +3,16 @@ FROM ghcr.io/forem/ruby:3.3.0@sha256:9cda49a45931e9253d58f7d561221e43bd0d47676b8 # HOTFIX: Remove broken nodesource list from base image to unblock apt update RUN rm -f /etc/apt/sources.list.d/nodesource.list +# HOTFIX: bullseye LTS ended 2026-08-31 and the bullseye-security pool is +# being purged from deb.debian.org / security.debian.org (their CDN edges 404 +# on .deb files the index still lists) while archive.debian.org has not +# picked the suite up yet. snapshot.debian.org keeps every package forever, so +# pin the security source to a snapshot from the last LTS day until the base +# image moves to a supported Debian release. +RUN find /etc/apt -type f \( -name "sources.list" -o -name "*.list" -o -name "*.sources" \) \ + -exec sed -i "s|http://[a-z.]*debian.org/debian-security|http://snapshot.debian.org/archive/debian-security/20260831T000000Z|g" {} + \ + && echo 'Acquire::Check-Valid-Until "false";' > /etc/apt/apt.conf.d/99snapshot + FROM base as builder # This is provided by BuildKit diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index ff038039a4c99..525bcb6940af7 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -33,7 +33,7 @@ def failure "uri:#{error.try(:error_uri)}", "provider:#{request.env['omniauth.strategy'].name}", "origin:#{request.env['omniauth.strategy.origin']}", - "params:#{request.env['omniauth.params']}", + "params:#{loggable_omniauth_params}", ], ) @@ -58,8 +58,115 @@ def passthru redirect_to root_path(signin: "true") end + def confirm_account_switch + pending = session.delete("pending_account_switch") + target = pending && User.find_by(id: pending["user_id"]) + + if invalid_pending_switch?(pending, target) + flash[:alert] = I18n.t("omniauth_callbacks_controller.account_switch_expired") + return redirect_to root_path + end + + payload = decrypt_pending_switch(pending) + return redirect_to(root_path) unless payload + + @user = Authentication::Authenticator.call(OmniAuth::AuthHash.new(payload), expected_user: target) + return redirect_to(root_path) unless user_persisted_and_valid? && @user.confirmed? + + complete_account_switch(pending) + rescue ::Authentication::Errors::Ineligible, ::Authentication::Errors::PreviouslySuspended, + ::Authentication::Errors::SpammyEmailDomain => e + flash[:global_notice] = e.message + redirect_to root_path + rescue ActiveRecord::RecordInvalid => e + flash[:alert] = e.record&.errors&.full_messages&.join(", ").presence || + I18n.t("omniauth_callbacks_controller.log_in_error", e: e.message) + redirect_to new_user_registration_url + rescue StandardError => e + Honeybadger.notify(e) + flash[:alert] = I18n.t("omniauth_callbacks_controller.log_in_error", e: e) + redirect_to new_user_registration_url + end + + def cancel_account_switch + session.delete("pending_account_switch") + redirect_to root_path + end + private + # Cross-account sign-in: the caller is signed in as one user and the incoming + # identity resolves to another. Nothing is persisted until the person confirms + # on the interstitial; the OmniAuth payload is held encrypted in the session. + def stage_account_switch(target_user, provider) + session["pending_account_switch"] = { + "user_id" => target_user.id, + "provider" => provider.to_s, + "payload" => account_switch_encryptor.encrypt_and_sign(stageable_auth_payload(provider), expires_in: 15.minutes), + "return_context" => Authentication::ExternalReturn.capture(request.env["omniauth.params"]), + "staged_at" => Time.current.iso8601 + } + @switch_target_username = target_user.username + render :account_switch + end + + def invalid_pending_switch?(pending, target) + pending.nil? || stale_switch?(pending) || target.nil? || target.spam_or_suspended? + end + + # OmniAuth request params can carry values we filter from logs (see + # config/initializers/filter_parameter_logging.rb); apply the same filter to + # telemetry tags. Non-Hash values are passed through untouched. + def loggable_omniauth_params + params = request.env["omniauth.params"] + return params unless params.is_a?(Hash) + + ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters).filter(params) + end + + def stale_switch?(pending) + Time.zone.iso8601(pending["staged_at"].to_s) < 15.minutes.ago + rescue ArgumentError, TypeError + true + end + + # Providers that must not hold OAuth credentials beyond the callback request + # get them blanked before staging; everyone else keeps them (encrypted) so the + # identity can be created with a token once the switch is confirmed. + def stageable_auth_payload(provider) + payload = request.env["omniauth.auth"].to_hash + payload["credentials"] = { "expires" => false } unless Authentication::Providers.get!(provider).persist_credentials? + payload + end + + def account_switch_encryptor + key = Rails.application.key_generator.generate_key("account-switch-payload", 32) + ActiveSupport::MessageEncryptor.new(key, cipher: "aes-256-gcm", serializer: JSON) + end + + # Returns the staged OmniAuth payload, or nil if it expired, was tampered + # with, or no longer matches the provider recorded at staging time. + def decrypt_pending_switch(pending) + payload = account_switch_encryptor.decrypt_and_verify(pending["payload"]) + payload if payload&.fetch("provider", nil) == pending["provider"] + rescue ActiveSupport::MessageEncryptor::InvalidMessage + nil + end + + def complete_account_switch(pending) + sign_out(current_user) if current_user + sign_in(@user, event: :authentication) + + set_flash_message(:notice, :success, kind: pending["provider"].to_s.titleize) if is_navigational_format? + @user.update_tracked_fields!(request) + remember_me(@user) + + return_url = Authentication::ExternalReturn.resolve(pending["return_context"]) + return redirect_to(return_url, allow_other_host: true) if return_url + + sign_in_and_redirect(@user, event: :authentication) + end + def callback_for(provider) auth_payload = request.env["omniauth.auth"] cta_variant = request.env["omniauth.params"]["state"].to_s @@ -84,7 +191,10 @@ def callback_for(provider) user_agent = request.user_agent - if ApplicationConfig["AUTH_TEST_USER_IDS"].present? && ApplicationConfig["AUTH_TEST_USER_IDS"].split(",").include?(@user.id.to_s) + if (external_return = Authentication::ExternalReturn.redirect_url_for(request.env["omniauth.params"])) + sign_in(@user, event: :authentication) + redirect_to external_return, allow_other_host: true + elsif ApplicationConfig["AUTH_TEST_USER_IDS"].present? && ApplicationConfig["AUTH_TEST_USER_IDS"].split(",").include?(@user.id.to_s) token = generate_auth_token(@user) test_path = ApplicationConfig["AUTH_TEST_USER_REDIRECT_PATH"] || "/menu" redirect_to "#{test_path}?jwt=#{token}" @@ -111,7 +221,7 @@ def callback_for(provider) # Handle error conditions. session["devise.#{provider}_data"] = request.env["omniauth.auth"] user_errors = @user.errors.full_messages - + Honeybadger.context({ username: @user.username, user_id: @user.id, @@ -120,11 +230,14 @@ def callback_for(provider) user_errors: user_errors }) Honeybadger.notify("Omniauth log in error") - + flash[:alert] = user_errors redirect_to new_user_registration_url end - rescue ::Authentication::Errors::PreviouslySuspended, ::Authentication::Errors::SpammyEmailDomain => e + rescue ::Authentication::Errors::AccountSwitchConfirmation => e + stage_account_switch(e.target_user, provider) + rescue ::Authentication::Errors::Ineligible, ::Authentication::Errors::PreviouslySuspended, + ::Authentication::Errors::SpammyEmailDomain => e flash[:global_notice] = e.message redirect_to root_path @@ -136,7 +249,6 @@ def callback_for(provider) flash[:alert] = I18n.t("omniauth_callbacks_controller.log_in_error", e: e) redirect_to new_user_registration_url end - def user_persisted_and_valid? @user.persisted? && @user.valid? diff --git a/app/javascript/packs/onboardingRedirectCheck.jsx b/app/javascript/packs/onboardingRedirectCheck.jsx index 95e2d0892b8fc..519cb6409ba2e 100644 --- a/app/javascript/packs/onboardingRedirectCheck.jsx +++ b/app/javascript/packs/onboardingRedirectCheck.jsx @@ -9,6 +9,8 @@ HTMLDocument.prototype.ready = new Promise((resolve) => { }); function redirectableLocation() { + if (document.querySelector('[data-account-switch-confirmation]')) return false; + return ![ '/onboarding', '/signout_confirm', diff --git a/app/services/authentication/authenticator.rb b/app/services/authentication/authenticator.rb index 364abaca43597..4ede5de536913 100644 --- a/app/services/authentication/authenticator.rb +++ b/app/services/authentication/authenticator.rb @@ -25,10 +25,11 @@ def self.call(...) end # auth_payload is the payload schema, see https://github.com/omniauth/omniauth/wiki/Auth-Hash-Schema - def initialize(auth_payload, current_user: nil, cta_variant: nil) + def initialize(auth_payload, current_user: nil, cta_variant: nil, expected_user: nil) @provider = load_authentication_provider(auth_payload) @current_user = current_user + @expected_user = expected_user @cta_variant = cta_variant end @@ -51,6 +52,7 @@ def call ActiveRecord::Base.transaction do user = proper_user(identity) + verify_expected_user!(user) user = if user.nil? find_or_create_user! @@ -187,6 +189,13 @@ def verified_email_user user&.confirmed? ? user : nil end + def verify_expected_user!(user) + return unless @expected_user + return if user == @expected_user && user.confirmed? && !user.spam_or_suspended? + + raise ::Authentication::Errors::Ineligible + end + def proper_user(identity) if current_user Rails.logger.debug { "Current user exists: #{current_user.id}" } diff --git a/app/services/authentication/external_return.rb b/app/services/authentication/external_return.rb new file mode 100644 index 0000000000000..17419e4932db7 --- /dev/null +++ b/app/services/authentication/external_return.rb @@ -0,0 +1,70 @@ +module Authentication + # TEMPORARY: returns the browser to an external application after sign-in. + # + # Today this is the MLH Core round trip: Core starts sign-in with an opaque + # `continuation` token and expects the browser back at FOREM_EXTERNAL_RETURN_URL + # carrying that token. The standard replacement is OIDC third-party initiated + # login with `target_link_uri`; when that ships, only this class (and the + # parameter-filter entry) should need to change. See Authentication::MlhCoreBridge + # for the removal checklist. + # + # Callers never inspect the wire format: `capture` turns the OmniAuth request + # params into an opaque, session-safe context, and `resolve` turns that context + # back into a redirect URL (or nil). Both are inert unless the bridge is + # explicitly enabled. + class ExternalReturn + CONTINUATION_PATTERN = /\A[A-Za-z0-9_\-]+\z/ + + # @return [Hash, nil] opaque context to carry across the interstitial + def self.capture(omniauth_params) + continuation = (omniauth_params || {})["continuation"].to_s + return unless continuation.match?(CONTINUATION_PATTERN) + + { "continuation" => continuation } + end + + # @return [String, nil] absolute URL to redirect to, or nil to fall through + def self.resolve(context) + new(context).redirect_url + end + + # Convenience for the direct (non-interstitial) callback. + def self.redirect_url_for(omniauth_params) + resolve(capture(omniauth_params)) + end + + def initialize(context) + @context = context || {} + end + + def redirect_url + continuation = @context["continuation"].to_s + return unless continuation.match?(CONTINUATION_PATTERN) + + entry = configured_uri + return unless entry + + "#{origin_of(entry)}#{entry.path}?continuation=#{continuation}" + end + + private + + # The receiving application owns the path; Forem only trusts this configured endpoint. + def configured_uri + return unless ENV.fetch("FOREM_EXTERNAL_RETURN_ENABLED", "false") == "true" + + uri = Addressable::URI.parse(ENV.fetch("FOREM_EXTERNAL_RETURN_URL", "").strip) + return unless uri.scheme == "https" && uri.host.present? && uri.path.start_with?("/") + return if uri.userinfo || uri.query || uri.fragment + + uri + rescue Addressable::URI::InvalidURIError + nil + end + + def origin_of(uri) + port = uri.port.nil? || uri.default_port == uri.port ? "" : ":#{uri.port}" + "#{uri.scheme}://#{uri.host}#{port}" + end + end +end diff --git a/app/services/authentication/mlh_core_bridge.rb b/app/services/authentication/mlh_core_bridge.rb new file mode 100644 index 0000000000000..5b68b87cec86e --- /dev/null +++ b/app/services/authentication/mlh_core_bridge.rb @@ -0,0 +1,45 @@ +module Authentication + # TEMPORARY: MLH "Core" integration bridge. + # + # When MLH_OAUTH_BASE_URL is set, MLH sign-in is brokered by MLH Core, which + # also proxies the MyMLH API on Forem's behalf. Everything Core-specific lives + # here and in Authentication::ExternalReturn so the integration can be removed + # in one place once Core speaks standard OIDC third-party initiated login + # (target_link_uri) instead of the custom continuation round trip. + # + # Removal checklist: + # - this file and Authentication::ExternalReturn (plus their specs) + # - the MlhCoreBridge call in config/initializers/devise.rb + # - the ExternalReturn call sites in OmniauthCallbacksController + # - MLH_OAUTH_BASE_URL, MLH_API_BASE_URL and FOREM_EXTERNAL_RETURN_* in .env_sample + # - "continuation" in config/initializers/filter_parameter_logging.rb + # - the gated block in db/seeds.rb + # - the Core sections of spec/requests/mlh_oauth_callbacks_spec.rb and + # spec/initializers/mlh_omniauth_setup_spec.rb + module MlhCoreBridge + # Core only issues these; anything wider is stripped from the request so + # the authorize call does not fail on an unknown scope. + PROXIED_SCOPES = %w[public user:read:profile mlh:read:user].freeze + + def self.enabled? + ENV["MLH_OAUTH_BASE_URL"].present? + end + + # Points the omniauth-mlh strategy at Core instead of my.mlh.io. + def self.apply!(strategy) + if enabled? + oauth_base = ENV["MLH_OAUTH_BASE_URL"].chomp("/") + strategy.options[:client_options][:site] = oauth_base + strategy.options[:client_options][:authorize_url] = "#{oauth_base}/oauth/authorize" + strategy.options[:client_options][:token_url] = "#{oauth_base}/oauth/token" + strategy.options[:scope] = strategy.options[:scope].split.intersection(PROXIED_SCOPES).join(" ") + # Core proxies provider API calls, so Forem must not persist bearer credentials. + strategy.options[:persist_credentials] = false + end + + return if ENV["MLH_API_BASE_URL"].blank? + + strategy.options[:client_options][:api_site] = ENV["MLH_API_BASE_URL"].chomp("/") + end + end +end diff --git a/app/services/authentication/providers/mlh.rb b/app/services/authentication/providers/mlh.rb index d616f566d411f..2dc2c7d3b41d4 100644 --- a/app/services/authentication/providers/mlh.rb +++ b/app/services/authentication/providers/mlh.rb @@ -15,6 +15,12 @@ def self.settings_url SETTINGS_URL end + # MLH access is brokered upstream; Forem never needs the bearer token + # after the callback, so it is not carried through an account switch. + def self.persist_credentials? + false + end + def self.sign_in_path(**kwargs) # For MLH, we do not inject a callback_url param; OmniAuth will use its # configured callback path, which must match the URL registered in MyMLH. diff --git a/app/services/authentication/providers/provider.rb b/app/services/authentication/providers/provider.rb index a3cceaa46a76c..c2f42ad67466f 100644 --- a/app/services/authentication/providers/provider.rb +++ b/app/services/authentication/providers/provider.rb @@ -29,6 +29,14 @@ def self.sign_in_path(**_kwargs) raise SubclassResponsibility end + # Whether Forem may hold this provider's OAuth credentials outside the + # callback request itself, e.g. while an account switch awaits + # confirmation. Providers whose upstream proxies API access on Forem's + # behalf override this so bearer material never leaves the callback. + def self.persist_credentials? + true + end + def initialize(auth_payload) @auth_payload = cleanup_payload(auth_payload.dup) @info = auth_payload.info diff --git a/app/views/omniauth_callbacks/account_switch.html.erb b/app/views/omniauth_callbacks/account_switch.html.erb new file mode 100644 index 0000000000000..056bdeaf230fe --- /dev/null +++ b/app/views/omniauth_callbacks/account_switch.html.erb @@ -0,0 +1,18 @@ +
+
+

<%= t("omniauth_callbacks_controller.account_switch_title") %>

+

+ <%= t("omniauth_callbacks_controller.account_switch_body_html", username: content_tag(:strong, @switch_target_username)) %> +

+
+ <%= button_to t("omniauth_callbacks_controller.account_switch_confirm"), + user_account_switch_confirm_path, + method: :post, + class: "crayons-btn" %> + <%= button_to t("omniauth_callbacks_controller.account_switch_cancel"), + user_account_switch_cancel_path, + method: :post, + class: "crayons-btn crayons-btn--ghost" %> +
+
+
diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 779c60a45720b..7d452f5fb03f7 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -81,6 +81,9 @@ def authorize_params env["omniauth.strategy"].options[:client_secret] = Settings::Authentication.mlh_secret # Note: redirect_uri is handled by the prepended MlhCallbackUrlOverride module # which overrides both callback_url and authorize_params to ensure no query parameters + + # TEMPORARY: MLH Core proxy endpoints; no-op unless MLH_OAUTH_BASE_URL / MLH_API_BASE_URL are set. + Authentication::MlhCoreBridge.apply!(env["omniauth.strategy"]) end Devise.setup do |config| diff --git a/config/initializers/filter_parameter_logging.rb b/config/initializers/filter_parameter_logging.rb index fd60d015014a8..b64a6bbdad247 100644 --- a/config/initializers/filter_parameter_logging.rb +++ b/config/initializers/filter_parameter_logging.rb @@ -2,5 +2,5 @@ # Configure sensitive parameters which will be filtered from the log file. Rails.application.config.filter_parameters += %i[ - password passw secret token _key crypt salt certificate otp ssn + password passw secret token _key crypt salt certificate otp ssn continuation ] diff --git a/config/locales/controllers/en.yml b/config/locales/controllers/en.yml index 8c0f9e0d93b87..bb383b9f631e0 100644 --- a/config/locales/controllers/en.yml +++ b/config/locales/controllers/en.yml @@ -76,6 +76,11 @@ en: omniauth_callbacks_controller: log_in_error: 'Log in error: %{e}' username_taken: username has already been taken + account_switch_title: "Confirm account switch" + account_switch_body_html: "You are signed in as a different user. Continue to %{username}? This will end your current session and sign you in to that account." + account_switch_confirm: "Confirm switch" + account_switch_cancel: "Cancel" + account_switch_expired: "This sign-in attempt expired. Please try signing in again." organizations_controller: secret_updated: Your org secret was updated deletion_scheduled: 'Your organization: "%{organization_name}" deletion is scheduled. You''ll be notified when it''s deleted.' diff --git a/config/locales/controllers/fr.yml b/config/locales/controllers/fr.yml index bf5740eeda519..7cb59b1f40369 100644 --- a/config/locales/controllers/fr.yml +++ b/config/locales/controllers/fr.yml @@ -74,6 +74,11 @@ fr: deleted: L'inscription a été supprimée avec succès. no_credit: Pas assez de crédits disponibles omniauth_callbacks_controller: + account_switch_title: "Confirmer le changement de compte" + account_switch_body_html: "Vous êtes connecté avec un autre compte. Continuer avec %{username} ? Votre session actuelle sera fermée et vous serez connecté à ce compte." + account_switch_confirm: "Confirmer le changement" + account_switch_cancel: "Annuler" + account_switch_expired: "Cette tentative de connexion a expiré. Veuillez réessayer." log_in_error: 'Erreur de connexion: %{e}' username_taken: le nom d'utilisateur a déjà été pris organizations_controller: diff --git a/config/locales/controllers/pt.yml b/config/locales/controllers/pt.yml index 9e9f6eb5b0e19..fec4584a05c68 100644 --- a/config/locales/controllers/pt.yml +++ b/config/locales/controllers/pt.yml @@ -74,6 +74,11 @@ pt: deleted: Anúncio foi excluído com sucesso. no_credit: Créditos disponíveis insuficientes omniauth_callbacks_controller: + account_switch_title: "Confirmar troca de conta" + account_switch_body_html: "Você está conectado com outra conta. Continuar como %{username}? Isso encerrará sua sessão atual e conectará você a essa conta." + account_switch_confirm: "Confirmar troca" + account_switch_cancel: "Cancelar" + account_switch_expired: "Esta tentativa de login expirou. Tente entrar novamente." log_in_error: 'Erro de login: %{e}' username_taken: nome de usuário já foi usado organizations_controller: diff --git a/config/routes.rb b/config/routes.rb index dc1ea6dd1c574..b78e0e6b726d0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -16,6 +16,10 @@ get "/enter", to: "registrations#new", as: :sign_up get "/confirm-email", to: "confirmations#new" delete "/sign_out", to: "devise/sessions#destroy" + post "/users/auth/account_switch/confirm", + to: "omniauth_callbacks#confirm_account_switch", as: :user_account_switch_confirm + post "/users/auth/account_switch/cancel", + to: "omniauth_callbacks#cancel_account_switch", as: :user_account_switch_cancel end # This route makes default Ahoy Email redirect URLs available to us diff --git a/db/seeds.rb b/db/seeds.rb index acb917f859d13..2e4cdfd7c1e42 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -1318,6 +1318,43 @@ end end +# TEMPORARY: fixtures matching MLH Core's local development users, so the +# account-switch interstitial can be exercised end to end against Core. +# Only seeded when the Core bridge is configured; see Authentication::MlhCoreBridge. +if Authentication::MlhCoreBridge.enabled? + seeder.create_if_doesnt_exist(User, "email", "bob.devrelay@mlh.test") do + User.create!( + name: "Bob Devrelay", + username: "bob_devrelay", + email: "bob.devrelay@mlh.test", + profile_image: Rails.root.join("app/assets/images/#{rand(1..40)}.png").open, + confirmed_at: Time.current, + registered_at: Time.current, + registered: true, + saw_onboarding: true, + checked_code_of_conduct: true, + checked_terms_and_conditions: true, + password: "password", + password_confirmation: "password", + ) + end + + seeder.create_if_doesnt_exist(User, "email", "carol.devrelay@mlh.test") do + carol = User.create!( + name: "Carol Devrelay", + username: "carol_devrelay", + email: "carol.devrelay@mlh.test", + profile_image: Rails.root.join("app/assets/images/#{rand(1..40)}.png").open, + confirmed_at: Time.current, + registered_at: Time.current, + registered: true, + password: "password", + password_confirmation: "password", + ) + carol.add_role(:suspended) + end +end + puts <<-ASCII :::::::::::::::::::::::::::::::::::::::::::::::::::::::::::: diff --git a/spec/initializers/mlh_omniauth_setup_spec.rb b/spec/initializers/mlh_omniauth_setup_spec.rb index f146ffb0cb45a..9faabff50400f 100644 --- a/spec/initializers/mlh_omniauth_setup_spec.rb +++ b/spec/initializers/mlh_omniauth_setup_spec.rb @@ -8,6 +8,28 @@ MLH_OMNIAUTH_SETUP.call("omniauth.strategy" => strategy) end + context "with local Core endpoints" do + around do |example| + original_oauth = ENV.fetch("MLH_OAUTH_BASE_URL", nil) + original_api = ENV.fetch("MLH_API_BASE_URL", nil) + ENV["MLH_OAUTH_BASE_URL"] = "https://core.example/" + ENV["MLH_API_BASE_URL"] = "https://api.core.example/" + example.run + ensure + ENV["MLH_OAUTH_BASE_URL"] = original_oauth + ENV["MLH_API_BASE_URL"] = original_api + end + + it "uses Core endpoints, limited scopes, and no persisted credentials", :aggregate_failures do + expect(strategy.options.client_options.site).to eq("https://core.example") + expect(strategy.options.client_options.authorize_url).to eq("https://core.example/oauth/authorize") + expect(strategy.options.client_options.token_url).to eq("https://core.example/oauth/token") + expect(strategy.options.client_options.api_site).to eq("https://api.core.example") + expect(strategy.options.scope.split).to contain_exactly("public", "user:read:profile", "mlh:read:user") + expect(strategy.options.persist_credentials).to be(false) + end + end + it "keeps OAuth state verification enabled" do expect(strategy.options.provider_ignores_state).to be(false) end diff --git a/spec/requests/account_switch_interstitial_spec.rb b/spec/requests/account_switch_interstitial_spec.rb new file mode 100644 index 0000000000000..dc8e3e6ef1d15 --- /dev/null +++ b/spec/requests/account_switch_interstitial_spec.rb @@ -0,0 +1,156 @@ +require "rails_helper" + +# Provider-neutral coverage for the account-switch interstitial. GitHub stands in +# for "any OmniAuth provider"; nothing here depends on a specific integration. +RSpec.describe "Account switch interstitial" do + include OmniauthHelpers + include OmniauthSessionHelpers + include ActiveSupport::Testing::TimeHelpers + + before do + allow(ForemStatsClient).to receive(:increment) + allow(Honeybadger).to receive(:notify) + allow(Settings::Authentication).to receive(:providers).and_return(Authentication::Providers.available) + sign_in session_user + end + + after { omniauth_reset_mock } + + def github_payload(uid:, email:, nickname:, token: "github-access-token") + OmniAuth::AuthHash.new( + provider: "github", + uid: uid, + info: OmniAuth::AuthHash::InfoHash.new(email: email, name: "GitHub User", nickname: nickname), + credentials: OmniAuth::AuthHash.new(token: token, secret: "sec"), + extra: { raw_info: { name: "GitHub User", created_at: 2.years.ago.iso8601 } }, + ) + end + + let(:session_user) { create(:user) } + let(:target) { create(:user) } + let(:payload) { github_payload(uid: "810004", email: target.email, nickname: target.username) } + + it "renders the interstitial without mutation, then switches on confirmation", :aggregate_failures do + omniauth_sign_in(:github, payload) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("data-account-switch-confirmation") + expect(response.body).to include(target.username) + expect(response.body).to include("/users/auth/account_switch/confirm") + expect(Identity.where(provider: "github", uid: "810004")).to be_none + expect(signed_in_user_id).to eq(session_user.id) + + post_with_session "/users/auth/account_switch/confirm" + + identity = Identity.find_by!(provider: "github", uid: "810004") + expect(identity.user_id).to eq(target.id) + expect(signed_in_user_id).to eq(target.id) + expect(session["pending_account_switch"]).to be_nil + end + + it "keeps provider credentials encrypted in the pending session and restores them on confirm", :aggregate_failures do + omniauth_sign_in(:github, payload) + + expect(session["pending_account_switch"].to_json).not_to include("github-access-token") + + post_with_session "/users/auth/account_switch/confirm" + + expect(Identity.find_by!(provider: "github", uid: "810004").token).to eq("github-access-token") + end + + it "rejects a target whose email changes before confirmation", :aggregate_failures do + omniauth_sign_in(:github, payload) + # Model callbacks stage reconfirmation; simulate a completed email change. + target.update_columns(email: "changed@example.com") + + expect { post_with_session "/users/auth/account_switch/confirm" }.not_to change(Identity, :count) + expect(signed_in_user_id).to eq(session_user.id) + end + + it "rejects an identity reassigned after staging", :aggregate_failures do + omniauth_sign_in(:github, payload) + other = create(:user) + identity = create(:identity, provider: "github", uid: "810004", user: other, token: "original") + + post_with_session "/users/auth/account_switch/confirm" + + expect(identity.reload.token).to eq("original") + expect(signed_in_user_id).to eq(session_user.id) + end + + it "rejects a target that becomes unconfirmed", :aggregate_failures do + omniauth_sign_in(:github, payload) + target.update_columns(confirmed_at: nil) + + expect { post_with_session "/users/auth/account_switch/confirm" }.not_to change(Identity, :count) + expect(signed_in_user_id).to eq(session_user.id) + end + + it "handles a newly blocked email domain without ending the current session", :aggregate_failures do + omniauth_sign_in(:github, payload) + allow(Settings::Authentication).to receive(:acceptable_domain?).and_return(false) + + post_with_session "/users/auth/account_switch/confirm" + + expect(response).to redirect_to(root_path) + expect(signed_in_user_id).to eq(session_user.id) + expect(Identity.where(provider: "github", uid: "810004")).to be_none + end + + it "rejects an expired staged payload", :aggregate_failures do + omniauth_sign_in(:github, payload) + travel_to 16.minutes.from_now do + post_with_session "/users/auth/account_switch/confirm" + end + + expect(response).to redirect_to(root_path) + expect(signed_in_user_id).to eq(session_user.id) + expect(Identity.where(provider: "github", uid: "810004")).to be_none + end + + it "reports a persistence failure during confirmation without switching sessions", :aggregate_failures do + omniauth_sign_in(:github, payload) + allow(Authentication::Authenticator).to receive(:call).and_wrap_original do |original, *args, **kwargs| + raise ActiveRecord::RecordInvalid, User.new if kwargs[:expected_user] + + original.call(*args, **kwargs) + end + + post_with_session "/users/auth/account_switch/confirm" + + expect(response).to redirect_to(new_user_registration_url) + expect(flash[:alert]).to be_present + expect(signed_in_user_id).to eq(session_user.id) + expect(Identity.where(provider: "github", uid: "810004")).to be_none + end + + it "redirects to the root when nothing is pending" do + post_with_session "/users/auth/account_switch/confirm" + + expect(response).to redirect_to(root_path) + expect(signed_in_user_id).to eq(session_user.id) + end + + it "preserves session and identity state when canceled", :aggregate_failures do + omniauth_sign_in(:github, payload) + expect(response.body).to include("/users/auth/account_switch/cancel") + + post_with_session "/users/auth/account_switch/cancel" + + expect(Identity.where(provider: "github", uid: "810004")).to be_none + expect(signed_in_user_id).to eq(session_user.id) + expect(session["pending_account_switch"]).to be_nil + end + + it "fails closed with zero identity mutation for a suspended resolution target", :aggregate_failures do + suspended = create(:user) + suspended.add_role(:suspended) + + expect do + omniauth_sign_in(:github, github_payload(uid: "810005", email: suspended.email, nickname: suspended.username)) + end.not_to change(Identity, :count) + + expect(response).to redirect_to(root_path) + expect(signed_in_user_id).to eq(session_user.id) + end +end diff --git a/spec/requests/mlh_oauth_callbacks_spec.rb b/spec/requests/mlh_oauth_callbacks_spec.rb new file mode 100644 index 0000000000000..59e75614a5625 --- /dev/null +++ b/spec/requests/mlh_oauth_callbacks_spec.rb @@ -0,0 +1,120 @@ +require "rails_helper" + +# MLH-specific behaviour only. Provider-neutral interstitial coverage lives in +# spec/requests/account_switch_interstitial_spec.rb. The "Core return" section +# is temporary; see Authentication::MlhCoreBridge for the removal checklist. +RSpec.describe "MLH OAuth callbacks" do + include OmniauthHelpers + include OmniauthSessionHelpers + + before do + allow(ForemStatsClient).to receive(:increment) + allow(Honeybadger).to receive(:notify) + allow(Settings::Authentication).to receive(:providers).and_return(Authentication::Providers.available) + omniauth_mock_mlh_payload + end + + after { omniauth_reset_mock } + + def mlh_payload(uid:, email:, token: "tok-#{uid}") + OmniAuth::AuthHash.new( + provider: "mlh", + uid: uid, + info: OmniAuth::AuthHash::InfoHash.new(email: email, name: "MLH User"), + credentials: OmniAuth::AuthHash.new(token: token, secret: "sec"), + extra: { raw_info: { created_at: 2.years.ago.iso8601 } }, + ) + end + + describe "account switch" do + let(:session_user) { create(:user) } + let(:target) { create(:user) } + let(:payload) { mlh_payload(uid: "910004", email: target.email) } + + before { sign_in session_user } + + it "does not carry MLH bearer credentials through the switch", :aggregate_failures do + omniauth_sign_in(:mlh, payload) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("data-account-switch-confirmation") + expect(session["pending_account_switch"].to_json).not_to include("tok-910004") + + post_with_session "/users/auth/account_switch/confirm" + + identity = Identity.find_by!(provider: "mlh", uid: "910004") + expect(identity.user_id).to eq(target.id) + expect(identity.token).to be_blank + expect(identity.secret).to be_blank + expect(signed_in_user_id).to eq(target.id) + end + end + + describe "Core return (temporary bridge)" do + around do |example| + original_enabled = ENV.fetch("FOREM_EXTERNAL_RETURN_ENABLED", nil) + original_url = ENV.fetch("FOREM_EXTERNAL_RETURN_URL", nil) + ENV["FOREM_EXTERNAL_RETURN_ENABLED"] = "true" + ENV["FOREM_EXTERNAL_RETURN_URL"] = "https://www.mlh.test/oauth/dev" + example.run + ensure + ENV["FOREM_EXTERNAL_RETURN_URL"] = original_url + ENV["FOREM_EXTERNAL_RETURN_ENABLED"] = original_enabled + end + + it "returns a directly signed-in user to Core, even before onboarding", :aggregate_failures do + incomplete = create(:user, saw_onboarding: false, checked_code_of_conduct: false, + checked_terms_and_conditions: false) + + omniauth_sign_in(:mlh, mlh_payload(uid: "910006", email: incomplete.email), + params: { continuation: "cont-token_1" }) + + expect(response).to redirect_to("https://www.mlh.test/oauth/dev?continuation=cont-token_1") + expect(signed_in_user_id).to eq(incomplete.id) + end + + it "carries the continuation through the interstitial and returns the switched user", :aggregate_failures do + sign_in create(:user) + target = create(:user) + omniauth_sign_in(:mlh, mlh_payload(uid: "910008", email: target.email), params: { continuation: "cont-token_1" }) + + expect(session["pending_account_switch"]["return_context"]).to eq("continuation" => "cont-token_1") + + post_with_session "/users/auth/account_switch/confirm" + + expect(response).to redirect_to("https://www.mlh.test/oauth/dev?continuation=cont-token_1") + expect(signed_in_user_id).to eq(target.id) + end + + it "ignores a malformed continuation" do + user = create(:user) + + omniauth_sign_in(:mlh, mlh_payload(uid: "910009", email: user.email), params: { continuation: "bad token&x=1" }) + + expect(response.location).to start_with("http://www.example.com/") + end + + it "filters the continuation out of failure telemetry" do + omniauth_setup_invalid_credentials(:mlh) + + get_with_session "/users/auth/mlh", params: { continuation: "cont-token_1" } + get_with_session "/users/auth/mlh/callback" + + expect(ForemStatsClient).to have_received(:increment).with( + "omniauth.failure", tags: array_including('params:{"continuation"=>"[FILTERED]"}') + ) + end + + it "keeps normal onboarding when the bridge is disabled despite a configured URL", :aggregate_failures do + ENV.delete("FOREM_EXTERNAL_RETURN_ENABLED") + incomplete = create(:user, saw_onboarding: false, checked_code_of_conduct: false, + checked_terms_and_conditions: false) + + omniauth_sign_in(:mlh, mlh_payload(uid: "910007", email: incomplete.email), + params: { continuation: "cont-token_1" }) + + expect(URI.parse(response.location).path).to eq("/onboarding") + expect(signed_in_user_id).to eq(incomplete.id) + end + end +end diff --git a/spec/services/authentication/external_return_spec.rb b/spec/services/authentication/external_return_spec.rb new file mode 100644 index 0000000000000..ff2478e3a664d --- /dev/null +++ b/spec/services/authentication/external_return_spec.rb @@ -0,0 +1,82 @@ +require "rails_helper" + +RSpec.describe Authentication::ExternalReturn, type: :service do + def with_return_url(value) + original = ENV.fetch("FOREM_EXTERNAL_RETURN_URL", nil) + ENV["FOREM_EXTERNAL_RETURN_URL"] = value + yield + ensure + ENV["FOREM_EXTERNAL_RETURN_URL"] = original + end + + around do |example| + original = ENV.fetch("FOREM_EXTERNAL_RETURN_ENABLED", nil) + ENV["FOREM_EXTERNAL_RETURN_ENABLED"] = "true" + example.run + ensure + ENV["FOREM_EXTERNAL_RETURN_ENABLED"] = original + end + + let(:params) { { "continuation" => "cont-token_1" } } + + describe ".capture" do + it "keeps only a well-formed continuation as opaque context", :aggregate_failures do + expect(described_class.capture(params)).to eq("continuation" => "cont-token_1") + expect(described_class.capture(params.merge("state" => "navbar"))).to eq("continuation" => "cont-token_1") + end + + it "returns nil for missing or malformed continuations", :aggregate_failures do + expect(described_class.capture("continuation" => "bad token&evil=1")).to be_nil + expect(described_class.capture("continuation" => "")).to be_nil + expect(described_class.capture({})).to be_nil + expect(described_class.capture(nil)).to be_nil + end + end + + describe ".resolve" do + it "builds the redirect from the configured endpoint, carrying the continuation verbatim" do + with_return_url("https://receiver.example/resume") do + url = described_class.resolve(described_class.capture(params)) + expect(url).to eq("https://receiver.example/resume?continuation=cont-token_1") + end + end + + it "keeps a non-default port and drops the default one", :aggregate_failures do + with_return_url("https://receiver.example:444/resume") do + expect(described_class.resolve(params)).to eq("https://receiver.example:444/resume?continuation=cont-token_1") + end + with_return_url("https://receiver.example:443/resume") do + expect(described_class.resolve(params)).to eq("https://receiver.example/resume?continuation=cont-token_1") + end + end + + it "returns nil for a missing or tampered context", :aggregate_failures do + with_return_url("https://receiver.example/resume") do + expect(described_class.resolve(nil)).to be_nil + expect(described_class.resolve({})).to be_nil + expect(described_class.resolve("continuation" => "bad token&evil=1")).to be_nil + end + end + + it "requires a configured HTTPS endpoint with a path and nothing else", :aggregate_failures do + ["http://receiver.example/resume", "https://receiver.example", "https://receiver.example/resume?extra=1", + "https://user:pass@receiver.example/resume", "https://receiver.example/resume#fragment", + "https://[", "", nil].each do |value| + with_return_url(value) do + expect(described_class.resolve(params)).to be_nil + end + end + end + end + + describe "feature gate" do + [nil, "", "false", "1", "TRUE"].each do |value| + it "resolves nothing when the flag is #{value.inspect}" do + ENV["FOREM_EXTERNAL_RETURN_ENABLED"] = value + with_return_url("https://receiver.example/resume") do + expect(described_class.redirect_url_for(params)).to be_nil + end + end + end + end +end diff --git a/spec/services/authentication/providers/github_spec.rb b/spec/services/authentication/providers/github_spec.rb index 22ced97580563..eaf34000213c6 100644 --- a/spec/services/authentication/providers/github_spec.rb +++ b/spec/services/authentication/providers/github_spec.rb @@ -13,6 +13,12 @@ end end + describe ".persist_credentials?" do + it "defaults to true" do + expect(described_class.persist_credentials?).to be(true) + end + end + describe ".sign_in_path" do let(:expected_path) do expected_callback_url = CGI.escape(URL.url("/users/auth/github/callback")) diff --git a/spec/services/authentication/providers/mlh_spec.rb b/spec/services/authentication/providers/mlh_spec.rb index d1deceeffada7..a4600d1a4900e 100644 --- a/spec/services/authentication/providers/mlh_spec.rb +++ b/spec/services/authentication/providers/mlh_spec.rb @@ -22,6 +22,12 @@ end end + describe ".persist_credentials?" do + it "is false so bearer material never outlives the callback request" do + expect(described_class.persist_credentials?).to be(false) + end + end + describe ".sign_in_path" do it "returns the correct sign in path without callback_url param" do path = described_class.sign_in_path diff --git a/spec/support/omniauth_session_helpers.rb b/spec/support/omniauth_session_helpers.rb new file mode 100644 index 0000000000000..75091868d060c --- /dev/null +++ b/spec/support/omniauth_session_helpers.rb @@ -0,0 +1,35 @@ +# Helpers for request specs that drive a full OmniAuth round trip and then +# continue with the same session (Redis-backed sessions need the cookie +# forwarded explicitly between requests). +module OmniauthSessionHelpers + def follow_session_cookie + cookie = response.headers["Set-Cookie"].to_s.split("\n").map { |c| c.split(";").first }.join("; ") + @session_cookie = cookie.presence if cookie.present? + end + + def session_headers + @session_cookie ? { "Cookie" => @session_cookie } : {} + end + + def get_with_session(path, params: {}) + get path, params: params, headers: session_headers + follow_session_cookie + end + + def post_with_session(path) + post path, headers: session_headers + follow_session_cookie + end + + # Runs the request phase and the callback phase for +provider+ with the + # given mocked payload; +params+ are sent with the initiating request. + def omniauth_sign_in(provider, payload, params: {}) + OmniAuth.config.mock_auth[provider.to_sym] = payload + get_with_session "/users/auth/#{provider}", params: params + get_with_session "/users/auth/#{provider}/callback" + end + + def signed_in_user_id + session["warden.user.user.key"].to_a.flatten.map(&:to_s).first&.to_i + end +end From ca21a8b54e2519985c5bc6bb94f9c1a72706944c Mon Sep 17 00:00:00 2001 From: Erin Osher <4386583+erinosher@users.noreply.github.com> Date: Mon, 14 Sep 2026 23:39:47 +0100 Subject: [PATCH 2/4] DEV-3974: add JWKS-backed delegated access verifier (#23830) * build: pin jwt for delegated authentication * feat: add JWKS-backed delegated-access verifier * Harden delegated access JWKS client and configuration loading - Accept the RFC 7517 JWK Set media type (application/jwk-set+json) as well as application/json, case-insensitively, and advertise both in Accept. An issuer using the registered type previously failed every verification as unavailable. - Add race_condition_ttl to the JWKS cache so that when the cached set expires under load, one caller per process refreshes it while the others keep the just-expired set for a few seconds instead of all hitting the issuer at once, and a single failed refresh no longer fails every concurrent request. - Build the configuration inside to_prepare and drop the hand-written require/require_relative lines so Zeitwerk owns loading of the DelegatedAccess constants. - Raise the same clear ArgumentError for a missing variable as for a blank one, naming the variable. - Document the key-rotation contract implied by the JWKS cache lifetime. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: jcsawyer123 Co-authored-by: Ben Halpern Co-authored-by: Claude Fable 5.1 --- .env_sample | 12 + Gemfile | 1 + Gemfile.lock | 1 + .../delegated_access/configuration.rb | 111 ++++++++ app/services/delegated_access/errors.rb | 6 + app/services/delegated_access/jwks_client.rb | 78 ++++++ app/services/delegated_access/verifier.rb | 193 ++++++++++++++ config/initializers/delegated_access.rb | 7 + .../delegated_access/configuration_spec.rb | 79 ++++++ .../delegated_access/jwks_client_spec.rb | 80 ++++++ .../delegated_access/verifier_spec.rb | 236 ++++++++++++++++++ 11 files changed, 804 insertions(+) create mode 100644 app/services/delegated_access/configuration.rb create mode 100644 app/services/delegated_access/errors.rb create mode 100644 app/services/delegated_access/jwks_client.rb create mode 100644 app/services/delegated_access/verifier.rb create mode 100644 config/initializers/delegated_access.rb create mode 100644 spec/services/delegated_access/configuration_spec.rb create mode 100644 spec/services/delegated_access/jwks_client_spec.rb create mode 100644 spec/services/delegated_access/verifier_spec.rb diff --git a/.env_sample b/.env_sample index eef1527342725..77693c5c7b2d7 100644 --- a/.env_sample +++ b/.env_sample @@ -73,6 +73,18 @@ NODE_ENV="development" ######### Optional 3rd Party Services ########## ################################################ +# Optional delegated API authentication +# DELEGATED_ACCESS_ENABLED=true +# DELEGATED_ACCESS_ISSUER="https://issuer.example.com" +# DELEGATED_ACCESS_AUDIENCE="https://forem.example.com" +# DELEGATED_ACCESS_IDENTITY_PROVIDER="external-provider" +# DELEGATED_ACCESS_OWNER_CLAIM="https://issuer.example.com/claims/dev_user_id" +# DELEGATED_ACCESS_JWKS_URI="https://issuer.example.com/.well-known/jwks.json" +# JWKS are cached this long; unknown key IDs never force a refetch, so the +# issuer must publish a new signing key at least this long before using it. +# DELEGATED_ACCESS_JWKS_MAX_AGE_SECONDS=300 +# DELEGATED_ACCESS_MAX_TOKEN_LIFETIME_SECONDS=60 + # Honeybadger for error tracking # (https://docs.honeybadger.io/lib/ruby/getting-started/introduction.html) HONEYBADGER_API_KEY="Optional" diff --git a/Gemfile b/Gemfile index e9a36324d6e83..bc34e6b981b7d 100644 --- a/Gemfile +++ b/Gemfile @@ -60,6 +60,7 @@ gem "jbuilder", "~> 2.11" # Create JSON structures via a Builder-style DSL gem "js-routes", "~> 2.2" # Brings Rails named routes to javascript gem "jsbundling-rails", "~> 1.2" # A Rails plugin to bundle JavaScript gem "jsonapi-serializer", "~> 2.2" # Serializer for Ruby objects +gem "jwt", "2.10.3" # Verify delegated access tokens gem "kaminari", "~> 1.2" # A Scope and Engine based, clean, powerful, customizable and sophisticated paginator gem "katex", "~> 0.9.0" # This rubygem enables you to render TeX math to HTML using KaTeX. It uses ExecJS under the hood gem "liquid", "~> 5.4" # A secure, non-evaling end user template engine with aesthetic markup diff --git a/Gemfile.lock b/Gemfile.lock index 38c9333d65c39..2642b58cce2b0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1187,6 +1187,7 @@ DEPENDENCIES js-routes (~> 2.2) jsbundling-rails (~> 1.2) jsonapi-serializer (~> 2.2) + jwt (= 2.10.3) kamal (~> 2.3) kaminari (~> 1.2) katex (~> 0.9.0) diff --git a/app/services/delegated_access/configuration.rb b/app/services/delegated_access/configuration.rb new file mode 100644 index 0000000000000..07eda406f29f6 --- /dev/null +++ b/app/services/delegated_access/configuration.rb @@ -0,0 +1,111 @@ +module DelegatedAccess + class Configuration + DEFAULT_JWKS_MAX_AGE_SECONDS = 300 + DEFAULT_MAX_TOKEN_LIFETIME_SECONDS = 60 + REGISTERED_CLAIMS = %w[iss sub aud exp nbf iat jti client_id scope nonce sid].freeze + private_constant :DEFAULT_JWKS_MAX_AGE_SECONDS, :DEFAULT_MAX_TOKEN_LIFETIME_SECONDS, :REGISTERED_CLAIMS + + attr_reader :audience, :enabled, :identity_provider, :issuer, :jwks_uri, :owner_claim, :verifier + + def self.from_env(env = ENV) + enabled_value = env.fetch("DELEGATED_ACCESS_ENABLED", "false") + unless %w[true false].include?(enabled_value) + raise ArgumentError, "DELEGATED_ACCESS_ENABLED must be true or false" + end + + return new(enabled: false) if enabled_value == "false" + + issuer = required(env, "DELEGATED_ACCESS_ISSUER") + audience = required(env, "DELEGATED_ACCESS_AUDIENCE") + identity_provider = required(env, "DELEGATED_ACCESS_IDENTITY_PROVIDER") + owner_claim = required(env, "DELEGATED_ACCESS_OWNER_CLAIM") + jwks_uri = required(env, "DELEGATED_ACCESS_JWKS_URI") + + validate_https_uri!(issuer, "DELEGATED_ACCESS_ISSUER") + parsed_jwks_uri = validate_https_uri!(jwks_uri, "DELEGATED_ACCESS_JWKS_URI", path_required: true) + validate_owner_claim!(owner_claim) + + jwks_max_age = positive_integer( + env, + "DELEGATED_ACCESS_JWKS_MAX_AGE_SECONDS", + DEFAULT_JWKS_MAX_AGE_SECONDS, + ) + maximum_token_lifetime = positive_integer( + env, + "DELEGATED_ACCESS_MAX_TOKEN_LIFETIME_SECONDS", + DEFAULT_MAX_TOKEN_LIFETIME_SECONDS, + ) + verifier = Verifier.new( + issuer: issuer, + audience: audience, + owner_claim: owner_claim, + jwks_client: JwksClient.new(uri: parsed_jwks_uri), + maximum_token_lifetime: maximum_token_lifetime, + jwks_cache_lifetime: jwks_max_age, + ) + + new( + enabled: true, + issuer: issuer, + audience: audience, + identity_provider: identity_provider, + owner_claim: owner_claim, + jwks_uri: jwks_uri, + verifier: verifier, + ) + end + + def initialize(enabled:, issuer: nil, audience: nil, identity_provider: nil, owner_claim: nil, + jwks_uri: nil, verifier: nil) + @enabled = enabled + @issuer = issuer + @audience = audience + @identity_provider = identity_provider + @owner_claim = owner_claim + @jwks_uri = jwks_uri + @verifier = verifier + end + + def invalidate_cache! + verifier&.invalidate_cache! + end + + class << self + private + + def required(env, name) + value = env.fetch(name, nil) + raise ArgumentError, "#{name} must be set when delegated access is enabled" if value.blank? + + value.dup.freeze + end + + def positive_integer(env, name, default) + value = Integer(env.fetch(name, default.to_s), 10) + raise ArgumentError, "#{name} must be positive" unless value.positive? + + value + rescue ArgumentError + raise ArgumentError, "#{name} must be a positive integer" + end + + def validate_https_uri!(value, name, path_required: false) + uri = URI.parse(value) + valid = uri.is_a?(URI::HTTPS) && uri.host.present? && uri.userinfo.nil? && uri.query.nil? && uri.fragment.nil? + valid &&= uri.path.present? && uri.path != "/" if path_required + raise ArgumentError, "#{name} must be an HTTPS URI" unless valid + + uri.freeze + rescue URI::InvalidURIError + raise ArgumentError, "#{name} must be an HTTPS URI" + end + + def validate_owner_claim!(owner_claim) + validate_https_uri!(owner_claim, "DELEGATED_ACCESS_OWNER_CLAIM", path_required: true) + return unless REGISTERED_CLAIMS.include?(owner_claim) + + raise ArgumentError, "DELEGATED_ACCESS_OWNER_CLAIM must be collision-resistant" + end + end + end +end diff --git a/app/services/delegated_access/errors.rb b/app/services/delegated_access/errors.rb new file mode 100644 index 0000000000000..3b8e1457dbe4c --- /dev/null +++ b/app/services/delegated_access/errors.rb @@ -0,0 +1,6 @@ +module DelegatedAccess + module Errors + class InvalidToken < StandardError; end + class Unavailable < StandardError; end + end +end diff --git a/app/services/delegated_access/jwks_client.rb b/app/services/delegated_access/jwks_client.rb new file mode 100644 index 0000000000000..043b5658f239d --- /dev/null +++ b/app/services/delegated_access/jwks_client.rb @@ -0,0 +1,78 @@ +require "net/http" + +module DelegatedAccess + class JwksClient + Error = Class.new(StandardError) + + # RFC 7517 registers application/jwk-set+json; plain JSON is common too. + JSON_MEDIA_TYPES = %w[application/json application/jwk-set+json].freeze + private_constant :JSON_MEDIA_TYPES + + class NetHttpAdapter + Response = Struct.new(:status, :headers, :body, keyword_init: true) + + OPEN_TIMEOUT_SECONDS = 2 + READ_TIMEOUT_SECONDS = 2 + MAX_RESPONSE_BYTES = 65_536 + private_constant :OPEN_TIMEOUT_SECONDS, :READ_TIMEOUT_SECONDS, :MAX_RESPONSE_BYTES + + def get(uri, headers:) + request = Net::HTTP::Get.new(uri.request_uri, headers) + response_headers = nil + response_body = +"" + status = nil + + Net::HTTP.start( + uri.host, + uri.port, + use_ssl: true, + open_timeout: OPEN_TIMEOUT_SECONDS, + read_timeout: READ_TIMEOUT_SECONDS, + ) do |http| + http.max_retries = 0 + http.request(request) do |response| + status = response.code.to_i + response_headers = response.each_header.to_h + response.read_body do |chunk| + response_body << chunk + raise Error, "JWKS response is too large" if response_body.bytesize > MAX_RESPONSE_BYTES + end + end + end + + Response.new(status: status, headers: response_headers, body: response_body.freeze) + rescue Net::OpenTimeout, Net::ReadTimeout, OpenSSL::SSL::SSLError, SocketError, SystemCallError, IOError => e + raise Error, e.class.name + end + end + + def initialize(uri:, adapter: NetHttpAdapter.new) + @uri = uri + @adapter = adapter + end + + def fetch + response = adapter.get( + uri, + headers: { + "Accept" => "application/jwk-set+json, application/json", + "User-Agent" => "Forem delegated-access JWKS verifier" + }, + ) + raise Error, "unexpected JWKS response status" unless response.status == 200 + + media_type = response.headers.fetch("content-type", "").split(";", 2).first.to_s.strip.downcase + raise Error, "JWKS response is not JSON" unless JSON_MEDIA_TYPES.include?(media_type) + + response.body + rescue Error + raise + rescue StandardError => e + raise Error, e.class.name + end + + private + + attr_reader :adapter, :uri + end +end diff --git a/app/services/delegated_access/verifier.rb b/app/services/delegated_access/verifier.rb new file mode 100644 index 0000000000000..d89e4caefcc34 --- /dev/null +++ b/app/services/delegated_access/verifier.rb @@ -0,0 +1,193 @@ +require "jwt" + +module DelegatedAccess + class Verifier + Claims = Struct.new(:subject, :owner_id, keyword_init: true) + + MAX_TOKEN_BYTES = 16_384 + MAX_KEYS = 32 + MAX_KID_BYTES = 128 + MAX_STRING_CLAIM_BYTES = 256 + CLOCK_SKEW_SECONDS = 5 + # When the cached key set expires, one caller per process refreshes it while + # the others keep using the just-expired set for this long, instead of every + # thread hitting the issuer at once. + JWKS_REFRESH_GRACE_SECONDS = 10 + REQUIRED_CLAIMS = %w[iss sub aud exp iat nbf jti].freeze + FORBIDDEN_PURPOSE_CLAIMS = %w[nonce sid].freeze + HEADER_MEMBERS = %w[alg kid typ].freeze + PRIVATE_RSA_PARAMETERS = %w[d p q dp dq qi oth].freeze + CACHE_KEY = :delegated_access_jwks + private_constant :MAX_TOKEN_BYTES, :MAX_KEYS, :MAX_KID_BYTES, :MAX_STRING_CLAIM_BYTES, + :CLOCK_SKEW_SECONDS, :JWKS_REFRESH_GRACE_SECONDS, :REQUIRED_CLAIMS, :FORBIDDEN_PURPOSE_CLAIMS, + :HEADER_MEMBERS, :PRIVATE_RSA_PARAMETERS, :CACHE_KEY + + ClaimError = Class.new(StandardError) + InvalidJwks = Class.new(StandardError) + private_constant :ClaimError, :InvalidJwks + + def initialize(issuer:, audience:, owner_claim:, jwks_client:, maximum_token_lifetime:, + jwks_cache_lifetime:, cache: ActiveSupport::Cache::MemoryStore.new) + @issuer = issuer + @audience = audience + @owner_claim = owner_claim + @jwks_client = jwks_client + @maximum_token_lifetime = maximum_token_lifetime + @jwks_cache_lifetime = jwks_cache_lifetime + @cache = cache + end + + def verify(token) + validate_token!(token) + _payload, header = JWT.decode(token, nil, false) + validate_header!(header) + + payload, = JWT.decode( + token, + nil, + true, + algorithms: ["RS256"], + jwks: method(:load_jwks), + iss: issuer, + verify_iss: true, + aud: audience, + verify_aud: true, + required_claims: REQUIRED_CLAIMS + [owner_claim], + verify_expiration: true, + verify_not_before: true, + verify_iat: true, + leeway: CLOCK_SKEW_SECONDS, + ) + claims = validate_claims(payload) + record(:accepted) + claims + rescue Errors::Unavailable + raise + rescue JWT::DecodeError, JWT::JWKError, JSON::ParserError, ArgumentError, TypeError, ClaimError + record(:rejected) + raise Errors::InvalidToken + end + + def invalidate_cache! + cache.delete(CACHE_KEY) + end + + private + + attr_reader :audience, :cache, :issuer, :jwks_cache_lifetime, :jwks_client, + :maximum_token_lifetime, :owner_claim + + def validate_token!(token) + raise ClaimError, "invalid token" unless token.is_a?(String) && token.bytesize <= MAX_TOKEN_BYTES + end + + def validate_header!(header) + raise ClaimError, "invalid protected header" unless header.is_a?(Hash) && header.keys.sort == HEADER_MEMBERS + raise ClaimError, "wrong token purpose" unless header["alg"] == "RS256" && header["typ"] == "at+jwt" + raise ClaimError, "invalid key ID" unless bounded_string(header["kid"], maximum_bytes: MAX_KID_BYTES) + end + + def load_jwks(_options) + cache_hit = true + jwks = cache.fetch(CACHE_KEY, expires_in: jwks_cache_lifetime, + race_condition_ttl: JWKS_REFRESH_GRACE_SECONDS) do + cache_hit = false + record(:refresh) + parse_jwks(jwks_client.fetch) + end + record(:cache_hit) if cache_hit + jwks + rescue JwksClient::Error, InvalidJwks + record(:unavailable_jwks) + raise Errors::Unavailable + end + + def parse_jwks(document) + parsed = JSON.parse(document) + signing_keys = validated_signing_keys(parsed) + + jwks = JWT::JWK::Set.new("keys" => signing_keys) + jwks.select! { |jwk| eligible_key?(jwk) } + raise InvalidJwks, "JWKS has no eligible signing keys" unless jwks.any? + + jwks.freeze + rescue JSON::ParserError, JWT::JWKError, OpenSSL::PKey::PKeyError, ArgumentError, TypeError => e + raise InvalidJwks, e.message + end + + def validated_signing_keys(parsed) + raw_keys = parsed["keys"] if parsed.is_a?(Hash) + raise InvalidJwks, "JWKS must contain keys" unless raw_keys.is_a?(Array) && raw_keys.any? + raise InvalidJwks, "JWKS contains too many keys" if raw_keys.size > MAX_KEYS + raise InvalidJwks, "JWKS keys must be objects" unless raw_keys.all?(Hash) + raise InvalidJwks, "JWKS contains private key material" if raw_keys.any? { |key| private_key?(key) } + + signing_keys = raw_keys.select { |key| signing_key?(key) } + raise InvalidJwks, "JWKS has no eligible signing keys" unless signing_keys.any? + + key_ids = signing_keys.pluck("kid") + raise InvalidJwks, "JWKS contains duplicate key IDs" unless key_ids.uniq.size == key_ids.size + + signing_keys + end + + def private_key?(key) + key.is_a?(Hash) && (key.keys & PRIVATE_RSA_PARAMETERS).any? + end + + def signing_key?(key) + key["kty"] == "RSA" && key["use"] == "sig" && key["alg"] == "RS256" && + bounded_string(key["kid"], maximum_bytes: MAX_KID_BYTES) + end + + def eligible_key?(jwk) + verification_key = jwk.verify_key + verification_key.is_a?(OpenSSL::PKey::RSA) && verification_key.public? && + verification_key.n.num_bits >= 2048 && verification_key.e.odd? && verification_key.e >= 3 + rescue JWT::JWKError, OpenSSL::PKey::PKeyError, ArgumentError, TypeError + false + end + + def validate_claims(payload) + raise ClaimError, "invalid claims" unless payload.is_a?(Hash) + raise ClaimError, "invalid audience" unless payload["aud"] == audience + raise ClaimError, "wrong token purpose" if FORBIDDEN_PURPOSE_CLAIMS.any? { |claim| payload.key?(claim) } + + subject = bounded_string(payload["sub"]) + raise ClaimError, "invalid subject" unless subject + raise ClaimError, "invalid JWT ID" unless bounded_string(payload["jti"]) + + validate_token_lifetime!(payload) + owner_id = parse_owner_id(payload[owner_claim]) + Claims.new(subject: subject.freeze, owner_id: owner_id).freeze + end + + def validate_token_lifetime!(payload) + issued_at, not_before, expires_at = %w[iat nbf exp].map { |claim| payload[claim] } + raise ClaimError, "invalid timestamps" unless [issued_at, not_before, expires_at].all?(Integer) + + lifetime = expires_at - issued_at + raise ClaimError, "invalid lifetime" unless lifetime.positive? && lifetime <= maximum_token_lifetime + raise ClaimError, "invalid not-before" if not_before > expires_at + end + + def bounded_string(value, maximum_bytes: MAX_STRING_CLAIM_BYTES) + value if value.is_a?(String) && value.present? && value.bytesize <= maximum_bytes + end + + def parse_owner_id(value) + raise ClaimError, "invalid owner" unless value.is_a?(String) && value.match?(/\A[1-9]\d{0,18}\z/) + + owner_id = Integer(value, 10) + raise ClaimError, "invalid owner" if owner_id > 9_223_372_036_854_775_807 + + owner_id + end + + def record(outcome) + ForemStatsClient.increment("delegated_access.verification", tags: ["outcome:#{outcome}"]) + rescue StandardError => e + Rails.logger.warn("[DelegatedAccess] telemetry_error=#{e.class.name}") + end + end +end diff --git a/config/initializers/delegated_access.rb b/config/initializers/delegated_access.rb new file mode 100644 index 0000000000000..01db01523fbe2 --- /dev/null +++ b/config/initializers/delegated_access.rb @@ -0,0 +1,7 @@ +# Built from the environment inside to_prepare so the autoloaded +# DelegatedAccess constants are resolved by Zeitwerk rather than required by +# hand. The configuration is frozen; in development it is simply rebuilt on +# code reload. +Rails.application.config.to_prepare do + Rails.application.config.x.delegated_access = DelegatedAccess::Configuration.from_env.freeze +end diff --git a/spec/services/delegated_access/configuration_spec.rb b/spec/services/delegated_access/configuration_spec.rb new file mode 100644 index 0000000000000..0371a3c2a90bd --- /dev/null +++ b/spec/services/delegated_access/configuration_spec.rb @@ -0,0 +1,79 @@ +require "rails_helper" + +RSpec.describe DelegatedAccess::Configuration do + let(:environment) do + { + "DELEGATED_ACCESS_ENABLED" => "true", + "DELEGATED_ACCESS_ISSUER" => "https://api.example.test", + "DELEGATED_ACCESS_AUDIENCE" => "https://community.example.test", + "DELEGATED_ACCESS_IDENTITY_PROVIDER" => "mlh", + "DELEGATED_ACCESS_OWNER_CLAIM" => "https://api.example.test/claims/dev_user_id", + "DELEGATED_ACCESS_JWKS_URI" => "https://api.example.test/.well-known/jwks.json" + } + end + + it "does not require trust configuration when delegated access is disabled" do + config = described_class.from_env({ "DELEGATED_ACCESS_ENABLED" => "false" }) + + expect(config.enabled).to be false + expect(config.verifier).to be_nil + end + + it "builds an enabled verifier from exact issuer, resource, identity, owner, and JWKS settings" do + config = described_class.from_env(environment) + + expect(config).to have_attributes( + enabled: true, + issuer: environment.fetch("DELEGATED_ACCESS_ISSUER"), + audience: environment.fetch("DELEGATED_ACCESS_AUDIENCE"), + identity_provider: environment.fetch("DELEGATED_ACCESS_IDENTITY_PROVIDER"), + owner_claim: environment.fetch("DELEGATED_ACCESS_OWNER_CLAIM"), + jwks_uri: environment.fetch("DELEGATED_ACCESS_JWKS_URI"), + ) + expect(config.verifier).to be_a(DelegatedAccess::Verifier) + expect(config).not_to respond_to(:key_id) + expect(config).not_to respond_to(:public_key) + end + + it "requires every trust and identity setting when enabled" do + required_names = environment.keys - ["DELEGATED_ACCESS_ENABLED"] + + required_names.each do |name| + expect { described_class.from_env(environment.except(name)) }.to raise_error(ArgumentError, /#{name}/) + expect { described_class.from_env(environment.merge(name => "")) }.to raise_error(ArgumentError, /#{name}/) + end + end + + it "requires a strict boolean enable flag" do + expect do + described_class.from_env(environment.merge("DELEGATED_ACCESS_ENABLED" => "TRUE")) + end.to raise_error(ArgumentError, /must be true or false/) + end + + it "requires configured HTTPS issuer, owner-claim, and JWKS URIs" do + %w[DELEGATED_ACCESS_ISSUER DELEGATED_ACCESS_OWNER_CLAIM DELEGATED_ACCESS_JWKS_URI].each do |name| + expect do + described_class.from_env(environment.merge(name => "http://api.example.test/value")) + end.to raise_error(ArgumentError, /HTTPS URI/) + end + end + + it "requires positive numeric safety bounds" do + %w[ + DELEGATED_ACCESS_JWKS_MAX_AGE_SECONDS + DELEGATED_ACCESS_MAX_TOKEN_LIFETIME_SECONDS + ].each do |name| + expect { described_class.from_env(environment.merge(name => "0")) }.to raise_error(ArgumentError) + expect { described_class.from_env(environment.merge(name => "invalid")) }.to raise_error(ArgumentError) + end + end + + it "exposes an immediate cache invalidation control" do + config = described_class.from_env(environment) + allow(config.verifier).to receive(:invalidate_cache!) + + config.invalidate_cache! + + expect(config.verifier).to have_received(:invalidate_cache!) + end +end diff --git a/spec/services/delegated_access/jwks_client_spec.rb b/spec/services/delegated_access/jwks_client_spec.rb new file mode 100644 index 0000000000000..fc755fe71df84 --- /dev/null +++ b/spec/services/delegated_access/jwks_client_spec.rb @@ -0,0 +1,80 @@ +require "rails_helper" + +RSpec.describe DelegatedAccess::JwksClient do + let(:uri) { URI("https://issuer.example.test/.well-known/jwks.json") } + let(:response_class) { DelegatedAccess::JwksClient::NetHttpAdapter::Response } + let(:response) do + response_class.new( + status: 200, + headers: { + "content-type" => "application/json; charset=utf-8" + }, + body: { keys: [] }.to_json, + ) + end + let(:adapter) { instance_double(DelegatedAccess::JwksClient::NetHttpAdapter) } + let(:client) { described_class.new(uri: uri, adapter: adapter) } + + before do + unless adapter.is_a?(DelegatedAccess::JwksClient::NetHttpAdapter) + allow(adapter).to receive(:get).and_return(response) + end + end + + it "fetches only the configured URI without credentials" do + result = client.fetch + + expect(adapter).to have_received(:get).with( + uri, + headers: { + "Accept" => "application/jwk-set+json, application/json", + "User-Agent" => "Forem delegated-access JWKS verifier" + }, + ) + expect(result).to eq(response.body) + end + + it "accepts the registered JWK Set media type, case-insensitively" do + ["application/jwk-set+json", "Application/JWK-Set+JSON; charset=utf-8", "APPLICATION/JSON"].each do |type| + allow(adapter).to receive(:get).and_return(response.dup.tap { |v| v.headers = { "content-type" => type } }) + expect(client.fetch).to eq(response.body) + end + end + + it "rejects redirects and non-JSON responses" do + invalid_responses = [ + response.dup.tap { |value| value.status = 302 }, + response.dup.tap { |value| value.headers = value.headers.except("content-type") }, + response.dup.tap { |value| value.headers = { "content-type" => "text/html" } }, + ] + + invalid_responses.each do |invalid_response| + allow(adapter).to receive(:get).and_return(invalid_response) + expect { client.fetch }.to raise_error(described_class::Error) + end + end + + describe DelegatedAccess::JwksClient::NetHttpAdapter do + let(:adapter) { described_class.new } + + it "reads a small HTTPS response without adding authorization or cookie headers" do + request = stub_request(:get, uri.to_s).to_return(status: 200, body: "{}") + + response = adapter.get(uri, headers: { "Accept" => "application/json" }) + + expect(response).to have_attributes(status: 200, body: "{}") + expect(request).to have_been_requested + expect( + a_request(:get, uri.to_s).with do |webmock_request| + !webmock_request.headers.key?("Authorization") && !webmock_request.headers.key?("Cookie") + end, + ).to have_been_made + end + + it "rejects a response larger than the configured bound" do + stub_request(:get, uri.to_s).to_return(status: 200, body: "x" * 65_537) + + expect { adapter.get(uri, headers: {}) }.to raise_error(DelegatedAccess::JwksClient::Error) + end + end +end diff --git a/spec/services/delegated_access/verifier_spec.rb b/spec/services/delegated_access/verifier_spec.rb new file mode 100644 index 0000000000000..f97f220737383 --- /dev/null +++ b/spec/services/delegated_access/verifier_spec.rb @@ -0,0 +1,236 @@ +require "rails_helper" + +RSpec.describe DelegatedAccess::Verifier do + let(:now) { Time.utc(2026, 9, 8, 12, 0, 0) } + let(:signing_key) { OpenSSL::PKey::RSA.generate(2048) } + let(:key_id) { "current-key" } + let(:issuer) { "https://api.example.test" } + let(:audience) { "https://community.example.test" } + let(:owner_claim) { "https://api.example.test/claims/dev_user_id" } + let(:cache) { ActiveSupport::Cache::MemoryStore.new } + let(:jwks_client) { instance_double(DelegatedAccess::JwksClient) } + let(:verifier) do + described_class.new( + issuer: issuer, + audience: audience, + owner_claim: owner_claim, + jwks_client: jwks_client, + maximum_token_lifetime: 60, + jwks_cache_lifetime: 1, + cache: cache, + ) + end + let(:claims) do + { + "iss" => issuer, + "sub" => "core-user-123", + "aud" => audience, + "iat" => now.to_i, + "nbf" => now.to_i, + "exp" => now.to_i + 30, + "jti" => "unique-token-id", + owner_claim => "123" + } + end + + def public_jwk(key, id) + JWT::JWK.new(key.public_key, id).export.transform_keys(&:to_s).merge( + "use" => "sig", + "alg" => "RS256", + ) + end + + def jwks_document(*keys) + { "keys" => keys }.to_json + end + + def token(payload = claims, key: signing_key, id: key_id, algorithm: "RS256", headers: {}) + JWT.encode(payload, key, algorithm, { kid: id, typ: "at+jwt" }.merge(headers)) + end + + before do + allow(ForemStatsClient).to receive(:increment) + allow(jwks_client).to receive(:fetch).and_return(jwks_document(public_jwk(signing_key, key_id))) + end + + around do |example| + Timecop.freeze(now) { example.run } + end + + it "returns bounded verified identity claims" do + result = verifier.verify(token) + + expect(result).to have_attributes(subject: "core-user-123", owner_id: 123) + end + + it "uses a fresh cached known key without another network request" do + 2.times { verifier.verify(token) } + + expect(jwks_client).to have_received(:fetch).once + expect(ForemStatsClient).to have_received(:increment) + .with("delegated_access.verification", tags: ["outcome:cache_hit"]) + end + + it "accepts a newly published key after the cache lifetime" do + successor_key = OpenSSL::PKey::RSA.generate(2048) + allow(jwks_client).to receive(:fetch).and_return( + jwks_document(public_jwk(signing_key, key_id)), + jwks_document(public_jwk(signing_key, key_id), public_jwk(successor_key, "successor-key")), + ) + + verifier.verify(token) + result = Timecop.travel(now + 2.seconds) do + verifier.verify(token(key: successor_key, id: "successor-key")) + end + + expect(result.owner_id).to eq(123) + expect(jwks_client).to have_received(:fetch).twice + end + + it "rejects unknown keys without refreshing the cache" do + unknown_key = OpenSSL::PKey::RSA.generate(2048) + verifier.verify(token) + + 2.times do |index| + expect do + verifier.verify(token(key: unknown_key, id: "unknown-#{index}")) + end.to raise_error(DelegatedAccess::Errors::InvalidToken) + end + + expect(jwks_client).to have_received(:fetch).once + end + + it "fails with an unavailable trust dependency on a cold-cache fetch failure" do + allow(jwks_client).to receive(:fetch).and_raise(DelegatedAccess::JwksClient::Error) + + expect { verifier.verify(token) }.to raise_error(DelegatedAccess::Errors::Unavailable) + end + + it "does not use an expired cached key when refresh fails" do + verifier.verify(token) + allow(jwks_client).to receive(:fetch).and_raise(DelegatedAccess::JwksClient::Error) + + expect do + Timecop.travel(now + 2.seconds) { verifier.verify(token) } + end.to raise_error(DelegatedAccess::Errors::Unavailable) + end + + it "does not reinterpret issuer-owned client or scope authorization context" do + issuer_context = claims.merge("client_id" => "another-client", "scope" => "articles:write") + + expect(verifier.verify(token(issuer_context)).owner_id).to eq(123) + expect(verifier.verify(token(claims)).owner_id).to eq(123) + end + + it "does not contact the trust endpoint for an unknown key while the cache is fresh" do + verifier.verify(token) + allow(jwks_client).to receive(:fetch).and_raise(DelegatedAccess::JwksClient::Error) + unknown_key = OpenSSL::PKey::RSA.generate(2048) + + expect do + verifier.verify(token(key: unknown_key, id: "unknown-key")) + end.to raise_error(DelegatedAccess::Errors::InvalidToken) + expect(jwks_client).to have_received(:fetch).once + end + + it "rejects malformed or attacker-directed protected headers before fetching keys" do + invalid_tokens = [ + "not-a-jwt", + token(id: ""), + token(headers: { typ: "JWT" }), + token(headers: { jku: "https://attacker.example/jwks" }), + token(claims, key: "shared-secret", algorithm: "HS256"), + ] + + invalid_tokens.each do |invalid_token| + expect do + verifier.verify(invalid_token) + end.to raise_error(DelegatedAccess::Errors::InvalidToken) + end + expect(jwks_client).not_to have_received(:fetch) + end + + it "rejects a signature that does not match the selected published key" do + other_key = OpenSSL::PKey::RSA.generate(2048) + + expect do + verifier.verify(token(key: other_key)) + end.to raise_error(DelegatedAccess::Errors::InvalidToken) + end + + it "requires exact trust, purpose, identity, time, and profile claims" do + invalid_claims = [ + claims.except("sub"), + claims.except("iat"), + claims.except("nbf"), + claims.except("exp"), + claims.except("jti"), + claims.except(owner_claim), + claims.merge("iss" => "https://other.example.test"), + claims.merge("aud" => [audience]), + claims.merge("sub" => ""), + claims.merge("jti" => ""), + claims.merge("iat" => now.to_f), + claims.merge("iat" => now.to_i + 10), + claims.merge("nbf" => now.to_i + 10), + claims.merge("exp" => now.to_i), + claims.merge("exp" => now.to_i + 61), + claims.merge(owner_claim => "not-an-id"), + claims.merge("sid" => "session-id"), + claims.merge("nonce" => "id-token-nonce"), + ] + + invalid_claims.each do |invalid_payload| + expect do + verifier.verify(token(invalid_payload)) + end.to raise_error(DelegatedAccess::Errors::InvalidToken) + end + end + + it "uses the jwt gem's JWK set parser and rejects unusable key sets" do + weak_key = OpenSSL::PKey::RSA.generate(1024) + weak_jwk = public_jwk(weak_key, "weak-key") + private_jwk = public_jwk(signing_key, key_id).merge("d" => "private") + duplicate_jwk = public_jwk(OpenSSL::PKey::RSA.generate(2048), key_id) + invalid_documents = [ + "not-json", + [].to_json, + {}.to_json, + { "keys" => [] }.to_json, + { "keys" => ["not-an-object"] }.to_json, + { "keys" => [public_jwk(signing_key, key_id).except("kid")] }.to_json, + { "keys" => [public_jwk(signing_key, key_id).merge("use" => "enc")] }.to_json, + { "keys" => [public_jwk(signing_key, key_id), duplicate_jwk] }.to_json, + { "keys" => [weak_jwk] }.to_json, + { "keys" => [private_jwk] }.to_json, + ] + + invalid_documents.each do |document| + allow(jwks_client).to receive(:fetch).and_return(document) + + expect { verifier.verify(token) }.to raise_error(DelegatedAccess::Errors::Unavailable) + end + end + + it "keeps serving the just-expired key set to other callers while a refresh fails" do + verifier.verify(token) + allow(jwks_client).to receive(:fetch).and_raise(DelegatedAccess::JwksClient::Error) + + Timecop.travel(now + 2.seconds) do + # The caller that triggers the refresh sees the failure... + expect { verifier.verify(token) }.to raise_error(DelegatedAccess::Errors::Unavailable) + # ...but the stale entry was extended by race_condition_ttl, so the next + # caller inside the grace window is served instead of piling on the issuer. + expect(verifier.verify(token).owner_id).to eq(123) + end + expect(jwks_client).to have_received(:fetch).twice + end + + it "can invalidate cached public keys immediately" do + verifier.verify(token) + verifier.invalidate_cache! + verifier.verify(token) + + expect(jwks_client).to have_received(:fetch).twice + end +end From 3313ab2da06c394bcc1c0979a48203e2aa809297 Mon Sep 17 00:00:00 2001 From: jcsawyer123 Date: Tue, 15 Sep 2026 00:12:49 +0100 Subject: [PATCH 3/4] DEV-3974: accept delegated bearer authentication (#23818) * feat: accept delegated bearer authentication Co-authored-by: Erin Osher <4386583+erinosher@users.noreply.github.com> * Only interpret Bearer tokens when delegated access is enabled API v1 ignored the Authorization header before this feature existed. With the feature disabled (the default on every instance), a request carrying a Bearer header alongside a valid api-key or session was being rejected with 401, which is a regression for any client that sends one. Check the feature flag before looking at the header so disabled instances behave exactly as before; the strict handling is unchanged when the feature is on. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Erin Osher <4386583+erinosher@users.noreply.github.com> Co-authored-by: Ben Halpern Co-authored-by: Claude Fable 5.1 --- app/controllers/api/v1/api_controller.rb | 61 +++++++++- spec/requests/api/v1/users_spec.rb | 136 +++++++++++++++++++++++ 2 files changed, 192 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/v1/api_controller.rb b/app/controllers/api/v1/api_controller.rb index 41a6b18c80903..ead86f4a5d3cb 100644 --- a/app/controllers/api/v1/api_controller.rb +++ b/app/controllers/api/v1/api_controller.rb @@ -19,6 +19,10 @@ class ApiController < ApplicationController rescue_from ActiveRecord::RecordNotFound, with: :error_not_found rescue_from Pundit::NotAuthorizedError, with: :error_unauthorized + rescue_from DelegatedAccess::Errors::InvalidToken, with: :error_unauthorized + # Only a trust-endpoint failure with no usable cached JWKS reaches this handler. + # Invalid tokens, including an unknown key with a fresh cache, remain unauthorized. + rescue_from DelegatedAccess::Errors::Unavailable, with: :error_delegated_access_unavailable protected @@ -34,6 +38,10 @@ def error_not_found render json: { error: "not found", status: 404 }, status: :not_found end + def error_delegated_access_unavailable + render json: { error: "delegated access unavailable", status: 503 }, status: :service_unavailable + end + # @note This method is used in ApplicationController within the # `verify_private_forem` method (read more in annotations there). # It uses `authenticate_with_api_key_or_current_user!` under the @@ -45,6 +53,46 @@ def authenticate! authenticate_with_api_key_or_current_user! end + # Bearer tokens are only interpreted when delegated access is enabled. + # Otherwise the Authorization header is ignored exactly as it was before + # this feature existed, so clients that send one alongside an api-key or + # session keep working on instances that never turn this on. + def authenticate_with_delegated_access + config = Rails.application.config.x.delegated_access + return unless config.enabled + + token = delegated_bearer_token + return unless token + + claims = config.verifier.verify(token) + + identity = Identity.includes(:user).where( + provider: config.identity_provider, + uid: claims.subject, + user_id: claims.owner_id, + ).sole + user = identity.user + if user&.spam_or_suspended? || !user&.registered? + raise DelegatedAccess::Errors::InvalidToken + end + + @user = @authenticated_user = user + rescue ActiveRecord::RecordNotFound, ActiveRecord::SoleRecordExceeded + raise DelegatedAccess::Errors::InvalidToken + end + + def delegated_bearer_token + authorization = request.authorization + return unless authorization&.match?(/\bBearer\b/i) + + scheme, token = authorization.split(" ", 2) + unless scheme.casecmp?("Bearer") && token.present? && token.exclude?(",") + raise DelegatedAccess::Errors::InvalidToken + end + + token + end + # @note This method is performing both authentication and authorization. The user suspended # should be something added to the corresponding pundit policy. def authenticate_with_api_key! @@ -82,12 +130,12 @@ def authenticate_with_api_key_or_current_user @user = authenticate_with_api_key || current_user end -def authenticated_user - return @authenticated_user if defined?(@authenticated_user) + def authenticated_user + return @authenticated_user if defined?(@authenticated_user) - user = authenticate_with_api_key || current_user - @authenticated_user = user&.spam_or_suspended? ? nil : user -end + user = authenticate_with_api_key || current_user + @authenticated_user = user&.spam_or_suspended? ? nil : user + end helper_method :authenticated_user def authorize_super_admin @@ -115,6 +163,9 @@ def pundit_user end def authenticate_with_api_key + delegated_user = authenticate_with_delegated_access + return delegated_user if delegated_user + api_key = request.headers["api-key"] return unless api_key diff --git a/spec/requests/api/v1/users_spec.rb b/spec/requests/api/v1/users_spec.rb index 4c05f73ff357e..b62edead6e0a1 100644 --- a/spec/requests/api/v1/users_spec.rb +++ b/spec/requests/api/v1/users_spec.rb @@ -109,6 +109,142 @@ end end + context "with delegated Bearer authentication" do + let(:signing_key) { OpenSSL::PKey::RSA.generate(2048) } + let(:issuer_user_id) { SecureRandom.uuid } + let(:delegated_user) { create(:user) } + let(:key_id) { "active-key" } + let(:jwks_client) { instance_double(DelegatedAccess::JwksClient) } + let(:verifier) do + DelegatedAccess::Verifier.new( + issuer: "https://issuer.example.test", + audience: "https://community.example.test", + owner_claim: "https://issuer.example.test/claims/user_id", + jwks_client: jwks_client, + maximum_token_lifetime: 60, + jwks_cache_lifetime: 60, + ) + end + let(:delegated_config) do + ActiveSupport::OrderedOptions.new.tap do |config| + config.enabled = true + config.issuer = "https://issuer.example.test" + config.audience = "https://community.example.test" + config.identity_provider = Authentication::Providers.available.first.to_s + config.owner_claim = "https://issuer.example.test/claims/user_id" + config.jwks_uri = "https://issuer.example.test/.well-known/jwks.json" + config.verifier = verifier + end.freeze + end + let(:claims) do + { + "iss" => delegated_config.issuer, + "aud" => delegated_config.audience, + "sub" => issuer_user_id, + "client_id" => "trusted-client", + "scope" => "profile:read", + "iat" => Time.current.to_i, + "nbf" => Time.current.to_i, + "exp" => 30.seconds.from_now.to_i, + "jti" => SecureRandom.uuid, + delegated_config.owner_claim => delegated_user.id.to_s + } + end + let(:token) { JWT.encode(claims, signing_key, "RS256", { kid: key_id, typ: "at+jwt" }) } + let(:jwk) do + JWT::JWK.new(signing_key.public_key, key_id).export.transform_keys(&:to_s).merge( + "use" => "sig", + "alg" => "RS256", + ) + end + + before do + Identity.create!(user: delegated_user, provider: delegated_config.identity_provider, uid: issuer_user_id) + allow(Rails.application.config.x).to receive(:delegated_access).and_return(delegated_config) + allow(jwks_client).to receive(:fetch).and_return({ "keys" => [jwk] }.to_json) + end + + it "returns the delegated user" do + get me_api_users_path, headers: headers.merge("Authorization" => "Bearer #{token}") + + expect(response).to have_http_status(:ok) + expect(response.parsed_body["id"]).to eq(delegated_user.id) + end + + it "leaves scope authorization to the trusted delegation service" do + claims["scope"] = "articles:read" + + get me_api_users_path, headers: headers.merge("Authorization" => "Bearer #{token}") + + expect(response).to have_http_status(:ok) + end + + it "rejects a token whose owner claim does not match the linked Forem user" do + claims[delegated_config.owner_claim] = create(:user).id.to_s + + get me_api_users_path, headers: headers.merge("Authorization" => "Bearer #{token}") + + expect(response).to have_http_status(:unauthorized) + end + + it "rejects a suspended mapped user" do + delegated_user.add_role(:suspended) + + get me_api_users_path, headers: headers.merge("Authorization" => "Bearer #{token}") + + expect(response).to have_http_status(:unauthorized) + end + + it "rejects an unregistered mapped user" do + delegated_user.update!(registered: false) + + get me_api_users_path, headers: headers.merge("Authorization" => "Bearer #{token}") + + expect(response).to have_http_status(:unauthorized) + end + + it "returns service unavailable when no usable key set can be retrieved" do + allow(jwks_client).to receive(:fetch).and_raise(DelegatedAccess::JwksClient::Error) + + get me_api_users_path, headers: headers.merge("Authorization" => "Bearer #{token}") + + expect(response).to have_http_status(:service_unavailable) + expect(response.parsed_body).to eq("error" => "delegated access unavailable", "status" => 503) + end + + it "does not fall back to a valid API key when Bearer appears in a malformed header" do + get me_api_users_path, headers: auth_headers.merge("Authorization" => "Basic ignored, Bearer invalid") + + expect(response).to have_http_status(:unauthorized) + end + end + + context "with a Bearer header while delegated access is disabled" do + before do + disabled = ActiveSupport::OrderedOptions.new.tap { |config| config.enabled = false }.freeze + allow(Rails.application.config.x).to receive(:delegated_access).and_return(disabled) + end + + it "ignores the Bearer header and authenticates with the api-key as before" do + get me_api_users_path, headers: auth_headers.merge("Authorization" => "Bearer some.jwt.token") + + expect(response).to have_http_status(:ok) + expect(response.parsed_body["id"]).to eq(api_secret.user.id) + end + + it "ignores a malformed Bearer header too" do + get me_api_users_path, headers: auth_headers.merge("Authorization" => "Basic ignored, Bearer invalid") + + expect(response).to have_http_status(:ok) + end + + it "does not treat a Bearer token as a credential" do + get me_api_users_path, headers: headers.merge("Authorization" => "Bearer some.jwt.token") + + expect(response).to have_http_status(:unauthorized) + end + end + context "when request is authenticated" do let(:user) { api_secret.user } From 2ffcc682dc2a6f0e06e2b4779140f37e7fdf902a Mon Sep 17 00:00:00 2001 From: Erin Osher <4386583+erinosher@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:28:58 +0100 Subject: [PATCH 4/4] DEV-3974: document delegated bearer authentication (#23831) * docs: document delegated JWKS authentication Co-authored-by: jcsawyer123 * Document Bearer auth on every api-key endpoint and the 503 outcome - Make bearer_auth part of the top-level security default alongside api-key, since ApiController accepts a delegated token on every endpoint that accepts an api-key. The eight per-operation overrides are now redundant and removed; public operations keep `security []`. - Document the 503 "delegated access unavailable" response on /api/users/me with a real generated example, and explain in the schema that it applies to every Bearer-capable endpoint. - Note in the docs that the Authorization header is ignored while the feature is disabled, and describe the short refresh grace window on the JWKS cache. - Regenerate swagger/v1/api_v1.json. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: jcsawyer123 Co-authored-by: Ben Halpern Co-authored-by: Claude Fable 5.1 --- docs/delegated_access.md | 91 +++++++++++++++++++++ spec/requests/api/v1/docs/articles_spec.rb | 4 +- spec/requests/api/v1/docs/followers_spec.rb | 4 +- spec/requests/api/v1/docs/users_spec.rb | 63 ++++++++++---- spec/swagger_helper.rb | 13 ++- swagger/v1/api_v1.json | 18 +++- 6 files changed, 171 insertions(+), 22 deletions(-) create mode 100644 docs/delegated_access.md diff --git a/docs/delegated_access.md b/docs/delegated_access.md new file mode 100644 index 0000000000000..a493ffd0e3b50 --- /dev/null +++ b/docs/delegated_access.md @@ -0,0 +1,91 @@ +# Delegated API access + +Forem can accept short-lived RFC 9068 access-token JWTs from one trusted +delegation service. This is an alternative to an API key when the trusted +service has authorized a request; it does not change account linking or +browser sign-in. + +## Configuration + +Set all of the following values before enabling delegated access: + +```text +DELEGATED_ACCESS_ENABLED=true +DELEGATED_ACCESS_ISSUER=https://api.example.com +DELEGATED_ACCESS_AUDIENCE=https://community.example.com +DELEGATED_ACCESS_IDENTITY_PROVIDER=external-provider +DELEGATED_ACCESS_OWNER_CLAIM=https://api.example.com/claims/dev_user_id +DELEGATED_ACCESS_JWKS_URI=https://api.example.com/.well-known/jwks.json +``` + +The issuer, audience, identity provider, owner claim, and JWKS URI are exact +deployment trust settings. The JWKS URI and issuer must be HTTPS and are never +selected from token claims or headers. Forem does not require a copied public +PEM, active key ID, or OAuth client ID. + +These optional safety bounds have conservative defaults: + +```text +DELEGATED_ACCESS_JWKS_MAX_AGE_SECONDS=300 +DELEGATED_ACCESS_MAX_TOKEN_LIFETIME_SECONDS=60 +``` + +`DELEGATED_ACCESS_JWKS_MAX_AGE_SECONDS` is the lifetime of each Puma worker's +in-process key cache. When an entry expires, one request per process refreshes +it while concurrent requests keep using the expired entry for a few seconds; +beyond that grace window an expired entry is never used if the fetch fails. + +While `DELEGATED_ACCESS_ENABLED` is not `true`, the `Authorization` header is +ignored by the API exactly as it was before this feature existed. + +## Token and key contract + +This contract follows [RFC 9068](https://www.rfc-editor.org/rfc/rfc9068.html), +particularly its resource-server checks for explicit access-token typing, +issuer, audience, signature, and expiration. The Core-to-Forem contract narrows +the permitted form to RS256 with `typ: at+jwt` and a non-blank `kid`, and also +requires bounded `sub`, `exp`, `iat`, `nbf`, `jti`, and the configured owner +claim. Session-purpose `sid` and `nonce` claims are rejected. Core remains +responsible for issuing the complete RFC 9068 claim set, including `client_id` +and `scope` where applicable; Forem does not use either claim for authorization. + +The `jwt` gem parses the token and JWK Set, selects the matching `kid`, verifies +the RS256 signature, and enforces the registered issuer, audience, and time +claims. Forem's surrounding code supplies the access-token profile checks, the +maximum token lifetime, and a bounded fetch from the configured JWKS URI. + +The configured JWKS endpoint must return JSON with a top-level `keys` array. It +must contain at least one unique RSA public signing key marked `use: sig` and +`alg: RS256`, with a modulus of at least 2048 bits. Other public keys are +ignored; private RSA parameters invalidate the complete response. + +Authorization remains inside the delegated-access service. Forem does not map +controller actions to scopes or reinterpret which OAuth clients may exercise a +grant. An endpoint that uses API-key authentication may accept a verified +delegated token; the trusted issuer must only mint that token when the grant +authorizes the requested operation. + +## Failures, caching, and rotation + +Malformed tokens, invalid claims or signatures, and unknown key IDs return `401 +Unauthorized`. When no usable cache entry exists and the configured trust +endpoint is unavailable or invalid, Forem returns `503 Service Unavailable`. +Neither case falls back from a presented Bearer token to API-key authentication +while delegated access is enabled. + +Fresh known keys are used without a request. An unknown key ID does not +invalidate the cache or contact the issuer, preventing attacker-selected IDs +from amplifying JWKS traffic. For rotation, publish both the old and successor +keys, wait at least one configured cache lifetime, and only then begin signing +with the successor. + +During a suspected key compromise, an operator can invalidate the in-process +key cache immediately from a Rails console: + +```ruby +Rails.application.config.x.delegated_access.invalidate_cache! +``` + +Run this command in every Forem application process, or restart the application +processes, after the compromised public key has been removed from the issuer's +JWKS. The cache contains only validated public key material and is not durable. diff --git a/spec/requests/api/v1/docs/articles_spec.rb b/spec/requests/api/v1/docs/articles_spec.rb index 6531d2999bd3c..75f5278bf5182 100644 --- a/spec/requests/api/v1/docs/articles_spec.rb +++ b/spec/requests/api/v1/docs/articles_spec.rb @@ -585,6 +585,7 @@ end end end + describe "GET /api/articles/semantic_search" do path "/api/articles/semantic_search" do get "Perform a semantic fuzzy search on articles" do @@ -605,7 +606,8 @@ schema: { type: :number } before do - allow_any_instance_of(Ai::Embedding).to receive(:call).and_return(Array.new(768, 0.1)) + embedding = instance_double(Ai::Embedding, call: Array.new(768, 0.1)) + allow(Ai::Embedding).to receive(:new).and_return(embedding) published_article.update_column(:semantic_embedding, Array.new(768, 0.1)) end diff --git a/spec/requests/api/v1/docs/followers_spec.rb b/spec/requests/api/v1/docs/followers_spec.rb index 8f750a1bd48c2..5a2c411103686 100644 --- a/spec/requests/api/v1/docs/followers_spec.rb +++ b/spec/requests/api/v1/docs/followers_spec.rb @@ -35,7 +35,9 @@ parameter "$ref": "#/components/parameters/pageParam" parameter "$ref": "#/components/parameters/perPageParam30to1000" parameter name: :sort, in: :query, required: false, - description: "Specifies the sort order for the follow relationship created_at field. Use `created_at` for chronological (oldest first) or `-created_at` for reverse chronological (newest first).", + description: "Specifies the sort order for the follow relationship created_at field. " \ + "Use `created_at` for chronological (oldest first) or `-created_at` for reverse " \ + "chronological (newest first).", schema: { type: :string }, example: "created_at" diff --git a/spec/requests/api/v1/docs/users_spec.rb b/spec/requests/api/v1/docs/users_spec.rb index fef2e48419120..c02d6d074618c 100644 --- a/spec/requests/api/v1/docs/users_spec.rb +++ b/spec/requests/api/v1/docs/users_spec.rb @@ -11,6 +11,14 @@ let(:user) { api_secret.user } let(:banned_user) { create(:user) } + let(:token) do + JWT.encode( + { "iss" => "https://issuer.example.test", "aud" => "https://community.example.test", "sub" => "core-1", + "iat" => Time.current.to_i, "nbf" => Time.current.to_i, "exp" => 30.seconds.from_now.to_i, + "jti" => SecureRandom.uuid, "https://issuer.example.test/claims/user_id" => user.id.to_s }, + OpenSSL::PKey::RSA.generate(2048), "RS256", { kid: "k1", typ: "at+jwt" } + ) + end let(:article) { create(:article, user: banned_user, published: true) } let(:comment) { create(:comment, user: banned_user, article: article) } @@ -25,7 +33,7 @@ description "This endpoint allows the client to retrieve information about the authenticated user. ### Usage Tips: -- Requires a valid `api-key` header to identify the user. +- Requires a valid `api-key` header or a configured delegated Bearer token. - Useful for checking permissions, verifying linking state, or retrieving user-specific profile settings." operationId "getUserMe" produces "application/json" @@ -43,6 +51,32 @@ add_examples run_test! end + + response "503", "Delegated access unavailable" do + description "Returned only for delegated Bearer tokens, on any Bearer-capable endpoint, when the " \ + "configured JWKS endpoint cannot be reached or returns an unusable key set and no cached " \ + "keys remain. Invalid tokens are 401, never 503." + let(:Authorization) { "Bearer #{token}" } + + before do + jwks_client = instance_double(DelegatedAccess::JwksClient) + allow(jwks_client).to receive(:fetch).and_raise(DelegatedAccess::JwksClient::Error) + verifier = DelegatedAccess::Verifier.new( + issuer: "https://issuer.example.test", audience: "https://community.example.test", + owner_claim: "https://issuer.example.test/claims/user_id", jwks_client: jwks_client, + maximum_token_lifetime: 60, jwks_cache_lifetime: 60 + ) + config = ActiveSupport::OrderedOptions.new.tap do |c| + c.enabled = true + c.identity_provider = "github" + c.verifier = verifier + end.freeze + allow(Rails.application.config.x).to receive(:delegated_access).and_return(config) + end + + add_examples + run_test! + end end end end @@ -392,7 +426,10 @@ end describe "POST /api/admin/users/{id}/merge" do - before { user.add_role(:super_admin) } + before do + user.add_role(:super_admin) + allow(Moderator::MergeUser).to receive(:call) + end path "/api/admin/users/{id}/merge" do post "Merge user into another (Admin)" do @@ -425,10 +462,6 @@ let(:another_user) { create(:user) } let(:merge_params) { { merge_user_id: another_user.id } } - before do - allow(Moderator::MergeUser).to receive(:call) - end - add_examples run_test! end @@ -520,7 +553,10 @@ end describe "POST /api/admin/users/{user_id}/identities" do - before { user.add_role(:super_admin) } + before do + user.add_role(:super_admin) + allow(Authentication::Providers).to receive(:enabled?).and_return(true) + end path "/api/admin/users/{user_id}/identities" do post "Link an identity to a user (Admin)" do @@ -548,10 +584,6 @@ required: %w[provider uid] } - before do - allow(Authentication::Providers).to receive(:enabled?).and_return(true) - end - response "201", "created" do let(:"api-key") { api_secret.secret } let(:user_id) { banned_user.id } @@ -591,7 +623,10 @@ end describe "POST /api/admin/users/identities/bulk" do - before { user.add_role(:super_admin) } + before do + user.add_role(:super_admin) + allow(Authentication::Providers).to receive(:enabled?).and_return(true) + end path "/api/admin/users/identities/bulk" do post "Bulk link identities (Admin)" do @@ -620,10 +655,6 @@ required: %w[provider identities] } - before do - allow(Authentication::Providers).to receive(:enabled?).and_return(true) - end - response "200", "successful" do let(:"api-key") { api_secret.secret } let(:bulk_params) { { provider: "github", identities: [{ user_id: banned_user.id, uid: "bulk123" }] } } diff --git a/spec/swagger_helper.rb b/spec/swagger_helper.rb index 0dddca46cbe1c..e718a4b8a6b2c 100644 --- a/spec/swagger_helper.rb +++ b/spec/swagger_helper.rb @@ -23,7 +23,10 @@ description: "Access Forem articles, users and other resources via API. For a real-world example of Forem in action, check out [DEV](https://www.dev.to). All endpoints can be accessed with the 'api-key' header and a accept header, but - some of them are accessible publicly without authentication. + some of them are accessible publicly without authentication. Instances that + enable delegated access additionally accept an `Authorization: Bearer` token + issued by their configured delegation service on every endpoint that accepts + an api-key. Dates and date times, unless otherwise specified, must be in the [RFC 3339](https://tools.ietf.org/html/rfc3339) format." @@ -35,7 +38,7 @@ description: "Production server" }, ], - security: [{ "api-key": [] }], + security: [{ "api-key": [] }, { bearer_auth: [] }], components: { securitySchemes: { "api-key": { @@ -61,6 +64,12 @@ - You'll see the newly generated key in the same view ![generated DEV API Key](https://user-images.githubusercontent.com/37842/172718151-e7fe26a0-9937-42e8-96c6-333acdab9e49.png)" + }, + bearer_auth: { + type: :http, + scheme: :bearer, + bearerFormat: "JWT", + description: "Short-lived RS256 RFC 9068 access token issued by the configured delegation service and verified against its configured JWKS. The issuer authorizes the client and requested operation before minting the token; Forem validates the token and resolves its subject and owner to a local user. An invalid token returns 401; an unavailable trust dependency with no usable cached key returns 503." } }, parameters: { diff --git a/swagger/v1/api_v1.json b/swagger/v1/api_v1.json index 007d421db145b..f7cbd92f48ed3 100644 --- a/swagger/v1/api_v1.json +++ b/swagger/v1/api_v1.json @@ -3,7 +3,7 @@ "info": { "title": "Forem API V1", "version": "1.0.0", - "description": "Access Forem articles, users and other resources via API.\n For a real-world example of Forem in action, check out [DEV](https://www.dev.to).\n All endpoints can be accessed with the 'api-key' header and a accept header, but\n some of them are accessible publicly without authentication.\n\n Dates and date times, unless otherwise specified, must be in\n the [RFC 3339](https://tools.ietf.org/html/rfc3339) format." + "description": "Access Forem articles, users and other resources via API.\n For a real-world example of Forem in action, check out [DEV](https://www.dev.to).\n All endpoints can be accessed with the 'api-key' header and a accept header, but\n some of them are accessible publicly without authentication. Instances that\n enable delegated access additionally accept an `Authorization: Bearer` token\n issued by their configured delegation service on every endpoint that accepts\n an api-key.\n\n Dates and date times, unless otherwise specified, must be in\n the [RFC 3339](https://tools.ietf.org/html/rfc3339) format." }, "paths": { "/api/agent_sessions": { @@ -5244,7 +5244,7 @@ "tags": [ "users" ], - "description": "This endpoint allows the client to retrieve information about the authenticated user.\n\n### Usage Tips:\n- Requires a valid `api-key` header to identify the user.\n- Useful for checking permissions, verifying linking state, or retrieving user-specific profile settings.", + "description": "Returned only for delegated Bearer tokens, on any Bearer-capable endpoint, when the configured JWKS endpoint cannot be reached or returns an unusable key set and no cached keys remain. Invalid tokens are 401, never 503.", "operationId": "getUserMe", "responses": { "200": { @@ -5262,6 +5262,9 @@ }, "401": { "description": "Unauthorized" + }, + "503": { + "description": "Delegated access unavailable" } } } @@ -6010,6 +6013,11 @@ { "api-key": [ + ] + }, + { + "bearer_auth": [ + ] } ], @@ -6020,6 +6028,12 @@ "name": "api-key", "in": "header", "description": "API Key authentication.\n\nAuthentication for some endpoints, like write operations on the\nArticles API require a DEV API key.\n\nAll authenticated endpoints are CORS disabled, the API key is intended for non-browser scripts.\n\n### Getting an API key\n\nTo obtain one, please follow these steps:\n\n - visit https://dev.to/settings/extensions\n - in the \"DEV API Keys\" section create a new key by adding a\n description and clicking on \"Generate API Key\"\n\n ![obtain a DEV API Key](https://user-images.githubusercontent.com/37842/172718105-bd93664e-76e0-477d-99c4-265dda0b06c5.png)\n\n - You'll see the newly generated key in the same view\n ![generated DEV API Key](https://user-images.githubusercontent.com/37842/172718151-e7fe26a0-9937-42e8-96c6-333acdab9e49.png)" + }, + "bearer_auth": { + "type": "http", + "scheme": "bearer", + "bearerFormat": "JWT", + "description": "Short-lived RS256 RFC 9068 access token issued by the configured delegation service and verified against its configured JWKS. The issuer authorizes the client and requested operation before minting the token; Forem validates the token and resolves its subject and owner to a local user. An invalid token returns 401; an unavailable trust dependency with no usable cached key returns 503." } }, "parameters": {