feat(api-key): route-scoped IMS handler and middleware-driven controller auth#2344
feat(api-key): route-scoped IMS handler and middleware-driven controller auth#2344ravverma wants to merge 9 commits into
Conversation
…ler auth
Implements the IMS-to-JWT migration design for the api-key controller from
mysticat-architecture/platform/decisions/ims-to-jwt-api-key-controller-migration.md
(Option A, conservative variant: ApiKeyImsHandler added alongside the global
AdobeImsHandler so Auto-Fix and other still-IMS routes continue to work until
ASO-607 lands).
What changes
- New `src/support/api-key-ims-handler.js`: subclass of AdobeImsHandler whose
`checkAuth` returns null for any path other than `/tools/api-keys*`,
delegating to super only for the api-key endpoints. The handler name stays
`ims`, so authInfo.getType() and downstream code see no difference for
IaaS callers.
- `src/index.js` AUTH_HANDLERS chain now reads:
[SkipAuthHandler, GitHubWebhookHmacHandler, JwtHandler,
ApiKeyImsHandler, AdobeImsHandler, ScopedApiKeyHandler, LegacyApiKeyHandler]
ApiKeyImsHandler precedes AdobeImsHandler so the scoped path matches first
for /tools/api-keys; for any other route it returns null and the existing
global handler still handles IMS auth. Once Auto-Fix migrates, removing
AdobeImsHandler is a one-line change.
- `src/controllers/api-key.js` no longer calls `imsClient.getImsUserProfile`
or extracts the bearer token itself. Per the design's six refactor steps:
1. Removed validateImsOrgId() and getImsUserToken() private functions.
2. Org membership now checked via `authInfo.hasOrganization(imsOrgId)`.
3. Removed `imsUserProfile.email` usage.
4. imsUserId taken from `authInfo.getProfile().email` (consistent for
both IMS and JWT auth - documented in the resolveCaller() JSDoc).
5. `imsClient` dependency removed from the controller.
6. Tests updated to mock `attributes.authInfo` instead of `imsClient`.
All three endpoints (POST/DELETE/GET /tools/api-keys[/<id>]) now flow
through a shared `resolveCaller()` helper that surfaces a structured
ErrorWithStatusCode for missing header / missing authInfo / missing
user_id / org-mismatch.
Tests
- `test/controllers/api-key.test.js`: rewritten to mock authInfo directly.
Covers all six branches (missing imsOrgId, missing authInfo, missing
user_id, org-mismatch, all happy paths, configuration parse failure).
- `test/support/api-key-ims-handler.test.js`: new file. esmock stubs the
parent class and asserts the path scoping in isolation - verifies super
is called for /tools/api-keys[/<id>], and is NOT called for /sites,
/tools, /tools/other-tool, missing pathInfo, or non-string suffix.
- `test/auth-handlers-order.test.js`: extended with an executable contract
asserting `ApiKeyImsHandler` precedes `AdobeImsHandler` in AUTH_HANDLERS.
Refactored to share the parser between assertions.
Suite: 9004 passing, 100% statement / branch / line coverage.
| GitHubWebhookHmacHandler, | ||
| JwtHandler, | ||
| ApiKeyImsHandler, | ||
| AdobeImsHandler, |
There was a problem hiding this comment.
Keeping AdobeImsHandler in the chain to support auto_fix flow before its migration
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @ravverma,
Strengths
resolveCaller()is a real improvement (api-key.js:95-130). Consolidating three scattered functions (token extraction, IMS round-trip, profile-derived ID) into a single operation with a well-defined error contract is cleaner and eliminates a per-request IMS call that the middleware already handles.- The subclass approach is minimal and deletable (
api-key-ims-handler.js:1-52). ExtendingAdobeImsHandlerwith only a path guard, inheriting the'ims'handler name soauthInfo.getType()is unchanged for downstream code, and keeping the file to 52 lines makes this easy to review and easy to rip out once ASO-607 lands. - Auth-handler ordering is mechanically enforced (
auth-handlers-order.test.js:57-76). The executable contract test catches accidental import reorders before they become security bugs. The sharedparseAuthHandlersOrder()helper refactor is a nice touch. - Strong negative-path test coverage. Org-membership 401 on delete (
api-key.test.js:236-243), org-membership 401 on get (api-key.test.js:262-268), and the handler's null-guard surface (api-key-ims-handler.test.js:72-101) all assert the right behavior with the right side-effect expectations (no DB calls when auth fails). - Controller is now a pure
authInfoconsumer. RemovingimsClientfrom the controller harmonizes this code with the rest of the codebase that trusts the auth middleware, and eliminates a class of bugs where controllers and middleware can disagree about identity.
Issues
Important (Should Fix)
1. Username-prefix behavior change for IMS callers is undocumented and untested.
src/controllers/api-key.js:172 - The old code derived the API key's username prefix from imsUserProfile.email (the real email from an IMS profile fetch). The new code derives it from authInfo.getProfile().email, which for IMS callers AdobeImsHandler.transformProfile sets to payload.user_id (a GUID-style ID like ABC123@AdobeID), not the real email. Existing IMS keys were named firstname-<uuid>; new IMS keys will be named <imsGuidPrefix>-<uuid>. This is user-visible for IaaS consumers. The bare-UUID test at api-key.test.js:187 asserts only STATUS_CREATED, not the actual key format, so neither the old nor new behavior is pinned.
Fix: (a) Add a test with an IMS-style profile ({ email: 'ABC123@AdobeID' }) and assert the key matches ^ABC123-<uuid>$. (b) Strengthen the @AdobeID test to assert the key matches bare UUID format. (c) Acknowledge the change in the PR description or release notes so IaaS UI consumers are not surprised.
2. Comment overstates the bare-UUID fallback surface.
src/controllers/api-key.js:169 - The comment says "falling back to a bare UUID (e.g. for IDs that lead with @ or have no @ at all)". But 'noatsign'.split('@') returns ['noatsign'] (truthy), so the key would be noatsign-<uuid>, not a bare UUID. Only a leading @ produces the fallback.
Fix: Drop "or have no @ at all" from the comment.
3. No observability signal for the IMS migration end-state.
src/support/api-key-ims-handler.js:50 - The whole architectural premise is that this handler is transitional and deletable once IaaS callers move to JWT. But nothing distinguishes "IaaS request authenticated via IMS" from "IaaS request authenticated via JWT" in logs or metrics. Without that signal, the deletion plan is fragile - someone has to guess when the last IMS caller has migrated.
Fix: After super.checkAuth() returns successfully, emit an info-level log (e.g. this.log.info('api-key request authenticated via scoped IMS handler - JWT migration pending')). Cheap to add, cheap to delete with the rest of the file.
Minor (Nice to Have)
1. Path-prefix check not anchored at word boundary.
api-key-ims-handler.js:46 - startsWith('/tools/api-keys') would also match a hypothetical /tools/api-keys-batch. No such routes exist today and the test documents the behavior, but the sibling GitHubWebhookHmacHandler uses a boundary-anchored regex. For consistency: suffix === API_KEY_PATH_PREFIX || suffix.startsWith(API_KEY_PATH_PREFIX + '/').
2. !authInfo error message is misleading.
api-key.js:131 - The guard protects against the middleware not running, but the message says "Missing Authorization header". The header was present (the middleware consumed it). Consider "auth middleware did not populate authInfo" or similar.
3. validateRequestData now runs before auth check.
api-key.js:147-148 - Previously token extraction ran first. Now body validation runs before resolveCaller(). Both orders are defensible, but the change is silent. Consider reordering to auth-first (fail fast on unauthenticated requests) or noting the intentional change.
Recommendations
- Track deletion of
ApiKeyImsHandleras a sub-task of ASO-607 or add a// TODO(ASO-607)comment in the source so the removal isn't dependent on someone remembering the design doc. - Consider exporting the
makeAuthInfotest helper from a shared test-helpers file if other controller tests need to stub authInfo.
Assessment
Ready to merge? With fixes.
The architecture is sound - a minimal, deletable, well-justified piece of transitional code that faithfully implements the design doc's Option A. The controller refactor is a real improvement. The three Important items are narrow: a silent behavior change in the API key display name for IMS callers that should be documented and tested, an inaccurate comment, and a missing log line for migration observability. All fixable with small commits.
| // (e.g. for IDs that lead with `@` or have no `@` at all). The username | ||
| // is purely cosmetic - it lets users recognize their own keys at a glance. | ||
| // imsUserId is guaranteed non-empty by resolveCaller(). | ||
| const [username] = imsUserId.split('@'); |
There was a problem hiding this comment.
Important: Silent behavior change for IMS callers. The old code derived the username prefix from imsUserProfile.email (the real email). The new code uses authInfo.getProfile().email, which for IMS callers is payload.user_id (a GUID-style ID like ABC123@AdobeID). New IMS keys will have a different-looking prefix than existing ones.
Consider adding a test with an IMS-style profile ({ email: 'ABC123@AdobeID' }) to pin the new behavior, and noting the change in the PR description.
| // `@` (e.g. an IMS-issued ID without a local-part), the username prefix | ||
| // is empty and we fall back to a bare UUID. | ||
| context.attributes.authInfo = makeAuthInfo({ profileEmail: '@AdobeID' }); | ||
| apiKeyController = ApiKeyController(context); |
There was a problem hiding this comment.
The test name promises "fall back to a bare UUID" but only asserts STATUS_CREATED. The UUID-only branch is not verified. Consider: const json = await response.json(); expect(json.apiKey).to.match(/^[0-9a-f]{8}-/); to confirm no username prefix is present.
| // Create the API key | ||
| // Username prefix is best-effort: we use the local-part of the caller's | ||
| // email/user_id when an `@` is present, falling back to a bare UUID | ||
| // (e.g. for IDs that lead with `@` or have no `@` at all). The username |
There was a problem hiding this comment.
Nit: "have no @ at all" is inaccurate here. 'noatsign'.split('@') returns ['noatsign'] (truthy), so the key would be noatsign-<uuid>, not a bare UUID. Only a leading @ triggers the fallback. Drop the "or have no @ at all" clause.
| // Out-of-scope route - skip cleanly so the auth chain advances. | ||
| return null; | ||
| } | ||
| return super.checkAuth(request, context); |
There was a problem hiding this comment.
Consider adding an info-level log here when the scoped IMS path succeeds, e.g.:
const result = await super.checkAuth(request, context);
if (result) {
this.log.info('api-key request authenticated via scoped IMS handler - JWT migration pending');
}
return result;This gives operators a queryable signal for "is anyone still hitting the IMS path?" - without it, the plan to delete this handler requires manual investigation.
Important
- Pin IMS-style username prefix behavior with two new tests:
- JWT caller: profile.email is the real email -> prefix is local-part.
- IMS caller: profile.email is payload.user_id (e.g. ABC123@AdobeID) ->
prefix is the GUID portion. Documents the silent behavior change from
the previous IMS-round-trip controller.
Strengthened the bare-UUID test to assert the actual UUID format
(no leading prefix), not just the status code.
- Drop the inaccurate "or have no `@` at all" clause from the username
comment. `'noatsign'.split('@')` returns `['noatsign']` (truthy), so
only a leading `@` triggers the bare-UUID fallback.
- Add an info-level log on successful scoped IMS auth so operators have a
queryable signal for "is anyone still hitting this path?". Once the log
stops firing in production, the handler can be safely deleted (TODO is
documented in the file header tied to ASO-607).
Minor
- Anchor the path-prefix check at a `/` boundary
(suffix === '/tools/api-keys' || suffix.startsWith('/tools/api-keys/')).
Prevents a hypothetical /tools/api-keys-batch from accidentally falling
under this scope. New test covers the look-alike case explicitly.
- Clarify the "no authInfo" error message: it now reads
"Unauthorized: auth middleware did not populate authInfo" instead of
"Missing Authorization header" (the header was already consumed
upstream; surfacing it as a request-side error was misleading).
- Reorder createApiKey to authenticate first, then validate body. Fails
fast on unauthenticated requests and avoids leaking the expected
request shape to anonymous callers.
- TODO(ASO-607) added in the handler file header so the deletion plan is
visible at the source, not only in the design doc.
Suite: 9008 passing, 100% statement / branch / line coverage.
|
This PR will trigger a minor release when merged. |
|
Thanks @solaris007 - addressed in Important1. Username-prefix behavior change for IMS callers - Pinned with two new tests in
2. Comment overstates fallback surface - Dropped "or have no 3. No observability signal - Added an info-level log in Plus a corresponding
Minor1. Path-prefix not anchored at word boundary - Now uses an exact-or- suffix === API_KEY_PATH_PREFIX || suffix.startsWith(`${API_KEY_PATH_PREFIX}/`)The pre-existing test that used to document the loose-prefix behavior has been replaced by 2. 3. Recommendations
Test status
Ready for re-review. |
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @ravverma,
Thanks for the thorough fix commit (3607d887) - all six prior findings are verifiably addressed. The remaining items below are independent concerns, mostly defensive/architectural, surfaced by a deeper second pass focused on the security-sensitive auth path.
Strengths
- All six prior review findings verified addressed in
3607d887:- Username-prefix behavior change pinned with three regression tests at
test/controllers/api-key.test.js:182-215(JWT real-email, IMS user_id, bare-UUID@-prefix fallback) - Bare-UUID comment now correctly says "only when ID leads with
@" (src/controllers/api-key.js:178-185) - Migration observability log (
src/support/api-key-ims-handler.js:75) plus removal-trigger TODO at lines 50-54 / 66-69 - Path-prefix anchored via
isApiKeyRoute()helper with explicit/tools/api-keys-batchboundary test !authInfoerror message now correctly points at the middleware boundary (src/controllers/api-key.js:122-124)resolveCallerruns beforevalidateRequestDataincreateApiKey(lines 153-154)
- Username-prefix behavior change pinned with three regression tests at
- Token validation hops are correctly preserved:
super.checkAuthinApiKeyImsHandlerruns the full upstreamAdobeImsHandlervalidation (signature, expiry, IMS profile and org round-trip). No token-validation bypass introduced. - Per-request IMS revocation propagation is preserved - the controller-level IMS round-trip removed by this PR was duplicative with the handler's
getImsUserProfile/getImsUserOrganizationscalls. - The executable AUTH_HANDLERS order contract in
test/auth-handlers-order.test.js:65-83makes the architectural choice non-regressable. - Negative-space coverage is excellent: tests assert
expect(...).to.not.have.been.calledon data-access stubs in the org-membership 401 paths (test/controllers/api-key.test.js:266-272,293-298), proving no DB access on auth failure. crypto.randomUUID()(~122 bits effective entropy) plushashWithSHA256is the correct primitive choice for high-entropy random API keys; salt-less SHA-256 is standard practice for this material.deleteApiKeyreturns 404 for both "not found" and "not yours" branches, preventing existence enumeration.
Issues
Important (Should Fix)
1. Cross-protocol identity divergence creates orphaned, unmanageable API keys. src/controllers/api-key.js:118-126, :241-244
The same human gets different imsUserId values depending on auth path:
- IMS via
ApiKeyImsHandler:profile.email = payload.user_id(e.g.ABC123@AdobeID, set byAdobeImsHandler.transformProfile) - JWT via
JwtHandler:profile.email = payload.email(real email)
The controller persists whichever value profile.email has at creation time. After this PR, IaaS-only orgs use the IMS path and store ABC123@AdobeID. Once an org migrates to JWT (the ASO-607 trajectory), the same human authenticating via JWT presents imsUserId = real@adobe.com, which does not match the persisted ownership. Result:
getApiKeysreturns empty list under the new identity - existing keys appear "lost"deleteApiKeyreturns 404 - users cannot revoke their own keys- The orphaned keys remain VALID (
ScopedApiKeyHandlerstill honors them) - they grant access but the legitimate owner cannot rotate or revoke them
The controller comment at lines 118-126 documents the asymmetry as "intentional and documented", but the operational consequence (cannot revoke own key after auth-mode change, key stays valid until 6mo expiry) is a security lifecycle gap, not just a UX note. For IaaS-only orgs that never migrate, the divergence is the steady state, not a transitional artifact - which makes imsUserId an actively misleading field name.
Fix options (in ascending invasiveness):
- Document the steady-state behavior explicitly and accept it via an ADR addendum, ensuring the deletion plan in ASO-607 includes a remap migration for keys created under the IMS identity
- Widen the ownership check to try both identity forms when the JWT path can produce a stable IMS user_id alongside the real email
- Query both shapes in
allByImsOrgIdAndImsUserIdduring the migration window and return the union
This is the architecture-level concern that warrants alignment before more controllers migrate and bake in the same assumption.
2. Strengthen the authInfo contract at resolveCaller. src/controllers/api-key.js:117, :130
Two related defensive gaps:
(a) The controller's safety net against non-{IMS, JWT} handlers populating authInfo is implicit - it depends on alternate handler profile shapes lacking an .email field. Verified today: LegacyApiKeyHandler returns { user_id: 'admin' | 'legacy-user' }, ScopedApiKeyHandler returns the ApiKey entity. Neither has email, so profile?.email falls through to 401. But this is absence-of-fields safety, not a positive control. A future spacecat-shared schema change adding .email to either shape would silently turn a 401 into successful auth across handler types. Add a positive whitelist:
const allowed = ['ims', 'jwt'];
if (!allowed.includes(authInfo.getType?.())) {
throw new ErrorWithStatusCode('Unauthorized', STATUS_UNAUTHORIZED);
}(b) !authInfo.hasOrganization?.(imsOrgId) (line 130) optional-chains a contract method. If hasOrganization is missing, the negation evaluates truthy and emits the "Unable to find a reference to the Organization provided" 401 - a misleading client-error message for what is actually a server-side authInfo-shape bug. Either gate explicitly:
if (typeof authInfo.hasOrganization !== 'function') {
throw new ErrorWithStatusCode('Server error: authInfo missing hasOrganization contract', STATUS_INTERNAL_SERVER_ERROR);
}
if (!authInfo.hasOrganization(imsOrgId)) { ... }or call unconditionally and let the resulting TypeError propagate as a 500. Both make the contract on authInfo explicit and surface handler-shape regressions as 5xx (server bug) rather than 401 (looks like client error).
3. Test stub diverges from production AbstractHandler.log contract; assertion is misleading. test/support/api-key-ims-handler.test.js:38-40, :96-99
The stub installs this.log = (message, level) => log?.[level || 'info']?.(message). Production AbstractHandler.log (verified in spacecat-shared-http-utils) is this.logger[level](\[${this.name}] ${message}`)- it adds an[ims]prefix. So in production the actual logger receives[ims] api-key request authenticated via scoped IMS handler - JWT migration pending, but the test asserts calledOnceWithExactly('api-key request authenticated...')` (no prefix).
The test passes today, but it pins the wrong shape. A future change to how the handler exposes its logger (e.g. someone overriding name or refactoring AbstractHandler) would silently keep the test green while the production migration signal output drifts. Either align the stub to call log[level](`[${this.name}] ${message}`) and update the assertion to match, or relax to a less brittle matcher (calledWithMatch(/api-key request authenticated/)). Same fragility applies to making the stub a thin extension of the real AbstractHandler shape so future logger refactors are exercised.
Minor (Nice to Have)
-
resolveCallerreturns unusedauthInfofield (src/controllers/api-key.js:147). All three call sites destructure onlyimsUserId. Drop the unused field, or comment-document a near-term use. -
401 message variation enables state probing (
src/controllers/api-key.js:113-130). Four distinct 401 messages let an authenticated caller distinguish "no org header", "no middleware", "profile incomplete", and "wrong org" - the org-membership message in particular lets a caller probe arbitrary IMS Org IDs to map their non-membership. Consider a single "Unauthorized" body for post-auth failures with detail only inx-errorheader / structured logs. -
createApiKeyerror log loses request context (src/controllers/api-key.js:217). Logs onlyerror.message; noimsOrgId, no error stack.deleteApiKeydoes better. PassimsOrgId(do NOT logimsUserId/email - PII).
Recommendations
- Lift
resolveCallerto a shared utility. This is the first instance of an(imsOrgId header -> authInfo -> hasOrganization -> imsUserId)pattern that 3+ other controllers will need as they migrate offimsClient.getImsUserProfile. Extracting now tosrc/support/auth-info-utils.js(or extendingaccess-control-util.js) avoids drift on error wording, status code choice, and theimsUserId = profile.emailaliasing logic. Easier while there is exactly one caller. - Add a Coralogix alert + dashboard tile on the migration log. The TODO(ASO-607) deletion plan rests on the
api-key request authenticated via scoped IMS handlerlog dropping to zero. A 30-day rolling alert plus a paired alert on/tools/api-keysoverall traffic prevents "log shipping broke last Tuesday" being mistaken for "no IaaS callers". Track in the ASO-607 ticket. - Constrain
:idto UUID at the routing layer.DELETE /tools/api-keys/:idshould reject non-UUIDidearly using the sameisValidUUIDV4regex used elsewhere inindex.js. Reduces attack surface againstApiKey.findByIdand bounds theidlength inlog.error. - Validate
data.featuresagainst an allowlist invalidateRequestData(src/controllers/api-key.js:78-93). Currently a non-empty array of unknown values (e.g.["bogus"]) passes validation and produces a key withscopes: {}- a broken record that consumes amaxApiKeysslot. Pre-existing behavior, not introduced by this PR, but adjacent to the touched code and a one-line fix:if (!data.features.every((f) => KNOWN_FEATURES.has(f))). Out-of-scope-strict; recommendation only. - Add a regression test for the
hasOrganizationsemantic shift.AuthInfo.hasOrganizationstrips@AdobeOrgsuffix and matches ontenant.id- different semantics from the oldimsUserProfile.organizations.includes(imsOrgId)strict equality. Add a test feedingimsOrgId = 'test-org@AdobeOrg'against tenants[{ id: 'test-org' }]to lock the equality contract. - Document the username-prefix divergence in the OpenAPI spec / changelog. Customer-visible behavior change for IMS callers (key shape changes from
<email-localpart>-<uuid>to<guid>-<uuid>).
Out of scope, worth tracking
- Concurrent
createApiKeyrace pastmaxApiKeys(read-modify-write without optimistic locking). Pre-existing behavior, not introduced here. maxApiKeys=3enforcement is per-(org, user); a determined attacker with N IMS users in the same org can amass 3N keys. Modeling decision belongs in a separate roadmap discussion.- ASO-607 worker migration trigger lives in a different repo.
- Standardizing route-scoped auth handlers (a
RouteScopedHandler(prefix, delegate)wrapper) belongs inspacecat-shared-http-utils, not here.
Assessment
Ready to merge? With fixes.
Reasoning: All prior findings are addressed and the code-level quality is high. The three Important items above are real concerns - identity divergence is a security-lifecycle gap (orphaned keys remain valid but unmanageable), the authInfo contract gaps are inexpensive defensive controls that close a future-fragility surface, and the test-stub fidelity drift undermines the migration signal's regression coverage. None are critical bugs in the running system, but the security-sensitive nature of the auth path means these warrant attention before merge or in a fast follow-up.
Next Steps
- Address the three Important issues - identity divergence at minimum needs an explicit ADR-level acceptance and observability signal; the
authInfowhitelist + contract check is a small, defensive code change; the test stub fix is a one-liner. - Pick up the Minor items as cleanup commits or roll into the ASO-607 work.
- Consider lifting
resolveCallernow while there is exactly one caller.
| throw new ErrorWithStatusCode('Missing Authorization header', STATUS_UNAUTHORIZED); | ||
| const profile = authInfo.getProfile?.(); | ||
| // While the property is named 'profile.email', for IMS auth this is the | ||
| // user_id (set by AdobeImsHandler.transformProfile, e.g. ABC123@AdobeID); |
There was a problem hiding this comment.
Important: Cross-protocol identity divergence creates orphaned, unmanageable keys.
The comment documents the profile.email shape asymmetry as intentional, but the operational consequence is a security-lifecycle gap. After an org migrates IMS->JWT (the ASO-607 trajectory), the same human presents imsUserId = real@adobe.com instead of ABC123@AdobeID, the ownership check at line 241-244 misses, getApiKeys returns empty, and deleteApiKey returns 404. The orphaned keys remain VALID via ScopedApiKeyHandler until 6mo expiry but cannot be rotated or revoked by their legitimate owner.
For IaaS-only orgs that never migrate, the divergence is the steady state - not a transitional artifact - which makes imsUserId an actively misleading field name.
Fix options (ascending invasiveness): (a) document the steady-state behavior explicitly via an ADR addendum + remap migration for ASO-607; (b) widen the ownership check to try both identity forms; (c) query both shapes in allByImsOrgIdAndImsUserId during the migration window and return the union.
| // for JWT auth it is the actual email. Either way, it is the stable | ||
| // per-user identifier we store as imsUserId on the ApiKey record. The | ||
| // username prefix derived below therefore differs in shape between IMS | ||
| // and JWT callers - this is intentional and documented. |
There was a problem hiding this comment.
Important: Strengthen the authInfo contract here. Two related defensive gaps:
(a) The controller's safety net against non-{IMS, JWT} handlers populating authInfo is implicit - it depends on alternate handler profile shapes lacking an .email field. Verified today: LegacyApiKeyHandler returns { user_id: 'admin' | 'legacy-user' }, ScopedApiKeyHandler returns the ApiKey entity. Neither has email, so profile?.email falls through to 401. But this is absence-of-fields safety, not a positive control. A future spacecat-shared schema change adding .email to either shape would silently turn a 401 into successful auth across handler types. Add a positive whitelist near line 117:
const allowed = ['ims', 'jwt'];
if (!allowed.includes(authInfo.getType?.())) {
throw new ErrorWithStatusCode('Unauthorized', STATUS_UNAUTHORIZED);
}(b) On this line, !authInfo.hasOrganization?.(imsOrgId) optional-chains a contract method. If hasOrganization is missing, the negation evaluates truthy and emits the misleading client-error 401 for what is actually a server-side authInfo-shape bug. Either gate explicitly with a typeof ... !== 'function' 500, or call unconditionally so the resulting TypeError propagates as a 500. Either makes the contract explicit and surfaces handler-shape regressions as 5xx, not 401.
|
|
||
| it('does NOT emit the success log when super.checkAuth returns null (auth failed)', async () => { | ||
| superCheckAuthStub.resolves(null); | ||
| await handler.checkAuth({}, { pathInfo: { suffix: '/tools/api-keys' } }); |
There was a problem hiding this comment.
Important: Stub diverges from production AbstractHandler.log; this assertion pins the wrong shape.
Production AbstractHandler.log (verified in spacecat-shared-http-utils) is this.logger[level](`[${this.name}] ${message}`) - it adds an [ims] prefix. So in production the logger actually receives [ims] api-key request authenticated via scoped IMS handler - JWT migration pending, but the stub at lines 38-40 strips the prefix and this calledOnceWithExactly asserts the bare message.
The test passes today but verifies less than its name implies. A future refactor to how the handler exposes its logger (e.g. someone overriding name or restructuring AbstractHandler) would keep the test green while the production migration signal output drifts.
Fix: align the stub to log[level](`[${this.name}] ${message}`) and update this assertion to match, or relax to calledWithMatch(/api-key request authenticated/). Better: make the stub a thin extension of the real AbstractHandler shape so future logger refactors are exercised.
…fo contract Addresses three review findings on PR #2344: - ApiKeyImsHandler now sets profile.email to profile.trial_email (the real address) so IMS and JWT callers store the same stable identity on ApiKey records, preventing orphaned keys when an IaaS org migrates to JWT. - resolveCaller adds an explicit ['ims', 'jwt'] type whitelist and replaces the optional-chained hasOrganization call with a typeof guard that returns 500 on a malformed authInfo, instead of a misleading 401. - Handler test stub mirrors AbstractHandler's [ims] log prefix and adds coverage for the profile-email normalisation path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the second pass — addressed all three Important findings in 1. Cross-protocol identity divergence — 2.
3. Test stub log-shape fidelity — The Also tightened the controller comment on All 36 tests across the two affected files pass; lint is clean. |
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @ravverma,
Thanks for the third-pass turnaround. The three Important findings from the 2026-05-08 review are all addressed cleanly in commit 0216443: the trial_email normalization restores cross-protocol identity continuity, the ALLOWED_AUTH_TYPES whitelist plus typeof guard make the authInfo contract explicit, and the test stub now mirrors the production [<name>] log prefix. The new commit is tightly scoped and the tests reflect the contract accurately.
No Important or Critical findings on this pass. A few Minor items below, mostly defense-in-depth around the new normalization path. Plus the identity-migration story for pre-existing keys is worth tracking before rollout.
Strengths
Prior Important findings from the 2026-05-08 review all addressed in 0216443:
- Cross-protocol identity divergence:
ApiKeyImsHandler.checkAuthnow readsprofile.trial_email(the server-attested real email, written byAdobeImsHandler.transformProfile) and overridesprofile.emailon the api-key path so IMS and JWT callers persist identical identities going forward. Two handler tests pin the normalize-when-present and leave-alone-when-absent behavior. - authInfo contract tightening:
ALLOWED_AUTH_TYPES = ['ims', 'jwt']whitelist atresolveCallerplus thetypeof authInfo.hasOrganization !== 'function'guard convert two implicit safety nets into explicit contracts. The 500 vs 401 split correctly distinguishes server-side bugs from client-side auth failures. Two controller tests cover both branches. - Test stub fidelity:
test/support/api-key-ims-handler.test.jsstub now mirrors AbstractHandler's[<name>] <message>log shape, and the migration-signal assertion checks the[ims]-prefixed output - the test now pins the production contract.
Architecture-level observations:
- The trial_email normalization is correctly scoped to the deprecated-path subclass (
src/support/api-key-ims-handler.js:77-82). It does not modify the upstreamAdobeImsHandler.transformProfilecontract; it is a localized transitional fix that disappears when ASO-607 deletes this handler. - The
ALLOWED_AUTH_TYPESwhitelist comment (src/controllers/api-key.js:124-130) names the rejected handlers (LegacyApiKeyHandler,ScopedApiKeyHandler), the failure mode (silent fall-through to ownership lookup), and the future-proofing motivation. A maintainer six months from now will immediately understand why this guard exists. - The 500 vs 401 separation is operationally correct: a missing
hasOrganizationis a server-side contract bug that should page on a 5xx, not a client-facing 401 that gets silently absorbed. - Upstream contract verified:
profile.trial_emailis server-attested viacontext.imsClient.getImsUserProfile(token)inAdobeImsHandler.checkAuth, not derived from the bearer token'semailclaim. There is no JWT-confusion path for hijacking key ownership throughtrial_email.
Issues
Minor (Nice to Have)
- Profile mutation in place is a quiet contract reach.
src/support/api-key-ims-handler.js:80-82
profile.email = profile.trial_email mutates the object returned by result.getProfile(). Today AuthInfo is constructed fresh per request inside AdobeImsHandler.checkAuth, so the mutation is request-scoped and the contract holds. But that is an unstated invariant of an external package: if @adobe/spacecat-shared-http-utils ever introduces caching on the IMS path (per-token memoization, profile cache), this mutation would silently leak across requests as a tampered email pinned onto a cached profile.
How to fix (any one):
- Construct a fresh profile rather than mutating:
result.withProfile?.({ ...profile, email: profile.trial_email })if such a setter exists. - Derive the normalized email at the call site without mutating the shared object.
- At minimum, add a one-line comment asserting "profile is per-request and not cached" so the invariant is documented at the mutation site, not just implied by spacecat-shared.
- 401 message variation continues to grow the state-probing surface.
src/controllers/api-key.js:118-150
Minor 2 from the 2026-05-08 review is unresolved, and the new whitelist branch adds a fourth distinct 401 body ("Unauthorized") to the existing three ("auth middleware did not populate authInfo", "caller profile is missing email/user_id", "Unable to find a reference to the Organization provided"). The new 500 ("authInfo missing hasOrganization") is appropriate as a server-error message and not in scope for this concern.
An authenticated probe can now distinguish all four 401 paths, including org-membership which lets a caller test arbitrary IMS Org IDs against their org set. The detail is useful in logs but does not need to be on the wire.
How to fix: collapse the 4xx auth-failure bodies to a single sanitized "Unauthorized" response and emit the discriminating reason as a structured log line at the throw site (with imsOrgId, authType, reason). The 500 should continue to differ in status code but its body can be normalized too.
- Empty-string or null trial_email silently leaves the IMS alias on profile.email.
src/support/api-key-ims-handler.js:80-82
The truthy check if (profile?.trial_email) skips normalization when trial_email is '' or null. In that path profile.email remains the IMS user_id alias (e.g. ABC123@AdobeID), the controller stores that as imsUserId, and the resulting ApiKey identity diverges from the JWT path - re-introducing the bug this PR is fixing.
I do not have a confirmed scenario where IMS returns an empty email (the upstream override in AdobeImsHandler.checkAuth reads imsProfile.email from the IMS profile API), but the failure mode is silent. The two existing tests cover trial_email present and trial_email absent (key missing entirely); neither covers '' or null.
How to fix: either (a) emit a warn log when trial_email is falsy-but-present so the divergence is observable for ASO-607 sign-off, and/or (b) add a third normalization test asserting profile.email is unchanged when trial_email: '' (and optionally null) so a future change to the guard cannot silently wipe identity.
Recommendations
- When the next controller migrates off
imsClient.getImsUserProfile, lift theresolveCallerpattern (whitelist + typeof guard + profile.email aliasing) intosrc/support/access-control-util.jsasAccessControlUtil.resolveCallerIdentity(imsOrgId). The existing module already encodesisAccessTypeIms,isAccessTypeJWT, andhasOrganizationhelpers, so the resolveCaller logic naturally belongs there. Acting once there is a second caller avoids cross-controller drift on error wording and status code choices. - Pin the ASO-607 cleanup checklist with three deletion targets: (a)
ApiKeyImsHandlerregistration inAUTH_HANDLERS, (b)ALLOWED_AUTH_TYPESwhitelist collapse to['jwt'](or move to AccessControlUtil per the previous recommendation), (c) thetrial_email-related comment inapi-key.jsaround line 132 - it becomes wrong once the IMS path is removed. The design degrades gracefully but the cleanup is multi-step. - Track the migration-signal log in Coralogix with a rolling alert tied to
/tools/api-keysoverall traffic, so "log shipping broke last Tuesday" is not mistaken for "no IaaS callers." This was a prior recommendation; restating because ASO-607 sign-off depends on it. - Previously flagged Minor (resolveCaller returns unused
authInfofield, all three call sites destructure onlyimsUserId) remains unresolved. Either dropauthInfofrom the return shape or comment-document it as exposed for future call sites - the function's contract is currently hard to test against because no callers exercise the authInfo half. - Add an integration test that walks the full auth path end-to-end: handler normalizes profile.email -> controller reads profile.email -> ApiKey record stores the real email. The split-level tests (handler unit, controller unit) verify each side of the contract independently but not the contract itself. Useful for ASO-607 sign-off.
Out of Scope, Worth Tracking
- Identity migration for pre-existing IMS-created keys: keys persisted before this PR with
imsUserId = <user_id>@AdobeIDwill not match an ownership lookup by real email after this PR ships. Affected users cannot list or rotate their existing keys via the API; the keys themselves remain valid bearer tokens until natural expiry (six months by default). A one-shot data migration (rewriteimsUserIdby resolving user_id to real email via IMS profile lookup) or a release-note disclosure with operator playbook should accompany the rollout. Tracked under ASO-607 territory; not addressable in this PR. - S2S JWT callers will pass the
ALLOWED_AUTH_TYPESwhitelist. A service-to-service request authenticated via the JwtHandler S2S wrapper surfacestype='jwt'with the S2S JWT's email claim asprofile.email. If machine identities are not supposed to create/own user-scoped API keys, the controller should additionally check!authInfo.isS2SConsumer()(or whatever the canonical predicate is). Confirm intent during the broader JWT migration. AuthInfo.getProfile()returning a live profile reference rather than a defensive copy or a typed setter is the upstream affordance that makes the in-place mutation possible. A hardening pass onspacecat-shared-http-utilscould close this footgun, but the contract change belongs there, not here.
Assessment
Ready to merge? Yes.
Reasoning: The third-pass changes correctly close the two material gaps from the 2026-05-08 review (identity divergence, authInfo contract) with appropriately scoped code, accurate comments, and matching test coverage. The remaining Minors are defense-in-depth and observability refinements that are well below merge-blocking severity on a third pass. The identity-migration story for pre-existing keys is the only operational item that warrants explicit tracking before customers notice; ASO-607 is the natural home.
Next Steps
- Optional: harden the profile mutation (Minor 1) or document the per-request invariant.
- Optional: collapse 4xx auth-failure response bodies (Minor 2) and add a structured audit-log line for observability.
- Optional: add the empty-string trial_email test (Minor 3).
- File an identity-migration ASO-607 sub-task before the rollout so existing IMS-keyed users aren't stranded.
2 participants
Implements the IMS-to-JWT migration design for the api-key controller from mysticat-architecture/platform/decisions/ims-to-jwt-api-key-controller-migration.md (Option A, conservative variant: ApiKeyImsHandler added alongside the global AdobeImsHandler so Auto-Fix and other still-IMS routes continue to work until ASO-607 lands).
What changes
New
src/support/api-key-ims-handler.js: subclass of AdobeImsHandler whosecheckAuthreturns null for any path other than/tools/api-keys*, delegating to super only for the api-key endpoints. The handler name staysims, so authInfo.getType() and downstream code see no difference for IaaS callers.src/index.jsAUTH_HANDLERS chain now reads: [SkipAuthHandler, GitHubWebhookHmacHandler, JwtHandler, ApiKeyImsHandler, AdobeImsHandler, ScopedApiKeyHandler, LegacyApiKeyHandler] ApiKeyImsHandler precedes AdobeImsHandler so the scoped path matches first for /tools/api-keys; for any other route it returns null and the existing global handler still handles IMS auth. Once Auto-Fix migrates, removing AdobeImsHandler is a one-line change.src/controllers/api-key.jsno longer callsimsClient.getImsUserProfileor extracts the bearer token itself. Per the design's six refactor steps:authInfo.hasOrganization(imsOrgId).imsUserProfile.emailusage.authInfo.getProfile().email(consistent for both IMS and JWT auth - documented in the resolveCaller() JSDoc).imsClientdependency removed from the controller.attributes.authInfoinstead ofimsClient.All three endpoints (POST/DELETE/GET /tools/api-keys[/]) now flow
through a shared
resolveCaller()helper that surfaces a structuredErrorWithStatusCode for missing header / missing authInfo / missing
user_id / org-mismatch.
Tests
test/controllers/api-key.test.js: rewritten to mock authInfo directly. Covers all six branches (missing imsOrgId, missing authInfo, missing user_id, org-mismatch, all happy paths, configuration parse failure).test/support/api-key-ims-handler.test.js: new file. esmock stubs the parent class and asserts the path scoping in isolation - verifies super is called for /tools/api-keys[/], and is NOT called for /sites, /tools, /tools/other-tool, missing pathInfo, or non-string suffix.test/auth-handlers-order.test.js: extended with an executable contract assertingApiKeyImsHandlerprecedesAdobeImsHandlerin AUTH_HANDLERS. Refactored to share the parser between assertions.Suite: 9004 passing, 100% statement / branch / line coverage.
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
Thanks for contributing!