fix: fix protected routes

Author: dziraf

src/authentication/protected-routes.handler.ts

@@ -3,38 +3,33 @@ import { Router } from "express";
 import { convertToExpressRoute } from "../convertRoutes";
 import { pathToRegexp } from "path-to-regexp";
 
+const doesNotRequireAuthentication = (
+  url: string,
+  { loginPath, logoutPath }
+) => {
+  return (
+    isAdminAsset(url) || url.startsWith(loginPath) || url.startsWith(logoutPath)
+  );
+};
+
 export const withProtectedRoutesHandler = (
   router: Router,
   admin: AdminJS
 ): void => {
   const { rootPath, loginPath, logoutPath } = admin.options;
 
   router.use((req, res, next) => {
-    if (isAdminAsset(req.originalUrl)) {
-      next();
-    } else if (
-      req.session.adminUser ||
-      // these routes doesn't need authentication
-      req.originalUrl.startsWith(loginPath) ||
-      req.originalUrl.startsWith(logoutPath)
+    if (
+      doesNotRequireAuthentication(req.originalUrl, { loginPath, logoutPath })
     ) {
-      next();
-    } else if (isAdminRoute(req.originalUrl, rootPath)) {
-      // If the redirection is caused by API call to some action just redirect to resource
-      const [redirectTo] = req.originalUrl.split("/actions");
-      req.session.redirectTo = redirectTo.includes(`${rootPath}/api`)
-        ? rootPath
-        : redirectTo;
+      return next();
+    }
 
-      req.session.save((err) => {
-        if (err) {
-          next(err);
-        }
-        res.redirect(loginPath);
-      });
-    } else {
-      next();
+    if (isAdminRoute(req.originalUrl, rootPath) && !!req.session.adminUser) {
+      return next();
     }
+
+    return res.redirect(loginPath);
   });
 };
 
@@ -43,7 +38,7 @@ export const isAdminRoute = (url: string, adminRootPath: string): boolean => {
     .map((route) => convertToExpressRoute(route.path))
     .filter((route) => route !== "");
 
-  let urlWithoutAdminRootPath = url;
+  let urlWithoutAdminRootPath = url.split("?")[0];
   if (adminRootPath !== "/") {
     urlWithoutAdminRootPath = url.replace(adminRootPath, "");
     if (!urlWithoutAdminRootPath.startsWith("/")) {
@@ -55,7 +50,7 @@ export const isAdminRoute = (url: string, adminRootPath: string): boolean => {
 
   return (
     isAdminRootUrl ||
-    !!adminRoutes.find((route) =>
+    adminRoutes.some((route) =>
       pathToRegexp(route).test(urlWithoutAdminRootPath)
     )
   );

test/protected-routes.test.ts

@@ -56,6 +56,18 @@ describe("Protected routes", () => {
       });
     });
 
+    it("should detect admin routes when query params are included", () => {
+      const route = "/resources/list?filters.someFilter=123";
+
+      expect(isAdminRoute(route, "/")).toBeTruthy();
+    });
+
+    it("should not detect admin routes when query params are included but root is different", () => {
+      const route = "/resources/list?filters.someFilter=123";
+
+      expect(isAdminRoute(route, "/admin")).toBeFalsy();
+    });
+
     it("should detect non-admin routes when root path is /", () => {
       expect(isAdminRoute("/api/my-endpoint", "/")).toBeFalsy();
     });