fix: review commits

Author: ravverma

packages/spacecat-shared-http-utils/src/auth/constants.js

@@ -10,4 +10,4 @@
  * governing permissions and limitations under the License.
  */
 
-export const FF_READ_ONLY_ADMIN = 'FT_LLMO-3008';
+export const FF_READ_ONLY_ORG = 'FT_READ_ONLY_ORG';

packages/spacecat-shared-http-utils/src/auth/handlers/ims.js

@@ -21,7 +21,7 @@ import { LaunchDarklyClient } from '@adobe/spacecat-shared-launchdarkly-client';
 import { getBearerToken } from './utils/bearer.js';
 import AbstractHandler from './abstract.js';
 import AuthInfo from '../auth-info.js';
-import { FF_READ_ONLY_ADMIN } from '../constants.js';
+import { FF_READ_ONLY_ORG } from '../constants.js';
 
 const IGNORED_PROFILE_PROPS = [
   'id',
@@ -103,22 +103,31 @@ function isUserASOAdmin(organizations) {
 }
 
 /**
- * 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
+ * Checks whether the read-only org gate flag is enabled for the user's first
+ * IMS organization. When true, ALL IMS-authenticated users in that org are
+ * blocked (not just RO admins) — this intentionally forces the entire org to
+ * authenticate via the JWT/auth-service path instead.
+ *
+ * NOTE: Only the first org in the array is evaluated. Multi-org users whose
+ * read-only org is not first may bypass this gate; this is an accepted
+ * limitation given IMS org ordering is not guaranteed stable.
+ *
+ * Fail-open: returns false (allowing authentication) when the LD client is
  * unavailable or evaluation errors.
  */
-async function isReadOnlyAdminEnabled(context, organizations) {
+async function isOrgBlockedFromImsAuth(context, organizations) {
   if (!isNonEmptyArray(organizations)) return false;
 
   try {
     const ldClient = LaunchDarklyClient.createFrom(context);
     if (!ldClient) return false;
 
+    // Only evaluate the first org — see NOTE above.
     const ident = organizations[0]?.orgRef?.ident;
     if (!ident) return false;
 
     const imsOrgId = `${ident}@AdobeOrg`;
-    return await ldClient.isFlagEnabledForIMSOrg(FF_READ_ONLY_ADMIN, imsOrgId);
+    return await ldClient.isFlagEnabledForIMSOrg(FF_READ_ONLY_ORG, imsOrgId);
   } catch {
     return false;
   }
@@ -196,7 +205,9 @@ export default class AdobeImsHandler extends AbstractHandler {
       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)) {
+      // Blocks ALL users in the org (not just RO admins) when the gate flag is
+      // enabled — this is intentional: the entire org must migrate to the JWT path.
+      if (await isOrgBlockedFromImsAuth(context, organizations)) {
         this.log('User belongs to a read-only org, blocking IMS authentication', 'warn');
         throw new Error('Unauthorized');
       }

packages/spacecat-shared-http-utils/src/auth/handlers/jwt.js

@@ -42,7 +42,7 @@ export default class JwtHandler extends AbstractHandler {
       payload.tenants = payload.tenants || [];
 
       if (payload.is_admin && payload.is_read_only_admin) {
-        this.log('Token has both is_admin and is_read_only_admin — rejecting', 'warn');
+        this.log('Token has both is_admin and is_read_only_admin - rejecting', 'warn');
         return null;
       }
 

packages/spacecat-shared-http-utils/src/auth/read-only-admin-wrapper.js

@@ -14,7 +14,7 @@ import { Response } from '@adobe/fetch';
 import { isObject } from '@adobe/spacecat-shared-utils';
 import { LaunchDarklyClient } from '@adobe/spacecat-shared-launchdarkly-client';
 
-import { FF_READ_ONLY_ADMIN } from './constants.js';
+import { FF_READ_ONLY_ORG } from './constants.js';
 import { guardNonEmptyRouteCapabilities, resolveRouteCapability } from './route-utils.js';
 
 function forbidden(message) {
@@ -44,7 +44,7 @@ async function evaluateFeatureFlag(context, authInfo) {
     if (!ident) return false;
 
     const imsOrgId = `${ident}@AdobeOrg`;
-    return await ldClient.isFlagEnabledForIMSOrg(FF_READ_ONLY_ADMIN, imsOrgId);
+    return await ldClient.isFlagEnabledForIMSOrg(FF_READ_ONLY_ORG, imsOrgId);
   } catch {
     return false;
   }
@@ -70,6 +70,9 @@ async function evaluateFeatureFlag(context, authInfo) {
  * @returns {Function} A wrapped handler.
  */
 export function readOnlyAdminWrapper(fn, { routeCapabilities } = {}) {
+  if (!routeCapabilities) {
+    throw new Error('readOnlyAdminWrapper: routeCapabilities is required');
+  }
   guardNonEmptyRouteCapabilities('readOnlyAdminWrapper', routeCapabilities);
 
   return async (request, context) => {
@@ -79,22 +82,37 @@ export function readOnlyAdminWrapper(fn, { routeCapabilities } = {}) {
     if (authInfo?.isReadOnlyAdmin?.()) {
       const ffEnabled = await evaluateFeatureFlag(context, authInfo);
       if (!ffEnabled) {
-        log.warn('[ro-admin] Feature flag disabled, denying RO admin access');
-        return forbidden('Read-only admin access is not enabled');
+        log.warn({
+          tag: 'ro-admin',
+          org: authInfo.getTenantIds?.()[0],
+        }, 'Feature flag disabled, denying RO admin access');
+        return forbidden('Forbidden');
       }
 
       if (isObject(routeCapabilities)) {
         const capability = resolveRouteCapability(context, routeCapabilities);
+        // capability format is 'resource:action' (e.g. 'site:read', 'site:write').
+        // split(':').pop() extracts the action; a missing or malformed value yields
+        // undefined, which is !== 'read' and correctly blocks the request.
         const action = capability?.split(':').pop();
 
         if (action !== 'read') {
-          const route = `${context.pathInfo?.method} ${context.pathInfo?.suffix}`;
-          log.warn(`[ro-admin] Read-only admin blocked from route: ${route}`);
-          return forbidden('Read-only admin users cannot perform write operations');
+          log.warn({
+            tag: 'ro-admin',
+            method: context.pathInfo?.method,
+            suffix: context.pathInfo?.suffix,
+            org: authInfo.getTenantIds?.()[0],
+          }, 'Read-only admin blocked from route');
+          return forbidden('Forbidden');
         }
       }
 
-      log.info(`[ro-admin-audit] RO admin accessed: ${context.pathInfo?.method} ${context.pathInfo?.suffix}`);
+      log.info({
+        tag: 'ro-admin-audit',
+        method: context.pathInfo?.method,
+        suffix: context.pathInfo?.suffix,
+        org: authInfo.getTenantIds?.()[0],
+      }, 'RO admin accessed route');
     }
 
     return fn(request, context);

packages/spacecat-shared-http-utils/test/auth/handlers/ims.test.js

@@ -449,7 +449,7 @@ describe('AdobeImsHandler', () => {
       expect(logStub.debug.calledWithMatch('Failed to validate token: Unauthorized')).to.be.true;
       expect(ldClient.isFlagEnabledForIMSOrg.calledOnce).to.be.true;
       const [flagKey, imsOrgId] = ldClient.isFlagEnabledForIMSOrg.firstCall.args;
-      expect(flagKey).to.equal('FT_LLMO-3008');
+      expect(flagKey).to.equal('FT_READ_ONLY_ORG');
       expect(imsOrgId).to.equal('org-ro-1@AdobeOrg');
     });
 

packages/spacecat-shared-http-utils/test/auth/handlers/jwt.test.js

@@ -312,7 +312,7 @@ describe('SpacecatJWTHandler', () => {
       const result = await handler.checkAuth({}, context);
 
       expect(result).to.be.null;
-      expect(logStub.warn.calledWith('[jwt] Token has both is_admin and is_read_only_admin — rejecting')).to.be.true;
+      expect(logStub.warn.calledWith('[jwt] Token has both is_admin and is_read_only_admin - rejecting')).to.be.true;
     });
 
     it('successfully validates a token with is_read_only_admin and adds read_only_admin scope', async () => {

packages/spacecat-shared-http-utils/test/auth/read-only-admin-wrapper.test.js

@@ -116,8 +116,8 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(result.status).to.equal(403);
       const body = await result.json();
-      expect(body.message).to.equal('Read-only admin access is not enabled');
-      expect(logStub.warn.calledWithMatch('Feature flag disabled')).to.be.true;
+      expect(body.message).to.equal('Forbidden');
+      expect(logStub.warn.calledWithMatch({ tag: 'ro-admin' }, 'Feature flag disabled, denying RO admin access')).to.be.true;
       expect(handler.called).to.be.false;
     });
 
@@ -134,7 +134,7 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(result.status).to.equal(403);
       const body = await result.json();
-      expect(body.message).to.equal('Read-only admin access is not enabled');
+      expect(body.message).to.equal('Forbidden');
       expect(handler.called).to.be.false;
     });
 
@@ -151,7 +151,7 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(result.status).to.equal(403);
       const body = await result.json();
-      expect(body.message).to.equal('Read-only admin access is not enabled');
+      expect(body.message).to.equal('Forbidden');
       expect(handler.called).to.be.false;
     });
 
@@ -162,7 +162,7 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(result.status).to.equal(403);
       const body = await result.json();
-      expect(body.message).to.equal('Read-only admin access is not enabled');
+      expect(body.message).to.equal('Forbidden');
       expect(handler.called).to.be.false;
     });
 
@@ -191,7 +191,7 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(ldClient.isFlagEnabledForIMSOrg.calledOnce).to.be.true;
       const [flagKey, imsOrgId] = ldClient.isFlagEnabledForIMSOrg.firstCall.args;
-      expect(flagKey).to.equal('FT_LLMO-3008');
+      expect(flagKey).to.equal('FT_READ_ONLY_ORG');
       expect(imsOrgId).to.equal('org-abc@AdobeOrg');
     });
   });
