Support for array of permissions in lookup entity v2 #1691
Conversation
…ithub.com:ankitsheoran1/permify into support-for-array-of-permissions-in-lookup-entity pull from origin and merge master
… support-for-array-of-permissions-in-lookup-entity-v1
WalkthroughThe changes in this pull request introduce significant enhancements to the Permify API, particularly in the area of permissions management. Two new endpoints, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🧹 Outside diff range and nitpick comments (11)
integration-test/usecases/notion_test.go (1)
114-116: LGTM! Consider enhancing test coverage and error handling.The changes align well with the expected modifications to the
PermissionLookupEntityRequeststructure and response handling. The use ofPermissionsas a slice and the updated response processing improve flexibility and robustness.To further strengthen the test case:
- Consider adding assertions to verify the structure of
res.GetEntityIds()to ensure it contains the expected permissions as keys.- Implement error handling for the case when
res.GetEntityIds()[permission]is not found, rather than silently continuing with an empty slice.- Add logging statements to provide more context in case of test failures.
Here's a suggested enhancement:
for permission, expected := range filter.Assertions { res, err := permissionClient.LookupEntity(ctx, &base.PermissionLookupEntityRequest{ // ... (existing code) }) Expect(err).ShouldNot(HaveOccurred()) // New assertion to check if the response contains the expected permission Expect(res.GetEntityIds()).To(HaveKey(permission), "Response should contain results for permission: %s", permission) var responseIdsForPermission []string if res, ok := res.GetEntityIds()[permission]; ok { responseIdsForPermission = res.Ids } else { // Log a warning if the permission is not found in the response GinkgoWriter.Printf("Warning: No entity IDs found for permission: %s\n", permission) } // Existing assertion Expect(IsSameArray(responseIdsForPermission, expected)).Should(Equal(true), "Mismatch in entity IDs for permission %s. Got: %v, Expected: %v", permission, responseIdsForPermission, expected) }This enhancement adds more comprehensive assertions and improves error reporting, making the test more robust and easier to debug.
Also applies to: 120-124
integration-test/usecases/facebook_groups_test.go (1)
114-124: LGTM! The changes improve the test case's clarity and flexibility.The modifications to the
LookupEntitymethod call and response handling are well-implemented:
- The
EntityTypeparameter is now explicitly set, which improves clarity.- Changing
PermissiontoPermissionsas a slice allows testing multiple permissions, enhancing flexibility.- The new response handling correctly extracts entity IDs for the specific permission, accommodating the updated API structure.
These changes align well with the updated API and provide a more robust test case.
For further improvement, consider:
- Adding a test case with multiple permissions to fully utilize the new
Permissionsslice capability.- Documenting the expected structure of the
LookupEntityresponse, perhaps as a comment, to clarify the need for the new handling logic.internal/servers/permissionServer.go (1)
133-152: Minor improvement for consistencyThe
LookupEntitiesStreammethod is well-implemented and consistent with the existing codebase structure. It correctly handles context tracing, request validation, error handling, and invoker interaction.For improved consistency with other stream methods in the file (e.g.,
LookupEntityStream), consider returning the validation error directly instead of wrapping it in a status error.Apply this small change to line 140:
- return v + return status.Error(GetStatus(v), v.Error())This change will make the error handling consistent across all methods in the file.
internal/engines/utils.go (1)
397-409: LGTM! Consider adding a comment for clarity.The
ConvertToPermissionsLookupEntityRequestfunction correctly converts aPermissionLookupEntityRequestto aPermissionsLookupEntityRequest. The implementation is straightforward and handles the conversion of the single permission to an array of permissions as expected.Consider adding a brief comment explaining the purpose of this function, especially highlighting the conversion from a single permission to an array of permissions. This would improve code readability and maintainability. For example:
// ConvertToPermissionsLookupEntityRequest converts a PermissionLookupEntityRequest to a PermissionsLookupEntityRequest. // It preserves all fields and converts the single Permission to an array of Permissions. func ConvertToPermissionsLookupEntityRequest(req *base.PermissionLookupEntityRequest) *base.PermissionsLookupEntityRequest { // ... (existing implementation) }pkg/cmd/validate.go (1)
367-371: Improved result comparison logicThe changes enhance the accuracy of result comparison by correctly handling empty results and using the
isSameArrayfunction for non-empty comparisons. This should reduce false negatives in the validation process.Consider extracting the condition
res.GetEntityIds() == nil && len(expected) == 0into a named boolean variable for improved readability. For example:-if res.GetEntityIds() == nil && len(expected) == 0 { +isEmptyResult := res.GetEntityIds() == nil && len(expected) == 0 +if isEmptyResult {docs/api-reference/openapiv2/apidocs.swagger.json (2)
768-824: New endpoint/lookup-entitieslooks good, with some suggestions.The new
/lookup-entitiesendpoint is well-structured and consistent with the existing API design. It includes proper documentation, parameters, and code samples. However, consider the following suggestions:
- The
tenant_idparameter description could be more specific to this endpoint's functionality.- Ensure that the
#/definitions/LookupEntitiesBodyschema is properly defined in the definitions section.- Review the
PermissionsLookupEntityResponseschema for consistency with other response structures in the API.Consider updating the
tenant_idparameter description to be more specific to the/lookup-entitiesendpoint functionality.
825-885: New streaming endpoint/lookup-entities-streamlooks good, with some suggestions.The new
/lookup-entities-streamendpoint is well-structured and consistent with the existing API design. It properly implements a streaming response and includes documentation and code samples. However, consider the following suggestions:
- The
tenant_idparameter description could be more specific to this endpoint's streaming functionality.- Ensure that the
#/definitions/LookupEntitiesStreamBodyschema is properly defined in the definitions section.- Review the
PermissionsLookupEntityStreamResponseschema for consistency with other streaming response structures in the API.- Add a cURL example for this endpoint to maintain consistency with other endpoint documentation and provide an easy way for users to test the API from the command line.
Consider updating the
tenant_idparameter description to be more specific to the/lookup-entities-streamendpoint functionality.Add a cURL example for the
/lookup-entities-streamendpoint to improve consistency and usability of the API documentation. Here's a template you can use:curl --location --request POST 'localhost:3476/v1/tenants/{tenant_id}/permissions/lookup-entities-stream' \ --header 'Content-Type: application/json' \ --data-raw '{ "metadata": { "snap_token": "", "schema_version": "", "depth": 20 }, "entity_type": "document", "permissions": ["view"], "subject": { "type": "user", "id": "1" }, "page_size": 20, "continuous_token": "" }'Note: Adjust the request body parameters as needed to match the exact requirements of the endpoint.
internal/schema/linkedSchema.go (2)
7-7: Organize imports by grouping standard library packagesThe import statement for the
"sync"package should be grouped with other standard library imports for better readability.Reorganize the imports as follows:
package schema import ( + "errors" + "sync" "github.com/Permify/permify/pkg/dsl/utils" base "github.com/Permify/permify/pkg/pb/base/v1" )
Line range hint
374-388: Handle errors consistently infindEntranceRewriteIn the
findEntranceRewritefunction, when an error occurs, ensure that it's properly returned and that all code paths handle errors consistently.Review the error handling logic to make sure that all errors are propagated correctly and that the function does not suppress any potential errors.
internal/engines/lookup.go (1)
Line range hint
217-226: Unused 'permission' parameter in callback functions.In the callback functions defined in
LookupEntityStreamandLookupSubject, thepermissionparameter is not used within the function body:callback := func(entityID, permission, token string) { // 'permission' parameter is unused }If the
permissionparameter is not required, consider omitting it or prefixing it with an underscore to indicate it's unused:callback := func(entityID, _ string, token string) { // Updated function signature }Also applies to: 361-366
internal/invoke/invoke.go (1)
344-344: Potential performance impact when logging large permission slicesIn the
LookupEntitiesmethod, the tracing attribute logsrequest.GetPermissions()usingattribute.StringSliceValue. If the permissions slice is large, this could affect performance or clutter the trace logs. Consider summarizing the permissions or limiting the number of permissions logged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (5)
pkg/pb/base/v1/health.pb.gois excluded by!**/*.pb.gopkg/pb/base/v1/health_grpc.pb.gois excluded by!**/*.pb.gopkg/pb/base/v1/service.pb.gois excluded by!**/*.pb.gopkg/pb/base/v1/service.pb.gw.gois excluded by!**/*.pb.gw.gopkg/pb/base/v1/service_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
- docs/api-reference/apidocs.swagger.json (4 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (4 hunks)
- integration-test/usecases/facebook_groups_test.go (1 hunks)
- integration-test/usecases/google_docs_test.go (1 hunks)
- integration-test/usecases/notion_test.go (1 hunks)
- internal/engines/bulk.go (6 hunks)
- internal/engines/entityFilter.go (18 hunks)
- internal/engines/lookup.go (11 hunks)
- internal/engines/utils.go (1 hunks)
- internal/invoke/invoke.go (4 hunks)
- internal/schema/linkedSchema.go (11 hunks)
- internal/servers/permissionServer.go (1 hunks)
- pkg/cmd/validate.go (1 hunks)
- pkg/pb/base/v1/health.pb.validate.go (1 hunks)
- pkg/pb/base/v1/service.pb.validate.go (8 hunks)
- proto/base/v1/service.proto (5 hunks)
🧰 Additional context used
🔇 Additional comments (32)
integration-test/usecases/google_docs_test.go (1)
114-124: Approve changes to LookupEntity request and response handlingThe modifications to the
LookupEntitymethod call and response handling look good. These changes align with the new support for an array of permissions in the lookup entity functionality. Here's a breakdown of the improvements:
Request changes:
- The
EntityTypeis now explicitly set, improving clarity.- The
Permissionsfield now accepts a slice of strings, allowing multiple permissions to be checked in a single request.Response handling:
- The new logic correctly extracts entity IDs for a specific permission from the response.
- This change accommodates the new response structure where entity IDs are grouped by permission.
These updates enhance the test's ability to verify the new multi-permission lookup functionality.
internal/servers/permissionServer.go (2)
112-131: LGTM: LookupEntities method implementationThe
LookupEntitiesmethod is well-implemented and consistent with the existing codebase structure. It correctly handles context tracing, request validation, error handling, and invoker interaction. The implementation follows best practices and maintains consistency with other methods in the file.
112-152: Summary of changes: New LookupEntities methodsThe addition of
LookupEntitiesandLookupEntitiesStreammethods to thePermissionServerstruct enhances its functionality by providing new capabilities for entity lookups based on permissions. These methods are well-implemented, following the established patterns in the codebase for error handling, tracing, and invoker interaction.The changes align with the PR objectives of supporting an array of permissions in lookup entity v2. They extend the API's capabilities while maintaining consistency with existing code structures.
pkg/cmd/validate.go (2)
Line range hint
472-476: Simplified result comparison logicThe changes streamline the result comparison process by directly using the
isSameArrayfunction in the condition. This simplification improves code readability and maintainability without altering the functionality.
Line range hint
1-624: Overall assessment of changesThe modifications in this file focus on improving the result comparison logic in both entity and subject filter processing sections. These changes enhance the accuracy of the validation process and improve code readability. The core functionality remains intact, and the changes are well-aligned with the file's purpose.
docs/api-reference/openapiv2/apidocs.swagger.json (1)
768-885: New endpoints successfully added, with a recommendation for schema verification.The addition of the
/lookup-entitiesand/lookup-entities-streamendpoints has been implemented correctly and consistently with the existing API structure. These new endpoints extend the API's functionality for looking up entities with permissions, including a streaming option.To ensure complete correctness of these additions:
Verify that the following schemas are properly defined in the definitions section of this file:
LookupEntitiesBodyLookupEntitiesStreamBodyPermissionsLookupEntityResponsePermissionsLookupEntityStreamResponseConsider updating the
tenant_idparameter descriptions to be more specific to each endpoint's functionality.Add a cURL example for the
/lookup-entities-streamendpoint to maintain consistency with other endpoint documentation.Overall, these additions enhance the API's capabilities and are well-integrated into the existing structure.
To confirm the existence and correctness of the new schema definitions, run the following script:
This script will help ensure that all necessary schema definitions for the new endpoints are present in the file.
✅ Verification successful
Schema definitions confirmed and verified successfully.
All necessary schemas (
LookupEntitiesBody,LookupEntitiesStreamBody,PermissionsLookupEntityResponse, andPermissionsLookupEntityStreamResponse) are present in theapidocs.swagger.jsonfile at the specified lines. This ensures that the newly added endpoints are properly supported by their respective schema definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the existence of new schema definitions echo "Checking for new schema definitions..." grep -n '"LookupEntitiesBody"' docs/api-reference/openapiv2/apidocs.swagger.json grep -n '"LookupEntitiesStreamBody"' docs/api-reference/openapiv2/apidocs.swagger.json grep -n '"PermissionsLookupEntityResponse"' docs/api-reference/openapiv2/apidocs.swagger.json grep -n '"PermissionsLookupEntityStreamResponse"' docs/api-reference/openapiv2/apidocs.swagger.json echo "If all schemas are found, their line numbers will be displayed above." echo "If any schema is missing, there will be no output for that schema."Length of output: 918
docs/api-reference/apidocs.swagger.json (3)
2409-2452: Review ofLookupEntitiesStreamBodydefinitionThe
LookupEntitiesStreamBodydefinition has been added to support the new/lookup-entities-streamendpoint. While the consistency withLookupEntitiesBodyis commendable, there are some points to consider:
Duplication: The
LookupEntitiesStreamBodyis identical toLookupEntitiesBody. This duplication might lead to maintenance issues if these structures evolve differently in the future.Naming Inconsistency: The description still refers to "PermissionsLookupEntityRequest" instead of "LookupEntitiesStreamBody".
Consider the following improvements:
To reduce duplication and improve maintainability, consider creating a common base type that both
LookupEntitiesBodyandLookupEntitiesStreamBodycan extend or reference. This would centralize any future changes to the shared structure.Update the description to use "LookupEntitiesStreamBody" instead of "PermissionsLookupEntityRequest".
If there are any stream-specific considerations (e.g., timeout handling, reconnection logic), consider adding documentation about these aspects in the description of
LookupEntitiesStreamBody.Here's a suggested refactoring approach:
"LookupEntitiesBaseBody": { "type": "object", "properties": { // ... all common properties ... } }, "LookupEntitiesBody": { "allOf": [ { "$ref": "#/definitions/LookupEntitiesBaseBody" }, { "type": "object", "properties": { // ... any properties specific to non-streaming lookup ... } } ] }, "LookupEntitiesStreamBody": { "allOf": [ { "$ref": "#/definitions/LookupEntitiesBaseBody" }, { "type": "object", "properties": { // ... any properties specific to streaming lookup ... } } ] }Let's verify the similarity between
LookupEntitiesBodyandLookupEntitiesStreamBody:#!/bin/bash # Compare the two definitions echo "Comparing LookupEntitiesBody and LookupEntitiesStreamBody:" diff <(sed -n '/LookupEntitiesBody/,/}/p' docs/api-reference/apidocs.swagger.json) <(sed -n '/LookupEntitiesStreamBody/,/}/p' docs/api-reference/apidocs.swagger.json) # Check for any references to PermissionsLookupEntityRequest echo "\nChecking for references to PermissionsLookupEntityRequest:" grep -n "PermissionsLookupEntityRequest" docs/api-reference/apidocs.swagger.jsonThis script will help us confirm the similarity between the two definitions and identify any remaining references to the incorrect name.
2870-2903: 🛠️ Refactor suggestion
⚠️ Potential issueReview of
PermissionsLookupEntityResponseandPermissionsLookupEntityStreamResponsedefinitionsThe new response structures for the lookup entities endpoints have been added. While they provide the necessary information, there are several points to address:
Naming Inconsistency: The prefix
Permissionsis used instead ofPermission, which is inconsistent with other similar definitions in the API.Complex Structure: The
entity_idsfield inPermissionsLookupEntityResponseis an object withEntityIdsas values, which seems unnecessarily complex compared to the previousPermissionLookupEntityResponsethat used a simple array of strings.Inconsistent Fields: The streaming response includes a
permissionfield, which is not present in the batch response. This inconsistency might confuse API consumers.Consider the following improvements:
Rename the responses to
PermissionLookupEntitiesResponseandPermissionLookupEntitiesStreamResponsefor consistency with other endpoint naming conventions.Simplify the
entity_idsstructure inPermissionLookupEntitiesResponse:"PermissionLookupEntitiesResponse": { "type": "object", "properties": { "entity_ids": { "type": "array", "items": { "type": "string" }, "description": "List of entity IDs that match the lookup criteria." }, "continuous_token": { "type": "string", "description": "Token for retrieving the next page of results." } } }
Consider adding the
permissionfield to the batch response as well, or remove it from the streaming response, depending on the intended use case. If it's kept only in the streaming response, provide a clear explanation of why it's needed there but not in the batch response.Update the descriptions to clearly explain the difference between the batch and streaming responses.
Let's verify the current structure and usage of these response types:
#!/bin/bash # Check the structure of PermissionsLookupEntityResponse echo "Structure of PermissionsLookupEntityResponse:" sed -n '/PermissionsLookupEntityResponse/,/}/p' docs/api-reference/apidocs.swagger.json # Check the structure of PermissionsLookupEntityStreamResponse echo "\nStructure of PermissionsLookupEntityStreamResponse:" sed -n '/PermissionsLookupEntityStreamResponse/,/}/p' docs/api-reference/apidocs.swagger.json # Check for any references to these response types in the API endpoints echo "\nReferences to the response types in API endpoints:" grep -n "PermissionsLookupEntityResponse\|PermissionsLookupEntityStreamResponse" docs/api-reference/apidocs.swagger.jsonThis script will help us confirm the current structure of the response types and their usage in the API specification, allowing us to make informed decisions about the suggested improvements.
768-885: New endpoints added for entity lookup operationsTwo new endpoints have been added to the Permify API:
/v1/tenants/{tenant_id}/permissions/lookup-entities(POST)/v1/tenants/{tenant_id}/permissions/lookup-entities-stream(POST)These endpoints provide functionality for looking up entities based on permissions. The stream version allows for handling large result sets more efficiently.
The addition of these endpoints enhances the API's capabilities for permission-based entity lookups. The implementation follows the existing patterns in the API specification, which is good for consistency.
However, there are a few points to consider:
Documentation: While the endpoints have basic descriptions, it would be beneficial to add more detailed documentation about their usage, especially regarding the streaming functionality.
Error Responses: The error responses are using the default
Statusobject. Consider adding specific error codes and messages for these endpoints to help API consumers handle errors more effectively.Pagination: The
page_sizeandcontinuous_tokenparameters are included, which is good for handling large result sets. Ensure that the API implementation properly handles these pagination parameters.Consider the following architectural aspects:
Caching: For frequently accessed permissions or entities, implement caching mechanisms to improve performance.
Rate Limiting: Ensure that appropriate rate limiting is in place, especially for the streaming endpoint, to prevent abuse and ensure fair usage of resources.
Monitoring: Add proper logging and monitoring for these new endpoints to track usage patterns and identify potential issues quickly.
To ensure the consistency and correctness of the new endpoints, let's verify their integration with the existing API structure:
This script will help us verify the proper integration of the new endpoints and their associated definitions within the API specification.
✅ Verification successful
Verification Successful: New Endpoints Properly Integrated
The new
/lookup-entitiesand/lookup-entities-streamendpoints are properly integrated with the existing permission-related endpoints. All necessary request and response bodies are defined, and references to these endpoints are consistent throughout the API specification.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the new endpoints are properly integrated with the existing permission-related endpoints echo "Checking integration of new endpoints:" grep -n "/v1/tenants/{tenant_id}/permissions/" docs/api-reference/apidocs.swagger.json # Verify that the new request and response bodies are properly defined echo "\nChecking new request and response body definitions:" grep -n "LookupEntitiesBody\|LookupEntitiesStreamBody\|PermissionsLookupEntityResponse\|PermissionsLookupEntityStreamResponse" docs/api-reference/apidocs.swagger.json # Check for any references to the new endpoints in other parts of the API specification echo "\nChecking references to new endpoints:" grep -n "lookup-entities\|lookupEntities" docs/api-reference/apidocs.swagger.jsonLength of output: 7573
pkg/pb/base/v1/health.pb.validate.go (1)
1-245: LGTM: Generated validation code is appropriateThe validation code for
HealthCheckRequestandHealthCheckResponsehas been correctly generated usingprotoc-gen-validate. Since there are no specific validation rules defined in the.protofile for the fields, the generatedValidateandValidateAllmethods appropriately handle these cases by returningnilwhen no violations are found.internal/engines/bulk.go (1)
268-283: Correct implementation of multiple permissions handling inPublishThe
Publishmethod now iterates over multiple permissions froms.request.GetPermissions(), submitting aPermissionCheckRequestfor each permission. This change appropriately extends the functionality to handle an array of permissions, ensuring comprehensive permission checks.internal/engines/lookup.go (2)
10-10: Importing 'sync' package is appropriate.The addition of the
"sync"package is necessary due to the use of mutexes for thread-safe operations.
128-148: Proper synchronization of 'entityIDsByPermission' map.The use of
sync.Mutexto protect access to theentityIDsByPermissionmap ensures thread safety when multiple goroutines invoke the callback function concurrently.internal/engines/entityFilter.go (17)
40-59: Direct match logic correctly identifies and publishes entitiesThe added code efficiently checks for direct matches between the subject and entrances. By publishing the result upon a match and breaking the loop, it avoids unnecessary processing.
66-69: Ensure correct parameters are passed toLinkedEntrancesIn the call to
cn.LinkedEntrances, the parameters have been updated to includerequest.GetEntrances()and the subject's entrance. Verify that this change aligns with the method's expected parameters and handles multiple entrances appropriately.
Line range hint
89-110: Update method calls to include the newpermissionChecksparameterMethods
relationEntrance,lt,attributeEntrance, andtupleToUserSetEntrancenow include the additionalpermissionChecks *VisitsMapparameter. Ensure that all calls to these methods have been updated accordingly and that the parameter is properly utilized within each method.
164-169: Review pagination logic for correctness inattributeEntranceThe pagination settings are now determined within a loop over
request.GetEntrances()to match the target entrance type. Ensure that this logic correctly handles pagination for multiple entrances and does not introduce unintended behavior.
203-203: Ensure correct parameter passed tolamethodIn the goroutine within
attributeEntrance, verify that the correctVisitsMapparameter (permissionChecksif renamed) is passed to thelamethod.
218-218: Confirm proper use ofpermissionChecksinrelationEntranceThe
relationEntrancemethod now includespermissionChecks *VisitsMap. Ensure that this parameter is utilized correctly within the method to prevent redundant permission checks.
256-261: Validate pagination logic inrelationEntranceThe pagination logic has been adjusted to dynamically set the pagination based on entrance types. Verify that this implementation correctly applies pagination and does not affect data retrieval.
295-295: PasspermissionCheckscorrectly toltinrelationEntranceWithin the goroutine, ensure that
permissionChecksis correctly passed to theltmethod to maintain consistent permission checking.
315-316: Ensure consistent parameter naming intupleToUserSetEntranceThe
tupleToUserSetEntrancemethod now acceptspermissionChecks *VisitsMap. Confirm that this parameter is consistently named and used similarly to other methods.
354-359: Review pagination adjustments intupleToUserSetEntranceThe pagination logic has been modified to accommodate multiple entrances. Verify that the changes correctly handle pagination and align with the intended functionality.
393-393: Confirm correct passing ofpermissionCheckstoltintupleToUserSetEntranceIn the goroutine, ensure that
permissionChecksis passed to theltmethod to prevent redundant permission evaluations.
407-407: Verify usage ofpermissionChecksinltmethodThe
ltmethod signature now includespermissionChecks *VisitsMap. Ensure that this parameter is correctly used within the method to track permission checks effectively.
419-422: Check parameters inLinkedEntrancescall withinltVerify that passing
request.GetEntrances()and the subject's entrance tocn.LinkedEntrancescorrectly retrieves the necessary linked entrances.
430-439: Logic for handling no linked entrances is appropriately implementedThe added code correctly handles cases where no linked entrances are found by checking for direct matches and publishing the entity if necessary.
Line range hint
448-459: Ensure recursive call toEntityFilterincludespermissionChecksIn the goroutine within the
ltmethod, confirm that the recursive call toEntityFilterincludes thepermissionChecksparameter and that the request is constructed properly.
481-481: Verify proper usage ofpermissionChecksinlamethodEnsure that after renaming, the
permissionChecksparameter is correctly utilized in thelamethod to manage permission checks.
37-37: Verify all calls toEntityFilterinclude the newpermissionChecksparameterThe
EntityFiltermethod signature now includes a new parameterpermissionChecks *VisitsMap. Please ensure that all invocations of this method have been updated to include this parameter to prevent compilation errors.Run the following script to verify:
✅ Verification successful
All calls to
EntityFilterinclude the newpermissionChecksparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to 'EntityFilter' that may be missing the 'permissionChecks' parameter. # Search for calls to 'EntityFilter' with the old parameter count (4 parameters). rg --type go --no-heading --line-number \ 'engine\.EntityFilter\(\s*[^,]+,\s*[^,]+,\s*[^,]+,\s*[^,]+\s*\)' \ -- 'internal/**/*.go' | grep -v 'func (engine \*EntityFilter) EntityFilter('Length of output: 240
Script:
#!/bin/bash # Description: Find all calls to 'EntityFilter' that may be missing the 'permissionChecks' parameter. # Search for calls to 'EntityFilter' with the old parameter count (4 parameters) across all Go files. rg --type go --no-heading --line-number \ 'EntityFilter\(\s*[^,]+,\s*[^,]+,\s*[^,]+,\s*[^,]+\s*\)' \ -- '**/*.go' | grep -v 'func (engine \*EntityFilter) EntityFilter('Length of output: 214
Script:
#!/bin/bash # Description: Using fd to locate all .go files and rg to find 'EntityFilter' calls missing the 'permissionChecks' parameter. # Find all .go files and search for 'EntityFilter' with four parameters fd --extension go --type file --hidden | xargs rg --no-heading --line-number \ 'EntityFilter\(\s*[^,]+,\s*[^,]+,\s*[^,]+,\s*[^,]+\s*\)' | grep -v 'func (engine \*EntityFilter) EntityFilter('Length of output: 190
pkg/pb/base/v1/service.pb.validate.go (1)
1731-1752: Verify the regex pattern for permission validationThe regex pattern used to validate permissions in the
Permissionsfield is^[a-zA-Z_][a-zA-Z0-9_]*$. This pattern requires that each permission starts with a letter or underscore, followed by any combination of letters, digits, or underscores. Ensure that this regex aligns with the intended permission naming conventions in your application.internal/invoke/invoke.go (1)
52-53: New methods added to theLookupinterface are properly definedThe methods
LookupEntitiesandLookupEntitiesStreamare correctly added to theLookupinterface with consistent signatures, aligning with existing methods.
| "LookupEntitiesBody": { | ||
| "type": "object", | ||
| "properties": { | ||
| "metadata": { | ||
| "$ref": "#/definitions/PermissionLookupEntityRequestMetadata", | ||
| "description": "Metadata associated with this request, required." | ||
| }, | ||
| "entity_type": { | ||
| "type": "string", | ||
| "description": "Type of the entity to lookup, required, must start with a letter and can include alphanumeric and underscore, max 64 bytes." | ||
| }, | ||
| "permissions": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "description": "Name of the permission to check, required, must start with a letter and can include alphanumeric and underscore, max 64 bytes." | ||
| }, | ||
| "subject": { | ||
| "$ref": "#/definitions/Subject", | ||
| "description": "Subject for which to check the permission, required." | ||
| }, | ||
| "context": { | ||
| "$ref": "#/definitions/Context", | ||
| "description": "Context associated with this request." | ||
| }, | ||
| "scope": { | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "$ref": "#/definitions/StringArrayValue" | ||
| }, | ||
| "description": "Scope: A map that associates entity types with lists of identifiers. Each entry\nhelps filter requests by specifying which entities are relevant to the operation." | ||
| }, | ||
| "page_size": { | ||
| "type": "integer", | ||
| "format": "int64", | ||
| "description": "page_size is the number of entities to be returned in the response.\nThe value should be between 1 and 100." | ||
| }, | ||
| "continuous_token": { | ||
| "type": "string", | ||
| "description": "continuous_token is an optional parameter used for pagination.\nIt should be the value received in the previous response." | ||
| } | ||
| }, | ||
| "description": "PermissionsLookupEntityRequest is the request message for the LookupEntities method in the Permission service." |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Detected in LookupEntitiesBody Definition
The description within LookupEntitiesBody incorrectly references PermissionsLookupEntityRequest. This inconsistency can lead to confusion and should be corrected to maintain clarity in the API documentation.
-
Action Required: Update the description to use "LookupEntitiesBody" instead of "PermissionsLookupEntityRequest".
Example:
"description": "LookupEntitiesBody is the request message for the LookupEntities method in the Permission service."
-
Further Investigation: Review other parts of the codebase to ensure that
PermissionsLookupEntityRequestis no longer in use or appropriately updated to prevent residual inconsistencies.
🔗 Analysis chain
Review of LookupEntitiesBody definition
The LookupEntitiesBody definition has been added to support the new /lookup-entities endpoint. Overall, the structure is well-defined and includes necessary fields for the lookup operation. However, there are a few points to address:
-
The
permissionsfield is an array, which differs from the singlepermissionfield in similar endpoints. This change allows for checking multiple permissions at once, which can be more efficient. However, ensure that this is intentional and documented clearly for API consumers. -
The
scopefield is a new addition. While it's described as a map for filtering entities, it might benefit from a more detailed explanation or examples in the documentation to clarify its usage. -
The description at the end of the definition refers to "PermissionsLookupEntityRequest", which is inconsistent with the actual name "LookupEntitiesBody". This should be corrected to maintain consistency.
Consider the following improvements:
- Update the description to use "LookupEntitiesBody" instead of "PermissionsLookupEntityRequest".
- Add more detailed documentation for the
scopefield, possibly including examples of its usage. - Ensure that the API documentation clearly explains the difference between this endpoint (which accepts multiple permissions) and other similar endpoints that might only accept a single permission.
Let's verify the consistency of the LookupEntitiesBody definition:
This script will help us identify any inconsistencies in naming, verify the permissions field definition, and check the usage of the new scope field across the API specification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any inconsistencies in naming
echo "Checking for naming inconsistencies:"
grep -n "PermissionsLookupEntityRequest" docs/api-reference/apidocs.swagger.json
# Verify that the permissions field is consistently defined as an array
echo "\nChecking permissions field definition:"
grep -n '"permissions":' docs/api-reference/apidocs.swagger.json
# Check for any other definitions using the scope field
echo "\nChecking usage of scope field in other definitions:"
grep -n '"scope":' docs/api-reference/apidocs.swagger.json
Length of output: 3643
| @@ -54,7 +54,7 @@ type BulkChecker struct { | |||
|
|
|||
| // callback is a function that handles the result of each permission check. | |||
| // It is called with the entity ID and the result of the permission check (e.g., allowed or denied). | |||
| callback func(entityID, ct string) | |||
| callback func(entityID, permission, ct string) | |||
There was a problem hiding this comment.
Update all implementations of callback to match the new signature
The callback function in BulkChecker now includes an additional permission parameter: func(entityID, permission, ct string). Ensure that all implementations and invocations of this callback function are updated to accept and handle the new permission parameter correctly.
| @@ -76,7 +76,7 @@ | |||
| // engine: the CheckEngine to use for permission checks | |||
| // callback: a callback function that handles the result of each permission check | |||
| // concurrencyLimit: the maximum number of concurrent permission checks | |||
| func NewBulkChecker(ctx context.Context, checker invoke.Check, typ BulkCheckerType, callback func(entityID, ct string), concurrencyLimit int) *BulkChecker { | |||
| func NewBulkChecker(ctx context.Context, checker invoke.Check, typ BulkCheckerType, callback func(entityID, permission, ct string), concurrencyLimit int) *BulkChecker { | |||
There was a problem hiding this comment.
Synchronize NewBulkChecker invocations with updated callback signature
The NewBulkChecker constructor now expects a callback function with the updated signature func(entityID, permission, ct string). Verify that all calls to NewBulkChecker across the codebase provide a callback function matching this new signature to prevent runtime errors.
| @@ -246,13 +250,13 @@ | |||
| type BulkEntityPublisher struct { | |||
| bulkChecker *BulkChecker | |||
|
|
|||
| request *base.PermissionLookupEntityRequest | |||
| request *base.PermissionsLookupEntityRequest | |||
There was a problem hiding this comment.
Ensure the correct request type is used in BulkEntityPublisher
The request field in BulkEntityPublisher has been updated to use *base.PermissionsLookupEntityRequest. Confirm that all instances where BulkEntityPublisher is instantiated pass a PermissionsLookupEntityRequest and that the previous PermissionLookupEntityRequest references are updated accordingly.
| // context to manage goroutines and cancellation | ||
| ctx context.Context | ||
| } | ||
|
|
||
| // NewBulkEntityPublisher creates a new BulkStreamer instance. | ||
| func NewBulkEntityPublisher(ctx context.Context, request *base.PermissionLookupEntityRequest, bulkChecker *BulkChecker) *BulkEntityPublisher { | ||
| func NewBulkEntityPublisher(ctx context.Context, request *base.PermissionsLookupEntityRequest, bulkChecker *BulkChecker) *BulkEntityPublisher { |
There was a problem hiding this comment.
Adjust NewBulkEntityPublisher calls to accept the new request type
The NewBulkEntityPublisher function now expects a *base.PermissionsLookupEntityRequest as its request parameter. Make sure that all calls to this constructor provide the correct type to prevent type mismatch errors.
| // Validate checks the field values on EntityIds with the rules defined in the | ||
| // proto definition for this message. If any rules are violated, the first | ||
| // error encountered is returned, or nil if there are no violations. | ||
| func (m *EntityIds) Validate() error { | ||
| return m.validate(false) | ||
| } | ||
|
|
||
| // ValidateAll checks the field values on PermissionLookupEntityResponse with | ||
| // the rules defined in the proto definition for this message. If any rules | ||
| // are violated, the result is a list of violation errors wrapped in | ||
| // PermissionLookupEntityResponseMultiError, or nil if none found. | ||
| func (m *PermissionLookupEntityResponse) ValidateAll() error { | ||
| // ValidateAll checks the field values on EntityIds with the rules defined in | ||
| // the proto definition for this message. If any rules are violated, the | ||
| // result is a list of violation errors wrapped in EntityIdsMultiError, or nil | ||
| // if none found. | ||
| func (m *EntityIds) ValidateAll() error { | ||
| return m.validate(true) | ||
| } | ||
|
|
||
| func (m *PermissionLookupEntityResponse) validate(all bool) error { | ||
| func (m *EntityIds) validate(all bool) error { | ||
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| // no validation rules for ContinuousToken | ||
|
|
||
| if len(errors) > 0 { | ||
| return PermissionLookupEntityResponseMultiError(errors) | ||
| return EntityIdsMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // PermissionLookupEntityResponseMultiError is an error wrapping multiple | ||
| // validation errors returned by PermissionLookupEntityResponse.ValidateAll() | ||
| // if the designated constraints aren't met. | ||
| type PermissionLookupEntityResponseMultiError []error | ||
| // EntityIdsMultiError is an error wrapping multiple validation errors returned |
There was a problem hiding this comment.
Ensure EntityIds validation is implemented if required
The Validate method for EntityIds is currently empty:
func (m *EntityIds) validate(all bool) error {
if m == nil {
return nil
}
var errors []error
if len(errors) > 0 {
return EntityIdsMultiError(errors)
}
return nil
}If EntityIds should have validation rules (e.g., checking for empty IDs), consider implementing the necessary validation logic to enforce data integrity.
| if m == nil { | ||
| return nil | ||
| } | ||
|
|
||
| var errors []error | ||
|
|
||
| // no validation rules for EntityId | ||
|
|
||
| // no validation rules for Permission | ||
|
|
||
| // no validation rules for ContinuousToken | ||
|
|
||
| if len(errors) > 0 { | ||
| return PermissionsLookupEntityStreamResponseMultiError(errors) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Add validation rules for fields in PermissionsLookupEntityStreamResponse
Currently, the Validate method for PermissionsLookupEntityStreamResponse contains comments indicating that there are no validation rules for EntityId, Permission, and ContinuousToken. If these fields are critical for the application's functionality, consider adding appropriate validation checks to ensure they contain valid data.
Example:
func (m *PermissionsLookupEntityStreamResponse) validate(all bool) error {
if m == nil {
return nil
}
var errors []error
+ if m.GetEntityId() == "" {
+ err := PermissionsLookupEntityStreamResponseValidationError{
+ field: "EntityId",
+ reason: "value is required",
+ }
+ if !all {
+ return err
+ }
+ errors = append(errors, err)
+ }
// Additional validation rules...
if len(errors) > 0 {
return PermissionsLookupEntityStreamResponseMultiError(errors)
}
return nil
}Committable suggestion was skipped due to low confidence.
| @@ -245,7 +247,7 @@ func (invoker *DirectInvoker) LookupEntity(ctx context.Context, request *base.Pe | |||
| ctx, span := tracer.Start(ctx, "lookup-entity", trace.WithAttributes( | |||
| attribute.KeyValue{Key: "tenant_id", Value: attribute.StringValue(request.GetTenantId())}, | |||
| attribute.KeyValue{Key: "entity_type", Value: attribute.StringValue(request.GetEntityType())}, | |||
| attribute.KeyValue{Key: "permission", Value: attribute.StringValue(request.GetPermission())}, | |||
| attribute.KeyValue{Key: "permissions", Value: attribute.StringValue(request.GetPermission())}, | |||
There was a problem hiding this comment.
Inconsistent attribute key: change "permissions" to "permission"
In the LookupEntity method, the tracing attribute key has been changed to "permissions", but request.GetPermission() returns a single permission. To maintain clarity and consistency, consider reverting the key back to "permission".
| @@ -292,7 +294,7 @@ | |||
| ctx, span := tracer.Start(ctx, "lookup-entity-stream", trace.WithAttributes( | |||
| attribute.KeyValue{Key: "tenant_id", Value: attribute.StringValue(request.GetTenantId())}, | |||
| attribute.KeyValue{Key: "entity_type", Value: attribute.StringValue(request.GetEntityType())}, | |||
| attribute.KeyValue{Key: "permission", Value: attribute.StringValue(request.GetPermission())}, | |||
| attribute.KeyValue{Key: "permissions", Value: attribute.StringValue(request.GetPermission())}, | |||
There was a problem hiding this comment.
Inconsistent attribute key: change "permissions" to "permission"
Similarly, in the LookupEntityStream method, the attribute key should be "permission" instead of "permissions" since it deals with a single permission.
| duration := time.Since(start) | ||
| invoker.lookupEntityDurationHistogram.Record(ctx, duration.Microseconds()) | ||
|
|
||
| // Increase the lookup entity count in the metrics. | ||
| invoker.lookupEntityCounter.Add(ctx, 1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define separate metrics for LookupEntities methods
The LookupEntities and LookupEntitiesStream methods are using invoker.lookupEntityDurationHistogram and incrementing invoker.lookupEntityCounter. To accurately monitor these new methods separately from LookupEntity, consider creating distinct metrics like lookupEntitiesDurationHistogram and lookupEntitiesCounter.
Also applies to: 422-426
|
Hey @ankitsheoran1, Thank you for updating the PR and rebasing it to latest version. There are some issues with naming and BulkChecker method. I have another PR affecting it. I wanted to make sure I highlight the other PR related to the bulk-checker. Here’s the link: feat: bulk permission check api is added. |
Summary by CodeRabbit
New Features
POST /v1/tenants/{tenant_id}/permissions/lookup-entitiesPOST /v1/tenants/{tenant_id}/permissions/lookup-entities-streamBug Fixes
Documentation