Skip to content

Commit

Permalink
chore: improve authentication handling
Browse files Browse the repository at this point in the history
Using the admin UI was previously plagued by auth errors that would
occur after just a few minutes of idle time and required refreshing the
page, losing any work in progress.

These changes should improve how we handle authentication, allowing an
admin to be idle for up to 30 minutes and signed in for up to 12 hours
before needing to reauthenticate, following the TID Session Management
guidelines and the prior art of mbta/screenplay#520.

This also revises and unifies how data fetching works in the admin UI,
to allow showing admins a message indicating their session has expired
when that happens (regardless of the interaction), rather than a generic
error message.

In support of the above feature being able to distinguish HTML page
requests from API requests, this _also_ disentangles some pipelines in
the router where some API endpoints were piped through both "browser"
and "api", causing their format to be improperly set as HTML.
  • Loading branch information
digitalcora committed Jan 24, 2025
1 parent 8c44263 commit d9a86bb
Show file tree
Hide file tree
Showing 19 changed files with 178 additions and 105 deletions.
6 changes: 3 additions & 3 deletions assets/src/components/admin/admin_form.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { useState, useEffect, useRef } from "react";
import { doSubmit } from "Util/admin";
import { fetch } from "Util/admin";

const validateJson = (json) => {
try {
Expand Down Expand Up @@ -30,7 +30,7 @@ const AdminValidateControls = ({
const config = configRef.current.value;
if (validateJson(config)) {
const dataToSubmit = { config };
doSubmit(validatePath, dataToSubmit).then(validateCallback);
fetch.post(validatePath, dataToSubmit).then(validateCallback);
} else {
alert("JSON is invalid!");
}
Expand Down Expand Up @@ -65,7 +65,7 @@ const AdminConfirmControls = ({
const confirmFn = () => {
const config = configRef.current.value;
const dataToSubmit = { config };
doSubmit(confirmPath, dataToSubmit).then(confirmCallback);
fetch.post(confirmPath, dataToSubmit).then(confirmCallback);
};

return (
Expand Down
26 changes: 7 additions & 19 deletions assets/src/components/admin/admin_image_manager.tsx
Original file line number Diff line number Diff line change
@@ -1,23 +1,17 @@
import React, { useCallback, useEffect, useState } from "react";
import { useDropzone } from "react-dropzone";
import _ from "lodash";
import getCsrfToken from "Util/csrf";

import { fetch } from "Util/admin";

interface FileWithPreview extends File {
preview: string;
}

const fetchWithCsrf = (resource: RequestInfo, init: RequestInit = {}) => {
return fetch(resource, {
...init,
headers: { ...(init?.headers || {}), "x-csrf-token": getCsrfToken() },
credentials: "include",
});
};

const fetchImageFilenames = async () => {
const response = await fetchWithCsrf("/api/admin/image_filenames");
const { image_filenames: imageFilenames } = await response.json();
const { image_filenames: imageFilenames } = await fetch.get(
"/api/admin/image_filenames",
);
return _.sortBy(imageFilenames);
};

Expand Down Expand Up @@ -88,12 +82,8 @@ const ImageUpload = (): JSX.Element => {
formData.append("image", stagedImageUpload, stagedImageUpload.name);

try {
const response = await fetchWithCsrf("/api/admin/image", {
method: "POST",
body: formData,
});
const result = await fetch.post("/api/admin/image", formData);

const result = await response.json();
if (result.success) {
alert(`Success. Image has been uploaded as "${result.uploaded_name}".`);
location.reload();
Expand Down Expand Up @@ -166,12 +156,10 @@ const ImageManager = ({ imageFilenames }): JSX.Element => {
setIsDeleting(true);

try {
const response = await fetchWithCsrf(
const result = await fetch.delete(
`/api/admin/image/${selectedFilename}`,
{ method: "DELETE" },
);

const result = await response.json();
if (result.success) {
alert(`Success. "${selectedFilename}" has been deleted.`);
location.reload();
Expand Down
6 changes: 3 additions & 3 deletions assets/src/components/admin/admin_screen_config_form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ import React from "react";
import _ from "lodash";

import AdminForm from "Components/admin/admin_form";
import { fetch } from "Util/admin";

const fetchConfig = async () => {
const result = await fetch("/api/admin/");
const resultJson = await result.json();
const { config } = await fetch.get("/api/admin");

// This sorts the entries alphanumerically by screen ID, and otherwise leaves the config alone.
const screens = _.chain(JSON.parse(resultJson.config).screens)
const screens = _.chain(JSON.parse(config).screens)
.toPairs()
.sortBy(([screenId, _screenData]) => screenId)
.fromPairs()
Expand Down
14 changes: 6 additions & 8 deletions assets/src/components/admin/admin_table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { useTable, useFilters, useRowSelect } from "react-table";
import _ from "lodash";
import weakKey from "weak-key";

import { doSubmit } from "Util/admin";
import { fetch } from "Util/admin";
import { IndeterminateCheckbox } from "Components/admin/admin_cells";
import AddModal from "Components/admin/admin_add_modal";
import EditModal from "Components/admin/admin_edit_modal";
Expand Down Expand Up @@ -157,15 +157,15 @@ const dataToConfig = (data) => {
const doValidate = async (data, onValidate) => {
const config = dataToConfig(data);
const dataToSubmit = { config: JSON.stringify(config, null, 2) };
const result = await doSubmit(VALIDATE_PATH, dataToSubmit);
const result = await fetch.post(VALIDATE_PATH, dataToSubmit);
const validatedConfig = await configToData(result.config);
onValidate(validatedConfig);
};

const doConfirm = async (data, setEditable) => {
const config = dataToConfig(data);
const dataToSubmit = { config: JSON.stringify(config, null, 2) };
const result = await doSubmit(CONFIRM_PATH, dataToSubmit);
const result = await fetch.post(CONFIRM_PATH, dataToSubmit);
if (result.success === true) {
alert("Config updated successfully");
window.location.reload();
Expand All @@ -182,7 +182,7 @@ const doRefresh = async (data, selectedRowIds) => {
const selectedRows = _.filter(data, (_row, i) => selectedRowIds[i]);
const selectedScreenIds = _.map(selectedRows, ({ id }) => id);
const dataToSubmit = { screen_ids: selectedScreenIds };
const result = await doSubmit(REFRESH_PATH, dataToSubmit);
const result = await fetch.post(REFRESH_PATH, dataToSubmit);
if (result.success === true) {
alert("Refresh scheduled successfully");
window.location.reload();
Expand Down Expand Up @@ -269,10 +269,8 @@ const AdminTable = ({ columns, dataFilter }): JSX.Element => {

// Fetch config on page load
const fetchConfig = async () => {
const result = await fetch("/api/admin/");
const json = await result.json();
const config = JSON.parse(json.config);
setData(configToData(config));
const { config } = await fetch.get("/api/admin");
setData(configToData(JSON.parse(config)));
};

useEffect(() => {
Expand Down
12 changes: 7 additions & 5 deletions assets/src/components/admin/devops.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import React, { useState, useEffect } from "react";

import { doSubmit } from "Util/admin";
import { fetch } from "Util/admin";

const DEVOPS_PATH = "/api/admin/devops";

const updateDisabledModes = async (disabledModes) => {
const result = await doSubmit(DEVOPS_PATH, { disabled_modes: disabledModes });
const result = await fetch.post(DEVOPS_PATH, {
disabled_modes: disabledModes,
});
if (result.success !== true) {
alert("Config update failed");
}
Expand All @@ -27,9 +29,9 @@ const Devops = () => {
const [loaded, setLoaded] = useState(false);

useEffect(() => {
fetch("/api/admin/")
.then((result) => result.json())
.then((json) => JSON.parse(json.config))
fetch
.get("/api/admin")
.then((response) => JSON.parse(response.config))
.then((config) => config.devops.disabled_modes)
.then(setDisabledModes)
.then((_) => setLoaded(true))
Expand Down
19 changes: 8 additions & 11 deletions assets/src/components/admin/inspector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import AdminForm from "./admin_form";

import { type AudioConfig } from "Components/v2/screen_container";

import { doSubmit, type Config, type Screen } from "Util/admin";
import { fetch, type Config, type Screen } from "Util/admin";
import {
type Message,
INSPECTOR_FRAME_NAME,
Expand Down Expand Up @@ -46,11 +46,10 @@ const Inspector: ComponentType = () => {
const [config, setConfig] = useState<Config | null>(null);

useEffect(() => {
fetch("/api/admin")
.then((result) => result.json())
.then((json) => JSON.parse(json.config))
.then((config) => setConfig(config))
.catch(() => alert("Failed to load config!"));
fetch
.get("/api/admin")
.then((response) => JSON.parse(response.config))
.then((config) => setConfig(config));
}, []);

const { search } = useLocation();
Expand Down Expand Up @@ -238,7 +237,8 @@ const ConfigControls: ComponentType<{ screen: ScreenWithId }> = ({
disabled={isRequestingReload}
onClick={() => {
setIsRequestingReload(true);
doSubmit("/api/admin/refresh", { screen_ids: [screen.id] })
fetch
.post("/api/admin/refresh", { screen_ids: [screen.id] })
.then(() => alert("Scheduled a reload for this screen."))
.finally(() => setIsRequestingReload(false));
}}
Expand Down Expand Up @@ -415,10 +415,7 @@ const AudioControls: ComponentType<{ screen: ScreenWithId }> = ({ screen }) => {
<button
onClick={() => {
setSSML("Loading...");
fetch(`${audioPath}/debug`)
.then((result) => result.text())
.then((text) => setSSML(text))
.catch(() => alert("Failed to fetch SSML!"));
fetch.text(`${audioPath}/debug`).then((text) => setSSML(text));
}}
>
Show SSML
Expand Down
44 changes: 32 additions & 12 deletions assets/src/util/admin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,43 @@ const gatherSelectOptions = (rows, columnId) => {
return Array.from(uniqueOptions);
};

const doSubmit = async (path, data) => {
try {
const result = await fetch(path, {
method: "POST",
const fetch = {
get: (path) => doFetch(path, {}),

post: (path, data) => {
return doFetch(path, {
body: JSON.stringify(data),
headers: {
"content-type": "application/json",
"x-csrf-token": getCsrfToken(),
},
credentials: "include",
body: JSON.stringify(data),
method: "POST",
});
const json = await result.json();
return json;
} catch (err) {
alert("An error occurred.");
throw err;
},

delete: (path) => doFetch(path, { method: "DELETE" }),

text: (path) => doFetch(path, {}, (response) => response.text()),
};

const doFetch = async (
path,
opts,
handleResponse = (response) => response.json(),
) => {
try {
const response = await window.fetch(path, opts);

if (response.status === 401) {
alert("Your session has expired; refresh the page to continue.");
throw new Error("unauthenticated");
} else {
return handleResponse(response);
}
} catch (error) {
alert(`An error occurred: ${error}`);
throw error;
}
};

export { gatherSelectOptions, doSubmit };
export { fetch, gatherSelectOptions };
17 changes: 15 additions & 2 deletions config/config.exs
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,25 @@ config :screens,
redirect_http?: true,
keycloak_role: "screens-admin"

config :screens, ScreensWeb.AuthManager, issuer: "screens"
max_session_time = 12 * 60 * 60

config :screens, ScreensWeb.AuthManager,
idle_time: 30 * 60,
issuer: "screens",
max_session_time: max_session_time

# Placeholder for Keycloak authentication, defined for real in environment configs
config :ueberauth, Ueberauth,
providers: [
keycloak: nil
keycloak: {
Ueberauth.Strategy.Oidcc,
authorization_params: %{max_age: "#{max_session_time}"},
authorization_params_passthrough: ~w(prompt login_hint),
issuer: :keycloak_issuer,
scopes: ~w(openid email),
uid_field: "email",
userinfo: true
}
]

config :ex_cldr,
Expand Down
7 changes: 0 additions & 7 deletions config/prod.exs
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,6 @@ config :screens, :screens_by_alert,
screens_last_updated_ttl_seconds: 3600,
screens_ttl_seconds: 40

# Configure Ueberauth to use Keycloak
config :ueberauth, Ueberauth,
providers: [
keycloak:
{Ueberauth.Strategy.Oidcc, userinfo: true, uid_field: "email", scopes: ~w(openid email)}
]

# ## SSL Support
#
# To get SSL working, you will need to add the `https` key
Expand Down
11 changes: 4 additions & 7 deletions config/runtime.exs
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,6 @@ if config_env() == :prod do
coder: Screens.ScreensByAlert.Memcache.SafeErlangCoder
]

keycloak_opts = [
issuer: :keycloak_issuer,
client_id: System.fetch_env!("KEYCLOAK_CLIENT_ID"),
client_secret: System.fetch_env!("KEYCLOAK_CLIENT_SECRET")
]

config :ueberauth_oidcc,
issuers: [
%{
Expand All @@ -59,7 +53,10 @@ if config_env() == :prod do
}
],
providers: [
keycloak: keycloak_opts
keycloak: [
client_id: System.fetch_env!("KEYCLOAK_CLIENT_ID"),
client_secret: System.fetch_env!("KEYCLOAK_CLIENT_SECRET")
]
]
end

Expand Down
3 changes: 3 additions & 0 deletions lib/screens/ueberauth/strategy/fake.ex
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ defmodule Screens.Ueberauth.Strategy.Fake do
def extra(conn) do
%Ueberauth.Auth.Extra{
raw_info: %UeberauthOidcc.RawInfo{
claims: %{
"iat" => System.system_time(:second)
},
userinfo: %{
"resource_access" => %{
"dev-client" => %{"roles" => Ueberauth.Strategy.Helpers.options(conn)[:roles]}
Expand Down
16 changes: 16 additions & 0 deletions lib/screens_web/auth_manager.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ defmodule ScreensWeb.AuthManager do

use Guardian, otp_app: :screens

@idle_time Application.compile_env!(:screens, [__MODULE__, :idle_time])
@max_session_time Application.compile_env!(:screens, [__MODULE__, :max_session_time])

@impl true
def subject_for_token(resource, _claims) do
{:ok, resource}
Expand All @@ -14,4 +17,17 @@ defmodule ScreensWeb.AuthManager do
end

def resource_from_claims(_), do: {:error, :invalid_claims}

@impl true
def verify_claims(%{"auth_time" => user_authed_at, "iat" => token_issued_at} = claims, _opts) do
auth_expires_at = user_authed_at + @max_session_time
token_expires_at = token_issued_at + @idle_time

# is either expiration time in the past?
if min(auth_expires_at, token_expires_at) < System.system_time(:second) do
{:error, {:auth_expired, claims["sub"]}}
else
{:ok, claims}
end
end
end
Loading

0 comments on commit d9a86bb

Please sign in to comment.