Merge pull request #7402 from weseek/fix/115621-unable-to-transition-requested-page-after-login

fix: Unable to transition requested page after login

packages/app/src/components/LoginForm.tsx

@@ -2,7 +2,6 @@ import React, {
   useState, useEffect, useCallback,
 } from 'react';
 
-import { USER_STATUS } from '@growi/core';
 import { useTranslation } from 'next-i18next';
 import { useRouter } from 'next/router';
 import ReactCardFlip from 'react-card-flip';
@@ -95,16 +94,12 @@ export const LoginForm = (props: LoginFormProps): JSX.Element => {
 
     try {
       const res = await apiv3Post('/login', { loginForm });
-      const { redirectTo, userStatus } = res.data;
+      const { redirectTo } = res.data;
 
       if (redirectTo != null) {
         return router.push(redirectTo);
       }
 
-      if (userStatus !== USER_STATUS.ACTIVE) {
-        window.location.href = '/';
-      }
-
       return router.push('/');
     }
     catch (err) {

packages/app/src/interfaces/user.ts

@@ -1,3 +1,5 @@
 export type {
-  IUser, IUserGroupRelation, IUserGroup, IUserHasId, IUserGroupHasId, IUserGroupRelationHasId,
+  IUser, IUserGroupRelation, IUserGroup, IUserHasId, IUserGroupHasId, IUserGroupRelationHasId, IUserStatus,
 } from '@growi/core';
+
+export { USER_STATUS } from '@growi/core';

packages/app/src/server/middlewares/login-required.js

@@ -1,3 +1,4 @@
+import { createRedirectToForUnauthenticated } from '~/server/util/createRedirectToForUnauthenticated';
 import loggerFactory from '~/utils/logger';
 
 const logger = loggerFactory('growi:middleware:login-required');
@@ -20,15 +21,9 @@ module.exports = (crowi, isGuestAllowed = false, fallback = null) => {
         // Active の人だけ先に進める
         return next();
       }
-      if (req.user.status === User.STATUS_REGISTERED) {
-        return res.redirect('/login/error/registered');
-      }
-      if (req.user.status === User.STATUS_SUSPENDED) {
-        return res.redirect('/login/error/suspended');
-      }
-      if (req.user.status === User.STATUS_INVITED) {
-        return res.redirect('/invited');
-      }
+
+      const redirectTo = createRedirectToForUnauthenticated(req.user.status) ?? '/login';
+      return res.redirect(redirectTo);
     }
 
     // check the route config and ACL

packages/app/src/server/routes/apiv3/index.js

@@ -51,7 +51,7 @@ module.exports = (crowi, app) => {
   const loginPassport = require('../login-passport')(crowi, app);
 
   routerForAuth.post('/login', applicationInstalled, loginFormValidator.loginRules(), loginFormValidator.loginValidation,
-    addActivity, loginPassport.isEnableLoginWithLocalOrLdap, loginPassport.loginWithLocal, loginPassport.loginWithLdap,
+    addActivity, loginPassport.injectRedirectTo, loginPassport.isEnableLoginWithLocalOrLdap, loginPassport.loginWithLocal, loginPassport.loginWithLdap,
     loginPassport.cannotLoginErrorHadnler, loginPassport.loginFailure);
 
   routerForAuth.use('/invited', require('./invited')(crowi));

packages/app/src/server/routes/index.js

@@ -91,12 +91,12 @@ module.exports = function(crowi, app) {
   // OAuth
   app.get('/passport/google'                      , loginPassport.loginWithGoogle, loginPassport.loginFailureForExternalAccount);
   app.get('/passport/github'                      , loginPassport.loginWithGitHub, loginPassport.loginFailureForExternalAccount);
-  app.get('/passport/oidc'                        , loginPassport.loginWithOidc, loginPassport.loginFailureForExternalAccount);
-  app.get('/passport/saml'                        , loginPassport.loginWithSaml, loginPassport.loginFailureForExternalAccount);
-  app.get('/passport/google/callback'             , loginPassport.loginPassportGoogleCallback   , loginPassport.loginFailureForExternalAccount);
-  app.get('/passport/github/callback'             , loginPassport.loginPassportGitHubCallback   , loginPassport.loginFailureForExternalAccount);
-  app.get('/passport/oidc/callback'               , loginPassport.loginPassportOidcCallback     , loginPassport.loginFailureForExternalAccount);
-  app.post('/passport/saml/callback'              , addActivity, loginPassport.loginPassportSamlCallback, loginPassport.loginFailureForExternalAccount);
+  app.get('/passport/oidc'                        , loginPassport.loginWithOidc,   loginPassport.loginFailureForExternalAccount);
+  app.get('/passport/saml'                        , loginPassport.loginWithSaml,   loginPassport.loginFailureForExternalAccount);
+  app.get('/passport/google/callback'             , loginPassport.injectRedirectTo, loginPassport.loginPassportGoogleCallback   , loginPassport.loginFailureForExternalAccount);
+  app.get('/passport/github/callback'             , loginPassport.injectRedirectTo, loginPassport.loginPassportGitHubCallback   , loginPassport.loginFailureForExternalAccount);
+  app.get('/passport/oidc/callback'               , loginPassport.injectRedirectTo, loginPassport.loginPassportOidcCallback     , loginPassport.loginFailureForExternalAccount);
+  app.post('/passport/saml/callback'              , addActivity, loginPassport.injectRedirectTo, loginPassport.loginPassportSamlCallback, loginPassport.loginFailureForExternalAccount);
 
   app.post('/_api/login/testLdap'    , loginRequiredStrictly , loginFormValidator.loginRules() , loginFormValidator.loginValidation , loginPassport.testLdapCredentials);
 

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

@@ -6,6 +6,7 @@ import { SupportedAction } from '~/interfaces/activity';
 import { LoginErrorCode } from '~/interfaces/errors/login-error';
 import { ExternalAccountLoginError } from '~/models/vo/external-account-login-error';
 import { NullUsernameToBeRegisteredError } from '~/server/models/errors';
+import { createRedirectToForUnauthenticated } from '~/server/util/createRedirectToForUnauthenticated';
 import loggerFactory from '~/utils/logger';
 
 
@@ -16,7 +17,6 @@ module.exports = function(crowi, app) {
   const logger = loggerFactory('growi:routes:login-passport');
   const passport = require('passport');
   const ExternalAccount = crowi.model('ExternalAccount');
-  const User = crowi.model('User');
   const passportService = crowi.passportService;
 
   const activityEvent = crowi.event('activity');
@@ -120,17 +120,26 @@ module.exports = function(crowi, app) {
 
     await crowi.activityService.createActivity(parameters);
 
+    const redirectToForUnauthenticated = createRedirectToForUnauthenticated(req.user.status);
+    const redirectTo = redirectToForUnauthenticated ?? res.locals.redirectTo ?? '/';
+
     if (isExternalAccount) {
-      return res.redirect('/');
+      return res.redirect(redirectTo);
     }
 
-    // check for redirection to '/invited'
-    const redirectTo = req.user.status === User.STATUS_INVITED ? '/invited' : req.session.redirectTo;
+    return res.apiv3({ redirectTo });
+  };
+
+  const injectRedirectTo = (req, res, next) => {
 
-    // remove session.redirectTo
-    delete req.session.redirectTo;
+    // Move "req.session.redirectTo" to "res.locals.redirectTo"
+    // Because the session is regenerated when req.login() is called
+    const redirectTo = req.session.redirectTo;
+    if (redirectTo != null) {
+      res.locals.redirectTo = redirectTo;
+    }
 
-    return res.apiv3({ redirectTo, userStatus: req.user.status });
+    next();
   };
 
   const isEnableLoginWithLocalOrLdap = (req, res, next) => {
@@ -595,6 +604,7 @@ module.exports = function(crowi, app) {
 
   return {
     cannotLoginErrorHadnler,
+    injectRedirectTo,
     isEnableLoginWithLocalOrLdap,
     loginFailure,
     loginFailureForExternalAccount,

packages/app/src/server/util/createRedirectToForUnauthenticated.ts

@@ -0,0 +1,17 @@
+import { USER_STATUS } from '~/interfaces/user';
+import type { IUserStatus } from '~/interfaces/user';
+
+export const createRedirectToForUnauthenticated = (userStatus: IUserStatus): string | null => {
+  switch (userStatus) {
+    case USER_STATUS.REGISTERED:
+      return '/login/error/registered';
+    case USER_STATUS.SUSPENDED:
+      return '/login/error/suspended';
+    case USER_STATUS.INVITED:
+      return '/invited';
+    case USER_STATUS.DELETED:
+      return '/login';
+    default:
+      return null;
+  }
+};

packages/app/test/integration/middlewares/login-required.test.js

@@ -286,7 +286,7 @@ describe('loginRequired', () => {
       expect(res.redirect).toHaveBeenCalledTimes(1);
       expect(res.redirect).toHaveBeenCalledWith('/login');
       expect(result).toBe('redirect');
-      expect(req.session.redirectTo).toBe('original url 1');
+      expect(req.session.redirectTo).toBe(undefined);
     });
 
   });