Conversation
…helpers - Add FF_READ_ONLY_ADMIN constant shared with LaunchDarkly checks - IMS: skip auth for read-only orgs (LD flag); log detail, throw caught as Unauthorized - readOnlyAdminWrapper: LD gate + route read/write enforcement for RO admins - route-utils, s2s-wrapper route capability guards; JWT read-only admin claims - Add @adobe/spacecat-shared-launchdarkly-client dependency; esmock for tests Made-with: Cursor
solaris007
left a comment
solaris007
left a comment
There was a problem hiding this comment.
Hey @ravverma,
Strengths
- Correct fail-mode asymmetry. The IMS handler fails open (LD unavailable = allow auth, preserving existing behavior) while the
readOnlyAdminWrapperfails closed (LD unavailable = deny RO admin access). This is the right default for each trust boundary. - JWT mutual exclusion guard (
jwt.js:44-47). Rejecting tokens that carry bothis_adminandis_read_only_adminprevents privilege confusion from malformed or tampered tokens. Clean and explicit. - Deny-by-default for unmapped routes. When
resolveRouteCapabilityreturns null,actionis undefined (not'read'), so unmapped routes are blocked for RO admins. Correct security posture, tested explicitly. - Clean route-utils extraction. Lifting
matchRoute,resolveRouteCapability, andguardNonEmptyRouteCapabilitiesout ofs2s-wrapper.jsinto a shared module is well-scoped refactoring that immediately pays for itself. - Thorough test coverage. Both fail-open (IMS: null LD client, LD throws, missing ident) and fail-closed (wrapper: null LD client, LD throws, empty tenants) paths are individually tested. The esmock approach for isolating LD is clean.
Issues
Critical (Must Fix)
1. IMS gate blocks ALL users in a flagged org, not just read-only admins (ims.js:199-202)
The isReadOnlyAdminEnabled check runs before isUserASOAdmin and before scope assignment. When the flag is enabled for an org, every user in that org - regular customers, non-admin users, even ASO admins - is blocked with throw new Error('Unauthorized'). The test "returns null for ASO admin when org FF is enabled" confirms this.
If the intent is to force flagged orgs onto the JWT auth path (via auth-service #514), this needs explicit documentation in the code. If the intent is only to gate read-only admin access, the check needs to move after admin determination.
Why it matters: Enabling this flag for an org silently breaks IMS-based access for all users in that org across all routes. The function name isReadOnlyAdminEnabled is misleading about the blast radius.
Suggested fix: Either (a) rename to isOrgBlockedFromImsAuth and add a clear comment explaining this is intentional and that auth-service #514 provides the alternative JWT path, or (b) move the check to only affect users who should be read-only admins.
Important (Should Fix)
2. readOnlyAdminWrapper silently passes through RO admins when routeCapabilities is omitted (read-only-admin-wrapper.js:86)
When routeCapabilities is undefined or null, the isObject() check is false, so the entire write-blocking logic is skipped. An RO admin passes through with no restrictions - only an audit log entry. The wrapper's name and JSDoc promise write-blocking, but omitting routeCapabilities silently disables enforcement.
Compare with guardNonEmptyRouteCapabilities, which catches {} but not undefined. A consumer who calls readOnlyAdminWrapper(fn) without options gets audit logging but zero authorization enforcement.
Suggested fix: Make routeCapabilities required - throw at creation time if it is falsy, matching the empty-object guard pattern.
3. First-org-only flag evaluation is fragile for multi-org users (ims.js:117, read-only-admin-wrapper.js:43)
Both isReadOnlyAdminEnabled and evaluateFeatureFlag evaluate the flag against only organizations[0] / tenantIds[0]. A user belonging to multiple IMS orgs where only some are read-only-flagged will get inconsistent behavior depending on array ordering, which is not guaranteed to be stable.
Suggested fix: Evaluate the flag against all orgs. For the IMS gate (block path), return true if any org is flagged. For the wrapper, resolve the specific org from context. Alternatively, document this as a known limitation with rationale.
4. Misleading JSDoc says "fail-closed" but IMS handler is fail-open (ims.js:107)
The JSDoc on isReadOnlyAdminEnabled says "Fail-closed: returns false when the LD client is unavailable." Returning false means "not read-only, allow through" - that is fail-open with respect to the blocking behavior. The wrapper's evaluateFeatureFlag has the same comment, but there false results in a 403 (actually fail-closed). The comment is correct for the wrapper but wrong for the IMS handler.
Suggested fix: Change to "Fail-open: returns false (allowing authentication) when the LD client is unavailable or evaluation errors."
5. Audit log lacks user identity (read-only-admin-wrapper.js:97)
The log entry [ro-admin-audit] RO admin accessed: GET /sites records method and path but not who performed the action. For a role with reduced privileges, knowing WHO accessed WHAT is essential for incident investigation.
Suggested fix: Include the user's email or org ID from authInfo, e.g. log.info({ tag: 'ro-admin-audit', method, path: suffix, org: tenantIds[0] }).
6. Log injection surface via context.pathInfo.suffix (read-only-admin-wrapper.js:92,97)
suffix comes from the HTTP request path. If the logging framework does not sanitize newlines, an attacker can inject fake log lines (e.g., GET /sites%0A[ro-admin-audit] RO admin accessed: DELETE /sites/victim-id), polluting the audit trail that is the primary observability for this feature.
Suggested fix: Use structured log fields instead of string interpolation: log.info({ tag: 'ro-admin-audit', method, suffix }), or sanitize suffix before interpolation.
7. Product-specific flag key in shared library (constants.js:13)
The flag key FT_LLMO-3008 is LLMO-product-specific, but this code lives in spacecat-shared-http-utils, consumed by all SpaceCat services. This couples the shared auth layer to LLMO's feature management namespace.
Suggested fix: Either accept the flag key as a configuration parameter (via context or wrapper options) or rename the flag to something product-neutral like FT_READ_ONLY_ORG.
Minor (Nice to Have)
8. Em-dash in JWT log message (jwt.js:45): 'Token has both is_admin and is_read_only_admin — rejecting' uses an em-dash. The codebase convention uses regular dashes.
9. Capability string parsing contract is undocumented (read-only-admin-wrapper.js:88): capability?.split(':').pop() assumes 'resource:action' format but this is not documented. A bare value like 'readonly' would be blocked since it does not equal 'read'.
10. Information disclosure in 403 responses: The two distinct messages ("Read-only admin access is not enabled" vs "Read-only admin users cannot perform write operations") reveal feature flag state and role classification. Consider returning a single generic "Forbidden" message and logging the specific reason server-side only.
Recommendations
- Document the two-gate architecture. The interaction between the IMS handler gate (blocks direct IMS auth for RO orgs) and the wrapper gate (enforces read-only on JWT-authenticated RO admins) is the key design decision. Neither the code comments nor the PR description explain this end-to-end flow clearly.
- Consider a shared
evaluateLDFlag(context, orgIdent, flagKey)utility. The LD client creation + org suffix + try/catch pattern is now in two places with different fail modes. A single function with afailDefaultparameter would reduce the surface for bugs. - Coordinate rollout documentation with auth-service #514. The blast radius of enabling
FT_LLMO-3008needs to be crystal clear since it blocks all IMS auth for the flagged org.
Assessment
Ready to merge? No - with fixes.
The security design is sound - fail-mode polarity is correct, JWT mutual exclusion is good, deny-by-default on unmapped routes is correct. However, Critical #1 (IMS gate blocks all users, not just RO admins) needs clarification or a code change - the naming and documentation do not match the behavior's blast radius. Important #2 (wrapper passthrough without routeCapabilities) is a misconfiguration trap that could silently grant RO admins full write access. Important #3 (first-org-only evaluation) is a real risk for multi-org users.
Next Steps
- Clarify or fix the IMS gate scope (Critical #1) - document if intentional, fix if not.
- Make
routeCapabilitiesrequired in the wrapper (Important #2). - Address first-org-only evaluation or document as accepted limitation (Important #3).
- Fix the misleading JSDoc and add user identity to audit logs (Important #4, #5).
- Minor items are optional but recommended.
| const payload = await this.#validateToken(token, config); | ||
| const imsProfile = await context.imsClient.getImsUserProfile(token); | ||
| const organizations = await context.imsClient.getImsUserOrganizations(token); | ||
| if (await isReadOnlyAdminEnabled(context, organizations)) { |
There was a problem hiding this comment.
Critical: This blocks ALL users in the flagged org (not just RO admins) since it runs before isUserASOAdmin and scope assignment. If intentional (forcing migration to JWT path via auth-service #514), rename isReadOnlyAdminEnabled to something like isOrgBlockedFromImsAuth and add a comment explaining the design. If not intentional, move this check after admin determination.
| return forbidden('Read-only admin access is not enabled'); | ||
| } | ||
|
|
||
| if (isObject(routeCapabilities)) { |
There was a problem hiding this comment.
Important: When routeCapabilities is omitted (undefined/null), this isObject check is false and the entire write-blocking logic is skipped. RO admins pass through with zero restrictions. Consider making routeCapabilities required - throw at creation time if falsy, matching the guardNonEmptyRouteCapabilities pattern for {}.
| const ldClient = LaunchDarklyClient.createFrom(context); | ||
| if (!ldClient) return false; | ||
|
|
||
| const ident = organizations[0]?.orgRef?.ident; |
There was a problem hiding this comment.
Important: Only the first org is evaluated. Multi-org users may bypass the gate if the read-only org is not first in the array (IMS org ordering is not guaranteed stable). Consider evaluating against all orgs, or document this as an accepted limitation.
|
|
||
| /** | ||
| * Checks whether the read-only admin feature flag is enabled for the user's | ||
| * first IMS organization. Fail-closed: returns false when the LD client is |
There was a problem hiding this comment.
Important: This says "Fail-closed" but returning false here means "not read-only, allow through" - that is fail-open. The wrapper's evaluateFeatureFlag has the same comment where it is correct (false = 403). Suggest rewording to: "Fail-open: returns false (allowing authentication) when the LD client is unavailable or evaluation errors."
2 participants
Summary
Adds read-only org / read-only admin behavior in
@adobe/spacecat-shared-http-utils: shared LaunchDarkly feature flag, IMS validation gate for read-only orgs, JWT claims support, route helpers, S2S wrapper updates, andreadOnlyAdminWrapperfor LD + read/write route enforcement.Coordinates with spacecat-auth-service: adobe/spacecat-auth-service#514.
Changes
constants.js—FF_READ_ONLY_ADMIN(FT_LLMO-3008) as single source for LD keyhandlers/ims.js— After profile/orgs, evaluate LD for first org; when flag is on, log (warn) and treat as unauthorized (client sees generic failure; existing catch logs detail)read-only-admin-wrapper.js—LaunchDarklyClient.createFrom(context); 403 for disallowed methods when RO admin + flag onroute-utils.js— Route read/write classification for wrappershandlers/jwt.js,auth-info.js,s2s-wrapper.js— Read-only admin claims / capability alignmentindex.js+index.d.tsfor new APIsims,read-only-admin-wrapper,route-utils(esmock for LaunchDarkly)@adobe/spacecat-shared-launchdarkly-client; devesmock;package-lock.jsonupdatedConsumer notes
@adobe/spacecat-shared-http-utilsin services that use IMS / admin wrappers after release.Test plan
npm run lint -w packages/spacecat-shared-http-utilsnpm test -w packages/spacecat-shared-http-utils(run in CI / locally)