Repository navigation
[#13859] fix(authz): expose inaccessible object tag and policy associations - #13863
Conversation
9fcb8e4 to
e396613
Compare
mchades
left a comment
mchades
left a comment
There was a problem hiding this comment.
Reviewed e396613. I did not find a concrete functional defect; the inline comments request regression coverage for the new visibility indicators. All 70 focused tests in TestResponses, TestMetadataObjectTagOperations, and TestMetadataObjectPolicyOperations passed locally. Docker authorization tests were not run locally.
| throw new AssertionError(error); | ||
| }); | ||
| Assertions.assertEquals(2, nameListResponse.getNames().length); | ||
| Assertions.assertTrue(nameListResponse.hasInaccessibleTags()); |
There was a problem hiding this comment.
Please add an untagged object and an object whose only tag is inherited and inaccessible, asserting empty arrays with false versus true for both response modes. The current assertions only cover mixed visibility, so the empty-array distinction in #13859 remains untested.
There was a problem hiding this comment.
Addressed in a4c8f52. Added real authorization coverage for both response modes using two distinct objects: an untagged catalog returns empty arrays with hasInaccessibleTags=false, while a table whose only effective tag is inherited and inaccessible returns empty arrays with hasInaccessibleTags=true. TagOperationsAuthorizationIT passes with Docker tests enabled.
| MetadataObjectPolicyListResponse details = | ||
| (MetadataObjectPolicyListResponse) detailsResponse.getEntity(); | ||
| Assertions.assertEquals(0, details.getPolicies().length); | ||
| Assertions.assertTrue(details.hasInaccessiblePolicies()); |
There was a problem hiding this comment.
Please add an authorization integration test for this flag. Both the dispatcher and authorization filter are mocked here, and PolicyAuthorizationIT does not call the object policy-list endpoint. Verify all-hidden and mixed-visible tag-derived policies through real authorization, checking the flag and visible arrays in both response modes.
There was a problem hiding this comment.
Addressed in a4c8f52. Added PolicyAuthorizationIT coverage through the real tag-to-policy resolution and authorization path. It verifies all-hidden and mixed-visible results, including both arrays and hasInaccessiblePolicies, for names and details responses. Both PolicyAuthorizationIT and TagOperationsAuthorizationIT pass with Docker tests enabled.
Code Coverage Report
Files |
mchades
left a comment
mchades
left a comment
There was a problem hiding this comment.
Two additional gaps in client access and user documentation for the new association visibility indicators.
| * | ||
| * @return true if at least one inaccessible tag was omitted, false otherwise | ||
| */ | ||
| public boolean hasInaccessibleTags() { |
There was a problem hiding this comment.
The public Java SupportsTags/SupportsPolicies methods and Python tag methods still return only arrays/lists, so SDK callers still cannot distinguish no associations from all-inaccessible associations. The new tests read the DTOs through RESTClient directly. Please add backward-compatible public APIs exposing the visible items and the corresponding flag, with tests through those APIs.
There was a problem hiding this comment.
Addressed in fa15ab2. Added backward-compatible Java APIs returning TagList/PolicyList with the visible items and inaccessible-association indicator, while keeping the existing array-returning methods unchanged. Added the equivalent Python TagList APIs for tags. The authorization tests now exercise these public APIs instead of reading REST DTOs directly, and client unit tests cover the compatibility defaults and propagated flags.
| properties: | ||
| hasInaccessibleTags: | ||
| type: boolean | ||
| description: Whether one or more inaccessible tags were omitted from the response |
There was a problem hiding this comment.
Please also update docs/manage-tags-in-gravitino.md and docs/manage-policies-in-gravitino.md. OpenAPI now describes the fields, but the user guides do not explain them. Document the four list/flag combinations, include an all-inaccessible response example, explain inherited associations, and show how users can read the indicators through the supported APIs.
There was a problem hiding this comment.
Addressed in fa15ab2. Updated both management guides to document the four visible-list/indicator combinations, all-inaccessible response examples, inherited association behavior before authorization filtering, and the supported Java and Python APIs. The documentation build passes.
…object tag and policy associations (#13863) (#13878) ### What changes were proposed in this pull request? Backport #13863 to branch-1.3. Add authorization-filtering indicators to metadata object tag and policy list responses: - `hasInaccessibleTags` - `hasInaccessiblePolicies` The indicators are included in both name and detailed responses. Direct and inherited associations are aggregated before authorization filtering. ### Why are the changes needed? When all associated tags or policies are inaccessible, the APIs currently return an empty list indistinguishable from an object with no associations. The new fields indicate whether associations were omitted without exposing their names, count, details, or inheritance source. Fix: #13859 ### Does this PR introduce _any_ user-facing change? Yes. Metadata object tag and policy list responses add boolean fields indicating whether inaccessible associations were omitted. ### How was this patch tested? - Common response DTO unit tests - Metadata object tag and policy REST tests - Metadata authorization helper tests - Tag and policy Docker authorization integration tests - `./gradlew :docs:build` - Spotless checks
mchades
left a comment
mchades
left a comment
There was a problem hiding this comment.
Re-reviewed fa15ab2. The public SDK APIs and user guides address the previous SDK/docs comments. The 48 focused Java client tests, API Javadoc, formatting checks, and OpenAPI build pass locally. The Python tag suite has two reproducible fixture errors noted inline. Docker authorization tests were not run locally.
| with patch( | ||
| "gravitino.utils.http_client.HTTPClient.get", | ||
| return_value=mock_base.mock_http_response(response.to_json()), | ||
| ): | ||
| tag_list = fileset.list_tags_with_visibility() |
There was a problem hiding this comment.
[P2] Supply an HTTP client to the visibility-test fixture
@mock_base.mock_data returns a GenericFileset constructed with rest_client=None. Consequently, this test and test_fileset_list_tags_info_with_visibility fail with AttributeError: 'NoneType' object has no attribute 'get' before reaching the patched HTTPClient.get. Both failures reproduce with python -m unittest tests.unittests.test_tag_api. Please provide an HTTP client in these fixtures so the tests exercise response parsing and flag propagation.
There was a problem hiding this comment.
Addressed in 548bb95. The shared fileset fixture now constructs GenericFileset with a real HTTPClient, so both visibility tests reach the patched HTTPClient.get and exercise response parsing and flag propagation. Verified with python3 -m unittest tests.unittests.test_tag_api: all 30 tests pass.
…object tag and policy associations (#13889) ### What changes were proposed in this pull request? Backport #13863 to branch-1.3. - Add `hasInaccessibleTags` and `hasInaccessiblePolicies` to metadata-object name and detail list responses. - Add backward-compatible Java visibility APIs using `TagList` and `PolicyList`, plus the equivalent Python tag APIs. - Preserve branch-1.3's direct policy-association APIs while applying authorization filtering consistently to direct and inherited associations. - Add SDK, authorization, response DTO, OpenAPI, and user-guide coverage. ### Why are the changes needed? When all associated tags or policies are inaccessible, an empty list is indistinguishable from an object with no associations. The new indicators report that associations were omitted without exposing their names, counts, details, or inheritance sources. Fix: #13859 ### Does this PR introduce _any_ user-facing change? Yes. Tag and policy list responses expose `hasInaccessibleTags` and `hasInaccessiblePolicies`. Java and Python clients add backward-compatible APIs that return visible items together with these indicators. ### How was this patch tested? - `./gradlew :api:build -x :api:test :clients:client-java:compileTestJava` - `./gradlew :clients:client-java:test --tests org.apache.gravitino.client.TestSupportTags --tests org.apache.gravitino.client.TestSupportPolicies` - `python3 -m unittest tests.unittests.test_tag_api` (29 tests passed) - `./gradlew :docs:build` - Spotless checks - Authorization Docker ITs were attempted, but both stopped during Hive container initialization because the mapped ports timed out before test execution.
2 participants
What changes were proposed in this pull request?
Add authorization-filtering indicators to metadata object tag and policy list responses:
hasInaccessibleTagshasInaccessiblePoliciesThe indicators are included in both name and detailed responses. Tag and policy associations are aggregated before authorization filtering so direct and inherited associations follow the same behavior.
Why are the changes needed?
When all associated tags or policies are inaccessible, the APIs currently return an empty list indistinguishable from an object with no associations.
The new fields indicate whether associations were omitted without exposing their names, count, details, or inheritance source.
Fix: #13859
Does this PR introduce any user-facing change?
Yes. The metadata object tag and policy list APIs add boolean fields indicating whether inaccessible associations were omitted.
How was this patch tested?
./gradlew :docs:build