Skip to content
Open
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
4 changes: 2 additions & 2 deletions scripts/rust-style-ceilings.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"tests/unit/livekit/session.rs": 6023,
"tests/unit/livekit/turn.rs": 3405,
"tests/unit/tasks/session.rs": 1861,
"tests/web/accounts.rs": 1808,
"tests/web/accounts.rs": 1760,
"tests/web/recording.rs": 2496,
"tests/web/token.rs": 2151
"tests/web/token.rs": 2101
}
5 changes: 5 additions & 0 deletions src/accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ pub struct SignedInUser {
/// `None` for every self-declared login, which is the whole point: a typed
/// handle is a label, and a recording is delivered to a person.
pub verified_email: Option<String>,
/// Stable account identity stored in `users.github_id`.
/// Positive values are GitHub account IDs. Negative values are fresh
/// random identities for self-declared accounts. Unlike `users.id`, this
/// value is not recycled when a throwaway account is deleted.
pub account_identity: i64,
/// GitHub's numeric account ID, for a login GitHub vouched for; `None` for
/// a self-declared one.
pub github_id: Option<u64>,
Expand Down
6 changes: 4 additions & 2 deletions src/accounts/sessions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,16 @@ pub fn session_user(
// how an unverified value becomes a delivery target one
// refactor later.
let verified: i64 = row.get(4)?;
let account_identity: i64 = row.get(5)?;
Ok(SignedInUser {
id: row.get(0)?,
// Trusted only under create_session's caller invariant.
identity_verified: row.get::<_, i64>(5)? > 0,
identity_verified: account_identity > 0,
login: row.get(1)?,
avatar_url: row.get(2)?,
verified_email: row.get::<_, Option<String>>(3)?.filter(|_| verified != 0),
github_id: u64::try_from(row.get::<_, i64>(5)?)
account_identity,
github_id: u64::try_from(account_identity)
.ok()
.filter(|id| *id > 0),
})
Expand Down
65 changes: 41 additions & 24 deletions src/web/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ pub(crate) fn github_oauth_scope(recording: bool) -> &'static str {
}
}

/// A stable browser-facing account identifier without exposing the sequential
/// `users.id`. The page only compares owner ids for equality.
pub(crate) fn account_owner_id(accounts: &Accounts, account_identity: i64) -> String {
crate::token::sign_hs256(
&accounts.config().session_secret,
&format!("account-owner:{account_identity}"),
)
}

pub fn login_config(config: &WebServerConfig) -> Option<GitHubLoginConfig> {
let (Some(session_secret), Some(db_path)) = (
config
Expand Down Expand Up @@ -561,32 +570,40 @@ pub(crate) async fn session_handler(
.as_ref()
.and_then(|dispatcher| dispatcher.candidate_video());
match current_user(state.accounts.as_ref(), request.headers()).await {
Ok(Some(user)) => json_response(
StatusCode::OK,
super::tasks::with_built_in_task(
&state,
with_peer_flags(
Ok(Some(user)) => {
let Some(accounts) = state.accounts.as_ref() else {
return state.accounts_error();
};
let account_id = account_owner_id(accounts, user.account_identity);
Comment on lines +574 to +577

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

current_user only returns Ok(Some(user)) when state.accounts is Some, so this else never runs, and binding accounts here is what pushes the whole json! body one level deeper. Leaving the arm as it was and computing the field in place keeps the diff to the one new line:

"user": {
    "id": state.accounts.as_ref()
        .map(|accounts| account_owner_id(accounts, user.account_identity)),
    "login": user.login,
    "avatarUrl": user.avatar_url
}

json_response(
StatusCode::OK,
super::tasks::with_built_in_task(
&state,
json!({
"signedIn": true,
"identityVerified": user.identity_verified,

// Task mode admits only a login GitHub vouched for; the
// task page asks before the learner types a PIN.
"githubSignedIn": user.github_id.is_some(),
"loginRequired": login_required,
"guestModeAvailable": state.guest_mode_available(),
"deviceLogin": device_client_id(&state.config).is_some(),
"maxDurationMin": max_duration_min,
"candidateVideo": candidate_video,
"user": {
"login": user.login,
"avatarUrl": user.avatar_url
}
}),
with_peer_flags(
&state,
json!({
"signedIn": true,
"identityVerified": user.identity_verified,

// Task mode admits only a login GitHub vouched for;
// the task page asks before the learner types a
// PIN.
"githubSignedIn": user.github_id.is_some(),
"loginRequired": login_required,
"guestModeAvailable": state.guest_mode_available(),
"deviceLogin": device_client_id(&state.config).is_some(),
"maxDurationMin": max_duration_min,
"candidateVideo": candidate_video,
"user": {
"id": account_id,
"login": user.login,
"avatarUrl": user.avatar_url
}
}),
),
),
),
),
)
}
Ok(None) => json_response(
StatusCode::OK,
super::tasks::with_built_in_task(
Expand Down
11 changes: 9 additions & 2 deletions src/web/interviews.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::accounts::{
};
use crate::current_epoch_seconds_i64;

use super::auth::Owner;
use super::auth::{Owner, account_owner_id};
use super::recordings::finish_recording;
use super::{
AppState, MAX_BODY_BYTES, MAX_REPORT_BYTES, json_response, rate_limited_response,
Expand All @@ -33,8 +33,15 @@ use super::{
pub const REPLAY_RATE_LIMIT: u32 = 120;

pub(crate) async fn list_reports_handler(Owner { accounts, user }: Owner) -> Response {
let account_id = account_owner_id(&accounts, user.account_identity);
match blocking(move || list_reports(&accounts, user.id)).await {
Ok(reports) => json_response(StatusCode::OK, json!({ "reports": reports })),
Ok(reports) => json_response(
StatusCode::OK,
json!({
"accountId": account_id,
"reports": reports,
}),
),
Err(_) => json_response(
StatusCode::INTERNAL_SERVER_ERROR,
json!({ "error": "Could not read reports." }),
Expand Down
8 changes: 7 additions & 1 deletion src/web/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use crate::config::{DEFAULT_DURATION_MIN, MAX_DURATION_MIN, MIN_DURATION_MIN};
use crate::current_epoch_seconds;
use crate::token::{LivekitTokenInput, TOKEN_TTL_SECONDS, livekit_observer_token, livekit_token};

use super::auth::{Owner, current_user};
use super::auth::{Owner, account_owner_id, current_user};
use super::consent::{checked_consent, claim_consent};
use super::pool::{ProviderChoice, room_and_available_provider};
use super::{
Expand Down Expand Up @@ -882,6 +882,12 @@ pub(crate) async fn token_handler(
// The same granted length the agent reads from its token metadata.
"durationMin": response.duration_min
});
if let Some(owner) = owner.as_ref() {
body["accountId"] = json!(account_owner_id(
&owner.accounts,
owner.user.account_identity
));
}
if let Some(assignment) = assignment {
body["problemPage"] = json!(assignment.page);
body["previousAssignment"] = json!(assignment.receipt);
Expand Down
11 changes: 7 additions & 4 deletions tests/browser/account.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ test("interview history routes through the shared persistence helper", () => {
assert.match(functionBody(script, "renderReport"), /saveResult: null/);
assert.match(
functionBody(script, "renderReportSaveStatus"),
/querySelector\("#done"\)\.disabled = state\.downloadingReport/,
/querySelector\("#done"\)\.disabled =\s*state\.reportReceiving \|\| state\.downloadingReport/,
);
assert.match(
functionBody(script, "renderReport"),
Expand Down Expand Up @@ -306,10 +306,11 @@ test("optional interview profile is accessible, bounded, and omitted when blank"
'if (profile.targetCompany) destination.searchParams.set("company", profile.targetCompany)',
);
assert.match(interview, /const interviewProfile = \{/);
// The focus travels in session storage, never the address bar.
assert.match(
// The focus travels in session storage, never the address bar, and sharing
// consent is checked again immediately before the handoff is stored.
assertIncludesCompact(
app,
/storeSharedFocus\(sessionStorage, focus\?\.weakness \?\? null\)/,
"storeSharedFocus(sessionStorage, nodes.practiceFocusShareInput.checked ? (focus?.weakness ?? null) : null)",
);
assert.doesNotMatch(app, /searchParams\.set\("focus"/);
assert.match(interview, /practiceFocus: consumeSharedFocus\(tabStorage\)/);
Expand Down Expand Up @@ -1509,6 +1510,7 @@ test("updateCategoryComparison renders category progress and degrades cleanly wh
"loadReportHistory",
"updateCategoryComparison",
"downloadReport",
"setPracticeFocusButtonsDisabled",
"buildMarkdown",
]
.map((name) => `${functionBody(interview, name)}\n}`)
Expand Down Expand Up @@ -1593,6 +1595,7 @@ test("updateCategoryComparison renders category progress and degrades cleanly wh
nodes: {
report: {
innerHTML: "existing-report-dom",
querySelectorAll: () => [],
querySelector(selector) {
if (selector === "#download-report") return downloadButton;
if (selector === ".category-comparison") {
Expand Down
19 changes: 16 additions & 3 deletions tests/browser/download.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ for (const [path, name] of [
const downloads = [];
const button = { disabled: false };
const done = { disabled: false };
const practice = [{ disabled: false }, { disabled: false }];
const scope = {
state: {
downloadingReport: false,
Expand All @@ -20,6 +21,7 @@ for (const [path, name] of [
},
nodes: {
report: {
querySelectorAll: () => practice,
querySelector: (selector) => (selector === "#done" ? done : button),
},
},
Expand All @@ -40,6 +42,7 @@ for (const [path, name] of [
"scope",
`with (scope) {
${name === "exportSavedReport" ? `${functionBody(read(path), "downloadSavedReport")}\n}` : ""}
${name === "downloadReport" ? `${functionBody(read(path), "setPracticeFocusButtonsDisabled")}\n}` : ""}
${functionBody(read(path), name)}
}
return ${name};
Expand All @@ -56,6 +59,7 @@ for (const [path, name] of [
run,
button,
done,
practice,
lookups,
downloads,
scope,
Expand All @@ -68,7 +72,10 @@ for (const [path, name] of [
const h = harness();
const first = h.run();
assert.equal(h.button.disabled, true);
if (name === "downloadReport") assert.equal(h.done.disabled, true);
if (name === "downloadReport") {
assert.equal(h.done.disabled, true);
assert.ok(h.practice.every((button) => button.disabled));
}
await h.run();
await h.run();
assert.equal(h.lookups.length, 1);
Expand All @@ -77,7 +84,10 @@ for (const [path, name] of [
await first;
assert.equal(h.downloads.length, 1);
assert.equal(h.button.disabled, false);
if (name === "downloadReport") assert.equal(h.done.disabled, false);
if (name === "downloadReport") {
assert.equal(h.done.disabled, false);
assert.ok(h.practice.every((button) => !button.disabled));
}
const next = h.run();
assert.equal(h.button.disabled, true);
h.lookups[1].resolve(null);
Expand All @@ -101,7 +111,10 @@ for (const [path, name] of [
else h.lookups[0].resolve(null);
await rejected;
assert.equal(h.button.disabled, false);
if (name === "downloadReport") assert.equal(h.done.disabled, false);
if (name === "downloadReport") {
assert.equal(h.done.disabled, false);
assert.ok(h.practice.every((button) => !button.disabled));
}
h.scope.downloadMarkdown = originalDownload;
const retry = h.run();
h.lookups[1].resolve(null);
Expand Down
14 changes: 14 additions & 0 deletions tests/browser/history.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,8 @@ test("the response window panel says what the number is worth, in words a test c
// Wiring: selectors, module paths, tags, classes, dataset and event names.
"",
"/lib.js",
"./problem-picker.js",
"./audio-output.js",
"/render.js",
"/report-original-problem.js",
"/api/reports",
Expand All @@ -477,6 +479,9 @@ test("the response window panel says what the number is worth, in words a test c
"aria-current",
"true",
"[data-moment]",
"[data-practice-focus]",
"[data-practice-focus-status]",
"sessionStorage",
"replay-item",
"replay-line",
"2d",
Expand Down Expand Up @@ -522,6 +527,7 @@ test("the response window panel says what the number is worth, in words a test c
"Could not load the replay for this interview.",
"Could not load the report.",
"Could not read your recordings.",
"Could not save this practice focus.",
"No report was saved for this interview.",
"Sign in to see your recordings.",
"No test run before this point.",
Expand All @@ -540,6 +546,7 @@ test("the response window panel says what the number is worth, in words a test c
// page calls and the separators it joins values with. The events request
// is here in the shape it is sent, which is the other half of the done
// condition for this feature.
"/",
"/api/recordings",
"/api/recordings/",
"/api/recordings//events?avatar=history",
Expand Down Expand Up @@ -569,6 +576,13 @@ test("the response window panel says what the number is worth, in words a test c
// person reading the test can see it.
});

test("replay practice actions carry the selected improvement to the lobby", () => {
const loadReport = functionBody(script, "loadReport");
assert.match(loadReport, /data-practice-focus/);
assert.match(loadReport, /storeSelectedPracticeFocus/);
assert.match(loadReport, /window\.location\.href = "\/"/);
});

test("the replay page asks for the avatar history and renders it beside the moments", () => {
// The done condition is the request, not what the page then draws with it.
// What it draws is asserted in `tests/browser/replay-render.test.js`, which
Expand Down
Loading
Loading