feat(authz): improve v2 request proto validation

[jakedoublev]

Proposed Changes

• Improve authorization service v2 proto validation
• Unit tests for protovalidate rules

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

[gemini-code-assist]

Hello @jakedoublev, I'm Gemini Code Assist¹! 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!

Summary of Changes

Hello team, Gemini here with a summary of this pull request. This PR, authored by @jakedoublev, focuses on enhancing the validation of the v2 authorization request protobuf messages. The primary goal is to ensure that incoming requests adhere to expected formats and constraints using buf.validate. Complementing these validation rule additions, the PR also introduces comprehensive unit tests to verify that these new validation rules are correctly applied and enforced for both successful and failing scenarios.

Highlights

• Protobuf Validation: Added new message-level validation rules to the GetDecisionRequest protobuf message in the v2 authorization service definition (authorization.proto). These rules enforce constraints on the resource field, specifically checking that registered_resource_value_fqn is a valid URI if present, and that the list of fqns within attribute_values is not empty if that resource type is used.
• Unit Tests: Introduced extensive unit tests in authorization_test.go to cover the newly added protovalidate rules. These tests validate various combinations of GetDecisionRequest and GetDecisionMultiResourceRequest payloads, ensuring that valid requests pass validation and invalid requests fail with the expected errors. This significantly improves test coverage for the authorization service's request handling.

Changelog

Click here to see the changelog

• service/authorization/v2/authorization.proto
  • Added message-level validation options to GetDecisionRequest (around lines 93-107) to validate the resource field content.
  • Ensured resource.registered_resource_value_fqn is a valid URI if present.
  • Ensured resource.attribute_values.fqns is not empty if attribute_values is used.
  • Made a minor formatting adjustment to the buf.validate.field annotation for EntityIdentifier.registered_resource_value_fqn (around lines 33-36).
• service/authorization/v2/authorization_test.go
  • Added imports for protovalidate, entity, and policy (around lines 7-10).
  • Added helper function getValidator for creating a protovalidate.Validator instance (around lines 25-31).
  • Added Test_GetDecisionRequest_Succeeds with multiple test cases covering valid GetDecisionRequest scenarios (around lines 33-183).
  • Added Test_GetDecisionRequest_Fails with multiple test cases covering invalid GetDecisionRequest scenarios and expected validation errors (around lines 185-378).
  • Added Test_GetDecisionMultiResourceRequest_Succeeds with multiple test cases covering valid GetDecisionMultiResourceRequest scenarios (around lines 380-497).
  • Added placeholder tests for GetDecisionMultiResourceRequest_Fails and GetEntitlementsRequest validation (around lines 498-500).

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

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.

---

Proto rules defined,
Validation keeps requests clean,
Tests make sure it's right.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request significantly improves the validation for GetDecisionRequest by adding new CEL rules and includes a substantial number of unit tests for these new rules, which is great. The tests for GetDecisionRequest (both success and failure cases) are well-structured and cover the new validation logic effectively.

There are a couple of areas for improvement, primarily concerning the test coverage for GetDecisionMultiResourceRequest and the implementation of placeholder test functions. Addressing these will further enhance the robustness of the v2 request validation.

It's also noted that the checklist item "I have added or updated unit tests" in the PR description is currently unchecked, despite the addition of many tests. This might be an oversight.

Summary of Findings

• Incorrect Test Expectation for Empty Resources: In Test_GetDecisionMultiResourceRequest_Succeeds, the test case "entity: token, action: create, empty resources array" expects success. However, the resources field in GetDecisionMultiResourceRequest has a min_items: 1 validation rule, meaning an empty list should fail validation. This test case should likely be in the failure suite.
• Incomplete Test Coverage for Protovalidate Rules: The test functions Test_GetDecisionMultiResourceRequest_Fails, Test_GetEntitlementsRequest_Succeeds, and Test_GetEntitlementsRequest_Fails are placeholders. To ensure comprehensive validation coverage as per the PR's goal, these should be implemented to test the existing protovalidate rules for their respective request messages.

Merge Readiness

The pull request makes good progress in improving proto validation and adding unit tests. However, there is a high-severity issue with an incorrect test expectation in Test_GetDecisionMultiResourceRequest_Succeeds that needs to be addressed. Additionally, implementing the placeholder test functions would significantly improve overall test coverage for existing validation rules.

I recommend addressing these points, particularly the incorrect test case, before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval from team members.

[github-actions]

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	5000
Denied Decision Requests	0
Total Time	475.720997ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	5000
Denied Decision Requests	0
Total Time	293.592211ms

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	342.103458ms
Throughput	292.31 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	53.753202997s
Average Latency	534.997808ms
Throughput	93.02 requests/second

NANOTDF Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	4950
Failed Requests	50
Concurrent Requests	50
Total Time	44.584205698s
Average Latency	440.676351ms
Throughput	111.03 requests/second

Error Summary:

Error Message	Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error rpc error: code = Internal desc = could not perform access	50 occurrences

Standard Benchmark Metrics Skipped or Failed

[github-actions]

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric	Value
Approved Decision Requests	5000
Denied Decision Requests	0
Total Time	460.406332ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric	Value
Approved Decision Requests	5000
Denied Decision Requests	0
Total Time	287.320279ms

Bulk Benchmark Results

Metric	Value
Total Decrypts	100
Successful Decrypts	100
Failed Decrypts	0
Total Time	359.204872ms
Throughput	278.39 requests/second

TDF3 Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	5000
Failed Requests	0
Concurrent Requests	50
Total Time	53.162267201s
Average Latency	528.548394ms
Throughput	94.05 requests/second

NANOTDF Benchmark Results:

Metric	Value
Total Requests	5000
Successful Requests	4945
Failed Requests	55
Concurrent Requests	50
Total Time	43.754029119s
Average Latency	431.47044ms
Throughput	113.02 requests/second

Error Summary:

Error Message	Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error rpc error: code = Internal desc = could not perform access	55 occurrences

Standard Benchmark Metrics Skipped or Failed

[jakedoublev]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves the v2 request proto validation by adding comprehensive protovalidate rules and thorough unit tests to verify them. The changes are well-implemented and the tests cover a wide range of scenarios for GetDecisionRequest, GetDecisionMultiResourceRequest, and GetEntitlementsRequest.

The new validation rules in authorization.proto for the Resource message, particularly for AttributeValues and registered_resource_value_fqn, are clear and enhance the robustness of the API.

The unit tests in authorization_test.go are well-structured using table-driven tests, making them readable and maintainable. The coverage of both success and failure cases for the validation rules is excellent.

Overall, this is a high-quality contribution that strengthens the authorization service's input validation. Great work!

Summary of Findings

• Clarity of Validation Message: The validation message for Resource.attribute_values ("if provided, resource.attribute_values must not be empty") in service/authorization/v2/authorization.proto primarily addresses the non-empty constraint (this.fqns.size() > 0). The associated CEL expression this.fqns.size() > 0 && this.fqns.all(item, item.isUri()) also enforces that all FQNs are URIs. While the current message isn't incorrect, it could be slightly more comprehensive by hinting at the URI format requirement as well. This is a minor observation (low severity) and was not commented on directly due to the review settings, as it doesn't impede functionality and the CEL expression itself is the source of truth for the validation logic.

Merge Readiness

The pull request is in good shape. The added validations are robust and the unit tests provide excellent coverage for these changes. I have no major concerns. As a reviewer, I am not authorized to approve pull requests, so please ensure it undergoes the standard approval process by other maintainers before merging.