feat(api): authorization-and-roles-management-service-2 FLOW-BE-34 #872
Conversation
WalkthroughThis set of changes introduces new policy update input and jobs in the GitHub Actions workflows, modifies the Makefile and policy generator for automated policy generation, and reorganizes environment configuration. Many test files, GraphQL resolvers, and usecase methods have been updated to remove the legacy operator parameter in favor of a centralized permissionChecker. In addition, RBAC definitions have been added and numerous internal adapter and usecase refactorings have streamlined permission checking and removed deprecated workspace filtering functions. Overall, the changes simplify method signatures and align permission management with the new Cerbos-based approach. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant P as Prepare Job
participant CP as CI-Policies Job
participant CI as CI Job
participant UP as Update-Policies Job
GH->>P: Trigger workflow (with force_update_policies option)
P-->>GH: Output detected policy changes
GH->>CP: Start CI-Policies if policies changed
CP-->>GH: Validate and compile policies
GH->>CI: Proceed with CI jobs (updates dependency on CI-Policies)
CI->>UP: Conditional trigger for update-policies (on push or manual trigger)
UP-->>GH: Sync policy files to Cloud Storage
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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.
|
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service-2
branch
from
a68ef19 to
4767ac7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
server/api/internal/adapter/gql/loader_project.go (1)
48-49: 🛠️ Refactor suggestionRemove debug print statements.
The code contains multiple debug print statements that should be removed or replaced with proper logging.
- fmt.Printf("DEBUG: Received pagination params: page=%d, pageSize=%d, orderBy=%v, orderDir=%v\n", - pagination.Page, pagination.PageSize, pagination.OrderBy, pagination.OrderDir) - fmt.Printf("DEBUG: Converted pagination params: page=%d, pageSize=%d\n", - paginationParam.Page.Page, paginationParam.Page.PageSize) - fmt.Printf("DEBUG: Returning %d nodes\n", len(nodes)) - for _, n := range nodes { - fmt.Printf("DEBUG: Node name=%s\n", n.Name) - }Also applies to: 54-55, 78-81
server/api/e2e/gql_projectAccess_test.go (1)
96-144: 🛠️ Refactor suggestionAdd test cases for permission checks.
The test suite should include test cases for the new permission checking functionality, such as:
- Attempting to share a project without proper permissions
- Attempting to unshare a project without proper permissions
- Verifying permission checks during project fetch
Would you like me to generate the additional test cases to cover these scenarios?
🧹 Nitpick comments (41)
server/api/internal/usecase/interactor/utils_test.go (2)
13-17: Add documentation for the constructor function.Consider adding a doc comment to explain the default behavior when
nilis passed ascheckFunc. This would help other developers understand that the mock will returntrue, nilby default.+// NewMockPermissionChecker creates a new mock permission checker. +// If checkFunc is nil, CheckPermission will always return true, nil. func NewMockPermissionChecker(checkFunc func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error)) *mockPermissionChecker {
19-24: Consider adding tests for the mock implementation.While this is a test utility, it's important to verify its behavior. Consider adding tests to ensure:
- The default behavior (returning
true, nil) works whencheckPermissionFuncis nil- The custom function is correctly called when provided
Would you like me to help generate these test cases?
server/api/internal/usecase/operator.go (1)
7-8: Align TODO plan with the removal timeline
We see a TODO referencing the migration to Cerbos for permission management. Make sure to track a timeline or open a follow-up issue to clarify next steps and avoid leaving stale code in the repository longer than necessary.Would you like me to open a tracking issue that outlines the steps needed before finally removing this file?
server/api/internal/usecase/interactor/asset.go (4)
35-38: Permission check in Fetch method uses rbac.ActionAny.
Consider using a more specific action (e.g.,rbac.ActionReadorrbac.ActionList) for clarity and alignment with read operations.
43-46: Permission check in FindByWorkspace also uses rbac.ActionAny.
Similarly, a list-oriented action (e.g.,rbac.ActionList) may improve clarity and match existing patterns in other interactors.
61-67: Permission check in Create method uses rbac.ActionAny.
Object creation typically pairs better withrbac.ActionCreateto convey intent more precisely.
93-99: Permission check in Remove method uses rbac.ActionAny.
Consider switching torbac.ActionDeleteto accurately reflect the removal operation.server/api/internal/usecase/interactor/parameter.go (6)
36-39: DeclareParameter guarded by rbac.ActionAny.
Create-like actions might best be enforced with rbac.ActionCreate for clarity.
97-100: Fetch method uses rbac.ActionAny.
Using rbac.ActionList or rbac.ActionRead might more precisely reflect data retrieval.
105-108: FetchByProject also protected by rbac.ActionAny.
Similarly, consider a read-focused action if you anticipate more granular permissions.
114-117: RemoveParameter guarded by rbac.ActionAny.
Deleting parameters typically maps to an action like rbac.ActionDelete.
162-165: UpdateParameterOrder also bound by rbac.ActionAny.
Reordering parameters is an edit-type operation; rbac.ActionEdit might be more expressive.
215-218: UpdateParameterValue guarded by rbac.ActionAny.
Since this modifies persisted data, rbac.ActionEdit might be more suitable.server/api/internal/usecase/interactor/job.go (3)
56-58: Consider mapping each action to more specific RBAC levels instead ofrbac.ActionAny.While
checkPermissionworks, usingrbac.ActionAnyfor each method call may overgrant privileges in some scenarios. For “read” or “update” flows, specifying narrower actions (e.g.,rbac.ActionReadorrbac.ActionEdit) can improve the principle of least privilege.
123-126: Re-checking permissions inrunMonitoringLoopcould be redundant.Repeated calls to
checkPermissioninside a long-running loop might introduce unnecessary overhead. Consider whether you truly need to re-check permissions for every iteration.
234-239: Leverage existing method calls for deep validation.This subscription approach calls
i.FindByIDinternally, which already checks permissions. The extra initial check is okay but could be optimized if the prior call is guaranteed.server/api/internal/usecase/interactor/trigger.go (2)
46-48: Ensure the correct RBAC actions for read operations.Landing on
rbac.ActionAnyfor methods likeFetchandFindByWorkspacemight be too permissive. Consider using more specific read-oriented actions if your authorization model mandates it.
243-246: Userbac.ActionDeletein the delete flow.While
rbac.ActionDeleteis typically more explicit for destructive operations, usingrbac.ActionDeleteinstead ofrbac.ActionAnyhelps clarify authorization requirements.server/api/internal/usecase/interactor/deployment.go (4)
51-53: Align permission checks with resource-specific actions.Using
rbac.ActionAnyincheckPermissionfor fetching deployments works but may be too broad. Consider more specific permission levels, such as “read,” to improve security granularity.
113-116: Replacerbac.ActionAnywithrbac.ActionCreatein the Create flow.Preserving the distinction between create and read actions ensures proper RBAC boundaries, especially if the system expands to more nuanced actions.
249-252: Deletion flow ideally usesrbac.ActionDelete.While
rbac.ActionAnycan functionally pass the check, employing a specialized delete action clarifies the nature of the authorized operation and better matches typical RBAC patterns.
292-295: Execute method might warrant a unique action code.Executing deployments is often more akin to a “run” or “launch” permission than a generic “read” or “edit.” Consider introducing an action like
rbac.ActionExecuteto separate this from general read/create/update.server/api/cmd/policy-generator/main.go (1)
10-18: Consider more graceful error handling.While the current implementation works, consider improving error handling to:
- Log more details about the policy generation process
- Allow for cleanup operations before exit
- Use a more structured logging approach
func main() { + log.Println("Starting policy generation...") if err := generator.GeneratePolicies( rbac.ServiceName, rbac.DefineResources, rbac.PolicyFileDir, ); err != nil { - log.Fatalf("Failed to generate policies: %v", err) + log.Printf("Failed to generate policies: %v", err) + os.Exit(1) } + log.Println("Successfully generated policies") }server/api/internal/usecase/gateway/mock_permission.go (1)
9-28: Consider enhancing the mock for better testability.The current implementation is good for basic testing, but could be enhanced to:
- Track method calls for verification
- Allow dynamic responses based on input parameters
- Support customization through constructor options
type MockPermissionChecker struct { Allow bool Error error + // Track method calls + Calls []struct { + Resource string + Action string + AuthInfo *appx.AuthInfo + } } func NewMockPermissionChecker() *MockPermissionChecker { return &MockPermissionChecker{ Allow: true, Error: nil, + Calls: make([]struct { + Resource string + Action string + AuthInfo *appx.AuthInfo + }, 0), } } func (m *MockPermissionChecker) CheckPermission(ctx context.Context, authInfo *appx.AuthInfo, resource string, action string) (bool, error) { + // Record the call + m.Calls = append(m.Calls, struct { + Resource string + Action string + AuthInfo *appx.AuthInfo + }{ + Resource: resource, + Action: action, + AuthInfo: authInfo, + }) + if m.Error != nil { return false, m.Error } return m.Allow, nil }server/api/internal/usecase/interfaces/deployment.go (1)
37-48: LGTM! Consider updating interface documentation.The removal of operator parameter from all methods aligns well with the centralized permission checking approach. The interface remains functionally complete while reducing complexity.
Consider adding interface documentation to clarify that permission checks are now handled internally rather than through operator parameters.
server/api/internal/adapter/gql/resolver_mutation_parameter.go (1)
25-27: Enhance error handling for better GraphQL error reporting.While the error handling is functional, it could be improved to provide better error context in GraphQL responses.
Consider wrapping errors with GraphQL-specific context:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to declare parameter: %w", err) }This pattern would help clients better understand where errors originate while maintaining the internal error details.
Also applies to: 41-44, 65-67, 79-81
server/api/internal/adapter/gql/loader_user.go (1)
46-49: LGTM! Consider documenting the first-match behavior.The implementation now explicitly handles empty results and returns the first matching user. Consider adding a comment to document that only the first matching user is returned when multiple matches exist.
+// SearchUser returns the first user matching the given name or email. +// If multiple users match, only the first one is returned. +// Returns nil if no matching users are found. func (c *UserLoader) SearchUser(ctx context.Context, nameOrEmail string) (*gqlmodel.User, error) {server/api/internal/usecase/interactor/project_test.go (2)
34-38: Consider adding test cases for permission checker errors.The mock permission checker only tests for success and denial cases. Consider adding test cases for error scenarios (e.g., when the permission checker returns an error).
mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { return true, nil }) + +mockPermissionCheckerError := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { + return false, errors.New("permission checker error") +})
93-97: Consider adding more specific error assertions.Instead of just checking the error message, consider using error type assertions for more robust testing.
-assert.EqualError(t, err, tt.wantErr.Error()) +if tt.wantErr == rerror.ErrNotFound { + assert.ErrorIs(t, err, rerror.ErrNotFound) +} else { + assert.EqualError(t, err, tt.wantErr.Error()) +}server/api/internal/rbac/definitions.go (2)
7-39: Consider using iota for action constants.For better maintainability and to prevent duplicate values, consider using iota for action constants.
const ( - ActionRead = "read" - ActionList = "list" - ActionCreate = "create" - ActionEdit = "edit" - ActionDelete = "delete" - ActionAny = "any" + ActionRead Action = iota + ActionList + ActionCreate + ActionEdit + ActionDelete + ActionAny ) + +var actionNames = map[Action]string{ + ActionRead: "read", + ActionList: "list", + ActionCreate: "create", + ActionEdit: "edit", + ActionDelete: "delete", + ActionAny: "any", +}
41-135: Consider adding documentation for resource permissions.The resource definitions would benefit from documentation explaining the permission model and role hierarchy.
+// DefineResources configures RBAC resources and their permissions. +// Role hierarchy: owner > maintainer > editor > reader > self +// Each resource defines specific actions and the roles that can perform them. func DefineResources(builder *generator.ResourceBuilder) []generator.ResourceDefinition {server/api/internal/app/main.go (1)
52-61: Consider more robust URL validation.The current URL validation only checks if the URL is parseable. Consider adding more specific validation for the dashboard host URL.
-if _, err := url.Parse(conf.DashboardHost); err != nil { +parsedURL, err := url.Parse(conf.DashboardHost) +if err != nil { log.Fatalf("invalid dashboard host URL: %v", err) } +if parsedURL.Scheme == "" || parsedURL.Host == "" { + log.Fatalf("dashboard host URL must include scheme and host") +}server/api/internal/usecase/interactor/projectAccess.go (1)
37-39: Consider enhancing error context.The
checkPermissionhelper method could benefit from including more context in the error message to aid debugging.-func (i *ProjectAccess) checkPermission(ctx context.Context, action string) error { - return checkPermission(ctx, i.permissionChecker, rbac.ResourceProjectAccess, action) +func (i *ProjectAccess) checkPermission(ctx context.Context, action string) error { + if err := checkPermission(ctx, i.permissionChecker, rbac.ResourceProjectAccess, action); err != nil { + return fmt.Errorf("project access permission check failed for action %s: %w", action, err) + } + return nil +}server/api/e2e/gql_projectAccess_test.go (1)
96-105: Document the new StartGQLServer parameter.The purpose of the new boolean parameter added to
StartGQLServershould be documented for clarity.server/api/e2e/gql_param_test.go (1)
71-76: Verify test coverage with permissions enabled.The test now runs with permissions enabled (
true). Consider adding test cases that verify behavior when permissions are disabled (false) to ensure comprehensive test coverage.Add a test case with permissions disabled:
func TestDeclareParameter(t *testing.T) { + testCases := []struct { + name string + allowPermission bool + expectedCode int + }{ + { + name: "with permissions enabled", + allowPermission: true, + expectedCode: http.StatusOK, + }, + { + name: "with permissions disabled", + allowPermission: false, + expectedCode: http.StatusForbidden, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e, _ := StartGQLServer(t, &config.Config{ + Origins: []string{"https://example.com"}, + AuthSrv: config.AuthSrvConfig{ + Disabled: true, + }, + }, true, baseSeederUser, tc.allowPermission)server/api/e2e/common.go (1)
148-151: Avoid code duplication in mock permission checker initialization.The mock permission checker initialization is duplicated in both
StartServerWithReposandStartGQLServerWithRepos.Extract the initialization to a helper function:
+func initMockPermissionChecker(allowPermission bool) *gateway.MockPermissionChecker { + mockPermissionChecker := gateway.NewMockPermissionChecker() + mockPermissionChecker.Allow = allowPermission + return mockPermissionChecker +}server/api/internal/usecase/interactor/trigger_test.go (1)
42-46: Consider extracting common test setup code.The mock permission checker and interactor setup is duplicated across multiple test functions. Consider extracting this into a helper function to improve maintainability and reduce duplication.
Example refactor:
+func setupTriggerInteractor(t *testing.T) (interfaces.Trigger, context.Context, *mongo.Client) { + ctx := context.Background() + c := mongotest.Connect(t)(t) + + repo := repo.Container{ + Trigger: mongo.NewTrigger(mongox.NewClientWithDatabase(c)), + Deployment: mongo.NewDeployment(mongox.NewClientWithDatabase(c)), + } + gateway := &gateway.Container{} + mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { + return true, nil + }) + job := NewJob(&repo, gateway, mockPermissionCheckerTrue) + i := NewTrigger(&repo, gateway, job, mockPermissionCheckerTrue) + + return i, ctx, c +}This would simplify each test function and make future maintenance easier.
Also applies to: 127-131, 211-215, 247-251
server/api/.env.example (1)
7-9: LGTM! Consider adding comments to describe each host's purpose.The new section improves configuration organization by grouping related host settings.
Add descriptive comments to clarify the purpose of each host:
# Host configurations -REEARTH_FLOW_HOST=https://localhost:8080 -REEARTH_DASHBOARD_HOST=http://localhost:8090 +# API server host +REEARTH_FLOW_HOST=https://localhost:8080 +# Dashboard web interface host +REEARTH_DASHBOARD_HOST=http://localhost:8090.github/workflows/ci_policies.yml (3)
5-5: Consider using a variable for the Go version.The Go version is hardcoded. Consider moving it to a repository variable or GitHub environment variable for easier maintenance.
12-12: Update checkout action to v4.The workflow uses
actions/checkout@v3while other actions use v4. Consider updating to maintain consistency.- uses: actions/checkout@v3 + uses: actions/checkout@v4
24-25: Enhance error messages with more context.The error messages could be more descriptive to help diagnose issues faster.
- echo "Policy generation failed" + echo "Error: Policy generation failed. Please check the policy generator logs above for details." exit 1 - echo "No policy files were generated" + echo "Error: No policy files were generated in the policies directory. Please verify the policy generator configuration." exit 1Also applies to: 41-42
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.work.sumis excluded by!**/*.sumserver/api/README.mdis excluded by!**/*.mdserver/api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (70)
.github/workflows/ci.yml(6 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)server/api/.env.example(1 hunks)server/api/Makefile(1 hunks)server/api/cmd/policy-generator/main.go(1 hunks)server/api/e2e/common.go(6 hunks)server/api/e2e/gql_deployment_pagination_test.go(1 hunks)server/api/e2e/gql_me_test.go(1 hunks)server/api/e2e/gql_pagination_test.go(3 hunks)server/api/e2e/gql_param_test.go(1 hunks)server/api/e2e/gql_projectAccess_test.go(1 hunks)server/api/e2e/gql_project_test.go(2 hunks)server/api/e2e/gql_trigger_test.go(3 hunks)server/api/e2e/gql_user_test.go(6 hunks)server/api/e2e/gql_workspace_test.go(6 hunks)server/api/e2e/ping_test.go(1 hunks)server/api/go.mod(2 hunks)server/api/internal/adapter/context.go(0 hunks)server/api/internal/adapter/gql/context.go(0 hunks)server/api/internal/adapter/gql/loader_asset.go(2 hunks)server/api/internal/adapter/gql/loader_deployment.go(6 hunks)server/api/internal/adapter/gql/loader_job.go(3 hunks)server/api/internal/adapter/gql/loader_project.go(2 hunks)server/api/internal/adapter/gql/loader_trigger.go(2 hunks)server/api/internal/adapter/gql/loader_user.go(1 hunks)server/api/internal/adapter/gql/loader_workspace.go(1 hunks)server/api/internal/adapter/gql/resolver_mutation_asset.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_deployment.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_parameter.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_project.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_projectAccess.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_trigger.go(3 hunks)server/api/internal/adapter/gql/resolver_project.go(1 hunks)server/api/internal/adapter/gql/resolver_subscription.go(1 hunks)server/api/internal/app/app.go(1 hunks)server/api/internal/app/auth_client.go(0 hunks)server/api/internal/app/config/config.go(1 hunks)server/api/internal/app/main.go(3 hunks)server/api/internal/app/repo.go(2 hunks)server/api/internal/app/trigger.go(1 hunks)server/api/internal/app/usecase.go(1 hunks)server/api/internal/rbac/definitions.go(1 hunks)server/api/internal/usecase/gateway/mock_permission.go(1 hunks)server/api/internal/usecase/gateway/permission.go(1 hunks)server/api/internal/usecase/interactor/asset.go(4 hunks)server/api/internal/usecase/interactor/asset_test.go(3 hunks)server/api/internal/usecase/interactor/common.go(4 hunks)server/api/internal/usecase/interactor/deployment.go(7 hunks)server/api/internal/usecase/interactor/job.go(8 hunks)server/api/internal/usecase/interactor/parameter.go(5 hunks)server/api/internal/usecase/interactor/parameter_test.go(6 hunks)server/api/internal/usecase/interactor/project.go(5 hunks)server/api/internal/usecase/interactor/projectAccess.go(4 hunks)server/api/internal/usecase/interactor/projectAccess_test.go(5 hunks)server/api/internal/usecase/interactor/project_test.go(1 hunks)server/api/internal/usecase/interactor/trigger.go(6 hunks)server/api/internal/usecase/interactor/trigger_test.go(9 hunks)server/api/internal/usecase/interactor/usecase.go(1 hunks)server/api/internal/usecase/interactor/usecase_test.go(5 hunks)server/api/internal/usecase/interactor/utils_test.go(1 hunks)server/api/internal/usecase/interfaces/asset.go(1 hunks)server/api/internal/usecase/interfaces/deployment.go(1 hunks)server/api/internal/usecase/interfaces/job.go(1 hunks)server/api/internal/usecase/interfaces/parameter.go(1 hunks)server/api/internal/usecase/interfaces/project.go(1 hunks)server/api/internal/usecase/interfaces/projectAccess.go(1 hunks)server/api/internal/usecase/interfaces/trigger.go(1 hunks)server/api/internal/usecase/operator.go(1 hunks)server/api/internal/usecase/repo/container.go(0 hunks)
💤 Files with no reviewable changes (4)
- server/api/internal/adapter/context.go
- server/api/internal/adapter/gql/context.go
- server/api/internal/usecase/repo/container.go
- server/api/internal/app/auth_client.go
✅ Files skipped from review due to trivial changes (2)
- server/api/e2e/gql_workspace_test.go
- server/api/go.mod
🧰 Additional context used
🧠 Learnings (2)
server/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`.
server/api/internal/usecase/interactor/deployment.go (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: api/internal/usecase/interactor/deployment.go:48-70
Timestamp: 2025-01-21T10:53:31.572Z
Learning: The Fetch and Find methods in the deployment service require permission checks using the new permission checker mechanism after migrating away from operator-based checks.
🔇 Additional comments (99)
server/api/internal/usecase/interactor/utils_test.go (2)
1-7: LGTM!The package structure and imports are well-organized and appropriate for the test utilities.
9-11: LGTM!The mock struct is well-designed with a flexible function field that allows customizing permission checking behavior in tests.
server/api/internal/usecase/interactor/usecase.go (4)
38-46: Ensure no missing permission checks
Now that the operator parameter is removed, verify if any permission checks or validations need to be applied separately within or before callingRun0.Would you like me to scan for references to
Run0usage to confirm that permission checks are properly handled?
48-56: Signature updated for Run1
No immediate issues. Ensure that removing the operator-based argument does not disrupt any underlying business logic or error handling.
58-66: Refactor aligns with new permission approach
The updated signature seems consistent within the overall changes.
68-79: Review placement of transaction boundaries
By removing the operator, confirm that any previously operator-bound permission checks are either unnecessary or happening at a different layer.server/api/internal/usecase/interactor/common.go (6)
5-5: Imported "fmt" for error formatting
No issues. This addition is useful for the newly introduced error variable.
7-7: Confirm use of internal adapter
The adapter package import suggests new integrations. Verify that all new references to its functions are valid and utilized.
20-20: Added ErrPermissionDenied
Defining a central permission denial error promotes consistent error handling.
31-47: Pass permissionChecker into NewContainer
Good approach to directly injectpermissionCheckerinto each interactor constructor, aligning with the learning from PR #580. This ensures a centralized handling of permissions across sub-usecases.
54-65: Check for missing permission validation
Before deleting a project, consider explicitly usingcheckPermissionto confirm the user’s rights. Without an operator parameter, ensure the new approach covers this scenario.
73-83: New checkPermission function
Implementation seems solid: retrieval of auth info, direct call to the permission checker, and consistent error handling if permission is denied.server/api/internal/usecase/interactor/asset.go (5)
8-8: Imported rbac package for permission checks.
No issues. This import is essential for the newly introduced authorization logic.
18-20: Added struct fields for repos, gateways, and permissionChecker.
Centralizing these dependencies fosters clarity and consistency in theAssetinteractor.
23-23: Updated NewAsset constructor to accept permissionChecker.
Ensure all constructor call sites are updated to supply the correct parameter.Would you like a verification script to confirm references to
NewAssetare passing the new argument?
25-27: Constructor assignment of injected dependencies.
This is a straightforward initialization pattern. All good here.
31-33: New checkPermission method for Asset.
Provides a clear, centralized entry point for RBAC enforcement on this resource.server/api/internal/usecase/interactor/project.go (11)
7-7: Imported rbac package for authorization handling.
No concerns; this is necessary for permission checks.
19-27: Added Project struct fields, including permissionChecker.
Centralizes dependencies and follows the revised RBAC architecture.
30-30: NewProject constructor now accepts permissionChecker.
Be sure that all calls instantiatingNewProjectprovide the new parameter.Would you like a script to ensure this parameter is passed everywhere?
32-39: Constructor assignments for Project dependencies.
This is consistent with the broader refactor toward centralized permission checks.
43-45: Introduction of checkPermission for Project resource.
This is appropriately scoped torbac.ResourceProject.
47-50: Fetch method leverages rbac.ActionList.
The chosen action name matches its read/list intent. Good alignment with RBAC best practices.
55-58: FindByWorkspace method with rbac.ActionList.
Again, consistent usage of a list-oriented action is clear and unambiguous.
63-64: Create method enforced with rbac.ActionCreate.
Well chosen; it disambiguates creation from other operations.
112-115: Update method enforced with rbac.ActionEdit.
Accurately represents the nature of this operation.
166-169: Delete method enforced with rbac.ActionDelete.
Aligns well with domain semantics for removing assets.
200-203: Run method enforced with rbac.ActionEdit.
If “run” is a distinctive privilege in your domain, consider a specialized RBAC action.server/api/internal/usecase/interactor/parameter.go (5)
6-7: Imported rbac and revised gateway references.
This alignsParameterwith the new centralized permission model.
17-20: Added fields for paramRepo, projectRepo, transaction, and permissionChecker.
Enhances clarity and ensures consistent usage of external resources and permission checks.
23-23: NewParameter constructor updated to accept permissionChecker.
Validate that all existing constructor calls are updated accordingly.
25-28: Assigned dependencies in Parameter constructor.
Looks good, no issues.
32-34: checkPermission method for Parameter resource.
Centralizes RBAC checks just like other interactors.server/api/internal/usecase/interactor/job.go (3)
7-31: Introduce a dedicated permission checker field in the struct.The addition of
permissionChecker gateway.PermissionCheckerto theJobstruct centralizes permission logic, which simplifies calls throughout this file. This approach nicely decouples permission checks from the rest of the business logic.
42-53: Constructor parameter ensures consistent permission-checking across the job use case.By passing
permissionCheckerintoNewJoband storing it in the struct, all Job-related methods can rely on a shared, standardized permission checker instead of passing an operator object around.
104-108: Permission check inside monitoring setup.Performing
i.checkPermissionbefore starting the monitoring loop is beneficial for security, but be mindful of potential performance overhead if repeatedly checking permissions in ongoing loops (e.g., inrunMonitoringLoop).server/api/internal/usecase/interactor/trigger.go (4)
8-29: Centralized permission checker field aligns with the new RBAC approach.Introducing
permissionChecker gateway.PermissionCheckerinTriggerand referencing it throughcheckPermissionpromotes consistent and maintainable authorization checks.
74-77: Correct usage ofrbac.ActionCreatefor creation.This aligns with the principle of enforcing granular permissions. Good job matching the action to the method’s intent.
143-146: Potential nil-pointer risk in API-driven triggers.When comparing
p.AuthenticationToken != *trigger.AuthToken(), ensure thattrigger.AuthToken()is never nil. A nil-pointer check could safeguard against unexpected panics.
192-195: Appropriate use ofrbac.ActionEditfor update operations.Specifying a distinct action for modifying triggers helps maintain the principle of least privilege.
server/api/internal/usecase/interactor/deployment.go (1)
11-33: Adoption of a dedicated permission checker aligns with recent RBAC refactoring.Adding
permissionChecker gateway.PermissionCheckerand removing the operator parameter from methods ensure compliance with the updated permission model. This is consistent with the [retrieved learnings] that mention moving away from operator-based checks.server/api/internal/usecase/gateway/permission.go (1)
9-11: LGTM! Well-designed interface for permission checking.The
PermissionCheckerinterface follows good design principles:
- Single responsibility
- Clear method signature
- Focused parameters
- Appropriate error handling
server/api/internal/usecase/interfaces/projectAccess.go (1)
12-13: LGTM! Clean interface refactoring.The removal of the operator parameter from
ShareandUnsharemethods aligns with the centralized permission checking approach, making the interface cleaner and more focused.server/api/internal/usecase/gateway/mock_permission.go (1)
14-14: LGTM! Good practice using type assertion.The type assertion ensures that
MockPermissionCheckerimplements thePermissionCheckerinterface at compile time.server/api/internal/usecase/interfaces/job.go (1)
11-19:❓ Verification inconclusive
Verify permission checks in Job interactor implementation.
The removal of
*usecase.Operatorfrom method signatures aligns with the centralized permission checking approach. However, we need to ensure that proper permission checks are implemented in the corresponding interactor.
🏁 Script executed:
#!/bin/bash # Description: Verify permission checks in Job interactor implementation. # Test: Search for checkPermission calls in the Job interactor. Expect: Permission checks for each method. ast-grep --pattern $'struct JobUsecase { $$$ checkPermission($$$) { $$$ } $$$ }' # Test: Search for permission checks in method implementations rg -A 5 'checkPermission' server/api/internal/usecase/interactor/job.goLength of output: 251
Action: Verify proper permission checks in the Job interactor
It appears that the expected file (
server/api/internal/usecase/interactor/job.go) was not found in the repository. Please verify the following:
- Confirm the correct location of the Job interactor implementation.
- Ensure permission checks are applied for each relevant method (
Fetch,FindByID, andGetStatus) as per the centralized permission checking approach.- Note that, per earlier learnings, permission checks in
FindByID/Fetchand forGetStatusare scheduled for future implementation. Make sure that once implemented, these checks are consistent with the overall design.server/api/e2e/ping_test.go (1)
16-16: LGTM!The addition of the permission check parameter to
StartServeraligns with the new authorization system. The test remains well-structured and comprehensive.server/api/internal/usecase/interfaces/asset.go (1)
28-33:❓ Verification inconclusive
Verify permission checks in Asset interactor implementation.
The removal of
*usecase.Operatorfrom method signatures aligns with the centralized permission checking approach. However, we need to ensure that proper permission checks are implemented in the corresponding interactor.
🏁 Script executed:
#!/bin/bash # Description: Verify permission checks in Asset interactor implementation. # Test: Search for checkPermission calls in the Asset interactor. Expect: Permission checks for each method. ast-grep --pattern $'struct AssetUsecase { $$$ checkPermission($$$) { $$$ } $$$ }' # Test: Search for permission checks in method implementations rg -A 5 'checkPermission' server/api/internal/usecase/interactor/asset.goLength of output: 257
Manual Verification Required: Confirm Permission Checks in Asset Interactor Implementation
- The expected file (
server/api/internal/usecase/interactor/asset.go) was not found, and no occurrences ofcheckPermissionwere detected using the initial search.- Please manually verify that the Asset interactor implementation (or its relocated equivalent) properly enforces permission checks in line with the centralized approach introduced by the removal of
*usecase.Operator.server/api/e2e/gql_me_test.go (1)
17-17: LGTM!The addition of the permission check parameter to
StartServeraligns with the new authorization system. The test remains well-structured and comprehensive.server/api/internal/adapter/gql/resolver_subscription.go (1)
22-22: LGTM! Operator removal aligns with centralized permission checking.The removal of
getOperator(ctx)from theSubscribemethod call is consistent with the PR's objective to centralize permission checking. The core subscription logic and cleanup remain intact.server/api/internal/adapter/gql/resolver_mutation_projectAccess.go (1)
16-16: LGTM! Operator removal maintains security while simplifying interface.The removal of
getOperator(ctx)from bothShareandUnsharemethod calls aligns with the centralized permission checking approach while maintaining proper error handling and input validation.Also applies to: 30-30
server/api/internal/adapter/gql/resolver_project.go (1)
26-26: LGTM! Operator removal maintains functionality.The removal of
getOperator(ctx)fromFetchByProjectis consistent with the centralized permission checking approach while maintaining proper error handling and response transformation.server/api/internal/usecase/interfaces/parameter.go (1)
31-36:❓ Verification inconclusive
LGTM! Interface simplification aligns with centralized permissions.
The removal of
*usecase.Operatorfrom all method signatures in theParameterinterface aligns with the centralized permission checking approach.Let's verify that all implementations of this interface have been updated accordingly:
🏁 Script executed:
#!/bin/bash # Description: Find all implementations of the Parameter interface to ensure they've been updated. ast-grep --pattern $'type \$_ struct { $$$ } func \($_\) DeclareParameter\(context.Context, DeclareParameterParam\) \(\*parameter.Parameter, error\) { $$$ }'Length of output: 163
LGTM! Interface changes properly support centralized permission checking.
The removal of
*usecase.Operatorfrom theParameterinterface inserver/api/internal/usecase/interfaces/parameter.gois correct; however, the initial verification script produced no output. Please run the following command or perform a manual check to ensure that all implementations of the interface have removed any references to*usecase.Operatorfrom their method signatures:#!/bin/bash # Verify that no implementation of the DeclareParameter method still expects a *usecase.Operator argument. rg "\*usecase\.Operator.*DeclareParameter"Once you confirm that no references remain, this review comment can be fully approved.
server/api/internal/app/usecase.go (1)
15-23: LGTM! Improved middleware with centralized permission checking.The changes enhance security by introducing a centralized permission checking mechanism through the
permissionCheckerparameter, replacing the previous operator-based filtering approach.server/api/internal/adapter/gql/resolver_mutation_asset.go (1)
18-21:❓ Verification inconclusive
Verify permission checks in Asset interactor.
The operator parameter has been removed from asset operations. Ensure that the Asset interactor properly implements permission checks using the new centralized permission checker.
Also applies to: 35-35
🏁 Script executed:
#!/bin/bash # Description: Verify permission checks in Asset interactor # Search for permission checks in Asset interactor implementation ast-grep --pattern $'struct Asset { $$$ permissionChecker gateway.PermissionChecker $$$ }' ast-grep --pattern $'func (i *Asset) Create(ctx context.Context, param interfaces.CreateAssetParam) (*asset.Asset, error) { $$$ i.permissionChecker.Check($$$) $$$ }' ast-grep --pattern $'func (i *Asset) Remove(ctx context.Context, aid id.AssetID) (id.AssetID, error) { $$$ i.permissionChecker.Check($$$) $$$ }'Length of output: 422
Confirm centralized permission checking in Asset interactor implementation.
The removal of the operator parameter is acceptable if the centralized permission checker is correctly integrated. However, the automated searches did not return any evidence of explicit calls to
permissionChecker.Checkin the Asset interactor'sCreateorRemovemethods. Please manually verify that either:
- The Asset interactor correctly calls the centralized permission checker during asset creation and removal;
- Or that permission validation is guaranteed by another mechanism in the asset workflow.
server/api/internal/usecase/interfaces/project.go (1)
40-47:❓ Verification inconclusive
LGTM! Interface simplified while maintaining functionality.
The Project interface has been streamlined by removing operator parameters, making it cleaner and more maintainable. This aligns with the centralized permission checking approach.
Ensure that the Project interactor implementation properly utilizes the permission checker for all these operations:
🏁 Script executed:
#!/bin/bash # Description: Verify permission checks in Project interactor # Search for permission checks in Project interactor implementation ast-grep --pattern $'struct Project { $$$ permissionChecker gateway.PermissionChecker $$$ }' ast-grep --pattern $'func (i *Project) Create(ctx context.Context, param interfaces.CreateProjectParam) (*project.Project, error) { $$$ i.permissionChecker.Check($$$) $$$ }' ast-grep --pattern $'func (i *Project) Update(ctx context.Context, param interfaces.UpdateProjectParam) (*project.Project, error) { $$$ i.permissionChecker.Check($$$) $$$ }' ast-grep --pattern $'func (i *Project) Delete(ctx context.Context, pid id.ProjectID) error { $$$ i.permissionChecker.Check($$$) $$$ }'Length of output: 603
LGTM – Interface Simplification Confirmed (Manual Verification Needed for Permission Checks)
The Project interface has been streamlined by removing the operator parameters, which simplifies the API while preserving functionality. However, the automated pattern search for permission checks in the Project interactor (in Create, Update, and Delete implementations) did not return any results. Please manually verify that these methods correctly invoke the permission checker to ensure that permissions are enforced as expected.
- Verify that the Project interactor’s Create, Update, and Delete implementations call the permission checker.
- Confirm that any centralized permission handling logic is properly integrated elsewhere if not directly visible in these methods.
server/api/internal/usecase/interfaces/trigger.go (1)
45-53:❓ Verification inconclusive
LGTM! Interface simplified with focus on security.
The Trigger interface has been streamlined by removing operator parameters. Given the sensitive nature of trigger operations, especially API trigger execution, proper permission checks are crucial.
Ensure that the Trigger interactor implementation properly implements permission checks, particularly for API trigger execution:
🏁 Script executed:
#!/bin/bash # Description: Verify permission checks in Trigger interactor # Search for permission checks in Trigger interactor implementation ast-grep --pattern $'struct Trigger { $$$ permissionChecker gateway.PermissionChecker $$$ }' ast-grep --pattern $'func (i *Trigger) ExecuteAPITrigger(ctx context.Context, param interfaces.ExecuteAPITriggerParam) (*job.Job, error) { $$$ i.permissionChecker.Check($$$) $$$ }' ast-grep --pattern $'func (i *Trigger) Create(ctx context.Context, param interfaces.CreateTriggerParam) (*trigger.Trigger, error) { $$$ i.permissionChecker.Check($$$) $$$ }' ast-grep --pattern $'func (i *Trigger) Update(ctx context.Context, param interfaces.UpdateTriggerParam) (*trigger.Trigger, error) { $$$ i.permissionChecker.Check($$$) $$$ }' ast-grep --pattern $'func (i *Trigger) Delete(ctx context.Context, tid id.TriggerID) error { $$$ i.permissionChecker.Check($$$) $$$ }'Length of output: 791
Security and Permission Checks Verification Required!
The updated Trigger interface simplifies the API by removing operator parameters, which improves maintainability. However, our automated AST grep searches did not yield evidence that the Trigger interactor implementations (such as for ExecuteAPITrigger, Create, Update, and Delete) include the necessary permission checks. Given that these operations are sensitive, please manually verify that the implementations correctly enforce permission verification (e.g., by invoking the relevant permissionChecker methods).
server/api/internal/app/trigger.go (1)
48-58: LGTM! Clean removal of operator parameter.The changes maintain security through token validation while simplifying the code by removing the operator parameter. Error handling remains robust.
server/api/internal/adapter/gql/resolver_mutation_trigger.go (1)
37-37:❓ Verification inconclusive
LGTM! Verify permission handling migration.
The removal of
getOperator(ctx)aligns with the centralized permission checking approach. Please ensure that the new permission checking mechanism is properly implemented in the trigger interactor.Also applies to: 72-72, 86-86
🏁 Script executed:
#!/bin/bash # Verify that permission checking is implemented in the trigger interactor ast-grep --pattern 'type Trigger struct { $$$ permissionChecker $_ $$$ }'Length of output: 82
LGTM – Please Manually Verify Permission Handling
The changes removing
getOperator(ctx)to centralize permission checks are in line with our objectives. However, the automated verification did not reveal an explicitpermissionCheckerfield in the trigger interactor. Please manually verify that the new permission checking mechanism is correctly implemented for all trigger operations (e.g., CreateTrigger, UpdateTrigger, DeleteTrigger).server/api/internal/usecase/interactor/usecase_test.go (1)
31-31:❓ Verification inconclusive
LGTM! Verify permission testing coverage.
The removal of unnecessary
nilparameters simplifies the code. However, sinceTestUc_checkPermissionhas been removed, ensure that permission checking is adequately tested in the new implementation.Also applies to: 47-47, 59-59, 74-74, 89-89
🏁 Script executed:
#!/bin/bash # Check for permission-related test coverage rg -l "TestPermission|test.*permission" -g "*_test.go"Length of output: 205
LGTM – but please verify that permission testing is adequately covered.
The removal of the unnecessary
nilparameters cleans up the function calls nicely. However, as the dedicatedTestUc_checkPermissionwas removed, it's important to ensure that permission checking is now exercised elsewhere in the test suite. The current automated check for permission-related tests produced no results due to file-filtering issues, so please manually confirm that the new implementation includes comprehensive permission test coverage (especially for the changes around lines 31, 47, 59, 74, and 89 inserver/api/internal/usecase/interactor/usecase_test.go).server/api/internal/adapter/gql/resolver_mutation_deployment.go (1)
32-32:❓ Verification inconclusive
LGTM! Verify deployment permission handling.
The removal of
getOperator(ctx)from deployment operations aligns with the centralized permission checking approach. Please ensure that the new permission checking mechanism is properly implemented in the deployment interactor.Also applies to: 49-49, 63-63, 78-78
🏁 Script executed:
#!/bin/bash # Verify that permission checking is implemented in the deployment interactor ast-grep --pattern 'type Deployment struct { $$$ permissionChecker $_ $$$ }'Length of output: 84
LGTM! Please verify that the new permission checking mechanism is active in the deployment interactor.
The removal of
getOperator(ctx)is in line with centralizing permission checks. However, the automated check for apermissionCheckerproperty yielded no output. Kindly perform a manual review in the deployment interactor (e.g.,api/internal/usecase/interactor/deployment.go) to confirm that the new permission validation mechanism is implemented correctly.server/api/internal/adapter/gql/resolver_mutation_project.go (1)
18-23:❓ Verification inconclusive
LGTM! Removal of operator parameter aligns with centralized permission model.
The changes align with the PR's goal of centralizing permission checks in the interactors.
Let's verify that permission checks are still being performed in the underlying use cases:
Also applies to: 37-45, 59-59, 77-80
🏁 Script executed:
#!/bin/bash # Search for permission checks in project use case implementations ast-grep --pattern 'func (i *$_) $_(ctx context.Context, $_, $_) $_ { $$$ i.permissionChecker.$_($$$) $$$ }'Length of output: 116
Permission Checks Verification Pending
The original approval for the removal of the operator parameter and the centralized permission model looks good, but our automated check did not conclusively verify that permission checks are still being executed in the underlying use cases. Please manually confirm that permission validations (whether by direct calls or through centralized components) are present in the implementations corresponding to these lines:
- Lines 18–23 in
server/api/internal/adapter/gql/resolver_mutation_project.go- Also, check sections at lines 37–45, 59, and 77–80
To assist with your investigation, you may run the following shell script to search for any occurrences of “permissionChecker” in the codebase:
#!/bin/bash # Verify if there are any calls to the permission checker in the repository rg "permissionChecker" -n .Once you confirm that permission checks are intact, please update the review accordingly.
server/api/internal/adapter/gql/loader_asset.go (1)
29-29: LGTM! Data loader changes maintain consistency with centralized permission model.The removal of operator parameters from data loader methods aligns with the architectural changes while maintaining proper error handling and data loader patterns.
Also applies to: 43-43
server/api/internal/adapter/gql/loader_workspace.go (1)
27-27:❓ Verification inconclusive
Verify the inconsistent operator usage in workspace loader.
There are inconsistencies in how the operator is handled:
Fetchpassesnilinstead of removing the operator parameterFindByUserstill usesgetAcOperator(ctx)This differs from other loaders where the operator parameter was removed entirely.
Let's check if this is intentional by examining the workspace interface and other implementations:
Also applies to: 45-45
🏁 Script executed:
#!/bin/bash # Search for workspace interface definition and implementations ast-grep --pattern 'type Workspace interface { $$$ Fetch(ctx context.Context, ids []$_, op $_) ($_, error) $$$ }' # Search for other workspace-related operator usage rg "getAcOperator\(ctx\)" -A 5Length of output: 303
Action Required: Manual Verification for Operator Consistency
The current automated searches didn’t yield sufficient results to confirm whether the operator parameter treatment in the
Fetchmethod is intentionally different from other loaders (which eliminate the operator parameter entirely). Please manually verify the following:
- Confirm the expected signature of the
Workspaceinterface—specifically how theFetchmethod should handle the operator parameter.- Check other workspace loader implementations to ensure consistency. In particular, compare:
- The call
res, err := c.usecase.Fetch(ctx, uids, nil)inserver/api/internal/adapter/gql/loader_workspace.go(lines 27 and 45)- The usage of
getAcOperator(ctx)inFindByUserwhich suggests an alternative (or legacy) operator handling.If the design intent is to remove the operator parameter entirely across all loaders, update the
Fetchmethod accordingly. Otherwise, document the rationale for the differing approaches.server/api/internal/adapter/gql/loader_trigger.go (1)
28-28: LGTM! Trigger loader changes maintain consistency with centralized permission model.The removal of operator parameters from data loader methods aligns with the architectural changes while maintaining proper error handling and data loader patterns.
Also applies to: 48-48
server/api/internal/usecase/interactor/project_test.go (2)
20-29: LGTM! Well-structured helper function.The
setupProjecthelper function is well-designed, making the test setup more maintainable and reusable.
48-83: LGTM! Well-structured table-driven tests.The test cases are well-organized using table-driven tests, covering normal creation, nonexistent workspace, and permission denied scenarios.
server/api/internal/adapter/gql/loader_job.go (1)
31-31: LGTM! Consistent removal of operator parameters.The removal of operator parameters aligns with the centralized permission management approach using the
PermissionChecker.Also applies to: 50-50, 66-66
server/api/internal/app/main.go (1)
80-88: LGTM! Well-structured configuration.The
ServerConfigstruct is well-organized with clear field names and types.server/api/internal/usecase/interactor/projectAccess.go (2)
24-24: LGTM: Permission checker integration looks good.The addition of the
permissionCheckerfield and its initialization in the constructor follows good dependency injection practices.Also applies to: 27-34
41-44:❓ Verification inconclusive
Verify consistent permission checks.
The permission checks have been added consistently to all methods (Fetch, Share, Unshare) using
rbac.ActionAny. Verify if this is the intended permission level or if more specific actions should be used.Also applies to: 61-64, 116-119
🏁 Script executed:
#!/bin/bash # Search for all rbac.Action* constants to verify permission granularity rg "rbac\.Action" -A 1Length of output: 173
🏁 Script executed:
#!/bin/bash # Re-run the search for all rbac.Action* constants in the repository, # including hidden and ignored files, to verify permission granularity. rg "rbac\.Action" --hidden --no-ignoreLength of output: 40
Review Verification: Consistent Permission Check Usage
The current implementation uses
rbac.ActionAnyuniformly for permission checks in methods like Fetch, Share, and Unshare (in server/api/internal/usecase/interactor/projectAccess.go, lines 41–44, 61–64, 116–119). Please manually verify that using this broad permission—rather than more specific actions—is the intended security design for these methods, and that it aligns with your overall RBAC strategy.server/api/internal/adapter/gql/loader_project.go (1)
29-29: LGTM: Operator parameter removal is consistent.The removal of the operator parameter from usecase calls aligns with the new permission management approach.
Also applies to: 57-58
server/api/internal/app/repo.go (1)
26-26: LGTM: Account database configuration improvement.The addition of
accountDatabaseNameconstant and its usage in setting the default account database name improves code maintainability.Also applies to: 47-47
server/api/internal/app/app.go (1)
80-85:❓ Verification inconclusive
Verify permission checker initialization.
The permission checker is now passed to the middleware, enhancing security. Ensure that
cfg.PermissionCheckeris properly initialized before this point.Run the following script to verify the permission checker initialization:
🏁 Script executed:
#!/bin/bash # Description: Check if PermissionChecker is properly initialized before use. # Search for PermissionChecker initialization rg -A 5 'type ServerConfig struct' server/api/internal/app/ rg -A 5 'func NewServer' server/api/internal/app/Length of output: 243
Action Required: Verify PermissionChecker Initialization
The middleware now receives
cfg.PermissionCheckerto enhance security. However, the previous automated check did not locate the expected file (server/api/internal/app/app.go), likely due to a change in the repository structure. Please manually verify thatcfg.PermissionCheckeris properly initialized (i.e., it is non-nil and correctly configured) before it is passed to the middleware. If the file has been relocated, update the file path in the verification process accordingly.server/api/e2e/common.go (1)
83-86: LGTM! Mock permission checker is properly initialized.The mock permission checker is initialized with the
allowPermissionvalue, providing good control over permission behavior in tests.server/api/internal/usecase/interactor/projectAccess_test.go (3)
21-23: LGTM! Mock permission checker is well integrated.The mock permission checker is correctly implemented and integrated into the test setup, providing a clean way to handle authorization in tests.
Also applies to: 27-27
104-106: LGTM! Permission checking is properly refactored.The test has been successfully updated to use the centralized permission checker, removing the operator parameter while maintaining comprehensive test coverage.
Also applies to: 113-113, 145-145
173-175: LGTM! Consistent permission handling.The test has been consistently updated to use the centralized permission checker, maintaining proper test coverage for both success and failure cases.
Also applies to: 181-181, 218-218
server/api/internal/app/config/config.go (1)
30-30:❓ Verification inconclusive
Verify the usage of the new DashboardHost field.
The new field is properly configured with environment variable binding, but its usage in the codebase should be verified.
Run the following script to check where this field is being used:
🏁 Script executed:
#!/bin/bash # Description: Search for usage of DashboardHost field. # Search for direct field access rg -A 5 'DashboardHost' # Search for environment variable usage rg -A 5 'REEARTH_DASHBOARD_HOST'Length of output: 354
ACTION: Verify DashboardHost Usage Manually
The updated
DashboardHostfield is correctly bound to theREEARTH_DASHBOARD_HOSTenvironment variable. However, our initial automated search did not yield any matches—likely because ripgrep applied unexpected file filters. Please verify that this field is actually being used in the codebase. You may consider running a more comprehensive search that includes hidden files and ignores default filtering. For example, try running:#!/bin/bash # Search for DashboardHost usages with comprehensive search options. rg --hidden --no-ignore -A 5 'DashboardHost' . rg --hidden --no-ignore -A 5 'REEARTH_DASHBOARD_HOST' .If the re-run still does not return any results, please perform a manual verification to ensure that the
DashboardHostfield is integrated wherever required.server/api/internal/usecase/interactor/parameter_test.go (1)
46-48: LGTM! Permission checking is well integrated.The test setup has been successfully updated to use the centralized permission checker, with proper integration into the Parameter interactor.
Also applies to: 50-50
server/api/e2e/gql_user_test.go (1)
104-104:❓ Verification inconclusive
Verify the purpose of the new boolean parameter in StartGQLServer.
The new boolean parameter has been added to all
StartGQLServercalls, but its purpose is not immediately clear from the context. This parameter appears to be related to permission checking based on the PR objectives.Let's check the implementation of
StartGQLServer:Also applies to: 130-130, 160-160, 189-189, 229-229, 252-252
🏁 Script executed:
#!/bin/bash # Search for the StartGQLServer function definition ast-grep --pattern 'func StartGQLServer($_, $_, $_, $_, $_) { $$$ }'Length of output: 72
Let's re-run our verification using a more general search tool since the initial AST search returned no output. We need to locate the definition and usages of
StartGQLServerto confirm whether the new boolean parameter is indeed meant for permission checking.Please run the following shell scripts to further verify:
Once these commands are executed and you review the results, you should be able to confirm:
- The complete signature of
StartGQLServer- How the boolean parameter is used across its different invocations
Based on that, you can update the review comment accordingly. If the parameter indeed governs permission checking as suggested by the PR objectives, please document its role clearly in the code comments or related documentation.
Action: Clarify the purpose of the new boolean parameter in
StartGQLServerby verifying its definition and usage as described above.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for the function definition of StartGQLServer using ripgrep rg "func StartGQLServer" -n .Length of output: 153
🏁 Script executed:
[run_scripts] #!/bin/bash # List all calls to StartGQLServer to see the context and parameter values rg "StartGQLServer\(" -n .Length of output: 150
Action Required: Manual Verification Needed for Boolean Parameter
It appears that our automated searches for the definition and call sites of
StartGQLServerdid not yield any output. This makes it difficult to determine how the new boolean parameter is being used or whether it’s intended for permission checking as the PR objectives suggest.Next Steps:
- Manually verify the implementation of
StartGQLServer: Please verify in your codebase whereStartGQLServeris defined and how the new boolean parameter influences its behavior.- Clarify the parameter’s purpose: If it indeed governs permission checking, consider adding a detailed comment in the function’s documentation to explain its role and impact.
server/api/e2e/gql_project_test.go (1)
21-21: Test coverage looks comprehensive!The test suite thoroughly covers project operations including:
- Project creation with proper workspace context
- Update operations with various fields
- Deletion verification
- Pagination and sorting functionality
- Error cases with invalid inputs
server/api/e2e/gql_trigger_test.go (1)
21-21: Well-structured test cases for trigger operations!The test suite effectively covers:
- Time-driven trigger creation and validation
- API-driven trigger creation with auth token verification
- Trigger update operations
Also applies to: 197-197, 307-307
server/api/e2e/gql_deployment_pagination_test.go (1)
23-23: Excellent implementation of pagination tests!The test suite provides thorough coverage of deployment pagination including:
- Basic pagination with page size limits
- Sorting by different fields (description, creation time)
- Edge cases (last page, invalid page numbers)
- Proper error handling
server/api/e2e/gql_pagination_test.go (1)
22-22:❓ Verification inconclusive
Verify the purpose of the new boolean parameter.
A new boolean parameter has been added to all
StartGQLServercalls. Please clarify the purpose of this parameter and consider adding a descriptive variable name to improve code readability.Also applies to: 265-265, 666-666
🏁 Script executed:
#!/bin/bash # Description: Find the definition of StartGQLServer to understand the new parameter. ast-grep --pattern $'func StartGQLServer($_, $_, $_, $_, $_) { $$$ }'Length of output: 72
Action Required: Clarify the Role of the New Boolean Parameter
The automated lookup for the
StartGQLServerdefinition didn’t yield results, so we couldn’t automatically determine the parameter’s purpose. Given that every call (including those at lines 22, 265, and 666 inserver/api/e2e/gql_pagination_test.go) now includes this extra boolean value, please ensure that its intent is unambiguously communicated. Consider one or both of the following improvements:
- Rename the parameter: Replace the literal
truewith a descriptively named constant or variable that clarifies its purpose.- Add inline documentation: Include a comment at the function definition (and possibly at call sites) explaining what the boolean controls.
Once the exact functionality of this parameter is verified and its usage is clearly documented, it will improve readability and maintainability.
server/api/Makefile (1)
22-24: LGTM!The new
gen-policiestarget is well-integrated with the existing Makefile structure and correctly marked as.PHONY. The command is clear and follows the project's conventions.Also applies to: 25-25
.github/workflows/ci_policies.yml (1)
31-31: LGTM! Good use of shell options.The use of
set -eo pipefailensures proper error handling in the shell scripts.Also applies to: 53-53
.github/workflows/update_policies.yml (6)
1-7: Initial Workflow Configuration:
The workflow is clearly defined with the correct trigger (workflow_call) and sets the environment variablesGO_VERSIONandGCS_BUCKET_PATH. These hard-coded values work well for now, but consider parameterizing them in the future if flexibility is needed.
8-11: Job Definition:
Theupdate-policiesjob is properly set to run onubuntu-latestand is well-structured. The overall job configuration follows best practices.
12-18: Checkout and Go Setup:
The steps to check out the repository (usingactions/checkout@v3) and set up the Go environment (usingactions/setup-go@v4with caching explicitly disabled) are implemented correctly. The explicit setting ofcache: falseensures a clean setup, which appears intentional.
19-26: Generate Policies Step:
The “Generate policies” step uses a shell script that callsmake gen-policies, and it correctly handles errors with an explicit exit if policy generation fails. Also, theworking-directory: server/apisetting ensures the command runs in the proper context.
27-33: Authentication and Cloud SDK Setup:
The workflow’s steps for authenticating to Google Cloud usinggoogle-github-actions/auth@v1with the service account key and setting up the Cloud SDK (google-github-actions/setup-gcloud@v1) are correctly configured.
34-71: Sync Policies with Cloud Storage:
This step thoughtfully handles the synchronization of local policy files with the bucket. It:
- Uses robust shell options (
set -euo pipefail) to enforce strict error handling.- Lists bucket and local files before processing.
- Iterates over local files to upload/update them.
- Iterates over bucket files to delete any that are not present locally.
If future enhancements are planned, consider refactoring the upload and deletion loops into a helper script for maintainability. For now, this implementation fulfills its purpose.
.github/workflows/ci.yml (7)
6-12: Manual Trigger Input Parameter:
The introduction of theworkflow_dispatchsection with theforce_update_policiesboolean input is a valuable addition. It allows manual triggering of the policy update workflow, providing flexibility for emergency or targeted updates. Ensure that any related documentation is updated accordingly.
14-20: Prepare Job Output Enhancement:
Including an output forpoliciesin thepreparejob is a smart move. This output allows downstream jobs to conditionally run based on changes in policy files, which enhances the CI pipeline’s responsiveness.
41-50: Changed Files for Policies Configuration:
The configuration for detecting changes in policy-related files is clearly defined via thechanged-filesaction. The paths listed (e.g.,server/api/internal/rbac/**and relevant workflow files) seem comprehensive. Just double-check that these paths cover all policy file locations in your repository.
101-104: New Job: ci-policies:
The addition of theci-policiesjob, which is triggered based on thepoliciesoutput from thepreparejob, is well executed. This separation enables focused validation of policy changes.
110-112: Job Indentation and Consistency:
Minor adjustments for theci-websocketjob’s indentation have improved the YAML’s readability and consistency. This kind of attention to detail is appreciated.
169-183: Update-Policies Job Integration:
Theupdate-policiesjob is integrated with a robust conditional check. The condition:
- Ensures the job runs only when there are no failures.
- Checks that the
ci-policiesjob succeeded.- Only triggers on push events to
main,release, or when the manual input (force_update_policies) is true.This logic is well thought out and should cover the intended scenarios. It’s worth double-checking these conditions periodically as the repository evolves.
184-239: Subsequent Build and Deploy Jobs:
The downstream jobs (build-and-deploy-api,build-and-deploy-ui,build-and-deploy-websocket, andbuild-docker-push-worker) correctly depend on the outputs from earlier steps and inherit secrets properly. The flow and dependency chain appear coherent and maintain a robust CI/CD process.
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service-2
branch
from
18888d9 to
25bad11
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
server/api/internal/usecase/interactor/deployment.go (1)
55-359: 🛠️ Refactor suggestionConsider using more granular permission actions.
Currently, all operations use
rbac.ActionAnyfor permission checks. Consider using more specific actions to implement the principle of least privilege:
- Read operations (Fetch, Find*): Use
rbac.ActionRead- Write operations (Create, Update): Use
rbac.ActionWrite- Delete operation: Use
rbac.ActionDelete- Execute operation: Use
rbac.ActionExecuteExample for the Fetch method:
-func (i *Deployment) Fetch(ctx context.Context, ids []id.DeploymentID) ([]*deployment.Deployment, error) { - if err := i.checkPermission(ctx, rbac.ActionAny); err != nil { +func (i *Deployment) Fetch(ctx context.Context, ids []id.DeploymentID) ([]*deployment.Deployment, error) { + if err := i.checkPermission(ctx, rbac.ActionRead); err != nil {Apply similar changes to other methods using appropriate action constants.
🧹 Nitpick comments (16)
server/api/internal/usecase/interactor/parameter_test.go (1)
22-22: Consider testing permission denial scenarios.While the mock permission checker implementation is clean, always returning
truemight miss important edge cases where permissions are denied.Consider adding test cases with a permission checker that returns
falseto verify proper error handling:func setupParameterInteractor() (interfaces.Parameter, context.Context, *repo.Container, id.ProjectID) { // ... existing setup code ... + return setupParameterInteractorWithPermissions(true) } + +func setupParameterInteractorWithPermissions(allowed bool) (interfaces.Parameter, context.Context, *repo.Container, id.ProjectID) { + // ... copy of existing setup code ... + mockPermissionChecker := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { + return allowed, nil + }) + i := NewParameter(r, mockPermissionChecker) + return i, ctx, r, pid +}Also applies to: 46-48
server/api/internal/usecase/interactor/project_test.go (1)
48-108: Consider adding more edge cases to the test suite.The table-driven tests are well-structured and cover the main scenarios effectively. Consider adding:
- Edge cases for empty/invalid project names and descriptions
- Verification that the permission checker is called with correct resource/action parameters
Example test case to add:
tests := []struct { name string param interfaces.CreateProjectParam permission *mockPermissionChecker wantErr error }{ + { + name: "empty project name", + param: interfaces.CreateProjectParam{ + WorkspaceID: ws.ID(), + Name: lo.ToPtr(""), + Description: lo.ToPtr("desc"), + }, + wantErr: errors.New("invalid project name"), + },server/api/internal/usecase/interactor/usecase.go (2)
74-77: Consider enhancing error context.The error from the inner function is returned directly without additional context. Consider wrapping errors with additional context to aid in debugging and error tracking.
err = usecasex.DoTransaction(ctx, t, retry, func(ctx context.Context) error { a, b, c, err = f(ctx) - return err + if err != nil { + return fmt.Errorf("failed to execute use case: %w", err) + } + return nil })
13-17: Add documentation for workspace access fields.The
ucstruct contains fields for managing workspace access, but their purpose and usage are not documented. Consider adding documentation to clarify their role in the new permission management system.type uc struct { + // tx indicates whether the use case should run within a transaction tx bool + // readableWorkspaces contains the list of workspace IDs that the current user can read readableWorkspaces accountdomain.WorkspaceIDList + // writableWorkspaces contains the list of workspace IDs that the current user can modify writableWorkspaces accountdomain.WorkspaceIDList }server/api/internal/usecase/operator.go (1)
7-8: LGTM! Consider creating tracking issues for the dependencies.The TODO comment clearly indicates the plan to delete this file. However, there are three dependencies that need to be addressed:
- Migration to Cerbos
- Modification of reearthx interfaces
- Modification of reearth-dashboard interfaces
Would you like me to help create tracking issues for these dependencies to ensure they're not forgotten?
server/api/internal/usecase/interactor/common.go (1)
73-83: Consider enhancing error context.While the error handling is good, consider providing more context in the error message by including the resource and action that were denied.
- return fmt.Errorf("failed to check permission: %w", err) + return fmt.Errorf("failed to check permission for %s/%s: %w", resource, action, err)server/api/internal/usecase/interactor/asset.go (1)
31-33: Consider adding resource-specific error context.The checkPermission helper method could be enhanced to provide asset-specific context in error messages.
func (i *Asset) checkPermission(ctx context.Context, action string) error { - return checkPermission(ctx, i.permissionChecker, rbac.ResourceAsset, action) + err := checkPermission(ctx, i.permissionChecker, rbac.ResourceAsset, action) + if err != nil { + return fmt.Errorf("asset permission check failed: %w", err) + } + return nil }server/api/internal/usecase/interactor/projectAccess.go (1)
37-39: Consider adding project-specific error context.The checkPermission helper method could be enhanced to provide project-specific context in error messages.
func (i *ProjectAccess) checkPermission(ctx context.Context, action string) error { - return checkPermission(ctx, i.permissionChecker, rbac.ResourceProjectAccess, action) + err := checkPermission(ctx, i.permissionChecker, rbac.ResourceProjectAccess, action) + if err != nil { + return fmt.Errorf("project access permission check failed: %w", err) + } + return nil }server/api/internal/usecase/interactor/project.go (4)
63-110: Consider improving transaction handling.While the permission check is correctly placed, consider moving it inside the transaction to ensure atomicity. This would prevent a scenario where permissions change between the check and the actual operation.
Apply this diff to improve transaction handling:
func (i *Project) Create(ctx context.Context, p interfaces.CreateProjectParam) (_ *project.Project, err error) { - if err := i.checkPermission(ctx, rbac.ActionCreate); err != nil { - return nil, err - } - tx, err := i.transaction.Begin(ctx) if err != nil { return } ctx = tx.Context() defer func() { if err2 := tx.End(ctx); err == nil && err2 != nil { err = err2 } }() + if err := i.checkPermission(ctx, rbac.ActionCreate); err != nil { + return nil, err + } +
112-164: Consider improving transaction handling.Similar to the
Createmethod, consider moving the permission check inside the transaction to ensure atomicity.Apply this diff to improve transaction handling:
func (i *Project) Update(ctx context.Context, p interfaces.UpdateProjectParam) (_ *project.Project, err error) { - if err := i.checkPermission(ctx, rbac.ActionEdit); err != nil { - return nil, err - } - tx, err := i.transaction.Begin(ctx) if err != nil { return } ctx = tx.Context() defer func() { if err2 := tx.End(ctx); err == nil && err2 != nil { err = err2 } }() + if err := i.checkPermission(ctx, rbac.ActionEdit); err != nil { + return nil, err + } +
166-198: Consider improving transaction handling.Similar to the
CreateandUpdatemethods, consider moving the permission check inside the transaction to ensure atomicity.Apply this diff to improve transaction handling:
func (i *Project) Delete(ctx context.Context, projectID id.ProjectID) (err error) { - if err := i.checkPermission(ctx, rbac.ActionDelete); err != nil { - return err - } - tx, err := i.transaction.Begin(ctx) if err != nil { return } ctx = tx.Context() defer func() { if err2 := tx.End(ctx); err == nil && err2 != nil { err = err2 } }() + if err := i.checkPermission(ctx, rbac.ActionDelete); err != nil { + return err + } +
200-236: Consider improving transaction handling.Similar to other methods, consider moving the permission check inside the transaction to ensure atomicity.
Apply this diff to improve transaction handling:
func (i *Project) Run(ctx context.Context, p interfaces.RunProjectParam) (started bool, err error) { - if err := i.checkPermission(ctx, rbac.ActionEdit); err != nil { - return false, err - } - 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 } }() + if err := i.checkPermission(ctx, rbac.ActionEdit); err != nil { + return false, err + } +server/api/internal/rbac/definitions.go (1)
12-22: Consider using iota for resource type enumeration.While string constants are explicit, using iota with String() method would provide type safety and prevent accidental string assignments.
Example implementation:
type ResourceType int const ( ResourceAsset ResourceType = iota ResourceDeployment ResourceJob // ... other resources ) func (r ResourceType) String() string { return [...]string{ "asset", "deployment", "job", // ... other resource strings }[r] }.github/workflows/ci_policies.yml (1)
29-44: Add error handling for policy file listing.The policy file listing step could be more robust by:
- Using
findinstead oflsfor better error handling- Adding a timeout for the file operations
- ls -la policies/ || echo "No files found" + timeout 30s find policies/ -type f -ls || echo "No files found" - if [ -d "policies" ] && [ "$(ls -A policies/)" ]; then + if [ -d "policies" ] && [ "$(find policies/ -type f)" ]; then.github/workflows/ci.yml (2)
6-11: Add more descriptive input documentation.The
force_update_policiesinput could benefit from more detailed documentation:force_update_policies: - description: 'Force update policies' + description: 'Force update of Cerbos policies in GCS bucket, regardless of changes detected' type: boolean default: false + required: false
169-182: Consider adding timeout for policy updates.The
update-policiesjob should have a timeout to prevent hanging:update-policies: + timeout-minutes: 10 needs: - ci - ci-policies - ci-collect-info
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.work.sumis excluded by!**/*.sumserver/api/README.mdis excluded by!**/*.mdserver/api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (69)
.github/workflows/ci.yml(6 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)server/api/.env.example(1 hunks)server/api/Makefile(1 hunks)server/api/cmd/policy-generator/main.go(1 hunks)server/api/e2e/common.go(6 hunks)server/api/e2e/gql_deployment_pagination_test.go(1 hunks)server/api/e2e/gql_me_test.go(1 hunks)server/api/e2e/gql_pagination_test.go(3 hunks)server/api/e2e/gql_param_test.go(1 hunks)server/api/e2e/gql_projectAccess_test.go(1 hunks)server/api/e2e/gql_project_test.go(2 hunks)server/api/e2e/gql_trigger_test.go(3 hunks)server/api/e2e/gql_user_test.go(6 hunks)server/api/e2e/gql_workspace_test.go(6 hunks)server/api/e2e/ping_test.go(1 hunks)server/api/go.mod(2 hunks)server/api/internal/adapter/context.go(0 hunks)server/api/internal/adapter/gql/context.go(0 hunks)server/api/internal/adapter/gql/loader_asset.go(2 hunks)server/api/internal/adapter/gql/loader_deployment.go(6 hunks)server/api/internal/adapter/gql/loader_job.go(3 hunks)server/api/internal/adapter/gql/loader_project.go(2 hunks)server/api/internal/adapter/gql/loader_trigger.go(2 hunks)server/api/internal/adapter/gql/loader_user.go(1 hunks)server/api/internal/adapter/gql/resolver_mutation_asset.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_deployment.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_parameter.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_project.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_projectAccess.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_trigger.go(3 hunks)server/api/internal/adapter/gql/resolver_project.go(1 hunks)server/api/internal/adapter/gql/resolver_subscription.go(1 hunks)server/api/internal/app/app.go(1 hunks)server/api/internal/app/auth_client.go(0 hunks)server/api/internal/app/config/config.go(1 hunks)server/api/internal/app/main.go(3 hunks)server/api/internal/app/repo.go(2 hunks)server/api/internal/app/trigger.go(1 hunks)server/api/internal/app/usecase.go(1 hunks)server/api/internal/rbac/definitions.go(1 hunks)server/api/internal/usecase/gateway/mock_permission.go(1 hunks)server/api/internal/usecase/gateway/permission.go(1 hunks)server/api/internal/usecase/interactor/asset.go(4 hunks)server/api/internal/usecase/interactor/asset_test.go(3 hunks)server/api/internal/usecase/interactor/common.go(4 hunks)server/api/internal/usecase/interactor/deployment.go(7 hunks)server/api/internal/usecase/interactor/job.go(8 hunks)server/api/internal/usecase/interactor/parameter.go(5 hunks)server/api/internal/usecase/interactor/parameter_test.go(6 hunks)server/api/internal/usecase/interactor/project.go(5 hunks)server/api/internal/usecase/interactor/projectAccess.go(4 hunks)server/api/internal/usecase/interactor/projectAccess_test.go(5 hunks)server/api/internal/usecase/interactor/project_test.go(1 hunks)server/api/internal/usecase/interactor/trigger.go(6 hunks)server/api/internal/usecase/interactor/trigger_test.go(9 hunks)server/api/internal/usecase/interactor/usecase.go(1 hunks)server/api/internal/usecase/interactor/usecase_test.go(5 hunks)server/api/internal/usecase/interactor/utils_test.go(1 hunks)server/api/internal/usecase/interfaces/asset.go(1 hunks)server/api/internal/usecase/interfaces/deployment.go(1 hunks)server/api/internal/usecase/interfaces/job.go(1 hunks)server/api/internal/usecase/interfaces/parameter.go(1 hunks)server/api/internal/usecase/interfaces/project.go(1 hunks)server/api/internal/usecase/interfaces/projectAccess.go(1 hunks)server/api/internal/usecase/interfaces/trigger.go(1 hunks)server/api/internal/usecase/operator.go(1 hunks)server/api/internal/usecase/repo/container.go(0 hunks)
💤 Files with no reviewable changes (4)
- server/api/internal/usecase/repo/container.go
- server/api/internal/adapter/context.go
- server/api/internal/adapter/gql/context.go
- server/api/internal/app/auth_client.go
🚧 Files skipped from review as they are similar to previous changes (45)
- server/api/internal/usecase/gateway/permission.go
- server/api/cmd/policy-generator/main.go
- server/api/internal/app/repo.go
- server/api/internal/adapter/gql/resolver_project.go
- server/api/internal/usecase/gateway/mock_permission.go
- server/api/internal/app/config/config.go
- server/api/e2e/gql_me_test.go
- server/api/internal/app/trigger.go
- server/api/.env.example
- server/api/internal/app/app.go
- server/api/e2e/gql_param_test.go
- server/api/e2e/ping_test.go
- server/api/internal/adapter/gql/resolver_mutation_deployment.go
- server/api/Makefile
- server/api/internal/adapter/gql/resolver_subscription.go
- server/api/go.mod
- server/api/internal/usecase/interactor/utils_test.go
- server/api/internal/usecase/interactor/asset_test.go
- server/api/internal/adapter/gql/resolver_mutation_project.go
- server/api/e2e/gql_trigger_test.go
- server/api/internal/adapter/gql/loader_user.go
- server/api/e2e/gql_deployment_pagination_test.go
- server/api/e2e/gql_project_test.go
- server/api/e2e/gql_projectAccess_test.go
- server/api/internal/adapter/gql/resolver_mutation_asset.go
- server/api/e2e/gql_pagination_test.go
- server/api/internal/adapter/gql/resolver_mutation_projectAccess.go
- server/api/e2e/gql_user_test.go
- server/api/internal/adapter/gql/loader_trigger.go
- server/api/e2e/gql_workspace_test.go
- server/api/internal/adapter/gql/resolver_mutation_trigger.go
- server/api/internal/adapter/gql/loader_project.go
- server/api/internal/usecase/interfaces/projectAccess.go
- server/api/internal/adapter/gql/loader_asset.go
- server/api/internal/adapter/gql/loader_job.go
- server/api/internal/usecase/interfaces/project.go
- server/api/e2e/common.go
- server/api/internal/usecase/interactor/trigger_test.go
- server/api/internal/usecase/interfaces/asset.go
- server/api/internal/adapter/gql/resolver_mutation_parameter.go
- server/api/internal/usecase/interfaces/job.go
- server/api/internal/adapter/gql/loader_deployment.go
- server/api/internal/usecase/interfaces/parameter.go
- server/api/internal/usecase/interactor/parameter.go
- server/api/internal/usecase/interfaces/deployment.go
🧰 Additional context used
🧠 Learnings (2)
server/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`.
server/api/internal/usecase/interactor/deployment.go (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: api/internal/usecase/interactor/deployment.go:48-70
Timestamp: 2025-01-21T10:53:31.572Z
Learning: The Fetch and Find methods in the deployment service require permission checks using the new permission checker mechanism after migrating away from operator-based checks.
🪛 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)
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)
.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 (30)
server/api/internal/app/usecase.go (2)
15-15: LGTM! Good improvement to the function signature.The addition of the
permissionCheckerparameter aligns well with the centralized permission handling approach. This change makes the authorization dependencies explicit and follows good dependency injection practices.
19-19: LGTM! Clean implementation of the permission checker integration.The simplified implementation correctly integrates the permission checker while maintaining the middleware's core responsibility. The removal of operator-based filtering in favor of centralized permission handling is a good architectural improvement.
server/api/internal/usecase/interactor/parameter_test.go (2)
16-16: LGTM! Import added for authorization support.The addition of the
appxpackage import is appropriate for the new permission checking functionality.
54-261: LGTM! Test coverage remains comprehensive.The test functions have been consistently updated to remove the operator parameter while maintaining thorough test coverage of the parameter functionality. The test cases continue to verify:
- Parameter declaration
- Error handling for non-existent projects
- Parameter fetching
- Parameter removal
- Parameter order updates
- Parameter value updates
server/api/internal/usecase/interactor/project_test.go (3)
5-5: LGTM! Import changes align with authorization refactoring.The addition of the
errorspackage and switch toappxsupport the new permission checking functionality.Also applies to: 13-13
20-29: Well-structured test helper function!Good practices observed:
- Marks as test helper
- Encapsulates common setup logic
- Enables dependency injection for permission testing
34-43: Clean and focused test setup!The mock permission checker and workspace setup provide a solid foundation for testing different authorization scenarios.
server/api/internal/usecase/interactor/usecase.go (1)
38-46:✅ Verification successful
Verify permission checks in individual interactors.
The removal of operator-based permission checks from these utility functions suggests that permission checking has been moved to individual interactors. While this aligns with the PR's goal of centralizing permission management, we should ensure that all interactors properly implement the new
checkPermissionfunction.Let's verify the implementation of permission checks in the interactors:
Also applies to: 48-56, 58-66, 68-79
🏁 Script executed:
#!/bin/bash # Search for checkPermission implementations in interactors echo "Searching for checkPermission implementations..." ast-grep --pattern 'func (i *$_) checkPermission($$$) error { $$$ }' # Search for any potential permission-related function calls echo "Searching for permission-related function calls..." rg -i "permission|authorize|access" --type goLength of output: 48733
Action: Permission Checks Implemented Correctly; Documentation and Error Handling Review Suggested
The removal of the operator parameter and permission-checking logic from the Run utility functions is in line with the PR objectives. Our verification confirms that each interactor (asset, trigger, deployment, job, parameter, project, and projectAccess) implements its own
checkPermissionfunction properly. Error handling in these functions follows the established pattern, and while minimal, it is consistent with the overall design.
- Verify that all interactors continue to call
if err := i.checkPermission(ctx, <action>); err != nil { … }where needed.- Ensure that documentation for permission management is updated to reflect the centralized approach.
- Consider whether additional error context should be added if complex failures occur.
No further action is required on the utility function itself, but please review the above points to maintain clarity and robustness across the codebase.
server/api/internal/usecase/interactor/usecase_test.go (1)
31-31:❓ Verification inconclusive
LGTM! Verify permission check test coverage.
The removal of the nil operator parameter from Run3 and Run0 function calls aligns with the migration away from operator-based permission checks. However, since TestUc_checkPermission has been removed, ensure that permission checks are adequately tested in the new Cerbos-based implementation.
Run the following script to verify test coverage for permission checks:
Also applies to: 47-47, 59-59, 74-74, 89-89
🏁 Script executed:
#!/bin/bash # Description: Check test coverage for permission-related functionality # Search for permission-related test files echo "Permission-related test files:" fd -e go -e test permission # Search for Cerbos-related test files echo -e "\nCerbos-related test files:" fd -e go -e test cerbos # Search for permission check test cases echo -e "\nPermission check test cases:" rg -A 5 "Test.*Permission|permission.*Test"Length of output: 419
ACTION REQUIRED: Confirm Comprehensive Testing for New Cerbos-based Permission Checks
The removal of the nil operator parameter from the Run3 and Run0 calls correctly reflects the migration away from operator-based permission checks. However, since the previous
TestUc_checkPermissionhas been removed, no explicit tests for permission checking were found (e.g., no "Test…Permission" cases or Cerbos-specific tests via our search).Please manually verify that:
- The new Cerbos-based permission check logic is covered by tests, either integrated within existing test suites or through new test cases.
- Files such as
server/api/internal/usecase/gateway/mock_permission.goandserver/api/internal/usecase/gateway/permission.goare appropriately exercised by tests for permission scenarios.Additionally, reconfirm test coverage at the corresponding locations (lines 31, 47, 59, 74, and 89) to ensure no regressions in permission handling.
server/api/internal/app/main.go (4)
7-7: LGTM! New imports for permission checking.The added imports support the new permission checking functionality using Cerbos.
Also applies to: 13-13, 18-18
80-88: LGTM! ServerConfig struct updated with permission checker.The addition of the
PermissionCheckerfield is well-typed and aligns with the PR's goal of implementing permission checking.
64-72: LGTM! Server initialization updated with permission checker.The permission checker is correctly passed to the server configuration.
55-57:❓ Verification inconclusive
Verify secure URL handling.
While the code validates the URL format, ensure that:
- The DashboardHost URL uses HTTPS in production
- The URL doesn't contain sensitive information in plaintext
- The connection to the Cerbos service is secure
Run this script to check the URL scheme in different environments:
🏁 Script executed:
#!/bin/bash # Description: Check DashboardHost URL scheme in configuration files # Check .env files echo "Checking .env files for DashboardHost URLs:" rg -l "REEARTH_DASHBOARD_HOST" | while read -r file; do echo "File: $file" rg "REEARTH_DASHBOARD_HOST.*=" "$file" done # Check deployment manifests echo -e "\nChecking deployment manifests for DashboardHost URLs:" fd -e yaml -e yml | xargs rg "dashboardHost|DASHBOARD_HOST"Length of output: 705
Secure DashboardHost URL Handling
While the code correctly validates the URL format, please ensure the following:
- In production, enforce the use of HTTPS for DashboardHost. The
.envexample currently showshttp://localhost:8090, which is acceptable for local development only.- Verify that the DashboardHost URL does not inadvertently contain sensitive information in plaintext.
- Confirm that the connection to the Cerbos service (or other downstream services) is secured by appropriate transport layer protocols.
Please double-check these settings, especially in production, to maintain the desired security posture.
server/api/internal/usecase/interfaces/trigger.go (1)
45-53: LGTM! Clean interface refactoring.The removal of operator parameters from method signatures aligns with the centralized permission checking approach, making the interface cleaner and more focused.
server/api/internal/usecase/interactor/common.go (2)
20-20: LGTM! Clear error definition.The error variable is well-defined with a descriptive message.
29-47: LGTM! Consistent dependency injection.The permissionChecker is correctly passed to all components, following the retrieved learning from PR #580.
server/api/internal/usecase/interactor/asset.go (2)
17-21: LGTM! Clean struct definition.The Asset struct is well-organized with clear field names and types.
35-41: LGTM! Consistent permission checks.All methods consistently check permissions before proceeding with their operations, with appropriate early returns on permission failures.
Also applies to: 43-59, 61-91, 93-116
server/api/internal/usecase/interactor/projectAccess.go (2)
19-25: LGTM! Well-structured struct definition.The ProjectAccess struct is well-organized with clear field names and types.
41-152: LGTM! Robust implementation.The methods maintain proper transaction handling while adding consistent permission checks. The error handling and business logic remain intact.
server/api/internal/usecase/interactor/project.go (3)
19-28: LGTM!The addition of the
permissionCheckerfield to theProjectstruct is well-structured and follows field ordering conventions.
30-41: LGTM!The
NewProjectfunction has been correctly updated to include and initialize thepermissionCheckerparameter.
43-45: LGTM!The
checkPermissionmethod is well-implemented, following the single responsibility principle and maintaining consistency with other interactors.server/api/internal/usecase/interactor/projectAccess_test.go (1)
21-23: LGTM!The mock permission checker is well-implemented and consistently used across test cases. The test cases have been appropriately updated to reflect the removal of the operator parameter.
Also applies to: 104-106, 173-175
server/api/internal/usecase/interactor/job.go (1)
23-32: LGTM!The changes to the
Jobstruct and its methods follow the same consistent pattern of centralized permission checking seen in other interactors.Also applies to: 42-53, 56-58
server/api/internal/usecase/interactor/deployment.go (3)
23-49: LGTM! Well-structured initialization of the permission checker.The addition of the
permissionCheckerfield and its initialization in the constructor is clean and follows good practices.
51-53: LGTM! Clean implementation of the permission check helper.The helper method provides a consistent way to check permissions and promotes code reuse.
354-354:❓ Verification inconclusive
Verify job monitoring implementation.
The
StartMonitoringcall uses a nil operator. Verify if the job monitoring system has been updated to use the permission checker instead of the operator-based checks.
🏁 Script executed:
#!/bin/bash # Search for StartMonitoring implementation to verify permission checking ast-grep --pattern 'func $_(ctx context.Context, job *job.Job, operator *usecase.Operator) error { $$$ }'Length of output: 109
Attention: Manual Verification of Permission Handling in Job Monitoring
The
StartMonitoringcall inserver/api/internal/usecase/interactor/deployment.go(line 354) currently passes a nil operator parameter. This raises concerns that the job monitoring system might not be leveraging the new permission checker as expected, which could be an oversight in the migration process.Please manually verify whether:
- The implementation of
StartMonitoring(likely in the job package) intentionally supports a nil operator to default correct behavior, or- It should now receive a non-nil permission checker for proper permission validation.
If the latter is correct, update the call accordingly. Otherwise, document and confirm that nil is acceptable in this context.
server/api/internal/rbac/definitions.go (2)
7-10: LGTM! Clear and well-defined service constants.The service name and policy directory constants are appropriately defined at the package level.
41-135: Verify resource definitions against business requirements.The resource definitions look well-structured, but there are a few concerns:
- The
ActionAnypermission might be too broad for some resources- Some resources like
ResourceAssetonly haveActionAnydefined- The role hierarchy isn't explicitly defined
Could you confirm if:
- The broad
ActionAnypermissions align with your security requirements?- Resources with only
ActionAnyshouldn't have more granular permissions?- The role hierarchy (reader → editor → maintainer → owner) is correctly represented?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/update_policies.yml (3)
1-7: Environment Initialization & Configuration
The workflow’s name and environment variables (GO_VERSION and GCS_BUCKET_PATH) are clearly defined. Consider whether critical values likeGCS_BUCKET_PATHshould be parameterized (for example, as a workflow input) to increase flexibility across environments.
19-26: Policy Generation Step
The "Generate policies" step executesmake gen-policieswith error handling that immediately exits if the command fails. This is effective; consider logging additional context (if available) on failure for easier debugging, especially if the policy generation process becomes more complex in the future.
33-65: Synchronizing Policies with Cloud Storage
The sync step loops over local policy files to upload them and then iterates over bucket files to delete orphans. The use ofset -euo pipefailand appropriate error handling ensures that failures are caught early. One suggestion: consider adding a retry mechanism around criticalgsutilcommands (such asgsutil cpandgsutil rm) to better handle transient network or service interruptions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yml(6 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci_policies.yml
🔇 Additional comments (11)
.github/workflows/update_policies.yml (2)
12-18: Checkout & Go Setup Steps
The checkout and Go setup steps properly useactions/checkout@v4andactions/setup-go@v5, respectively. This update confirms alignment with the latest recommended versions.
27-32: Cloud Authentication & SDK Setup
The steps to authenticate to Google Cloud and set up the Cloud SDK use the proper actions (google-github-actions/auth@v2andgoogle-github-actions/setup-gcloud@v2). These configurations are correctly implemented to support subsequent GCS operations..github/workflows/ci.yml (9)
6-12: Introducing Manual Trigger Input
The addition of theworkflow_dispatchsection with theforce_update_policiesboolean input is a useful enhancement. This input provides a manual override for policy updates, increasing the workflow’s flexibility.
14-21: Preparing Job Outputs
The inclusion of thepoliciesoutput in thepreparejob (line 20) alongside other module outputs ensures that subsequent jobs can conditionally run based on detected changes. This addition is well integrated with the existing changed-files steps.
28-40: Changed Files for API
The file patterns listed for detecting API changes (includingserver/api/**,engine/schema/**, and several workflow files) appear comprehensive. Ensure that the inclusion of.github/workflows/ci.ymlin this list serves the intended purpose and does not trigger unnecessary job runs.
41-51: Changed Files for Policies
The patterns under the policies section encompass the RBAC directory and relevant YAML files for policy checks. This configuration is aligned with the new policy management focus. It might be beneficial to verify that these patterns fully capture all modifications related to policy changes to avoid false negatives.
52-92: Changed Files for Subscriber, UI, WebSocket, and Engine
The additional steps to detect changes in subscriber, UI, websocket, and engine modules use consistent logic withtj-actions/changed-files@v45. Their file patterns are clearly defined and maintain consistency across the modules.
101-104: New ci-policies Job
The newly addedci-policiesjob is conditionally triggered based on changes detected in policies. This modular approach strengthens the CI process. Make sure that the referenced workflow (.github/workflows/ci_policies.yml) is adequately tested and maintained alongside these changes.
110-112: ci-websocket Indentation Correction
The adjustments made to theci-websocketjob (noted by the tilde markers) correct the indentation, ensuring consistency with the other job definitions.
169-182: update-policies Job Condition & Dependencies
Theupdate-policiesjob is a critical part of the workflow:
- It correctly lists dependencies on
ci,ci-policies, andci-collect-info.- The conditional expression checks that there are no failures, that the
ci-policiesjob succeeded, and that either the push event is on a valid branch or a manual override is specified.This logical structure is well thought out. Double-check that the condition correctly interprets
inputs.force_update_policiesin all scenarios (especially when manually triggering the workflow).
183-238: Build & Deployment Jobs
The subsequent jobs forbuild-and-deploy-api,build-and-deploy-ui,build-and-deploy-websocket, andbuild-docker-push-workercontinue to use the outputs from theci-collect-infojob and are conditionally executed based on push events and branch specifications. This coherent integration ensures that deployments are only triggered after a successful CI run.
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service-2
branch
from
c40e2ea to
72e5de1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
server/api/e2e/gql_projectAccess_test.go (1)
96-144: 🛠️ Refactor suggestion❓ Verification inconclusive
Add test cases for permission checking.
The test suite appears to be missing critical test cases for authorization scenarios:
- Attempting to share/unshare a project as an unauthorized user
- Attempting to share/unshare a project as a user without proper permissions
- Attempting to share/unshare a project in a workspace where the user has limited access
Let me know if you'd like me to generate the additional test cases to improve the test coverage for the new permission checking functionality.
🏁 Script executed:
#!/bin/bash # Description: Verify if the removed test case exists in git history # and check for other permission-related test cases # Check git history for removed test case git log -p -- server/api/e2e/gql_projectAccess_test.go | grep -A 10 "without permission" # Look for other permission-related test files fd -e go . | grep -i "permission.*test"Length of output: 41
Permission checks missing in end-to-end tests
The current tests in
server/api/e2e/gql_projectAccess_test.goeffectively validate the sharing flow under normal conditions but do not include negative test cases for authorization. In particular, it would be beneficial to add tests for:
- An unauthorized user (or one lacking the necessary permissions) attempting to share a project.
- An unauthorized user (or one lacking the necessary permissions) attempting to unshare a project.
- Access attempts to project data in cases where the user should not be granted access.
While the PR objectives mention a removed test case for unauthorized access, our search did not reveal any residual or alternative tests covering these scenarios. Please consider addressing these gaps to ensure comprehensive permission checking.
server/api/internal/usecase/interactor/deployment.go (2)
113-290: 🛠️ Refactor suggestionImplement granular permissions for CRUD operations
Currently, all CRUD operations use
rbac.ActionAny. Based on the retrieved learning about migrating from operator-based checks, consider implementing specific permissions for different operations.+const ( + ActionCreate = "create" + ActionRead = "read" + ActionUpdate = "update" + ActionDelete = "delete" +) -func (i *Deployment) Create(ctx context.Context, dp interfaces.CreateDeploymentParam) (result *deployment.Deployment, err error) { - if err := i.checkPermission(ctx, rbac.ActionAny); err != nil { +func (i *Deployment) Create(ctx context.Context, dp interfaces.CreateDeploymentParam) (result *deployment.Deployment, err error) { + if err := i.checkPermission(ctx, ActionCreate); err != nil { -func (i *Deployment) Update(ctx context.Context, dp interfaces.UpdateDeploymentParam) (_ *deployment.Deployment, err error) { - if err := i.checkPermission(ctx, rbac.ActionAny); err != nil { +func (i *Deployment) Update(ctx context.Context, dp interfaces.UpdateDeploymentParam) (_ *deployment.Deployment, err error) { + if err := i.checkPermission(ctx, ActionUpdate); err != nil { -func (i *Deployment) Delete(ctx context.Context, deploymentID id.DeploymentID) (err error) { - if err := i.checkPermission(ctx, rbac.ActionAny); err != nil { +func (i *Deployment) Delete(ctx context.Context, deploymentID id.DeploymentID) (err error) { + if err := i.checkPermission(ctx, ActionDelete); err != nil {
292-359: 🛠️ Refactor suggestionImplement specific permission for deployment execution
The Execute method should use a dedicated permission instead of
rbac.ActionAny.+const ActionExecute = "execute" -func (i *Deployment) Execute(ctx context.Context, p interfaces.ExecuteDeploymentParam) (_ *job.Job, err error) { - if err := i.checkPermission(ctx, rbac.ActionAny); err != nil { +func (i *Deployment) Execute(ctx context.Context, p interfaces.ExecuteDeploymentParam) (_ *job.Job, err error) { + if err := i.checkPermission(ctx, ActionExecute); err != nil {
🧹 Nitpick comments (8)
server/api/internal/usecase/interactor/parameter_test.go (1)
46-48: Add test cases for permission denial scenarios.The mock permission checker always returns
true, which doesn't test authorization failure cases. Consider parameterizing the mock to enable testing both successful and failed permission checks.-func setupParameterInteractor() (interfaces.Parameter, context.Context, *repo.Container, id.ProjectID) { +func setupParameterInteractor(allowPermissions bool) (interfaces.Parameter, context.Context, *repo.Container, id.ProjectID) { // ... existing setup code ... - mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { - return true, nil - }) + mockPermissionChecker := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { + return allowPermissions, nil + }) - i := NewParameter(r, mockPermissionCheckerTrue) + i := NewParameter(r, mockPermissionChecker) return i, ctx, r, pid }Then add test cases for permission denial:
func TestParameter_DeclareParameter_PermissionDenied(t *testing.T) { i, ctx, _, pid := setupParameterInteractor(false) p, err := i.DeclareParameter(ctx, interfaces.DeclareParameterParam{ ProjectID: pid, Name: "param1", Type: parameter.TypeText, Value: "test", }) assert.Nil(t, p) assert.ErrorIs(t, err, rerror.ErrPermissionDenied) }server/api/internal/usecase/interactor/project_test.go (1)
34-36: Consider adding more test cases for permission checker.While the current mock implementation is good, consider adding test cases for permission checker failures (e.g., when it returns an error instead of just false).
mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { return true, nil }) +mockPermissionCheckerError := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { + return false, errors.New("permission check failed") +})server/api/internal/usecase/interactor/projectAccess.go (2)
37-39: Consider enhancing error context in checkPermission.While the helper method is good, consider adding more context to the error message to help with debugging.
func (i *ProjectAccess) checkPermission(ctx context.Context, action string) error { - return checkPermission(ctx, i.permissionChecker, rbac.ResourceProjectAccess, action) + if err := checkPermission(ctx, i.permissionChecker, rbac.ResourceProjectAccess, action); err != nil { + return fmt.Errorf("project access permission check failed for action %s: %w", action, err) + } + return nil }
116-152: Consider adding metrics for permission denials.The permission check implementation is correct, but adding metrics would help monitor authorization patterns and potential issues.
func (i *ProjectAccess) Unshare(ctx context.Context, projectID id.ProjectID) (err error) { if err := i.checkPermission(ctx, rbac.ActionAny); err != nil { + // TODO: Add metrics for permission denials + // metrics.IncCounter("project_access.permission_denied", map[string]string{"action": "unshare"}) return err }server/api/internal/usecase/interactor/projectAccess_test.go (2)
21-23: Consider testing permission checker edge cases.While the basic permission check is tested, consider adding test cases for when the permission checker returns an error.
mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { return true, nil }) +mockPermissionCheckerError := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { + return false, errors.New("internal permission check error") +})
104-106: Consider testing different permission actions.The mock permission checker always returns true regardless of the action. Consider testing different actions to ensure proper authorization granularity.
mockPermissionCheckerTrue := NewMockPermissionChecker(func(ctx context.Context, authInfo *appx.AuthInfo, resource, action string) (bool, error) { - return true, nil + switch action { + case "read": + return true, nil + case "write": + return true, nil + default: + return false, nil + } })Also applies to: 173-175
server/api/internal/usecase/interactor/job.go (1)
104-232: Consider using more granular permissions for job monitoringCurrently, all monitoring-related methods use
rbac.ActionAny. Consider implementing more specific permissions likeActionMonitororActionReadto better control access to job monitoring functionality.-func (i *Job) checkPermission(ctx context.Context, action string) error { - return checkPermission(ctx, i.permissionChecker, rbac.ResourceJob, action) -} +const ( + ActionMonitor = "monitor" + ActionRead = "read" +) + +func (i *Job) checkPermission(ctx context.Context, action string) error { + return checkPermission(ctx, i.permissionChecker, rbac.ResourceJob, action) +}Then update the monitoring methods to use specific actions:
-if err := i.checkPermission(ctx, rbac.ActionAny); err != nil { +if err := i.checkPermission(ctx, ActionMonitor); err != nil {server/api/internal/usecase/interactor/deployment.go (1)
325-325: TODO comment needs implementationThe TODO comment indicates that asset handling needs to be implemented.
Would you like me to help implement the asset handling functionality or create an issue to track this task?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.work.sumis excluded by!**/*.sumserver/api/README.mdis excluded by!**/*.mdserver/api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (69)
.github/workflows/ci.yml(6 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)server/api/.env.example(1 hunks)server/api/Makefile(1 hunks)server/api/cmd/policy-generator/main.go(1 hunks)server/api/e2e/common.go(6 hunks)server/api/e2e/gql_deployment_pagination_test.go(1 hunks)server/api/e2e/gql_me_test.go(1 hunks)server/api/e2e/gql_pagination_test.go(3 hunks)server/api/e2e/gql_param_test.go(1 hunks)server/api/e2e/gql_projectAccess_test.go(1 hunks)server/api/e2e/gql_project_test.go(2 hunks)server/api/e2e/gql_trigger_test.go(3 hunks)server/api/e2e/gql_user_test.go(6 hunks)server/api/e2e/gql_workspace_test.go(6 hunks)server/api/e2e/ping_test.go(1 hunks)server/api/go.mod(2 hunks)server/api/internal/adapter/context.go(0 hunks)server/api/internal/adapter/gql/context.go(0 hunks)server/api/internal/adapter/gql/loader_asset.go(2 hunks)server/api/internal/adapter/gql/loader_deployment.go(6 hunks)server/api/internal/adapter/gql/loader_job.go(3 hunks)server/api/internal/adapter/gql/loader_project.go(2 hunks)server/api/internal/adapter/gql/loader_trigger.go(2 hunks)server/api/internal/adapter/gql/loader_user.go(1 hunks)server/api/internal/adapter/gql/resolver_mutation_asset.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_deployment.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_parameter.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_project.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_projectAccess.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_trigger.go(3 hunks)server/api/internal/adapter/gql/resolver_project.go(1 hunks)server/api/internal/adapter/gql/resolver_subscription.go(1 hunks)server/api/internal/app/app.go(1 hunks)server/api/internal/app/auth_client.go(0 hunks)server/api/internal/app/config/config.go(1 hunks)server/api/internal/app/main.go(3 hunks)server/api/internal/app/repo.go(2 hunks)server/api/internal/app/trigger.go(1 hunks)server/api/internal/app/usecase.go(1 hunks)server/api/internal/rbac/definitions.go(1 hunks)server/api/internal/usecase/gateway/mock_permission.go(1 hunks)server/api/internal/usecase/gateway/permission.go(1 hunks)server/api/internal/usecase/interactor/asset.go(4 hunks)server/api/internal/usecase/interactor/asset_test.go(3 hunks)server/api/internal/usecase/interactor/common.go(4 hunks)server/api/internal/usecase/interactor/deployment.go(7 hunks)server/api/internal/usecase/interactor/job.go(8 hunks)server/api/internal/usecase/interactor/parameter.go(5 hunks)server/api/internal/usecase/interactor/parameter_test.go(6 hunks)server/api/internal/usecase/interactor/project.go(5 hunks)server/api/internal/usecase/interactor/projectAccess.go(4 hunks)server/api/internal/usecase/interactor/projectAccess_test.go(5 hunks)server/api/internal/usecase/interactor/project_test.go(1 hunks)server/api/internal/usecase/interactor/trigger.go(6 hunks)server/api/internal/usecase/interactor/trigger_test.go(9 hunks)server/api/internal/usecase/interactor/usecase.go(1 hunks)server/api/internal/usecase/interactor/usecase_test.go(5 hunks)server/api/internal/usecase/interactor/utils_test.go(1 hunks)server/api/internal/usecase/interfaces/asset.go(1 hunks)server/api/internal/usecase/interfaces/deployment.go(1 hunks)server/api/internal/usecase/interfaces/job.go(1 hunks)server/api/internal/usecase/interfaces/parameter.go(1 hunks)server/api/internal/usecase/interfaces/project.go(1 hunks)server/api/internal/usecase/interfaces/projectAccess.go(1 hunks)server/api/internal/usecase/interfaces/trigger.go(1 hunks)server/api/internal/usecase/operator.go(1 hunks)server/api/internal/usecase/repo/container.go(0 hunks)
💤 Files with no reviewable changes (4)
- server/api/internal/app/auth_client.go
- server/api/internal/adapter/gql/context.go
- server/api/internal/adapter/context.go
- server/api/internal/usecase/repo/container.go
🚧 Files skipped from review as they are similar to previous changes (46)
- server/api/cmd/policy-generator/main.go
- server/api/e2e/ping_test.go
- server/api/internal/app/repo.go
- server/api/internal/adapter/gql/resolver_project.go
- server/api/internal/app/trigger.go
- server/api/e2e/gql_trigger_test.go
- server/api/Makefile
- server/api/internal/usecase/interactor/utils_test.go
- server/api/e2e/gql_param_test.go
- server/api/internal/adapter/gql/resolver_mutation_project.go
- server/api/internal/adapter/gql/resolver_subscription.go
- server/api/e2e/gql_user_test.go
- server/api/e2e/gql_workspace_test.go
- server/api/internal/usecase/gateway/mock_permission.go
- server/api/internal/adapter/gql/resolver_mutation_deployment.go
- server/api/.env.example
- server/api/internal/usecase/gateway/permission.go
- server/api/internal/adapter/gql/loader_user.go
- server/api/internal/adapter/gql/resolver_mutation_asset.go
- server/api/internal/usecase/interfaces/projectAccess.go
- server/api/e2e/gql_deployment_pagination_test.go
- server/api/internal/app/config/config.go
- server/api/e2e/gql_project_test.go
- server/api/e2e/gql_pagination_test.go
- server/api/e2e/gql_me_test.go
- server/api/internal/app/app.go
- server/api/internal/adapter/gql/loader_trigger.go
- server/api/internal/usecase/interfaces/project.go
- server/api/internal/adapter/gql/loader_project.go
- server/api/internal/usecase/interactor/trigger_test.go
- server/api/internal/adapter/gql/resolver_mutation_trigger.go
- server/api/internal/usecase/interactor/asset_test.go
- server/api/internal/adapter/gql/loader_asset.go
- server/api/internal/usecase/interfaces/job.go
- server/api/e2e/common.go
- server/api/go.mod
- .github/workflows/update_policies.yml
- .github/workflows/ci_policies.yml
- server/api/internal/adapter/gql/resolver_mutation_parameter.go
- server/api/internal/rbac/definitions.go
- server/api/internal/adapter/gql/loader_deployment.go
- server/api/internal/adapter/gql/resolver_mutation_projectAccess.go
- server/api/internal/usecase/interfaces/parameter.go
- server/api/internal/usecase/interfaces/asset.go
- server/api/internal/adapter/gql/loader_job.go
- server/api/internal/usecase/interactor/parameter.go
🧰 Additional context used
🧠 Learnings (2)
server/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`.
server/api/internal/usecase/interactor/deployment.go (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: api/internal/usecase/interactor/deployment.go:48-70
Timestamp: 2025-01-21T10:53:31.572Z
Learning: The Fetch and Find methods in the deployment service require permission checks using the new permission checker mechanism after migrating away from operator-based checks.
🔇 Additional comments (36)
server/api/internal/app/usecase.go (2)
15-15: LGTM! Good enhancement to centralize permission handling.The addition of the
permissionCheckerparameter aligns well with the PR's goal of improving authorization capabilities. This change helps establish a more consistent and centralized approach to permission management.
19-19: LGTM! Good simplification of the initialization logic.The updated initialization removes operator-based filtering in favor of the new centralized permission checking mechanism, resulting in cleaner and more maintainable code.
server/api/internal/usecase/interactor/parameter_test.go (1)
54-261: Test cases effectively verify core functionality.The test suite maintains good coverage of the parameter management operations, including declaration, fetching, removal, ordering, and value updates. The modifications to remove the operator parameter are consistent and well-executed.
server/api/internal/usecase/interactor/trigger.go (4)
8-8: LGTM! Clean struct organization with new permission checking.The addition of the
permissionCheckerfield and rbac import aligns well with the new permission management system.Also applies to: 20-30
32-48: LGTM! Well-structured permission checking implementation.The constructor properly initializes the permissionChecker, and the helper method promotes code reuse across all operations.
50-72: LGTM! Consistent permission checks across CRUD operations.Each method appropriately checks permissions using the correct rbac action constants before proceeding with the operation.
Also applies to: 74-120, 192-241, 243-266
122-147:⚠️ Potential issueConsider the order of permission and authentication checks.
The permission check is performed before validating the authentication token. This could potentially leak information about the existence of triggers to unauthorized users.
Apply this diff to improve security:
func (i *Trigger) ExecuteAPITrigger(ctx context.Context, p interfaces.ExecuteAPITriggerParam) (_ *job.Job, err error) { - if err := i.checkPermission(ctx, rbac.ActionCreate); err != nil { - return nil, err - } - tx, err := i.transaction.Begin(ctx) if err != nil { return } ctx = tx.Context() defer func() { if err2 := tx.End(ctx); err == nil && err2 != nil { err = err2 } }() trigger, err := i.triggerRepo.FindByID(ctx, p.TriggerID) if err != nil { return nil, err } if trigger.EventSource() == "API_DRIVEN" { if p.AuthenticationToken != *trigger.AuthToken() { return nil, fmt.Errorf("invalid auth token") } } + if err := i.checkPermission(ctx, rbac.ActionCreate); err != nil { + return nil, err + } +server/api/internal/app/main.go (3)
51-61: Improve error handling in permission checker initialization.The current implementation uses
log.Fatalfwhich makes the application terminate abruptly. This approach:
- Makes it difficult to handle errors gracefully in tests
- Could cause issues in containerized environments
- Discards the parsed URL result
Apply this diff to improve error handling:
// PermissionChecker if conf.DashboardHost == "" { - log.Fatalf("dashboard host configuration is required") + return fmt.Errorf("dashboard host configuration is required") } -if _, err := url.Parse(conf.DashboardHost); err != nil { - log.Fatalf("invalid dashboard host URL: %v", err) +parsedURL, err := url.Parse(conf.DashboardHost) +if err != nil { + return fmt.Errorf("invalid dashboard host URL: %w", err) } -permissionChecker := cerbosClient.NewPermissionChecker(rbac.ServiceName, conf.DashboardHost) + +opts := &cerbosClient.Options{ + // Add configurable options like timeout, retries, etc. +} +permissionChecker := cerbosClient.NewPermissionChecker(rbac.ServiceName, parsedURL.String(), opts) if permissionChecker == nil { - log.Fatalf("failed to initialize permission checker") + return fmt.Errorf("failed to initialize permission checker") }
80-88: LGTM! Good addition of PermissionChecker to ServerConfig.The ServerConfig struct is well-organized and the new PermissionChecker field is appropriately placed.
64-72: LGTM! Proper initialization of ServerConfig with PermissionChecker.The NewServer call correctly includes all required fields including the new PermissionChecker.
.github/workflows/ci.yml (3)
6-12: LGTM! Well-structured workflow_dispatch input.The force_update_policies input is properly defined with clear description and default value.
41-50: LGTM! Comprehensive policy file monitoring.The changed files configuration for policies correctly includes all relevant paths:
- RBAC policy files
- Related workflow files
- Changelog
169-182:❓ Verification inconclusive
Verify the condition for update-policies job.
The job conditions look complex. Let's verify they work as expected:
- No failures in previous jobs
- ci-policies succeeded
- Either:
- Push to main/release branches
- Manual trigger with force_update_policies
🏁 Script executed:
#!/bin/bash # Description: Verify the workflow conditions by checking recent workflow runs gh api graphql -f query=' query($owner:String!, $name:String!) { repository(owner:$owner, name:$name) { workflowRuns(first:5) { nodes { workflow { name } event status conclusion jobs(first:10) { nodes { name status conclusion } } } } } }' -f owner=reearth -f name=reearth-flowLength of output: 719
Action Required: Manual Verification of Update-Policies Job Conditions
The automatic verification attempt using a GitHub GraphQL query failed because the field
workflowRunsdoes not exist on theRepositorytype. As a result, the script did not provide conclusive output. Please manually verify that theupdate-policiesjob conditions work as intended:
- Ensure there are no failures in previous jobs.
- Confirm that the
ci-policiesjob has succeeded.- Verify that the job runs only when either:
- The push is to the
mainorreleasebranches (or branches starting withrelease/), or- A manual trigger with
force_update_policiesis provided.Consider reviewing recent workflow runs via the GitHub Actions UI or using an alternative CLI command (for example,
gh run list --workflow=".github/workflows/ci.yml") to validate that the conditions trigger as expected.server/api/e2e/gql_projectAccess_test.go (1)
104-104:❓ Verification inconclusive
Verify the impact of the new boolean parameter.
The
StartGQLServerfunction now accepts an additional boolean parameter. Please ensure this change is documented and its purpose is clear.Run this script to check the function definition and its usage across test files:
🏁 Script executed:
#!/bin/bash # Description: Check StartGQLServer function definition and usage # Find the StartGQLServer function definition rg -A 5 "func StartGQLServer" # Find all test files using StartGQLServer rg "StartGQLServer.*true.*true" -g "*.go"Length of output: 1309
Action Required: Clarify and Verify the New 'allowPermission' Parameter
Please review and document the purpose of the additional boolean parameter (
allowPermission) introduced in theStartGQLServerfunction. In particular:
- Parameter Documentation: Update the code comments and any relevant documentation to explain the effect and intended usage of
allowPermission.- Test Consistency: Confirm that passing
trueforallowPermission(as seen inserver/api/e2e/gql_projectAccess_test.goat line 104) is correct and behaves as expected in all test environments.- Manual Verification: Given that automated script results were inconclusive regarding some usage patterns, please manually verify that this parameter change does not inadvertently affect the server’s permission controls.
server/api/internal/usecase/interactor/project_test.go (2)
20-29: Well-structured helper function!The
setupProjecthelper function improves test maintainability by centralizing the setup logic and reducing code duplication.
48-83: Comprehensive test coverage with table-driven tests!The test cases effectively cover the main scenarios including permission checks. The table-driven approach makes it easy to add more test cases.
server/api/internal/usecase/interactor/projectAccess.go (2)
19-35: Clean integration of permission checker!The addition of
permissionCheckerand its initialization in the constructor is well-structured. The dependency injection pattern allows for better testing and flexibility.
61-114:❓ Verification inconclusive
Verify transaction rollback on permission check failure.
The permission check is correctly placed before starting the transaction, but we should ensure the transaction is properly rolled back in all error cases.
🏁 Script executed:
#!/bin/bash # Search for all transaction.Begin calls and verify proper rollback handling ast-grep --pattern 'tx, err := $_.transaction.Begin($ctx)'Length of output: 60
Transaction Rollback Verification – Manual Confirmation Needed
Based on the code snippet, the permission check is performed before starting a transaction. This means that when a permission error occurs, no transaction is initiated (and therefore no rollback is required). In the transaction block, the deferred call to
tx.End(ctx)appears to handle rollback for errors that occur after the transaction is begun.However, since the initial AST grep did not produce any output to confirm the expected pattern, please manually verify that:
- All paths that call
transaction.Begin(ctx)in the codebase follow the pattern shown here (i.e., they perform necessary error handling via the deferredtx.End(ctx)).- No transaction is initiated before the permission check (ensuring that rollback on permission failure isn’t necessary).
If manual review confirms the above, no changes are needed in this function.
server/api/internal/usecase/operator.go (1)
7-11:✅ Verification successful
LGTM! Clear deprecation notice and minimal struct definition.
The TODO comment effectively communicates the temporary nature of this file and its planned removal after the Cerbos migration. The struct is kept minimal with just the essential field, which is good practice during a transition period.
Let's verify that this struct is indeed being phased out by checking its usage:
🏁 Script executed:
#!/bin/bash # Description: Check remaining usages of the Operator struct rg -A 5 "type Operator struct" --type go rg "usecase\.Operator" --type goLength of output: 1289
LGTM! Clear deprecation notice with a minimal transitional struct definition.
The TODO comment clearly states that this file and its usages will be removed once Cerbos has been fully integrated. We verified that the
Operatorstruct (with its singleAcOperatorfield) is still referenced in several parts of the code (e.g., inauth_client.go,adapter/context.go, andadapter/gql/context.go), which is expected given its temporary role for backward compatibility during the migration.Once the Cerbos migration is complete, please ensure that the referenced usages across the codebase are also addressed accordingly.
server/api/internal/usecase/interfaces/trigger.go (1)
45-53:❓ Verification inconclusive
LGTM! Clean interface refactoring.
The removal of the operator parameter from all methods simplifies the interface while maintaining its core functionality. This change aligns well with the migration to a centralized permission management system.
Let's verify that all implementations of this interface have been updated:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining implementations that might still use the old signature ast-grep --pattern 'type $_ struct { $$$ } func ($_ $_) ExecuteAPITrigger(ctx context.Context, param ExecuteAPITriggerParam, op *usecase.Operator)'Length of output: 152
Below is an additional verification step using an alternative search command. This script looks for any occurrence of the operator parameter in the old ExecuteAPITrigger signature:
Once this script runs, please review the output. If no results are found, it would confirm that all implementations have been updated to the new signature. If any results do appear, those locations would need further attention.
LGTM! Clean interface refactoring.
The operator parameter removal from all methods simplifies the interface and aligns perfectly with the migration towards centralized permission management—assuming the new script confirms no lingering references to the old signature.
Please verify manually with the above search to ensure no instances of the old signature remain.
🏁 Script executed:
[run_scripts] #!/bin/bash # Description: Check for any remaining references to the old ExecuteAPITrigger signature that includes the operator parameter. rg -n "func .*ExecuteAPITrigger\(.*op\s*\*usecase\.Operator" .Length of output: 186
Below is an updated review comment based on an additional verification step. Although the previous script didn't yield any output due to a formatting issue with the header, please execute the following revised command to search for any lingering references to the old operator parameter:
#!/bin/bash # Description: Check for any usage of *usecase.Operator in the codebase rg -n 'op\s*\*usecase\.Operator' .If this search does not return any results, it confirms that all implementations have been updated to the new signature.
LGTM! Clean interface refactoring.
The removal of the operator parameter from the
ExecuteAPITriggermethod and related interface methods cleanly simplifies the code and supports the migration to centralized permission management. Please run the revised verification script above to ensure no lingering usage of*usecase.Operatorremains.server/api/internal/usecase/interfaces/deployment.go (1)
37-48:✅ Verification successful
LGTM! Consistent interface simplification.
The removal of the operator parameter from all methods maintains consistency with other interfaces while preserving the core deployment functionality. This change supports the transition to centralized permission management.
Let's verify that all implementations have been updated:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining implementations using old signatures ast-grep --pattern 'type $_ struct { $$$ } func ($_ $_) Fetch(ctx context.Context, ids []id.DeploymentID, op *usecase.Operator)'Length of output: 133
I'll run an additional verification to ensure that no implementation still references the old signature with the operator parameter. In the meantime, please manually verify the entire codebase if possible.
LGTM – Interface Update Verification
The interface inserver/api/internal/usecase/interfaces/deployment.gohas been successfully simplified by removing the operator parameter, consistent with our transition to centralized permission management. Our additional search usingrgdid not return any references to the old signature. Please review and confirm that no outdated operator parameter usages remain.
🏁 Script executed:
[run_scripts] #!/bin/bash # Verify the absence of any occurrences of the old operator parameter in function signatures rg -n "op\s*\*usecase\.Operator" .Length of output: 158
Interface Update Verification – Please Confirm
The removal of the operator parameter from the deployment interface methods appears to be correctly applied. The above script is intended to search for any instances of an outdated Fetch function signature that still includes the parameterop *usecase.Operator. If the script returns no output, it confirms that no outdated implementations remain.Please run this script in your repository environment to manually verify the absence of the old operator parameter.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-verify that no function definitions for Fetch still include the outdated operator parameter. rg -n 'func\s+Fetch\(.*,\s*op\s*\*usecase\.Operator' .Length of output: 178
Interface Simplification Verified – Approved
The deployment interface inserver/api/internal/usecase/interfaces/deployment.gohas been successfully updated to remove the operator parameter. Our verification checks (usingrgandast-grep) confirmed that no implementations of theFetchmethod or similar functions still reference the removedop *usecase.Operatorparameter. This update aligns with the centralized permission management design.
- Deployment interface updated (no operator parameter).
- No lingering references to the old signature found in the codebase.
server/api/internal/usecase/interactor/usecase.go (1)
38-79:✅ Verification successful
LGTM! Clean refactoring of generic Run functions.
The removal of the operator parameter from Run functions simplifies the API while maintaining essential functionality:
- Transaction handling remains robust
- Workspace context is preserved in the uc struct
- Generic type parameters are properly utilized
Let's verify that all callers have been updated to the new signatures:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining calls using the old signature rg "Run[0-3]\(.*usecase\.Operator\)" --type goLength of output: 48
LGTM! Clean refactoring confirmed.
The removal of the operator parameter from the Run functions has successfully simplified the API while ensuring that:
- Transaction handling remains robust.
- The workspace context in the
ucstruct is preserved.- No outdated calls using
usecase.Operatorwere found in the codebase.After re-running the verification script, there were no references to the old signature, confirming that all callers have been updated.
server/api/internal/usecase/interactor/usecase_test.go (1)
30-32: LGTM! The changes align with the new permission model.The removal of the
nilargument fromRun3andRun0function calls reflects the shift from operator-based permission checks to a centralized permission-checking mechanism.Also applies to: 46-48, 58-60, 73-75, 88-90
server/api/internal/usecase/interactor/common.go (3)
20-20: LGTM! Clear and descriptive error variable.The error variable provides a clear message for permission denial scenarios.
29-47: LGTM! The changes align with the learnings from PR #580.The
NewContainerfunction now acceptspermissionCheckerdirectly as an argument and propagates it to all components, maintaining consistency with other parameters.
73-83: LGTM! Well-structured permission checking function.The
checkPermissionfunction:
- Retrieves authentication information from the context
- Uses the
permissionCheckerto verify permissions- Returns clear error messages
server/api/internal/usecase/interactor/asset.go (3)
17-21: LGTM! Clear and well-organized struct fields.The
Assetstruct fields are logically grouped and named descriptively.
31-33: LGTM! Encapsulated permission check.The
checkPermissionmethod encapsulates the permission checking logic, making it reusable across all methods.
35-41: LGTM! Consistent permission checks across all methods.Each method:
- Checks permissions before proceeding
- Returns clear error messages
- Maintains consistent error handling
Also applies to: 43-59, 61-91, 93-116
server/api/internal/usecase/interactor/project.go (3)
18-28: LGTM! Clear and well-organized struct fields.The
Projectstruct fields are logically grouped and named descriptively.
43-45: LGTM! Encapsulated permission check.The
checkPermissionmethod encapsulates the permission checking logic, making it reusable across all methods.
47-53: LGTM! Granular permission checks with specific actions.Each method:
- Checks permissions with appropriate actions (list, create, edit, delete)
- Returns clear error messages
- Maintains consistent error handling
Also applies to: 55-61, 63-110, 112-164, 166-198, 200-236
server/api/internal/usecase/interactor/job.go (3)
22-54: LGTM: Clean implementation of permission checker integrationThe Job struct and constructor have been properly updated to include the permission checker, maintaining clean dependency injection and proper initialization of all required fields.
56-58: LGTM: Well-structured permission check implementationThe checkPermission method provides a clean abstraction for job-specific permission checks, properly utilizing the RBAC resource type.
133-167: LGTM: Robust error handling and loggingThe error handling in the monitoring loop is well-implemented, with proper context cancellation handling and informative error logging.
server/api/internal/usecase/interactor/deployment.go (1)
23-49: LGTM: Consistent implementation with Job interactorThe Deployment struct and constructor follow the same clean pattern as the Job interactor, properly integrating the permission checker.
pyshx
changed the title
pyshx
changed the title
pyshx
changed the title
akiyatomohiro
force-pushed
the
feat/authorization-and-roles-management-service-2
branch
from
e1ccc55 to
4993bb1
Compare
This reverts commit 4993bb1.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (15)
server/api/internal/usecase/interactor/common.go (2)
20-20: Consider enhancing the error message.While the error variable follows Go conventions, consider making the message more descriptive by including the resource and action that were denied.
-var ErrPermissionDenied = fmt.Errorf("permission denied") +var ErrPermissionDenied = fmt.Errorf("permission denied: %s %s")Then update the usage in
checkPermission:- return ErrPermissionDenied + return fmt.Errorf(ErrPermissionDenied.Error(), resource, action)
73-83: Add documentation for the checkPermission function.Consider adding a doc comment to describe:
- The purpose of the function
- Parameter descriptions
- Return value semantics
- Example usage
+// checkPermission verifies if the authenticated user has the specified permission. +// Parameters: +// - ctx: The context containing authentication information +// - permissionChecker: The permission checking service +// - resource: The resource being accessed +// - action: The action being performed +// Returns an error if permission is denied or if the check fails. func checkPermission(ctx context.Context, permissionChecker gateway.PermissionChecker, resource string, action string) error {server/api/e2e/gql_projectAccess_test.go (1)
104-104: Document the purpose of the new boolean parameter.The purpose of the new boolean parameter in
StartGQLServeris not clear from its usage. Consider adding a comment or using a named parameter to clarify its role in the server configuration.- }, true, baseSeederUser, true) + }, true, baseSeederUser, enablePermissionChecking /* enables Cerbos-based permission checking */)server/api/internal/usecase/interactor/usecase.go (1)
68-79:❓ Verification inconclusive
Document permission checking requirements.
The removal of the operator parameter aligns with the centralized permission checking approach. Since this is the core function that other
Run*functions delegate to, consider adding documentation about where and how permission checks should be performed.Add a comment block above the function to document:
- The responsibility for permission checking
- How to integrate with the new centralized permission checker
- Example usage with the new approach
Run this script to find all callers of
Run3that might need updates:
🏁 Script executed:
#!/bin/bash # Search for calls to Run3 that might still pass the operator parameter ast-grep --pattern 'Run3($$$, op, $$$)'Length of output: 41
Action Required: Document Permission Checking
The removal of the operator parameter is in line with the shift toward centralized permission checking. However, the automated search for residual
opusage inRun3was inconclusive. Please manually verify that no call sites are still passing an operator parameter. In addition, add a comment block above theRun3function that clearly documents:
- Where and how permission checks should be performed.
- How the function integrates with the centralized permission checker.
- An example of the updated usage pattern.
server/api/internal/usecase/operator.go (2)
7-8: Track the TODO comment in the issue tracker.The TODO comment indicates that this file will be deleted after completing the migration to Cerbos. To ensure this technical debt is properly tracked:
- Create a tracking issue for the deletion of this file
- Document the dependencies that need to be resolved first:
- Migration to Cerbos
- Modification of reearthx interfaces
- Modification of reearth-dashboard interfaces
Would you like me to help create a tracking issue for this TODO?
9-11: Document the deprecation status.Since this struct is pending deletion, consider adding a deprecation notice to help other developers:
// TODO: After migrating to Cerbos for permission management and modifying reearthx and reearth-dashboard interfaces, // this file and all its usages will be deleted. +// Deprecated: This struct will be removed after the migration to Cerbos is complete. +// Use the new permission checking mechanisms instead. type Operator struct { AcOperator *accountusecase.Operator }server/api/internal/rbac/definitions.go (5)
7-10: Add documentation and consider cross-platform path handling.Consider the following improvements:
- Add documentation comments for the constants to explain their purpose and usage.
- Consider using
filepath.Joinfor PolicyFileDir to ensure cross-platform compatibility.+// ServiceName is the identifier for the flow service in the RBAC system const ( ServiceName = "flow" + // PolicyFileDir is the directory where RBAC policy files are stored PolicyFileDir = "policies" )
12-22: Add documentation for resource types.Each resource type should be documented to explain its purpose and usage in the RBAC system.
const ( + // ResourceAsset represents an asset in the system ResourceAsset = "asset" + // ResourceDeployment represents a deployment configuration ResourceDeployment = "deployment" + // ResourceJob represents a job execution ResourceJob = "job" // ... add documentation for remaining resources )
24-31: Document actions and clarify the scope of 'ActionAny'.The actions need documentation, and the scope of 'ActionAny' should be clarified as it might be too permissive.
const ( + // ActionRead allows reading a resource ActionRead = "read" + // ActionList allows listing multiple resources ActionList = "list" + // ActionCreate allows creating new resources ActionCreate = "create" + // ActionEdit allows modifying existing resources ActionEdit = "edit" + // ActionDelete allows removing resources ActionDelete = "delete" + // ActionAny grants all permissions on a resource + // TODO: Consider if this broad permission is necessary or if it should be more granular ActionAny = "any" )
33-39: Improve role definitions and consider making them public.The roles would benefit from:
- Documentation explaining each role's responsibilities
- Making constants public for reuse
- Explicit definition of role hierarchy
+// Role constants define the available permission levels in ascending order: +// Self < Reader < Editor < Maintainer < Owner const ( - roleSelf = "self" - roleReader = "reader" - roleEditor = "editor" - roleMaintainer = "maintainer" - roleOwner = "owner" + // RoleSelf allows users to manage their own resources + RoleSelf = "self" + // RoleReader provides read-only access to resources + RoleReader = "reader" + // RoleEditor allows modifying existing resources + RoleEditor = "editor" + // RoleMaintainer provides advanced management capabilities + RoleMaintainer = "maintainer" + // RoleOwner has full control over resources + RoleOwner = "owner" )
41-135: Consider modularizing the DefineResources function.The function is quite long and could be split into smaller, more focused functions for each resource type.
Example refactor:
func defineAssetResource(builder *generator.ResourceBuilder) *generator.ResourceBuilder { return builder.AddResource(ResourceAsset, []generator.ActionDefinition{ generator.NewActionDefinition(ActionAny, []string{ roleMaintainer, }), }) } func defineProjectResource(builder *generator.ResourceBuilder) *generator.ResourceBuilder { return builder.AddResource(ResourceProject, []generator.ActionDefinition{ // ... project actions }) } func DefineResources(builder *generator.ResourceBuilder) ([]generator.ResourceDefinition, error) { if builder == nil { panic("ResourceBuilder cannot be nil") } builder = defineAssetResource(builder) builder = defineProjectResource(builder) // ... other resources return builder.Build() }server/api/internal/usecase/interactor/projectAccess.go (1)
133-139: Improve error message clarity.The error message "project access not found" would be clearer if it included the project ID.
- return errors.New("project access not found") + return fmt.Errorf("project access not found for project ID: %s", projectID)server/api/internal/usecase/interactor/project.go (1)
221-222: Replace fmt.Println with proper logging.Using
fmt.Printlnfor debugging is not recommended in production code. Consider using a proper logging framework.- fmt.Println("RunProjectParam", p) + // TODO: Replace with proper logging framework + // log.Debug("Running project with parameters", "params", p)server/api/internal/usecase/interactor/projectAccess_test.go (1)
55-75: Consider grouping test cases by behavior.The test cases could be better organized by grouping them into positive and negative test cases with clear comments.
tests := []struct { name string token string wantErr bool }{ + // Positive test cases { name: "success: fetch public project", token: paPublic.Token(), wantErr: false, }, + // Negative test cases { name: "failure: invalid token", token: "invalid-token", wantErr: true, }, { name: "failure: not public", token: paPrivate.Token(), wantErr: true, }, }server/api/internal/usecase/interactor/deployment.go (1)
51-54: Consider differentiating permission levels for different actions.Currently, all methods use
rbac.ActionAnyfor permission checks. Consider implementing more granular permissions:
- Read permissions for methods like
Fetch,FindByWorkspace, etc.- Write permissions for methods like
Create,Update,Delete- Execute permissions for the
ExecutemethodThis would provide better security control over different operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
engine/Cargo.lockis excluded by!**/*.lock,!**/*.lockgo.work.sumis excluded by!**/*.sumserver/api/README.mdis excluded by!**/*.mdserver/api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (72)
.github/workflows/ci.yml(6 hunks).github/workflows/ci_policies.yml(1 hunks).github/workflows/update_policies.yml(1 hunks)engine/Cargo.toml(1 hunks)engine/plateau-gis-quality-checker/src-tauri/Cargo.toml(1 hunks)server/api/.env.example(1 hunks)server/api/Makefile(1 hunks)server/api/cmd/policy-generator/main.go(1 hunks)server/api/e2e/common.go(6 hunks)server/api/e2e/gql_deployment_pagination_test.go(1 hunks)server/api/e2e/gql_me_test.go(1 hunks)server/api/e2e/gql_pagination_test.go(3 hunks)server/api/e2e/gql_param_test.go(1 hunks)server/api/e2e/gql_projectAccess_test.go(1 hunks)server/api/e2e/gql_project_test.go(2 hunks)server/api/e2e/gql_trigger_test.go(3 hunks)server/api/e2e/gql_user_test.go(6 hunks)server/api/e2e/gql_workspace_test.go(6 hunks)server/api/e2e/ping_test.go(1 hunks)server/api/go.mod(2 hunks)server/api/internal/adapter/context.go(0 hunks)server/api/internal/adapter/gql/context.go(0 hunks)server/api/internal/adapter/gql/loader_asset.go(2 hunks)server/api/internal/adapter/gql/loader_deployment.go(6 hunks)server/api/internal/adapter/gql/loader_job.go(3 hunks)server/api/internal/adapter/gql/loader_project.go(2 hunks)server/api/internal/adapter/gql/loader_trigger.go(2 hunks)server/api/internal/adapter/gql/loader_user.go(1 hunks)server/api/internal/adapter/gql/resolver_mutation_asset.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_deployment.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_job.go(1 hunks)server/api/internal/adapter/gql/resolver_mutation_parameter.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_project.go(4 hunks)server/api/internal/adapter/gql/resolver_mutation_projectAccess.go(2 hunks)server/api/internal/adapter/gql/resolver_mutation_trigger.go(3 hunks)server/api/internal/adapter/gql/resolver_project.go(1 hunks)server/api/internal/adapter/gql/resolver_subscription.go(1 hunks)server/api/internal/app/app.go(1 hunks)server/api/internal/app/auth_client.go(0 hunks)server/api/internal/app/config/config.go(1 hunks)server/api/internal/app/main.go(3 hunks)server/api/internal/app/repo.go(2 hunks)server/api/internal/app/trigger.go(1 hunks)server/api/internal/app/usecase.go(1 hunks)server/api/internal/rbac/definitions.go(1 hunks)server/api/internal/usecase/gateway/mock_permission.go(1 hunks)server/api/internal/usecase/gateway/permission.go(1 hunks)server/api/internal/usecase/interactor/asset.go(4 hunks)server/api/internal/usecase/interactor/asset_test.go(3 hunks)server/api/internal/usecase/interactor/common.go(4 hunks)server/api/internal/usecase/interactor/deployment.go(7 hunks)server/api/internal/usecase/interactor/job.go(9 hunks)server/api/internal/usecase/interactor/parameter.go(5 hunks)server/api/internal/usecase/interactor/parameter_test.go(6 hunks)server/api/internal/usecase/interactor/project.go(5 hunks)server/api/internal/usecase/interactor/projectAccess.go(4 hunks)server/api/internal/usecase/interactor/projectAccess_test.go(5 hunks)server/api/internal/usecase/interactor/project_test.go(1 hunks)server/api/internal/usecase/interactor/trigger.go(6 hunks)server/api/internal/usecase/interactor/trigger_test.go(9 hunks)server/api/internal/usecase/interactor/usecase.go(1 hunks)server/api/internal/usecase/interactor/usecase_test.go(5 hunks)server/api/internal/usecase/interactor/utils_test.go(1 hunks)server/api/internal/usecase/interfaces/asset.go(1 hunks)server/api/internal/usecase/interfaces/deployment.go(1 hunks)server/api/internal/usecase/interfaces/job.go(1 hunks)server/api/internal/usecase/interfaces/parameter.go(1 hunks)server/api/internal/usecase/interfaces/project.go(1 hunks)server/api/internal/usecase/interfaces/projectAccess.go(1 hunks)server/api/internal/usecase/interfaces/trigger.go(1 hunks)server/api/internal/usecase/operator.go(1 hunks)server/api/internal/usecase/repo/container.go(0 hunks)
💤 Files with no reviewable changes (4)
- server/api/internal/adapter/gql/context.go
- server/api/internal/adapter/context.go
- server/api/internal/app/auth_client.go
- server/api/internal/usecase/repo/container.go
✅ Files skipped from review due to trivial changes (2)
- engine/Cargo.toml
- engine/plateau-gis-quality-checker/src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (51)
- server/api/.env.example
- server/api/internal/usecase/gateway/permission.go
- server/api/cmd/policy-generator/main.go
- server/api/internal/app/trigger.go
- server/api/e2e/gql_me_test.go
- server/api/e2e/gql_param_test.go
- server/api/internal/app/repo.go
- server/api/internal/app/config/config.go
- server/api/e2e/gql_deployment_pagination_test.go
- server/api/internal/adapter/gql/resolver_subscription.go
- server/api/e2e/gql_trigger_test.go
- server/api/internal/adapter/gql/resolver_mutation_deployment.go
- server/api/internal/usecase/interactor/utils_test.go
- server/api/internal/adapter/gql/loader_user.go
- server/api/e2e/gql_user_test.go
- server/api/internal/app/app.go
- server/api/internal/adapter/gql/loader_trigger.go
- server/api/internal/usecase/gateway/mock_permission.go
- server/api/e2e/ping_test.go
- server/api/Makefile
- server/api/internal/adapter/gql/resolver_mutation_asset.go
- server/api/internal/usecase/interactor/asset_test.go
- server/api/internal/adapter/gql/resolver_mutation_projectAccess.go
- server/api/internal/adapter/gql/loader_project.go
- .github/workflows/update_policies.yml
- server/api/internal/adapter/gql/resolver_mutation_trigger.go
- server/api/e2e/gql_project_test.go
- server/api/internal/adapter/gql/resolver_project.go
- server/api/internal/usecase/interfaces/project.go
- server/api/internal/adapter/gql/loader_job.go
- server/api/internal/usecase/interactor/trigger_test.go
- server/api/e2e/gql_pagination_test.go
- server/api/go.mod
- server/api/e2e/gql_workspace_test.go
- server/api/e2e/common.go
- server/api/internal/usecase/interfaces/asset.go
- server/api/internal/usecase/interactor/asset.go
- .github/workflows/ci_policies.yml
- server/api/internal/adapter/gql/resolver_mutation_project.go
- server/api/internal/adapter/gql/loader_deployment.go
- server/api/internal/usecase/interfaces/parameter.go
- server/api/internal/app/main.go
- server/api/internal/usecase/interfaces/projectAccess.go
- server/api/internal/usecase/interactor/parameter.go
- server/api/internal/usecase/interfaces/trigger.go
- server/api/internal/usecase/interfaces/job.go
- server/api/internal/adapter/gql/loader_asset.go
- server/api/internal/usecase/interactor/trigger.go
- server/api/internal/usecase/interactor/parameter_test.go
- server/api/internal/adapter/gql/resolver_mutation_parameter.go
- server/api/internal/usecase/interfaces/deployment.go
🧰 Additional context used
🧠 Learnings (2)
server/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`.
server/api/internal/usecase/interactor/deployment.go (1)
Learnt from: akiyatomohiro
PR: reearth/reearth-flow#580
File: api/internal/usecase/interactor/deployment.go:48-70
Timestamp: 2025-01-21T10:53:31.572Z
Learning: The Fetch and Find methods in the deployment service require permission checks using the new permission checker mechanism after migrating away from operator-based checks.
🔇 Additional comments (37)
server/api/internal/app/usecase.go (2)
15-15: LGTM! Function signature updated to support centralized permission checking.The addition of the
permissionCheckerparameter aligns with the PR's objective to enhance authorization capabilities across all interactors.
17-19: LGTM! Implementation simplified by removing operator-based filtering.The code has been streamlined by:
- Removing complex operator-based filtering logic
- Directly passing repositories to the interactor
- Using a centralized permission checking mechanism
server/api/internal/usecase/interactor/common.go (3)
5-5: LGTM!The new imports are necessary for error handling and auth info retrieval.
Also applies to: 7-7
29-47: LGTM!The changes consistently integrate permission checking across all components, aligning with the previous feedback about passing permissionChecker directly.
54-65: Document or remove the unused force parameter.The
forceparameter is not utilized in the implementation. Consider either:
- Documenting its intended purpose and implementing the functionality, or
- Removing it if it's no longer needed
server/api/e2e/gql_projectAccess_test.go (1)
96-144:❓ Verification inconclusive
Verify test coverage for unauthorized access scenarios.
A test case for unauthorized project sharing has been removed. Given that this PR focuses on authorization and roles management, we should ensure that these scenarios are still being tested, possibly in a different way with the new Cerbos-based permission checking.
Let's verify if similar test cases exist elsewhere:
🏁 Script executed:
#!/bin/bash # Description: Search for test cases related to unauthorized project access # Look for test cases involving project access and permissions rg -A 10 "TestProject.*" "server/api/e2e/" # Look for test cases involving Cerbos or permissions rg -A 10 "Test.*Permission" "server/api/e2e/"Length of output: 3481
Action: Confirm Unauthorized Access Testing for Project Sharing
It appears that explicit test cases for unauthorized project sharing scenarios are missing from the current e2e test suite. Given that the removed test case was intended to cover unauthorized access — and considering this PR focuses on updating authorization and roles (via Cerbos-based permission checks) — please verify that these security scenarios are adequately covered. If they aren’t, kindly add the necessary tests.
- Ensure new Cerbos-based permission tests include scenarios where unauthorized sharing attempts fail.
- If such tests exist elsewhere, update the documentation or comments to clearly point to their locations.
server/api/internal/usecase/interactor/usecase_test.go (2)
31-31: LGTM! Clean and consistent parameter updates.The removal of the
nilargument fromRun3andRun0function calls simplifies the code while maintaining the test coverage. This change aligns well with the broader refactoring towards centralized permission checking.Also applies to: 47-47, 59-59, 74-74, 89-89
1-98:❓ Verification inconclusive
Verify the migration of permission checking tests.
The removal of
TestUc_checkPermissionaligns with the centralization of permission checking. However, we should verify that the permission checking functionality is adequately tested in the new implementation.Let's search for the new permission checking tests:
🏁 Script executed:
#!/bin/bash # Search for permission-related test files echo "Searching for permission checker test files..." rg -l "TestPermission|permission.*test" --type go # Search for permission checker implementations echo -e "\nSearching for permission checker implementations..." ast-grep --pattern 'type $_ struct { $$$ permissionChecker $_ $$$ }'Length of output: 5689
Permission-checker test migration verification required
The removal of
TestUc_checkPermissionis consistent with centralizing permission checks. However, our repository search indicates that there are no dedicated permission-check tests (e.g., files or functions named with "TestPermission" or similar). Instead, the permission-checking mechanism is embedded within several usecase implementations (see files such asasset.go,deployment.go,trigger.go,job.go,projectAccess.go,parameter.go, andproject.go). Please double-check that the integrated tests covering these components sufficiently validate the permission-checking functionality. If gaps exist, consider adding focused tests for the permission checker.server/api/internal/usecase/interactor/usecase.go (3)
58-66:❓ Verification inconclusive
Verify the removal of operator parameter impact.
The removal of the operator parameter aligns with the centralized permission checking approach. However, ensure that all callers of this generic function have been updated to use the new permission checking mechanism.
Run this script to find all callers of
Run2that might need updates:
🏁 Script executed:
#!/bin/bash # Search for calls to Run2 that might still pass the operator parameter ast-grep --pattern 'Run2($$$, op, $$$)'Length of output: 41
Manual Verification for Operator Parameter Removal
The removal of the operator parameter aligns with the shift towards centralized permission checking. However, the initial automated search did not conclusively report any caller still passing the operator parameter. Please manually verify that all call sites invoking
Run2have been updated accordingly:
- Verify that no callers are using the operator parameter.
- Ensure all call sites have been refactored to leverage the new permission checking mechanism.
48-56:❓ Verification inconclusive
Verify the removal of operator parameter impact.
The removal of the operator parameter aligns with the centralized permission checking approach. However, ensure that all callers of this generic function have been updated to use the new permission checking mechanism.
Run this script to find all callers of
Run1that might need updates:
🏁 Script executed:
#!/bin/bash # Search for calls to Run1 that might still pass the operator parameter ast-grep --pattern 'Run1($$$, op, $$$)'Length of output: 41
Verify removal of operator parameter in Run1 invocations.
The operator parameter removal aligns with our centralized permission checking mechanism. Although our initial automated search using AST patterns produced no results, this outcome might be due to limited pattern matching. Please manually verify that none of the callers of
Run1are still passing an operator (e.g., usinggrep "Run1(" | grep -E "op"). Ensure all call sites have been updated to match the revised function signature.
38-46:❓ Verification inconclusive
Verify the removal of operator parameter impact.
The removal of the operator parameter aligns with the centralized permission checking approach. However, ensure that all callers of this function have been updated to use the new permission checking mechanism.
Run this script to find all callers of
Run0that might need updates:
🏁 Script executed:
#!/bin/bash # Search for calls to Run0 that might still pass the operator parameter ast-grep --pattern 'Run0($$$, op, $$$)'Length of output: 41
Operator Parameter Removal Impact Review
- The migration to remove the operator parameter appears aligned with the centralized permission checking strategy.
- The initial AST grep search for calls passing an operator (
Run0($$$, op, $$$)) produced no output, suggesting no outdated call sites were found.- However, because the search did not return any definitive results, please ensure to manually verify (or run additional queries) that all invocations of
Run0adhere to the new signature.server/api/internal/usecase/operator.go (1)
1-11:❓ Verification inconclusive
Verify the migration progress.
Let's verify the status of the migration to ensure this file can be safely deleted:
🏁 Script executed:
#!/bin/bash # Description: Check the migration progress by: # 1. Finding remaining usages of the Operator struct # 2. Verifying the implementation of new permission checking mechanisms echo "Checking remaining usages of Operator struct..." rg -l "type.*Operator.*struct" --type go echo "\nChecking new permission checking implementations..." rg -l "checkPermission" --type go echo "\nChecking Cerbos policy files..." fd -e yaml -e yml . | grep -i "cerbos"Length of output: 911
Migration progress verification for Operator deletion and permission improvements
- The
Operatorstruct appears only inserver/api/internal/usecase/operator.go, with no other references detected.- New permission checking implementations using
checkPermissionare present in various interactor files.- No Cerbos policy files (YAML/YML) were found. Please verify manually whether Cerbos configurations are managed differently or confirm that the migration to Cerbos is complete.
server/api/internal/rbac/definitions.go (4)
1-5: LGTM! Package and imports are well-structured.The package name appropriately reflects its RBAC functionality, and the import is correctly specified.
41-45: LGTM! Good defensive programming with nil check.The nil check for the builder parameter is a good practice.
46-66: Review broad permissions granted by ActionAny.Several resources (Asset, Deployment, Job, Parameter) only have 'ActionAny' permission for maintainers. This might be too permissive and should be more granular.
Consider breaking down permissions into specific actions:
AddResource(ResourceAsset, []generator.ActionDefinition{ - generator.NewActionDefinition(ActionAny, []string{ - roleMaintainer, - }), + generator.NewActionDefinition(ActionRead, []string{ + roleReader, + roleEditor, + roleMaintainer, + }), + generator.NewActionDefinition(ActionCreate, []string{ + roleEditor, + roleMaintainer, + }), + generator.NewActionDefinition(ActionEdit, []string{ + roleEditor, + roleMaintainer, + }), + generator.NewActionDefinition(ActionDelete, []string{ + roleMaintainer, + }), })
67-89: Review inconsistent role assignments in Project resource.The Project resource has some potential issues:
- ActionList is only available to 'self' but ActionAny includes reader/editor
- ActionAny might override more specific permissions
Consider reorganizing permissions to be more consistent:
AddResource(ResourceProject, []generator.ActionDefinition{ generator.NewActionDefinition(ActionList, []string{ - roleSelf, + roleSelf, + roleReader, + roleEditor, + roleMaintainer, + roleOwner, }), generator.NewActionDefinition(ActionCreate, []string{ roleMaintainer, roleOwner, }), generator.NewActionDefinition(ActionEdit, []string{ + roleEditor, roleMaintainer, roleOwner, }), generator.NewActionDefinition(ActionDelete, []string{ roleMaintainer, roleOwner, }), - generator.NewActionDefinition(ActionAny, []string{ - roleReader, - roleEditor, - roleOwner, - roleMaintainer, - }), })server/api/internal/adapter/gql/resolver_mutation_job.go (2)
10-22: LGTM! Clean implementation of the CancelJob resolver.The changes align well with the broader refactoring goals of centralizing permission checks and removing legacy operator parameters. The implementation is clean and follows the consistent pattern applied across other resolvers.
16-16:❓ Verification inconclusive
Verify authorization checks in the Job.Cancel usecase.
The removal of
getOperator(ctx)aligns with the broader refactoring to use a centralized permission checker. However, we should verify that proper authorization checks are implemented in the Job usecase layer.Let's verify the implementation of authorization checks in the Job usecase:
🏁 Script executed:
#!/bin/bash # Search for permission checks in the Job usecase implementation ast-grep --pattern 'func (uc *JobUsecase) Cancel(ctx context.Context, $$$) { $$$ }' # Search for checkPermission calls within the Cancel method rg -A 5 'checkPermission.*Cancel' server/api/internal/usecase/Length of output: 152
Action Required: Verify Authorization in Cancel Method
The removal of
getOperator(ctx)aligns with the refactoring toward a centralized permission checker, but our search did not return evidence of explicit authorization checks (e.g., calls tocheckPermission) in the Job usecase'sCancelmethod. Please manually verify that the authorization is properly handled in the cancellation logic.
- File under review:
server/api/internal/adapter/gql/resolver_mutation_job.go(call toJob.Cancel)- Action: Confirm the implementation of proper authorization checks within the
Cancelmethod in the usecase layer.
🏁 Script executed:
[run_scripts] #!/bin/bash # Check for the definition of the Cancel method in Job usecase implementations rg -n 'func\s\*\?JobUsecase\s.*Cancel' server/api/internal/usecase/ # Check explicitly for any usage of checkPermission within all usecase files rg -n 'checkPermission' server/api/internal/usecase/Length of output: 8311
Verify authorization in job cancellation
The removal of
getOperator(ctx)is in line with the move toward a centralized permission checker. However, our search did not uncover an explicit authorization check (such as a call tocheckPermission) within the Job usecase'sCancelmethod. Please manually verify that the cancellation flow properly enforces permission checks before executing the cancellation logic.
- File under review:
server/api/internal/adapter/gql/resolver_mutation_job.go(line 16)- Action: Confirm that the implementation of
Cancelin the Job usecase includes the necessary authorization checks. If the permission validation is missing, add the required logic to ensure secure cancellation..github/workflows/ci.yml (9)
6-12: Introduce manual workflow dispatch inputforce_update_policies.
This change leverages theworkflow_dispatchevent to allow manual triggers so that users can force a policy update when needed. The input is clearly defined as a boolean with a default value offalse, which aligns well with the PR objectives.
20-20: Add output for policy file changes.
The addition ofpolicies: ${{ steps.policies.outputs.any_changed }}within thepreparejob ensures that any modifications in policy-related files are captured. This output is crucial for conditionally triggering subsequent jobs that depend on policy updates.
32-39: Update file glob patterns for API changes detection.
The updatedfilesblock now includes directories such asserver/api/**,engine/schema/**, and several CI workflow files. This inclusive pattern matching helps in accurately tracking changes related to API modifications. Please verify that the inclusion ofengine/schema/**is intentional.
41-50: Configure file glob patterns for policy-related changes.
This segment refines the detection of changes by monitoring dedicated policy directories and associated workflow files (e.g.,check_cerbos_policies.yml,update_cerbos_policies.yml). This targeted configuration supports the modularization of policy validations within the CI process.
101-104: Addci-policiesjob for policy checks.
The new jobci-policiesutilizes the output from thepreparejob to run only when policy-related files have changed. This modular approach is effective for isolating policy validations and streamlining the overall CI workflow.
110-112: Refactorci-websocketjob indentation and trigger condition.
The adjustments in theci-websocketjob, including proper indentation and clear dependency on the websocket change output, improve readability and maintain consistency with other jobs.
122-122: Includeci-policiesas a dependency for the consolidatedcijob.
By addingci-policiesto the list of dependencies for thecijob, the workflow now ensures that successful policy validations are part of the overall CI status.
169-182: Implement conditional execution for theupdate-policiesjob.
This condition is well-structured to execute theupdate-policiesjob only if:
- All preceding jobs (especially the policy check) are successful.
- The event meets the criteria (a push to
main/releasebranches) or the user opts for manual triggering via theforce_update_policiesinput.
This logic enhances control over policy updates while integrating security checks from upstream steps.
196-196: Consistent use of secrets inheritance in deployment jobs.
The addition ofsecrets: inheritin thebuild-and-deploy-apijob ensures that workflow-level secrets are correctly passed down, maintaining security best practices across deployment stages.server/api/internal/usecase/interactor/project_test.go (2)
20-29: LGTM! Well-structured helper function.The
setupProjecthelper function is a good addition that reduces code duplication and improves test maintainability.
48-108: LGTM! Well-organized test cases.The table-driven test approach is well-implemented with:
- Clear test case names
- Comprehensive scenarios (normal, error cases)
- Proper error handling and assertions
server/api/internal/usecase/interactor/projectAccess.go (1)
37-39: LGTM! Clean permission check implementation.The
checkPermissionmethod provides a clean abstraction for permission validation.server/api/internal/usecase/interactor/project.go (1)
43-45: LGTM! Clean permission check implementation.The
checkPermissionmethod provides a clean abstraction for permission validation.server/api/internal/usecase/interactor/projectAccess_test.go (1)
21-23: LGTM! Clean mock implementation.The mock permission checker is well-implemented with a clear function signature.
server/api/internal/usecase/interactor/job.go (2)
23-33: LGTM! Constructor properly initializes the permission checker.The changes correctly integrate the permission checker into the Job struct and its constructor.
Also applies to: 43-54
57-59: LGTM! Permission checking is properly implemented.The method follows the established pattern for permission checking and correctly uses the job resource type.
server/api/internal/usecase/interactor/deployment.go (3)
23-34: LGTM! Constructor properly initializes the permission checker.The changes correctly integrate the permission checker into the Deployment struct and its constructor.
Also applies to: 36-49
51-54: LGTM! Permission checking is properly implemented.The method follows the established pattern for permission checking and correctly uses the deployment resource type.
292-296:❓ Verification inconclusive
Consider consolidating permission checks.
The method performs two permission checks:
- For deployment execution (line 293)
- For job monitoring through
i.job.StartMonitoring(line 354)Since both checks use
rbac.ActionAny, consider whether both are necessary or if one comprehensive check would suffice.Also applies to: 354-356
🏁 Script executed:
#!/bin/bash # Check if there are other places where both deployment and job permissions are checked rg -A 5 "checkPermission.*rbac\.ResourceDeployment.*rbac\.ActionAny" | rg -A 5 "checkPermission.*rbac\.ResourceJob.*rbac\.ActionAny"Length of output: 134
Double-check redundant permission checks in deployment execution versus job monitoring.
The
Executemethod currently invokes two permission checks usingrbac.ActionAny:
- An initial check at the beginning of the method (around line 293).
- Another check when calling
i.job.StartMonitoring(around line 354).Please verify manually whether both checks are genuinely necessary. If the permission requirements are equivalent, consider consolidating them into a single check to remove potential redundancy. Otherwise, ensure that subtle differences in context justify retaining both.
Overview
What I've done
ref: feat(api): authorization-and-roles-management-service #580
fix cerbos workflow
There was a lint error, but it is not necessary in the policyfile.
The yml file has been confirmed to work properly.
ref: https://eukarya-inc.slack.com/archives/C06GWJ9R8RH/p1739388687593309?thread_ts=1739167148.679479&cid=C06GWJ9R8RH
add checkPermission to all interactors
fix fetch workspace error
ref: https://eukarya-inc.slack.com/archives/C064TUMGNDA/p1739983151112709?thread_ts=1739394205.301159&cid=C064TUMGNDA
・run
cargo set-version --bump patchIs it correct?
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Refactor