[BB-1925] Add workspace role #69
Conversation
WalkthroughAdds Slack workspace role support and session-backed workspace name cache: new workspaceRole resource and builder, session helpers to set/get workspace names, and per-user role grant generation in workspace grants. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Client
participant WS as Workspace Resource
participant BPC as Business+ Client
participant SS as SessionStore
participant WR as WorkspaceRoles
CLI->>WS: List Workspaces
WS->>BPC: Fetch workspaces
BPC-->>WS: workspaces[]
WS->>SS: SetWorkspaceNames(ctx, ss, workspaces)
SS-->>WS: OK
CLI->>WR: List Roles (parent: workspace)
WR-->>CLI: role resources
CLI->>WR: Entitlements(role)
WR->>SS: GetWorkspaceNames(ctx, ss, [workspaceID])
SS-->>WR: workspace name (or missing)
WR-->>CLI: Entitlement (uses workspace name)
CLI->>WS: Get Grants (workspace)
WS->>BPC: Fetch workspace members & flags
BPC-->>WS: members + flags
WS-->>CLI: Grants (per-user role mappings)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/workspace.go (1)
88-107: Remove duplicateSetWorkspaceNamescall.Workspace names are stored twice in succession: first via the method call at line 89, then via the package-level function at line 104. Only one call is needed.
🔎 Proposed fix
Keep one call and remove the other. If keeping the package-level function:
- if o.businessPlusClient != nil { - err = o.businessPlusClient.SetWorkspaceNames(ctx, attrs.Session, workspaces) - if err != nil { - return nil, nil, fmt.Errorf("storing workspace names in session: %w", err) - } - } - rv := make([]*v2.Resource, 0, len(workspaces))Or if keeping the method, remove lines 104-107 and adjust the conditional at lines 88-93 to always execute when businessPlusClient is available.
🧹 Nitpick comments (2)
pkg/connector/workspace.go (1)
238-244: Redundant!user.Deletedcheck.Deleted users are already filtered out at lines 182-184, so
!user.Deletedin this condition is always true and can be removed for clarity.🔎 Proposed fix
- if !user.IsRestricted && !user.IsUltraRestricted && !user.IsInvitedUser && !user.IsBot && !user.Deleted { + if !user.IsRestricted && !user.IsUltraRestricted && !user.IsInvitedUser && !user.IsBot {pkg/connector/workspaceRoles.go (1)
17-29: UnusedRegularRoleIDconstant.
RegularRoleIDis defined but not used in therolesmap or elsewhere in the codebase. Consider removing it if unused, or add it to the roles map if it should represent a valid role type.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/connector/client/slack.gopkg/connector/connector.gopkg/connector/resource_types.gopkg/connector/workspace.gopkg/connector/workspaceRoles.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/workspace.gopkg/connector/workspaceRoles.gopkg/connector/client/slack.go
🧬 Code graph analysis (2)
pkg/connector/workspace.go (2)
pkg/connector/client/slack.go (1)
SetWorkspaceNames(73-79)pkg/connector/workspaceRoles.go (9)
PrimaryOwnerRoleID(18-18)RoleAssignmentEntitlement(26-26)OwnerRoleID(19-19)AdminRoleID(20-20)SingleChannelGuestRoleID(22-22)MultiChannelGuestRoleID(21-21)InvitedMemberRoleID(23-23)MemberRoleID(25-25)BotRoleID(24-24)
pkg/connector/workspaceRoles.go (2)
pkg/connector/client/slack.go (1)
Client(28-35)pkg/helpers.go (1)
MakeResourceList(30-52)
🪛 GitHub Actions: ci
pkg/connector/workspace.go
[error] 144-144: golangci-lint: Comment should end in a period (godot)
🪛 GitHub Check: go-lint
pkg/connector/workspace.go
[failure] 144-144:
Comment should end in a period (godot)
🔇 Additional comments (8)
pkg/connector/resource_types.go (1)
38-45: LGTM!The new
resourceTypeWorkspaceRolefollows the established pattern of other resource type declarations and correctly usesTRAIT_ROLEfor role semantics.pkg/connector/connector.go (1)
165-165: LGTM!The
workspaceRoleBuilderis correctly registered with only thebusinessPlusClient, which aligns with theworkspaceRoleTypestruct definition that only requires this client for workspace role operations.pkg/connector/client/slack.go (1)
524-548: LGTM!The
GetWorkspaceNamesmethod handles edge cases well—filtering empty IDs and returning both found names and missing IDs for better error reporting downstream.pkg/connector/workspace.go (2)
63-63: LGTM!Adding
resourceTypeWorkspaceRoleas a child resource type correctly establishes the parent-child relationship for workspace roles.
190-252: LGTM!The role grant logic comprehensively covers all Slack user types (PrimaryOwner, Owner, Admin, guests, invited members, bots, and regular members) with consistent error handling using
fmt.Errorfwith%wto preserve error context. Based on learnings, this approach is correct sinceroleResourcedoesn't wrap errors with gRPC codes.pkg/connector/workspaceRoles.go (3)
83-106: LGTM!The
Listmethod correctly handles the nilparentResourceIDcase and uses the helperMakeResourceListto iterate over roles efficiently.
58-81: LGTM!The
roleResourcefunction properly validates the role ID against the known roles map and constructs a composite ID that includes the parent workspace ID, ensuring uniqueness across workspaces.
150-165: LGTM!The empty
Grantsimplementation is intentional and well-documented. The comment clearly explains that role grants are emitted inworkspace.go'sGrantsmethod for efficiency, avoiding redundant user enumeration per role.
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/workspace.go (1)
88-107: Remove duplicate workspace name storage.Lines 88-93 conditionally store workspace names via
o.businessPlusClient.SetWorkspaceNames(), and lines 104-107 unconditionally store the same data viaclient.SetWorkspaceNames(). Both implementations are identical and write to the same session namespace with identical parameters. The unconditional call at lines 104-107 makes the conditional call redundant; whenbusinessPlusClientis not nil, workspace names are stored twice. Remove one of these duplicate calls.
♻️ Duplicate comments (1)
pkg/connector/workspace.go (1)
144-144: Fix lint error: comment should end in a period.The comment should end with a period to satisfy the godot linter.
🔎 Proposed fix
-// sets workspace memberships and workspace roles +// Sets workspace memberships and workspace roles.
🧹 Nitpick comments (1)
pkg/connector/workspace.go (1)
238-238: Remove redundant deleted user check.The condition checks
!user.Deleted, but deleted users are already filtered out at lines 182-184. This check is redundant and can be removed for clarity.🔎 Proposed fix
- if !user.IsRestricted && !user.IsUltraRestricted && !user.IsInvitedUser && !user.IsBot && !user.Deleted { + if !user.IsRestricted && !user.IsUltraRestricted && !user.IsInvitedUser && !user.IsBot {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/workspace.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/workspace.go
🧬 Code graph analysis (1)
pkg/connector/workspace.go (2)
pkg/connector/client/slack.go (1)
SetWorkspaceNames(73-79)pkg/connector/workspaceRoles.go (9)
PrimaryOwnerRoleID(18-18)RoleAssignmentEntitlement(26-26)OwnerRoleID(19-19)AdminRoleID(20-20)SingleChannelGuestRoleID(22-22)MultiChannelGuestRoleID(21-21)InvitedMemberRoleID(23-23)MemberRoleID(25-25)BotRoleID(24-24)
🔇 Additional comments (4)
pkg/connector/workspace.go (4)
14-14: Good addition of context-aware logging.The ctxzap import enables proper structured logging throughout the Grants function, improving observability.
63-63: LGTM: Workspace role registered as child resource.This correctly establishes the parent-child relationship between workspace and workspace role resources.
150-153: LGTM: Proper logging and early return.The context-aware logger extraction and debug-level logging when Business+ client is unavailable follows best practices.
190-254: LGTM: Role grant implementation follows consistent pattern.The role grant logic correctly:
- Handles multiple user types (PrimaryOwner, Owner, Admin, Guests, Member, Bot)
- Uses appropriate role constants from workspaceRoles.go
- Allows users to have multiple roles (e.g., PrimaryOwner + Owner + Admin)
- Always grants workspace membership entitlement regardless of role
- Follows consistent error handling pattern
| DisplayName: "Workspace Role", | ||
| Traits: []v2.ResourceType_Trait{ | ||
| v2.ResourceType_TRAIT_ROLE, | ||
| }, |
There was a problem hiding this comment.
Annotations: annotations.New(&v2.SkipGrants{}), since grants is a noop?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/connector/workspaceRoles.go (1)
81-104: Non-deterministic role ordering in List output.Using
maps.Keys(roles)returns keys in non-deterministic order. While this doesn't affect correctness, it may cause unnecessary churn in sync operations if the order changes between runs.Consider sorting the keys for consistent ordering:
🔎 Proposed fix
func (o *workspaceRoleType) List( ctx context.Context, parentResourceID *v2.ResourceId, _ resources.SyncOpAttrs, ) ( []*v2.Resource, *resources.SyncOpResults, error, ) { if parentResourceID == nil { return nil, &resources.SyncOpResults{}, nil } + roleIDs := slices.Collect(maps.Keys(roles)) + slices.Sort(roleIDs) + output, err := pkg.MakeResourceList( ctx, - slices.Collect(maps.Keys(roles)), + roleIDs, parentResourceID, roleResource, )pkg/connector/workspace.go (1)
231-237: Redundant!user.Deletedcheck.The
user.Deletedcondition at line 231 is redundant since deleted users are already skipped at lines 175-177.🔎 Proposed fix
- if !user.IsRestricted && !user.IsUltraRestricted && !user.IsInvitedUser && !user.IsBot && !user.Deleted { + if !user.IsRestricted && !user.IsUltraRestricted && !user.IsInvitedUser && !user.IsBot { rr, err := roleResource(ctx, MemberRoleID, resource.Id)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/connector/client/slack.gopkg/connector/resource_types.gopkg/connector/workspace.gopkg/connector/workspaceRoles.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/resource_types.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/workspace.gopkg/connector/workspaceRoles.gopkg/connector/client/slack.go
🧬 Code graph analysis (2)
pkg/connector/workspace.go (2)
pkg/connector/client/slack.go (1)
SetWorkspaceNames(506-512)pkg/connector/workspaceRoles.go (9)
PrimaryOwnerRoleID(18-18)RoleAssignmentEntitlement(26-26)OwnerRoleID(19-19)AdminRoleID(20-20)SingleChannelGuestRoleID(22-22)MultiChannelGuestRoleID(21-21)InvitedMemberRoleID(23-23)MemberRoleID(25-25)BotRoleID(24-24)
pkg/connector/workspaceRoles.go (2)
pkg/connector/client/slack.go (1)
GetWorkspaceNames(515-540)pkg/helpers.go (1)
MakeResourceList(30-52)
🔇 Additional comments (13)
pkg/connector/client/slack.go (2)
505-512: LGTM!The
SetWorkspaceNamesfunction correctly maps workspace IDs to names and persists them to the session store using the appropriate namespace.
514-540: LGTM!The
GetWorkspaceNamesfunction is well-structured:
- Properly filters out empty IDs before querying
- Returns early with empty results when no valid IDs exist
- Correctly computes and returns missing IDs for caller awareness
- Error propagation is appropriate
pkg/connector/workspaceRoles.go (5)
17-38: LGTM!Role constants and display name mappings are well-organized and comprehensive for Slack's user role types.
40-54: LGTM!The builder pattern is consistent with other resource types in the codebase.
56-79: LGTM!The
roleResourcehelper correctly validates role IDs and creates composite identifiers using theparentResourceID:roleIDformat, ensuring role uniqueness per workspace.
106-146: LGTM!The Entitlements method correctly fetches workspace names from the session store and constructs descriptive entitlements. The error handling for missing workspace names is appropriate.
Note: The past review comment about nil pointer dereference was for a previous implementation that called
o.businessPlusClient.GetWorkspaceNames(). The current code uses the package-levelclient.GetWorkspaceNames()function which doesn't require a nil check onbusinessPlusClient.
148-164: LGTM!The Grants stub is well-documented, clearly explaining that role grants are emitted in
workspace.gofor efficiency reasons aligned with the Slack API design.pkg/connector/workspace.go (6)
14-14: LGTM!Good addition of the
ctxzaplogger for structured debug logging.
60-65: LGTM!Correctly registers
WorkspaceRoleas a child resource type of Workspace, enabling the resource hierarchy.
88-91: LGTM!Storing workspace names in the session during List enables the Entitlements method in
workspaceRoles.goto retrieve them efficiently. Good error handling with appropriate context.
137-147: LGTM!The comment now ends with a period (addressing the past lint issue), and the debug log message is helpful for troubleshooting when Business+ credentials are not configured.
175-177: LGTM!Good addition to skip deleted users early, preventing unnecessary grant processing.
183-248: Role grant logic is comprehensive and well-structured.The implementation correctly maps Slack user attributes to workspace roles:
- Primary owners, owners, and admins receive their respective elevated role grants
- Guest users are properly differentiated between single-channel (ultra-restricted) and multi-channel guests
- Invited members, regular members, and bots each receive appropriate role grants
- The workspace membership grant is always appended at the end
The use of
fmt.Errorfwith%wfor error wrapping preserves gRPC error codes, which aligns with the project's error handling conventions. Based on learnings, this is the correct pattern when role resource creation errors need to propagate proper codes for retry logic.
originally removed because grants function was empty, but after further inspection, the workspace roles were being added in the workspace.go
Grantsfunction, alongside the workspace's membershipSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.