feat(fga): support ABAC context in check, whoCanAccess and whatCanTargetAccess - #348
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 ABAC context overloads to check, whoCanAccess and whatCanTargetAccess, and replaces the hand-rolled Map casting in check/loadSchema with typed response DTOs so relation, info, version and conditions finally survive.
Sniffed out 1 issue:
- 1 🟢 LOW:
loadSchemacan hand back a nullconditionslist
Good bones — the overloads delegate cleanly, mapOf returns a mutable HashMap so the conditional put("context", …) is safe, the empty-context-not-sent behaviour is pinned by tests, and FGASchema keeps its 1-arg constructor so existing call sites still compile. Woof!
See inline comment for details.
Declared coverage: FULL — 13/13 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds ABAC context overloads to check, whoCanAccess and whatCanTargetAccess, and swaps the hand-rolled Map casting in check/loadSchema for typed DTOs so relation, info, version and conditions finally survive.
Sniffed out 1 issue:
- 1 🟢 LOW: README example uses a namespace that doesn't match its own schema
Re-sniffed the previous finding: loadSchema now seeds conditions with an empty list before any early return, so the README's iterate-the-conditions example can't NPE — fixed, and pinned by testLoadSchema_ConditionsAreNeverNull. Delegation keeps validation intact, mapOf hands back a mutable HashMap so the conditional put("context", …) is safe, and FGASchema's 1-arg constructor keeps existing call sites compiling. Good bones! Woof!
See inline comment for details.
Declared coverage: FULL — 13/13 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds ABAC context overloads to check, whoCanAccess and whatCanTargetAccess, and replaces the hand-rolled Map casting in check/loadSchema with typed DTOs so relation, info, version and conditions finally survive.
Sniffed out 1 issue:
- 1 🟢 LOW: new
missingContextlist can come back null
Both earlier findings re-sniffed and confirmed fixed: the README snippet now queries document, and loadSchema seeds conditions before every return (pinned by testLoadSchema_ConditionsAreNeverNull). apiProxy.get/post route through the same exchange as the old getArray/postAndGetArray with FAIL_ON_UNKNOWN_PROPERTIES=false, mapOf hands back a mutable HashMap so the conditional put("context", …) is safe, and FGASchema's new 1-arg constructor keeps existing call sites compiling alongside @AllArgsConstructor. Good bones! Woof!
See inline comment for details.
Declared coverage: FULL — 13/13 changed files reviewed.
|
@shuni review |
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Adds ABAC context overloads to check, whoCanAccess and whatCanTargetAccess, and swaps the hand-rolled Map casting in check/loadSchema for typed DTOs so relation, info, version and conditions finally survive.
No new issues — nothing survived verification this round.
All three earlier findings re-sniffed and confirmed fixed: check seeds missingContext with an empty list (FGAServiceImpl.java:137, pinned by testCheck_MissingInfoIsNotNull), loadSchema seeds conditions before every return, and the README snippet queries the document namespace its own schema declares. Also re-checked the rest: apiProxy.get/post route through the same exchange with FAIL_ON_UNKNOWN_PROPERTIES=false, mapOf returns a mutable HashMap so the conditional put("context", …) is safe, FGACheckResult(allowed, relation, info) matches the arg order used, and FGASchema's explicit 1-arg constructor keeps existing call sites compiling next to @AllArgsConstructor/@Builder. Good bones! Woof!
Declared coverage: FULL — 13/13 changed files reviewed.
Declared coverage: FULL — 13/13 changed files reviewed.
yosiharan
left a comment
yosiharan
left a comment
There was a problem hiding this comment.
self review done
…getAccess Adds context overloads mirroring CheckWithContext, WhoCanAccessWithContext and WhatCanTargetAccessWithContext in the Go SDK. The context map is sent only when non-empty, and its keys become variables for the CEL conditions in the schema. Also surfaces the response side, which was previously discarded: check now returns the relation it was asked about and the info object (conditional, missingContext, conditionalErr, factUsed), and loadSchema now returns the schema version and its conditions. Overloads rather than renames, and FGASchema keeps its single-argument constructor, so existing callers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The README tells callers to iterate schema.getConditions(), so default it to an empty list when the server omits the schema object or its conditions, matching how check normalizes a missing info. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The surrounding example declares type document, so the query has to use it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same reason as conditions: callers iterate it, and the server omits it when nothing is missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only the conditions on the deciding path are evaluated, so "any schema conditions" overstates it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8a6700c to
e821553
Compare
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-sniffed after the rebase onto main (#347 is in, so the PR diff is now just the ABAC work). Adds context overloads to check, whoCanAccess and whatCanTargetAccess, and swaps the hand-rolled Map casting in check/loadSchema for typed DTOs so relation, info, version and conditions survive.
No new issues — nothing survived verification.
Delta since the last review is javadoc wording only ("evaluating any schema conditions" → "evaluating schema conditions") in AuthzService/FGAService; the code is byte-identical. All three earlier findings stay fixed: loadSchema seeds conditions before every return, check seeds info.missingContext, and the README snippet queries the document namespace its own schema declares. Re-confirmed on this checkout that mapOf returns a mutable HashMap (so the conditional put("context", …) is safe), apiProxy.get(URI, Class) routes through the same exchange as the getArray it replaces, and FGACheckResult(allowed, relation, info) matches the arg order used. Good bones! Woof!
Declared coverage: FULL — 13/13 changed files reviewed.
Declared coverage: FULL — 13/13 changed files reviewed.
2 participants
Related Issues
Required for:
https://github.com/descope/etc/issues/17574
Related PRs
Upstream PRs
In a Nutshell
check,whoCanAccess,whatCanTargetAccesscontext overloadscheckfinally returns the relation andinfoloadSchemafinally returnsversionandconditionsFGACondition/FGAConditionParammodelsDescription
Adds ABAC support to the FGA and authz surfaces, mirroring
CheckWithContext,WhoCanAccessWithContextandWhatCanTargetAccessWithContextin the Go SDK. Callers can pass a context map whose keys become variables for the CEL conditions defined in the schema; the map is only sent when non-empty, matching Go.The response half is included because a context map is not usable without it.
checkpreviously built each result fromallowedalone and dropped the relation echo and the wholeinfoobject, so there was no way to tell a conditional denial from a plain one;loadSchemadropped the schemaversionand itsconditions. Both are now parsed into typed response models instead of hand-rolledMapcasting.These are overloads, not renames, and
FGASchemakeeps its single-argument constructor, so existing call sites are untouched.Second of four stacked PRs — based on #347.
Must