Uh oh!
There was an error while loading. Please reload this page.
[auth] Scenarios for scope selection - #36
Conversation
| const scenario = getScenario(scenarioName)!; | ||
| console.log(`Starting scenario: ${scenarioName}`); | ||
| console.error(`Starting scenario: ${scenarioName}`); |
There was a problem hiding this comment.
this is to make it possible to pipe verbose output to jq or jless
| server.setRequestHandler( | ||
| CallToolRequestSchema, | ||
| async (request): Promise<CallToolResult> => { | ||
| if (request.params.name === 'test-tool') { |
There was a problem hiding this comment.
this is so we can test an "auth only required on a tool call" scenario. It's via setRequestHandler because we're in the low level interface
| private expectedScopes: string[] = [] | ||
| ) {} | ||
| registerToken(token: string, scopes: string[]) { |
There was a problem hiding this comment.
we're keeping track of the scopes here as a convenience. we could also fetch them from the AS, and might want to switch to doing that in the future.
| InlineClientRunner | ||
| } from './test_helpers/testClient.js'; | ||
| import path from 'path'; | ||
| import { runClient as goodClient } from '../../../../examples/clients/typescript/auth-test.js'; |
There was a problem hiding this comment.
I refactored the client tests to allow running them "inline" rather than spawning a subshell.
A subshell took a good 600ms, so the full auth suite took about 13s. With this, it's about 200ms for the full suite, so vitest watch is like a live refresh.
| 'examples/clients/typescript/auth-test.ts' | ||
| ); | ||
| beforeAll(() => { | ||
| setLogLevel('error'); |
There was a problem hiding this comment.
without this, we get a bunch of annoying stdout lines littering the test output
| // Verify that only the expected checks failed | ||
| const failures = nonInfoChecks.filter((c) => c.status === 'FAILURE'); | ||
| const failures = nonInfoChecks.filter( | ||
| (c) => c.status === 'FAILURE' || c.status === 'WARNING' |
There was a problem hiding this comment.
I'm counting warning as failure in this test because we want the examples to not have warnings
pcarleton
commented
Nov 18, 2025
This is ready for review, but needs a typsecript change for tests to pass, will push that shortly. |
pcarleton
commented
Nov 18, 2025
typescript-sdk PR to have this pass: I think I'll comment out the broken ones for now since we'll need to wait for typescript release i think to get the fix. (unless we want to patch release?) |
pcarleton
commented
Nov 18, 2025
okay I skipped tests, we can unskip them later |
| * Broken client that only responds to 401, not 403. | ||
| * BUG: Ignores 403 responses which are used for step-up auth. | ||
| */ | ||
| export async function runClient(serverUrl: string): Promise<void> { |
There was a problem hiding this comment.
more of a style nit: all these "broken" examples look very similar (identical?) except for the specific way in which they're "broken".
Could consider having a single implementation and having the specific way it's actually broken be a param to createBrokenHandler, a higher level function that creates the handle401broken.
type ClientBehavior =
| { type: 'well-behaved' } // or just omit for default
| { type: 'ignore-resource-metadata' }
| { type: 'ignore-scope' }
| { type: 'partial-scopes'; scope: string }
| { type: 'ignore-403' };
Could make this less verbose.
There was a problem hiding this comment.
Potentially the well-behaved client could also share the same implementation.
There was a problem hiding this comment.
Maybe we prefer explicitness over conciseness though for conformance tests, so definitely just a thought 🤔
There was a problem hiding this comment.
agreed on the repetitiveness, i've been resisting the urge to de-dupe too aggressively in order to let abstractions fall out from a few more iterations on tests before committing to one.
| "@types/express": "^5.0.3", | ||
| "@types/node": "^22.10.2", | ||
| "@typescript/native-preview": "^7.0.0-dev.20251030.1", | ||
| "cors": "^2.8.5", |
There was a problem hiding this comment.
Is adding cors here intentional for this PR? Doesn't seem like we're importing this anywhere?
There was a problem hiding this comment.
when I ran npm link this started complaining, i think because the everything server uses it (even if we have a package.json in the server client repo).
I'd prefer to keep it to let the link flow be smoother even if it's a little odd.
Uh oh!
There was an error while loading. Please reload this page.
Motivation and Context
fixes#32
How Has This Been Tested?
Breaking Changes
tests and negative tests
Types of changes
Checklist
Additional context
I did a few related refactors: