suggestion: Handle session expiration gracefully #526
Conversation
| @@ -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 Plug.Conn.get_session(conn, :previous_path) =~ "api" do | |||
There was a problem hiding this comment.
Shouldn't we be looking at the current request path rather than the previous one? I'm surprised we set a "previous path" for API requests at all — I'd expect that value to always be some browser-navigable route.
There was a problem hiding this comment.
I think the main reason for holding onto the previous path is for the sake of auth redirects. When you log in to Keycloak, you're temporarily not at the URL you want to be at so we help the user get there by holding on to their original request path. I used it here just in case it landed in this function from a Keycloak request. Because save_previous_path/2 is part of the request pipeline, it would be the same URL regardless of where the error surfaced.
There was a problem hiding this comment.
I think we should check the current request path in any case, given that sometimes we may not have a previous path or it may be a non-API page path (like for the first API request that happens after doing a full page load). "If current path is api or previous path is api" in other words.
| .catch((response: Response) => { | ||
| if (response.status === 403) { | ||
| setIsAlertsIntervalRunning(false); | ||
| setShowModal(true); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did you end up writing this task? (sorry for letting this review drop off)
Even after #520, we still need to handle errors when someone has Screenplay open during a deploy. I think this can best be handled by looking for
apiin the previous path and returning a 403 instead of a redirect. This allows us to throw/catch an exception on the client and display a modal letting the user know they need to refresh the page.