feat: tier-aware auto_fix scope gate for EDS autofix trigger #2267
feat: tier-aware auto_fix scope gate for EDS autofix trigger #2267
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@mimchome Please hold these PRs until we conclude on thread. |
solaris007
left a comment
There was a problem hiding this comment.
Hey @mimchome,
Strengths
- Correct architectural direction: moving the scope gate from login.js to the API layer enables per-site differentiation that the blanket grant could never support (
suggestions.js:1044-1058). - Fail-closed semantics throughout: missing tenant, unknown tier, empty profile, or absent
tenantsarray all collapse to requiringauto_fixscope - the stricter path (suggestions.js:1053-1055). - Uses
DELIVERY_TYPES.AEM_EDGEconstant for delivery type comparison, matching codebase conventions. - Good test matrix covering EDS/non-EDS x paid/free-tier combinations, plus the missing-email audit log edge case.
- Tier data comes from the signed JWT (not caller-controllable) and site-org identification is server-authoritative from the DB.
Issues
Important (Should Fix)
1. Unguarded siteOrg/getImsOrgId() dereference - suggestions.js:1052-1053
site.getOrganization() can return null (deleted org, dangling FK, provisioning race). The next line throws a TypeError, returning a 500 instead of the intended 403. Before this PR, autofixSuggestions did not call getOrganization(), so this is a new failure mode on a live auth path. Fix: add a null guard and a corresponding test.
const siteOrg = await site.getOrganization();
if (!siteOrg?.getImsOrgId()) {
return forbidden('User does not belong to the organization or does not have sufficient permissions');
}2. Hardcoded tier string literals - suggestions.js:1056
['FREE_TRIAL', 'PLG'] duplicates constants that exist as EntitlementModel.TIERS.FREE_TRIAL and EntitlementModel.TIERS.PLG in @adobe/spacecat-shared-data-access, used consistently elsewhere in this repo (e.g. access-control-util.js:151, utils.js:1572). If data-access renames a tier, every other caller updates through the type while this one silently diverges - and the fail-closed default means PLG users quietly lose the bypass without an error.
3. EDS tier gate can be bypassed via cross-org scope - suggestions.js:1057-1058
For EDS sites, subService='auto_fix' always, and hasAccess delegates to hasScope('user', 'dx_aem_perf_auto_fix'). But hasScope does not filter by tenant domain - it passes if any tenant contributed auto_fix. A user who is FREE_TRIAL in org C but PAID (with auto_fix) in org B can trigger EDS autofix on org C's sites. Since this PR's intent is tier-aware gating for EDS, consider adding an explicit pre-check:
if (isEdsSite && isFreeTierCaller) {
return forbidden('EDS autofix not available for free-tier organizations');
}If the cross-org behavior is intentional, add a comment documenting that so future readers don't assume the gate is site-org-tight.
4. Missing PLG tier test - test/controllers/suggestions.test.js
The code gates on ['FREE_TRIAL', 'PLG'] but only FREE_TRIAL is tested. A typo or case mismatch on 'PLG' would ship undetected. Add a parallel test with tier: 'PLG' on a non-EDS site.
5. Missing cross-tenant mismatch test - test/controllers/suggestions.test.js
When a caller's tenants exist but none match orgIdent, callerTenant is undefined, isFreeTierCaller is false, and the gate falls to requiring auto_fix. This is the security-relevant "fail-closed" branch and it has no test. Add a test with profile.tenants = [{ id: 'OTHERORG', entitlement: { tier: 'FREE_TRIAL' } }] asserting 403.
Minor (Nice to Have)
6. Audit log missing decision context - suggestions.js:1294-1300
The [autofix-triggered] log answers who/what/when but not which path the gate took. Adding deliveryType and callerTier would make the free-tier bypass directly observable in Coralogix without a join.
7. Audit log fires before SQS dispatch completes - suggestions.js:1294
[autofix-triggered] fires before the Promise.all that dispatches grouped SQS messages. If dispatch throws, the log claims a trigger that never completed. Consider moving it after successful dispatch, or renaming to [autofix-accepted].
Recommendations
- Add an
[autofix-denied]log on theforbidden(...)branch withsiteId,opportunityId, andtriggeredBy. This controller is now the last line of defense for FREE_TRIAL users on EDS - denied attempts should leave a structured trace for monitoring. - Once a second endpoint needs the "paid users need scope X, free-tier users pass on org membership" pattern, extract it into
AccessControlUtilrather than duplicating the controller block. - Consider using
profile.sub(IMS user ID) instead of email in the audit log to reduce PII exposure in Coralogix shared storage.
Assessment
Ready to merge? With fixes.
The architectural direction is correct and the fail-closed semantics are sound. The null guard (finding 1) is the highest priority - it is a real 500 risk on a live auth path. The tier constants (finding 2) and missing tests (findings 4-5) are cheap fixes that prevent silent breakage. The cross-org scope question (finding 3) deserves a deliberate answer (tighten or document) before this ships.
Next Steps
- Add the null guard for
siteOrg/getImsOrgId()and a corresponding test. - Replace hardcoded tier strings with
EntitlementModel.TIERSconstants. - Decide on cross-org EDS behavior: add a pre-check or document the intent.
- Add PLG tier and cross-tenant mismatch tests.
- Minor items and recommendations are optional.
| // access via org membership — AEM CS has its own apply-time auth, so the scope gate | ||
| // there is only enforced for paid users as before. | ||
| const isEdsSite = site.getDeliveryType() === DELIVERY_TYPES.AEM_EDGE; | ||
| const siteOrg = await site.getOrganization(); |
There was a problem hiding this comment.
Unguarded dereference - site.getOrganization() can return null (deleted org, dangling FK). Next line throws TypeError -> 500 instead of 403. This is a new failure mode introduced by this PR (the prior code did not call getOrganization() on this path). Add a null guard:
const siteOrg = await site.getOrganization();
if (!siteOrg?.getImsOrgId()) {
return forbidden('User does not belong to the organization or does not have sufficient permissions');
}Also add a test for getOrganization() returning null.
| const orgIdent = siteOrg.getImsOrgId().split('@')[0]; | ||
| const callerTenant = context.attributes?.authInfo?.getProfile?.()?.tenants | ||
| ?.find((t) => t.id === orgIdent); | ||
| const isFreeTierCaller = ['FREE_TRIAL', 'PLG'].includes(callerTenant?.entitlement?.tier); |
There was a problem hiding this comment.
Hardcoded tier literals - Should use EntitlementModel.TIERS.FREE_TRIAL and EntitlementModel.TIERS.PLG from @adobe/spacecat-shared-data-access, matching the pattern elsewhere in this repo (e.g. access-control-util.js:151, utils.js:1572). Silent divergence risk if a tier is renamed.
| const callerTenant = context.attributes?.authInfo?.getProfile?.()?.tenants | ||
| ?.find((t) => t.id === orgIdent); | ||
| const isFreeTierCaller = ['FREE_TRIAL', 'PLG'].includes(callerTenant?.entitlement?.tier); | ||
| const subService = (isEdsSite || !isFreeTierCaller) ? 'auto_fix' : ''; |
There was a problem hiding this comment.
Cross-org scope gap - For EDS, subService='auto_fix' always. But hasScope does not filter by tenant domain, so a user who is FREE_TRIAL in this org but PAID in another org passes via the other org's scope. If intent is tier-tight gating for EDS, add:
if (isEdsSite && isFreeTierCaller) {
return forbidden('EDS autofix not available for free-tier organizations');
}before the hasAccess check. If cross-org is intentional, document it in the comment block.
https://jira.corp.adobe.com/browse/SITES-42941
Summary
Moves tier-based autofix enforcement from login.js to the API layer so it can be
site-aware. This is the only place
auto_fixscope is checked in spacecat-api-service.autofixSuggestions): EDS sites now require theauto_fixscope inand PLG users on non-EDS are passed through on org membership only — preserving their
existing AEM CS autofix access, which previously worked because login.js auto-granted
the scope at login.
[autofix-triggered]log entry withsiteId,opportunityId,action,suggestionCount, andtriggeredBy(caller email from JWT profile).Relation to auth-service
The token-side change lives in
spacecat-auth-servicefeat/eds-credential-api-autofix-scope(separate PR). That PR removes
auto_fixfrom FREE_TRIAL/PLG tokens at login — withoutit, this scope gate would never block free-tier users on EDS since they'd still carry
the scope. The two PRs are designed to ship together, with the auth-service change going
first.
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!