Skip to content

Commit 6573c44

Browse files
committed
fix(auth-ui): device "Code expired", OS column, datumctl login callback
Three staging bugs surfaced during #755 testing: - device: redirect authorize/deny to a terminal /device/complete route so React Router 7 post-action revalidation can't re-resolve the already-consumed device authorization and render a false "Code expired" over a successful authorize. - sessions: thread the active ceremony requestId through the /accounts picker (loader + switch/remove forms + switchAccount/resolveNextPath) so selecting an account during a datumctl OIDC login resumes /authorize -> createCallback instead of hanging on "Waiting for authentication callback". - user-agent: emit the OS token as "Mac OS" (not "macOS") so the cloud-portal session gateway parses the OS column from the createSession description. datum-cloud/enhancements#755
1 parent f1d69b3 commit 6573c44

18 files changed

Lines changed: 466 additions & 69 deletions

app/modules/i18n/locales/en.po

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ msgstr "{0, plural, one {# attempt remaining.} other {# attempts remaining.}}"
2525
msgid "{0}"
2626
msgstr "{0}"
2727

28-
#: app/routes/device/authorize.tsx:109
28+
#: app/routes/device/authorize.tsx:104
2929
msgid "<0>{appName}</0> is requesting access."
3030
msgstr "<0>{appName}</0> is requesting access."
3131

@@ -37,15 +37,15 @@ msgstr "A new code has been sent to your email."
3737
msgid "Activate your device"
3838
msgstr "Activate your device"
3939

40-
#: app/routes/accounts.tsx:79
40+
#: app/routes/accounts.tsx:86
4141
msgid "Add an account"
4242
msgstr "Add an account"
4343

4444
#: app/routes/setup/mfa.tsx:160
4545
msgid "Add an extra layer of security to your account by setting up a second factor."
4646
msgstr "Add an extra layer of security to your account by setting up a second factor."
4747

48-
#: app/routes/accounts.tsx:149
48+
#: app/routes/accounts.tsx:161
4949
msgid "Add another account"
5050
msgstr "Add another account"
5151

@@ -92,7 +92,7 @@ msgstr "Authenticator code"
9292
msgid "Authenticator QR code"
9393
msgstr "Authenticator QR code"
9494

95-
#: app/routes/device/authorize.tsx:96
95+
#: app/routes/device/complete.tsx:33
9696
#: app/routes/signed-in.tsx:62
9797
msgid "Authorization complete"
9898
msgstr "Authorization complete"
@@ -101,11 +101,11 @@ msgstr "Authorization complete"
101101
msgid "Authorization failed"
102102
msgstr "Authorization failed"
103103

104-
#: app/routes/device/authorize.tsx:130
104+
#: app/routes/device/authorize.tsx:125
105105
msgid "Authorize"
106106
msgstr "Authorize"
107107

108-
#: app/routes/device/authorize.tsx:105
108+
#: app/routes/device/authorize.tsx:100
109109
msgid "Authorize device"
110110
msgstr "Authorize device"
111111

@@ -145,7 +145,7 @@ msgstr "Check your email"
145145
msgid "Choose a new password"
146146
msgstr "Choose a new password"
147147

148-
#: app/routes/accounts.tsx:66
148+
#: app/routes/accounts.tsx:73
149149
msgid "Choose an account"
150150
msgstr "Choose an account"
151151

@@ -209,14 +209,18 @@ msgstr "Create a new account"
209209
msgid "Create account"
210210
msgstr "Create account"
211211

212-
#: app/routes/device/authorize.tsx:139
212+
#: app/routes/device/authorize.tsx:134
213213
msgid "Deny"
214214
msgstr "Deny"
215215

216216
#: app/routes/device/index.tsx:69
217217
msgid "Device code"
218218
msgstr "Device code"
219219

220+
#: app/routes/device/complete.tsx:41
221+
msgid "Device denied"
222+
msgstr "Device denied"
223+
220224
#: app/routes/login/index.tsx:206
221225
#: app/routes/login/index.tsx:347
222226
#: app/routes/signup/index.tsx:266
@@ -338,7 +342,7 @@ msgstr "Linked accounts"
338342
msgid "Manual setup key"
339343
msgstr "Manual setup key"
340344

341-
#: app/routes/accounts.tsx:118
345+
#: app/routes/accounts.tsx:128
342346
msgid "Needs re-authentication"
343347
msgstr "Needs re-authentication"
344348

@@ -355,7 +359,7 @@ msgstr "No account was found and sign-up is not available."
355359
msgid "No sign-in method is available for this account."
356360
msgstr "No sign-in method is available for this account."
357361

358-
#: app/routes/accounts.tsx:74
362+
#: app/routes/accounts.tsx:81
359363
msgid "No signed-in accounts."
360364
msgstr "No signed-in accounts."
361365

@@ -468,7 +472,7 @@ msgstr "Scan the QR code below with your authenticator app, then enter the 6-dig
468472
msgid "Security key"
469473
msgstr "Security key"
470474

471-
#: app/routes/accounts.tsx:67
475+
#: app/routes/accounts.tsx:74
472476
msgid "Select an account to continue or add a new one."
473477
msgstr "Select an account to continue or add a new one."
474478

@@ -484,7 +488,7 @@ msgstr "Send reset link"
484488
msgid "Service temporarily unavailable. Please try again."
485489
msgstr "Service temporarily unavailable. Please try again."
486490

487-
#: app/routes/accounts.tsx:116
491+
#: app/routes/accounts.tsx:126
488492
msgid "Session active"
489493
msgstr "Session active"
490494

@@ -627,6 +631,10 @@ msgstr "The sign-in link was incomplete or expired."
627631
msgid "This device code is invalid or has expired."
628632
msgstr "This device code is invalid or has expired."
629633

634+
#: app/routes/device/complete.tsx:42
635+
msgid "This device was not granted access. You may close this window."
636+
msgstr "This device was not granted access. You may close this window."
637+
630638
#: app/routes/sso/provider/error.tsx:22
631639
msgid "This directory account can't be linked here yet. Sign in with your password instead."
632640
msgstr "This directory account can't be linked here yet. Sign in with your password instead."
@@ -767,7 +775,7 @@ msgstr "You can now sign in using <0>{loginName}</0>."
767775
msgid "You don't have permission to do that."
768776
msgstr "You don't have permission to do that."
769777

770-
#: app/routes/device/authorize.tsx:97
778+
#: app/routes/device/complete.tsx:34
771779
msgid "You may return to your device."
772780
msgstr "You may return to your device."
773781

app/resources/device/__tests__/device.service.test.ts

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ describe('loadDeviceConsent (/device/authorize loader)', () => {
159159
// ── /device/authorize action → resolveDeviceDecision (from authorize.test.ts) ──────
160160

161161
describe('resolveDeviceDecision (/device/authorize action)', () => {
162-
it('authorize with a session → device is authorized; outcome is not a redirect', async () => {
162+
it('authorize with a session → device is authorized; redirects to the terminal /device/complete', async () => {
163163
const req = await requestWithSession('http://localhost/id/device/authorize');
164164
const outcome = await resolveDeviceDecision(
165165
fakeProvider(),
@@ -173,14 +173,16 @@ describe('resolveDeviceDecision (/device/authorize action)', () => {
173173
})
174174
);
175175

176-
// Must NOT be a 302 redirect
177-
expect(outcome.kind).toBe('done');
178-
const res = decisionOutcomeToResponse(outcome);
179-
expect(res instanceof Response && (res as Response).status === 302).toBe(false);
176+
// Success is a 302 redirect to the TERMINAL completion screen carrying the decision —
177+
// authorizeDevice consumed the device-auth request, so re-rendering this route's loader
178+
// would NOT_FOUND; /device/complete has no getDeviceAuth loader.
179+
expect(outcome.kind).toBe('redirect');
180+
const location = outcome.kind === 'redirect' ? outcome.location : '';
181+
expect(location).toBe('/device/complete?decision=authorize');
180182

181-
// Response should carry done: 'authorize'
182-
const asData = res as { data?: { done?: string } };
183-
expect(asData.data?.done).toBe('authorize');
183+
const res = decisionOutcomeToResponse(outcome) as Response;
184+
expect(res.status).toBe(302);
185+
expect(res.headers.get('location')).toBe('/device/complete?decision=authorize');
184186

185187
// The device should now be marked as authorized in the fake singleton.
186188
// dev-authorize is dedicated to this test — no other test mutates it.
@@ -211,22 +213,22 @@ describe('resolveDeviceDecision (/device/authorize action)', () => {
211213
expect(fake.isDeviceAuthorized('dev-1')).toBe(false);
212214
});
213215

214-
it('deny → device stays unauthorized; outcome is not a redirect', async () => {
216+
it('deny → device stays unauthorized; redirects to the terminal /device/complete', async () => {
215217
const req = await requestWithSession('http://localhost/id/device/authorize');
216218
const outcome = await resolveDeviceDecision(
217219
fakeProvider(),
218220
req,
219221
formData({ decision: 'deny', deviceAuthId: 'dev-deny', requestId: 'device_dev-deny' })
220222
);
221223

222-
// Must NOT be a 302 redirect
223-
expect(outcome.kind).toBe('done');
224-
const res = decisionOutcomeToResponse(outcome);
225-
expect(res instanceof Response && (res as Response).status === 302).toBe(false);
224+
// Deny also lands on the terminal completion screen, carrying decision=deny.
225+
expect(outcome.kind).toBe('redirect');
226+
const location = outcome.kind === 'redirect' ? outcome.location : '';
227+
expect(location).toBe('/device/complete?decision=deny');
226228

227-
// Response should carry done: 'deny'
228-
const asData = res as { data?: { done?: string } };
229-
expect(asData.data?.done).toBe('deny');
229+
const res = decisionOutcomeToResponse(outcome) as Response;
230+
expect(res.status).toBe(302);
231+
expect(res.headers.get('location')).toBe('/device/complete?decision=deny');
230232

231233
// The device should NOT be authorized
232234
expect(fakeProvider().isDeviceAuthorized('dev-deny')).toBe(false);

app/resources/device/device.service.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -199,12 +199,16 @@ export function deviceConsentErrorToResponse(error: DeviceConsentError) {
199199

200200
/**
201201
* Typed outcome of the /device/authorize action. The route turns it into a Response via
202-
* `decisionOutcomeToResponse`: a redirect to /login (unauthenticated authorize), a `data()`
203-
* success carrying the decision, or a `data()` error with the appropriate status.
202+
* `decisionOutcomeToResponse`: a redirect to /login (unauthenticated authorize), a redirect to
203+
* the terminal /device/complete screen (decision applied), or a `data()` error.
204+
*
205+
* The success path is a REDIRECT, not a `data()` payload: authorizeDevice LEGITIMATELY CONSUMES
206+
* the device-auth request, so RR7's post-action auto-revalidation of this route's loader
207+
* (getDeviceAuth) would NOT_FOUND and render a false "Code expired". Sending the decision to the
208+
* terminal /device/complete route (which has NO getDeviceAuth loader) sidesteps that revalidation.
204209
*/
205210
export type DeviceDecisionOutcome =
206211
| { kind: 'redirect'; location: string }
207-
| { kind: 'done'; decision: 'authorize' | 'deny' }
208212
| { kind: 'error'; error: 'invalid_input'; status: 400 }
209213
| { kind: 'error'; error: 'provider_error'; status: 502 };
210214

@@ -267,16 +271,17 @@ export async function resolveDeviceDecision(
267271
actor: entry?.loginName ? hashActor(entry.loginName) : undefined,
268272
});
269273

270-
return { kind: 'done', decision };
274+
// Redirect to the terminal completion screen carrying the decision. authorizeDevice has
275+
// consumed the device-auth request, so re-rendering THIS route (whose loader re-resolves
276+
// getDeviceAuth) would NOT_FOUND; /device/complete carries the decision and never re-resolves.
277+
return { kind: 'redirect', location: paths.device.complete({ decision }) };
271278
}
272279

273280
/** Turn a DeviceDecisionOutcome into the Response the /device/authorize route returns. */
274281
export function decisionOutcomeToResponse(outcome: DeviceDecisionOutcome) {
275282
switch (outcome.kind) {
276283
case 'redirect':
277284
return redirect(outcome.location);
278-
case 'done':
279-
return data({ done: outcome.decision });
280285
case 'error':
281286
return data({ error: outcome.error }, { status: outcome.status });
282287
}

app/resources/session/__tests__/session.service.switch.test.ts

Lines changed: 135 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
import { FakeAuthProvider } from '@/modules/auth/providers/fake/fake-provider';
1818
import { sessionsCookie } from '@/modules/auth/session/cookie';
1919
import type { User } from '@/modules/auth/types';
20-
import { switchAccount } from '@/resources/session';
20+
import { removeAccount, switchAccount } from '@/resources/session';
2121
import { describe, it, expect } from 'vitest';
2222

2323
const BASE_URL = 'http://localhost/id/accounts';
@@ -47,10 +47,19 @@ function makeRequest(cookie: string): Request {
4747
return new Request(BASE_URL, { headers: { cookie: cookie.split(';')[0] } });
4848
}
4949

50-
function switchForm(sessionId: string): FormData {
50+
function switchForm(sessionId: string, requestId?: string): FormData {
5151
const form = new FormData();
5252
form.set('intent', 'switch');
5353
form.set('sessionId', sessionId);
54+
if (requestId !== undefined) form.set('requestId', requestId);
55+
return form;
56+
}
57+
58+
function removeForm(sessionId: string, requestId?: string): FormData {
59+
const form = new FormData();
60+
form.set('intent', 'remove');
61+
form.set('sessionId', sessionId);
62+
if (requestId !== undefined) form.set('requestId', requestId);
5463
return form;
5564
}
5665

@@ -111,3 +120,127 @@ describe('switchAccount — 755-M10 MFA-setup nudge suppression', () => {
111120
expect(outcome.location).toContain('checkAfter=true');
112121
});
113122
});
123+
124+
describe('switchAccount — current ceremony requestId threading (datumctl OIDC hang)', () => {
125+
// A no-MFA user whose org seeds a non-zero skip window resolves to /signed-in (the same setup
126+
// as the nudge-suppression spec). The cookie entry carries NO requestId, so any requestId on
127+
// the destination must come from the CURRENT ceremony form field, not the stale cookie one.
128+
const user: User = { id: 'u1', loginName: 'alice@acme.test', displayName: 'Alice' };
129+
130+
function makeProvider(): FakeAuthProvider {
131+
const provider = new FakeAuthProvider({
132+
users: [user],
133+
authMethods: { u1: [] },
134+
settingsByOrg: { 'nudge-org': { mfaInitSkipLifetimeMs: 10_000 } },
135+
});
136+
provider.seedLiveSession({ id: 's1', token: 'tok-s1', user });
137+
return provider;
138+
}
139+
140+
async function cookie(): Promise<string> {
141+
return mintCookie({
142+
id: 's1',
143+
token: 'tok-s1',
144+
loginName: 'alice@acme.test',
145+
organization: 'nudge-org',
146+
});
147+
}
148+
149+
it('threads the CURRENT ceremony requestId onto the resolved /signed-in destination', async () => {
150+
const provider = makeProvider();
151+
const reqId = 'oidc_V3-current';
152+
153+
const outcome = await switchAccount(
154+
provider,
155+
makeRequest(await cookie()),
156+
switchForm('s1', reqId)
157+
);
158+
159+
expect(outcome.kind).toBe('redirect');
160+
if (outcome.kind !== 'redirect') throw new Error('expected redirect');
161+
expect(outcome.location).toMatch(/^\/signed-in(\?|$)/);
162+
expect(outcome.location).toContain(`requestId=${encodeURIComponent(reqId)}`);
163+
});
164+
165+
it('omits requestId when none is provided (standalone switch fallback unchanged)', async () => {
166+
const provider = makeProvider();
167+
168+
const outcome = await switchAccount(provider, makeRequest(await cookie()), switchForm('s1'));
169+
170+
expect(outcome.kind).toBe('redirect');
171+
if (outcome.kind !== 'redirect') throw new Error('expected redirect');
172+
expect(outcome.location).toMatch(/^\/signed-in(\?|$)/);
173+
expect(outcome.location).not.toContain('requestId=');
174+
});
175+
176+
it('ignores a non-allowlisted requestId (treated as no ceremony)', async () => {
177+
const provider = makeProvider();
178+
179+
const outcome = await switchAccount(
180+
provider,
181+
makeRequest(await cookie()),
182+
switchForm('s1', 'evil_https://attacker.example')
183+
);
184+
185+
expect(outcome.kind).toBe('redirect');
186+
if (outcome.kind !== 'redirect') throw new Error('expected redirect');
187+
expect(outcome.location).toMatch(/^\/signed-in(\?|$)/);
188+
expect(outcome.location).not.toContain('requestId=');
189+
});
190+
});
191+
192+
describe('removeAccount — ceremony requestId threading', () => {
193+
// removeAccount deletes the entry provider-side (best-effort, seeded so it succeeds) then
194+
// redirects back to /accounts. The CURRENT ceremony id must ride that redirect so removing an
195+
// account mid-ceremony keeps the flow — but only an allowlisted value is reflected (no injection).
196+
const user: User = { id: 'u1', loginName: 'alice@acme.test', displayName: 'Alice' };
197+
198+
function makeProvider(): FakeAuthProvider {
199+
const provider = new FakeAuthProvider({ users: [user] });
200+
provider.seedLiveSession({ id: 's1', token: 'tok-s1', user });
201+
return provider;
202+
}
203+
204+
async function cookie(): Promise<string> {
205+
return mintCookie({ id: 's1', token: 'tok-s1', loginName: 'alice@acme.test' });
206+
}
207+
208+
it('carries an allowlisted requestId onto the /accounts redirect', async () => {
209+
const provider = makeProvider();
210+
const reqId = 'oidc_V3-current';
211+
212+
const outcome = await removeAccount(
213+
provider,
214+
makeRequest(await cookie()),
215+
removeForm('s1', reqId)
216+
);
217+
218+
expect(outcome.kind).toBe('redirect');
219+
if (outcome.kind !== 'redirect') throw new Error('expected redirect');
220+
expect(outcome.location).toBe(`/accounts?requestId=${encodeURIComponent(reqId)}`);
221+
});
222+
223+
it('redirects to bare /accounts when no requestId is provided', async () => {
224+
const provider = makeProvider();
225+
226+
const outcome = await removeAccount(provider, makeRequest(await cookie()), removeForm('s1'));
227+
228+
expect(outcome.kind).toBe('redirect');
229+
if (outcome.kind !== 'redirect') throw new Error('expected redirect');
230+
expect(outcome.location).toBe('/accounts');
231+
});
232+
233+
it('drops a non-allowlisted requestId (no injection onto /accounts)', async () => {
234+
const provider = makeProvider();
235+
236+
const outcome = await removeAccount(
237+
provider,
238+
makeRequest(await cookie()),
239+
removeForm('s1', 'evil_x')
240+
);
241+
242+
expect(outcome.kind).toBe('redirect');
243+
if (outcome.kind !== 'redirect') throw new Error('expected redirect');
244+
expect(outcome.location).toBe('/accounts');
245+
});
246+
});

0 commit comments

Comments
 (0)