feat(marketing): enforce fga access on marketing bff endpoints#1018
feat(marketing): enforce fga access on marketing bff endpoints#1018dealako wants to merge 12 commits into
Conversation
Add server-side authorization for the Marketing Impact and Campaigns BFF endpoints (LFXV2-2235). Previously these were UI-gated only. Changes: - Extend AccessCheckAccessType with marketing_dashboard_viewer, campaign_viewer, campaign_manager - Add Project interface fields for the marketing access probe (marketingDashboardViewer, campaignViewer, campaignManager) - Add AccessCheckService.checkMultipleAccess() for batched multi-relation checks on a single resource (avoids Map-by-id collision bug in checkAccess()) - New requireProjectAccess() middleware factory: reads foundationSlug from req.query, resolves via NATS (cached on req to prevent N+1), checks FGA relation fail-closed; gated behind MARKETING_ACCESS_ENFORCEMENT=true env flag for safe rollout - Mount marketing_dashboard_viewer on the 5 Marketing Impact analytics endpoints; campaign_viewer on GET campaign routes; campaign_manager on write routes - Wire marketing access probe through project controller and service (?marketing_access=true opt-in flag) - CampaignService sends foundationSlug on all campaign requests so the server can resolve and check the selected foundation No ED-persona fast-path — FGA relation is sole source of truth. Deploy flag-gated until tuples are confirmed across all foundations. ED-view enforcement tracked in LFXV2-2483; UI guards in LFXV2-2236. Refs: LFXV2-2235 Signed-off-by: David Deal <ddeal@linuxfoundation.org>
Replace `error: error.message` with `err: error` in warning-level catch handlers so Pino serializes the full Error object including stack trace and custom properties. Refs: LFXV2-2235 Signed-off-by: David Deal <ddeal@linuxfoundation.org>
- Fix checkMultipleAccess docstring: accurately describes positional indexing into a Record<AccessCheckAccessType, boolean> rather than the incorrect "id#access keying" claim - Add comment in requireProjectAccess explaining why pre-error WARN logs are intentional on each deny path (security audit events, not accidental double-logging) - Document in campaigns.route.ts that campaign_manager implies campaign_viewer in FGA model 14.1.0 (both resolve to executive_director or marketing_ops), preventing a theoretical create-but-cannot-poll gap Refs: LFXV2-2235 Signed-off-by: David Deal <ddeal@linuxfoundation.org>
…iant source Co-authored-by: David Deal <ddeal@linuxfoundation.org> Signed-off-by: David Deal <ddeal@linuxfoundation.org>
Signed-off-by: David Deal <ddeal@linuxfoundation.org>
- Add requireMarketingDashboardViewer to /revenue-impact and /marketing-attribution routes (missed in original pass; both back the Marketing Impact page per LFXV2-2235 plan) - Guard checkMultipleAccess against upstream result-count mismatch: log warning and fail-closed when response.results.length differs from accessTypes.length - Normalize AuthorizationError client messages to a single generic string to eliminate slug-validity and slug-existence enumeration - Fix misleading PROJECT_UID_CACHE comment (cache is per-request, not per-page-load; helps when two requireProjectAccess instances stack on the same route) Refs: LFXV2-2235 Signed-off-by: David Deal <ddeal@linuxfoundation.org>
dealako
added
the
ai-assisted
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds FGA-based access control for campaign and analytics server routes via a new ChangesMarketing Access Control — FGA gating for campaign and analytics routes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AnalyticsRoute
participant requireMarketingDashboardViewer
participant AccessCheckService
participant Backend
Client->>AnalyticsRoute: GET /web-activities-summary?foundationSlug=acme
AnalyticsRoute->>requireMarketingDashboardViewer: Check access
requireMarketingDashboardViewer->>AccessCheckService: checkSingleAccess(resource: project, relation: marketing_dashboard_viewer)
AccessCheckService->>Backend: POST /access-check
Backend-->>AccessCheckService: {granted: true}
AccessCheckService-->>requireMarketingDashboardViewer: Access granted
requireMarketingDashboardViewer->>AnalyticsRoute: next()
AnalyticsRoute->>Backend: Fetch analytics data
Backend-->>AnalyticsRoute: Data
AnalyticsRoute-->>Client: 200 with analytics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
There was a problem hiding this comment.
Pull request overview
This PR enforces Marketing-related FGA relations server-side in the Self Serve BFF (analytics “Marketing Impact” endpoints and campaign endpoints), closing a gap where access was previously gated only in the Angular UI. It also adds an opt-in project “access probe” surface for the UI to query Marketing permissions.
Changes:
- Added
requireProjectAccess(relation)middleware (flag-gated byMARKETING_ACCESS_ENFORCEMENT) and applied it to marketing analytics routes and campaign routes. - Extended shared access-check types and project response shape to support
marketing_*/campaign_*access probes (?marketing_access=true). - Updated the Angular
CampaignServiceto includefoundationSlugon campaign requests so the server can resolve the target foundation and enforce FGA.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/project.interface.ts | Adds response-only marketing access probe booleans on Project. |
| packages/shared/src/interfaces/access-check.interface.ts | Extends AccessCheckAccessType union with marketing/campaign relations. |
| apps/lfx-one/src/server/services/project.service.ts | Adds optional marketing access probe behavior when fetching a project. |
| apps/lfx-one/src/server/services/access-check.service.ts | Adds checkMultipleAccess() for checking multiple relations for one resource. |
| apps/lfx-one/src/server/routes/campaigns.route.ts | Applies project-scoped FGA middleware to campaign read/write routes. |
| apps/lfx-one/src/server/routes/analytics.route.ts | Applies marketing_dashboard_viewer middleware to marketing analytics endpoints. |
| apps/lfx-one/src/server/middleware/require-project-access.middleware.ts | Introduces new flag-gated middleware enforcing project access via slug→UID→FGA. |
| apps/lfx-one/src/server/controllers/project.controller.ts | Wires ?marketing_access=true through to ProjectService. |
| apps/lfx-one/src/app/shared/services/campaign.service.ts | Attaches foundationSlug query param to campaign requests from selected foundation context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/lfx-one/src/server/services/project.service.ts`:
- Around line 346-362: The issue is that checkMultipleAccess() may return
all-false fallback values on internal failure, which would then be assigned to
the marketing fields even though the check didn't succeed cleanly. This
contradicts the Project contract where failed checks should remain undefined. In
the getProjectById method where checkMultipleAccess is called, instead of
relying on the catch block to handle errors, you need to either use a strict
access check method that throws on failure (ensuring fields stay undefined on
any failure), or add validation logic to verify that marketingResults represents
a clean successful check before assigning the marketingDashboardViewer,
campaignViewer, and campaignManager properties, ensuring transient FGA failures
do not get serialized as definitive false values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ea9edb4-b82a-4088-a8d7-07991069a917
📒 Files selected for processing (9)
apps/lfx-one/src/app/shared/services/campaign.service.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/server/middleware/require-project-access.middleware.tsapps/lfx-one/src/server/routes/analytics.route.tsapps/lfx-one/src/server/routes/campaigns.route.tsapps/lfx-one/src/server/services/access-check.service.tsapps/lfx-one/src/server/services/project.service.tspackages/shared/src/interfaces/access-check.interface.tspackages/shared/src/interfaces/project.interface.ts
Address review comments from copilot-pull-request-reviewer and coderabbitai: - access-check.service.ts: checkMultipleAccess now throws on upstream errors and result-count mismatches instead of returning all-false fallback; probe callers (project.service.ts) already wrap in .catch() and return undefined, so fields are omitted rather than set to false on transient FGA failures (per copilot-pull-request-reviewer, coderabbitai) - require-project-access.middleware.ts: rephrase FGA deny log from "denied" to "did not grant access (denied or upstream failure)" since checkSingleAccess is fail-closed and the log fires for both denial and upstream error (per copilot-pull-request-reviewer) - campaign.service.ts: capture this.foundationSlug once per method (getMonitorData, getLinkedInAccounts, getLinkedInMonitorData, getRedditAccounts, getRedditMonitorData, getMetaAccounts, getMetaMonitorData, getKeywords, getAudience) to avoid double signal evaluation (per copilot-pull-request-reviewer) - docs/user/dashboards/index.md: fix Prettier prose-wrap formatting (line 12 exceeded printWidth:160 in CI environment) Resolves 5 review threads. Refs: LFXV2-2235 Signed-off-by: David Deal <ddeal@linuxfoundation.org>
Review Feedback AddressedCommit: $SHA Changes Made
Threads Resolved5 of 5 unresolved threads addressed in this iteration. |
Collapse multi-line throw to single line; fits within printWidth:160. Refs: LFXV2-2235 Signed-off-by: David Deal <ddeal@linuxfoundation.org>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/lfx-one/src/server/services/access-check.service.ts (1)
129-132: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the JSDoc fail-closed contract to match runtime behavior.
Line 131 says this method returns all-false on upstream errors, but Lines 171-174 and Lines 200-205 now throw. Please align the doc so callers know they must handle exceptions and deny access explicitly.
Suggested doc fix
- * Fails-closed: returns all-false on upstream error. + * Fails-closed: throws on upstream error; callers should deny access when this method errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/lfx-one/src/server/services/access-check.service.ts` around lines 129 - 132, The JSDoc comment starting around line 129 incorrectly documents that this method fails-closed by returning all-false on upstream errors, but the actual implementation at lines 171-174 and 200-205 throws exceptions instead. Update the JSDoc comment to remove or correct the fail-closed statement and add documentation that clearly indicates the method throws exceptions on upstream errors rather than returning all-false values, so callers understand they must handle exceptions and implement their own access denial logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/lfx-one/src/server/services/access-check.service.ts`:
- Around line 129-132: The JSDoc comment starting around line 129 incorrectly
documents that this method fails-closed by returning all-false on upstream
errors, but the actual implementation at lines 171-174 and 200-205 throws
exceptions instead. Update the JSDoc comment to remove or correct the
fail-closed statement and add documentation that clearly indicates the method
throws exceptions on upstream errors rather than returning all-false values, so
callers understand they must handle exceptions and implement their own access
denial logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9e4f919-bb38-4765-945e-9a03ab6e8078
📒 Files selected for processing (4)
apps/lfx-one/src/app/shared/services/campaign.service.tsapps/lfx-one/src/server/middleware/require-project-access.middleware.tsapps/lfx-one/src/server/services/access-check.service.tsdocs/user/dashboards/index.md
✅ Files skipped from review due to trivial changes (1)
- docs/user/dashboards/index.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/lfx-one/src/server/middleware/require-project-access.middleware.ts
- apps/lfx-one/src/app/shared/services/campaign.service.ts
Address review comments from copilot-pull-request-reviewer: - access-check.service.ts: update checkMultipleAccess JSDoc — method now throws on upstream error / result-count mismatch, not returns all-false; clarify callers must catch and decide how to fail (per copilot-pull-request-reviewer) - require-project-access.middleware.ts: clarify factory JSDoc — slug→UID cache is per-request (stacked middleware), not shared across parallel requests; remove misleading "parallel analytics calls" language (per copilot-pull-request-reviewer) Resolves 2 review threads. Third thread (job store IDOR) documented as pre-existing limitation, addressed in thread response. Signed-off-by: David Deal <ddeal@linuxfoundation.org>
Review Feedback AddressedCommit: bb287b8 Changes Made
Deferred / Still Open
Threads Resolved2 of 3 unresolved threads addressed in this iteration. |
…nsistency Address review comments from copilot-pull-request-reviewer: - access-check.service.ts: make checkMultipleAccess generic over T extends AccessCheckAccessType; return type is now Record<T, boolean> instead of Record<AccessCheckAccessType, boolean>, removing the unsound cast that implied every access type key was present regardless of what was requested (per copilot-pull-request-reviewer) - campaign.service.ts: capture this.foundationSlug once in lookupHubSpotUtm before building params, consistent with every other method in the service (per copilot-pull-request-reviewer) Resolves 2 review threads. Signed-off-by: David Deal <ddeal@linuxfoundation.org>
Review Feedback Addressed (iteration 2)Commit: 70620bf Changes Made
Threads Resolved2 of 2 new unresolved threads addressed in this iteration. Still Open (from prior iteration)
|
There was a problem hiding this comment.
Review — PR #1018 feat/LFXV2-2235
Verdict: PASS
Overview
Enforces FGA access server-side on Marketing Impact and Campaign BFF endpoints via a new requireProjectAccess(relation) middleware. Adds checkMultipleAccess() for multi-relation checks on a single resource. Frontend attaches foundationSlug from ProjectContextService on all marketing/campaign requests. Flag-gated behind MARKETING_ACCESS_ENFORCEMENT=true for safe rollout.
Secrets check
Clean — no credentials or API keys.
Code standards
- ✅ New interfaces in
packages/shared/src/interfaces/ - ✅ No HTML template changes — backend-only PR
- ✅ No constants in component files
FGA correctness — all clear
Access-check call shape — checkSingleAccess() and checkMultipleAccess() both POST to /access-check with the correct resourceType:id#relation shape, consistent with how AccessCheckService.checkAccess() is used elsewhere in the codebase ✅
Slug → UID resolution — Middleware validates foundationSlug against SLUG_PATTERN = /^[a-z0-9-]+$/ before resolution, type-checks the value, and validates the NATS result. Frontend sources from ProjectContextService.selectedFoundation().slug (canonical state) ✅
Fail-closed behavior — Every error path (invalid slug, slug not found, NATS failure, FGA denied, unexpected exception) calls next(AuthorizationError) → 403. Enforcement is opt-in (MARKETING_ACCESS_ENFORCEMENT=false by default), so no accidental lockout on rollout ✅
Per-request caching — req[PROJECT_UID_CACHE] uses a Symbol key scoped to each request object — no cross-request contamination or stale data ✅
checkMultipleAccess() collision fix — Old checkAccess() keyed results by resource ID, causing collisions when the same ID appeared with multiple access types. New method takes a single ID + accessTypes[] and returns Record<T, boolean> keyed by access type — no collision possible ✅
Route ordering — FGA middleware applied after auth middleware (existing pattern), so req.user is set before FGA checks run ✅
Access probe endpoint — GET /projects/:slug?marketing_access=true calls checkMultipleAccess(['marketing_dashboard_viewer', 'campaign_viewer', 'campaign_manager']) and spreads the result booleans onto the response correctly ✅
IDOR — Any user can probe any slug, but the FGA relation check is against the authenticated user server-side — not exploitable ✅
FGA efficiency note 🔵
The PR body acknowledges per-endpoint FGA calls ("optimization deferred"). This is safe and correct — the per-request Symbol cache prevents redundant slug resolutions within a single request, but each endpoint does make its own /access-check POST. Acceptable for now; a batched check or middleware-level result caching would reduce latency if multiple FGA-gated routes are hit in the same user session.
Audit: PR #1018 — feat(marketing): enforce FGA access on marketing BFF endpointsAudited head: Scope: Server-side FGA enforcement on Marketing Impact analytics + Campaign BFF routes; marketing access probe on Lanes: CI gates · manual secrets pass · architecture review · code-standards review · prior bot/human threads on head. Verdict: ✅ ApprovedMerge readiness: Safe to merge as-is. Do not set 1. Why this changeMarketing Impact and Campaigns were UI-gated only — any authenticated user could call BFF endpoints directly. This PR closes that gap with server-side FGA on the BFF, dark-deployed behind an env flag. 2. How it was done
No ED-persona fast-path on the server — FGA is the gate when enforcement is on. 3. Is it correctYes. GitHub CI on head passes (Code Quality, CodeQL, lint/analyze, license, DCO). Prior review threads (Copilot / CodeRabbit) addressed on @MRashad26 approved at 4. Gaps (non-blocking)
5. Q&A
6. Business rules & ADRs
7. Open questions (confirm)
Cursor audit — comment-only review. |
luismoriguerra
left a comment
luismoriguerra
left a comment
There was a problem hiding this comment.
Audit inline — one ops hardening note not covered in existing threads. Full report in top-level comment.
| body: request, | ||
| }); | ||
| const slug = this.foundationSlug; | ||
| const url = slug ? `/api/campaigns/brief/generate?foundationSlug=${encodeURIComponent(slug)}` : '/api/campaigns/brief/generate'; |
There was a problem hiding this comment.
Audit (non-blocking, pre-flag-flip): When selectedFoundation() is null, campaign requests proceed without foundationSlug. That is fine while MARKETING_ACCESS_ENFORCEMENT is off, but once enforcement is enabled every call here will 403. Before flipping the flag, consider failing fast client-side (or blocking calls until foundation context is set) so users get a clear UX rather than opaque 403s. Pair with LFXV2-2236 guards.
LFXV2-2235 — Enforce Marketing Ops access on Self Serve APIs (BFF)
Previously the Marketing Impact and Campaigns BFF endpoints were gated only in the Angular UI — any authenticated user could call them directly. This PR enforces the
marketing_*FGA relations server-side.Summary
requireProjectAccess(relation)factory readsfoundationSlugfromreq.query, resolves slug → project UID via NATS (cached onreqto prevent redundant lookups when stacked), posts to/access-checkon LFX_V2_SERVICE, and fails closed on any error. Gated behindMARKETING_ACCESS_ENFORCEMENT=trueenv flag for safe rollout.marketing_dashboard_vieweron all Marketing Impact analytics endpoints (/web-activities-summary,/email-ctr,/social-reach,/keyword-performance,/social-media,/social-media/monthly,/revenue-impact,/marketing-attribution);campaign_vieweron GET campaign routes;campaign_manageron write routes.GET /projects/:slug?marketing_access=truereturnsmarketingDashboardViewer,campaignViewer,campaignManagerbooleans for the UI guards in LFXV2-2236.checkMultipleAccess()— new batched method onAccessCheckServicefor multi-relation checks on a single resource; avoids the Map-by-id collision bug incheckAccess().CampaignServiceattachesfoundationSlugfromProjectContextServiceto all campaign requests so the BFF can resolve and check the selected foundation.Authorization model
FGA model 14.1.0 (LFXV2-1760):
marketing_dashboard_viewer,campaign_viewer,campaign_managerall resolve toexecutive_director or marketing_opson theprojecttype. No ED-persona fast-path — FGA relation is the sole source of truth.Scope
Deploy instructions
Do not set
MARKETING_ACCESS_ENFORCEMENT=trueuntil FGA tuples are confirmed. Before enabling:executive_director/marketing_opstuples exist for all ED and marketing users across all foundations in play (not just CNCF / test users).MARKETING_ACCESS_ENFORCEMENT=truein the Helm values for the target environment.Trade-offs documented
9f495289header is 80 chars — exceeds the 72-char style target (commitlint allows ≤100, CI will pass).require-project-access.middleware.tsis intentional new infrastructure for this feature; reviewed in this PR.checkMultipleAccessfallback — typed as fullRecord<AccessCheckAccessType, boolean>but only contains requested keys; all current consumers are safe. Deferred narrowing to follow-up./access-checkcall. Optimization deferred.event-growth/brand-reach/brand-healthnot gated — ED-dashboard routes, tracked under LFXV2-2483.Test plan
marketing_dashboard_vieweron foundation X → direct calls to/api/analytics/web-activities-summary?foundationSlug=Xreturn 403 (with enforcement on)GET /api/projects/:slug?marketing_access=truereturns the three access booleans/brief/generate,/brief/refine, etc.) requirecampaign_managerMARKETING_ACCESS_ENFORCEMENT=false(default) → all routes pass through unchangedRelated
🤖 Generated with Claude Code