Skip to content

Conversation

jiparis
Copy link
Member

@jiparis jiparis commented Jun 27, 2025

This small patch adds some flexibility to the RBAC policies syncer so that it doesn't delete policies for "unknown" resources. This allows other sources for policies to coexist seamlessly.
The Enforcer can now be configured with a custom set of role mappings and managed resources.

Refs #2121

jiparis added 3 commits June 27, 2025 01:49
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
@jiparis jiparis requested review from migmartri, javirln and Copilot June 27, 2025 07:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a Config parameter to the RBAC enforcer to limit policy deletions to known resources, letting external policy sources coexist.

  • Introduces authz.Config with ManagedResources and RolesMap
  • Updates all New*Enforcer and doSync calls to accept and use the new config
  • Adjusts tests and wiring code to pass default or custom configs

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
app/controlplane/pkg/authz/authz.go Added Config struct, ManagedResources, updated doSync logic
app/controlplane/pkg/authz/authz_test.go Passed &Config{RolesMap: RolesMap} into sync calls
app/controlplane/pkg/authz/authz_integration_test.go Updated enforcer instantiation to include empty &Config{}
app/controlplane/pkg/biz/testhelpers/wire.go Added local authzConfig helper
app/controlplane/pkg/biz/testhelpers/wire_gen.go Added local authzConfig helper
app/controlplane/cmd/wire.go Added local authzConfig helper
app/controlplane/cmd/wire_gen.go Added local authzConfig helper
Comments suppressed due to low confidence (2)

app/controlplane/pkg/authz/authz.go:522

  • The call to RemovePolicy uses a slice of strings (gotPolicies) rather than passing the individual role, resource, action arguments. Consider deconstructing the slice and calling e.RemovePolicy(role, resource, action) for consistency and to match the method signature.
		wantPolicies, ok := config.RolesMap[Role(role)]

app/controlplane/pkg/authz/authz.go:43

  • [nitpick] The new Config struct should have a doc comment explaining its purpose (e.g., default behavior, fields semantics). Adding GoDoc will improve maintainability and clarity for future contributors.
type Config struct {

jiparis added 2 commits June 27, 2025 13:55
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
@jiparis jiparis changed the title feat(rbac): sync only known resource policies feat(rbac): sync only managed resource policies Jun 27, 2025
@jiparis
Copy link
Member Author

jiparis commented Jun 27, 2025

Added some tests for collisions

jiparis added 2 commits June 27, 2025 16:01
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
@jiparis jiparis merged commit 23c5ef1 into chainloop-dev:main Jun 27, 2025
13 checks passed
@jiparis jiparis deleted the PFM-3194-authsync branch June 27, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants