feat: add policy file generation and check permission function #53
Conversation
WalkthroughThe changes introduce several new files and functionalities related to permission checking and resource management in the Cerbos client. A Changes
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 (
|
akiyatomohiro
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (16)
cerbos/client/permission_checker.go (1)
10-13: Add documentation to the struct and its fields.The struct and its fields should be documented to explain their purpose, any validation rules, and usage constraints.
+// PermissionChecker handles permission verification against a dashboard service. type PermissionChecker struct { + // Service identifies the service name for permission checks Service string + // DashboardURL is the endpoint URL for the permission verification service DashboardURL string }cerbos/generator/builder.go (2)
3-11: Add documentation comments for exported types and fields.The exported types
ResourceDefinitionandActionDefinitionshould have documentation comments explaining their purpose and usage. This helps with API documentation and code clarity.Apply this diff:
+// ResourceDefinition represents a resource and its associated actions type ResourceDefinition struct { + // Resource is the fully qualified resource name Resource string + // Actions is a list of permitted actions on this resource Actions []ActionDefinition } +// ActionDefinition represents an action that can be performed on a resource +// and the roles permitted to perform it type ActionDefinition struct { + // Action is the name of the permitted action Action string + // Roles is a list of roles allowed to perform this action Roles []string }
37-46: Consider sorting results for consistent output.The
Buildmethod should sort the results to ensure consistent output order across multiple builds.Apply this diff:
+import "sort" func (b *ResourceBuilder) Build() []ResourceDefinition { result := make([]ResourceDefinition, 0, len(b.resources)) for resource, actions := range b.resources { result = append(result, ResourceDefinition{ Resource: b.serviceName + ":" + resource, Actions: actions, }) } + // Sort for consistent output + sort.Slice(result, func(i, j int) bool { + return result[i].Resource < result[j].Resource + }) return result }cerbos/client/check.go (2)
24-34: Enhance client configuration with timeout and validation.The client initialization could be improved with:
- HTTP client timeout configuration
- Dashboard URL validation
- Option to customize HTTP client settings
Consider this enhancement:
type Client struct { httpClient *http.Client dashboardURL string } +type ClientOption func(*Client) + +func WithHTTPClient(client *http.Client) ClientOption { + return func(c *Client) { + c.httpClient = client + } +} + +func WithTimeout(timeout time.Duration) ClientOption { + return func(c *Client) { + c.httpClient.Timeout = timeout + } +} + -func NewClient(dashboardURL string) *Client { +func NewClient(dashboardURL string, opts ...ClientOption) (*Client, error) { + if _, err := url.Parse(dashboardURL); err != nil { + return nil, fmt.Errorf("invalid dashboard URL: %w", err) + } + + c := &Client{ + httpClient: &http.Client{ + Timeout: 30 * time.Second, + }, + dashboardURL: dashboardURL, + } + + for _, opt := range opts { + opt(c) + } + - return &Client{ - httpClient: &http.Client{}, - dashboardURL: dashboardURL, - } + return c, nil }
55-66: Enhance CheckPermission with timeout, retries, and rate limiting.The permission check could be more robust with:
- Context timeout handling
- Retry mechanism for transient failures
- Rate limiting to prevent API abuse
Consider implementing a circuit breaker pattern and rate limiting:
+type ClientConfig struct { + MaxRetries int + RetryDelay time.Duration + RateLimit rate.Limit + BurstLimit int +} + +func (c *Client) CheckPermission(ctx context.Context, authInfo *appx.AuthInfo, input CheckPermissionInput) (bool, error) { + // Add rate limiting + if err := c.rateLimiter.Wait(ctx); err != nil { + return false, fmt.Errorf("rate limit exceeded: %w", err) + } + if err := c.validateInput(authInfo); err != nil { return false, err } - req, err := c.createRequest(ctx, authInfo, input) - if err != nil { - return false, err - } + var result bool + err := retry.Do( + func() error { + req, err := c.createRequest(ctx, authInfo, input) + if err != nil { + return err + } + + result, err = c.executeRequest(req) + return err + }, + retry.Context(ctx), + retry.Attempts(c.config.MaxRetries), + retry.Delay(c.config.RetryDelay), + retry.OnRetry(func(n uint, err error) { + log.Printf("Retrying permission check after error: %v (attempt %d)", err, n+1) + }), + ) - return c.executeRequest(req) + return result, err }cerbos/generator/policy_test.go (2)
11-112: Well-structured table-driven tests with comprehensive test cases.The test cases effectively cover the main scenarios including successful generations and error cases.
Consider adding these additional test cases to improve coverage:
- Empty actions list
- Duplicate resource names
- Resource names with special characters
Example test case:
{ name: "duplicate resource names", serviceName: "flow", defineResources: func(builder *ResourceBuilder) []ResourceDefinition { return builder. AddResource("project", []ActionDefinition{ NewActionDefinition("read", []string{"owner"}), }). AddResource("project", []ActionDefinition{ NewActionDefinition("write", []string{"owner"}), }). Build() }, wantErr: "duplicate resource name: project", },
114-142: Test execution logic is robust with proper cleanup.The implementation properly handles file operations and verifies both content and file count.
Consider using
filepath.FromSlash()when constructing expected file paths to ensure cross-platform compatibility:- content, err := os.ReadFile(filepath.Join(testDir, filename)) + content, err := os.ReadFile(filepath.Join(testDir, filepath.FromSlash(filename)))cerbos/client/check_test.go (2)
14-21: Consider adding edge cases to the NewClient test.While the basic initialization test is good, consider adding test cases for:
- Malformed dashboard URLs
- Custom HTTP client configurations
- Empty/invalid dashboard URLs
// Example additional test cases: func TestClient_NewClient(t *testing.T) { tests := []struct { name string dashboardURL string wantErr bool }{ { name: "valid URL", dashboardURL: "http://test-dashboard", wantErr: false, }, { name: "invalid URL", dashboardURL: "://invalid", wantErr: true, }, { name: "empty URL", dashboardURL: "", wantErr: true, }, } // ... test execution }
23-109: Enhance test coverage with additional scenarios.The current test cases cover the basic flows well, but consider adding tests for:
- Invalid service/resource/action values
- Context cancellation handling
- Request timeout scenarios
- Malformed server responses
// Example additional test cases to add: { name: "error - invalid service", authInfo: &appx.AuthInfo{Token: "test-token"}, input: CheckPermissionInput{ Service: "", // empty service Resource: "project", Action: "read", }, wantErr: "invalid service", }, { name: "error - context cancelled", authInfo: &appx.AuthInfo{Token: "test-token"}, input: CheckPermissionInput{ Service: "flow", Resource: "project", Action: "read", }, useContext: context.WithCancel, // implement context cancellation wantErr: "context canceled", },cerbos/generator/builder_test.go (3)
9-28: Consider adding more test cases for better coverage.While the basic test case is good, consider adding cases for:
- Empty serviceName
- Special characters in serviceName
- Very long serviceName
Example addition:
tests := []struct { name string serviceName string }{ { name: "create new builder", serviceName: "flow", }, + { + name: "empty service name", + serviceName: "", + }, + { + name: "special characters in service name", + serviceName: "[email protected]", + }, + { + name: "very long service name", + serviceName: strings.Repeat("a", 256), + }, }
30-54: Enhance test coverage with edge cases.Consider adding test cases for:
- Empty action string
- Empty roles array
- Duplicate roles
- Invalid role names
Example addition:
tests := []struct { name string action string roles []string expected ActionDefinition }{ { name: "create action definition", action: "read", roles: []string{"owner", "reader"}, expected: ActionDefinition{ Action: "read", Roles: []string{"owner", "reader"}, }, }, + { + name: "empty action", + action: "", + roles: []string{"owner"}, + expected: ActionDefinition{ + Action: "", + Roles: []string{"owner"}, + }, + }, + { + name: "empty roles", + action: "write", + roles: []string{}, + expected: ActionDefinition{ + Action: "write", + Roles: []string{}, + }, + }, + { + name: "duplicate roles", + action: "read", + roles: []string{"owner", "owner"}, + expected: ActionDefinition{ + Action: "read", + Roles: []string{"owner", "owner"}, + }, + }, }
94-182: LGTM! Well-structured test cases with good coverage.The test cases cover both single and multiple resources with various action combinations. The resource name formatting (
flow:project) is consistently tested.Minor suggestion: Consider adding a test case for resource names with special characters to verify the formatting behavior.
Example addition:
{ name: "build multiple resources", serviceName: "flow", // ... existing test case ... }, + { + name: "resource name with special characters", + serviceName: "flow-service", + resources: map[string][]ActionDefinition{ + "project/v1": { + { + Action: "read", + Roles: []string{"owner"}, + }, + }, + }, + expected: []ResourceDefinition{ + { + Resource: "flow-service:project/v1", + Actions: []ActionDefinition{ + { + Action: "read", + Roles: []string{"owner"}, + }, + }, + }, + }, + },cerbos/generator/policy.go (4)
33-33: Clarify the error message whendefineResourcesis nilThe error message could be more precise. Consider updating it to clearly indicate which parameter is nil.
Apply this diff to improve the error message:
- return fmt.Errorf("define resources function is required") + return fmt.Errorf("defineResources function cannot be nil")
41-41: Include resource name in the error message for better debuggingIncluding the invalid resource name in the error message can help with debugging.
Apply this diff to enhance the error message:
- return fmt.Errorf("invalid resource name") + return fmt.Errorf("invalid resource name: %s", resource.Resource)
56-56: Consider makingEffectconfigurableThe
Effectfield is currently hardcoded as"EFFECT_ALLOW". For future flexibility, consider making this configurable to support different effects like"EFFECT_DENY".You can update the
Rulestruct andActionDefinitionto include anEffectfield or determine the effect based on context.type Rule struct { Actions []string `yaml:"actions"` - Effect string `yaml:"effect"` + Effect string `yaml:"effect"` // Could be set per action Roles []string `yaml:"roles"` } // Within the loop over actions for _, action := range resource.Actions { rule := Rule{ Actions: []string{action.Action}, - Effect: "EFFECT_ALLOW", + Effect: action.Effect, // Assume ActionDefinition has an Effect field Roles: action.Roles, }
74-76: Handle partial failures when writing policy filesCurrently, if an error occurs while writing a policy file, the function returns immediately, potentially leaving some policies unwritten. Consider logging the error and continuing to process the remaining resources or accumulating errors to return a comprehensive error report.
Modify the code to collect errors:
+ var errs []error for _, resource := range resources { // ... existing code ... if err := os.WriteFile(outputPath, data, 0644); err != nil { - return fmt.Errorf("failed to write file: %w", err) + errs = append(errs, fmt.Errorf("failed to write file '%s': %w", outputPath, err)) + continue } } + if len(errs) > 0 { + return fmt.Errorf("encountered errors during policy generation: %v", errs) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
cerbos/client/check.go(1 hunks)cerbos/client/check_test.go(1 hunks)cerbos/client/permission_checker.go(1 hunks)cerbos/generator/builder.go(1 hunks)cerbos/generator/builder_test.go(1 hunks)cerbos/generator/policy.go(1 hunks)cerbos/generator/policy_test.go(1 hunks)go.mod(1 hunks)
🔇 Additional comments (6)
cerbos/client/permission_checker.go (1)
1-9: LGTM! Clean package structure and imports.
The package structure is well-organized with minimal necessary imports.
cerbos/generator/policy_test.go (2)
1-9: LGTM! Appropriate imports for testing requirements.
The package name and imports are well-structured, using standard testing packages and the testify/assert package for clean assertions.
33-44: Verify YAML structure against Cerbos policy schema.
While the YAML structure appears correct, it would be beneficial to validate it against the official Cerbos policy schema.
Also applies to: 62-78, 79-91
✅ Verification successful
YAML structure is validated through Go struct definitions
The YAML structure is implicitly validated through Go struct type definitions in cerbos/generator/policy.go. The CerbosPolicy, ResourcePolicy, and Rule structs enforce the schema structure during YAML unmarshaling. The code also includes basic validation like checking for empty resource names.
Key validations in place:
- Required fields through struct tags
- Type safety for fields (string, arrays)
- Predefined API version ("api.cerbos.dev/v1")
- Non-empty resource name check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Cerbos schema validation in tests
# Expected: Find schema validation code or consider adding it
# Look for schema validation
rg -g '!vendor' -g '*.go' 'yaml.v[23]?.Unmarshal|policy.*schema|cerbos.*schema'
Length of output: 81
Script:
#!/bin/bash
# Let's check for YAML validation in test files and policy-related code
rg -g '*.go' 'yaml\.' -A 3
# Also check for any policy validation or verification code
rg -g '*.go' 'ValidatePolicy|Validate.*Policy|policy.*validation' -A 3
# Look for test helper functions that might handle policy validation
rg -g '*_test.go' 'func.*Test.*Policy|func.*policy' -A 3
Length of output: 1872
Script:
#!/bin/bash
# Let's check the policy.go file to understand the policy handling
cat cerbos/generator/policy.go
# Also check if there are any validation tests
rg -g '*_test.go' 'invalid.*policy|policy.*invalid|policy.*error' -A 3
# Look for any schema or validation-related files
fd -e yaml -e json schema
Length of output: 2074
cerbos/client/check_test.go (1)
1-12: LGTM! Clean import organization.
The imports are well-organized and all dependencies are properly utilized throughout the test file.
cerbos/generator/builder_test.go (1)
1-8: LGTM! Clean and minimal imports.
The package name matches the directory and imports are appropriate for the test file.
go.mod (1)
47-47: 🛠️ Refactor suggestion
Consider consolidating YAML libraries.
The codebase already uses github.com/goccy/go-yaml v1.11.3. Adding gopkg.in/yaml.v2 as another YAML parser could lead to:
- Inconsistent YAML parsing behavior across the codebase
- Increased maintenance overhead
- Potential confusion for developers
Let's check the YAML parser usage in the codebase:
Consider one of these approaches:
- Use the existing
github.com/goccy/go-yamlfor the new policy file generation - If
yaml.v2has specific features needed, consider migrating the entire codebase to use it consistently
| func NewPermissionChecker(service string, dashboardURL string) *PermissionChecker { | ||
| return &PermissionChecker{ | ||
| Service: service, | ||
| DashboardURL: dashboardURL, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation to the constructor.
The constructor should validate its inputs and be properly documented.
+// NewPermissionChecker creates a new permission checker instance.
+// Returns an error if service is empty or dashboardURL is invalid.
func NewPermissionChecker(service string, dashboardURL string) *PermissionChecker {
+ if service == "" {
+ return nil, fmt.Errorf("service name cannot be empty")
+ }
+ if dashboardURL == "" {
+ return nil, fmt.Errorf("dashboard URL cannot be empty")
+ }
+ if _, err := url.Parse(dashboardURL); err != nil {
+ return nil, fmt.Errorf("invalid dashboard URL: %w", err)
+ }
return &PermissionChecker{
Service: service,
DashboardURL: dashboardURL,
- }
+ }, nil
}Committable suggestion skipped: line range outside the PR's diff.
| func (p *PermissionChecker) CheckPermission(ctx context.Context, authInfo *appx.AuthInfo, resource string, action string) (bool, error) { | ||
| if p == nil { | ||
| return false, fmt.Errorf("permission checker not found") | ||
| } | ||
|
|
||
| if authInfo == nil { | ||
| return false, fmt.Errorf("auth info not found") | ||
| } | ||
|
|
||
| input := CheckPermissionInput{ | ||
| Service: p.Service, | ||
| Resource: resource, | ||
| Action: action, | ||
| } | ||
|
|
||
| client := NewClient(p.DashboardURL) | ||
| return client.CheckPermission(ctx, authInfo, input) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed for the CheckPermission method.
- Creating a new client for each permission check is inefficient. Consider storing the client as a field in the PermissionChecker struct.
- The method lacks proper documentation and parameter validation.
- No timeout management for the context.
+// CheckPermission verifies if the authenticated user has permission to perform
+// the specified action on the given resource.
+// Returns false and an error if the check fails or times out.
func (p *PermissionChecker) CheckPermission(ctx context.Context, authInfo *appx.AuthInfo, resource string, action string) (bool, error) {
if p == nil {
- return false, fmt.Errorf("permission checker not found")
+ return false, fmt.Errorf("permission checker is nil")
}
if authInfo == nil {
- return false, fmt.Errorf("auth info not found")
+ return false, fmt.Errorf("authentication info is required")
+ }
+
+ if resource == "" {
+ return false, fmt.Errorf("resource cannot be empty")
+ }
+
+ if action == "" {
+ return false, fmt.Errorf("action cannot be empty")
}
input := CheckPermissionInput{
@@ -34,6 +38,11 @@
Action: action,
}
- client := NewClient(p.DashboardURL)
- return client.CheckPermission(ctx, authInfo, input)
+ // Add timeout if not already set
+ if _, ok := ctx.Deadline(); !ok {
+ var cancel context.CancelFunc
+ ctx, cancel = context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+ }
+
+ return p.client.CheckPermission(ctx, authInfo, input)
}Additionally, update the struct to include the client:
type PermissionChecker struct {
Service string
DashboardURL string
+ client *Client
}
func NewPermissionChecker(service string, dashboardURL string) (*PermissionChecker, error) {
+ client := NewClient(dashboardURL)
return &PermissionChecker{
Service: service,
DashboardURL: dashboardURL,
+ client: client,
}, nil
}Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Based on the verification results, I can now provide the final response:
Security implementation in permission checking is adequate but needs HTTPS enforcement
The implementation shows proper security handling:
- Authentication validation is present with proper error handling
- GraphQL client properly validates auth info before making requests
- Error cases are properly handled and returned
However, there are two security recommendations:
- The client accepts any URL scheme (both HTTP and HTTPS) as shown in the test cases. The
NewClientfunction should validate and enforce HTTPS URLs in production environments. - Consider adding logging for permission check failures to aid in security auditing.
Example locations that need attention:
cerbos/client/check.go: Add URL scheme validation inNewClientcerbos/client/check.go: Add logging when permission checks fail
🔗 Analysis chain
Verify security implications of permission checking.
Since this method is crucial for security decisions, we should verify:
- How authentication failures are handled in the GraphQL client
- If there's proper logging of permission check failures
- If the dashboard URL is using HTTPS
Let me generate additional verification scripts to check the client implementation and URL validation specifically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the client handles authentication and uses HTTPS
# Look for authentication error handling in the client
echo "Checking authentication error handling:"
ast-grep --pattern 'func (c *Client) CheckPermission($ctx, $auth, $input) {
$$$
if $err != nil {
$$$
}
$$$
}'
# Check if there's logging for permission failures
echo "Checking logging implementation:"
rg -A 2 "log\."
# Verify HTTPS usage
echo "Checking HTTPS URL validation:"
rg -A 2 "url\.Parse"
Length of output: 9420
Script:
#!/bin/bash
# Check client implementation and URL validation
# Look for the Client implementation and permission checking
echo "Checking Client implementation:"
ast-grep --pattern 'type Client struct {
$$$
}'
ast-grep --pattern 'func NewClient($url) *Client {
$$$
}'
# Check URL validation in the client
echo "Checking URL validation in client:"
rg -A 5 "NewClient" cerbos/client/
# Check error handling in permission checking
echo "Checking error handling in permission checking:"
rg -A 5 "CheckPermission" cerbos/client/
Length of output: 9887
| func (b *ResourceBuilder) AddResource(resource string, actions []ActionDefinition) *ResourceBuilder { | ||
| b.resources[resource] = actions | ||
| return b | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for AddResource method.
The method should validate its inputs to prevent invalid resource definitions.
Apply this diff:
+// AddResource adds a resource and its associated actions to the builder.
+// It panics if resource is empty or if no actions are provided.
func (b *ResourceBuilder) AddResource(resource string, actions []ActionDefinition) *ResourceBuilder {
+ if resource == "" {
+ panic("resource cannot be empty")
+ }
+ if len(actions) == 0 {
+ panic("at least one action must be specified")
+ }
b.resources[resource] = actions
return b
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b *ResourceBuilder) AddResource(resource string, actions []ActionDefinition) *ResourceBuilder { | |
| b.resources[resource] = actions | |
| return b | |
| } | |
| // AddResource adds a resource and its associated actions to the builder. | |
| // It panics if resource is empty or if no actions are provided. | |
| func (b *ResourceBuilder) AddResource(resource string, actions []ActionDefinition) *ResourceBuilder { | |
| if resource == "" { | |
| panic("resource cannot be empty") | |
| } | |
| if len(actions) == 0 { | |
| panic("at least one action must be specified") | |
| } | |
| b.resources[resource] = actions | |
| return b | |
| } |
| func NewActionDefinition(action string, roles []string) ActionDefinition { | ||
| return ActionDefinition{ | ||
| Action: action, | ||
| Roles: roles, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation for NewActionDefinition.
The constructor should validate its inputs and include documentation.
Apply this diff:
+// NewActionDefinition creates a new ActionDefinition with the specified action
+// and permitted roles. It panics if action is empty or no roles are provided.
func NewActionDefinition(action string, roles []string) ActionDefinition {
+ if action == "" {
+ panic("action cannot be empty")
+ }
+ if len(roles) == 0 {
+ panic("at least one role must be specified")
+ }
return ActionDefinition{
Action: action,
Roles: roles,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func NewActionDefinition(action string, roles []string) ActionDefinition { | |
| return ActionDefinition{ | |
| Action: action, | |
| Roles: roles, | |
| } | |
| } | |
| // NewActionDefinition creates a new ActionDefinition with the specified action | |
| // and permitted roles. It panics if action is empty or no roles are provided. | |
| func NewActionDefinition(action string, roles []string) ActionDefinition { | |
| if action == "" { | |
| panic("action cannot be empty") | |
| } | |
| if len(roles) == 0 { | |
| panic("at least one role must be specified") | |
| } | |
| return ActionDefinition{ | |
| Action: action, | |
| Roles: roles, | |
| } | |
| } |
| type ResourceBuilder struct { | ||
| serviceName string | ||
| resources map[string][]ActionDefinition | ||
| } | ||
|
|
||
| func NewResourceBuilder(serviceName string) *ResourceBuilder { | ||
| return &ResourceBuilder{ | ||
| serviceName: serviceName, | ||
| resources: make(map[string][]ActionDefinition), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation for ResourceBuilder.
The ResourceBuilder type and its constructor need documentation. Additionally, the constructor should validate the serviceName parameter.
Apply this diff:
+// ResourceBuilder facilitates the construction of resource definitions
+// for a specific service
type ResourceBuilder struct {
serviceName string
resources map[string][]ActionDefinition
}
+// NewResourceBuilder creates a new ResourceBuilder for the specified service.
+// It returns an error if serviceName is empty.
func NewResourceBuilder(serviceName string) *ResourceBuilder {
+ if serviceName == "" {
+ panic("serviceName cannot be empty")
+ }
return &ResourceBuilder{
serviceName: serviceName,
resources: make(map[string][]ActionDefinition),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ResourceBuilder struct { | |
| serviceName string | |
| resources map[string][]ActionDefinition | |
| } | |
| func NewResourceBuilder(serviceName string) *ResourceBuilder { | |
| return &ResourceBuilder{ | |
| serviceName: serviceName, | |
| resources: make(map[string][]ActionDefinition), | |
| } | |
| } | |
| // ResourceBuilder facilitates the construction of resource definitions | |
| // for a specific service | |
| type ResourceBuilder struct { | |
| serviceName string | |
| resources map[string][]ActionDefinition | |
| } | |
| // NewResourceBuilder creates a new ResourceBuilder for the specified service. | |
| // It returns an error if serviceName is empty. | |
| func NewResourceBuilder(serviceName string) *ResourceBuilder { | |
| if serviceName == "" { | |
| panic("serviceName cannot be empty") | |
| } | |
| return &ResourceBuilder{ | |
| serviceName: serviceName, | |
| resources: make(map[string][]ActionDefinition), | |
| } | |
| } |
| CheckPermission: struct { | ||
| Allowed bool "json:\"allowed\"" | ||
| }{ | ||
| Allowed: true, | ||
| }, | ||
| }, | ||
| }, | ||
| wantAllowed: true, | ||
| }, | ||
| { | ||
| name: "success - permission denied", | ||
| authInfo: &appx.AuthInfo{ | ||
| Token: "test-token", | ||
| }, | ||
| input: CheckPermissionInput{ | ||
| Service: "flow", | ||
| Resource: "project", | ||
| Action: "write", | ||
| }, | ||
| serverStatus: http.StatusOK, | ||
| serverResp: CheckPermissionResponse{ | ||
| Data: struct { | ||
| CheckPermission struct { | ||
| Allowed bool "json:\"allowed\"" | ||
| } "json:\"checkPermission\"" | ||
| }{ | ||
| CheckPermission: struct { | ||
| Allowed bool "json:\"allowed\"" | ||
| }{ | ||
| Allowed: false, | ||
| }, | ||
| }, | ||
| }, | ||
| wantAllowed: false, | ||
| }, | ||
| { | ||
| name: "error - nil auth info", | ||
| authInfo: nil, | ||
| input: CheckPermissionInput{ | ||
| Service: "flow", | ||
| Resource: "project", | ||
| Action: "read", | ||
| }, | ||
| wantErr: "auth info is required", | ||
| }, | ||
| { | ||
| name: "error - server error", | ||
| authInfo: &appx.AuthInfo{ | ||
| Token: "test-token", | ||
| }, | ||
| input: CheckPermissionInput{ | ||
| Service: "flow", | ||
| Resource: "project", | ||
| Action: "read", | ||
| }, | ||
| serverStatus: http.StatusInternalServerError, | ||
| wantErr: "failed to decode response", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| assert.Equal(t, "POST", r.Method) | ||
| assert.Equal(t, "/api/graphql", r.URL.Path) | ||
| assert.Equal(t, "application/json", r.Header.Get("Content-Type")) | ||
| if tt.authInfo != nil { | ||
| assert.Equal(t, "Bearer "+tt.authInfo.Token, r.Header.Get("Authorization")) | ||
| } | ||
|
|
||
| var gqlRequest GraphQLQuery | ||
| err := json.NewDecoder(r.Body).Decode(&gqlRequest) | ||
| assert.NoError(t, err) | ||
| assert.Contains(t, gqlRequest.Query, "query CheckPermission") | ||
| assert.Contains(t, gqlRequest.Query, "checkPermission") | ||
|
|
||
| w.WriteHeader(tt.serverStatus) | ||
| if tt.serverStatus == http.StatusOK { | ||
| if err := json.NewEncoder(w).Encode(tt.serverResp); err != nil { | ||
| t.Fatalf("failed to encode response: %v", err) | ||
| return | ||
| } | ||
| } | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| client := NewClient(server.URL) | ||
| allowed, err := client.CheckPermission(context.Background(), tt.authInfo, tt.input) | ||
|
|
||
| if tt.wantErr != "" { | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.wantErr) | ||
| return | ||
| } | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.Equal(t, tt.wantAllowed, allowed) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage needs improvement
The current test file only covers Client methods but misses testing for the PermissionChecker type. Key missing test coverage:
NewPermissionCheckerconstructorPermissionChecker.CheckPermissionmethod including its edge cases:- nil checker
- nil auth info
- successful permission check
- permission denied case
🔗 Analysis chain
Verify test coverage against implementation.
The test file provides good coverage, but let's verify it covers all methods and edge cases from the implementation.
Let me try a different approach to verify the test coverage.
Now I can see the complete implementation and test files. Let me verify one more aspect about the permission_checker.go file to ensure we have a complete picture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all exported methods in check.go are covered by tests
# Find all exported methods in check.go
echo "Checking for exported methods in check.go:"
ast-grep --pattern 'func ($c Client) $method($$$) {$$$}' cerbos/client/check.go
# Find corresponding test methods
echo "Checking for corresponding test methods:"
ast-grep --pattern 'func Test$$$($$$) {$$$}' cerbos/client/check_test.go
Length of output: 342
Script:
#!/bin/bash
# Let's first check if check.go exists and see its content
echo "Content of check.go:"
cat cerbos/client/check.go
# Also check for any other potential implementation files
echo -e "\nListing all go files in cerbos/client:"
fd -e go . cerbos/client/
Length of output: 3005
Script:
#!/bin/bash
# Check the content of permission_checker.go to see if it contains any additional functionality that needs testing
echo "Content of permission_checker.go:"
cat cerbos/client/permission_checker.go
Length of output: 936
| func TestResourceBuilder_AddResource(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| serviceName string | ||
| resource string | ||
| actions []ActionDefinition | ||
| expected map[string][]ActionDefinition | ||
| }{ | ||
| { | ||
| name: "add single resource", | ||
| serviceName: "flow", | ||
| resource: "project", | ||
| actions: []ActionDefinition{ | ||
| { | ||
| Action: "read", | ||
| Roles: []string{"owner", "reader"}, | ||
| }, | ||
| }, | ||
| expected: map[string][]ActionDefinition{ | ||
| "project": { | ||
| { | ||
| Action: "read", | ||
| Roles: []string{"owner", "reader"}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| builder := NewResourceBuilder(tt.serviceName) | ||
| builder.AddResource(tt.resource, tt.actions) | ||
| assert.Equal(t, tt.expected, builder.resources) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for resource management scenarios.
The current test only covers adding a single resource. Consider adding test cases for:
- Adding multiple resources
- Overwriting an existing resource
- Adding a resource with no actions
- Adding a resource with duplicate actions
Example addition:
tests := []struct {
name string
serviceName string
resource string
actions []ActionDefinition
expected map[string][]ActionDefinition
}{
{
name: "add single resource",
serviceName: "flow",
resource: "project",
actions: []ActionDefinition{
{
Action: "read",
Roles: []string{"owner", "reader"},
},
},
expected: map[string][]ActionDefinition{
"project": {
{
Action: "read",
Roles: []string{"owner", "reader"},
},
},
},
},
+ {
+ name: "overwrite existing resource",
+ serviceName: "flow",
+ resource: "project",
+ actions: []ActionDefinition{
+ {
+ Action: "write",
+ Roles: []string{"owner"},
+ },
+ },
+ expected: map[string][]ActionDefinition{
+ "project": {
+ {
+ Action: "write",
+ Roles: []string{"owner"},
+ },
+ },
+ },
+ },
+ {
+ name: "add resource with no actions",
+ serviceName: "flow",
+ resource: "project",
+ actions: []ActionDefinition{},
+ expected: map[string][]ActionDefinition{
+ "project": {},
+ },
+ },
}Committable suggestion skipped: line range outside the PR's diff.
| filename := strings.ReplaceAll(resource.Resource, ":", "_") | ||
| outputPath := filepath.Join(outputDir, fmt.Sprintf("%s.yaml", filename)) |
There was a problem hiding this comment.
Sanitize filenames to prevent invalid or unsafe characters
Replacing colons with underscores might not be sufficient to ensure the filename is safe. Consider further sanitizing resource.Resource to remove or replace other potentially problematic characters, such as slashes, backslashes, or special characters.
You can use a function like this to sanitize filenames:
import "regexp"
// Helper function to sanitize filenames
func sanitizeFilename(name string) string {
// Replace any non-word character with an underscore
reg := regexp.MustCompile(`[^\w]+`)
return reg.ReplaceAllString(name, "_")
}Then update the filename assignment:
- filename := strings.ReplaceAll(resource.Resource, ":", "_")
+ filename := sanitizeFilename(resource.Resource)| if err := os.MkdirAll(outputDir, 0755); err != nil { | ||
| return fmt.Errorf("failed to create directory: %w", err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move directory creation outside of the loop to improve efficiency
Calling os.MkdirAll(outputDir, 0755) inside the loop can result in redundant system calls since outputDir remains constant. Move this directory creation before the loop to avoid unnecessary overhead.
Apply this diff to move the directory creation:
func GeneratePolicies(serviceName string, defineResources DefineResourcesFunc, outputDir string) error {
+ if err := os.MkdirAll(outputDir, 0755); err != nil {
+ return fmt.Errorf("failed to create directory: %w", err)
+ }
for _, resource := range resources {
- if err := os.MkdirAll(outputDir, 0755); err != nil {
- return fmt.Errorf("failed to create directory: %w", err)
- }Committable suggestion skipped: line range outside the PR's diff.
| for _, action := range resource.Actions { | ||
| rule := Rule{ | ||
| Actions: []string{action.Action}, | ||
| Effect: "EFFECT_ALLOW", | ||
| Roles: action.Roles, | ||
| } | ||
| policy.ResourcePolicy.Rules = append(policy.ResourcePolicy.Rules, rule) | ||
| } |
There was a problem hiding this comment.
Check for empty actions to prevent generating policies without rules
If resource.Actions is empty, the generated policy will have no rules, resulting in potentially invalid policy files. Consider adding a check to ensure resource.Actions is not empty before proceeding.
Apply this diff to add the check:
+ if len(resource.Actions) == 0 {
+ return fmt.Errorf("no actions defined for resource: %s", resource.Resource)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, action := range resource.Actions { | |
| rule := Rule{ | |
| Actions: []string{action.Action}, | |
| Effect: "EFFECT_ALLOW", | |
| Roles: action.Roles, | |
| } | |
| policy.ResourcePolicy.Rules = append(policy.ResourcePolicy.Rules, rule) | |
| } | |
| if len(resource.Actions) == 0 { | |
| return fmt.Errorf("no actions defined for resource: %s", resource.Resource) | |
| } | |
| for _, action := range resource.Actions { | |
| rule := Rule{ | |
| Actions: []string{action.Action}, | |
| Effect: "EFFECT_ALLOW", | |
| Roles: action.Roles, | |
| } | |
| policy.ResourcePolicy.Rules = append(policy.ResourcePolicy.Rules, rule) | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Clientstruct for interacting with a GraphQL API for permission checks.PermissionCheckerstruct for managing permission checks.ResourceBuilderand related types.Bug Fixes
Tests
Dependencies
gopkg.in/yaml.v2 v2.4.0.