feat(api): authorization-and-roles-management-service #580
Conversation
WalkthroughThe changes introduce a new environment variable in the Changes
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- api/.env.example (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
api/.env.example (2)
9-9: Verify the use of HTTP for dashboard hostI noticed that the new
REEARTH_DASHBOARD_HOSTvariable uses "http" while other localhost URLs in this file use "https". While using HTTP for localhost is common in development, this inconsistency might lead to confusion or potential security issues if not handled correctly in the application code.Consider using HTTPS for consistency:
-REERTH_DASHBOARD_HOST=http://localhost:8090 +REEARTH_DASHBOARD_HOST=https://localhost:8090If HTTP is intentional for the dashboard host, please ensure that the application code handles this difference appropriately and consider adding a comment explaining why HTTP is used here.
8-9: Clarify the scope of the authorization and roles management serviceThe PR title mentions "authorization-and-roles-management-service", but the only change observed in this file is the addition of a dashboard host variable. This seems to be a discrepancy between the expected scope of the PR and the actual changes implemented.
Could you please provide more information about:
- The implementation of the authorization and roles management service?
- Any additional files or changes that are part of this PR but not visible in the current diff?
- How the dashboard host relates to the authorization and roles management service?
This information will help in understanding the full scope of the changes and ensure a more comprehensive review.
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service
branch
from
3194248 to
9db6ea8
Compare
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service
branch
from
24b58b1 to
1043eea
Compare
akiyatomohiro
changed the title
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service
branch
2 times, most recently
from
e74071e to
9910a50
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (30)
api/cmd/policy-generator/main.go (2)
10-18: Add success logging and configuration validation.While the implementation is functional, consider these improvements for better operational visibility and robustness:
func main() { + // Log start of policy generation + log.Println("Starting policy generation...") + + // Validate configuration + if rbac.PolicyFileDir == "" { + log.Fatal("PolicyFileDir is not configured") + } + if err := generator.GeneratePolicies( rbac.ServiceName, rbac.DefineResources, rbac.PolicyFileDir, ); err != nil { log.Fatalf("Failed to generate policies: %v", err) } + log.Println("Successfully generated policies") }
16-17: Consider graceful error handling for CI environments.Using
log.Fatalimmediately terminates the program, which might not be ideal for CI environments where you might want to perform cleanup or additional logging before exit.- log.Fatalf("Failed to generate policies: %v", err) + log.Printf("Failed to generate policies: %v", err) + // Optionally perform cleanup + os.Exit(1)api/Makefile (1)
22-23: Add documentation and validation for the policy generator target.While the implementation is straightforward, it would benefit from additional documentation and validation.
Consider applying these improvements:
+# gen-policies generates authorization policy files for the flow service +# Output: Policy files in the specified format for cerbos gen-policies: + @which go > /dev/null || (echo "error: go is required" && exit 1) + @test -d ./cmd/policy-generator || (echo "error: policy-generator not found" && exit 1) go run ./cmd/policy-generatorapi/internal/usecase/interactor/utils_test.go (3)
9-11: Add documentation for the mock struct.Consider adding documentation to explain the purpose and usage of this mock, especially since it's a crucial testing utility for the authorization system.
+// mockPermissionChecker provides a mock implementation of permission checking for testing. +// It allows injecting custom permission checking behavior through checkPermissionFunc. type mockPermissionChecker struct { checkPermissionFunc func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) }
13-17: Improve constructor documentation and parameter naming.The constructor would benefit from documentation and a more descriptive parameter name.
+// NewMockPermissionChecker creates a new mock permission checker with the specified checking behavior. +// If no checking function is provided (nil), the mock will always return true. -func NewMockPermissionChecker(checkFunc func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error)) *mockPermissionChecker { +func NewMockPermissionChecker(permissionCheckFn func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error)) *mockPermissionChecker { return &mockPermissionChecker{ - checkPermissionFunc: checkFunc, + checkPermissionFunc: permissionCheckFn, } }
1-24: Consider enhancing test coverage and mock flexibility.The mock implementation is well-structured, but consider these architectural improvements:
- Add helper methods for common permission scenarios to reduce test boilerplate
- Consider implementing a builder pattern for more flexible mock configuration
- Add methods to track permission check calls for more detailed test assertions
This would make the tests more maintainable and comprehensive.
.github/workflows/ci_policies.yml (3)
1-6: Consider adding workflow triggers for direct policy file changes.While the workflow_call trigger allows this workflow to be called from other workflows, consider adding direct triggers for changes to policy-related files to ensure policy verification runs independently when needed.
name: ci-policies on: workflow_call: + push: + paths: + - 'api/policies/**' + - 'api/cmd/policy-generator/**' + pull_request: + paths: + - 'api/policies/**' + - 'api/cmd/policy-generator/**' env: GO_VERSION: '1.22'
14-17: Consider enabling Go module caching.The current setup disables Go module caching (
cache: false), which could slow down the workflow execution. Unless there's a specific reason to disable caching, enabling it would improve performance.uses: actions/setup-go@v4 with: go-version: ${{ env.GO_VERSION }} - cache: false + cache: true
1-35: Consider adding policy diff visualization.To help reviewers understand policy changes, consider adding a step to show the diff between existing and newly generated policies.
+ - name: Show policy changes + if: github.event_name == 'pull_request' + run: | + git fetch origin ${{ github.base_ref }} + echo "Policy changes in this PR:" + git diff origin/${{ github.base_ref }} HEAD -- api/policies/ + working-directory: apiAlso, consider adding artifacts upload for the generated policies:
+ - name: Upload policy artifacts + uses: actions/upload-artifact@v3 + with: + name: generated-policies + path: api/policies/ + if-no-files-found: errorapi/internal/rbac/definitions.go (3)
7-10: Consider making PolicyFileDir configurable via environment variables.Hardcoding the policy directory path might cause issues in different deployment environments. Consider making it configurable through environment variables for better flexibility.
const ( ServiceName = "flow" - PolicyFileDir = "policies" ) + +var PolicyFileDir = getEnvOrDefault("POLICY_FILE_DIR", "policies") + +func getEnvOrDefault(key, defaultValue string) string { + if value := os.Getenv(key); value != "" { + return value + } + return defaultValue +}
12-15: Add documentation for resource types.While the resource names are clear, adding documentation would help explain what each resource represents and how they should be used in the RBAC context.
const ( + // ResourceProject represents a project entity in the system ResourceProject = "project" + // ResourceWorkflow represents a workflow within a project ResourceWorkflow = "workflow" )
17-20: Consider adding more CRUD operations and documentation.The current set of actions (read/edit) might be insufficient for a complete RBAC system. Consider:
- Adding more standard CRUD operations (create/delete)
- Adding documentation for each action
const ( + // ActionCreate allows creation of new resources + ActionCreate = "create" + // ActionRead allows viewing resources ActionRead = "read" + // ActionEdit allows modification of existing resources ActionEdit = "edit" + // ActionDelete allows removal of resources + ActionDelete = "delete" )api/internal/app/usecase.go (1)
38-40: Consider adding nil check for permissionCheckerWhile the implementation is correct, consider adding validation for the permissionChecker parameter to ensure robust error handling.
func UsecaseMiddleware(r *repo.Container, g *gateway.Container, ar *accountrepo.Container, ag *accountgateway.Container, permissionChecker *cerbosClient.PermissionChecker, config interactor.ContainerConfig) echo.MiddlewareFunc { + if permissionChecker == nil { + panic("permissionChecker must not be nil") + } return ContextMiddleware(func(ctx context.Context) context.Context {.github/workflows/update_policies.yml (2)
1-7: Enhance workflow configuration robustnessConsider these improvements for better maintainability and flexibility:
- Pin Go version to specific patch version (e.g., '1.22.0') for reproducibility
- Make GCS bucket path configurable via workflow inputs instead of hardcoding
name: update-policies on: workflow_call: + inputs: + gcs_bucket_path: + required: true + type: string + description: "GCS bucket path for policy files" env: - GO_VERSION: '1.22' + GO_VERSION: '1.22.0' - GCS_BUCKET_PATH: gs://cerbos-oss-policyfile-bucket + GCS_BUCKET_PATH: ${{ inputs.gcs_bucket_path }}
8-21: Improve setup reliability and performance
- Consider enabling Go module caching to improve workflow execution time
- Add error handling for policy generation step
uses: actions/setup-go@v4 with: go-version: ${{ env.GO_VERSION }} - cache: false + cache: true - name: Generate policies - run: make gen-policies + run: | + if ! make gen-policies; then + echo "::error::Policy generation failed" + exit 1 + fi working-directory: apiapi/internal/app/main.go (1)
71-77: Consider adding documentation for the ServerConfig struct fields.While the struct is well-organized, it would benefit from documentation explaining the purpose of each field, especially the newly added
PermissionChecker.type ServerConfig struct { + // Config holds the application configuration Config *config.Config + // Debug indicates if the server is running in debug mode Debug bool + // Repos contains the application repositories Repos *repo.Container + // AccountRepos contains the account-related repositories AccountRepos *accountrepo.Container + // Gateways contains the application gateways Gateways *gateway.Container + // AccountGateways contains the account-related gateways AccountGateways *accountgateway.Container + // PermissionChecker handles authorization and role-based access control PermissionChecker *cerbosClient.PermissionChecker }api/internal/app/repo.go (1)
47-48: Add documentation and logging for database fallback behavior.While the logic is correct, consider these improvements:
- Document the purpose and implications of
accountRepoCompat- Add debug logging when falling back to the default database name
if accountDatabase == "" { + log.Debugf("No account database specified, falling back to default: %s", accountDatabaseName) accountDatabase = accountDatabaseName accountRepoCompat = true // Add comment explaining compatibility mode }api/internal/usecase/interactor/project_test.go (4)
22-31: LGTM! Consider adding documentation.The helper function effectively centralizes the Project setup logic, improving test maintainability. Consider adding a brief comment describing its purpose and parameters.
+// setupProject creates a new Project instance with mock dependencies for testing. +// It takes a mockPermissionChecker to allow flexible permission testing scenarios. func setupProject(t *testing.T, permissionChecker *mockPermissionChecker) *Project {
36-38: Consider a more descriptive name for the mock.The variable name could better reflect its behavior of always allowing access.
-mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { +mockPermissionCheckerAllowAll := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) {
50-113: Consider consistent parameter initialization across test cases.While the test cases are comprehensive, some test cases initialize optional parameters while others don't. Consider initializing all parameters consistently for better test maintainability.
{ name: "nonexistent workspace", param: interfaces.CreateProjectParam{ WorkspaceID: wsid2, + Name: lo.ToPtr(""), // Initialize with zero values + Description: lo.ToPtr(""), // for consistency + Archived: lo.ToPtr(false), }, operator: &usecase.Operator{
115-138: Consider adding assertions for permission checking calls.While the test execution is thorough, consider adding assertions to verify that the permission checker was called with the expected parameters.
Example implementation:
// Add to mockPermissionChecker struct type mockPermissionChecker struct { check func(context.Context, *appx.AuthInfo, string, action string) (bool, error) calls []struct { resource string action string } } // Update test to verify calls assert.Equal(t, "project", checker.calls[0].resource) assert.Equal(t, "create", checker.calls[0].action)api/internal/app/app.go (1)
83-86: Consider adding logging for permission checks.To aid in debugging and monitoring, consider adding logging middleware to track permission check results.
e.Use(UsecaseMiddleware(cfg.Repos, cfg.Gateways, cfg.AccountRepos, cfg.AccountGateways, cfg.PermissionChecker, interactor.ContainerConfig{ SignupSecret: cfg.Config.SignupSecret, AuthSrvUIDomain: cfg.Config.Host_Web, + LogPermissionChecks: true, // Add this option to ContainerConfig }))api/internal/app/config/config.go (2)
30-30: LGTM! Consider adding documentation.The new
DashboardHostfield is properly structured with appropriate tags. Consider adding a comment to document its purpose and expected format.+ // DashboardHost is the URL of the dashboard service (e.g., http://localhost:8090) DashboardHost string `envconfig:"REEARTH_DASHBOARD_HOST" pp:",omitempty"`
Line range hint
91-125: Add HTTP scheme handling for DashboardHost.The
DashboardHostfield should be validated and have the HTTP scheme added if missing, similar to other host fields in theReadConfigfunction.Add the following code after the existing host validations:
if c.AuthSrv.UIDomain == "" { c.AuthSrv.UIDomain = c.Host_Web } else { c.AuthSrv.UIDomain = addHTTPScheme(c.AuthSrv.UIDomain) } + if c.DashboardHost != "" { + c.DashboardHost = addHTTPScheme(c.DashboardHost) + } return &c, err.github/workflows/ci.yml (1)
Line range hint
12-166: Consider implementing policy validation checks.While the current implementation handles policy generation and updates well, consider adding explicit policy validation steps:
- Validate policy syntax and structure before deployment
- Run policy simulation tests against predefined test cases
- Consider implementing a dry-run mode for policy updates
This would help catch policy configuration issues early in the CI pipeline.
api/go.mod (1)
42-42: Consider using yaml.v3 instead of yaml.v2.While yaml.v2 is stable, yaml.v3 offers improved features and security:
- Better error handling
- Strict parsing by default
- Support for YAML 1.2
- Security improvements
If this is a transitive dependency from another package, you might want to check if that package has a version supporting yaml.v3.
api/internal/usecase/interactor/project.go (4)
55-62: Use consistent error handling for permission denialInstead of returning
fmt.Errorf("permission denied"), consider using a predefined error variable or a standardized error type for permission denials. This practice promotes consistency and eases error handling across the codebase.Apply this diff to define and use a standard error:
+var ErrPermissionDenied = errors.New("permission denied") + func (i *Project) Create(ctx context.Context, p interfaces.CreateProjectParam, operator *usecase.Operator) (_ *project.Project, err error) { authInfo := adapter.GetAuthInfo(ctx) hasPermission, err := i.permissionChecker.CheckPermission(ctx, authInfo, rbac.ResourceProject, rbac.ActionEdit) if err != nil { return nil, err } if !hasPermission { - return nil, fmt.Errorf("permission denied") + return nil, ErrPermissionDenied }
55-63: Avoid redundant permission checksThe
Createmethod performs a permission check usingpermissionChecker.CheckPermissionand then callsi.CanWriteWorkspace. Verify whether both checks are necessary or if they can be consolidated to streamline the authorization logic and avoid redundancy.
Line range hint
84-84: Replacefmt.Printlnwith structured loggingThe line
fmt.Println("RunProjectParam", p)appears to be used for debugging purposes. Consider replacing it with the application's structured logging system to provide consistent and configurable log output.Apply this diff to use structured logging:
-import "fmt" +import "log/slog" func (i *Project) Run(ctx context.Context, p interfaces.RunProjectParam, operator *usecase.Operator) (started bool, err error) { if p.Workflow == nil { return false, nil } tx, err := i.transaction.Begin(ctx) if err != nil { return false, err } ctx = tx.Context() defer func() { if err2 := tx.End(ctx); err == nil && err2 != nil { err = err2 } }() - fmt.Println("RunProjectParam", p) + slog.Info("RunProjectParam", "param", p)
Line range hint
85-115: Remove commented-out code for cleaner codebaseThe
Runmethod contains a significant block of commented-out code. If this code is deprecated or no longer needed, consider removing it to enhance code readability and maintainability. Version control systems retain the history, so the code can be retrieved if necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
api/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.github/workflows/ci.yml(5 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)api/.env.example(1 hunks)api/Makefile(1 hunks)api/cmd/policy-generator/main.go(1 hunks)api/go.mod(2 hunks)api/internal/adapter/gql/loader_user.go(1 hunks)api/internal/app/app.go(1 hunks)api/internal/app/config/config.go(1 hunks)api/internal/app/main.go(3 hunks)api/internal/app/repo.go(2 hunks)api/internal/app/usecase.go(2 hunks)api/internal/rbac/definitions.go(1 hunks)api/internal/usecase/gateway/permission.go(1 hunks)api/internal/usecase/interactor/common.go(2 hunks)api/internal/usecase/interactor/project.go(3 hunks)api/internal/usecase/interactor/project_test.go(2 hunks)api/internal/usecase/interactor/utils_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/.env.example
🧰 Additional context used
🪛 actionlint
.github/workflows/update_policies.yml
29-29: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:5:26: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2231:info:20:20: Quote expansions in this for loop glob to prevent wordsplitting, e.g. "$dir"/*.txt
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:29:11: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (29)
api/internal/usecase/gateway/permission.go (2)
1-8: LGTM! Package and imports are well-organized.
The package name and import structure follow Go conventions appropriately.
9-11: Verify consistent permission checking implementation.
Since this interface is part of the core authorization system, we should verify its usage for consistency and security.
✅ Verification successful
Let me gather more information about the resource and action constants being used.
Let me try a different approach to find the constants and their usage.
Let me check the contents of the rbac package and the policy generator.
Permission checking implementation is consistent and well-defined
The implementation shows a well-structured RBAC system with:
- Clearly defined resources (
project,workflow) - Standard actions (
read,edit) - Consistent role hierarchy (
owner,maintainer,writer,reader) - Centralized definition in
rbacpackage - Policy generation infrastructure in place
The only usage found in project.go correctly uses the constants defined in the RBAC package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of PermissionChecker
# Check for all implementations of PermissionChecker
echo "Checking PermissionChecker implementations:"
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) CheckPermission(ctx context.Context, authInfo *appx.AuthInfo, resource string, action string) (bool, error) {
$$$
}'
# Check for all calls to CheckPermission to verify consistent resource/action strings
echo -e "\nChecking CheckPermission calls:"
rg -A 2 'CheckPermission\(.*\)' --type go
# Look for potential security-related TODOs or FIXMEs
echo -e "\nChecking for security-related comments:"
rg -i 'TODO|FIXME|HACK|XXX' -A 2 -g '*.go' | rg -i 'security|permission|auth'
Length of output: 3245
Script:
#!/bin/bash
# Check for resource and action constant definitions
echo "Checking for resource and action constants:"
rg "const.*Resource|const.*Action" -B 1 -A 5 --type go
# Check the rbac package specifically
echo -e "\nChecking rbac package contents:"
fd "rbac" --type f --exec cat {}
# Look for any other permission-related constants
echo -e "\nChecking other permission-related constants:"
rg "const.*Permission" -B 1 -A 5 --type go
Length of output: 417
Script:
#!/bin/bash
# Search for rbac-related constants with broader pattern
echo "Checking for rbac-related constants:"
rg "rbac\." -B 2 -A 2 --type go
# Look for string literals that might be resources or actions
echo -e "\nChecking for potential resource/action strings:"
ast-grep --pattern 'CheckPermission($$$, $$$, "$_", "$_")'
# Check for any files with rbac in the name or path
echo -e "\nListing rbac-related files:"
fd rbac
Length of output: 1620
Script:
#!/bin/bash
# Check contents of rbac directory
echo "Checking rbac directory contents:"
ls -la api/internal/rbac/
cat api/internal/rbac/*
# Check policy generator
echo -e "\nChecking policy generator:"
cat api/cmd/policy-generator/main.go
# Check for any policy files
echo -e "\nChecking for policy files:"
fd -e yaml -e yml policy
Length of output: 1973
api/cmd/policy-generator/main.go (2)
11-15: Verify rbac package configuration.
Let's verify the rbac package configuration to ensure proper policy generation.
#!/bin/bash
# Description: Verify rbac package configuration and resource definitions
# Test: Check rbac package constants and resource definitions
ast-grep --pattern 'const ServiceName = $_'
ast-grep --pattern 'const PolicyFileDir = $_'
ast-grep --pattern $'func DefineResources() []generator.Resource {
$$$
}'
# Test: Verify policy file directory exists in the expected location
rg -l "PolicyFileDir.*=.*" api/internal/rbac/1-9: Verify reearthx/cerbos/generator dependency version.
The imports look good and are appropriate for the policy generation task.
Let's verify the reearthx dependency version and its stability:
✅ Verification successful
The previous script didn't provide the expected output, so I'm trying to gather more context about the dependency and implementation to verify if the imports are correctly used and the dependency is properly configured.
The previous commands failed to access the files. Let's try a different approach to verify the existence of files and dependencies.
The output shows that the files exist and there's widespread usage of reearthx across the codebase. Let's verify the specific versions and implementation details to ensure the imports are properly configured.
✓ The reearthx dependency is properly configured and used
The imports are correctly set up with reearthx version v0.0.0-20241106140509-60843e8dca2f. The implementation shows proper usage of the cerbos generator package:
- The policy generator correctly uses the RBAC definitions from the internal package
- The cerbos generator is properly integrated with well-defined resources and actions
- The implementation follows the expected pattern for policy generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the reearthx dependency version and its recent changes
# Test: Look for go.mod entries related to reearthx
rg -A 3 "github.com/reearth/reearthx" go.mod
# Test: Check if the cerbos generator package exists and is stable
rg -l "package generator" $(fd -t f . --full-path "**/reearthx/cerbos/generator")
Length of output: 251
Script:
#!/bin/bash
# Let's check the go.mod file directly for reearthx dependency
cat go.mod
# Check the actual implementation of the policy generator
fd -t f . --full-path "**/policy-generator/main.go" --exec cat {}
# Look for any existing policy files or definitions
rg -l "type.*Policy" api/internal/rbac/
Length of output: 278
Script:
#!/bin/bash
# Let's try different approaches to locate and verify the dependencies
# Check if the file exists first
ls -la api/cmd/policy-generator/main.go
# Look for any references to reearthx in the codebase
rg -l "github.com/reearth/reearthx"
# Search for cerbos-related imports or usage
rg -l "cerbos"
# Look for RBAC-related definitions
rg -l "rbac" api/internal/
Length of output: 4932
Script:
#!/bin/bash
# Let's check the specific dependency configuration and implementation
# Check the go.mod in the api directory
cat api/go.mod
# Check the content of the RBAC definitions
cat api/internal/rbac/definitions.go
# Check the actual implementation of the policy generator
cat api/cmd/policy-generator/main.go
Length of output: 9674
api/Makefile (1)
25-25: LGTM!
The .PHONY declaration is correctly updated to include the new target while maintaining proper formatting and including all necessary targets.
api/internal/usecase/interactor/utils_test.go (2)
1-8: LGTM! Package and imports are well-structured.
19-24: Consider enhancing the CheckPermission method implementation.
While the implementation is functional, there are several potential improvements:
- The default
return true, nilbehavior might hide test issues by accidentally allowing actions that should be explicitly tested. - The method would benefit from documentation.
- Consider using constants for better readability.
+// CheckPermission implements the permission checking interface.
+// It delegates to the provided checkPermissionFunc if available,
+// otherwise returns true (allowed) by default.
func (m *mockPermissionChecker) CheckPermission(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) {
if m.checkPermissionFunc != nil {
return m.checkPermissionFunc(ctx, authInfo, resource, action)
}
+ // Consider logging a warning here when using default behavior
return true, nil
}Let's verify if this mock is being used consistently across test files:
.github/workflows/ci_policies.yml (1)
23-35: 🛠️ Refactor suggestion
Enhance policy file verification.
The current verification only lists files and their content. Consider adding validation of the policy files' structure and syntax.
Let's verify if there are any policy validation tools in the repository:
Additionally, consider this improvement:
- name: List generated policy files
run: |
echo "Generated policy files in api/policies/:"
ls -la policies/ || echo "No files found"
if [ -d "policies" ] && [ "$(ls -A policies/)" ]; then
+ # Validate YAML syntax for all policy files
+ for file in policies/*.yaml; do
+ echo "Validating $file"
+ if ! yamllint "$file"; then
+ echo "Invalid YAML format in $file"
+ exit 1
+ fi
+ done
echo "Content of policy files:"
for file in policies/*.yaml; do
echo "--- Content of $file ---"
cat "$file"
done
else
- echo "No policy files were generated"
+ echo "Error: No policy files were generated"
+ exit 1
fi
working-directory: apiapi/internal/rbac/definitions.go (1)
1-5: LGTM! Clean package structure and imports.
The package name and import are well-organized and follow Go conventions.
api/internal/app/usecase.go (2)
13-13: LGTM: Clean import addition
The cerbos client import is properly aliased and aligns with the new permission checking functionality.
16-16: Verify all callers of UsecaseMiddleware
The function signature change requires all callers to be updated with the new permissionChecker parameter.
✅ Verification successful
Based on the search results, I can generate the final response as we have found all the callers of UsecaseMiddleware. The search shows that there is only one caller in api/internal/app/app.go, and it has been properly updated to include the new permissionChecker parameter.
All callers of UsecaseMiddleware are properly updated
The only caller in api/internal/app/app.go correctly passes the cfg.PermissionChecker parameter to match the updated function signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of UsecaseMiddleware to ensure they've been updated
# Expected: All callers should pass the permissionChecker parameter
# Search for UsecaseMiddleware calls
rg -l "UsecaseMiddleware\(" | while read -r file; do
echo "=== $file ==="
rg -A 2 "UsecaseMiddleware\(" "$file"
done
Length of output: 880
.github/workflows/update_policies.yml (1)
28-59:
Improve shell script robustness and safety
The synchronization script needs several improvements for better reliability and safety:
- Fix shell script quoting issues
- Add proper error handling
- Validate policy files before sync
run: |
+ set -euo pipefail
+
echo "All files in bucket (before sync):"
- gsutil ls $GCS_BUCKET_PATH/ || true
+ gsutil ls "${GCS_BUCKET_PATH}/" || true
echo "Current flow files in bucket:"
- bucket_files=$(gsutil ls $GCS_BUCKET_PATH/flow_*.yaml || true)
+ bucket_files=$(gsutil ls "${GCS_BUCKET_PATH}/flow_"*.yaml || true)
echo "$bucket_files"
echo "Local policy files:"
- local_files=$(ls policies/flow_*.yaml || true)
+ local_files=$(ls policies/flow_*.yaml 2>/dev/null || true)
echo "$local_files"
- for file in policies/flow_*.yaml; do
+ # Validate policy files
+ for file in policies/flow_*.yaml; do
+ if [ -f "$file" ]; then
+ if ! yamllint "$file"; then
+ echo "::error::Invalid YAML in $file"
+ exit 1
+ fi
+ fi
+ done
+
+ # Sync files
+ for file in policies/flow_*.yaml; do
if [ -f "$file" ]; then
file_name=$(basename "$file")
echo "Uploading/Updating: $file_name"
- gsutil cp "$file" "$GCS_BUCKET_PATH/$file_name"
+ if ! gsutil cp "$file" "${GCS_BUCKET_PATH}/${file_name}"; then
+ echo "::error::Failed to upload ${file_name}"
+ exit 1
+ fi
fi
done
- for bucket_file in $GCS_BUCKET_PATH/flow_*.yaml; do
+ # Clean up old files
+ for bucket_file in "${GCS_BUCKET_PATH}/flow_"*.yaml; do
file_name=$(basename "$bucket_file")
if [ ! -f "policies/$file_name" ] && [[ "$file_name" == flow_* ]]; then
echo "Deleting: $file_name"
- gsutil rm "$bucket_file"
+ if ! gsutil rm "$bucket_file"; then
+ echo "::warning::Failed to delete ${bucket_file}"
+ fi
fi
done
echo "Sync completed. All files in bucket:"
- gsutil ls $GCS_BUCKET_PATH/ || true
+ gsutil ls "${GCS_BUCKET_PATH}/" || trueAdditionally:
- Consider adding a step to verify policy file syntax using
cerbos verifybefore upload - Add retries for gsutil operations to handle transient failures
- Consider using a checksum comparison to avoid unnecessary uploads
🧰 Tools
🪛 actionlint
29-29: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:5:26: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2231:info:20:20: Quote expansions in this for loop glob to prevent wordsplitting, e.g. "$dir"/*.txt
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:29:11: Double quote to prevent globbing and word splitting
(shellcheck)
api/internal/adapter/gql/loader_user.go (1)
46-49: Verify the intended behavior of returning only the first search result.
The implementation now returns only the first matching user from the search results. This could be a significant behavioral change if clients were previously expecting multiple results.
Let's verify the intended behavior and usage:
Consider the following architectural recommendations:
- If single-result behavior is intended, consider renaming the method to
FindFirstUseror similar to better reflect its behavior. - If multiple results might be needed, consider adding a separate method like
SearchUsersthat returns all matches.
✅ Verification successful
The implementation correctly matches the GraphQL schema and usage
The current implementation returning only the first user is correct because:
- The GraphQL schema explicitly defines
searchUserto return a singleUsertype, not a list:searchUser(nameOrEmail: String!): User - The client query in
ui/src/lib/gql/user/queries.graphqlexpects a single user result - The test in
api/e2e/gql_user_test.goverifies single-user return behavior
The previous implementation was incorrect as it attempted to return multiple results for a schema that expects a single user.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any GraphQL queries that might expect multiple results from SearchUser
# Test 1: Look for GraphQL queries using SearchUser
rg -A 5 'SearchUser.*\{' --type graphql
# Test 2: Check for any tests that might verify SearchUser behavior
rg -A 5 'TestSearchUser' --type go
# Test 3: Look for any documentation about the SearchUser functionality
fd -e md -e txt -x rg -l 'SearchUser'
Length of output: 476
Script:
#!/bin/bash
# Let's gather more context about the SearchUser implementation and its usage
# Check the test implementation to understand expected behavior
cat api/e2e/gql_user_test.go
# Look for GraphQL schema definitions that might show the return type
fd -e graphql -e gql . || rg -l '(type|input).*User'
# Check the previous implementation of SearchUser
git log -p api/internal/adapter/gql/loader_user.go
# Look for any other loaders or similar search implementations
rg -A 5 'func.*Search.*User'
Length of output: 11977
Script:
#!/bin/bash
# Let's check the GraphQL schema and client queries to understand the expected behavior
# Check the GraphQL schema definition for searchUser
cat api/gql/user.graphql
# Look for client-side usage of searchUser query
cat ui/src/lib/gql/user/queries.graphql
# Check if there are any other files referencing searchUser
rg -l 'searchUser' --type ts --type tsx
Length of output: 1404
api/internal/usecase/interactor/common.go (1)
18-18: LGTM: Clean import addition for permission checking.
The cerbos client import is properly aliased and sourced from the shared library.
api/internal/app/main.go (4)
12-12: LGTM! Imports are well-organized.
The new imports for rbac and cerbosClient are appropriately added and align with the authorization feature implementation.
Also applies to: 17-17
55-61: LGTM! Server configuration is properly updated.
The PermissionChecker is correctly integrated into the server configuration with consistent field ordering.
50-52: Consider handling graceful shutdown of the permission checker.
The permission checker might need cleanup during server shutdown. Consider adding a cleanup mechanism in the server's Shutdown method if required by the cerbosClient implementation.
#!/bin/bash
# Check if PermissionChecker has any cleanup methods that need to be called
ast-grep --pattern 'type PermissionChecker struct {
$$$
}'
# Check if there are any Close/Shutdown methods
rg -A 5 "func \(.*PermissionChecker\) (Close|Shutdown)"50-52: 🛠️ Refactor suggestion
Consider adding error handling and validation for the permission checker.
The permission checker initialization could be more robust:
- Add validation for
conf.DashboardHost - Add connection testing to ensure the permission service is accessible
- Consider using a context with timeout for initialization
Here's a suggested improvement:
// PermissionChecker
-permissionChecker := cerbosClient.NewPermissionChecker(rbac.ServiceName, conf.DashboardHost)
+if conf.DashboardHost == "" {
+ log.Fatalf("DashboardHost configuration is required")
+}
+
+ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+defer cancel()
+
+permissionChecker, err := cerbosClient.NewPermissionChecker(ctx, rbac.ServiceName, conf.DashboardHost)
+if err != nil {
+ log.Fatalf("failed to initialize permission checker: %v", err)
+}api/internal/app/repo.go (2)
Line range hint 28-91: Verify integration of permission checking functionality.
The PR objectives mention implementing permission checking functionality, but this file doesn't show any permission-related initialization. Please verify if additional changes are needed in initReposAndGateways to support the new permission checking feature.
#!/bin/bash
# Description: Check for permission-related interfaces and implementations
# Expected: Find permission checker initialization and usage
# Search for permission-related types and implementations
ast-grep --pattern 'type PermissionChecker interface {
$$$
}'
# Search for permission checker initialization
rg 'NewPermissionChecker|InitPermissionChecker'26-26: LGTM! Verify consistent database naming across the codebase.
The new constant follows proper naming conventions and maintains consistency with the existing database naming pattern.
✅ Verification successful
Database naming convention is consistently followed
The codebase maintains consistent database naming with the "reearth-" prefix pattern:
reearth-flowfor the main databasereearth-accountfor the account database
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent database naming across the codebase
# Expected: All database names should follow the "reearth-*" pattern
# Search for database name strings and verify naming consistency
rg -g '!*.md' -g '!*.json' '"reearth-[a-z]+"'
Length of output: 517
Script:
#!/bin/bash
# Let's search specifically for database name patterns in configuration and initialization code
rg -g '!*.md' -g '!*.json' -g '!*.yaml' 'database.*"reearth-' -A 2 -B 2
# Also search for any MongoDB related configuration or URI patterns that might contain database names
rg -g '!*.md' -g '!*.json' -g '!*.yaml' 'mongodb.*database' -A 2 -B 2
Length of output: 356
api/internal/app/app.go (1)
83-86: Verify PermissionChecker initialization and error handling.
The integration of PermissionChecker into the middleware chain is good. However, we should ensure proper initialization and error handling.
#!/bin/bash
# Description: Verify PermissionChecker initialization and related configurations
# Test 1: Check if PermissionChecker is properly initialized before use
rg -A 5 "type ServerConfig struct"
# Test 2: Check for error handling during PermissionChecker initialization
rg "PermissionChecker.*New|New.*PermissionChecker"Consider these architectural recommendations:
- Add a validation check for
cfg.PermissionCheckerbefore using it in the middleware chain - Consider moving the permission middleware before the route-specific middleware but after authentication for proper security layering
.github/workflows/ci.yml (4)
12-12: LGTM: Well-structured policy change detection.
The prepare job correctly integrates policy-related changes detection, following the established pattern used for other components. The file patterns effectively capture all relevant policy-related changes.
Also applies to: 24-41
98-98: LGTM: Proper integration of policy checks in CI pipeline.
The ci-policies job is correctly added as a dependency, ensuring policy checks must pass as part of the CI process.
78-81: Verify the existence of ci_policies.yml workflow.
The job configuration looks correct, following the established pattern of other CI jobs.
✅ Verification successful
The referenced workflow file ci_policies.yml exists and is properly located
The ci-policies job configuration is correct and the referenced workflow file .github/workflows/ci_policies.yml exists in the repository, confirming that the job will be able to execute successfully when triggered.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the referenced workflow file exists
if [ ! -f .github/workflows/ci_policies.yml ]; then
echo "Error: ci_policies.yml workflow file not found"
exit 1
fi
Length of output: 50
Script:
#!/bin/bash
# Let's try using fd to search for the workflow file
fd "ci_policies.yml" .github/workflows/
# Also check for any similar named files in case of typos
fd "policies.*\.yml" .github/workflows/
# List all workflow files for context
ls -la .github/workflows/
Length of output: 1038
159-166: Verify the update-policies workflow configuration.
The job configuration looks correct, following the pattern of other deployment jobs. The conditional execution and secret inheritance are properly configured.
✅ Verification successful
The update-policies workflow configuration is properly implemented
The workflow file exists and is correctly configured as a reusable workflow with the following verified aspects:
- Properly defined with
workflow_calltrigger - Contains the necessary job configuration for policy updates
- Includes required authentication and cloud storage synchronization steps
- Referenced correctly from ci.yml with appropriate conditions and dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the referenced workflow file exists and contains expected configuration
if [ ! -f .github/workflows/update_policies.yml ]; then
echo "Error: update_policies.yml workflow file not found"
exit 1
fi
# Check if the workflow file contains required elements
rg -q 'workflow_call' .github/workflows/update_policies.yml || {
echo "Error: update_policies.yml should be a reusable workflow"
exit 1
}
Length of output: 114
Script:
#!/bin/bash
# Let's check if the file exists and examine its content
if [ -f .github/workflows/update_policies.yml ]; then
echo "File exists. Content of update_policies.yml:"
cat .github/workflows/update_policies.yml
else
echo "File not found: .github/workflows/update_policies.yml"
fi
# Also check for any references to this workflow in other files
echo -e "\nReferences to update_policies.yml in other workflow files:"
rg "update_policies.yml" .github/workflows/ -l
Length of output: 2257
api/go.mod (1)
20-20: Verify the major version update of reearthx.
The reearthx dependency has been updated from April to November version. Given this significant update, please ensure:
- All breaking changes are handled
- The specific commit (60843e8dca2f) is stable
- Changes are documented in the changelog
✅ Verification successful
The reearthx update is safe to proceed
The version update from April to November primarily adds new permission checking functionality, which aligns perfectly with this PR's objectives. The changes are non-breaking:
- New permission checking implementation (
permission_checker.go) - Utility functions for permission checking (
permission_checker_utils.go) - Multiple commits fixing and improving the permission checking functionality
- Other changes are mostly improvements and bug fixes in mongox and account packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in reearthx between the old and new versions
# Get the commit messages and changes between versions
git clone https://github.com/reearth/reearthx.git /tmp/reearthx
cd /tmp/reearthx
git log --oneline 5cbc45bf18eb..60843e8dca2f
Length of output: 1201
Script:
#!/bin/bash
# Let's check the actual changes in the permission-related files to understand the impact
cd /tmp/reearthx
git diff 5cbc45bf18eb..60843e8dca2f -- '**/permission*.go'
Length of output: 1140
Script:
#!/bin/bash
# Let's check if there are any breaking changes in the API by examining permission_checker_utils.go
cd /tmp/reearthx
git show 44c50f7:cerbos/client/permission_checker_utils.go
Length of output: 906
api/internal/usecase/interactor/project.go (3)
7-8: Approved: Necessary imports added
The imports for "github.com/reearth/reearth-flow/api/internal/adapter" and "github.com/reearth/reearth-flow/api/internal/rbac" are appropriately added and are required for permission handling.
30-30: Approved: Addition of permissionChecker enhances security
Adding the permissionChecker field to the Project struct integrates permission checks, enhancing the security of operations within this interactor.
33-42: Verify that all callers of NewProject are updated
The NewProject function signature now includes the permissionChecker parameter. Ensure that all instantiations of NewProject across the codebase are updated to pass this new parameter to prevent build errors.
Run the following script to find all calls to NewProject and check if they include the new parameter:
✅ Verification successful
Let me analyze the results. I can see multiple NewProject functions in different packages, but the one we're concerned with is in api/internal/usecase/interactor/project.go. Looking at the results, there's only one caller of this specific NewProject function in api/internal/usecase/interactor/common.go:34, and it's already passing the permissionChecker parameter correctly.
Let me verify this is indeed the only caller by searching with more context:
Let me try a different approach to find the imports and usage:
Based on the results, I can now generate the final response:
All callers are properly updated with the new parameter
The only caller of the NewProject function from api/internal/usecase/interactor/project.go is in api/internal/usecase/interactor/common.go, and it's already correctly passing all three parameters including the new permissionChecker. Other NewProject functions found in the codebase belong to different packages (mongo, memory) and are unrelated to this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to NewProject and display the lines.
# Expect: All calls should include the `permissionChecker` parameter.
rg --type go --no-heading --line-number 'NewProject\('
Length of output: 1667
Script:
#!/bin/bash
# Search for the specific NewProject from interactor package with surrounding context
rg --type go -B 2 -A 2 'interactor\.NewProject|usecase\.NewProject'
Length of output: 69
Script:
#!/bin/bash
# Find files that import the interactor package
rg --type go -l "\".*interactor\""
# Then find the actual instantiation pattern with more context
rg --type go -B 3 -A 3 'Project:\s+New(Project|Interactor)'
Length of output: 2562
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #580 +/- ##
==========================================
- Coverage 18.30% 17.69% -0.61%
==========================================
Files 105 108 +3
Lines 5185 5447 +262
==========================================
+ Hits 949 964 +15
- Misses 4154 4398 +244
- Partials 82 85 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
.github/workflows/ci_policies.yml (2)
1-6: Pin Go version to specific patch version for reproducibility.Using a major.minor version ('1.22') could lead to unexpected behavior if a patch release introduces changes. Consider pinning to a specific patch version.
- GO_VERSION: '1.22' + GO_VERSION: '1.22.0'
8-17: Update checkout action and consider enabling caching.A few suggestions to improve the setup:
- Update to the latest checkout action version
- Consider enabling Go modules cache unless there's a specific reason to disable it
- uses: actions/checkout@v3 + uses: actions/checkout@v4 uses: actions/setup-go@v4 with: go-version: ${{ env.GO_VERSION }} - cache: false + cache: true
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/ci_policies.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/ci_policies.yml (1)
18-21: The previous review comment about error handling is still applicable.
The policy generation step should include error handling and proper status checking as suggested in the previous review.
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service
branch
from
67dc333 to
84da78b
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (20)
api/internal/usecase/gateway/mock_permission.go (3)
9-14: Consider enhancing the mock for better test coverage.While the current implementation is good for basic testing, consider adding fields to track method calls and input parameters for more thorough testing:
type MockPermissionChecker struct { Allow bool Error error + // Track method calls for verification in tests + Calls struct { + CheckPermission []struct { + Resource string + Action string + AuthInfo *appx.AuthInfo + } + } }
16-21: Consider adding constructor options for flexibility.The current constructor uses fixed defaults. Consider adding options for more flexible initialization:
-func NewMockPermissionChecker() *MockPermissionChecker { +func NewMockPermissionChecker(opts ...func(*MockPermissionChecker)) *MockPermissionChecker { m := &MockPermissionChecker{ Allow: true, Error: nil, } + for _, opt := range opts { + opt(m) + } return m }
23-28: Add parameter validation and logging for better test debugging.The current implementation could benefit from parameter validation and logging to help debug test failures:
func (m *MockPermissionChecker) CheckPermission(ctx context.Context, authInfo *appx.AuthInfo, resource string, action string) (bool, error) { + // Track method calls + if m.Calls.CheckPermission == nil { + m.Calls.CheckPermission = make([]struct { + Resource string + Action string + AuthInfo *appx.AuthInfo + }, 0) + } + m.Calls.CheckPermission = append(m.Calls.CheckPermission, struct { + Resource string + Action string + AuthInfo *appx.AuthInfo + }{resource, action, authInfo}) + + // Validate parameters + if resource == "" || action == "" { + return false, fmt.Errorf("resource and action cannot be empty") + } + if m.Error != nil { return false, m.Error } return m.Allow, nil }api/e2e/ping_test.go (1)
Line range hint
1-40: Consider adding permission-related test cases.Since this PR implements authorization and roles management, consider adding test cases to verify:
- Behavior when permissions are disabled (
false)- Error cases for unauthorized access
Example test cases to add:
func TestPingAPIWithoutPermissions(t *testing.T) { e := StartServer(t, &config.Config{ Origins: []string{"https://example.com"}, AuthSrv: config.AuthSrvConfig{ Disabled: true, }, }, false, nil, false) // permissions disabled // Verify behavior remains the same r := e.GET("/api/ping"). WithHeader("Origin", "https://example.com"). Expect() r.Status(http.StatusOK). JSON(). String(). IsEqual("pong") }api/e2e/gql_me_test.go (1)
Add authentication token tests similar to other test files
The search results show that other test files in the same directory (
gql_workspace_test.go,gql_project_test.go,gql_user_test.go, etc.) consistently use authentication tokens in their tests withWithHeader("authorization", "Bearer test"). However, ingql_me_test.go, the authentication header is commented out, relying only on the debug user header.
- Add test cases in
gql_me_test.gofollowing the pattern used in other test files:
- Include
WithHeader("authorization", "Bearer test")alongside the existing headers- Consider removing the debug user header to properly test the authentication flow
- Add negative test cases with invalid/missing tokens to verify authorization behavior
🔗 Analysis chain
Line range hint
29-30: Consider testing with actual authentication token.The test is currently using a debug user header (
X-Reearth-Debug-User) with authentication disabled. Since this change is part of an authorization system implementation, it would be valuable to test with actual authentication.Let's check if other test files have examples of authentication token usage:
Consider adding test cases with:
- Valid authentication token
- Invalid authentication token
- Missing authentication token
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for authentication token usage in tests rg -g '**/*_test.go' -B 2 -A 2 'Bearer.*token|authorization.*Bearer'Length of output: 7744
.github/workflows/ci_policies.yml (1)
1-42: Consider enhancing the workflow with additional operational features.To improve the robustness of the policy management pipeline, consider:
- Uploading generated policies as workflow artifacts for debugging and auditing
- Adding integration tests with sample policy scenarios
- Implementing policy diff checks to highlight changes
Example implementation for artifacts:
+ - name: Upload policy artifacts + uses: actions/upload-artifact@v4 + with: + name: cerbos-policies + path: api/policies/ + retention-days: 5🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/update_policies.yml (3)
5-6: Consider making GCS bucket path configurableThe GCS bucket path is currently hardcoded in the workflow file. Consider moving it to a repository secret or environment variable to make it easier to maintain different environments.
env: GO_VERSION: '1.22' - GCS_BUCKET_PATH: gs://cerbos-oss-policyfile-bucket + GCS_BUCKET_PATH: ${{ secrets.GCS_POLICY_BUCKET_PATH }}
12-14: Update checkout action to latest versionThe
actions/checkoutaction has a newer version available.- name: checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
2-3: Add permissions and concurrency controlConsider adding explicit permissions and concurrency control to the workflow:
on: workflow_call: + +permissions: + contents: read + id-token: write # Required for Workload Identity Federation + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: trueapi/internal/app/main.go (1)
71-77: Add documentation for the PermissionChecker fieldWhile the struct changes are correct, consider adding documentation to explain the purpose and requirements of the PermissionChecker field.
type ServerConfig struct { Config *config.Config Debug bool Repos *repo.Container AccountRepos *accountrepo.repo.Container Gateways *gateway.Container AccountGateways *accountgateway.Container + // PermissionChecker handles authorization checks for API endpoints + // using the Cerbos policy decision point service PermissionChecker gateway.PermissionChecker }api/internal/usecase/interactor/project_test.go (5)
22-31: LGTM! Consider adding documentation.The helper function effectively centralizes the test setup. Consider adding a brief documentation comment to describe its purpose and parameters.
+// setupProject creates a new Project instance with the given permission checker for testing func setupProject(t *testing.T, permissionChecker *mockPermissionChecker) *Project {
36-38: Consider using a more descriptive name for the mock.The mock permission checker that always returns true could be more clearly named to indicate its behavior.
-mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { +mockPermissionCheckerAlwaysAllow := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) {
40-45: Add cleanup for test resources.Consider adding cleanup code to ensure the test workspace is removed after tests complete.
err := uc.workspaceRepo.Save(ctx, ws) assert.NoError(t, err, "workspace save should not return error") +t.Cleanup(func() { + _ = uc.workspaceRepo.Remove(ctx, ws.ID()) +})
50-113: Add test case for input validation.The test cases cover permission and existence checks well, but consider adding a test case for invalid input validation (e.g., empty name).
tests := []struct { // ... existing fields }{ // ... existing test cases + { + name: "invalid input - empty name", + param: interfaces.CreateProjectParam{ + WorkspaceID: ws.ID(), + Name: lo.ToPtr(""), // empty name + Description: lo.ToPtr("description"), + }, + operator: &usecase.Operator{ + AcOperator: &accountusecase.Operator{ + WritableWorkspaces: workspace.IDList{ws.ID()}, + }, + }, + wantErr: interfaces.ErrInvalidInput, + }, }
115-138: Enhance error assertions with more specific checks.Consider adding more detailed assertions for error cases to verify not just the error message but also the error type and any additional context.
if tt.wantErr != nil { - assert.EqualError(t, err, tt.wantErr.Error()) + assert.ErrorIs(t, err, tt.wantErr) + if tt.wantErr == interfaces.ErrOperationDenied { + assert.Contains(t, err.Error(), "operation denied") + } assert.Nil(t, got, "project should be nil when error is expected") return }api/e2e/gql_project_test.go (3)
11-44: LGTM! Consider documenting the new permission parameter.The refactoring to use sub-tests improves test organization. However, the new boolean parameter in
StartServer(line 17) needs documentation to explain its purpose and impact.Consider adding a comment explaining the purpose of the last boolean parameter in StartServer:
}, true, baseSeeder, true) // Add comment: true enables permission checks
46-79: Consider improving error assertions and test code reuse.While the permission denied test case is well-structured, there are two potential improvements:
- The error message assertion could be more specific to avoid false positives
- The request body setup is duplicated from the success case
Consider these improvements:
- Make the error assertion more specific:
- Contains("permission denied") + Equal("permission denied: insufficient permissions to create project")
- Extract the common request body setup:
+func createProjectRequest(workspaceId string) GraphQLRequest { + return GraphQLRequest{ + OperationName: "CreateProject", + Query: "mutation CreateProject($workspaceId: ID!, $name: String!, $description: String!) {...}", + Variables: map[string]any{ + "name": "test", + "description": "abc", + "workspaceId": workspaceId, + }, + } +}
Missing permission test cases in other GraphQL endpoints
The codebase analysis reveals that permission testing is not consistently implemented across all GraphQL endpoints. Specifically:
TestCreateProjectproperly includes a permission_denied test caseTestRunProjectlacks permission testing despite using the same StartServer setup- Other GraphQL test files (deployment, workspace, user) should also include permission test cases since they use the same permission checking mechanism through
mockPermissionCheckerKey findings:
- The
common.goshows a centralized permission checking mechanism usingmockPermissionChecker- The
allowPermissionparameter inStartServercontrols permission behavior- Only the project creation endpoint has proper permission denial testing
🔗 Analysis chain
Line range hint
1-143: Verify test coverage for permission scenarios.Let's ensure we have comprehensive test coverage for the new permission functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for permission-related code # Look for other permission-related test cases rg -l "permission.*denied" "api/e2e" # Check for other test files that might need similar permission tests fd -e go -x grep -l "StartServer.*true.*baseSeeder" {} \; api/e2e/Length of output: 474
Script:
#!/bin/bash # Let's improve our search strategy # Look for all test files in api/e2e directory fd -e go . api/e2e/ # Check for StartServer usage in test files rg "StartServer\(" api/e2e/ -A 2 -B 2 # Look for other permission-related tests rg "permission" api/e2e/ -A 2 -B 2Length of output: 2553
Script:
#!/bin/bash # Let's check other GraphQL test files for permission testing patterns # Check gql_deployment_test.go rg "TestDeployment" -A 10 api/e2e/gql_deployment_test.go # Check gql_workspace_test.go rg "TestWorkspace" -A 10 api/e2e/gql_workspace_test.go # Check gql_user_test.go rg "TestUser" -A 10 api/e2e/gql_user_test.go # Look at the common.go to understand permission handling cat api/e2e/common.goLength of output: 5303
api/internal/usecase/interactor/project.go (1)
55-62: Enhance permission check implementation.While the permission check logic is functional, consider these improvements:
- Use a more descriptive error message that includes the action and resource type
- Add logging for permission denied events for security auditing
- Consider moving the permission check after workspace validation to avoid unnecessary permission checks on invalid workspaces
authInfo := adapter.GetAuthInfo(ctx) + if err := i.CanWriteWorkspace(p.WorkspaceID, operator); err != nil { + return nil, err + } + hasPermission, err := i.permissionChecker.CheckPermission(ctx, authInfo, rbac.ResourceProject, rbac.ActionEdit) if err != nil { + log.Printf("permission check error: %v", err) return nil, err } if !hasPermission { - return nil, fmt.Errorf("permission denied") + log.Printf("permission denied for user %s to %s %s", authInfo.UserID, rbac.ActionEdit, rbac.ResourceProject) + return nil, fmt.Errorf("permission denied: user does not have '%s' permission for '%s'", rbac.ActionEdit, rbac.ResourceProject) } - - if err := i.CanWriteWorkspace(p.WorkspaceID, operator); err != nil { - return nil, err - }.github/workflows/ci.yml (1)
Line range hint
1-166: Consider documenting the policy deployment processThe integration of policy management into the CI/CD pipeline is well-structured. Consider adding documentation that explains:
- The policy deployment process
- The structure of policy files
- How to test policy changes locally
- Recovery procedures in case of policy deployment issues
This will help maintain the system and assist other developers in making policy changes safely.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
api/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.github/workflows/ci.yml(5 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)api/.env.example(1 hunks)api/Makefile(1 hunks)api/cmd/policy-generator/main.go(1 hunks)api/e2e/common.go(3 hunks)api/e2e/gql_me_test.go(1 hunks)api/e2e/gql_project_test.go(2 hunks)api/e2e/ping_test.go(1 hunks)api/go.mod(2 hunks)api/internal/adapter/gql/loader_user.go(1 hunks)api/internal/app/app.go(1 hunks)api/internal/app/config/config.go(1 hunks)api/internal/app/main.go(3 hunks)api/internal/app/repo.go(2 hunks)api/internal/app/usecase.go(2 hunks)api/internal/rbac/definitions.go(1 hunks)api/internal/usecase/gateway/mock_permission.go(1 hunks)api/internal/usecase/gateway/permission.go(1 hunks)api/internal/usecase/interactor/common.go(1 hunks)api/internal/usecase/interactor/project.go(3 hunks)api/internal/usecase/interactor/project_test.go(2 hunks)api/internal/usecase/interactor/utils_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/internal/app/config/config.go
🚧 Files skipped from review as they are similar to previous changes (12)
- api/.env.example
- api/Makefile
- api/cmd/policy-generator/main.go
- api/go.mod
- api/internal/adapter/gql/loader_user.go
- api/internal/app/app.go
- api/internal/app/repo.go
- api/internal/app/usecase.go
- api/internal/rbac/definitions.go
- api/internal/usecase/gateway/permission.go
- api/internal/usecase/interactor/common.go
- api/internal/usecase/interactor/utils_test.go
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci_policies.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/update_policies.yml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
15-15: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
23-23: the runner of "google-github-actions/auth@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "google-github-actions/setup-gcloud@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
29-29: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:5:26: Double quote to prevent globbing and word splitting
(shellcheck)
29-29: shellcheck reported issue in this script: SC2231:info:20:20: Quote expansions in this for loop glob to prevent wordsplitting, e.g. "$dir"/*.txt
(shellcheck)
29-29: shellcheck reported issue in this script: SC2086:info:29:11: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (22)
api/internal/usecase/gateway/mock_permission.go (2)
1-8: LGTM! Clean imports and package declaration.
The imports are minimal and appropriate for the mock implementation.
1-28: Verify test coverage of this mock implementation.
Let's ensure this mock is being used effectively in tests:
✅ Verification successful
Let me gather more information about the test coverage by examining the actual test cases.
Mock implementation has good test coverage and is being used effectively
The mock is well utilized in test cases with:
- Proper usage in project creation tests with both positive and negative permission scenarios
- A flexible implementation in
utils_test.goallowing custom permission check behaviors - Integration with the project interactor's test setup
- Coverage of permission denied scenarios in e2e tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find test files using this mock
echo "Files using MockPermissionChecker in tests:"
rg -l "MockPermissionChecker" --type go --glob "*_test.go"
# Check for test coverage
echo -e "\nTest scenarios using CheckPermission:"
rg -A 5 "CheckPermission" --type go --glob "*_test.go"
Length of output: 928
Script:
#!/bin/bash
# Check the actual test cases in the files
echo "Content of project_test.go:"
rg -B 5 -A 10 "MockPermissionChecker" api/internal/usecase/interactor/project_test.go
echo -e "\nContent of utils_test.go around MockPermissionChecker usage:"
rg -B 5 -A 10 "MockPermissionChecker" api/internal/usecase/interactor/utils_test.go
# Look for any other permission-related test cases
echo -e "\nOther permission-related test cases:"
rg "permission.*test" --type go
Length of output: 2170
api/e2e/ping_test.go (1)
16-16: Verify if permissions should be enabled for ping endpoint.
While the change aligns with the new permission system, consider if the ping endpoint should require permissions to be enabled (true). The ping endpoint is typically used for basic health checks and might not need permission checks.
✅ Verification successful
The permission setting for ping endpoint is correct
The privateCache middleware only sets cache control headers and doesn't involve authentication. The ping endpoint is publicly accessible and the allowPermission parameter in StartServer is correctly set to true as it's part of the server's global configuration for testing, not specific to the ping endpoint's access control.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other health/ping endpoints in the codebase require permissions
rg -A 5 "func.*[Pp]ing|/ping"
Length of output: 1564
Script:
#!/bin/bash
# Check if the ping endpoint is under any middleware or auth checks
ast-grep --pattern 'api.GET("/ping", $$$)'
# Check if there are any auth-related middleware applied globally
ast-grep --pattern 'api := $$.Group($$$)'
Length of output: 155
Script:
#!/bin/bash
# Check what privateCache middleware does and if it's related to auth
ast-grep --pattern 'privateCache :='
# Check if there are any auth-related middleware applied at the root level
ast-grep --pattern 'e.Use($$$)'
Length of output: 72
Script:
#!/bin/bash
# Let's try a different approach to find middleware definitions
rg -A 5 "privateCache.*=|func privateCache"
# Also check how the server is started in other tests
rg "StartServer.*\(" api/e2e/
Length of output: 1459
.github/workflows/ci_policies.yml (4)
1-6: LGTM! Good practice pinning the Go version.
The workflow configuration is well-structured with a clear trigger and properly pinned Go version.
18-21: Add error handling for policy generation.
This was previously identified in a past review. The policy generation step needs proper error handling.
22-35: Enhance security and reliability of policy file listing.
This security concern was previously identified in a past review. The current implementation could expose sensitive information.
36-42: Improve Cerbos setup reliability and validation.
This was previously identified in a past review. Using 'latest' version can lead to reproducibility issues.
.github/workflows/update_policies.yml (1)
22-27:
Security: Replace service account key with Workload Identity Federation
Using long-lived service account keys stored in secrets is not recommended. Instead, use Workload Identity Federation for secure, short-lived credentials.
- name: Authenticate to Google Cloud
uses: google-github-actions/auth@v1
with:
- credentials_json: ${{ secrets.GCP_SA_KEY }}
+ workload_identity_provider: ${{ secrets.WORKLOAD_IDENTITY_PROVIDER }}
+ service_account: ${{ secrets.SERVICE_ACCOUNT_EMAIL }}🧰 Tools
🪛 actionlint (1.7.4)
23-23: the runner of "google-github-actions/auth@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "google-github-actions/setup-gcloud@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
api/internal/app/main.go (3)
12-12: LGTM: Import additions are appropriate
The new imports for rbac and cerbos client packages align well with the PR's objective of implementing permission checking functionality.
Also applies to: 17-17
55-61: LGTM: Clean integration of PermissionChecker
The permission checker is properly integrated into the server configuration, maintaining consistency with the existing initialization pattern.
50-52: Verify the permission checker integration
Let's ensure the permission checker is properly integrated across the codebase.
Also applies to: 71-77
✅ Verification successful
Permission checker integration is properly implemented
The permission checker is well integrated within the codebase:
- Interface is properly defined in
api/internal/usecase/gateway/permission.go - Implementation is used in project interactor for permission checks
- Proper mocks exist for testing (
mock_permission.go) - The permission checks are used for critical actions (e.g., project editing)
Note: The handlers and middleware are located in the websocket directory, which is a separate component and doesn't require direct permission checker integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the permission checker integration
# Check for middleware usage of PermissionChecker
echo "Checking middleware usage..."
rg -l "PermissionChecker" "api/internal/middleware"
# Check for interface implementation
echo "Checking interface implementation..."
ast-grep --pattern 'type PermissionChecker interface {
$$$
}'
# Check for actual permission checks in handlers
echo "Checking handler usage..."
rg "permissionChecker\." "api/internal/handler"
Length of output: 782
Script:
#!/bin/bash
# Let's verify the actual usage and integration points with a broader search
# Check for all files containing PermissionChecker
echo "Files containing PermissionChecker:"
rg -l "PermissionChecker"
# Check actual usage of permission checks
echo -e "\nPermission check calls:"
rg -A 2 "CheckPermission\("
# Check for handler directory
echo -e "\nLocating handler directory:"
fd -t d "handler"
# Check middleware directory
echo -e "\nLocating middleware directory:"
fd -t d "middleware"
Length of output: 1995
api/e2e/common.go (3)
32-33: LGTM: Clean parameter propagation
The addition of the allowPermission parameter is properly propagated to the underlying StartServerAndRepos function.
63-66: LGTM: Consistent parameter handling
The function correctly initializes resources and propagates the allowPermission parameter to StartServerWithRepos.
83-93: Consider enhancing permission checker test coverage
While the mock permission checker is functional, it might be too simplistic for comprehensive testing:
- It only returns a fixed boolean value
- It doesn't verify which permissions are being checked
- It doesn't track the number of times permissions are checked
Consider enhancing the mock to:
- Track which permissions are being checked
- Count the number of permission checks
- Support different responses for different permissions
Let's verify how the permission checker is being used:
api/internal/usecase/interactor/project.go (3)
7-8: LGTM! Clean struct modification with proper imports.
The new imports and struct field additions are well-organized and follow Go conventions.
Also applies to: 22-30
33-43: LGTM! Constructor properly updated.
The constructor has been correctly modified to handle the new permissionChecker dependency, maintaining proper initialization of all fields.
Line range hint 209-259: Clean up Run method implementation.
The method contains several issues that should be addressed:
- Remove the debug
fmt.Printlnstatement - Either restore or remove the commented code block
- Consider adding comments explaining why the previous workflow-related code was removed/commented out
- fmt.Println("RunProjectParam", p)
+ // TODO: Add proper logging if neededLet's verify if the commented workflow-related code is still needed:
.github/workflows/ci.yml (5)
12-12: LGTM: Output variable addition is well-structured
The new policies output variable follows the established pattern and integrates well with the existing job structure.
24-31: LGTM: Improved formatting of file patterns
The reformatting of the file patterns improves readability while maintaining the same functionality.
98-98: LGTM: Proper integration of policy checks
The addition of ci-policies to the ci job's dependencies ensures that policy checks must pass as part of the CI process.
78-81: LGTM: Well-structured CI policies job
The new ci-policies job is well-integrated into the workflow. However, ensure that the referenced .github/workflows/ci_policies.yml workflow file is also reviewed for completeness.
✅ Verification successful
LGTM: The referenced workflow file is properly configured
The ci_policies.yml workflow is well-structured and contains appropriate steps for policy validation:
- Sets up Go environment for policy generation
- Generates policies using
make gen-policies - Lists and validates generated policy files
- Uses Cerbos for policy compilation and validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the ci_policies.yml workflow
# Check if the workflow file exists
if [ -f ".github/workflows/ci_policies.yml" ]; then
echo "ci_policies.yml exists. Showing content:"
cat ".github/workflows/ci_policies.yml"
else
echo "Warning: ci_policies.yml not found!"
fi
Length of output: 1339
32-41: Verify completeness of policy file patterns
The policy change detection looks well-configured, but let's verify that all relevant policy-related files are included in the pattern.
✅ Verification successful
Policy file monitoring configuration is complete
Based on the verification results, the current policy file monitoring configuration in the CI workflow is appropriate and complete. The search results show that:
- All RBAC-related code is contained within
api/internal/rbac/**, which is correctly included in the pattern - Policy files are generated in the
policiesdirectory under RBAC (as seen indefinitions.go) - The workflow files mentioned in the pattern are the ones handling policy-related tasks
- No additional policy-related files or directories were found that would need monitoring
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any additional policy-related files that might need to be included
# Look for potential policy-related files that might need to be monitored
echo "Searching for potential policy files..."
fd -t f -e yaml -e yml -e json . | grep -i "policy\|rbac\|auth\|permission\|role"
# Look for references to policy files in the codebase
echo "Searching for policy file references in code..."
rg -g '!{.git,node_modules}' -i "policy.*\.(yaml|yml|json)"
Length of output: 339
Script:
#!/bin/bash
# Let's try a different approach to find policy-related files and references
# Search for files in rbac directory
echo "Files in rbac directory:"
fd . api/internal/rbac/
# Search for policy-related imports and references
echo -e "\nPolicy-related code references:"
rg -g '!{.git,node_modules}' -i "rbac|policy|permission" api/
# Check workflow files for policy-related tasks
echo -e "\nPolicy-related workflow configurations:"
cat .github/workflows/check_cerbos_policies.yml
echo -e "\n---\n"
cat .github/workflows/update_cerbos_policies.yml
Length of output: 8418
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
.github/workflows/ci_policies.yml (1)
1-6: Enhance workflow configuration with description and conditions.Consider adding:
- A description to clarify the workflow's purpose
- Conditions to control when this workflow should run
name: ci-policies + +# Generates and validates Cerbos policies for RBAC + on: workflow_call: + inputs: + should_run: + description: 'Whether to run policy checks' + required: false + type: boolean + default: true env: GO_VERSION: '1.22'.github/workflows/update_policies.yml (2)
4-6: Consider parameterizing the GCS bucket pathThe GCS bucket path is hardcoded in the workflow environment variables. Consider making it configurable through workflow inputs or organization variables for better flexibility across environments.
on: workflow_call: + inputs: + gcs_bucket_path: + required: false + type: string + default: 'gs://cerbos-oss-policyfile-bucket' env: GO_VERSION: '1.22' - GCS_BUCKET_PATH: gs://cerbos-oss-policyfile-bucket + GCS_BUCKET_PATH: ${{ inputs.gcs_bucket_path }}
20-27: Simplify error handling in the policy generation stepThe current implementation has redundant error handling. Since
set -eis used, the explicit exit code check is unnecessary.- name: Generate policies run: | set -e make gen-policies - if [ $? -ne 0 ]; then - echo "Policy generation failed" - exit 1 - fi working-directory: api🧰 Tools
🪛 actionlint (1.7.4)
20-20: shellcheck reported issue in this script: SC2181:style:3:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
api/README.mdis excluded by!**/*.md
📒 Files selected for processing (2)
.github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/ci_policies.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
19-19: shellcheck reported issue in this script: SC2181:style:3:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
.github/workflows/update_policies.yml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
15-15: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
20-20: shellcheck reported issue in this script: SC2181:style:3:6: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
29-29: the runner of "google-github-actions/auth@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
33-33: the runner of "google-github-actions/setup-gcloud@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
35-35: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
35-35: shellcheck reported issue in this script: SC2086:info:5:26: Double quote to prevent globbing and word splitting
(shellcheck)
35-35: shellcheck reported issue in this script: SC2231:info:20:20: Quote expansions in this for loop glob to prevent wordsplitting, e.g. "$dir"/*.txt
(shellcheck)
35-35: shellcheck reported issue in this script: SC2086:info:29:11: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (5)
.github/workflows/ci_policies.yml (3)
11-17: Update GitHub Actions to latest versions.
The current action versions are outdated. Update to the latest stable versions for better security and features.
- uses: actions/checkout@v3
+ uses: actions/checkout@v4
- uses: actions/setup-go@v4
+ uses: actions/setup-go@v5🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-42: Enhance security and reliability of policy file listing.
Displaying policy contents in logs might expose sensitive information. Consider only showing file metadata.
43-52: Verify Cerbos version compatibility.
The implementation looks good, but please verify that version 0.36.0 is the latest stable version compatible with your requirements.
.github/workflows/update_policies.yml (2)
28-33:
Security: Replace service account key with Workload Identity Federation
Using long-lived service account keys stored in secrets is not recommended. Instead, use Workload Identity Federation for secure, short-lived credentials.
- name: Authenticate to Google Cloud
uses: google-github-actions/auth@v1
with:
- credentials_json: ${{ secrets.GCP_SA_KEY }}
+ workload_identity_provider: ${{ secrets.WORKLOAD_IDENTITY_PROVIDER }}
+ service_account: ${{ secrets.SERVICE_ACCOUNT_EMAIL }}🧰 Tools
🪛 actionlint (1.7.4)
29-29: the runner of "google-github-actions/auth@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
33-33: the runner of "google-github-actions/setup-gcloud@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
35-65:
Improve shell script robustness and error handling
The current shell script needs improvements in several areas:
- Proper quoting to prevent globbing and word splitting
- Better error handling for gsutil operations
- Validation of policy files before upload
run: |
+ set -euo pipefail
+
echo "All files in bucket (before sync):"
- gsutil ls $GCS_BUCKET_PATH/ || true
+ gsutil ls "${GCS_BUCKET_PATH}/" || true
echo "Current flow files in bucket:"
- bucket_files=$(gsutil ls $GCS_BUCKET_PATH/flow_*.yaml || true)
+ bucket_files=$(gsutil ls "${GCS_BUCKET_PATH}/flow_*.yaml" || true)
echo "$bucket_files"
echo "Local policy files:"
- local_files=$(ls policies/flow_*.yaml || true)
+ local_files=$(ls policies/flow_*.yaml 2>/dev/null || true)
echo "$local_files"
+ # Validate policy files before upload
+ for file in policies/flow_*.yaml; do
+ if [ -f "$file" ]; then
+ if ! yamllint "$file"; then
+ echo "Error: Invalid YAML in $file"
+ exit 1
+ fi
+ fi
+ done
- for file in policies/flow_*.yaml; do
+ for file in ./policies/flow_*.yaml; do
if [ -f "$file" ]; then
file_name=$(basename "$file")
echo "Uploading/Updating: $file_name"
- gsutil cp "$file" "$GCS_BUCKET_PATH/$file_name"
+ if ! gsutil cp "$file" "${GCS_BUCKET_PATH}/$file_name"; then
+ echo "Error: Failed to upload $file_name"
+ exit 1
+ fi
fi
done🧰 Tools
🪛 actionlint (1.7.4)
35-35: shellcheck reported issue in this script: SC2086:info:2:11: Double quote to prevent globbing and word splitting
(shellcheck)
35-35: shellcheck reported issue in this script: SC2086:info:5:26: Double quote to prevent globbing and word splitting
(shellcheck)
35-35: shellcheck reported issue in this script: SC2231:info:20:20: Quote expansions in this for loop glob to prevent wordsplitting, e.g. "$dir"/*.txt
(shellcheck)
35-35: shellcheck reported issue in this script: SC2086:info:29:11: Double quote to prevent globbing and word splitting
(shellcheck)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/ci_policies.yml (1)
14-17: Consider enabling Go module cachingThe Go setup step has caching disabled (
cache: false). Enabling caching could significantly improve workflow execution time by reusing downloaded Go modules across runs.uses: actions/setup-go@v4 with: go-version: ${{ env.GO_VERSION }} - cache: false + cache: true🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/update_policies.yml (3)
19-26: Enhance error handling in policy generationConsider using stricter error handling flags for better script reliability.
run: | - set -e + set -euo pipefail if ! make gen-policies; then echo "Policy generation failed" exit 1 fi
27-32: Update Google Cloud action versionsThe Google Cloud actions are using outdated versions.
- uses: google-github-actions/auth@v1 + uses: google-github-actions/auth@v2 - uses: google-github-actions/setup-gcloud@v1 + uses: google-github-actions/setup-gcloud@v2🧰 Tools
🪛 actionlint (1.7.4)
28-28: the runner of "google-github-actions/auth@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
32-32: the runner of "google-github-actions/setup-gcloud@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
33-80: Optimize policy sync script efficiencyWhile the script is well-structured with proper error handling, consider these improvements:
- Combine YAML validation and upload loops to avoid duplicate iteration:
- for file in policies/flow_*.yaml; do - if [ -f "$file" ]; then - if ! yamllint "$file"; then - echo "Error: Invalid YAML in $file" - exit 1 - fi - fi - done - - for file in policies/flow_*.yaml; do + # Check if yamllint is available + if ! command -v yamllint >/dev/null 2>&1; then + echo "Warning: yamllint not found, skipping YAML validation" + SKIP_VALIDATION=1 + fi + + for file in policies/flow_*.yaml; do if [ -f "$file" ]; then + # Validate YAML if yamllint is available + if [ -z "${SKIP_VALIDATION:-}" ] && ! yamllint "$file"; then + echo "Error: Invalid YAML in $file" + exit 1 + fi + file_name=$(basename "$file") echo "Uploading/Updating: $file_name"
- Consider adding a check for empty policy directory before processing:
+ # Check if policy directory exists and contains files + if [ ! -d "policies" ] || [ -z "$(find policies -name 'flow_*.yaml' -type f)" ]; then + echo "Error: No policy files found in policies directory" + exit 1 + fi + echo "All files in bucket (before sync):"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)api/internal/app/main.go(3 hunks)
🧰 Additional context used
📓 Learnings (2)
.github/workflows/ci_policies.yml (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: .github/workflows/ci_policies.yml:11-17
Timestamp: 2024-12-12T07:59:34.827Z
Learning: In the 'reearth-flow' repository, when reviewing GitHub Actions workflows (e.g., `.github/workflows/ci_policies.yml`), be aware that the user prefers to maintain consistency across CI workflows and may not want to update action versions in individual files separately. Avoid suggesting action version updates unless they are applied uniformly across all relevant workflows.
.github/workflows/update_policies.yml (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: .github/workflows/update_policies.yml:28-33
Timestamp: 2024-12-12T07:53:05.733Z
Learning: In this project, in workflow files like `.github/workflows/update_policies.yml`, using long-lived service account keys stored in secrets (e.g., `credentials_json: ${{ secrets.GCP_SA_KEY }}`) is acceptable for consistency with other implementations.
🪛 actionlint (1.7.4)
.github/workflows/ci_policies.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/update_policies.yml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
15-15: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
28-28: the runner of "google-github-actions/auth@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
32-32: the runner of "google-github-actions/setup-gcloud@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (8)
.github/workflows/ci_policies.yml (3)
1-6: LGTM: Well-structured workflow configuration
The workflow configuration is well-defined with:
- Clear workflow name
- Reusable workflow setup using
workflow_call - Properly pinned Go version
26-41: Enhance security and reliability of policy file listing.
Two concerns with the current implementation:
- Displaying policy contents in logs might expose sensitive information
- The script could fail silently on permission issues
- name: List generated policy files
run: |
set -eo pipefail
echo "Generated policy files in api/policies/:"
ls -la policies/ || echo "No files found"
if [ -d "policies" ] && [ "$(ls -A policies/)" ]; then
- echo "Content of policy files:"
- for file in policies/*.yaml; do
- echo "--- Content of $file ---"
- cat "$file"
- done
+ echo "Found $(ls -1 policies/*.yaml | wc -l) policy files"
else
echo "No policy files were generated"
exit 1
fi
working-directory: api42-51: LGTM: Robust Cerbos setup and validation
The implementation includes:
- Pinned Cerbos version for reproducibility
- Proper error handling with
set -eo pipefail - Clear validation success message
.github/workflows/update_policies.yml (2)
1-7: LGTM: Well-structured workflow configuration
The workflow configuration is clear and follows best practices with descriptive name and proper environment variable definitions.
12-18: Update GitHub Actions versions and review cache configuration
Several actions are using outdated versions that should be updated for security and performance improvements.
- uses: actions/checkout@v3
+ uses: actions/checkout@v4
- uses: actions/setup-go@v4
+ uses: actions/setup-go@v5Also, consider enabling cache (cache: true) for better performance unless there's a specific reason to disable it.
🧰 Tools
🪛 actionlint (1.7.4)
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
15-15: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
api/internal/app/main.go (3)
7-7: LGTM: Import additions are appropriate
The new imports support the permission checking functionality and are well-organized.
Also applies to: 13-13, 18-18
51-61: Enhance error messages for better debugging
While the error handling is good, consider making the error messages more descriptive:
if conf.DashboardHost == "" {
- log.Fatalf("dashboard host configuration is required")
+ log.Fatalf("dashboard host configuration is required in environment variables")
}
if _, err := url.Parse(conf.DashboardHost); err != nil {
- log.Fatalf("invalid dashboard host URL: %v", err)
+ log.Fatalf("invalid dashboard host URL '%s': %v", conf.DashboardHost, err)
}Also, verify the service name usage across the codebase.
✅ Verification successful
I want to verify if the service name is hardcoded consistently or if there are any configuration options that should be used instead of the hardcoded value.
Service name usage is consistent and properly encapsulated
The service name "flow" is consistently defined as a constant in rbac.ServiceName and properly used across the codebase. The constant is:
- Defined once in
api/internal/rbac/definitions.go - Used appropriately in permission checker initialization
- Referenced correctly in policy generator
No issues found with the current implementation as it follows good practices:
- Single source of truth for the service name
- Encapsulated within the rbac package
- Consistent usage across different components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of rbac.ServiceName usage
# Check for the definition and usage of ServiceName
echo "Checking ServiceName definition:"
rg -p "ServiceName.*=.*" ./api/internal/rbac/
echo "\nChecking ServiceName usage:"
rg "rbac\.ServiceName" --type go
Length of output: 459
Script:
#!/bin/bash
# Let's verify the usage of "flow" service name in the codebase
echo "Checking 'flow' service name usage:"
rg -p "\"flow\"" --type go
# Let's also check for any configuration or environment variables related to service name
echo -e "\nChecking for service name configuration:"
rg -p "ServiceName|service.*name" --type go -i
Length of output: 557
81-87: Verify permission checker integration
The ServerConfig changes look good, but let's verify:
- The interface implementation
- The usage of the permission checker throughout the application
Also applies to: 65-71
✅ Verification successful
Let me gather more information about how the permission checker is actually used in the codebase.
Permission checker integration is properly implemented
The verification shows that:
- The
PermissionCheckerinterface is well-defined with a clearCheckPermissionmethod ingateway/permission.go - The permission checker is:
- Correctly initialized in
main.go - Properly injected through the middleware chain
- Actually used in the project interactor for permission validation
- Well-tested with mock implementations
- Correctly initialized in
The integration is complete and follows good practices with proper dependency injection and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify permission checker integration
# Check the PermissionChecker interface definition
echo "Checking PermissionChecker interface:"
ast-grep --pattern 'type PermissionChecker interface {
$$$
}'
# Check for places where PermissionChecker should be used
echo "\nChecking PermissionChecker usage in handlers and middleware:"
rg -p "func.*\(.*PermissionChecker.*\)" --type go
Length of output: 1561
Script:
#!/bin/bash
# Check for actual usage of CheckPermission method
echo "Checking for CheckPermission method usage:"
rg "CheckPermission\(" -B 2 -A 2
# Check for project handler implementations to verify permission checks
echo "\nChecking project handler implementations:"
ast-grep --pattern 'func (p *Project) $_($$$) {
$$$
}'
# Check for other potential permission-related code
echo "\nChecking for other permission-related patterns:"
rg "permission" -i --type go
Length of output: 9987
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service
branch
from
317bfdd to
06545d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
api/internal/usecase/interactor/project.go (2)
Line range hint
108-196: Add permission checks to Update and Delete methods.For consistency and security, permission checks should be added to the Update and Delete methods, similar to the Create method.
Consider implementing permission checks in these methods using the suggested
checkPermissionhelper method above.
Remove commented workflow management code
The commented code block can be safely removed as the workflow management functionality has been simplified. The current implementation only requires submitting a job with the workflow path, while the previous complex workflow management (involving workflow repository, finding/removing/saving workflows) has been intentionally removed. This is evident from:
- The active code only uses
batch.SubmitJob()with workflow path- Similar pattern is seen in the deployment interactor where only job submission is needed
- The commented code's complexity (workflow repository operations, workspace permissions) has been deliberately removed in favor of a simpler approach
- Remove lines 18-42 in
api/internal/usecase/interactor/project.go🔗 Analysis chain
Line range hint
200-250: Address commented out code in Run method.Large blocks of commented code should either be:
- Implemented if the functionality is needed
- Removed if it's no longer required
- If it's for future reference, move it to a separate document or ticket
This improves code maintainability and reduces confusion.
Let me gather more context about this code to understand if the commented workflow functionality has been moved elsewhere or is still needed.
Let me check one more thing to see if this workflow functionality has been moved to a different location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are related issues or PRs about workflow functionality gh issue list --search "workflow" --state all gh pr list --search "workflow" --state allLength of output: 4123
Script:
#!/bin/bash # Check for workflow-related functionality in the codebase rg -A 5 "workflowRepo|projectRepo.FindByID|CanWriteWorkspace" --type go # Check for recent changes to this file git log -p --since="3 months ago" -- api/internal/usecase/interactor/project.goLength of output: 8520
Script:
#!/bin/bash # Look for recent PRs that might have moved this functionality git log --oneline --since="6 months ago" | grep -i "workflow\|refactor" # Check for similar workflow management code rg "SubmitJob.*Workflow" --type go -A 5Length of output: 934
🧹 Nitpick comments (12)
api/e2e/gql_user_test.go (2)
229-229: Maintaining uniform server configurations
Here, the Node test references the updated StartGQLServer signature. Please confirm that all references to StartGQLServer use the same pattern, preventing discrepancies in test environments.
252-252: Added boolean parameter for Nodes test
The Nodes test now includes the new boolean parameter. It might be beneficial to clarify the purpose or expected effect of this parameter via function documentation if it is not sufficiently described elsewhere..github/workflows/ci_policies.yml (2)
32-36: Consider adding content validation for policy filesThe current implementation displays the content of policy files in logs, which might expose sensitive information. Consider adding validation without exposing the content.
- echo "Content of policy files:" - for file in policies/*.yaml; do - echo "--- Content of $file ---" - cat "$file" - done + echo "Found $(ls -1 policies/*.yaml | wc -l) policy files" + for file in policies/*.yaml; do + if ! yamllint "$file"; then + echo "Error: Invalid YAML in $file" + exit 1 + fi + done
42-51: Add version validation for CerbosConsider adding a version check to ensure compatibility with the specified Cerbos version.
- name: Validate Cerbos policies run: | set -eo pipefail + cerbos version | grep -q "0.36.0" || echo "Warning: Unexpected Cerbos version" cerbos compile ./policies echo "Policy validation successful" working-directory: api.github/workflows/update_policies.yml (2)
6-6: Consider parameterizing the GCS bucket pathThe GCS bucket path is hardcoded. Consider making it configurable through workflow inputs or environment variables.
- GCS_BUCKET_PATH: gs://cerbos-oss-policyfile-bucket + GCS_BUCKET_PATH: ${{ inputs.gcs_bucket_path || 'gs://cerbos-oss-policyfile-bucket' }}
47-54: Add retry mechanism for YAML validationThe YAML validation could fail due to temporary issues. Consider adding retries.
+ max_retries=3 for file in policies/flow_*.yaml; do if [ -f "$file" ]; then - if ! yamllint "$file"; then - echo "Error: Invalid YAML in $file" - exit 1 + attempt=1 + while [ $attempt -le $max_retries ]; do + if yamllint "$file"; then + break + fi + echo "Attempt $attempt of $max_retries: YAML validation failed for $file" + attempt=$((attempt + 1)) + [ $attempt -le $max_retries ] && sleep 2 + done + if [ $attempt -gt $max_retries ]; then + echo "Error: YAML validation failed after $max_retries attempts for $file" + exit 1 fi fi doneapi/internal/app/main.go (1)
51-61: Consider adding connection test for permission checkerWhile the URL validation is good, consider adding a connection test to ensure the permission checker can communicate with the dashboard.
permissionChecker := cerbosClient.NewPermissionChecker(rbac.ServiceName, conf.DashboardHost) if permissionChecker == nil { log.Fatalf("failed to initialize permission checker") } + // Test connection + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if _, err := permissionChecker.CheckPermission(ctx, nil, "test", "read"); err != nil { + log.Warnf("permission checker connection test failed: %v", err) + }api/internal/usecase/interactor/project_test.go (4)
22-31: Add documentation to the helper function.Consider adding a doc comment to explain the purpose and usage of the
setupProjecthelper function.+// setupProject initializes a Project instance with the given permission checker for testing. +// It sets up all required dependencies including repositories and transaction handler. func setupProject(t *testing.T, permissionChecker *mockPermissionChecker) *Project {
36-38: Improve mock permission checker naming.The variable name could be more descriptive to indicate its behavior.
-mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { +mockPermissionCheckerAlwaysAllow := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) {
50-112: Consider adding more edge cases to test suite.The test cases cover the main scenarios well, but consider adding tests for:
- Invalid project name (empty or too long)
- Special characters in description
- Edge cases for workspace permissions
124-126: Enhance error assertion messages.Consider adding more descriptive messages to the assertions to make test failures more informative.
-assert.EqualError(t, err, tt.wantErr.Error()) -assert.Nil(t, got, "project should be nil when error is expected") +assert.EqualError(t, err, tt.wantErr.Error(), "unexpected error message in %s case", tt.name) +assert.Nil(t, got, "project should be nil when error is expected in %s case", tt.name)api/internal/usecase/interactor/project.go (1)
29-30: Document the permissionChecker field.Add documentation to explain the purpose of the permissionChecker field.
+// permissionChecker handles authorization checks for project operations permissionChecker gateway.PermissionChecker
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
api/README.mdis excluded by!**/*.mdapi/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
.github/workflows/ci.yml(5 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)api/.env.example(1 hunks)api/Makefile(1 hunks)api/cmd/policy-generator/main.go(1 hunks)api/e2e/common.go(6 hunks)api/e2e/gql_me_test.go(1 hunks)api/e2e/gql_param_test.go(1 hunks)api/e2e/gql_project_test.go(2 hunks)api/e2e/gql_user_test.go(6 hunks)api/e2e/gql_workspace_test.go(6 hunks)api/e2e/ping_test.go(1 hunks)api/go.mod(2 hunks)api/internal/adapter/gql/loader_user.go(1 hunks)api/internal/app/app.go(1 hunks)api/internal/app/config/config.go(1 hunks)api/internal/app/main.go(3 hunks)api/internal/app/repo.go(2 hunks)api/internal/app/usecase.go(2 hunks)api/internal/rbac/definitions.go(1 hunks)api/internal/usecase/gateway/mock_permission.go(1 hunks)api/internal/usecase/gateway/permission.go(1 hunks)api/internal/usecase/interactor/common.go(2 hunks)api/internal/usecase/interactor/project.go(3 hunks)api/internal/usecase/interactor/project_test.go(2 hunks)api/internal/usecase/interactor/utils_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/e2e/gql_workspace_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- api/cmd/policy-generator/main.go
- api/internal/usecase/gateway/permission.go
- api/.env.example
- api/internal/app/app.go
- api/Makefile
- api/internal/app/repo.go
- api/e2e/ping_test.go
- api/internal/app/config/config.go
- api/internal/usecase/interactor/common.go
- api/internal/app/usecase.go
- api/internal/usecase/gateway/mock_permission.go
- api/internal/usecase/interactor/utils_test.go
- api/internal/adapter/gql/loader_user.go
- api/e2e/gql_me_test.go
- api/e2e/gql_project_test.go
- api/go.mod
- api/internal/rbac/definitions.go
- api/e2e/common.go
🧰 Additional context used
📓 Learnings (3)
.github/workflows/ci.yml (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: .github/workflows/ci.yml:159-166
Timestamp: 2024-12-12T08:11:40.839Z
Learning: Policy file validation is performed by the `ci-policies.yml` workflow in the `.github/workflows/` directory.
.github/workflows/ci_policies.yml (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: .github/workflows/ci_policies.yml:11-17
Timestamp: 2024-12-12T07:59:34.827Z
Learning: In the 'reearth-flow' repository, when reviewing GitHub Actions workflows (e.g., `.github/workflows/ci_policies.yml`), be aware that the user prefers to maintain consistency across CI workflows and may not want to update action versions in individual files separately. Avoid suggesting action version updates unless they are applied uniformly across all relevant workflows.
.github/workflows/update_policies.yml (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: .github/workflows/update_policies.yml:28-33
Timestamp: 2024-12-12T07:53:05.733Z
Learning: In this project, in workflow files like `.github/workflows/update_policies.yml`, using long-lived service account keys stored in secrets (e.g., `credentials_json: ${{ secrets.GCP_SA_KEY }}`) is acceptable for consistency with other implementations.
🪛 actionlint (1.7.4)
.github/workflows/ci_policies.yml
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/update_policies.yml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
15-15: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
28-28: the runner of "google-github-actions/auth@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
32-32: the runner of "google-github-actions/setup-gcloud@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (8)
api/e2e/gql_param_test.go (1)
76-76: New parameter passed to StartGQLServer
You’ve introduced a new boolean parameter to the server initialization. Ensure that the code properly handles it downstream and it is covered by test scenarios if it affects the server’s configuration or behavior.
api/e2e/gql_user_test.go (4)
104-104: Added boolean parameter in StartGQLServer
The addition of the new boolean parameter can affect how the server is set up. Please verify that any relevant feature toggles or environment configurations are appropriately updated or tested.
130-130: Introduced new parameter in test setup
Another instance of adding an extra parameter to StartGQLServer. Ensure consistency in parameter usage throughout all test environments and confirm that the code referencing this new parameter is adequately tested.
160-160: Ensuring consistency with the new parameter
Adding the same parameter for DeleteMe test setup. If this parameter enables special logic, confirm that you’ve included coverage for scenarios requiring or bypassing that logic in these tests.
189-189: Check for test coverage of the new parameter
The SearchUser test also includes the new boolean parameter. Make sure that none of the existing tests inadvertently bypass or override the logic introduced with this parameter.
.github/workflows/ci_policies.yml (1)
1-17: LGTM: Workflow configuration is well-structured
The workflow configuration is clear and well-organized. Note that while there are newer versions of the actions available, keeping them consistent with other workflows as per your preference.
🧰 Tools
🪛 actionlint (1.7.4)
12-12: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
14-14: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
api/internal/app/main.go (1)
81-87: LGTM: ServerConfig struct changes are well-organized
The addition of PermissionChecker to ServerConfig is clean and follows the existing pattern.
.github/workflows/ci.yml (1)
78-81: LGTM! Well-structured CI workflow for policy management.
The workflow appropriately:
- Checks for policy-related changes
- Runs policy validation via ci-policies workflow
- Updates policies only after successful validation
Note: As per the learnings, policy file validation is handled by the ci-policies.yml workflow.
Also applies to: 159-166
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/internal/usecase/interactor/project.go (2)
263-273: Add logging to permission check failures.The permission check implementation is good, but adding logging would help with debugging and auditing.
Consider adding logging:
func (i *Project) checkPermission(ctx context.Context, action string) error { authInfo := adapter.GetAuthInfo(ctx) hasPermission, err := i.permissionChecker.CheckPermission(ctx, authInfo, rbac.ResourceProject, action) if err != nil { + log.Printf("permission check failed: %v", err) return fmt.Errorf("failed to check permission: %w", err) } if !hasPermission { + log.Printf("permission denied for action %s", action) return ErrPermissionDenied } return nil }
63-66: Consider reordering permission checks.The workspace permission check should precede the RBAC permission check as it's more specific.
Reorder the checks:
func (i *Project) Create(ctx context.Context, p interfaces.CreateProjectParam, operator *usecase.Operator) (_ *project.Project, err error) { + if err := i.CanWriteWorkspace(p.WorkspaceID, operator); err != nil { + return nil, err + } if err := i.checkPermission(ctx, rbac.ActionCreate); err != nil { return nil, err } - if err := i.CanWriteWorkspace(p.WorkspaceID, operator); err != nil { - return nil, err - }Also applies to: 67-68
api/internal/usecase/interactor/common.go (1)
Line range hint
21-44: Document the permission system architecture.Consider adding documentation that covers:
- The role of
permissionCheckerin the authorization flow- Integration points with reearthx's permission system
- Available permissions and their scope
- Error handling and permission denial scenarios
This will help future maintainers understand the authorization system's design and implementation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/internal/rbac/definitions.go(1 hunks)api/internal/usecase/interactor/common.go(3 hunks)api/internal/usecase/interactor/project.go(5 hunks)
🧰 Additional context used
📓 Learnings (2)
api/internal/usecase/interactor/common.go (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: api/internal/usecase/interactor/common.go:0-0
Timestamp: 2024-12-12T07:54:31.640Z
Learning: In the `NewContainer` function within `api/internal/usecase/interactor/common.go`, pass `permissionChecker` directly as an argument instead of including it in `ContainerConfig`, to maintain consistency with other parameters like `accountgateway.Container`.
api/internal/rbac/definitions.go (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: api/internal/rbac/definitions.go:22-27
Timestamp: 2024-12-12T07:50:03.037Z
Learning: In `api/internal/rbac/definitions.go`, sample roles are intended for internal use and do not need to be exported or documented.
🔇 Additional comments (10)
api/internal/usecase/interactor/project.go (1)
47-50: LGTM! Consistent permission checks across CRUD operations.
The permission checks are consistently implemented across all CRUD operations using the helper method.
Also applies to: 55-58, 63-66, 116-119, 173-176
api/internal/rbac/definitions.go (7)
1-5: LGTM: Package structure and imports are well-defined
The package name 'rbac' clearly indicates its purpose, and the import is correctly specified.
12-17: LGTM: Resource constants are well-defined
The resource constants follow a consistent naming pattern and cover the essential entities in the system.
19-29: LGTM: Action constants provide comprehensive coverage
The action constants cover both standard CRUD operations and specialized actions like import/export, providing a complete set of possible operations.
31-35: LGTM: Role constants align with internal usage
The role constants are appropriately unexported as they are sample roles for internal use.
37-38:
Add parameter validation to prevent panic
The function should validate the builder parameter to prevent potential panic.
func DefineResources(builder *generator.ResourceBuilder) []generator.ResourceDefinition {
+ if builder == nil {
+ panic("ResourceBuilder cannot be nil")
+ }
return builder.Likely invalid or redundant comment.
7-10: Verify service name and policy directory configuration
Let's ensure these values align with the broader system configuration.
✅ Verification successful
Let me gather more context about how these constants are used in the codebase.
Service name and policy directory constants are correctly configured and used
The constants are appropriately used throughout the codebase:
ServiceNameis used in permission checker initialization and policy generationPolicyFileDiris used in the policy generator command- Both constants are properly scoped in the
rbacpackage and referenced by other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify service name and policy directory usage
# Check if service name is consistently used
echo "=== Checking service name usage ==="
rg -l "ServiceName.*flow"
# Verify policy directory exists and is used in CI
echo "=== Checking policy directory usage ==="
rg -l "PolicyFileDir.*policies"
fd "policies" .
Length of output: 313
Script:
#!/bin/bash
# Search for actual usage of these constants in the codebase
echo "=== Checking ServiceName usage ==="
rg "ServiceName" -A 2 -B 2
echo "=== Checking PolicyFileDir usage ==="
rg "PolicyFileDir" -A 2 -B 2
# Check if there are any policy-related files or directories
echo "=== Looking for policy files ==="
fd -t f ".*policy.*" .
Length of output: 1728
39-97: Verify permission assignments align with business requirements
Let's verify that:
- The general role can edit user profiles and manage workspaces
- Only admin role can manage members
- Editor role has appropriate project-level permissions
✅ Verification successful
Based on the gathered information from the shell scripts, I can now provide a final response:
Permission assignments align with business requirements
The permission definitions in the codebase show:
- General role can edit user profiles and has full workspace management (list/create/edit/delete) as defined
- Only admin role has member management permissions (create/edit/delete)
- Editor role has comprehensive project-level permissions including list, create, edit, delete, get, import, export, duplicate, and transfer
The implementation is supported by:
- A proper
PermissionCheckerinterface and its implementations - Permission validation in project interactors
- Unit tests for permission checking logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify permission assignments across the codebase
# Check for any existing permission checks
echo "=== Checking permission usage patterns ==="
rg -A 3 "CheckPermission.*ResourceUser.*ActionEdit"
rg -A 3 "CheckPermission.*ResourceWorkspace"
rg -A 3 "CheckPermission.*ResourceMember"
rg -A 3 "CheckPermission.*ResourceProject"
# Look for any tests validating these permissions
echo "=== Checking for permission tests ==="
fd -e go . | rg -l "Test.*Permission"
Length of output: 764
Script:
#!/bin/bash
# Let's try a different approach to verify permission implementations
# Check for permission-related middleware or handlers
echo "=== Checking permission middleware and handlers ==="
rg -A 5 "func.*CheckPermission"
rg -A 5 "func.*HasPermission"
# Look for permission-related test files
echo "=== Checking permission test files ==="
fd "permission.*test.go"
# Check for actual usage of these resources in handlers/controllers
echo "=== Checking resource usage in handlers ==="
rg "ResourceUser|ResourceWorkspace|ResourceMember|ResourceProject" --type go -A 2
# Check for any permission validation in the business logic
echo "=== Checking business logic permission validation ==="
ast-grep --pattern 'if $perm, $err := $checker.CheckPermission($ctx, $_, $resource, $_); $err != nil { $$$ }'
Length of output: 3312
Script:
#!/bin/bash
# Let's check the implementation of permission checks and their usage in handlers
# Check for permission middleware implementation
echo "=== Checking permission middleware implementation ==="
rg -A 10 "type.*PermissionChecker" --type go
# Look for handler implementations using these resources
echo "=== Checking handlers implementation ==="
rg -A 5 "func.*Handle.*User" --type go
rg -A 5 "func.*Handle.*Workspace" --type go
rg -A 5 "func.*Handle.*Member" --type go
rg -A 5 "func.*Handle.*Project" --type go
# Check for role-based implementations
echo "=== Checking role-based implementations ==="
rg "roleGeneral|roleAdmin|roleEditor" --type go -A 2
# Look for any permission validation tests
echo "=== Checking permission validation tests ==="
rg -A 5 "Test.*Permission.*" --type go
Length of output: 6337
api/internal/usecase/interactor/common.go (2)
5-5: LGTM! Error definition follows Go conventions.
The new error variable is well-defined and follows Go's error naming conventions.
Also applies to: 21-22
40-40: Verify NewProject implementation.
The permissionChecker is correctly passed to NewProject, but let's verify its implementation.
✅ Verification successful
Let me gather more context about the permission checking implementation to ensure it's properly integrated.
Let me check one final aspect - how the permission checking is integrated into the Project's methods to ensure it's properly used across all operations.
Permission checker implementation verified successfully
The permissionChecker is properly integrated:
- Correctly initialized in NewProject constructor
- Implements the required PermissionChecker interface
- Consistently used across all Project operations (Create, Update, Delete, Fetch, FindByWorkspace)
- Properly checks against the "project" resource with appropriate actions (list, create, edit, delete)
- Follows the expected error handling pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check NewProject implementation and usage of permissionChecker
# Check NewProject implementation
echo "=== NewProject Implementation ==="
rg -A 10 "func NewProject.*permissionChecker" .
# Check permissionChecker usage in Project methods
echo -e "\n=== Permission Checker Usage ==="
rg "permissionChecker\." .
Length of output: 1289
Script:
#!/bin/bash
# Check the Project struct definition and interface
echo "=== Project Struct Definition ==="
rg -A 10 "type Project struct" .
echo -e "\n=== Project Interface Definition ==="
rg -A 15 "type Project interface" .
# Check the permission checker interface and usage context
echo -e "\n=== Permission Checker Interface ==="
rg -A 5 "type PermissionChecker interface" .
# Check where CheckPermission is called in project.go
echo -e "\n=== CheckPermission Usage Context ==="
rg -B 5 -A 5 "CheckPermission\(ctx, authInfo" .
Length of output: 8103
Script:
#!/bin/bash
# Check where checkPermission is called within Project methods
echo "=== Project Method Permission Checks ==="
rg -B 2 -A 2 "checkPermission\(ctx," ./api/internal/usecase/interactor/project.go
# Check the RBAC resource constants
echo -e "\n=== RBAC Resource Constants ==="
rg "ResourceProject.*=" .
# Check all Project methods to verify permission check coverage
echo -e "\n=== Project Methods ==="
ast-grep --pattern 'func \(i \*Project\) $_($$$) $_' ./api/internal/usecase/interactor/project.go
Length of output: 1651
| func NewContainer(r *repo.Container, g *gateway.Container, | ||
| ar *accountrepo.Container, ag *accountgateway.Container, | ||
| permissionChecker gateway.PermissionChecker, | ||
| config ContainerConfig, | ||
| ) interfaces.Container { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil check for permissionChecker.
Consider adding validation to handle nil permissionChecker gracefully:
func NewContainer(r *repo.Container, g *gateway.Container,
ar *accountrepo.Container, ag *accountgateway.Container,
permissionChecker gateway.PermissionChecker,
config ContainerConfig,
) interfaces.Container {
+ if permissionChecker == nil {
+ panic("permissionChecker must not be nil")
+ }
job := NewJob(r, g)📝 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 NewContainer(r *repo.Container, g *gateway.Container, | |
| ar *accountrepo.Container, ag *accountgateway.Container, | |
| permissionChecker gateway.PermissionChecker, | |
| config ContainerConfig, | |
| ) interfaces.Container { | |
| func NewContainer(r *repo.Container, g *gateway.Container, | |
| ar *accountrepo.Container, ag *accountgateway.Container, | |
| permissionChecker gateway.PermissionChecker, | |
| config ContainerConfig, | |
| ) interfaces.Container { | |
| if permissionChecker == nil { | |
| panic("permissionChecker must not be nil") | |
| } | |
| job := NewJob(r, g) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/internal/rbac/definitions.go (1)
18-25: Consider documenting the purpose ofActionAny.While most actions are self-explanatory CRUD operations, the purpose and implications of
ActionAnycould benefit from clarification.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/internal/rbac/definitions.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
api/internal/rbac/definitions.go (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: api/internal/rbac/definitions.go:22-27
Timestamp: 2024-12-12T07:50:03.037Z
Learning: In `api/internal/rbac/definitions.go`, sample roles are intended for internal use and do not need to be exported or documented.
🔇 Additional comments (4)
api/internal/rbac/definitions.go (4)
1-5: LGTM!
Package declaration and imports are appropriate for the RBAC implementation.
12-16: LGTM!
Resource constants are well-defined and follow a consistent naming pattern.
27-33: LGTM!
Role constants are appropriately defined as unexported sample roles.
7-10: Verify the service name and policy directory configuration.
The constants look good and are appropriately exported for use by the policy generator.
✅ Verification successful
The previous search only found the definition. Let's check actual usage of these constants in the codebase to verify if they are properly utilized.
Constants are correctly defined and used throughout the codebase
The constants are appropriately used in critical parts of the application:
ServiceNameis used in permission checker initialization and policy generationPolicyFileDiris used in the policy generator as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the usage of service configuration constants
echo "=== Checking usage of ServiceName and PolicyFileDir ==="
rg -l "ServiceName.*=.*\"flow\"|PolicyFileDir.*=.*\"policies\""
Length of output: 215
Script:
#!/bin/bash
# Search for usage of ServiceName and PolicyFileDir constants
echo "=== Checking references to ServiceName ==="
rg "ServiceName" -A 2
echo -e "\n=== Checking references to PolicyFileDir ==="
rg "PolicyFileDir" -A 2
Length of output: 1149
| func DefineResources(builder *generator.ResourceBuilder) []generator.ResourceDefinition { | ||
| return builder. | ||
| AddResource(ResourceUser, []generator.ActionDefinition{ | ||
| generator.NewActionDefinition(ActionRead, []string{ | ||
| roleSelf, | ||
| }), | ||
| generator.NewActionDefinition(ActionEdit, []string{ | ||
| roleSelf, | ||
| }), | ||
| }). | ||
| AddResource(ResourceWorkspace, []generator.ActionDefinition{ | ||
| generator.NewActionDefinition(ActionList, []string{ | ||
| roleSelf, | ||
| }), | ||
| generator.NewActionDefinition(ActionCreate, []string{ | ||
| roleSelf, | ||
| }), | ||
| generator.NewActionDefinition(ActionEdit, []string{ | ||
| roleOwner, | ||
| }), | ||
| generator.NewActionDefinition(ActionDelete, []string{ | ||
| roleOwner, | ||
| }), | ||
| }). | ||
| AddResource(ResourceProject, []generator.ActionDefinition{ | ||
| generator.NewActionDefinition(ActionList, []string{ | ||
| roleSelf, | ||
| }), | ||
| generator.NewActionDefinition(ActionCreate, []string{ | ||
| roleOwner, | ||
| roleMaintainer, | ||
| }), | ||
| generator.NewActionDefinition(ActionEdit, []string{ | ||
| roleOwner, | ||
| roleMaintainer, | ||
| }), | ||
| generator.NewActionDefinition(ActionDelete, []string{ | ||
| roleOwner, | ||
| roleMaintainer, | ||
| }), | ||
| generator.NewActionDefinition(ActionAny, []string{ | ||
| roleReader, | ||
| roleEditor, | ||
| roleOwner, | ||
| roleMaintainer, | ||
| }), | ||
| }). | ||
| Build() | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Builder validation needed, but ActionAny permissions are appropriate
- The nil check for the builder parameter is still recommended as there's no validation present.
- After analyzing the codebase, the
ActionAnypermission withroleReaderis appropriate because:- The role hierarchy is well-defined with clear separation (
self,reader,editor,owner,maintainer) ActionAnyis only used in the Project resource context- The permission is consistently applied alongside other role-based checks
- E2E tests show proper role-based access control implementation
- The role hierarchy is well-defined with clear separation (
func DefineResources(builder *generator.ResourceBuilder) []generator.ResourceDefinition {
+ if builder == nil {
+ panic("ResourceBuilder cannot be nil")
+ }
return builder.🔗 Analysis chain
Add parameter validation and consider security implications.
Two suggestions for improvement:
- Add validation for the
builderparameter to prevent panic on nil. - Review the broad permissions granted by
ActionAny, especially forroleReader.
Add builder validation:
func DefineResources(builder *generator.ResourceBuilder) []generator.ResourceDefinition {
+ if builder == nil {
+ panic("ResourceBuilder cannot be nil")
+ }
return builder.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential security implications of broad permissions
echo "=== Checking usage of ActionAny permissions ==="
rg -A 5 "ActionAny.*roleReader" .
Length of output: 140
Script:
#!/bin/bash
# Let's first check the usage of ActionAny in the codebase
echo "=== Checking all usages of ActionAny ==="
rg "ActionAny" -B 2 -A 2
# Check how the permissions are enforced
echo "=== Checking permission enforcement ==="
ast-grep --pattern 'func $_($$$ *generator.ResourceDefinition $$$) bool {
$$$
}'
# Check for any tests related to these permissions
echo "=== Checking related tests ==="
rg "TestDefineResources|ActionAny.*test" .
Length of output: 924
Script:
#!/bin/bash
# Let's check how these permissions are used in the codebase
echo "=== Checking generator package usage ==="
rg "generator\.ResourceBuilder|generator\.ActionDefinition" -B 2 -A 2
# Check the roles definitions and their usage
echo "=== Checking role definitions ==="
rg "roleReader|roleEditor|roleOwner|roleMaintainer|roleSelf" -B 1 -A 1
# Check if there's any permission validation logic
echo "=== Checking permission validation ==="
ast-grep --pattern 'func $_($$$ Action $$$) bool {
$$$
}'
Length of output: 6663
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests