Skip to content

feat: tier-aware auto_fix scope gate for EDS autofix trigger #2267

Open
mimchome wants to merge 2 commits intomainfrom
feat/eds-autofix-scope-gate
Open

feat: tier-aware auto_fix scope gate for EDS autofix trigger #2267
mimchome wants to merge 2 commits intomainfrom
feat/eds-autofix-scope-gate

Conversation

@mimchome
Copy link
Copy Markdown
Contributor

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_fix scope is checked in spacecat-api-service.

  • Scope gate (autofixSuggestions): EDS sites now require the auto_fix scope in
    and 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.
  • Audit log: Adds [autofix-triggered] log entry with siteId, opportunityId,
    action, suggestionCount, and triggeredBy (caller email from JWT profile).

Relation to auth-service

The token-side change lives in spacecat-auth-service feat/eds-credential-api-autofix-scope
(separate PR). That PR removes auto_fix from FREE_TRIAL/PLG tokens at login — without
it, 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:

  • make sure you update the API specification and the examples to reflect the changes.

If the PR is introducing a new audit type:

  • make sure you update the API specification with the type, schema of the audit result and an example

Related Issues

Thanks for contributing!

@mimchome mimchome requested a review from solaris007 April 24, 2026 09:57
@mimchome mimchome self-assigned this Apr 24, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ravverma
Copy link
Copy Markdown
Contributor

@mimchome Please hold these PRs until we conclude on thread.
cc: @solaris007 (from review pov)

Copy link
Copy Markdown
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

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

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 tenants array all collapse to requiring auto_fix scope - the stricter path (suggestions.js:1053-1055).
  • Uses DELIVERY_TYPES.AEM_EDGE constant 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 the forbidden(...) branch with siteId, opportunityId, and triggeredBy. 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 AccessControlUtil rather 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

  1. Add the null guard for siteOrg/getImsOrgId() and a corresponding test.
  2. Replace hardcoded tier strings with EntitlementModel.TIERS constants.
  3. Decide on cross-org EDS behavior: add a pre-check or document the intent.
  4. Add PLG tier and cross-tenant mismatch tests.
  5. 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();
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.

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);
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.

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' : '';
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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants