Merge pull request #11693 from neinteractiveliterature/login-troubleshooting-part-2

Fix white-screen-on-login (cross-origin OIDC discovery), plus third-party OAuth sign-in and refresh diagnostics

Author: nbudin

.rubocop_todo.yml

@@ -50,11 +50,12 @@ Metrics/BlockLength:
     - 'config/environments/production.rb'
     - 'config/initializers/doorkeeper.rb'
 
-# Offense count: 4
+# Offense count: 5
 # Configuration parameters: CountComments, Max, CountAsOne.
 Metrics/ClassLength:
   Exclude:
     - 'app/controllers/application_controller.rb'
+    - 'app/controllers/oauth_sessions_controller.rb'
     - 'app/graphql/types/ability_type.rb'
     - 'app/graphql/types/mutation_type.rb'
     - 'app/graphql/types/query_type.rb'

app/controllers/client_configuration_controller.rb

@@ -14,6 +14,14 @@ def show
     render json: {
              oauth_frontend_application_uid: Doorkeeper::Application.find_by(is_intercode_frontend: true)&.uid,
              oidc_issuer_url: oidc_issuer_url,
+             # The SPA used to fetch these via the OIDC discovery document, but that's a
+             # cross-origin request to the issuer host (a convention page reaching the root
+             # site), which gets blocked (Brave shields, untrusted dev certs, etc.) and
+             # leaves login hanging. Serving them here — same-origin — removes that
+             # dependency. Built from the issuer URL so they point at the issuer host
+             # rather than whatever convention host is making this request.
+             oidc_authorization_endpoint: oidc_authorization_endpoint,
+             oidc_end_session_endpoint: oidc_end_session_endpoint,
              rails_default_active_storage_service_name: Rails.application.config.active_storage.service.to_s,
              rails_direct_uploads_url: rails_direct_uploads_url,
              recaptcha_site_key: Recaptcha.configuration.site_key,
@@ -22,4 +30,14 @@ def show
              rollbar_client_access_token: ENV["ROLLBAR_CLIENT_ACCESS_TOKEN"].presence
            }
   end
+
+  private
+
+  def oidc_authorization_endpoint
+    URI.join(oidc_issuer_url, oauth_authorization_path).to_s
+  end
+
+  def oidc_end_session_endpoint
+    URI.join(oidc_issuer_url, destroy_user_session_path).to_s
+  end
 end

app/controllers/oauth_sessions_controller.rb

@@ -46,10 +46,20 @@ def exchange
 
   def refresh
     refresh_token_value = cookies[COOKIE_NAME]
-    return render_oauth_error(:invalid_grant, status: :unauthorized) if refresh_token_value.blank?
+    if refresh_token_value.blank?
+      report_refresh_failure(:cookie_absent)
+      return render_oauth_error(:invalid_grant, status: :unauthorized)
+    end
 
     access_token = Doorkeeper::AccessToken.by_refresh_token(refresh_token_value)
-    return render_oauth_error(:invalid_grant, status: :unauthorized) if access_token.nil?
+    if access_token.nil?
+      # The cookie carried a refresh token but no access token row matches it.
+      # The most likely cause is that the row was deleted out from under the
+      # cookie (e.g. the nightly CleanupDbService pruning a revoked token) — this
+      # is the case we most want to catch for the "logged out overnight" reports.
+      report_refresh_failure(:token_not_found)
+      return render_oauth_error(:invalid_grant, status: :unauthorized)
+    end
 
     frontend_app = OAuthApplication.find_by(is_intercode_frontend: true)
     return render_oauth_error(:invalid_client, status: :bad_request) unless frontend_app
@@ -59,7 +69,7 @@ def refresh
     credentials = Doorkeeper::OAuth::Client::Credentials.new(frontend_app.uid, nil)
     request = Doorkeeper::OAuth::RefreshTokenRequest.new(Doorkeeper.config, access_token, credentials)
 
-    respond_with_token_request(request)
+    respond_with_token_request(request, refreshed_token: access_token)
   end
 
   def sign_out
@@ -74,9 +84,39 @@ def sign_out
 
   private
 
-  def respond_with_token_request(request)
+  # Records *why* a cookie-backed refresh failed, so we can diagnose the
+  # convention-site "logged out overnight" reports from real data instead of
+  # guesswork. Deliberately logs no token material — just the failure reason
+  # and (when a row exists) its lifecycle timestamps, which let us distinguish
+  # a deleted row from one that was revoked by rotation. Surfaces to Sentry and
+  # Rollbar with a filterable `oauth_refresh_failure` tag.
+  def report_refresh_failure(reason, access_token: nil, doorkeeper_error: nil)
+    context = { reason: reason, doorkeeper_error: doorkeeper_error }.compact
+
+    if access_token
+      context.merge!(
+        resource_owner_id: access_token.resource_owner_id,
+        application_id: access_token.application_id,
+        token_created_at: access_token.created_at,
+        token_revoked_at: access_token.revoked_at,
+        token_expires_in: access_token.expires_in,
+        has_previous_refresh_token: access_token.previous_refresh_token.present?
+      )
+    end
+
+    ErrorReporting.info("oauth_session refresh failed", tags: { oauth_refresh_failure: reason.to_s }, **context)
+  end
+
+  def respond_with_token_request(request, refreshed_token: nil)
     response = request.authorize
     if response.is_a?(Doorkeeper::OAuth::ErrorResponse)