@@ -239,8 +239,8 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(result.status).to.equal(403);
       const body = await result.json();
-      expect(body.message).to.equal('Read-only admin users cannot perform write operations');
-      expect(logStub.warn.calledWithMatch('blocked from route')).to.be.true;
+      expect(body.message).to.equal('Forbidden');
+      expect(logStub.warn.calledWithMatch({ tag: 'ro-admin' }, 'Read-only admin blocked from route')).to.be.true;
       expect(handler.called).to.be.false;
     });
 
@@ -251,7 +251,7 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(result.status).to.equal(403);
       const body = await result.json();
-      expect(body.message).to.equal('Read-only admin users cannot perform write operations');
+      expect(body.message).to.equal('Forbidden');
       expect(handler.called).to.be.false;
     });
 
@@ -271,8 +271,8 @@ describe('readOnlyAdminWrapper', () => {
 
       expect(result.status).to.equal(403);
       const body = await result.json();
-      expect(body.message).to.equal('Read-only admin users cannot perform write operations');
-      expect(logStub.warn.calledWithMatch('blocked from route')).to.be.true;
+      expect(body.message).to.equal('Forbidden');
+      expect(logStub.warn.calledWithMatch({ tag: 'ro-admin' }, 'Read-only admin blocked from route')).to.be.true;
       expect(handler.called).to.be.false;
     });
 
@@ -297,36 +297,14 @@ describe('readOnlyAdminWrapper', () => {
   });
 
   describe('no routeCapabilities provided', () => {
-    let mockedWrapper;
-
-    beforeEach(async () => {
-      const ldClient = {
-        isFlagEnabledForIMSOrg: sinon.stub().resolves(true),
-      };
-      const mockedModule = await esmock('../../src/auth/read-only-admin-wrapper.js', {
-        '@adobe/spacecat-shared-launchdarkly-client': {
-          LaunchDarklyClient: {
-            createFrom: sinon.stub().returns(ldClient),
-          },
-        },
-      });
-      mockedWrapper = mockedModule.readOnlyAdminWrapper;
-    });
-
-    it('passes through RO admin when routeCapabilities is not provided', async () => {
-      const wrapped = mockedWrapper(handler);
-      const result = await wrapped({}, context);
-
-      expect(result).to.deep.equal({ status: 200 });
-      expect(handler.calledOnce).to.be.true;
+    it('throws at creation time when routeCapabilities is not provided', () => {
+      expect(() => readOnlyAdminWrapper(handler))
+        .to.throw('readOnlyAdminWrapper: routeCapabilities is required');
     });
 
-    it('passes through RO admin when routeCapabilities is null', async () => {
-      const wrapped = mockedWrapper(handler, { routeCapabilities: null });
-      const result = await wrapped({}, context);
-
-      expect(result).to.deep.equal({ status: 200 });
-      expect(handler.calledOnce).to.be.true;
+    it('throws at creation time when routeCapabilities is null', () => {
+      expect(() => readOnlyAdminWrapper(handler, { routeCapabilities: null }))
+        .to.throw('readOnlyAdminWrapper: routeCapabilities is required');
     });
   });
 
@@ -352,23 +330,23 @@ describe('readOnlyAdminWrapper', () => {
       const wrapped = mockedWrapper(handler, { routeCapabilities });
       await wrapped({}, context);
 
-      expect(logStub.info.calledWithMatch('[ro-admin-audit] RO admin accessed: GET /sites')).to.be.true;
+      expect(logStub.info.calledWithMatch({ tag: 'ro-admin-audit', method: 'GET', suffix: '/sites' }, 'RO admin accessed route')).to.be.true;
     });
 
     it('does not emit audit log for non-RO-admin requests', async () => {
       context.attributes.authInfo.isReadOnlyAdmin = () => false;
       const wrapped = mockedWrapper(handler, { routeCapabilities });
       await wrapped({}, context);
 
-      expect(logStub.info.calledWithMatch('[ro-admin-audit]')).to.be.false;
+      expect(logStub.info.calledWithMatch({ tag: 'ro-admin-audit' })).to.be.false;
     });
 
     it('does not emit audit log when RO admin is blocked', async () => {
       context.pathInfo = { method: 'POST', suffix: '/sites' };
       const wrapped = mockedWrapper(handler, { routeCapabilities });
       await wrapped({}, context);
 
-      expect(logStub.info.calledWithMatch('[ro-admin-audit]')).to.be.false;
+      expect(logStub.info.calledWithMatch({ tag: 'ro-admin-audit' })).to.be.false;
     });
   });
 });