Merge pull request #11389 from growilabs/refactor/login-success-handler-respond-with-redirect

refactor(auth): loginSuccessHandler の isExternalAccount を respondWithRedirect にリネーム＋回帰テスト追加

Author: yuki-takei

apps/app/src/server/routes/login-passport.js

@@ -7,6 +7,7 @@ import { createRedirectToForUnauthenticated } from '~/server/util/createRedirect
 import loggerFactory from '~/utils/logger';
 
 import { externalAccountService } from '../service/external-account';
+import { sendLoginSuccessResponse } from './login-success-response';
 
 /** @param {import('~/server/crowi').default} crowi Crowi instance */
 module.exports = (crowi, app) => {
@@ -59,7 +60,7 @@ module.exports = (crowi, app) => {
     res,
     user,
     action,
-    isExternalAccount = false,
+    respondWithRedirect = false,
   ) => {
     // update lastLoginAt
     user.updateLastLoginAt(new Date(), (err, userData) => {
@@ -87,11 +88,7 @@ module.exports = (crowi, app) => {
     const redirectTo =
       redirectToForUnauthenticated ?? res.locals.redirectTo ?? '/';
 
-    if (isExternalAccount) {
-      return res.safeRedirect(redirectTo);
-    }
-
-    return res.apiv3({ redirectTo });
+    return sendLoginSuccessResponse(res, redirectTo, respondWithRedirect);
   };
 
   const injectRedirectTo = (req, res, next) => {
@@ -252,6 +249,10 @@ module.exports = (crowi, app) => {
         return next(err);
       }
 
+      // LDAP login is submitted through the AJAX login form (POST /_api/v3/login), the
+      // same as local login, so it must respond with JSON. Do NOT pass respondWithRedirect
+      // here even though LDAP is an external account: a 302 would leave the client stuck on
+      // the login page until a manual reload. See issue #11384.
       return loginSuccessHandler(
         req,
         res,

apps/app/src/server/routes/login-success-response.spec.ts

@@ -0,0 +1,52 @@
+import { mock } from 'vitest-mock-extended';
+
+import type { ResWithSafeRedirect } from '~/server/middlewares/safe-redirect';
+
+import type { ApiV3Response } from './apiv3/interfaces/apiv3-response';
+import { sendLoginSuccessResponse } from './login-success-response';
+
+type LoginSuccessResponse = ApiV3Response & ResWithSafeRedirect;
+
+describe('sendLoginSuccessResponse', () => {
+  const redirectTo = '/path/to/redirect';
+
+  // Contract: the response TRANSPORT the client experiences (JSON body vs HTTP 302),
+  // not which internal helper is called. See issue #11384.
+
+  describe('when respondWithRedirect is omitted (AJAX login form: local & LDAP)', () => {
+    it('responds with JSON { redirectTo } and never issues a redirect', () => {
+      // Arrange
+      const res = mock<LoginSuccessResponse>();
+
+      // Act
+      sendLoginSuccessResponse(res, redirectTo);
+
+      // Assert: the client reads redirectTo from the body and navigates itself.
+      // A 302 here would be followed silently by the XHR (the #11384 regression).
+      expect(res.apiv3).toHaveBeenCalledWith({ redirectTo });
+      expect(res.safeRedirect).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('when respondWithRedirect is false', () => {
+    it('responds with JSON { redirectTo } and never issues a redirect', () => {
+      const res = mock<LoginSuccessResponse>();
+
+      sendLoginSuccessResponse(res, redirectTo, false);
+
+      expect(res.apiv3).toHaveBeenCalledWith({ redirectTo });
+      expect(res.safeRedirect).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('when respondWithRedirect is true (full-page callback: OAuth/SAML)', () => {
+    it('responds with an HTTP 302 and never returns a JSON body', () => {
+      const res = mock<LoginSuccessResponse>();
+
+      sendLoginSuccessResponse(res, redirectTo, true);
+
+      expect(res.safeRedirect).toHaveBeenCalledWith(redirectTo);
+      expect(res.apiv3).not.toHaveBeenCalled();
+    });
+  });
+});

apps/app/src/server/routes/login-success-response.ts

@@ -0,0 +1,35 @@
+import type { ResWithSafeRedirect } from '~/server/middlewares/safe-redirect';
+
+import type { ApiV3Response } from './apiv3/interfaces/apiv3-response';
+
+type LoginSuccessResponse = ApiV3Response & ResWithSafeRedirect;
+
+/**
+ * Send the response for a successful login.
+ *
+ * `respondWithRedirect` selects the response TRANSPORT, not whether the account is
+ * external:
+ * - `false` (default): reply with JSON `{ redirectTo }` via `res.apiv3`. Used by the
+ *   AJAX login form (`POST /_api/v3/login`) for BOTH local and LDAP login — the client
+ *   reads `redirectTo` from the body and navigates on its own (`router.push`).
+ * - `true`: reply with an HTTP 302 via `res.safeRedirect`. Used by the OAuth/SAML callback
+ *   routes (`GET`/`POST` under `/passport/`), which are full-page browser navigations that
+ *   the server itself must redirect.
+ *
+ * NOTE: LDAP is an external account but still uses `false`, because it is submitted
+ * through the AJAX form rather than a full-page callback. Passing `true` for LDAP returns
+ * a 302 that the XHR follows silently, so the client never receives `redirectTo` and the
+ * user stays on the login page until a manual reload. See issue #11384.
+ */
+export const sendLoginSuccessResponse = (
+  res: LoginSuccessResponse,
+  redirectTo: string,
+  respondWithRedirect = false,
+): void => {
+  if (respondWithRedirect) {
+    res.safeRedirect(redirectTo);
+    return;
+  }
+
+  res.apiv3({ redirectTo });
+};