+      # `refreshed_token` is only passed by the refresh flow: the access token row
+      # exists but Doorkeeper rejected the grant (e.g. it was already revoked, or
+      # this is refresh-token reuse). Capturing the row's state lets us tell a
+      # rotation/race problem apart from the row being deleted entirely.
+      if refreshed_token
+        report_refresh_failure(:grant_rejected, access_token: refreshed_token, doorkeeper_error: response.name)
+      end
       clear_refresh_cookie
       render json: response.body, status: response.status
       return

app/controllers/sessions_controller.rb

@@ -13,8 +13,22 @@ def create
     set_flash_message!(:notice, :signed_in)
     sign_in(resource_name, resource)
     path = after_sign_in_path_for(resource)
-    uri = parse_uri_silently(path.to_s)
-    redirect_to path, allow_other_host: uri&.host.present? && trusted_origin?(uri.host)
+
+    respond_to do |format|
+      # The SPA sign-in form posts with `Accept: application/json` and performs the
+      # post-sign-in redirect itself, as a top-level navigation. We must NOT issue an
+      # HTTP redirect for it: fetch() follows the redirect chain as subresource
+      # requests, so when that chain crosses into a third-party OAuth app's callback
+      # (i.e. signing in to a relying app) the cross-origin hop is CORS-blocked — and
+      # the one-time authorization code is burned in the process. Handing the location
+      # back as JSON lets the browser walk the redirect chain natively via a top-level
+      # navigation, which isn't subject to CORS.
+      format.json { render json: { location: safe_sign_in_location(path) } }
+      format.any do
+        uri = parse_uri_silently(path.to_s)
+        redirect_to path, allow_other_host: uri&.host.present? && trusted_origin?(uri.host)
+      end
+    end
   end
 
   # Revoke the user's intercode-frontend access tokens before Devise tears down
@@ -41,6 +55,16 @@ def respond_to_on_destroy(non_navigational_status: :no_content)
 
   private
 
+  # Only hand the client a location it's safe to send the browser to: same-origin
+  # (no host) or a trusted origin. Mirrors the `allow_other_host` guard on the
+  # navigational redirect so the JSON path can't be coerced into an open redirect.
+  def safe_sign_in_location(path)
+    uri = parse_uri_silently(path.to_s)
+    return path.to_s if uri&.host.blank? || trusted_origin?(uri.host)
+
+    root_path
+  end
+
   def after_sign_out_path_for(_resource_or_scope)
     trusted_referer_url || root_path
   end

app/javascript/AppContexts.ts

@@ -10,6 +10,8 @@ import AuthenticityTokensManager from 'AuthenticityTokensContext';
 export type ClientConfiguration = {
   oauth_frontend_application_uid: string | null;
   oidc_issuer_url: string | null;
+  oidc_authorization_endpoint: string | null;
+  oidc_end_session_endpoint: string | null;
   rails_default_active_storage_service_name: string;
   rails_direct_uploads_url: string;
   recaptcha_site_key: string | null;

app/javascript/AppRoot.tsx

@@ -1,5 +1,5 @@
 import { Suspense, useMemo, useState, useEffect, useRef } from 'react';
-import { useLocation, useLoaderData, useRouteLoaderData, Outlet, useNavigation } from 'react-router';
+import { useLocation, useLoaderData, Outlet, useNavigation } from 'react-router';
 import { Settings } from 'luxon';
 import { PageLoadingIndicator } from '@neinteractiveliterature/litform';
 
@@ -11,8 +11,7 @@ import { timespanFromConvention } from './TimespanUtils';
 import { LazyStripeContext } from './LazyStripe';
 import { Stripe } from '@stripe/stripe-js';
 import { reloadOnAppEntrypointHeadersMismatch } from './checkAppEntrypointHeadersMatch';
-import { initErrorReporting } from 'ErrorReporting';
-import { RootLoaderData } from 'root';
+import errorReporting from 'ErrorReporting';
 
 export function buildAppRootContextValue(
   data: AppRootQueryData | null | undefined,
@@ -82,15 +81,12 @@ function AppRoot(): React.JSX.Element {
   const navigationBarRef = useRef<HTMLElement>(null);
 
   const [stripePromise, setStripePromise] = useState<Promise<Stripe | null> | null>(null);
-  const rootLoaderData = useRouteLoaderData('root') as RootLoaderData | undefined;
 
   useEffect(() => {
-    initErrorReporting(
-      data.currentUser?.id,
-      rootLoaderData?.clientConfiguration?.sentry_frontend_dsn,
-      rootLoaderData?.clientConfiguration?.rollbar_client_access_token,
-    );
-  }, [data?.currentUser?.id, rootLoaderData?.clientConfiguration]);
+    // Error reporting is initialized in the boot sequence (see packs/application.tsx);
+    // here we just associate subsequent reports with the signed-in user.
+    errorReporting().setCurrentUser(data.currentUser?.id);
+  }, [data?.currentUser?.id]);
 
   useEffect(() => {
     reloadOnAppEntrypointHeadersMismatch();

app/javascript/AppRouter.tsx

@@ -117,7 +117,7 @@ export function AppRootContextRouteGuard({ guard }: AppRootContextRouteGuardProp
 function LoginRequiredRouteGuard() {
   const loginRequired = useLoginRequired();
   if (loginRequired) {
-    return <></>;
+    return loginRequired;
   }
 
   return <Outlet />;

app/javascript/Authentication/DeviseSignInPage.tsx

@@ -26,10 +26,16 @@ function DeviseSignInPage() {
     try {
       const response = await fetch('/users/sign_in', {
         method: 'POST',
+        headers: { Accept: 'application/json' },
         body: new FormData(event.currentTarget),
       });
       if (response.ok) {
-        window.location.href = response.url || '/';
+        // The server returns the post-sign-in location as JSON rather than
+        // redirecting, so the browser navigates the redirect chain top-level.
+        // If we let fetch() follow it, a cross-origin hop into a relying OAuth
+        // app's callback would be CORS-blocked (and burn the one-time code).
+        const data = (await response.json()) as { location?: string };
+        window.location.href = data.location ?? '/';
       } else {
         const data = (await response.json()) as { error?: string };
         setError(data.error ?? t('authentication.signInForm.genericError'));

app/javascript/Authentication/authenticationManager.ts

@@ -1,5 +1,5 @@
 import { Configuration } from 'openid-client';
-import { discoverOpenidConfig, generatePKCEChallenge, getAuthorizationRedirectURL } from './openid';
+import { buildOpenidConfig, generatePKCEChallenge, getAuthorizationRedirectURL } from './openid';
 import { createContext } from 'react';
 import * as z from 'zod/mini';
 
@@ -53,6 +53,8 @@ type TokenResponseBody = {
 export class AuthenticationManager {
   clientId?: string;
   issuerUrl?: string;
+  authorizationEndpoint?: string;
+  endSessionEndpoint?: string;
   openidConfig?: Configuration;
   currentLoginFlowData?: LoginFlowData;
   jwtToken?: string;
@@ -99,7 +101,15 @@ export class AuthenticationManager {
       throw new Error('OAuth client ID not configured');
     }
 
-    this.openidConfig = await discoverOpenidConfig(this.clientId, this.issuerUrl);
+    if (!this.issuerUrl) {
+      throw new Error('OIDC issuer URL not configured');
+    }
+
+    this.openidConfig = buildOpenidConfig(this.clientId, {
+      issuer: this.issuerUrl,
+      authorizationEndpoint: this.authorizationEndpoint,
+      endSessionEndpoint: this.endSessionEndpoint,
+    });
     return this.openidConfig;
   }
 

app/javascript/Authentication/openid.ts

@@ -1,5 +1,4 @@
 import {
-  discovery,
   buildAuthorizationUrl,
   Configuration,
   randomPKCECodeVerifier,
@@ -12,13 +11,28 @@ export type PKCEChallengeData = {
   state: string;
 };
 
-export async function discoverOpenidConfig(clientId: string, issuerUrl?: string): Promise<Configuration> {
-  const issuer = new URL(issuerUrl ?? window.location.href);
-  issuer.pathname = '/';
-  issuer.search = '';
-  issuer.hash = '';
-  const config = await discovery(issuer, clientId);
-  return config;
+export type OpenidServerMetadata = {
+  issuer: string;
+  authorizationEndpoint?: string;
+  endSessionEndpoint?: string;
+};
+
+// Build the openid-client Configuration from metadata we already have (delivered
+// same-origin via /client_configuration) rather than fetching the OIDC discovery
+// document. Discovery is a cross-origin request to the issuer host, which gets
+// blocked (Brave shields, untrusted dev certs) and silently wedges login. We only
+// need the authorization endpoint (to build the redirect) and the end-session
+// endpoint (for sign-out); token exchange/refresh go through our own same-origin
+// /oauth_session/* endpoints.
+export function buildOpenidConfig(clientId: string, metadata: OpenidServerMetadata): Configuration {
+  return new Configuration(
+    {
+      issuer: metadata.issuer,
+      authorization_endpoint: metadata.authorizationEndpoint,
+      end_session_endpoint: metadata.endSessionEndpoint,
+    },
+    clientId,
+  );
 }
 
 export async function generatePKCEChallenge(): Promise<PKCEChallengeData> {