feat(fga): support routing FGA calls to an FGA cache instance - #349
Conversation
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Adds Config.fgaCacheUrl / DESCOPE_FGA_CACHE_URL and a single ManagementsBase.getFgaUri helper that routes six FGA/authz calls to an FGA cache host, leaving everything else on the base URL.
Sniffed out 0 issues. Good bones! 🦴
Verified: trailing-slash trim produces correct URIs, blank/null cache URL falls back to getUri, the auth header in AbstractProxyImpl.addHeaders is URI-independent so routing doesn't affect auth, and the six routed call sites match the README. New tests' Mockito stubs are all consumed under strict stubs.
Woof!
Declared coverage: FULL — 11/11 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds Config.fgaCacheUrl / DESCOPE_FGA_CACHE_URL plus a single ManagementsBase.getFgaUri helper that routes six FGA/authz calls to an FGA cache host, leaving everything else on the base URL.
Sniffed out 1 issue:
- 1 🟢 LOW: new field inserted mid-class changes the Lombok all-args constructor signature
Verified the six routed call sites match the README, trailing-slash trimming produces correct URIs, blank/null cache URL falls back to getUri, and the Bearer <projectId>:<managementKey> header in AbstractProxyImpl.addHeaders is URI-independent so routing can't affect auth. Good bones! Woof!
Declared coverage: FULL — 11/11 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds Config.fgaCacheUrl / DESCOPE_FGA_CACHE_URL and a single ManagementsBase.getFgaUri helper that routes six FGA/authz calls to an FGA cache host, leaving everything else on the base URL.
Sniffed out 0 issues. Good bones! 🦴
Re-checked the earlier constructor-compat concern: fgaCacheUrl/fgaCacheUri are now the last fields, the explicit legacy-arity constructors don't clash with the Lombok all-args ones, and @Builder.build() still binds to the @AllArgsConstructor signature in field order — so positional callers keep compiling. Also verified trailing-slash trimming, the blank/null fallback to getUri, URI-independent auth headers, and the routed call sites matching the README.
Woof!
Declared coverage: FULL — 11/11 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds Config.fgaCacheUrl / DESCOPE_FGA_CACHE_URL and a single ManagementsBase.getFgaUri helper that routes six FGA/authz calls to an FGA cache host, leaving everything else on the base URL.
Sniffed out 0 issues. Good bones! 🦴
Verified this pass: the six routed call sites match the README (FGA saveSchema/createRelations/deleteRelations/check, authz whoCanAccess/whatCanTargetAccess), route constants match the URLs asserted in the new tests, trailing-slash trimming and the blank/null fallback to getUri both behave, AbstractProxyImpl.addHeaders builds the bearer header independently of the URI so routing can't affect auth, and DescopeClient.getClient is the only Client.builder() in main so the new field can't be silently dropped. The earlier constructor-compat concern stays resolved — fgaCacheUrl/fgaCacheUri are last, the explicit legacy-arity constructors don't clash with the Lombok all-args ones, and build() still binds to the all-args signature.
Woof!
Declared coverage: FULL — 11/11 changed files reviewed.
yosiharan
left a comment
yosiharan
left a comment
There was a problem hiding this comment.
self review done
89e117e to
74700e2
Compare
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds Config.fgaCacheUrl / DESCOPE_FGA_CACHE_URL and one ManagementsBase.getFgaUri helper that routes six FGA/authz calls to an FGA cache host, leaving everything else on the base URL.
Sniffed out 0 issues. Good bones! 🦴
The previously reviewed commit is gone after the rebase, so I re-sniffed the whole PR diff — it's unchanged, and the rebased base didn't add FGA endpoints that would need routing too. Re-verified: exactly six routed call sites (FGAServiceImpl 50/94/107/129, AuthzServiceImpl 214/255) matching the README; route constants match the URLs asserted in the new tests; trailing-slash trim and the blank/null fallback to getUri both behave; AbstractProxyImpl.addHeaders builds the bearer header from the client, not the URI, so routing can't affect auth; DescopeClient.getClient is still the only Client.builder() in src/main. fgaCacheUrl/fgaCacheUri sit last with explicit legacy-arity constructors, so positional callers keep compiling.
Woof!
Declared coverage: FULL — 11/11 changed files reviewed.
Adds Config.fgaCacheUrl, also readable from DESCOPE_FGA_CACHE_URL, mirroring Config.FGACacheURL in the Go SDK. When set, the calls an FGA cache serves go to it instead of the Descope base URL: saveSchema, createRelations, deleteRelations and check on the FGA service, and whoCanAccess and whatCanTargetAccess on the authz service. Everything else stays on the base URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both are published API, so adding a field to the Lombok @AllArgsConstructor would break callers that construct them positionally. Put the new field last and keep a constructor with the previous parameter list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
74700e2 to
b0c3162
Compare
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds Config.fgaCacheUrl / DESCOPE_FGA_CACHE_URL and one ManagementsBase.getFgaUri helper that routes six FGA/authz calls to an FGA cache host, leaving everything else on the base URL.
Sniffed out 0 issues. Good bones! 🦴
The stack landed — this now rebases onto main and the diff is standalone and unchanged. Re-sniffed the whole thing against the new base: six routed call sites (FGAServiceImpl 50/94/107/129, AuthzServiceImpl 214/255) matching the README, route constants matching the URLs asserted in the new tests, trailing-slash trim and blank/null fallback to getUri both behaving, DescopeClient.getClient still the only Client.builder() in src/main, and the proxy never reading client.getUri() so routing can't affect auth. The explicit legacy-arity constructors on Config/Client keep positional callers compiling — same pattern FGASchema already uses in main.
Woof!
Declared coverage: FULL — 11/11 changed files reviewed.
2 participants
Related Issues
Required for:
https://github.com/descope/etc/issues/17574
Related PRs
Upstream PRs
In a Nutshell
Config.fgaCacheUrl+DESCOPE_FGA_CACHE_URLManagementsBase.getFgaUrihelperDescription
Mirrors
Config.FGACacheURLfrom the Go SDK. When an FGA cache (authzcache) URL is configured, the calls that the cache serves are sent to it instead of the Descope base URL:saveSchema,createRelations,deleteRelationsandcheckon the FGA service, andwhoCanAccessandwhatCanTargetAccesson the authz service — exactly the six the Go SDK marks withoptions.BaseURL = fgaCacheURL. Everything else, includingloadSchema,dryRunSchemaand the resource details calls, stays on the base URL.Auth is unaffected: the proxy builds the
Bearer <projectId>:<managementKey>header independently of the URI, and the cache serves the same/v1/mgmt/...paths, so only the host changes. A trailing slash on the configured URL is trimmed so it cannot produce a double slash.Third of four stacked PRs — based on #348.
Must