suggestion: Handle session expiration gracefully (#526)

* Return 403 for api endpoints instead of redirecting. * Stop interval and show modal when API fetches fail. * Created new ErrorModal component. * Only show modal on 403. * Tests. * Fix lint issues. * Fix client tests. * Check current and previous path.
mbta · Nov 15, 2024 · 2dcad9a · 2dcad9a
1 parent 6e323da
commit 2dcad9a
Show file tree

Hide file tree

Showing 11 changed files with 157 additions and 93 deletions.
diff --git a/assets/css/error-modal.scss b/assets/css/error-modal.scss
@@ -0,0 +1,40 @@
+.modal.error-modal {
+  color: $text-primary;
+  .modal-dialog {
+    margin-top: 362px;
+  }
+
+  .modal-content {
+    background-color: $cool-gray-20;
+
+    .modal-header {
+      border-bottom: none;
+
+      .btn-close:hover {
+        background-color: transparent;
+      }
+    }
+
+    .modal-footer {
+      background-color: $cool-gray-20;
+      border-top: none;
+      height: 62px;
+      padding: 0 12px 0 0;
+
+      .btn {
+        height: 38px;
+      }
+
+      .error-modal__refresh-button {
+        background-color: $button-primary;
+        color: $button-secondary;
+      }
+
+      .error-modal__cancel-button {
+        background-color: $cool-gray-20;
+        color: #c1e4ff;
+        border: transparent;
+      }
+    }
+  }
+}
diff --git a/assets/css/screenplay.scss b/assets/css/screenplay.scss
@@ -43,6 +43,7 @@ $form-feedback-invalid-color: $text-error;
 @import "sort-label.scss";
 @import "search-bar.scss";
 @import "dashboard/picker.scss";
+@import "error-modal.scss";
 @import "dashboard/toast.scss";
 @import "dashboard/kebab-menu.scss";
 @import "dashboard/filter-group.scss";

diff --git a/assets/css/workflow.scss b/assets/css/workflow.scss
@@ -56,46 +56,3 @@
     padding-left: 8px;
   }
 }
-
-.modal {
-  &.error-modal {
-    color: $text-primary;
-    .modal-dialog {
-      margin-top: 362px;
-    }
-
-    .modal-content {
-      background-color: $cool-gray-20;
-
-      .modal-header {
-        border-bottom: none;
-
-        .btn-close:hover {
-          background-color: transparent;
-        }
-      }
-
-      .modal-footer {
-        background-color: $cool-gray-20;
-        border-top: none;
-        height: 62px;
-        padding: 0 12px 0 0;
-
-        .btn {
-          height: 38px;
-        }
-
-        .error-modal__refresh-button {
-          background-color: $button-primary;
-          color: $button-secondary;
-        }
-
-        .error-modal__cancel-button {
-          background-color: $cool-gray-20;
-          color: #c1e4ff;
-          border: transparent;
-        }
-      }
-    }
-  }
-}
diff --git a/assets/js/components/Dashboard/Dashboard.tsx b/assets/js/components/Dashboard/Dashboard.tsx
@@ -13,6 +13,7 @@ import AlertBanner from "Components/AlertBanner";
 import LinkCopiedToast from "Components/LinkCopiedToast";
 import ActionOutcomeToast from "Components/ActionOutcomeToast";
 import { useLocation } from "react-router-dom";
+import ErrorModal from "Components/ErrorModal";
 
 const Dashboard: ComponentType = () => {
   const {
@@ -24,6 +25,8 @@ const Dashboard: ComponentType = () => {
   } = useScreenplayContext();
   const dispatch = useScreenplayDispatchContext();
   const [bannerDone, setBannerDone] = useState(false);
+  const [isAlertsIntervalRunning, setIsAlertsIntervalRunning] = useState(true);
+  const [showModal, setShowModal] = useState(false);
 
   useEffect(() => {
     fetchAlerts().then(
@@ -52,23 +55,35 @@ const Dashboard: ComponentType = () => {
   }, []); // eslint-disable-line react-hooks/exhaustive-deps
 
   // Fetch alerts every 4 seconds.
-  useInterval(() => {
-    fetchAlerts().then(
-      ({
-        all_alert_ids: allAPIalertIds,
-        alerts: newAlerts,
-        screens_by_alert: screensByAlertMap,
-      }) => {
-        findAndSetBannerAlert(alerts, newAlerts);
-        dispatch({
-          type: "SET_ALERTS",
-          alerts: newAlerts,
-          allAPIAlertIds: allAPIalertIds,
-          screensByAlertMap: screensByAlertMap,
+  useInterval(
+    () => {
+      fetchAlerts()
+        .then(
+          ({
+            all_alert_ids: allAPIalertIds,
+            alerts: newAlerts,
+            screens_by_alert: screensByAlertMap,
+          }) => {
+            findAndSetBannerAlert(alerts, newAlerts);
+            dispatch({
+              type: "SET_ALERTS",
+              alerts: newAlerts,
+              allAPIAlertIds: allAPIalertIds,
+              screensByAlertMap: screensByAlertMap,
+            });
+          },
+        )
+        .catch((response: Response) => {
+          if (response.status === 403) {
+            setIsAlertsIntervalRunning(false);
+            setShowModal(true);
+          } else {
+            throw response;
+          }
         });
-      },
-    );
-  }, 4000);
+    },
+    isAlertsIntervalRunning ? 4000 : null,
+  );
 
   const findAndSetBannerAlert = (oldAlerts: Alert[], newAlerts: Alert[]) => {
     const now = new Date();
@@ -187,6 +202,14 @@ const Dashboard: ComponentType = () => {
         )}
         <Outlet />
       </div>
+      <ErrorModal
+        title="Session expired"
+        showErrorModal={showModal}
+        onHide={() => setShowModal(false)}
+        errorMessage="Your session has expired, please refresh your browser."
+        confirmButtonLabel="Refresh now"
+        onConfirm={() => window.location.reload()}
+      />
     </div>
   );
 };

diff --git a/assets/js/components/Dashboard/ErrorModal.tsx b/assets/js/components/Dashboard/ErrorModal.tsx
@@ -0,0 +1,44 @@
+import React from "react";
+import { Button, Modal } from "react-bootstrap";
+
+interface ErrorModalProps {
+  title: string;
+  showErrorModal: boolean;
+  onHide: () => void;
+  errorMessage: string;
+  confirmButtonLabel: string;
+  onConfirm: () => void;
+}
+
+const ErrorModal = ({
+  title,
+  showErrorModal,
+  onHide,
+  errorMessage,
+  confirmButtonLabel,
+  onConfirm,
+}: ErrorModalProps) => {
+  return (
+    <Modal
+      show={showErrorModal}
+      className="error-modal"
+      backdrop="static"
+      onHide={onHide}
+    >
+      <Modal.Header closeButton closeVariant="white">
+        {title && <Modal.Title>{title}</Modal.Title>}
+      </Modal.Header>
+      <Modal.Body>{errorMessage}</Modal.Body>
+      <Modal.Footer>
+        <Button onClick={onHide} className="error-modal__cancel-button">
+          Cancel
+        </Button>
+        <Button className="error-modal__refresh-button" onClick={onConfirm}>
+          {confirmButtonLabel}
+        </Button>
+      </Modal.Footer>
+    </Modal>
+  );
+};
+
+export default ErrorModal;
diff --git a/assets/js/components/Dashboard/PermanentConfiguration/Workflows/GlEink/GlEinkWorkflow.tsx b/assets/js/components/Dashboard/PermanentConfiguration/Workflows/GlEink/GlEinkWorkflow.tsx
@@ -10,14 +10,15 @@ import ConfigureScreensWorkflowPage, {
 import BottomActionBar from "Components/PermanentConfiguration/BottomActionBar";
 import { useLocation, useNavigate } from "react-router-dom";
 import StationSelectPage from "Components/PermanentConfiguration/Workflows/GlEink/StationSelectPage";
-import { Alert, Button, Modal } from "react-bootstrap";
+import { Alert } from "react-bootstrap";
 import { ExclamationCircleFill } from "react-bootstrap-icons";
 import {
   useConfigValidationContext,
   useConfigValidationDispatchContext,
 } from "Hooks/useScreenplayContext";
 import { putPendingScreens } from "Utils/api";
 import { useScreenplayContext } from "Hooks/useScreenplayContext";
+import ErrorModal from "Components/ErrorModal";
 
 interface EditNavigationState {
   place_id: string;
@@ -275,35 +276,15 @@ const GlEinkWorkflow: ComponentType = () => {
       };
       layout = (
         <>
-          <Modal
-            show={showErrorModal}
-            className="error-modal"
+          <ErrorModal
+            title="Someone else is configuring these screens"
+            showErrorModal={showErrorModal}
             onHide={() => setShowErrorModal(false)}
-          >
-            <Modal.Header closeButton closeVariant="white">
-              <Modal.Title>
-                Someone else is configuring these screens
-              </Modal.Title>
-            </Modal.Header>
-            <Modal.Body>
-              In order not to overwrite each others work, please refresh your
-              browser and fill-out the form again.
-            </Modal.Body>
-            <Modal.Footer>
-              <Button
-                onClick={() => setShowErrorModal(false)}
-                className="error-modal__cancel-button"
-              >
-                Cancel
-              </Button>
-              <Button
-                className="error-modal__refresh-button"
-                onClick={() => window.location.reload()}
-              >
-                Refresh now
-              </Button>
-            </Modal.Footer>
-          </Modal>
+            errorMessage="In order not to overwrite each others work, please refresh your
+              browser and fill-out the form again."
+            confirmButtonLabel="Refresh now"
+            onConfirm={() => window.location.reload()}
+          />
           <ConfigureScreensWorkflowPage
             selectedPlaces={filteredPlaces}
             setPlacesAndScreensToUpdate={setPlacesAndScreensToUpdate}

diff --git a/assets/js/hooks/useInterval.tsx b/assets/js/hooks/useInterval.tsx
@@ -6,7 +6,7 @@ function noop() {
   // do nothing
 }
 
-export function useInterval(callback: () => void, delay: number) {
+export function useInterval(callback: () => void, delay: number | null) {
   const savedCallback = useRef<() => void>(noop);
 
   // Remember the latest callback.

diff --git a/assets/js/utils/api.ts b/assets/js/utils/api.ts
@@ -19,7 +19,11 @@ interface AlertsResponse {
 
 export const fetchAlerts = async (): Promise<AlertsResponse> => {
   const response = await fetch("/api/alerts");
-  return await response.json();
+  if (response.status === 200) {
+    return await response.json();
+  } else {
+    throw response;
+  }
 };
 
 export const fetchActiveAndFutureAlerts = async (): Promise<AlertsResponse> => {

diff --git a/assets/tests/setup.ts b/assets/tests/setup.ts
@@ -33,6 +33,7 @@ beforeEach(() => {
     .fn()
     .mockReturnValueOnce(
       Promise.resolve({
+        status: 200,
         json: () =>
           Promise.resolve({
             all_alert_ids: allAPIAlertIds,

diff --git a/lib/screenplay_web/auth_manager/error_handler.ex b/lib/screenplay_web/auth_manager/error_handler.ex
@@ -9,8 +9,12 @@ defmodule ScreenplayWeb.AuthManager.ErrorHandler do
 
   @impl Guardian.Plug.ErrorHandler
   def auth_error(conn, error, _opts) do
-    auth_params = auth_params_for_error(error)
-    Phoenix.Controller.redirect(conn, to: ~p"/auth/keycloak?#{auth_params}")
+    if conn.request_path =~ "api" or Plug.Conn.get_session(conn, :previous_path) =~ "api" do
+      Plug.Conn.send_resp(conn, 403, "Session expired")
+    else
+      auth_params = auth_params_for_error(error)
+      Phoenix.Controller.redirect(conn, to: ~p"/auth/keycloak?#{auth_params}")
+    end
   end
 
   def auth_params_for_error({:invalid_token, {:auth_expired, sub}}) do

diff --git a/test/screenplay_web/auth_manager/error_handler_test.exs b/test/screenplay_web/auth_manager/error_handler_test.exs
@@ -4,10 +4,19 @@ defmodule ScreenplayWeb.AuthManager.ErrorHandlerTest do
   alias ScreenplayWeb.AuthManager.ErrorHandler
 
   describe "auth_error/3" do
+    test "returns 403 for API endpoints if there's no refresh key", %{conn: conn} do
+      conn =
+        conn
+        |> init_test_session(%{previous_path: "api/test"})
+        |> ErrorHandler.auth_error({:some_type, :reason}, [])
+
+      assert %{status: 403} = conn
+    end
+
     test "redirects to Keycloak login if there's no refresh key", %{conn: conn} do
       conn =
         conn
-        |> init_test_session(%{})
+        |> init_test_session(%{previous_path: "test"})
         |> ErrorHandler.auth_error({:some_type, :reason}, [])
 
       assert html_response(conn, 302) =~ "\"/auth/keycloak\?prompt%3Dlogin"