Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
966 changes: 815 additions & 151 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@
"@adobe/spacecat-shared-drs-client": "1.4.1",
"@adobe/spacecat-shared-data-access-v2": "npm:@adobe/spacecat-shared-data-access@2.109.0",
"@adobe/spacecat-shared-gpt-client": "1.6.20",
"@adobe/spacecat-shared-http-utils": "1.25.1",
"@adobe/spacecat-shared-http-utils": "https://gist.github.com/ravverma/73fbd39cad0536f360791528e21c3857/raw/c039c780f04856012e2c2d7382a4ac7702965fbe/adobe-spacecat-shared-http-utils-1.25.1.tgz",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: This resolves @adobe/spacecat-shared-http-utils from a personal GitHub Gist instead of npm. The companion PR (adobe/spacecat-shared#1469) must be merged and published to npm first, then this should reference the published version. Gist-hosted tarballs bypass all supply chain controls (npm 2FA, provenance attestation, CI). The tarball also masquerades as version 1.25.1 while containing different code.

"@adobe/spacecat-shared-ims-client": "1.12.2",
"@adobe/spacecat-shared-rum-api-client": "2.40.10",
"@adobe/spacecat-shared-scrape-client": "2.6.0",
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/audits.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ function AuditsController(ctx) {
return badRequest('Audit type required');
}

if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Admin access required');
}

Expand Down
4 changes: 2 additions & 2 deletions src/controllers/configuration.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ function ConfigurationController(ctx) {
* @return {Promise<Response>} Configuration response.
*/
const getByVersion = async (context) => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view configurations');
}
const configurationVersion = context.params?.version;
Expand All @@ -79,7 +79,7 @@ function ConfigurationController(ctx) {
* @return {Promise<Response>} Configuration response.
*/
const getLatest = async () => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view configurations');
}
const configuration = await Configuration.findLatest();
Expand Down
4 changes: 2 additions & 2 deletions src/controllers/organizations.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function OrganizationsController(ctx, env) {
* @returns {Promise<Response>} Array of organizations response.
*/
const getAll = async () => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view all Organizations');
}

