Skip to content

Commit b323a21

Browse files
KzoepsKzoeps
committed
fix: address PR review feedback
1 parent 265df25 commit b323a21

4 files changed

Lines changed: 94 additions & 129 deletions

File tree

‎e2e/step-definitions/consent.steps.ts‎

Lines changed: 26 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import { Given, Then, When } from '@cucumber/cucumber'
1111
import { expect } from '@playwright/test'
12+
import type { Locator, Page } from '@playwright/test'
1213
import type { EpdsWorld } from '../support/world.js'
1314
import { testEnv } from '../support/env.js'
1415
import {
@@ -65,6 +66,29 @@ function escapeRegex(value: string): string {
6566
return value.replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`)
6667
}
6768

69+
async function openIdentityTooltip(page: Page): Promise<Locator> {
70+
const tooltipControl = page
71+
.getByRole('main')
72+
.getByRole('button', { name: 'Identity information' })
73+
74+
await expect(tooltipControl).toHaveAttribute('type', 'button')
75+
await expect(tooltipControl).toHaveAttribute('aria-expanded', 'false')
76+
const describedBy = await tooltipControl.getAttribute('aria-describedby')
77+
expect(describedBy?.trim()).toBeTruthy()
78+
if (!describedBy?.trim()) {
79+
throw new Error('Expected aria-describedby to reference a tooltip')
80+
}
81+
const [tooltipId] = describedBy.trim().split(/\s+/)
82+
83+
await tooltipControl.click()
84+
await expect(tooltipControl).toHaveAttribute('aria-expanded', 'true')
85+
86+
const tooltip = page.locator(`#${tooltipId}`)
87+
await expect(tooltip).toHaveAttribute('role', 'tooltip')
88+
await expect(tooltip).toBeVisible()
89+
return tooltip
90+
}
91+
6892
Then('a consent screen is displayed', async function (this: EpdsWorld) {
6993
const page = getPage(this)
7094

@@ -165,25 +189,7 @@ Then(
165189
async function (this: EpdsWorld) {
166190
const page = getPage(this)
167191
const publicHandle = formatPublicHandle(requireScenarioHandle(this))
168-
const tooltipControl = page
169-
.getByRole('main')
170-
.getByRole('button', { name: 'Identity information' })
171-
172-
await expect(tooltipControl).toHaveAttribute('type', 'button')
173-
await expect(tooltipControl).toHaveAttribute('aria-expanded', 'false')
174-
const describedBy = await tooltipControl.getAttribute('aria-describedby')
175-
expect(describedBy?.trim()).toBeTruthy()
176-
if (!describedBy?.trim()) {
177-
throw new Error('Expected aria-describedby to reference a tooltip')
178-
}
179-
const [tooltipId] = describedBy.trim().split(/\s+/)
180-
181-
await tooltipControl.click()
182-
await expect(tooltipControl).toHaveAttribute('aria-expanded', 'true')
183-
184-
const tooltip = page.locator(`#${tooltipId}`)
185-
await expect(tooltip).toHaveAttribute('role', 'tooltip')
186-
await expect(tooltip).toBeVisible()
192+
const tooltip = await openIdentityTooltip(page)
187193
await expect(tooltip).toContainText('Public AT Protocol handle:')
188194
await expect(tooltip).toContainText(publicHandle)
189195
},
@@ -194,25 +200,7 @@ Then(
194200
async function (this: EpdsWorld) {
195201
const page = getPage(this)
196202
const scenarioEmail = requireScenarioEmail(this)
197-
const tooltipControl = page
198-
.getByRole('main')
199-
.getByRole('button', { name: 'Identity information' })
200-
201-
await expect(tooltipControl).toHaveAttribute('type', 'button')
202-
await expect(tooltipControl).toHaveAttribute('aria-expanded', 'false')
203-
const describedBy = await tooltipControl.getAttribute('aria-describedby')
204-
expect(describedBy?.trim()).toBeTruthy()
205-
if (!describedBy?.trim()) {
206-
throw new Error('Expected aria-describedby to reference a tooltip')
207-
}
208-
const [tooltipId] = describedBy.trim().split(/\s+/)
209-
210-
await tooltipControl.click()
211-
await expect(tooltipControl).toHaveAttribute('aria-expanded', 'true')
212-
213-
const tooltip = page.locator(`#${tooltipId}`)
214-
await expect(tooltip).toHaveAttribute('role', 'tooltip')
215-
await expect(tooltip).toBeVisible()
203+
const tooltip = await openIdentityTooltip(page)
216204
await expect(tooltip).toContainText('This handle is associated with')
217205
await expect(tooltip).toContainText(scenarioEmail)
218206
},

‎packages/auth-service/src/__tests__/callback-handle-mode.test.ts‎

Lines changed: 23 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,23 @@ function verifySignedCallbackUrl(url: URL): boolean {
136136
)
137137
}
138138

139+
async function fetchCompleteRedirect(handleMode: HandleMode | null) {
140+
const app = await startApp(makeCtx(handleMode), makeAuth())
141+
try {
142+
const res = await fetch(`${app.baseUrl}/auth/complete`, {
143+
redirect: 'manual',
144+
headers: { cookie: `${AUTH_FLOW_COOKIE}=flow-1` },
145+
})
146+
147+
expect(res.status).toBe(303)
148+
const url = parseRedirect(res)
149+
expect(url.pathname).toBe('/oauth/epds-callback')
150+
return url
151+
} finally {
152+
await app.close()
153+
}
154+
}
155+
139156
describe('auth-service epds-callback handle mode threading', () => {
140157
let priorEnv: { pdsInternalUrl?: string; internalSecret?: string }
141158

@@ -182,42 +199,16 @@ describe('auth-service epds-callback handle mode threading', () => {
182199

183200
it('includes the stored canonical handle mode for random-mode new users', async () => {
184201
mocks.getDidByEmail.mockResolvedValue(null)
185-
const app = await startApp(makeCtx('random'), makeAuth())
186-
try {
187-
const res = await fetch(`${app.baseUrl}/auth/complete`, {
188-
redirect: 'manual',
189-
headers: { cookie: `${AUTH_FLOW_COOKIE}=flow-1` },
190-
})
191-
192-
expect(res.status).toBe(303)
193-
const url = parseRedirect(res)
194-
expect(url.pathname).toBe('/oauth/epds-callback')
195-
expect(url.searchParams.get('epds_handle_mode')).toBe('random')
196-
expect(url.searchParams.has('handle')).toBe(false)
197-
expect(verifySignedCallbackUrl(url)).toBe(true)
198-
} finally {
199-
await app.close()
200-
}
202+
const url = await fetchCompleteRedirect('random')
203+
expect(url.searchParams.get('epds_handle_mode')).toBe('random')
204+
expect(url.searchParams.has('handle')).toBe(false)
205+
expect(verifySignedCallbackUrl(url)).toBe(true)
201206
})
202207

203208
it('includes the stored canonical handle mode for existing users', async () => {
204209
mocks.getDidByEmail.mockResolvedValue(randomBytes(16).toString('hex'))
205-
const app = await startApp(makeCtx('picker-with-random'), makeAuth())
206-
try {
207-
const res = await fetch(`${app.baseUrl}/auth/complete`, {
208-
redirect: 'manual',
209-
headers: { cookie: `${AUTH_FLOW_COOKIE}=flow-1` },
210-
})
211-
212-
expect(res.status).toBe(303)
213-
const url = parseRedirect(res)
214-
expect(url.pathname).toBe('/oauth/epds-callback')
215-
expect(url.searchParams.get('epds_handle_mode')).toBe(
216-
'picker-with-random',
217-
)
218-
} finally {
219-
await app.close()
220-
}
210+
const url = await fetchCompleteRedirect('picker-with-random')
211+
expect(url.searchParams.get('epds_handle_mode')).toBe('picker-with-random')
221212
})
222213

223214
it('includes the stored canonical handle mode for chosen-handle callbacks', async () => {

‎packages/auth-service/src/routes/choose-handle.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ export function createChooseHandleRouter(
404404
approved: '1',
405405
new_account: '1',
406406
handle: normalizedLocal,
407-
...(flow.handleMode ? { epds_handle_mode: flow.handleMode } : {}),
407+
epds_handle_mode: flow.handleMode ?? '',
408408
}
409409
if (flow.clientId) callbackParams.client_id = flow.clientId
410410
const { sig, ts } = signCallback(

‎packages/shared/src/__tests__/crypto.test.ts‎

Lines changed: 44 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,36 @@ describe('generateRandomHandle', () => {
131131
})
132132
})
133133

134+
function makeCallbackParams(overrides: Partial<CallbackParams> = {}) {
135+
return {
136+
request_uri: 'urn:ietf:params:oauth:request_uri:test',
137+
email: 'alice@example.com',
138+
approved: '1',
139+
new_account: '1',
140+
...overrides,
141+
} satisfies CallbackParams
142+
}
143+
144+
function expectSignedCallbackToVerify(callbackParams: CallbackParams) {
145+
const secret = 'test-secret'
146+
const { sig, ts } = signCallback(callbackParams, secret)
147+
expect(verifyCallback(callbackParams, ts, sig, secret)).toBe(true)
148+
}
149+
150+
function expectTamperedHandleModeToFail(callbackParams: CallbackParams) {
151+
const secret = 'test-secret'
152+
const { sig, ts } = signCallback(callbackParams, secret)
153+
expect(verifyCallback(callbackParams, ts, sig, secret)).toBe(true)
154+
expect(
155+
verifyCallback(
156+
{ ...callbackParams, epds_handle_mode: 'random' },
157+
ts,
158+
sig,
159+
secret,
160+
),
161+
).toBe(false)
162+
}
163+
134164
describe('signCallback / verifyCallback', () => {
135165
const secret = 'test-secret-32bytes-padding-here'
136166
const params: CallbackParams = {
@@ -190,35 +220,22 @@ describe('signCallback / verifyCallback', () => {
190220
expect(verifyCallback(params, staleTs, staleSig, secret)).toBe(false)
191221
})
192222

193-
it('signs and verifies callback with epds_handle_mode param', () => {
194-
const secret = 'test-secret'
195-
const params: CallbackParams = {
196-
request_uri: 'urn:ietf:params:oauth:request_uri:test',
197-
email: 'alice@example.com',
198-
approved: '1',
199-
new_account: '1',
200-
handle: 'alice',
201-
epds_handle_mode: 'picker',
202-
}
203-
const { sig, ts } = signCallback(params, secret)
204-
expect(verifyCallback(params, ts, sig, secret)).toBe(true)
205-
})
223+
it.each([
224+
{ name: 'with handle', handle: 'alice' },
225+
{ name: 'without handle', handle: undefined },
226+
])(
227+
'signs and verifies callback with epds_handle_mode $name',
228+
({ handle }) => {
229+
expectSignedCallbackToVerify(
230+
makeCallbackParams({ handle, epds_handle_mode: 'picker' }),
231+
)
232+
},
233+
)
206234

207235
it('rejects tampered epds_handle_mode', () => {
208-
const secret = 'test-secret'
209-
const params: CallbackParams = {
210-
request_uri: 'urn:ietf:params:oauth:request_uri:test',
211-
email: 'alice@example.com',
212-
approved: '1',
213-
new_account: '1',
214-
epds_handle_mode: 'picker',
215-
}
216-
const { sig, ts } = signCallback(params, secret)
217-
const tamperedParams: CallbackParams = {
218-
...params,
219-
epds_handle_mode: 'random',
220-
}
221-
expect(verifyCallback(tamperedParams, ts, sig, secret)).toBe(false)
236+
expectTamperedHandleModeToFail(
237+
makeCallbackParams({ epds_handle_mode: 'picker' }),
238+
)
222239
})
223240

224241
it('rejects future timestamp', async () => {
@@ -332,37 +349,6 @@ describe('signCallback / verifyCallback with handle', () => {
332349
}
333350
expect(verifyCallback(tamperedParams, ts, sig, secret)).toBe(false)
334351
})
335-
336-
it('signs and verifies callback with epds_handle_mode param', () => {
337-
const secret = 'test-secret'
338-
const params: CallbackParams = {
339-
request_uri: 'urn:ietf:params:oauth:request_uri:test',
340-
email: 'alice@example.com',
341-
approved: '1',
342-
new_account: '1',
343-
handle: 'alice',
344-
epds_handle_mode: 'picker',
345-
}
346-
const { sig, ts } = signCallback(params, secret)
347-
expect(verifyCallback(params, ts, sig, secret)).toBe(true)
348-
})
349-
350-
it('rejects tampered epds_handle_mode', () => {
351-
const secret = 'test-secret'
352-
const params: CallbackParams = {
353-
request_uri: 'urn:ietf:params:oauth:request_uri:test',
354-
email: 'alice@example.com',
355-
approved: '1',
356-
new_account: '1',
357-
epds_handle_mode: 'picker',
358-
}
359-
const { sig, ts } = signCallback(params, secret)
360-
const tamperedParams: CallbackParams = {
361-
...params,
362-
epds_handle_mode: 'random',
363-
}
364-
expect(verifyCallback(tamperedParams, ts, sig, secret)).toBe(false)
365-
})
366352
})
367353

368354
describe('signCallback / verifyCallback with client_id', () => {

0 commit comments

Comments
 (0)