feat(permissions): add POST /sites/:siteId/permissions/check/deploy - #3038
feat(permissions): add POST /sites/:siteId/permissions/check/deploy#3038ravverma wants to merge 3 commits into
Conversation
Site-scoped deploy-permission probe: returns 200 { hasPermission: true } when the
caller may deploy (apply / roll back optimizations), 403 otherwise. Intended for
UI flows that show or hide a deploy/apply control before the user acts.
- FACS-governed for both products: mapped to llmo/can_deploy and aso/can_deploy,
so facsWrapper enforces the capability for FACS-enrolled orgs (ASO via the
:siteId primary resource, LLMO via the site->brands secondary resolver).
- Internal route: absent from routeRequiredCapabilities, so it is never exposed
to S2S consumers.
- Controller adds the ReBAC-disabled fallback the wrapper leaves to it:
* LLMO -> hasLlmoCapabilityForSite (legacy isLLMOAdministrator fallback).
* ASO -> hasAccess(site, 'auto_fix') only when facs_enabled is not set.
Includes unit tests, route-wiring tests, and the OpenAPI path. Relates to LLMO-6938.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @ravverma,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - the missing x-product header validation creates a path where the permission probe returns a false-positive for non-FACS orgs.
Complexity: HIGH - medium diff; auth + ReBAC + API surface signals.
Changes: Adds a deploy-permission probe endpoint (POST /sites/:siteId/permissions/check/deploy) that UI flows call to decide whether to show deploy controls (8 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI spec). The bot review is a complement to, not a replacement for, a human read here.
Must fix before merge
- [Important] Missing x-product header validation allows a weaker permission check for non-FACS orgs -
src/controllers/sites.js:~2328(details inline)
Non-blocking (3): minor issues and suggestions
- nit: OpenAPI spec does not document x-product as a required request header parameter, so consumers reading the spec will not know to send it -
docs/openapi/site-api.yaml:25 - suggestion: The LLMO branch delegates FACS-awareness entirely to hasLlmoCapabilityForSite, while the ASO branch inlines the facs_enabled check. Consider extracting the ASO FACS+legacy check into a method on AccessControlUtil for symmetry -
src/controllers/sites.js:~2339 - suggestion: If more /permissions/check/{capability} probes are planned, define the URL scheme, response shape, and header contract now to avoid divergent one-offs
| * - ASO (and any non-LLMO product) -> when ReBAC is disabled (`facs_enabled` not set), require | ||
| * org access carrying the `auto_fix` sub-service scope — the legacy ASO deploy gate. The | ||
| * `auto_fix` scope maps to `dx_aem_perf_auto_fix`, minted only for ASO logins. | ||
| * |
There was a problem hiding this comment.
issue (blocking): When x-product is absent or unrecognized (e.g. undefined, empty, lowercase 'llmo'), the handler falls into the ASO/fallback branch and sets autoFixSubService = '' because xProduct === 'ASO' is false. This means hasAccess(site, '') performs a plain org-membership check with no capability scoping - a weaker gate than the actual deploy endpoints enforce.
The permission probe then returns { hasPermission: true } for a user who will get a 403 on the real deploy call (which enforces the header and the auto_fix scope). This creates a misleading UI state where the deploy button appears but clicking it fails.
For FACS-enrolled orgs the wrapper compensates, but for non-FACS orgs the handler IS the enforcement layer and the fallback is too permissive.
Fix: Normalize and validate at the top of the handler:
const xProduct = context.pathInfo?.headers?.[X_PRODUCT_HEADER]?.toUpperCase?.();
if (xProduct !== 'LLMO' && xProduct !== 'ASO') {
return badRequest('x-product header is required and must be LLMO or ASO');
}This aligns with the resolveSite pattern in the same controller.
|
This PR will trigger a minor release when merged. |
Address PR review (#3038): without x-product validation the ReBAC-disabled fallback fell into the ASO branch with an empty sub-service and reported hasPermission:true via a plain org-membership check — a false positive relative to the real deploy endpoints, and inconsistent with facsWrapper, which lowercases x-product before its route lookup. - Normalize x-product to upper case and require it to be LLMO or ASO (400 else). - Extract the ASO FACS + legacy deploy check into AccessControlUtil.hasAsoDeployCapabilityForSite, symmetric with hasLlmoCapabilityForSite. - Document x-product as a required header parameter in the OpenAPI spec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lucianfelix
left a comment
There was a problem hiding this comment.
As mentioned, implementation of this API is not necessary.
There was a problem hiding this comment.
Hey @ravverma,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - the prior blocking finding (x-product validation gap) is properly addressed; no new blocking issues found.
Complexity: HIGH - medium diff; auth + ReBAC + API surface signals.
Changes: Adds a deploy-permission probe endpoint with x-product validation, case normalization, and a symmetric ASO deploy-capability helper (11 files).
Note: Recommend a human read before merge - this change modifies a shared contract (OpenAPI spec). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (3): minor issues and suggestions
- suggestion:
hasAsoDeployCapabilityForSitereturnstrueunconditionally whenfacs_enabledis set, relying on the invariant that ASO's wrapper never defers (the site is the primary resource). Consider adding a defensive log.warn for the unexpected-deferral case so it fails closed rather than open if the FACS topology ever evolves -src/support/access-control-util.js:309 - nit: OpenAPI
xProductparameter enum lists values in upper case but description says case-insensitive; consumers reading the schema strictly may not realize lowercase is accepted -docs/openapi/parameters.yaml:430 - suggestion: The ASO 403 message is a static string while the LLMO branch uses a FACS-aware
llmoForbiddenMessage()helper. Consider a symmetricasoForbiddenMessage()so FACS-enrolled users get a contextual denial reason -src/controllers/sites.js:2353
Previously flagged, now resolved
- x-product header validation now required and normalized (commit 629e3ae); the false-positive permission probe for non-FACS orgs is eliminated
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 2s | Cost: $8.66 | Commit: 629e3ae6e06f4877645076c1678ef5138a17ad75
If this code review was useful, please react with 👍. Otherwise, react with 👎.
What
Adds a new internal endpoint
POST /sites/:siteId/permissions/check/deploy— a site-scoped probe for the "deploy" capability (apply / roll back optimizations). It returns200 { "hasPermission": true }when the caller may deploy and403otherwise, so UI flows can show/hide a deploy control before the user acts.Authorization
The route is FACS-governed for both products — mapped to
llmo/can_deployandaso/can_deployinfacs-capabilities.js. For FACS‑enrolled orgs (ReBAC enabled)facsWrapperenforcescan_deploybefore the handler runs (ASO via the:siteIdprimary resource, LLMO via the site→brands secondary resolver — established in #3030), so a200already means the caller is authorized.The controller only adds the ReBAC‑disabled fallback the wrapper leaves to it for non‑FACS orgs:
hasLlmoCapabilityForSite(site)(falls back to the legacyisLLMOAdministrator()claim).hasAccess(site, 'auto_fix')only whenfacs_enabledis not set (the legacy ASO deploy gate;auto_fixmaps todx_aem_perf_auto_fix, minted only for ASO logins). This mirrors thefacs_enabledgate already used byedge-routing-auth/hasLlmoCapabilityForSite.It is an internal route — intentionally absent from
routeRequiredCapabilities(INTERNAL_ROUTES), so it is never exposed to S2S consumers.Changes
src/controllers/sites.js— newcheckDeployPermissionmethod (+ importX_PRODUCT_HEADER; removed a now‑duplicate localX_PRODUCT_HEADERinresolveSite).src/routes/index.js— route → handler.src/routes/facs-capabilities.js—llmo/can_deploy+aso/can_deploy.src/routes/required-capabilities.js— added toINTERNAL_ROUTES.docs/openapi/{site-api,api}.yaml— OpenAPI path (docs/index.htmlis gitignored; regenerate vianpm run docs:build).Testing
test/controllers/sites.test.js): LLMO grant/deny, ASO ReBAC‑disabled grant/deny (asserts theauto_fixscope), ASOfacs_enabledbypass (asserts the legacy check does not run), no‑header default, 404.test/routes/index.test.js) + FACS union‑equality / required‑capabilities coverage invariants pass.npm run lint,npm run type-check,npm run docs:lint(valid, 0 new warnings) all green.Relates to LLMO-6938.
🤖 Generated with Claude Code