feat(core): add fine-grain authorization support#2999
Conversation
- support multi group claims - refactor for efficiency - refactor for future casbin adapter support
Co-authored-by: Jake Van Vorhis <83739412+jakedoublev@users.noreply.github.com>
Summary of ChangesHello @alkalescent, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the platform's authorization capabilities by introducing a new fine-grained system. It shifts the focus from broad path-based access to granular control based on specific RPC methods and dynamic resource attributes. This enhancement provides a more robust and flexible security model, allowing services to define and enforce access policies at a much finer level of detail, while ensuring a smooth transition and continued support for existing authorization rules. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. A fine-grain net, so strong and new, Resources guarded, clear and true. No path alone, but context deep, Where access rules, the secrets keep. Footnotes
|
There was a problem hiding this comment.
Code Review
This is an excellent pull request that introduces a well-designed and robust fine-grained authorization system. The use of a pluggable authorizer interface, a resolver registry for service-specific logic, and the Inversion of Control pattern are all great architectural choices that will improve the platform's extensibility and maintainability. The code is clean, well-structured, and thoroughly tested. The accompanying documentation and ADRs are very detailed and helpful for understanding the new system. I have one high-severity comment regarding an inconsistency in the ADR document that should be addressed to avoid confusion. Overall, this is a fantastic contribution.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
|
I chose a hybrid approach, using the platform cache manager for inter-request caching and keeping the resolver context for intra-request caching. The benefit of the resolver context is that the handlers don't need knowledge of the cache keys. |
jentfoo
left a comment
There was a problem hiding this comment.
A couple defensive recommendations, the rolePrefix check is a critical fix.
Signed-off-by: Ryan Schumacher <jschumacher@virtru.com>
Invalidated by push of 51e8779
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
X-Test Failure Report✅ go-v0.9.0 |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
X-Test Failure Report✅ go-v0.9.0 |
| return | ||
| } | ||
|
|
||
| // Build authorization request |
There was a problem hiding this comment.
The gRPC gateway was removed, is this still needed?
1.) merge main --------- Signed-off-by: Mary Dickson <mary.dickson@virtru.com> Signed-off-by: Paul Flynn <pflynn@virtru.com> Signed-off-by: strantalis <strantalis@virtru.com> Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com> Signed-off-by: Krish Suchak <suchak.krish@gmail.com> Signed-off-by: David Mihalcik <dmihalcik@virtru.com> Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com> Signed-off-by: Diego <74568547+dsm20@users.noreply.github.com> Signed-off-by: Dana Morris <dmorris@virtru.com> Signed-off-by: Jeremy Haage <jeremy.haage@virtru.com> Signed-off-by: Krish Suchak <ksuchak@virtru.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com> Co-authored-by: Krish Suchak <42231639+alkalescent@users.noreply.github.com> Co-authored-by: Paul Flynn <43211074+pflynn-virtru@users.noreply.github.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Mary Dickson <mary.dickson@virtru.com> Co-authored-by: dominic reed <dominic.reed@virtru.com> Co-authored-by: Elizabeth Healy <35498075+elizabethhealy@users.noreply.github.com> Co-authored-by: Sean Trantalis <18211470+strantalis@users.noreply.github.com> Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Jake Van Vorhis <83739412+jakedoublev@users.noreply.github.com> Co-authored-by: Louie <85858507+el-virt@users.noreply.github.com> Co-authored-by: dmihalcik-virtru <dmihalcik-virtru@users.noreply.github.com> Co-authored-by: Krish Suchak <suchak.krish@gmail.com> Co-authored-by: Dave Mihalcik <dmihalcik@virtru.com> Co-authored-by: Artem A. <67011886+hi-artem@users.noreply.github.com> Co-authored-by: Diego <74568547+dsm20@users.noreply.github.com> Co-authored-by: Paul Flynn <pflynn-virtru@users.noreply.github.com> Co-authored-by: Dillon Thompson <dj.thompson715@gmail.com> Co-authored-by: Dana Morris <damorris25@gmail.com> Co-authored-by: Nick <79929408+ntrevino-virtru@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: sievdokymov-virtru <100794336+sievdokymov-virtru@users.noreply.github.com> Co-authored-by: Jeremy Haage <jeremy.haage@virtru.com> Co-authored-by: Tim Tschampel <timothy.tschampel@gmail.com> Co-authored-by: sujankota <sreddy@virtru.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dmihalcik-virtru <38867245+dmihalcik-virtru@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency ReviewThe following issues were found:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
### Proposed Changes * ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
## Summary Step 1 production-readiness security changes for PR #2999. This wires the authz resolver registry into the server authenticator path so v2 authorization can resolve request-specific resource dimensions during interceptor authorization. It also adds default Casbin v2 grouping statements for the built-in admin and standard roles, without reintroducing deprecated `RoleMap` behavior. ## Changes - Pass `AuthzResolverRegistry` from server startup into `auth.NewAuthenticator` - Share the same resolver registry between service registration and auth enforcement - Add built-in v2 Casbin grouping statements: - `role:opentdf-admin -> role:admin` - `role:opentdf-standard -> role:standard` - Add unit coverage proving: - registered authz resolvers are invoked by v2 authorization - resolved resource context is passed into authorization - default v2 policy allows `opentdf-admin` through grouping statements ## Verification Ran: ```sh go test ./service/internal/auth/... ./service/internal/server/... ./service/pkg/server/... ``` Also ran targeted tests for resolver invocation and default role grouping behavior. ## Notes Customer policy override behavior is preserved. A customer can still replace the built-in Casbin policy with server.auth.policy.csv and omit the default opentdf-admin role mapping if they do not want that role.
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Proposed Changes
Checklist
Testing Instructions