refactor(core,express): give the auth API contract values one home - #129
Merged
Conversation
The external-delivery header and the service-token identity were written out at each call site: the x-seamless-auth-delivery-mode header in three core handlers, the fixed issuer and audience in three places across the adapter, and the dev-main key id fallback in three more. Each is defined by seamless-auth-api, so changing one is coordinated cross-repo work and finding every copy was part of the job. Move them to core as named constants, add buildExternalDeliveryAuthorization for the external-delivery token, and point every call site at them. No behavior change: the minted tokens carry the same header and claims, confirmed by decoding them. A new test asserts each contract value literally, so a change breaks a named test rather than surfacing as an upstream rejection at runtime. Refs #72
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR 4 of #72.
Problem
Values that
seamless-auth-apidefines were written out at each call site:x-seamless-auth-delivery-mode: "external"issuer: "seamless-portal-api"/audience: "seamless-auth""dev-main"key id fallbackChanging any of them is coordinated cross-repo work, and finding every copy was part of the job.
Change
New
packages/core/src/apiContract.tsexportingAUTH_DELIVERY_MODE_HEADER,EXTERNAL_DELIVERY_MODE,EXTERNAL_DELIVERY_HEADERS,SERVICE_TOKEN_ISSUER,SERVICE_TOKEN_AUDIENCE,DEV_JWKS_KID,EXTERNAL_DELIVERY_TOKEN_SUBJECT, andbuildExternalDeliveryAuthorization.Every call site now reads from it.
buildInternalServiceAuthorizationin the adapter becomes a one-line delegation, keeping its name so nothing that imports it changes.The reason the service-token issuer and audience are fixed, rather than the adopter's configured audience, is now stated where the constants are defined instead of in a comment at one of the three call sites.
No behavior change
The minted tokens are the same. Decoded from the built adapter:
{"header":{"alg":"HS256","kid":"test-main"}, "iss":"seamless-portal-api","aud":"seamless-auth", "sub":"seamless-auth-external-delivery"}A grep for the old literals across both packages now returns nothing outside
apiContract.ts.New tests
packages/core/tests/apiContract.test.jsasserts each contract value literally. That is the point: these are the API's values, so a change should break a named test rather than surface as an upstream rejection at runtime. It also covers the token's header and claims, thedev-mainfallback, verification against the service secret, that an adopter-supplied audience cannot override the service audience, and that the shared header object cannot be mutated by a caller.What I did not do
The epic also lists route path strings. I left those alone deliberately. Unlike the values above, each upstream path appears exactly once, so there is no duplication to remove: centralizing them would add a lookup table and an indirection without collapsing anything, and a new adapter still has to write its own route registrations either way. Happy to do it if you disagree, but it looked like churn rather than the single-edit win the other three are.
Checks
pnpm buildclean.pnpm testpasses: 49 suites, 287 tests (up 7).