Conversation
PR SummaryMedium Risk Overview A new Reviewed by Cursor Bugbot for commit a0bd478. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Concurrent cache misses duplicate token exchanges, and the failure test does not reliably exercise the member-tiers error path.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Replaces the member-tier stub with an authenticated Heimdall API call using cached M2M credentials.
Changes:
- Adds client-credentials token acquisition and caching.
- Calls the live member-tiers endpoint.
- Adds environment configuration and tests.
| File | Description |
|---|---|
workers/api-gateway/src/m2m.ts |
Implements M2M token retrieval and caching. |
workers/api-gateway/src/tiers.ts |
Calls member-tiers with the M2M token. |
workers/api-gateway/src/env.ts |
Defines required M2M environment variables. |
workers/api-gateway/tests/tiers.test.ts |
Tests token reuse and API failures. |
workers/api-gateway/tests/gateway.test.ts |
Updates the gateway test environment. |
workers/api-gateway/README.md |
Documents removal of the member-tier stub. |
workers/api-gateway/.dev.vars.example |
Adds local M2M configuration placeholders. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6bd79b1 to
76ab0a0
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Tier selection mishandles the upstream taxonomy, and concurrent cache misses can trigger redundant token requests.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
76ab0a0 to
62475ec
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The token refresh boundary needs automated coverage before the authentication cache can be safely approved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (1)
themarolt
left a comment
There was a problem hiding this comment.
looks good - one question on how member-tiers answers for users without a membership
| const response = await fetch(url, { | ||
| headers: { authorization: `Bearer ${await m2mToken(env)}` }, | ||
| }); | ||
| if (!response.ok) throw new Error(`member-tiers request failed: ${response.status}`); |
There was a problem hiding this comment.
what does member-tiers return for a user with no membership - 200 [] or a 404? if it's a 404, this throws and the worker answers 500 instead of the forbidden() 403 in handle, or am I missing something?
There was a problem hiding this comment.
member-tiers answers 200 [] for users without a membership (unknown users too), so this lands on forbidden()
gaspergrom
left a comment
There was a problem hiding this comment.
Traced the token cache and the outbound URL. Reuse works and the username gets encoded correctly. Two things worth resolving before this merges, both about what happens when the upstream call isn't a clean success.
| const response = await fetch(url, { | ||
| headers: { authorization: `Bearer ${await m2mToken(env)}` }, | ||
| }); | ||
| if (!response.ok) throw new Error(`member-tiers request failed: ${response.status}`); |
There was a problem hiding this comment.
What does member-tiers return for a user with no membership, a 200 with an empty array or a 404? If it can 404, this throw has nothing catching it in handle(), so the worker would answer with a raw 500 instead of the 403 that forbidden() already builds for exactly that case.
There was a problem hiding this comment.
member-tiers answers 200 [] for users without a membership (unknown users too), so this lands on forbidden()
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
Signed-off-by: anilb <epipav@gmail.com>
62475ec to
9fa13d4
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The tests do not verify the client-credentials request payload that authorizes the new integration.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (1)
Signed-off-by: anilb <epipav@gmail.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Repository workflow requires an IN-### JIRA reference in the PR body or commit history.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: None
Resolved since last review (1)
gaspergrom
left a comment
There was a problem hiding this comment.
Approving now, thanks for the update.



Replaces the member-tiers stub with the real call:
GET <LFX_API_URL>b2b_orgs/member-tiers/{username}?v=1through Heimdall, authorized with a client-credentials token that is reused until shortly before it expires.LFX_API_URL,M2M_ISSUER_URL,M2M_AUDIENCE,M2M_CLIENT_ID,M2M_CLIENT_SECRET.memberonteam:member_tiers_callerin OpenFGA.Stacked on #2310; retargets to
mainonce it merges.main