Expand Down Expand Up @@ -150,7 +150,7 @@ function OrganizationsController(ctx, env) {
* @throws {Error} If IMS org ID is not provided, org not found, or Slack config not found.
*/
const getSlackConfigByImsOrgID = async (context) => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view Slack configurations');
}
const response = await getByImsOrgID(context);
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/plg/plg-onboarding.js
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@ function PlgOnboardingController(ctx) {

// Admin/API key holders can access any org's status
const accessControlUtil = AccessControlUtil.fromContext(context);
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
// Non-admin: validate caller's IMS tenant matches requested imsOrgId
const profile = authInfo.getProfile();

Expand Down
2 changes: 1 addition & 1 deletion src/controllers/project.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ function ProjectsController(ctx, env) {
* @returns {Promise<Response>} Array of projects response.
*/
const getAll = async () => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view all projects');
}

Expand Down
10 changes: 5 additions & 5 deletions src/controllers/sites.js
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ function SitesController(ctx, log, env) {
* @returns {Promise<Response>} Array of sites response.
*/
const getAll = async () => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view all sites');
}

Expand All @@ -343,7 +343,7 @@ function SitesController(ctx, log, env) {
* @returns {Promise<Response>} Array of sites response.
*/
const getAllByDeliveryType = async (context) => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view all sites');
}
const deliveryType = context.params?.deliveryType;
Expand All @@ -367,7 +367,7 @@ function SitesController(ctx, log, env) {
* @return {Promise<Response>} Array of sites response.
*/
const getAllWithLatestAudit = async (context) => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view all sites');
}
const auditType = context.params?.auditType;
Expand All @@ -393,7 +393,7 @@ function SitesController(ctx, log, env) {
* @returns {Promise<Response>} XLS file.
*/
const getAllAsXLS = async () => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view all sites');
}
const sites = await Site.all();
Expand All @@ -405,7 +405,7 @@ function SitesController(ctx, log, env) {
* @returns {Promise<Response>} CSV file.
*/
const getAllAsCSV = async () => {
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
return forbidden('Only admins can view all sites');
}
const sites = await Site.all();
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/user-details.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function UserDetailsController(ctx) {
*/
const fetchFromImsIfAdmin = async (externalUserId, organizationId) => {
// Check if requestor has admin access
if (!accessControlUtil.hasAdminAccess()) {
if (!accessControlUtil.hasAdminReadAccess()) {
log.debug(`User is not admin, returning system defaults for ${externalUserId}`);
return {
firstName: 'system',
Expand Down
5 changes: 5 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
AdobeImsHandler,
JwtHandler,
s2sAuthWrapper,
readOnlyAdminWrapper,
} from '@adobe/spacecat-shared-http-utils';
import AuthInfo from '@adobe/spacecat-shared-http-utils/src/auth/auth-info.js';
import { imsClientWrapper } from '@adobe/spacecat-shared-ims-client';
Expand Down Expand Up @@ -315,7 +316,11 @@ const { WORKSPACE_EXTERNAL } = SLACK_TARGETS;
// Wrapper execution order (helix-shared-wrap: last .with() = outermost = runs first):
// 1. s2sAuthWrapper — intercepts S2S JWT bearer tokens, passes through non-S2S to authWrapper
// 2. authWrapper — handles JWT, IMS, scoped API key, legacy API key
// 3. readOnlyAdminWrapper — enforces read-only access for read-only admin tokens (see
// adobe/spacecat-shared#1469); routes not present in routeCapabilities default to deny
// (fail-closed), so unmapped routes are blocked for read-only admins
const wrappedMain = wrap(run)
.with(readOnlyAdminWrapper, { routeCapabilities: routeRequiredCapabilities })
.with(authWrapper, {
authHandlers: [JwtHandler, AdobeImsHandler, ScopedApiKeyHandler, LegacyApiKeyHandler],
})
Expand Down
20 changes: 12 additions & 8 deletions src/routes/required-capabilities.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ export const INTERNAL_ROUTES = [
'POST /consumers/register',
'PATCH /consumers/:consumerId',
'POST /consumers/:consumerId/revoke',

// API Keys - scoped API key management; end-user/admin flow, not exposed to S2S consumers
'POST /tools/api-keys',
'DELETE /tools/api-keys/:id',
'GET /tools/api-keys',
];

/**
Expand Down Expand Up @@ -356,11 +361,6 @@ const routeRequiredCapabilities = {
// Trigger — GET triggers side effect; consider POST for RFC 7231 semantics (follow-up)
'GET /trigger': 'audit:write',

// API Keys
'POST /tools/api-keys': 'apiKey:write',
'DELETE /tools/api-keys/:id': 'apiKey:write',
'GET /tools/api-keys': 'apiKey:read',

// Import Jobs
'POST /tools/import/jobs': 'importJob:write',
'GET /tools/import/jobs/:jobId': 'importJob:read',
Expand Down Expand Up @@ -400,9 +400,13 @@ const routeRequiredCapabilities = {
'GET /sites/:siteId/llmo/sheet-data/:dataSource': 'site:read',
'GET /sites/:siteId/llmo/sheet-data/:sheetType/:dataSource': 'site:read',
'GET /sites/:siteId/llmo/sheet-data/:sheetType/:week/:dataSource': 'site:read',
'POST /sites/:siteId/llmo/sheet-data/:dataSource': 'site:write',
'POST /sites/:siteId/llmo/sheet-data/:sheetType/:dataSource': 'site:write',
'POST /sites/:siteId/llmo/sheet-data/:sheetType/:week/:dataSource': 'site:write',
// These POST sheet-data routes use POST only to accommodate complex query payloads that exceed
// URL length limits. They are non-mutating (no side effects) and intentionally require
// only site:read, which also allows read-only admins and S2S consumers with read-only tokens
// to query sheet data.
'POST /sites/:siteId/llmo/sheet-data/:dataSource': 'site:read',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: These POST routes were downgraded from site:write to site:read. This means (a) read-only admins can call them, and (b) S2S consumers with site:read can now call them. Please confirm these endpoints are truly non-mutating (POST used for complex query payloads, no side effects) and add a code comment explaining the pattern.

'POST /sites/:siteId/llmo/sheet-data/:sheetType/:dataSource': 'site:read',
'POST /sites/:siteId/llmo/sheet-data/:sheetType/:week/:dataSource': 'site:read',
'GET /sites/:siteId/llmo/data': 'site:read',
'GET /sites/:siteId/llmo/data/:dataSource': 'site:read',
'GET /sites/:siteId/llmo/data/:sheetType/:dataSource': 'site:read',
Expand Down
11 changes: 9 additions & 2 deletions src/support/access-control-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ export default class AccessControlUtil {
return this.authInfo.isAdmin();
}

hasAdminReadAccess() {
return this.hasAdminAccess() || this.authInfo.isReadOnlyAdmin?.() === true;
}

hasS2SAdminAccess() {
return this.authInfo.isS2SAdmin();
}
Expand Down Expand Up @@ -157,8 +161,11 @@ export default class AccessControlUtil {
}

const { authInfo } = this;
// Check admin access first - admins bypass product code validation
if (this.hasAdminAccess()) {
// Full admins and read-only admins both bypass org/product validation for data reads.
// Write operations are protected separately: the readOnlyAdminWrapper middleware blocks
// all non-GET requests for read-only admin tokens before they reach any controller,
// so a read-only admin can never mutate data even though hasAccess() returns true here.
if (this.hasAdminReadAccess()) {
return true;
}

Expand Down
23 changes: 23 additions & 0 deletions test/controllers/audits.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,29 @@ describe('Audits Controller', () => {
const result = await controller.getAllLatest({ params: { auditType } });
expect(result.status).to.equal(403);
});

it('allows read-only admin to get all latest audits', async () => {
const authContextReadOnlyAdmin = {
attributes: {
authInfo: new AuthInfo()
.withType('jwt')
.withScopes([{ name: 'user' }])
.withProfile({ is_admin: false, is_read_only_admin: true })
.withAuthenticated(true),
},
};
const controller = AuditsController({
dataAccess: mockDataAccess,
pathInfo: { headers: { 'x-product': 'llmo' } },
...authContextReadOnlyAdmin,
});

const auditType = 'security';
mockDataAccess.LatestAudit.allByAuditType.resolves(mockLatestAudits);

const result = await controller.getAllLatest({ params: { auditType } });
expect(result.status).to.equal(200);
});
});

describe('getAllLatestForSite', () => {
Expand Down
8 changes: 8 additions & 0 deletions test/controllers/configurations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,14 @@ describe('Configurations Controller', () => {
expect(error.message).to.include('Configuration data validation failed');
});

it('gets latest configuration for read-only admin', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });

const result = await configurationsController.getLatest();

expect(result.status).to.equal(200);
});

it('gets latest configuration for non admin users', async () => {
context.attributes.authInfo.withProfile({ is_admin: false });
const result = await configurationsController.getLatest();
Expand Down
23 changes: 23 additions & 0 deletions test/controllers/organizations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,18 @@ describe('Organizations Controller', () => {
expect(error).to.have.property('message', 'Only admins can create new Organizations');
});

it('returns forbidden for read-only admin when creating an organization', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });
const controller = OrganizationsController(context, env);
const response = await controller.createOrganization({
data: { name: 'Org 1' },
...context,
});
expect(response.status).to.equal(403);
const error = await response.json();
expect(error).to.have.property('message', 'Only admins can create new Organizations');
});

it('returns bad request when creating an organization fails', async () => {
mockDataAccess.Organization.create.rejects(new Error('Failed to create organization'));
const response = await organizationsController.createOrganization({
Expand Down Expand Up @@ -453,6 +465,17 @@ describe('Organizations Controller', () => {
expect(resultOrganizations[1]).to.have.property('id', '5f3b3626-029c-476e-924b-0c1bba2e871f');
});

it('gets all organizations for read-only admin', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });
mockDataAccess.Organization.all.resolves(organizations);

const result = await organizationsController.getAll();

expect(result.status).to.equal(200);
const resultOrgs = await result.json();
expect(resultOrgs).to.be.an('array').with.lengthOf(4);
});

it('gets all organizations for non admin users', async () => {
context.attributes.authInfo.withProfile({ is_admin: false });
mockDataAccess.Organization.all.resolves(organizations);
Expand Down
4 changes: 2 additions & 2 deletions test/controllers/plg/plg-onboarding.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ describe('PlgOnboardingController', () => {
},
'../../../src/support/access-control-util.js': {
default: {
fromContext: () => ({ hasAdminAccess: () => false }),
fromContext: () => ({ hasAdminAccess: () => false, hasAdminReadAccess: () => false }),
},
},
},
Expand Down Expand Up @@ -1799,7 +1799,7 @@ describe('PlgOnboardingController', () => {
},
'../../../src/support/access-control-util.js': {
default: {
fromContext: () => ({ hasAdminAccess: () => true }),
fromContext: () => ({ hasAdminAccess: () => true, hasAdminReadAccess: () => true }),
},
},
},
Expand Down
31 changes: 31 additions & 0 deletions test/controllers/project.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,18 @@ describe('Projects Controller', () => {
expect(responseBody.message).to.equal('Only admins can create new projects');
});

it('should return forbidden for read-only admin users', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });
const response = await ProjectsController(context, { TEST_ENV: 'true' }).createProject({
data: { projectName: 'New Project', organizationId: '9033554c-de8a-44ac-a356-09b51af8cc28' },
...context,
});

expect(response.status).to.equal(403);
const responseBody = await response.json();
expect(responseBody.message).to.equal('Only admins can create new projects');
});

it('should return bad request when creation fails', async () => {
mockDataAccess.Project.create.rejects(new Error('Validation failed'));
const response = await projectsController.createProject({
Expand Down Expand Up @@ -289,6 +301,13 @@ describe('Projects Controller', () => {
const responseBody = await response.json();
expect(responseBody.message).to.equal('Only admins can view all projects');
});

it('should allow read-only admin to get all projects', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });
const response = await projectsController.getAll(context);

expect(response.status).to.equal(200);
});
});

describe('getByID', () => {
Expand Down Expand Up @@ -378,6 +397,18 @@ describe('Projects Controller', () => {
expect(responseBody.message).to.equal('Only admins can delete projects');
});

it('should return forbidden for read-only admin users', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });
const response = await ProjectsController(context, { TEST_ENV: 'true' }).removeProject({
params: { projectId: '550e8400-e29b-41d4-a716-446655440000' },
...context,
});

expect(response.status).to.equal(403);
const responseBody = await response.json();
expect(responseBody.message).to.equal('Only admins can delete projects');
});

it('should return bad request for invalid project ID', async () => {
const response = await projectsController.removeProject({
params: { projectId: 'invalid-id' },
Expand Down
21 changes: 21 additions & 0 deletions test/controllers/sites.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,16 @@ describe('Sites Controller', () => {
expect(error).to.have.property('message', 'Only admins can create new sites');
});

it('returns forbidden for read-only admin when creating a site', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });
const response = await sitesController.createSite({ data: { baseURL: 'https://site1.com' } });

expect(mockDataAccess.Site.create).to.have.not.been.called;
expect(response.status).to.equal(403);
const error = await response.json();
expect(error).to.have.property('message', 'Only admins can create new sites');
});

it('returns bad request when creating a site without baseURL', async () => {
const response = await sitesController.createSite({ data: {} });

Expand Down Expand Up @@ -551,6 +561,17 @@ describe('Sites Controller', () => {
expect(resultSites[0]).to.not.have.any.keys('hlxConfig', 'authoringType', 'deliveryConfig', 'pageTypes', 'projectId', 'isPrimaryLocale', 'language', 'code', 'audits', 'updatedBy', 'isLiveToggledAt');
});

it('gets all sites for a read-only admin user', async () => {
context.attributes.authInfo.withProfile({ is_admin: false, is_read_only_admin: true });
mockDataAccess.Site.all.resolves(sites);

const result = await sitesController.getAll();
const resultSites = await result.json();

expect(result.status).to.equal(200);
expect(resultSites).to.be.an('array').with.lengthOf(2);
});

it('gets all sites for a non-admin user', async () => {
context.attributes.authInfo.withProfile({ is_admin: false });
mockDataAccess.Site.all.resolves(sites);
Expand Down
Loading
Loading