feat(organization): add suspension support#3007
feat(organization): add suspension support#3007Piskoo wants to merge 2 commits intochainloop-dev:mainfrom
Conversation
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
There was a problem hiding this comment.
1 issue found across 26 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/controlplane/pkg/authz/authz.go">
<violation number="1" location="app/controlplane/pkg/authz/authz.go:500">
P2: `regexp.MatchString` recompiles the pattern on every call. Since this runs in a request-hot loop over all `ServerOperationsMap` keys, precompile the regex patterns once at package init. Currently every OrgMetrics API call (and any unmapped operation) triggers ~55 regex compilations.
Consider extracting regex-pattern keys into a separate `map[*regexp.Regexp][]*Policy` built at init time, or precompile them with `regexp.MustCompile` in a package-level variable, similar to how `suspensionExemptRegexp` is handled in the suspension middleware.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
|
||
| // Second pass: regex match | ||
| for k, policies := range ServerOperationsMap { | ||
| found, _ := regexp.MatchString(k, apiOperation) |
There was a problem hiding this comment.
P2: regexp.MatchString recompiles the pattern on every call. Since this runs in a request-hot loop over all ServerOperationsMap keys, precompile the regex patterns once at package init. Currently every OrgMetrics API call (and any unmapped operation) triggers ~55 regex compilations.
Consider extracting regex-pattern keys into a separate map[*regexp.Regexp][]*Policy built at init time, or precompile them with regexp.MustCompile in a package-level variable, similar to how suspensionExemptRegexp is handled in the suspension middleware.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/controlplane/pkg/authz/authz.go, line 500:
<comment>`regexp.MatchString` recompiles the pattern on every call. Since this runs in a request-hot loop over all `ServerOperationsMap` keys, precompile the regex patterns once at package init. Currently every OrgMetrics API call (and any unmapped operation) triggers ~55 regex compilations.
Consider extracting regex-pattern keys into a separate `map[*regexp.Regexp][]*Policy` built at init time, or precompile them with `regexp.MustCompile` in a package-level variable, similar to how `suspensionExemptRegexp` is handled in the suspension middleware.</comment>
<file context>
@@ -475,3 +480,28 @@ func (Role) Values() (roles []string) {
+
+ // Second pass: regex match
+ for k, policies := range ServerOperationsMap {
+ found, _ := regexp.MatchString(k, apiOperation)
+ if found {
+ return policies, nil
</file context>
| // This covers two cases: | ||
| // - Empty-policy reads: operations mapped with {} in ServerOperationsMap that are actually reads | ||
| // - Self-service writes: operations the user needs to leave/delete even when suspended | ||
| var suspensionExemptRegexp = regexp.MustCompile(`controlplane.v1.CASCredentialsService/Get|controlplane.v1.UserService/(ListMemberships|SetCurrentMembership|DeleteMembership)|controlplane.v1.GroupService/(List|Get|ListMembers|ListProjects|ListPendingInvitations)|controlplane.v1.AuthService/DeleteAccount|controlplane.v1.OrganizationService/Delete$`) |
There was a problem hiding this comment.
This is fragile and difficult to maintain if we add more endpoints. What if instead we extend the endpoint info in authz.go to add whether an endpoint is mutating or not? This would require adding all endpoints there though (default to mutating=true for maximum security if it's not found)
Another option is trying to get the metadata from the proto definitions, but I don't think Kratos exposes that.
There was a problem hiding this comment.
Nevermind, I've just seen that logic below. Thanks
|
I have mixed feelings about this feature; I’m not sure why we need to support read-only. |
Summary
Added a
suspendedfield to the Organization schema and a suspension middleware that enforces read-only mode when an org is suspended. The middleware classifies operations usingServerOperationsMappolicy actions — read/list actions are allowed, everything else is blocked by default. An exempt regex covers empty-policy reads that lack action metadata and self-service writes so users can still leave or delete a suspended org.Examples
Write fails
Read goes through
Closes #3005