[BB-1829] business plus support #64
Conversation
WalkthroughReplaces Enterprise Grid–centric APIs with a Business+ (SCIM) client: removes enterprise workspace/role resources and interfaces, rewires builders/connectors to use a businessPlus SCIM client, centralizes SCIM requests and error wrapping, and updates configuration to use BusinessPlusToken. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Config as Config
participant Connector as Connector / ResourceSyncer
participant Client as BusinessPlus SCIM Client
participant SlackAPI as Slack SCIM API
Config->>Connector: provide BusinessPlusToken
Connector->>Client: NewClient(token)
Connector->>Client: ListIDPUsers(startIndex, count)
Client->>SlackAPI: GET /scim/{org}/Users (Authorization: token)
SlackAPI-->>Client: 200 OK (SCIM users + pagination)
Client-->>Connector: SCIMResponse[UserResource] + rateLimit
Connector->>Connector: map SCIM users -> internal Resources
Connector->>Config: SetWorkspaceNames(session store)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/connector/client/models.go (1)
89-89: Incorrect type forRolesfield.The
Rolesfield is typed asvalue,display, andtypefields, not email-specific fields likeprimary. Consider defining a dedicatedRoletype or reusingGroupif the structure matches.+type Role struct { + Value string `json:"value"` + Display string `json:"display,omitempty"` + Type string `json:"type,omitempty"` + Primary bool `json:"primary,omitempty"` +} + type UserResource struct { // ... other fields ... - Roles []Email `json:"roles"` + Roles []Role `json:"roles"` // ... other fields ... }pkg/connector/client/request.go (1)
100-100: Redundant address-of operator ontarget.Taking the address of
target(already aninterface{}) creates a double pointer if the caller passes a pointer type. This can cause unmarshaling issues. The same pattern appears inpatchScimBytes(line 118) andpatchScim(line 137).func (c *Client) getScim( ctx context.Context, path string, target interface{}, queryParameters map[string]interface{}, ) ( *v2.RateLimitDescription, error, ) { return c.doRequest( ctx, http.MethodGet, c.getUrl(path, queryParameters, true), - &target, + target, WithBearerToken(c.token), ) }Apply the same fix to
patchScimBytes(line 118) andpatchScim(line 137).pkg/connector/user.go (1)
29-92: Performance impact of additional per-user API calls in SCIM flow is real but architectural constraints limit optimization.
scimUserResourcefetches full user info viaGetUserInfoContextfor each SCIM user because the SCIM API (ListIDPUsers) returns only limited user data (name, emails, etc.) and lacks critical fields like admin status, bot flags, MFA status, and user profile details.The existing comment at line 30 correctly identifies this as a compatibility measure. However:
- Batching is not viable: Slack API provides no batch
users.infoendpoint- Using SCIM data directly is not viable: SCIM UserResource lacks IsAdmin, IsOwner, IsBot, Has2FA, Profile fields, and other required attributes
- Consider documenting: If this is planned to be addressed, clarify the timeline or constraints preventing optimization
If performance becomes critical, the only architectural alternative would be switching from the SCIM provisioning API to the standard
users.listAPI for the entire user sync flow.
🧹 Nitpick comments (10)
pkg/connector/actions.go (1)
122-164: Consider consolidating the duplicate handler logic.The
handleDisableUserandhandleEnableUserfunctions share nearly identical structure—they differ only in the operation name and the client method called. This duplication could be reduced by extracting a common handler function.Example refactor to reduce duplication:
// handleUserStatusChange is a generic handler for enable/disable operations func (s *Slack) handleUserStatusChange( ctx context.Context, args *structpb.Struct, operation string, clientFunc func(context.Context, string) (*v2.RateLimitDescription, error), ) (*structpb.Struct, annotations.Annotations, error) { l := ctxzap.Extract(ctx) userIDValue, ok := args.Fields["user_id"] if !ok { return nil, nil, fmt.Errorf("user_id parameter is required") } userID := userIDValue.GetStringValue() if userID == "" { return nil, nil, fmt.Errorf("user_id cannot be empty") } l.Debug(fmt.Sprintf("%s user via SCIM", operation), zap.String("user_id", userID)) if s.businessPlusClient == nil { return nil, nil, fmt.Errorf("business+ client not available - SCIM API requires Business+ plan") } ratelimitData, err := clientFunc(ctx, userID) if err != nil { l.Error(fmt.Sprintf("failed to %s user", operation), zap.String("user_id", userID), zap.Error(err)) return nil, nil, fmt.Errorf("failed to %s user %s: %w", operation, userID, err) } outputAnnotations := annotations.New() if ratelimitData != nil { outputAnnotations.WithRateLimiting(ratelimitData) } l.Info(fmt.Sprintf("user %s successfully", operation), zap.String("user_id", userID)) return &structpb.Struct{ Fields: map[string]*structpb.Value{ "success": {Kind: &structpb.Value_BoolValue{BoolValue: true}}, "message": {Kind: &structpb.Value_StringValue{StringValue: fmt.Sprintf("User %s %s successfully", userID, operation)}}, "user_id": {Kind: &structpb.Value_StringValue{StringValue: userID}}, }, }, outputAnnotations, nil } // handleDisableUser deactivates a Slack user by setting active to false via SCIM API. func (s *Slack) handleDisableUser(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { return s.handleUserStatusChange(ctx, args, "disable", s.businessPlusClient.DisableUser) } // handleEnableUser activates a Slack user by setting active to true via SCIM API. func (s *Slack) handleEnableUser(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { return s.handleUserStatusChange(ctx, args, "enable", s.businessPlusClient.EnableUser) }Also applies to: 166-209
pkg/connector/client/helpers.go (1)
11-25: Consider logging level and potential sensitive data exposure.The
logBodyhelper logs response body content atInfolevel. Response bodies may contain sensitive information (tokens, user data, error details with PII). Consider usingDebuglevel instead for production safety.- l.Info("response body: ", zap.String("body", string(body))) + l.Debug("response body: ", zap.String("body", string(body)))pkg/connector/client/request.go (2)
252-254: Simplify error creation.
fmt.Errorf("%s", detail)can be simplified toerrors.New(detail)since there's no formatting involved.+import "errors" + if detail, ok := errorResponse["detail"].(string); ok { - return &ratelimitData, uhttp.WrapErrors(codes.Internal, "SCIM API error", fmt.Errorf("%s", detail)) + return &ratelimitData, uhttp.WrapErrors(codes.Internal, "SCIM API error", errors.New(detail)) }
211-217: Minor: DuplicategetUrlcall.
getUrlis called twice indeleteScim- once for logging (line 211) and once for the request (line 217). Consider storing the result in a variable to avoid redundant computation.func (c *Client) deleteScim( ctx context.Context, path string, ) ( *v2.RateLimitDescription, error, ) { + deleteUrl := c.getUrl(path, nil, true) logger := ctxzap.Extract(ctx) logger.Debug( "making request", zap.String("method", http.MethodDelete), - zap.String("url", c.getUrl(path, nil, true).String()), + zap.String("url", deleteUrl.String()), ) request, err := c.wrapper.NewRequest( ctx, http.MethodDelete, - c.getUrl(path, nil, true), + deleteUrl, WithBearerToken(c.token), uhttp.WithAcceptJSONHeader(), )pkg/config/config.go (1)
23-28: Deprecated field still defined but unused.
SSOEnabledFieldis marked as deprecated but still defined in the package. Since it's not included inConfigurationFields(line 39-43), it won't be exposed to users. Consider removing this field definition entirely to avoid confusion, or document why it's retained (e.g., backward compatibility for existing configs).pkg/connector/connector.go (1)
123-135: Variable shadowing and redundant error prefix.Two minor issues:
- Line 125-126: The
errvariable is pre-declared on line 125, but line 126 uses:=which shadows it. Use=instead for clarity.- Line 133: The error message includes "Error:" which is redundant when using
%wformatting.var businessPlusClient *client.Client if businessPlusKey != "" { - var err error - businessPlusClient, err = client.NewClient( + var clientErr error + businessPlusClient, clientErr = client.NewClient( httpClient, businessPlusKey, apiKey, govEnv, ) - if err != nil { - return nil, fmt.Errorf("failed to create Business+ client. Error: %w", err) + if clientErr != nil { + return nil, fmt.Errorf("failed to create Business+ client: %w", clientErr) } }pkg/connector/workspace.go (1)
159-161: Potential double-wrapping of errors may overwrite gRPC codes.Based on the retrieved learnings, when client methods like
GetUsersalready wrap errors with specific gRPC codes viahandleError(), usinguhttp.WrapErrorsagain could overwrite those codes (e.g.,Unavailable,PermissionDenied) needed for proper retry logic and alerting.Consider using
fmt.Errorfwith%wto preserve the underlying error codes:if err != nil { - return nil, nil, uhttp.WrapErrors(codes.Internal, "fetching users for workspace", err) + return nil, nil, fmt.Errorf("fetching users for workspace: %w", err) }pkg/connector/user.go (2)
37-55: Code duplication in profile building.The profile map construction is nearly identical between
scimUserResource(lines 37-55) anduserResource(lines 100-118). Consider extracting a helper function to reduce duplication:func buildUserProfile(user *slack.User) map[string]interface{} { profile := make(map[string]interface{}) profile["first_name"] = user.Profile.FirstName // ... rest of profile fields return profile }This would also ensure both paths stay in sync if profile fields change.
Also applies to: 100-118
242-242: Consider using the PageSizeDefault constant.The count value is hardcoded to 100. Consider using
client.PageSizeDefaultfor consistency:- count := 100 + count := client.PageSizeDefaultpkg/connector/client/slack.go (1)
413-461: Consider using SCIM "remove" operation for efficiency.The current implementation fetches all members, filters locally, then replaces the entire member list. For large groups, this could be inefficient.
SCIM 2.0 supports a direct "remove" operation:
Operations: []ScimOperate{ { Op: "remove", Path: fmt.Sprintf("members[value eq \"%s\"]", user), }, }However, the current approach may be more compatible across SCIM implementations. If this is intentional, consider adding a comment explaining the design choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (37)
.github/workflows/ci.yamlis excluded by none and included by noneREADME.mdis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by nonevendor/github.com/davecgh/go-spew/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/bypass.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/bypasssafe.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/common.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/config.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/doc.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/dump.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/format.gois excluded by!vendor/**and included by nonevendor/github.com/davecgh/go-spew/spew/spew.gois excluded by!vendor/**and included by nonevendor/github.com/pmezard/go-difflib/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/pmezard/go-difflib/difflib/difflib.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/LICENSEis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_compare.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_format.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_format.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_forward.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_forward.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertion_order.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/assertions.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/doc.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/errors.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/forward_assertions.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/http_assertions.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/yaml/yaml_custom.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/yaml/yaml_default.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/assert/yaml/yaml_fail.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/doc.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/forward_requirements.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require_forward.gois excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/require_forward.go.tmplis excluded by!vendor/**and included by nonevendor/github.com/stretchr/testify/require/requirements.gois excluded by!vendor/**and included by nonevendor/modules.txtis excluded by!vendor/**and included by none
📒 Files selected for processing (21)
go.mod(1 hunks)pkg/config/conf.gen.go(1 hunks)pkg/config/config.go(2 hunks)pkg/connector/actions.go(2 hunks)pkg/connector/client/helpers.go(1 hunks)pkg/connector/client/mock.go(1 hunks)pkg/connector/client/models.go(1 hunks)pkg/connector/client/path.go(2 hunks)pkg/connector/client/request.go(6 hunks)pkg/connector/client/slack.go(5 hunks)pkg/connector/client/slack_interface.go(0 hunks)pkg/connector/connector.go(5 hunks)pkg/connector/enterprise_roles.go(0 hunks)pkg/connector/group.go(10 hunks)pkg/connector/resource_types.go(0 hunks)pkg/connector/roles.go(0 hunks)pkg/connector/user.go(4 hunks)pkg/connector/user_group.go(3 hunks)pkg/connector/workspace.go(5 hunks)pkg/connector/workspace_test.go(0 hunks)pkg/helpers.go(4 hunks)
💤 Files with no reviewable changes (5)
- pkg/connector/client/slack_interface.go
- pkg/connector/workspace_test.go
- pkg/connector/resource_types.go
- pkg/connector/roles.go
- pkg/connector/enterprise_roles.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/actions.gopkg/connector/connector.gopkg/connector/client/request.gopkg/connector/client/models.gopkg/connector/workspace.gopkg/connector/client/path.gopkg/helpers.gopkg/connector/user_group.gopkg/connector/group.gopkg/connector/client/slack.go
🧬 Code graph analysis (4)
pkg/connector/user_group.go (3)
pkg/connector/client/slack.go (1)
Client(32-39)pkg/connector/connector.go (1)
New(145-158)pkg/helpers.go (1)
WrapError(173-222)
pkg/connector/group.go (3)
pkg/connector/client/slack.go (2)
Client(32-39)PageSizeDefault(21-21)pkg/connector/client/models.go (1)
GroupResource(148-154)pkg/connector/connector.go (1)
New(145-158)
pkg/connector/client/slack.go (2)
pkg/connector/client/models.go (3)
SCIMResponse(18-24)GroupResource(148-154)UserResource(72-94)pkg/connector/client/path.go (2)
UrlPathIDPGroups(22-22)UrlPathIDPUsers(24-24)
pkg/connector/user.go (3)
pkg/connector/client/slack.go (1)
Client(32-39)pkg/connector/client/models.go (4)
UserResource(72-94)Name(118-122)User(33-59)pkg/helpers.go (2)
WrapError(173-222)AnnotationsForError(227-242)
🔇 Additional comments (37)
pkg/connector/actions.go (2)
140-148: Client migration to businessPlusClient is correct.The switch from
enterpriseClienttobusinessPlusClientis straightforward and verified. ThebusinessPlusClientfield exists on the Slack struct as*client.Client, and bothDisableUser()andEnableUser()methods exist with the correct signatures accepting(ctx context.Context, userID string)and returning(*v2.RateLimitDescription, error).Error handling correctly uses
fmt.Errorfwith%wto preserve gRPC error codes from the client's internal error wrapping, ensuring proper retry logic and alerting. This approach aligns with best practices for error propagation.Both handlers follow identical patterns and could benefit from optional refactoring to reduce duplication, but the current implementation is clear and functional.
185-193: Client migration looks correct.The changes mirror those in
handleDisableUserand maintain consistency across parameter validation, error handling, and response formatting. The error handling correctly preserves wrapped error codes using the%wformat specifier, and theEnableUsermethod signature on the business plus client is confirmed.pkg/connector/client/mock.go (1)
9-21: Remove unused mock implementation or implement the corresponding interface.The
MockSlackEnterpriseServiceis dead code—it's never imported or used anywhere in the codebase, and it references methods (AddUser,RemoveUser) and an interface (SlackEnterpriseService) that don't exist. Either remove this mock or implement the interface it's intended to mock.Likely an incorrect or invalid review comment.
pkg/connector/client/helpers.go (1)
1-1: Package rename aligns with the Business+ refactor.The package declaration change from
enterprisetoclientis consistent with the broader migration.pkg/connector/client/models.go (2)
1-1: Package rename consistent with the refactor.The package declaration aligns with the migration to the Business+ client architecture.
156-165: TheScimOperatestruct is used exclusively for group membership operations in the codebase, where theValuefield always contains a[]UserIDarray. The current typing is appropriate and does not impose unnecessary constraints.go.mod (2)
87-87: Indirect dependency addition looks reasonable.The
stretchr/objxpackage is typically pulled in as an indirect dependency oftestify/mock. This addition appears consistent with the dependency tree.
3-3: Go 1.25.2 is a valid release version. As of December 2025, Go 1.25.5 is the latest stable release, making 1.25.2 a standard patch release in the current 1.25 series.pkg/connector/client/request.go (2)
1-1: Package rename consistent with refactor.The package declaration aligns with the Business+ client migration.
171-173: Error wrapping approach is appropriate.Using
uhttp.WrapErrorsfor errors generated directly in request handling (HTTP creation, body reading, unmarshaling) is correct per the established pattern. This preserves proper error categorization for retry logic. Based on learnings.pkg/connector/client/path.go (1)
1-25: Path constants properly streamlined for Business+ architecture.The removal of enterprise-specific paths (role assignments, admin operations) and retention of standard Slack API and SCIM paths aligns well with the Business+ refactor. The SCIM paths correctly use format strings for team ID substitution.
pkg/helpers.go (2)
171-222: LGTM! Function rename and error handling pattern are appropriate.The rename from
WrapSlackClientErrortoWrapErrorsimplifies the API. The function correctly handles rate limit errors withcodes.ResourceExhaustedand maps other Slack errors using the centralizedMapSlackErrorToGRPCCodefunction.
19-76: Error wrapping follows the established pattern.Using
uhttp.WrapErrorsdirectly for validation errors (InvalidArgumentinParseID,ParseRole,ParsePageToken) and internal errors (InternalinMakeResourceList) is correct. Based on learnings, this approach is appropriate for errors generated directly in these functions rather than from Slack client calls.pkg/config/config.go (1)
17-22: Configuration field and relationships are properly defined.The
BusinessPlusTokenFieldis correctly marked as a secret, and the description accurately reflects its purpose for both Business+ and Enterprise Grid contexts. TheFieldRelationshipscorrectly enforces thatGovEnvironmentFielddepends onBusinessPlusTokenField.Also applies to: 47-53
pkg/config/conf.gen.go (1)
6-10: Generated struct aligns with configuration changes.The
Slackstruct fields (Token,BusinessPlusToken,GovEnv) correctly reflect the updatedConfigurationFieldsinconfig.go. Since this is generated code, no manual changes should be made here.pkg/connector/user_group.go (2)
82-97: Error handling follows the correct pattern.Using
pkg.WrapErrorfor the Slack client call (line 84) preserves the appropriate gRPC codes, whileuhttp.WrapErrorswithcodes.Internal(line 92) is correct for resource conversion errors generated within this function. Based on learnings, this pattern avoids double-wrapping issues.
133-165: Grants method correctly handles errors from Slack API calls.The error wrapping is consistent:
pkg.WrapErrorfor Slack client calls (GetUserGroupMembersContext,GetUserInfoContext) anduhttp.WrapErrorsfor internal resource creation. This preserves the proper gRPC codes for retry logic.pkg/connector/connector.go (3)
72-93: Validate method follows correct error handling patterns.Slack client errors (
AuthTestContext,GetUserInfoContext) are wrapped withpkg.WrapErrorto preserve gRPC codes, while the permission validation error usesuhttp.WrapErrorsdirectly. This aligns with the established pattern.
160-167: ResourceSyncers correctly wires the Business+ client to all builders.The builders receive the appropriate clients based on their needs. The
groupBuilderonly needsbusinessPlusClient(likely for SCIM operations), while other builders need both the standard Slack client and the Business+ client.
145-158: Config mapping correctly uses the newBusinessPlusTokenfield.The
Newfunction properly passesconfig.BusinessPlusTokentoNewSlack, aligning with the configuration field rename fromEnterpriseTokentoBusinessPlusToken.pkg/connector/workspace.go (3)
21-40: LGTM - Clean struct and builder refactor.The struct and builder have been correctly updated to use the Business+ client pattern, replacing the enterprise client references.
68-110: LGTM - List method with appropriate error handling.The List method correctly uses
pkg.WrapErrorfor Slack API errors (preserving gRPC codes) anduhttp.WrapErrorsfor local errors like token parsing and resource creation. This aligns with the established error handling pattern.
187-189: Good documentation on API limitations.The comment clearly explains the Enterprise Grid API requirement for workspace membership management, which is helpful for future maintainers.
pkg/connector/group.go (5)
26-42: LGTM - Clean struct and builder refactor for groups.The struct fields and builder signature have been correctly simplified to use the Business+ client pattern.
96-136: LGTM - List method with proper nil check and error handling.The graceful degradation when
businessPlusClientis nil and the error wrapping pattern are correct. SinceListIDPGroupsusesfmt.Errorf(nothandleError), applyinguhttp.WrapErrorshere is appropriate.
171-204: LGTM - Grants method correctly handles IDP group members.The error wrapping is appropriate since
GetIDPGroupusesfmt.Errorfwithout gRPC codes.
206-250: LGTM - Grant method with proper precondition checks.The precondition checks for nil client, gov environment, and principal type are well-structured. Error wrapping is appropriate.
252-303: LGTM - Revoke method mirrors Grant structure.Consistent precondition checks and the
GrantAlreadyRevokedannotation handling is a good touch for user experience.pkg/connector/user.go (3)
181-202: LGTM - Clean routing between SCIM and standard API.The List method provides clear routing with helpful debug logging for troubleshooting.
204-229: LGTM - Standard API listing with proper error handling.The use of
pkg.AnnotationsForErrorcorrectly handles rate limiting from the Slack API.
258-261: LGTM - Correct SCIM pagination logic.The pagination calculation correctly handles SCIM's 1-based indexing.
pkg/connector/client/slack.go (6)
32-39: LGTM - Clean Client struct definition.The struct fields are well-organized for supporting both standard Slack API and SCIM endpoints.
41-74: LGTM - NewClient with gov environment support.The constructor properly handles both standard and gov environment URL configurations. Token validation may be happening at the config level.
76-123: LGTM - Centralized error handling with gRPC code mapping.Good use of the centralized
MapSlackErrorToGRPCCodefunction. The context messages provide helpful debugging information.
125-132: LGTM - Clean standalone function for session storage.Converting to a standalone function is appropriate since it doesn't require Client state.
304-330: LGTM - SCIM group listing with proper pagination.The method correctly uses the SCIM version for URL path formatting and passes pagination parameters.
496-547: LGTM - Enable/Disable user via SCIM.The asymmetric approach (DELETE for disable, PATCH for enable) correctly matches Slack's SCIM API behavior for user lifecycle management.
| type userGroupResourceType struct { | ||
| resourceType *v2.ResourceType | ||
| client *slack.Client | ||
| enterpriseID string | ||
| enterpriseClient *enterprise.Client | ||
| resourceType *v2.ResourceType | ||
| client *slack.Client | ||
| businessPlusClient *client.Client | ||
| } | ||
|
|
||
| func (o *userGroupResourceType) ResourceType(_ context.Context) *v2.ResourceType { | ||
| return o.resourceType | ||
| } | ||
|
|
||
| func userGroupBuilder( | ||
| client *slack.Client, | ||
| enterpriseID string, | ||
| enterpriseClient *enterprise.Client, | ||
| slackClient *slack.Client, | ||
| businessPlusClient *client.Client, | ||
| ) *userGroupResourceType { | ||
| return &userGroupResourceType{ | ||
| resourceType: resourceTypeUserGroup, | ||
| client: client, | ||
| enterpriseID: enterpriseID, | ||
| enterpriseClient: enterpriseClient, | ||
| resourceType: resourceTypeUserGroup, | ||
| client: slackClient, | ||
| businessPlusClient: businessPlusClient, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'businessPlusClient' pkg/connector/user_group.goRepository: ConductorOne/baton-slack
Length of output: 189
🏁 Script executed:
# Check if businessPlusClient is used in any methods or beyond assignment
rg -A 50 'func.*userGroupResourceType' pkg/connector/user_group.go | head -100Repository: ConductorOne/baton-slack
Length of output: 2626
businessPlusClient field is declared but never used in this file.
The businessPlusClient field is assigned in the struct initialization but not referenced in any method (List, Entitlements, Grants). Remove it unless it's intended for future functionality in a way not yet implemented.
🤖 Prompt for AI Agents
In pkg/connector/user_group.go around lines 20 to 39, the businessPlusClient
field is declared on userGroupResourceType and set in userGroupBuilder but never
used; remove the unused field from the struct, remove the businessPlusClient
parameter from userGroupBuilder, and stop assigning it in the returned struct.
Also update any call sites of userGroupBuilder to no longer pass a
businessPlusClient argument so the code compiles cleanly.
aldevv
force-pushed
the
business_plus_and_actions_fix
branch
2 times, most recently
from
5e5fc9a to
204a1ef
Compare
…ne/baton-slack into business_plus_and_actions_fix
aldevv
force-pushed
the
business_plus_and_actions_fix
branch
from
204a1ef to
5d8f7d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/client/request.go (1)
105-122: Resolve unusedpatchScimBytesto fix golangci-lint failure
golangci-lintreportsfunc (*Client).patchScimBytes is unused (unused)at Line 105. This will keep CI red until addressed.If you no longer need the raw-
[]byteSCIM patch variant, it’s simplest to remove it:-func (c *Client) patchScimBytes( - ctx context.Context, - path string, - target interface{}, - payload []byte, -) ( - *v2.RateLimitDescription, - error, -) { - return c.doRequest( - ctx, - http.MethodPatch, - c.getUrl(path, nil, true), - &target, - WithBearerToken(c.token), - uhttp.WithJSONBody(payload), - ) -} -If you still need this behavior, consider wiring it up through the new Business+ SCIM paths so that it’s referenced somewhere, or add a comment and
//nolint:unusedonly if it’s intentionally kept for future use (but removal is cleaner).
🧹 Nitpick comments (6)
pkg/connector/client/slack.go (4)
124-131: SetWorkspaceNames helper is fine; optional micro‑optimizationThe helper correctly maps workspace IDs to names and stores them in the session. If this ever runs over many workspaces, you could pre‑size the map:
-workspaceMap := make(map[string]string) +workspaceMap := make(map[string]string, len(workspaces))Purely optional.
303-329: ListIDPGroups SCIM pagination and error handling are appropriateThe SCIM list call uses the expected
startIndex/countquery params and defers HTTP error classification togetScim, only adding context viafmt.Errorf, which avoids double‑wrapping gRPC codes. Based on learnings, this is the right pattern relative touhttp.WrapErrors.You might consider normalizing obviously invalid inputs (e.g., coercing
startIndex <= 0to1or defaultingcountwhen<= 0) to avoid avoidable 4xx responses from Slack.
331-357: ListIDPUsers mirrors ListIDPGroups; potential DRY helperImplementation mirrors
ListIDPGroupsand looks correct from a SCIM and error‑handling standpoint. If you find yourself adding more SCIM list endpoints, consider a small generic helper likelistScim[T any](path string, startIndex, count int)to remove the duplication of theSCIMResponse[T]boilerplate.
391-405: AddUserToGroup is correct and idempotent; you can simplify the PATCH payloadThe flow (fetch group → short‑circuit if user already a member → PATCH) is correct and idempotent. For the PATCH body, you don't need to resend all existing members; SCIM semantics allow you to send only the new member. That would reduce payload size and the chance of subtle races with concurrent membership changes:
- var members []UserID - for _, member := range group.Members { - if member.Value == user { - return ratelimitData, nil - } - members = append(members, UserID{Value: member.Value}) - } - - members = append(members, UserID{Value: user}) + for _, member := range group.Members { + if member.Value == user { + // already present, nothing to do + return ratelimitData, nil + } + } + + members := []UserID{{Value: user}}The surrounding error wrapping with
fmt.Errorf("...: %w", err)is consistent with the existing HTTP wrappers and preserves underlying gRPC codes. Based on learnings, ...Also applies to: 410-413, 417-417
pkg/connector/client/request.go (2)
200-225:doScimRequesthelper looks good; consider migrating SCIM helpers to itThe new
doScimRequestnicely centralizes:
- SCIM base URL selection (
c.getUrl(path, nil, true)),- bearer token handling (
WithBearerToken(c.token)),- and optional JSON payload wiring.
This will help keep Business+ SCIM flows consistent.
Follow-up suggestion (non-blocking):
- Over time, consider refactoring
getScim/patchScim(and any new SCIM verbs) to delegate todoScimRequestfor consistency and to avoid repeating token and URL logic.- If you end up needing query parameters for some SCIM GETs, you might extend
doScimRequestwith an optionalqueryParametersargument instead of adding more one-off wrappers.
227-285: SCIM delete error wrapping is consistent but could map codes more precisely (optional)The
deleteScimflow now:
- Wraps request creation and body/error parsing failures with
uhttp.WrapErrors(codes.Internal, ...).- Logs and returns the underlying error from
wrapper.Dodirectly for transport/HTTP failures (preserving any mappingwrapper.Doalready applies).- Extracts SCIM
"detail"and wraps it as acodes.Internalerror.This is consistent with the earlier learning about avoiding double-wrapping
uhttp.WrapErrorsaround already-coded errors (you only wrap errors created here, and leavewrapper.Do’s own codes intact).Optional improvement:
- If you care about distinguishing common SCIM failures at the gRPC layer (e.g., 404 →
codes.NotFound, 403 →codes.PermissionDenied, 429/503 →codes.Unavailable), you could branch onresponse.StatusCodeand choose more specificcodes.*when wrapping the"detail"message, rather than alwayscodes.Internal.Not a blocker, but might improve retry behavior and observability for downstream callers.
If you decide to refine the gRPC codes here, please double-check expectations from any existing callers (e.g., retries or alerts keyed on
codes.Internalvs. more specific codes).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci.yamlis excluded by none and included by none
📒 Files selected for processing (2)
pkg/connector/client/request.go(6 hunks)pkg/connector/client/slack.go(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/client/request.gopkg/connector/client/slack.go
🧬 Code graph analysis (2)
pkg/connector/client/request.go (1)
pkg/connector/client/slack.go (1)
Client(31-38)
pkg/connector/client/slack.go (2)
pkg/connector/client/models.go (6)
SCIMResponse(18-24)GroupResource(148-154)UserResource(72-94)UserID(167-169)PatchOp(156-159)ScimOperate(161-165)pkg/connector/client/path.go (3)
UrlPathIDPGroups(22-22)UrlPathIDPUsers(24-24)UrlPathIDPGroup(21-21)
🪛 GitHub Actions: ci
pkg/connector/client/request.go
[error] 105-105: golangci-lint run --timeout=3m: pkg/connector/client/request.go:105:18: func (*Client).patchScimBytes is unused (unused)
🔇 Additional comments (4)
pkg/connector/client/slack.go (3)
31-37: Client SCIM configuration and gov routing look solidThe Client struct and NewClient wiring (base URL vs SCIM URL, token/botToken, govEnv switching between v1/v2) are coherent and side‑effect free. No correctness issues stand out here.
Also applies to: 40-72
487-492: patchGroup’s switch to doScimRequest is consistentUsing
doScimRequestwithhttp.MethodPatchhere is consistent with the other SCIM helpers, and the additional context fromfmt.Errorf("error patching IDP group: %w", err)is appropriate while still delegating HTTP/grpc code mapping to the underlying client.
441-452: Update SCIM PATCH operation format for removing users from Slack groupsSlack's official SCIM documentation specifies that removing a user from a group requires a PATCH with
"operation": "delete"inside the members array with the user's value included. The current approach using a separate remove operation with a filtered path does not match Slack's documented format and may fail or be ignored.Verify that the PATCH request includes
membersarray with objects containing"value": "user-id"and"operation": "delete", as shown in Slack's SCIM Groups PATCH documentation.pkg/connector/client/request.go (1)
87-141: Fix JSON unmarshalling target usage indoRequestand its SCIM callersThe review identifies a theoretical issue with how
doRequesthandles thetargetparameter when unmarshalling JSON. According to Go'sjson.Unmarshalsemantics, passing a pointer to aninterface{}variable prevents the unmarshaller from modifying the original caller's destination struct or map—instead, the unmarshaller replaces the interface value with a newly allocated map or other representation.The review claims:
doRequestcallsjson.Unmarshal(bodyBytes, &target)wheretargetisinterface{}- Callers (
getScim,patchScimBytes,patchScim) pass&targettodoRequest- This causes the caller's destination to remain unpopulated
The suggested fix is to have callers pass
target(not&target) anddoRequestunmarshal intotarget(not&target), allowing the unmarshaller to populate the caller's pointer correctly.The fix logic is sound according to Go semantics, but actual verification of the bug's presence in the current code is required, as the implementation of
doRequestitself and the real caller patterns outside this file cannot be inspected.Additionally,
patchScimBytesis reported as unused; confirm whether it should be removed to resolve the golangci-lint failure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/connector/user.go (1)
42-55: Remove redundantprofile["user_id"]assignments in both flows
profile["user_id"]is assigned twice in bothscimUserResourceanduserResource; the secondfmt.Sprint(...)write is redundant becauseslackUser.ID/user.IDis already a string. Keeping both only adds noise.Suggested change:
@@ func (o *userResourceType) scimUserResource(...) - profile["is_stranger"] = slackUser.IsStranger - profile["is_deleted"] = slackUser.Deleted - profile["user_id"] = fmt.Sprint(slackUser.ID) + profile["is_stranger"] = slackUser.IsStranger + profile["is_deleted"] = slackUser.Deleted @@ func userResource(...) - profile["is_stranger"] = user.IsStranger - profile["is_deleted"] = user.Deleted - profile["user_id"] = fmt.Sprint(user.ID) + profile["is_stranger"] = user.IsStranger + profile["is_deleted"] = user.DeletedAlso applies to: 105-118
🧹 Nitpick comments (3)
pkg/connector/user.go (3)
29-35: SCIM list path does an N+1 Slack Web API call per user
listScimAPIpages through SCIM users, and for each onescimUserResourcedoesGetUserInfoContextto fetch full Slack user data. For large workspaces this becomes an N+1 pattern (1 SCIM call + N Web API calls per page) and will hit Slack rate limits quickly.If
client.UserResourcealready includes the needed fields (names, email, status, etc.), consider:
- Building the profile and
UserTraitprimarily from the SCIM object.- Falling back to
GetUserInfoContextonly when specific non‑SCIM fields are required or missing.- Optionally making the extra Slack lookup feature‑flagged or configurable if strict parity with the non‑SCIM profile isn’t always needed.
This keeps Business+ SCIM listing performant while preserving compatibility where necessary.
Also applies to: 243-247, 249-255
245-247: Verify whetherListIDPUserserrors are already wrapped before callinguhttp.WrapErrorsHere errors from
o.businessPlusClient.ListIDPUsersare wrapped again withuhttp.WrapErrors(codes.Internal, ...). IfListIDPUsers(or its underlying client) already maps Slack errors to specific gRPC codes viahandleError/uhttp.WrapErrors, this will overwrite those codes withInternal, which can break retry logic and alerting.If
ListIDPUsersalready does that mapping, prefer preserving the original code and just adding context, e.g.:return nil, &resource.SyncOpResults{Annotations: annos}, fmt.Errorf("fetching SCIM users: %w", err)If it currently returns bare errors, your existing
uhttp.WrapErrorshere is appropriate.Based on learnings, please double‑check the client implementation for this method.
231-263: Optional: consider honoringattrs.Limitin SCIM pagination
listScimAPIcurrently uses a fixedcount := 100for each SCIM page and only readsattrs.PageToken.Token. If Baton sends a smallerattrs.Limit, you could optionally cap the SCIMcountto respect that limit:count := 100 if attrs.Limit > 0 && int(attrs.Limit) < count { count = int(attrs.Limit) }Not required for correctness, but it can make paging behavior more predictable across connectors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/user.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/user.go
🔇 Additional comments (3)
pkg/connector/user.go (3)
194-202: List selection between SCIM and standard API looks goodThe
Listmethod’s behavior of preferring SCIM whenbusinessPlusClientis non‑nil and clearly logging which path is used is clean and predictable, with a safe early‑exit whenparentResourceIDis nil.
204-229: Standard (non‑SCIM) listing flow is consistent with existing patternsUsing
GetUsersContextwithGetUsersOptionTeamID, mapping throughuserResource, and threadingAnnotationsForErrorup viaSyncOpResultsaligns with the existing connector style. The internaluserResourceerrors are correctly wrapped withuhttp.WrapErrors(codes.Internal, ...)since they originate locally in this code.
265-273: Builder wiring for Slack and Business+ clients looks straightforward
userBuildercleanly initializesuserResourceTypewith both the Slack client and the optional Business+ SCIM client while reusing the sharedresourceTypeUser. No issues here.
aldevv
force-pushed
the
business_plus_and_actions_fix
branch
from
ddfdb18 to
c5c1976
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/connector/user.go (1)
37-56: Remove duplicateuser_idassignments in user profile maps
profile["user_id"]is assigned twice in bothscimUserResourceanduserResource(once with the raw ID, then again withfmt.Sprint). The second write is redundant becauseslack.User.ID/user.IDare already strings, and it just overwrites the same key.You can simplify and avoid confusion by keeping a single assignment in each function:
@@ func (o *userResourceType) scimUserResource(... - profile["user_id"] = slackUser.ID + profile["user_id"] = slackUser.ID @@ - profile["user_id"] = fmt.Sprint(slackUser.ID)@@ func userResource(... - profile["user_id"] = user.ID + profile["user_id"] = user.ID @@ - profile["user_id"] = fmt.Sprint(user.ID)Also applies to: 100-118
🧹 Nitpick comments (4)
pkg/connector/client/request.go (2)
183-208: Clean centralization of SCIM request handling.The new
doScimRequestfunction provides a unified entry point for SCIM calls with optional JSON payload support. Note that this function passestargetdirectly todoRequest(not&target), which is more correct than whatgetScimandpatchScimdo at lines 100 and 118. Consider unifying this behavior in a follow-up.
262-264: Consider preserving more context in SCIM error responses.Using
codes.Internalfor all SCIM API errors loses semantic meaning. The SCIM API might return errors for various reasons (invalid input, not found, permission denied) that could map to more specific gRPC codes.If you want to preserve more context, consider parsing the SCIM error schema more fully:
- if detail, ok := errorResponse["detail"].(string); ok { - return &ratelimitData, uhttp.WrapErrors(codes.Internal, "SCIM API error", fmt.Errorf("%s", detail)) + if detail, ok := errorResponse["detail"].(string); ok { + code := codes.Internal + if status, ok := errorResponse["status"].(string); ok && status == "404" { + code = codes.NotFound + } + return &ratelimitData, uhttp.WrapErrors(code, "SCIM API error", errors.New(detail)) }Also,
fmt.Errorf("%s", detail)could be simplified toerrors.New(detail)(requires adding"errors"import).pkg/connector/user.go (2)
29-35: SCIM list path does N+1 Web API calls for user detailsIn
listScimAPI, each SCIM user fromListIDPUsersis passed toscimUserResource, which then callsGetUserInfoContextonce per user. On large workspaces this becomes N+1 pattern per page (1 SCIM call + up to 100 Web API calls), increasing latency and rate‑limit risk.Consider restructuring so that you either:
- Derive as much as possible directly from the SCIM
UserResource(only falling back to Web API when essential), or- Batch Web API user retrieval (e.g., a single
GetUsersContextfor the team) and look up per‑user data from an in‑memory map instead of per‑user RPCs.This keeps the current compatibility goal while being more scalable for Business+ tenants.
Also applies to: 231-255
241-247: Align SCIM error handling with centralized Slack error mappingFor SCIM listing errors you currently do:
return nil, &resource.SyncOpResults{Annotations: annos}, uhttp.WrapErrors(codes.Internal, "fetching SCIM users", err)whereas the standard API path uses
pkg.AnnotationsForError/pkg.WrapErrorto map Slack error strings to more specific gRPC codes (Unauthenticated, PermissionDenied, etc.).If
ListIDPUsersreturns Slack‑style errors, consider reusing the same centralized helpers (or a SCIM‑aware variant) so SCIM failures surface with consistent and more precise gRPC codes, while still attachingratelimitDataviaannos.WithRateLimiting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/connector/client/request.go(6 hunks)pkg/connector/user.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/client/request.go
🧬 Code graph analysis (1)
pkg/connector/user.go (3)
pkg/connector/client/slack.go (1)
Client(31-38)pkg/connector/client/models.go (4)
UserResource(72-94)Name(118-122)User(33-59)pkg/helpers.go (2)
WrapError(173-222)AnnotationsForError(227-242)
🔇 Additional comments (4)
pkg/connector/client/request.go (4)
1-17: LGTM on package rename and imports.The package rename from
enterprisetoclientand the addition of thegrpc/codesimport align with the Business+ SCIM refactoring objectives.
153-154: Appropriate use of error wrapping.Wrapping with
uhttp.WrapErrorsfor request creation errors is correct here since this is a locally-generated error (not returned from a client method that already wraps errors). Based on learnings, this aligns with the convention.
173-178: Good defensive check before unmarshaling.This correctly skips JSON unmarshaling for
204 No Contentresponses and empty bodies, preventing unnecessary unmarshal errors.
210-268: Verify if deleteScim could leverage doScimRequest.The new
doScimRequesthelper centralizes SCIM calls, butdeleteScimstill uses direct request construction. This appears intentional due to the specialized response handling (NoContent check, error body parsing). If this pattern is expected for other SCIM operations, the current approach is reasonable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/connector/user_group.go (1)
18-37:businessPlusClientfield is declared but never used.The
businessPlusClientfield is assigned in the struct but not referenced inList,Entitlements, orGrantsmethods. Either remove it or integrate it into the methods if Business+ functionality is intended.pkg/connector/user.go (2)
40-53: Duplicateprofile["user_id"]assignment.
profile["user_id"]is assigned on line 40 and overwritten on line 53. The second assignment usingfmt.Sprintis redundant sinceslackUser.IDis already a string.profile["user_id"] = slackUser.ID // ... other assignments ... -profile["user_id"] = fmt.Sprint(slackUser.ID)
103-116: Duplicateprofile["user_id"]assignment.Same issue as in
scimUserResource-profile["user_id"]is set on line 103 and overwritten on line 116.profile["user_id"] = user.ID // ... other assignments ... -profile["user_id"] = fmt.Sprint(user.ID)
🧹 Nitpick comments (3)
pkg/connector/user_group.go (1)
140-159: Consider batching user info fetches to reduce API calls.The current implementation makes an individual
GetUserInfoContextcall for each group member, resulting in N+1 API calls. For groups with many members, this could be slow and may hit rate limits.Consider using
GetUsersInfoContext(batch API) if available, or caching user info from the parent workspace sync.pkg/connector/workspace.go (1)
141-143: Silent fallback whenbusinessPlusClientis nil may hide configuration issues.When
businessPlusClientis nil, the method silently returns empty grants. Consider logging a warning to help diagnose configuration issues when Business+ functionality is expected but the client is not configured.if o.businessPlusClient == nil { + l := ctxzap.Extract(ctx) + l.Debug("Business+ client not available, skipping workspace grants") return nil, &resources.SyncOpResults{}, nil }pkg/connector/user.go (1)
27-34: Performance concern: N+1 API calls in SCIM user listing.Each SCIM user triggers an additional
GetUserInfoContextcall to fetch the full profile. For workspaces with many users, this could be slow and may hit rate limits during pagination. Consider caching user info or batching requests if the Slack API supports it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/connector/group.go(10 hunks)pkg/connector/user.go(4 hunks)pkg/connector/user_group.go(3 hunks)pkg/connector/workspace.go(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/workspace.gopkg/connector/user.gopkg/connector/group.gopkg/connector/user_group.go
🧬 Code graph analysis (3)
pkg/connector/workspace.go (3)
pkg/connector/client/slack.go (2)
Client(31-38)SetWorkspaceNames(125-131)pkg/helpers.go (2)
WrapError(173-222)ParsePageToken(61-76)pkg/connector/connector.go (1)
New(145-158)
pkg/connector/user.go (3)
pkg/connector/client/slack.go (1)
Client(31-38)pkg/connector/client/models.go (3)
Name(118-122)User(33-59)pkg/helpers.go (2)
WrapError(173-222)AnnotationsForError(227-242)
pkg/connector/user_group.go (3)
pkg/connector/client/slack.go (1)
Client(31-38)pkg/connector/connector.go (1)
New(145-158)pkg/helpers.go (1)
WrapError(173-222)
🔇 Additional comments (12)
pkg/connector/user_group.go (1)
62-96: LGTM!The
Listmethod correctly handles the nil parent case, fetches user groups scoped by team ID, and wraps errors consistently withpkg.WrapError.pkg/connector/workspace.go (4)
29-38: LGTM!The builder correctly initializes the workspace resource type with both the standard Slack client and the Business+ client.
66-108: LGTM!The
Listmethod correctly handles pagination, lists teams via the standard Slack API, creates workspace resources, and stores workspace names in the session for later reference.
176-177: LGTM!The simplified grant logic correctly creates workspace membership grants without the Enterprise Grid role-based grants.
185-187: LGTM!Good documentation explaining why Grant/Revoke are not implemented for Business+ plans.
pkg/connector/user.go (3)
179-200: LGTM!The routing logic correctly prioritizes the SCIM API when the Business+ client is available and falls back to the standard API otherwise. Debug logging provides good visibility.
229-261: LGTM!The SCIM-based listing correctly implements 1-indexed pagination, handles rate limiting annotations, and properly calculates the next page token.
202-227: ThelistStandardAPImethod is correct as written. The Slack SDK'sGetUsersContextautomatically handles pagination internally and returns the aggregated list of all users, hiding the cursor loop from the caller. UnlikelistScimAPI, which needs explicit pagination for sync operation page tokens,listStandardAPIrelies on this built-in behavior and does not require manual pagination implementation.Likely an incorrect or invalid review comment.
pkg/connector/group.go (4)
36-42: LGTM!The builder correctly initializes the group resource type with the Business+ client and government environment flag.
105-109: LGTM!Good defensive check with debug logging when the Business+ client is unavailable.
287-296: Same double-wrapping concern as in Grant.The error from
RemoveUserFromGroupis wrapped withcodes.Internal, which may overwrite more specific error codes if the client method already wraps errors. Apply the same fix as suggested forGrant.
298-300: LGTM!Good idempotent handling - the
GrantAlreadyRevokedannotation correctly signals when the grant was already revoked without returning an error.
| ratelimitData, err := g.businessPlusClient.AddUserToGroup( | ||
| ctx, | ||
| entitlement.Resource.Id.Resource, | ||
| principal.Id.Resource, | ||
| ) | ||
| outputAnnotations.WithRateLimiting(ratelimitData) | ||
| if err != nil { | ||
| return outputAnnotations, fmt.Errorf("failed to add user to IDP group during grant operation: %w", err) | ||
| return outputAnnotations, uhttp.WrapErrors(codes.Internal, "adding user to IDP group", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find businessPlusClient type and AddUserToGroup implementation
rg "businessPlusClient" pkg/connector/group.go -A 2 -B 2Repository: ConductorOne/baton-slack
Length of output: 1719
🏁 Script executed:
# Search for businessPlusClient field definition in the file
rg "type.*Manager" pkg/connector/group.go -A 20Repository: ConductorOne/baton-slack
Length of output: 50
🏁 Script executed:
# Find AddUserToGroup method implementation
rg "func.*AddUserToGroup" --type-list -g "*.go" -A 15Repository: ConductorOne/baton-slack
Length of output: 457
🏁 Script executed:
# Find where client package is imported and AddUserToGroup is defined
rg "AddUserToGroup" --type go -A 10Repository: ConductorOne/baton-slack
Length of output: 1345
🏁 Script executed:
# List client package files
fd "client" --type f -e goRepository: ConductorOne/baton-slack
Length of output: 50
🏁 Script executed:
# Check imports in group.go to find client package location
head -30 pkg/connector/group.go | grep -E "import|client"Repository: ConductorOne/baton-slack
Length of output: 171
🏁 Script executed:
# Get full AddUserToGroup implementation
sed -n '/^func (c \*Client) AddUserToGroup/,/^func [a-zA-Z]/p' pkg/connector/client/slack.go | head -60Repository: ConductorOne/baton-slack
Length of output: 1456
🏁 Script executed:
# Check GetIDPGroup implementation to see error wrapping
rg "func.*GetIDPGroup" pkg/connector/client/slack.go -A 20Repository: ConductorOne/baton-slack
Length of output: 482
🏁 Script executed:
# Search for handleError usage in client package
rg "handleError" pkg/connector/client/ -B 3 -A 3Repository: ConductorOne/baton-slack
Length of output: 2609
🏁 Script executed:
# Check patchGroup implementation
rg "func.*patchGroup" pkg/connector/client/slack.go -A 20Repository: ConductorOne/baton-slack
Length of output: 503
🏁 Script executed:
# Check getScim implementation to see if it wraps errors with gRPC codes
rg "func.*getScim" pkg/connector/client/slack.go -A 25Repository: ConductorOne/baton-slack
Length of output: 50
🏁 Script executed:
# Search for doScimRequest implementation
rg "func.*doScimRequest" pkg/connector/client/ -A 30Repository: ConductorOne/baton-slack
Length of output: 1544
🏁 Script executed:
# Search for gRPC codes being wrapped in client package
rg "codes\." pkg/connector/client/ -B 2 -A 2Repository: ConductorOne/baton-slack
Length of output: 3210
🏁 Script executed:
# Get full doRequest implementation in request.go
rg "func.*doRequest" pkg/connector/client/request.go -A 80 | head -120Repository: ConductorOne/baton-slack
Length of output: 1703
🏁 Script executed:
# Check if there's HTTP status to gRPC code mapping
rg "http.Status" pkg/connector/client/ -B 3 -A 3Repository: ConductorOne/baton-slack
Length of output: 963
🏁 Script executed:
# Check for HTTP status code validation in request.go - look after Do() call
sed -n '/func (c \*Client) doRequest/,/^func/p' pkg/connector/client/request.go | tail -100Repository: ConductorOne/baton-slack
Length of output: 1349
🏁 Script executed:
# Search for status code checking in the entire client package
rg "StatusCode" pkg/connector/client/ -B 5 -A 5Repository: ConductorOne/baton-slack
Length of output: 1384
🏁 Script executed:
# Get the full deleteScim function and any other request handling functions
rg "func.*Scim" pkg/connector/client/request.go -A 35Repository: ConductorOne/baton-slack
Length of output: 2184
🏁 Script executed:
# Check what uhttp.wrapper is and if it handles HTTP errors
rg "wrapper" pkg/connector/client/request.go | head -20Repository: ConductorOne/baton-slack
Length of output: 209
🏁 Script executed:
# Find wrapper initialization and type
rg "wrapper" pkg/connector/client/slack.go -B 2 -A 2Repository: ConductorOne/baton-slack
Length of output: 268
🏁 Script executed:
# Check imports in client files
head -50 pkg/connector/client/slack.go | grep -E "import|uhttp"Repository: ConductorOne/baton-slack
Length of output: 158
🏁 Script executed:
# Search for uhttp.Do documentation or error handling
rg "uhttp" pkg/connector/ -lRepository: ConductorOne/baton-slack
Length of output: 179
🏁 Script executed:
# Check if there's status code validation for errors anywhere in request.go
cat pkg/connector/client/request.go | grep -A 50 "c.wrapper.Do"Repository: ConductorOne/baton-slack
Length of output: 2069
🏁 Script executed:
# Search for any HTTP error status handling
rg "http.Status" pkg/connector/client/request.go -B 5 -A 10Repository: ConductorOne/baton-slack
Length of output: 910
🏁 Script executed:
# Get line numbers for doRequest and deleteScim to understand scope
rg "func.*doRequest\|func.*deleteScim" pkg/connector/client/request.go -nRepository: ConductorOne/baton-slack
Length of output: 50
🏁 Script executed:
# Check if there are other request handler functions that validate status codes
rg "response.StatusCode" pkg/connector/client/request.goRepository: ConductorOne/baton-slack
Length of output: 189
HTTP error status validation is missing in doRequest, causing SCIM API errors to be silently ignored.
The doRequest method in pkg/connector/client/request.go does not validate HTTP status codes for error responses (4xx, 5xx). Unlike deleteScim, which properly checks for error status codes and returns wrapped errors, doRequest only checks StatusCode != NoContent && len(bodyBytes) > 0 to unmarshal the response body as success. This means when the SCIM API returns a 4xx or 5xx error, doRequest returns nil error rather than reporting the failure. Consequently, AddUserToGroup and other methods relying on doRequest don't receive errors to wrap, and the operation appears to succeed when it actually failed.
Add HTTP status code validation in doRequest following the same pattern as deleteScim: check if response.StatusCode >= 400 and return an appropriate error before attempting to unmarshal the response body as target data.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/user.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/user.go
🔇 Additional comments (7)
pkg/connector/user.go (7)
3-15: LGTM!The import changes appropriately support the new SCIM functionality and logging requirements.
17-21: LGTM!The addition of
businessPlusClientfield properly supports the dual-client architecture.
92-151: LGTM! Duplicate assignment issue resolved.The duplicate
profile["user_id"]assignments flagged in previous reviews have been fixed. The profile map now correctly assigns each field only once.
177-198: LGTM!The routing logic cleanly separates SCIM and standard API paths with appropriate debug logging.
200-225: LGTM!The standard API listing logic is correct. Error from
userResourceis appropriately wrapped since it's a local function.Note: The error handling for
GetUsersContextat line 210 should be verified as part of the earlier comment about slack client error wrapping patterns.
261-269: LGTM!The builder function correctly accepts and initializes both the standard and Business+ clients.
227-259: No changes needed—pkg.WrapErroris the correct wrapper forbusinessPlusClient.ListIDPUsers.The current code at line 242 using
pkg.WrapError(err, "fetching SCIM users")is correct. Unlike the learning which cautions against double-wrapping whenhandleError()is already involved,ListIDPUsersdoes not usehandleError(). Instead,pkg.WrapErroris the proper wrapper here because it specifically maps Slack error strings to gRPC codes viaMapSlackErrorToGRPCCode. Usingfmt.Errorfwith%wwould lose the gRPC code mapping thatpkg.WrapErrorprovides, whereaspkg.WrapErrorpreserves and properly assigns gRPC codes needed for retry logic and alerting.Likely an incorrect or invalid review comment.
| func (o *userResourceType) scimUserResource(ctx context.Context, scimUser client.UserResource, parentResourceID *v2.ResourceId) (*v2.Resource, error) { | ||
| // NOTE: this is mainly to maintain compatibility with existing profile in non scim flow. | ||
| slackUser, err := o.client.GetUserInfoContext(ctx, scimUser.ID) | ||
| if err != nil { | ||
| wrappedErr := pkg.WrapError(err, fmt.Sprintf("fetching user info for SCIM user %s", scimUser.ID)) | ||
| return nil, wrappedErr | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg "GetUserInfoContext" -B 2 -A 15Repository: ConductorOne/baton-slack
Length of output: 3384
🏁 Script executed:
rg "handleError" -B 2 -A 10Repository: ConductorOne/baton-slack
Length of output: 4641
🏁 Script executed:
rg "pkg\.WrapError|uhttp\.WrapError" -B 2 -A 2Repository: ConductorOne/baton-slack
Length of output: 14383
🏁 Script executed:
fd "client" -type f -name "*.go" | head -20Repository: ConductorOne/baton-slack
Length of output: 238
🏁 Script executed:
rg "func WrapError" -B 2 -A 10Repository: ConductorOne/baton-slack
Length of output: 745
🏁 Script executed:
rg "MapSlackErrorToGRPCCode" -B 2 -A 20Repository: ConductorOne/baton-slack
Length of output: 4782
Fix error handling: use fmt.Errorf with %w instead of pkg.WrapError.
The o.client.GetUserInfoContext() call already wraps errors with gRPC codes via handleError(). Using pkg.WrapError causes double-wrapping and overwrites those codes. Instead, wrap with fmt.Errorf to preserve the gRPC context:
slackUser, err := o.client.GetUserInfoContext(ctx, scimUser.ID)
if err != nil {
return nil, fmt.Errorf("fetching user info for SCIM user %s: %w", scimUser.ID, err)
}
Address the N+1 API call pattern for SCIM users.
For every SCIM user, this method makes an additional GetUserInfoContext API call. With hundreds or thousands of users, this creates significant performance degradation and rate limit risk. Consider whether SCIM user data can provide the needed fields directly, or if a batch API exists to fetch multiple user profiles at once.
pkg/connector/client/request.go
Outdated
|
|
||
| bodyBytes, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| logBody(ctx, response) |
There was a problem hiding this comment.
I would verify if response is nill before logging
There was a problem hiding this comment.
logBody tries to read from the body after io.ReadAll has already consumed it. Maybe you could refactor logBody to accept []byte and pass bodyBytes directly, or log bodyBytes without using logBody.
| uhttp.WithRatelimitData(&ratelimitData), | ||
| ) | ||
| if err != nil { | ||
| logBody(ctx, response) |
|
|
||
| bodyBytes, err := io.ReadAll(response.Body) | ||
| if err != nil { | ||
| logBody(ctx, response) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/client/helpers.go(2 hunks)
🔇 Additional comments (2)
pkg/connector/client/helpers.go (2)
1-1: Package rename looks consistent with location
package clientmatches thepkg/connector/clientpath and keeps helpers colocated with the HTTP client; no concerns here.
14-22: Good defensive checks on nil response/BodyThe extra guards for
response == nilandresponse.Body == nilavoid panics in logging and give clearer error messages. This is a solid robustness improvement.
| body := make([]byte, 512) | ||
| _, err := bodyCloser.Read(body) | ||
| _, err := response.Body.Read(body) | ||
| if err != nil { | ||
| l.Error("error reading response body", zap.Error(err)) | ||
| return | ||
| } | ||
| l.Info("response body: ", zap.String("body", string(body))) | ||
| l.Info("response body", zap.String("body", string(body))) |
There was a problem hiding this comment.
Use the actual bytes read and consider a bounded io.ReadAll
Right now you ignore the byte count from Read and always log the full 512‑byte buffer. That can introduce trailing \x00 bytes into logs and makes partial reads harder to reason about. It’s also a bit clunky compared to using io.LimitReader.
A small refactor would improve correctness and clarity:
-import (
- "context"
- "net/http"
-
- "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
- "go.uber.org/zap"
-)
+import (
+ "context"
+ "io"
+ "net/http"
+
+ "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap"
+ "go.uber.org/zap"
+)
func logBody(ctx context.Context, response *http.Response) {
@@
- body := make([]byte, 512)
- _, err := response.Body.Read(body)
- if err != nil {
- l.Error("error reading response body", zap.Error(err))
- return
- }
- l.Info("response body", zap.String("body", string(body)))
+ // Limit how much we read for logging while respecting the actual length read.
+ body, err := io.ReadAll(io.LimitReader(response.Body, 512))
+ if err != nil {
+ l.Error("error reading response body", zap.Error(err))
+ return
+ }
+ l.Info("response body", zap.ByteString("body", body))
}This keeps the 512‑byte cap, logs exactly what was read, and avoids zero‑padding artifacts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| body := make([]byte, 512) | |
| _, err := bodyCloser.Read(body) | |
| _, err := response.Body.Read(body) | |
| if err != nil { | |
| l.Error("error reading response body", zap.Error(err)) | |
| return | |
| } | |
| l.Info("response body: ", zap.String("body", string(body))) | |
| l.Info("response body", zap.String("body", string(body))) | |
| // Limit how much we read for logging while respecting the actual length read. | |
| body, err := io.ReadAll(io.LimitReader(response.Body, 512)) | |
| if err != nil { | |
| l.Error("error reading response body", zap.Error(err)) | |
| return | |
| } | |
| l.Info("response body", zap.ByteString("body", body)) |
🤖 Prompt for AI Agents
In pkg/connector/client/helpers.go around lines 24 to 30, the code ignores the
byte count returned by response.Body.Read and always logs the full 512‑byte
buffer (producing trailing \x00), and it should instead log only the bytes
actually read and cap reads to 512 bytes: replace the current Read call with
either io.ReadAll(io.LimitReader(response.Body, 512)) and log the returned slice
as a string (handling errors and io.EOF appropriately), or keep the fixed buffer
but use n, err := response.Body.Read(body); if err != nil && err != io.EOF { log
error; return } then log string(body[:n]) so only the actual bytes read are
logged and zero‑padding is avoided.
|
@aldevv do we have a migration plan for existing customers that are already using slack enterprise grid? (with this repo, not with new repo) |
not as far as I know |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/connector/client/request.go (1)
182-207: ThedoScimRequesthelper is actively used, but the refactoring observation remains valid.
doScimRequestis called frompkg/connector/client/slack.go:486with HTTP PATCH operations and a payload. However, thepatchScimmethod (line 105) implements nearly identical logic: both execute PATCH requests with bearer token authentication and JSON payload handling. This represents a consistency gap wherepatchScimcould be refactored to delegate todoScimRequestto avoid duplication. ThegetScimmethod cannot use this helper due to its query parameter support, anddeleteScimuses a different pattern with custom logging and direct wrapper calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/client/request.go(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T17:48:53.644Z
Learnt from: mateoHernandez123
Repo: ConductorOne/baton-slack PR: 57
File: pkg/connector/roles.go:217-219
Timestamp: 2025-10-28T17:48:53.644Z
Learning: In the baton-slack connector, when client methods like SetWorkspaceRole(), RemoveUser(), or other enterprise/slack client calls already wrap errors with specific gRPC codes via handleError(), use fmt.Errorf with %w to preserve those codes while adding context. Only use uhttp.WrapErrors for errors generated directly in the function (e.g., validation, precondition checks). Double-wrapping with uhttp.WrapErrors would overwrite the specific codes (Unavailable, PermissionDenied, etc.) needed for proper retry logic and alerting.
<!-- [/add_learning]
Applied to files:
pkg/connector/client/request.go
🧬 Code graph analysis (1)
pkg/connector/client/request.go (1)
pkg/connector/client/slack.go (1)
Client(31-38)
🔇 Additional comments (4)
pkg/connector/client/request.go (4)
1-1: LGTM!The package rename from
enterprisetoclientand the addition of the gRPC codes import support the refactoring to Business+ SCIM.Also applies to: 16-16
153-153: LGTM!The error wrapping for request creation and body reading uses
uhttp.WrapErrorsappropriately for errors generated directly in this function. Based on learnings, this is the correct pattern for internal operation errors (vs. errors from wrapped client methods).Also applies to: 169-169
172-177: LGTM!The defensive check for
StatusNoContentand non-empty body before unmarshaling prevents unnecessary unmarshal attempts on empty responses. The error wrapping is consistent with the pattern used throughout the file.
231-231: LGTM!The error wrapping for SCIM delete operations appropriately uses
uhttp.WrapErrorsfor internal operation errors with descriptive context. The pattern is consistent with the learning that internal operations should be wrapped this way.Also applies to: 252-252, 259-259
| } | ||
| if detail, ok := errorResponse["detail"].(string); ok { | ||
| return &ratelimitData, fmt.Errorf("SCIM API error: %s", detail) | ||
| return &ratelimitData, uhttp.WrapErrors(codes.Internal, "SCIM API error", fmt.Errorf("%s", detail)) |
There was a problem hiding this comment.
Map HTTP status to appropriate gRPC code instead of always using codes.Internal.
The code unconditionally wraps SCIM API errors with codes.Internal, but the actual HTTP status could indicate a client error (4xx) or server error (5xx). Using the wrong code affects retry logic and alerting.
Inspect response.StatusCode to map to the appropriate gRPC code:
- 400-level errors →
codes.InvalidArgument,codes.NotFound,codes.AlreadyExists, etc. - 500-level errors →
codes.Internal,codes.Unavailable, etc.
Apply this pattern to map status codes appropriately:
+ // Map HTTP status to appropriate gRPC code
+ code := codes.Internal
+ if response.StatusCode >= 400 && response.StatusCode < 500 {
+ code = codes.InvalidArgument // or more specific based on status
+ }
+
if detail, ok := errorResponse["detail"].(string); ok {
- return &ratelimitData, uhttp.WrapErrors(codes.Internal, "SCIM API error", fmt.Errorf("%s", detail))
+ return &ratelimitData, uhttp.WrapErrors(code, "SCIM API error", fmt.Errorf("%s", detail))
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/connector/client/request.go around line 262, the error returned always
uses codes.Internal regardless of response.StatusCode; update the code to
inspect response.StatusCode and map HTTP ranges/statuses to appropriate gRPC
codes before wrapping the error. For 4xx responses map common codes (e.g.,
400→codes.InvalidArgument, 401→codes.PermissionDenied or codes.Unauthenticated
as appropriate, 403→codes.PermissionDenied, 404→codes.NotFound,
409→codes.AlreadyExists, 429→codes.ResourceExhausted) and for 5xx responses map
to server-side codes (e.g., 500→codes.Internal, 503→codes.Unavailable), then
call uhttp.WrapErrors with the chosen gRPC code and include the HTTP status and
response detail in the error message to preserve context.
pkg/config/config.go
Outdated
| "sso-enabled", | ||
| field.WithDisplayName("SSO Enabled"), | ||
| field.WithDescription("Flag indicating that the SSO has been configured for Enterprise Grid Organization. Enables usage of SCIM API"), | ||
| field.WithDescription("Deprecated: Enterprise Grid features moved to baton-slack-enterprise. This connector focuses on Business+ plans."), |
There was a problem hiding this comment.
I think we should just remove the field, or at least use WithExportTarget(ExportTargetNone) so it doesn't show up in the UI
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/config/config.go (1)
17-22: BusinessPlus token field looks consistent; consider clarifying when it’s requiredThe rename to
BusinessPlusTokenFieldand the"business-plus-token"key look consistent with the shift away from the enterprise-specific field, and keeping it secret-only (not required) matches the idea that it’s only needed for certain plans.You might optionally tweak the description to make the usage a bit more self‑service, e.g., explicitly call out that it’s only needed for Business+ or Enterprise Grid tenants and which capabilities it unlocks (Admin/SCIM vs. standard bot API), so non‑Business+ users know they can safely leave it empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/config.go(2 hunks)
🔇 Additional comments (1)
pkg/config/config.go (1)
35-47: Gov env dependency on BusinessPlus token wiring looks correctAdding
BusinessPlusTokenFieldintoConfigurationFieldsand updatingFieldRelationshipssoGovEnvironmentFielddepends onBusinessPlusTokenFieldpreserves the previous constraint pattern (Gov env implies an admin/SCIM-style token), just with the new field.No issues from a correctness standpoint; the relationship and the updated comment about Gov instances being Business+ or Enterprise Grid read coherently with the rest of the PR.
| - name: Install Go | ||
| uses: actions/setup-go@v5 | ||
| - name: Grant/revoke | ||
| uses: ConductorOne/github-workflows/actions/sync-test@v2 |
There was a problem hiding this comment.
I think there's a newer version of the sync test now.
| field.WithDisplayName("Enterprise Token"), | ||
| field.WithDescription("The Slack user oauth token used to connect to the Slack Enterprise Grid Admin API"), | ||
| BusinessPlusTokenField = field.StringField( | ||
| "business-plus-token", |
There was a problem hiding this comment.
Should we just keep the old name? I'm sure Slack will rebrand this product in the future, and we don't want to break backward compatibility every time they do that.
| AccessTokenField, | ||
| EnterpriseTokenField, | ||
| SSOEnabledField, | ||
| BusinessPlusTokenField, |
There was a problem hiding this comment.
If SSOEnabledField is removed, I don't think it'll show up anywhere in --help, and I think if someone specifies it, the connector will error with "unknown flag: --sso-enabled".
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.