Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggestion: Handle session expiration gracefully #526

Merged
merged 9 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions assets/css/error-modal.scss
Original file line number Diff line number Diff line change
@@ -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;
}
}
}
}
1 change: 1 addition & 0 deletions assets/css/screenplay.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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";

html {
Expand Down
43 changes: 0 additions & 43 deletions assets/css/workflow.scss
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
}
}
55 changes: 39 additions & 16 deletions assets/js/components/Dashboard/Dashboard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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(
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we'd only show this modal if, specifically, a background alerts request fails. How is the experience if some other kind of "foreground" request fails? Should we try to integrate this into all instances where we fetch data from the server?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this task was already kind of scope creep-y, I focused specifically on what I knew to be the problem we see in Sentry.

You do raise a good point though. We've had a habit of assuming a request will be successful so we unintentionally fail silently in some areas. For example, I hardcoded a 500 for the Associate with alert page and the only indicator that something went wrong is that the table is empty. Because this would be a bigger task, I'll get it added to the backlog for us to talk about in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you end up writing this task? (sorry for letting this review drop off)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wrote it up here.

} else {
throw response;
}
});
},
);
}, 4000);
},
isAlertsIntervalRunning ? 4000 : null,
);

const findAndSetBannerAlert = (oldAlerts: Alert[], newAlerts: Alert[]) => {
const now = new Date();
Expand Down Expand Up @@ -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>
);
};
Expand Down
44 changes: 44 additions & 0 deletions assets/js/components/Dashboard/ErrorModal.tsx
Original file line number Diff line number Diff line change
@@ -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;
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}
Expand Down
2 changes: 1 addition & 1 deletion assets/js/hooks/useInterval.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
6 changes: 5 additions & 1 deletion assets/js/utils/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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> => {
Expand Down
1 change: 1 addition & 0 deletions assets/tests/setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ beforeEach(() => {
.fn()
.mockReturnValueOnce(
Promise.resolve({
status: 200,
json: () =>
Promise.resolve({
all_alert_ids: allAPIAlertIds,
Expand Down
8 changes: 6 additions & 2 deletions lib/screenplay_web/auth_manager/error_handler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 10 additions & 1 deletion test/screenplay_web/auth_manager/error_handler_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Loading