feat(fga): skip caching conditional check results + wire CheckWithContext#464
feat(fga): skip caching conditional check results + wire CheckWithContext#464yosiharan wants to merge 4 commits into
Conversation
|
🐕 Review complete — View session on Shuni Portal 🐾 |
![shuni-bot-dev[bot]](https://avatars.githubusercontent.com/u/97479186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Adds ABAC/CEL support: skips caching conditional check results, wires CheckWithContext through controller → service → SDK, and forwards new conditional info fields in the response proto.
Sniffed out 2 issues:
- 1 🔴 CRITICAL: build is broken — authzservice proto dependency missing required fields
- 1 🟡 MEDIUM:
WhoCanAccess/WhatCanTargetAccesscandidate filtering drops context silently
This one needs a bath before it can go out! 🐾
| } | ||
|
|
||
| func (a *authzCache) Check(ctx context.Context, relations []*descope.FGARelation) ([]*descope.FGACheck, error) { | ||
| return a.CheckWithContext(ctx, relations, nil) |
There was a problem hiding this comment.
🟡 MEDIUM: filterWhoCanAccessCandidates and filterWhatCanTargetAccessCandidates (lines 181–202, 233–254) call a.Check() which delegates to CheckWithContext(ctx, relations, nil) — so candidate re-verification never passes the caller's extra context.
If a conditional relation was originally allowed because the caller supplied context, the nil-context re-check here could flip it to denied, silently filtering out valid candidates. This may be intentional (lookup cache doesn't carry context), but worth a comment or explicit design note since it's a subtle behavioral difference.
There was a problem hiding this comment.
[CLAUDE 🤖] Valid design concern — added a comment to filterWhoCanAccessCandidates explaining why nil context is safe here: UpdateCacheWithChecks never caches conditional results, so any candidate stored in the WhoCanAccess/WhatCanTargetAccess cache is guaranteed to be non-conditional and re-validates correctly with nil context.
There was a problem hiding this comment.
- I don't see this comment nowhere
- comment doesn't compile into code :)
There was a problem hiding this comment.
Ah sorry, I later removed this comment manually as it was redundant and confusing, should have updated the comment thread 🙏
There was a problem hiding this comment.
Pull request overview
Adds end-to-end ABAC/CEL support by threading caller-provided context through the authzcache stack and preventing context-dependent (conditional) authorization decisions from being cached.
Changes:
- Skip caching of conditional FGA check results to avoid serving stale, context-dependent decisions from cache.
- Wire
CheckWithContextthrough controller → service → SDK, forwarding the request’s CEL context map. - Expand Check API response info to include
Conditional,MissingContext, andConditionalErr.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/services/caches/projectauthzcache.go | Skips caching FGACheckInfo.Conditional results in UpdateCacheWithChecks. |
| internal/services/caches/projectauthzcache_test.go | Adds unit test asserting conditional results are not stored in cache. |
| internal/services/authz.go | Introduces CheckWithContext and routes Check through it; calls SDK CheckWithContext. |
| internal/services/authz_mock.go | Extends AuthzCacheMock with CheckWithContext support. |
| internal/services/authz_test.go | Updates mocks/bench to use CheckWithContext* expectations/responses. |
| internal/controllers/controller.go | Forwards request context map into CheckWithContext and returns additional check info fields. |
| go.mod | Bumps github.com/descope/go-sdk dependency to v1.16.0. |
| go.sum | Updates checksums for the go-sdk version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yosiharan
force-pushed
the
feat/abac-skip-conditional-cache
branch
2 times, most recently
from
d54c59a to
3691015
Compare
…text - UpdateCacheWithChecks skips entries with Info.Conditional==true so context-dependent results are always re-evaluated by authzservice - CheckWithContext threaded through controller, service, and SDK call - Controller now forwards Conditional, MissingContext, ConditionalErr fields in the gRPC CheckResponse - Unit tests updated to use CheckWithContextAssert/Response mocks; new test asserts conditional results are never served from cache Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
yosiharan
force-pushed
the
feat/abac-skip-conditional-cache
branch
from
2708e32 to
929b91e
Compare
Related Issues
Resolves: https://github.com/descope/etc/issues/15115
Related PRs
Upstream PRs
In a Nutshell
Conditional == trueCheckWithContextthrough controller → service → SDKConditional,MissingContext,ConditionalErrin gRPC responseDescription
Adds ABAC/CEL support to authzcache end-to-end:
UpdateCacheWithChecksnow skips entries withInfo.Conditional == trueso context-dependent FGA decisions are always re-evaluated by authzservice rather than served from a stale cache.CheckWithContextis threaded through the gRPC controller → service → SDK call chain, passing the caller-supplied CEL context map to authzservice.Conditional,MissingContext, andConditionalErrfields in theCheckResponseInfoproto.Depends on: #488 (backend monorepo migration)
Must