Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 63f64e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
||||||||||||||||
|
|
||||||||||||||||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
An empty resources array (user clicked "Specific projects" but selected none) was treated identically to null (unrestricted), silently inserting a wildcard "*" grant. Now: - Server: nil = wildcard, empty slice = no grant inserted - Frontend: scopes with empty resource lists are filtered out Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace http.DefaultClient (no timeout, no retries) with retryablehttp.NewClient().StandardClient() — consistent with the rest of the codebase. This adds automatic retries with backoff for transient failures and a default timeout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
retryablehttp.NewClient().StandardClient() doesn't set a timeout by default. Add an explicit 30s timeout to prevent indefinite blocking if WorkOS is unresponsive. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Seed script: use Postgres array literal '{id1,id2}' cast to text[]
instead of ARRAY[:'org_ids'] which produced a single concatenated
string for multi-org seeding
- Submit button: count only effective grants (unrestricted or with
selected resources) so the button disables when all scopes have
empty resource lists
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace sequential per-member GetUser API calls with a single paginated ListUsers(organizationID) call. Returns a map for O(1) lookup per membership. Eliminates N round-trips to WorkOS API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
System roles (admin, member) now have disabled checkboxes and hidden resource pickers in the edit dialog, matching the existing behavior of disabled name/description fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
deleteRole onSuccess was only invalidating the roles cache. When a role is deleted, WorkOS reassigns members to a default role, so the members cache must also be invalidated to avoid showing stale role assignments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract RoleProvider interface from concrete *workos.RoleClient so the access service can be tested with an in-memory mock. Add configurable endpoint to RoleClient for httptest-based testing of the actual HTTP layer. Service-layer tests cover CreateRole, UpdateRole, DeleteRole, ListRoles including grant persistence, system role protection, and member counts. RoleClient tests cover all 8 methods against a fake WorkOS HTTP server. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| {Slug: "build:write", Description: "Create and manage projects and deployments", ResourceType: "project"}, | ||
| {Slug: "mcp:read", Description: "View MCP server configurations", ResourceType: "mcp"}, | ||
| {Slug: "mcp:write", Description: "Create and manage MCP server configurations", ResourceType: "mcp"}, | ||
| {Slug: "mcp:connect", Description: "Connect to and use MCP servers", ResourceType: "mcp"}, |
There was a problem hiding this comment.
Note for later: these scopes will be defined in the access package I'm creating in my PR - we should use the constants from there.
| // mcp:read, mcp:write, mcp:connect | ||
| // - Grants are additive (union), no deny | ||
| // - Each grant is unrestricted (resources=null) or allowlisted (resources=[...]) | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
We should probably get rid of these comments - it's the type of comment that'll go stale quite fast
| return "", fmt.Errorf("get organization metadata: %w", err) | ||
| } | ||
| if !org.WorkosID.Valid || org.WorkosID.String == "" { | ||
| return "", gen.MakeBadRequest(errors.New("organization is not linked to WorkOS")) |
There was a problem hiding this comment.
I don't believe this is standard in our codebase - I can't see other places where we use this. You should be using oops here.
There was a problem hiding this comment.
This should apply to all gen.Make clauses we have throughout.
|
|
||
| Meta("openapi:operationId", "listRoles") | ||
| Meta("openapi:extension:x-speakeasy-name-override", "listRoles") | ||
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "ListRoles"}`) |
There was a problem hiding this comment.
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "ListRoles"}`) | |
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "Roles"}`) |
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "Grants"}`) | ||
| Meta("openapi:operationId", "getRole") | ||
| Meta("openapi:extension:x-speakeasy-name-override", "getRole") | ||
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "GetRole"}`) |
There was a problem hiding this comment.
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "GetRole"}`) | |
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "Role"}`) |
|
|
||
| Meta("openapi:operationId", "listScopes") | ||
| Meta("openapi:extension:x-speakeasy-name-override", "listScopes") | ||
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "ListScopes"}`) |
There was a problem hiding this comment.
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "ListScopes"}`) | |
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "Scopes"}`) |
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "RemovePrincipalGrants"}`) | ||
| Meta("openapi:operationId", "listMembers") | ||
| Meta("openapi:extension:x-speakeasy-name-override", "listMembers") | ||
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "ListMembers"}`) |
There was a problem hiding this comment.
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "ListMembers"}`) | |
| Meta("openapi:extension:x-speakeasy-react-hook", `{"name": "Members"}`) |
| ListRoles(ctx context.Context, orgID string) ([]roles.Role, error) | ||
| CreateRole(ctx context.Context, orgID string, opts workos.CreateRoleOpts) (*roles.Role, error) | ||
| UpdateRole(ctx context.Context, orgID string, roleSlug string, opts workos.UpdateRoleOpts) (*roles.Role, error) | ||
| DeleteRole(ctx context.Context, orgID string, roleSlug string) error | ||
| ListMembers(ctx context.Context, orgID string) ([]usermanagement.OrganizationMembership, error) | ||
| UpdateMemberRole(ctx context.Context, membershipID string, roleSlug string) (*usermanagement.OrganizationMembership, error) | ||
| GetUser(ctx context.Context, userID string) (*usermanagement.User, error) | ||
| ListOrgUsers(ctx context.Context, orgID string) (map[string]usermanagement.User, error) |
There was a problem hiding this comment.
the dependence on workos types makes this a leaky abstraction. can we do without it?
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
|
|
||
| if resp.StatusCode >= 400 { |
There was a problem hiding this comment.
Do we want to handle all errors the same way? 4xx and 5xx are different in nature.
Additionally: how will we be handling these upstream (what will the user see)?
| } | ||
|
|
||
| var role roles.Role | ||
| if err := rc.doAPI(ctx, http.MethodPost, "/authorization/organizations/"+orgID+"/roles", body, &role); err != nil { |
There was a problem hiding this comment.
Does workos SDK not have this?
|
|
||
| // doAPI performs a raw HTTP request against the WorkOS REST API. | ||
| // Used for endpoints not covered by the Go SDK (role CRUD). | ||
| func (rc *RoleClient) doAPI(ctx context.Context, method, path string, body []byte, out any) error { |
There was a problem hiding this comment.
Just for your knowledge, you could just have this method be called do (no need for doAPI). It's a common/well known go idiom.
Not blocking, just information
| // mockRoleProvider is an in-memory implementation of access.RoleProvider for | ||
| // tests. It stores roles and memberships in maps so tests can exercise the | ||
| // service layer without hitting WorkOS. | ||
| type mockRoleProvider struct { |
There was a problem hiding this comment.
I'm unsure about our DB connections (please check it), but I wouldn't keep the mocks for testing in the same file as the setup_test - I'd expect setup test to be all about setting up the test (and any potential shared test methods). Ignore if this is how we typically do it.
| if err != nil { | ||
| return fmt.Errorf("begin transaction: %w", err) | ||
| } | ||
| defer tx.Rollback(ctx) //nolint:errcheck // rollback after commit is a no-op; error is safe to ignore |
There was a problem hiding this comment.
You should be using:
defer o11y.NoLogDefer(func() error { return dbtx.Rollback(ctx) })
| return "", fmt.Errorf("get organization metadata: %w", err) | ||
| } | ||
| if !org.WorkosID.Valid || org.WorkosID.String == "" { | ||
| return "", gen.MakeBadRequest(errors.New("organization is not linked to WorkOS")) |
There was a problem hiding this comment.
This should apply to all gen.Make clauses we have throughout.
| Description: p.Description, | ||
| }) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "409") || strings.Contains(err.Error(), "slug_conflict") || strings.Contains(err.Error(), "already in use") { |
There was a problem hiding this comment.
Is this the only way to check for WorkOS errors? I'd like to avoid these string comparisons if possible - they're a recipe for disaster. Tomorrow WorkOS changes the wording and our logic blows up (this happened with Polar recently)
| return &gen.UpsertGrantsResult{Grants: grants}, nil | ||
| // Sync scope grants to local DB. If this fails, clean up the WorkOS role | ||
| // to avoid leaving an orphan. | ||
| if len(p.Grants) > 0 { |
There was a problem hiding this comment.
Suggestion: move this line to the syncGrants method. It will save you a nested if.
Inside syncGrants your first line can be:
if len(grants) == 0 { return nil }
## Summary - Adds `RoleClient` to the WorkOS third-party package wrapping WorkOS role and membership APIs - Supports listing, creating, updating, and deleting org-scoped roles via raw HTTP - Supports listing org members, updating member role assignments, and fetching user details via WorkOS SDK - Uses a retryable HTTP client (30s timeout) for resilience - Exposes `NewRoleClientWithEndpoint` for hermetic testing against an `httptest.Server` Part of the RBAC feature split from #1970. <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/speakeasy-api/gram/pull/2011" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end -->
Summary
principal_grantstablegram-rbacflag (org-level sidebar nav + route guard)Key Architecture
principal_grantstable owns fine-grained scope/resource grantsorg-slug prefix, unique per organizationChanges
Server
impl.goFrontend
useListScopesAPIDatabase
principal_grantsmigration for fine-grained scope/resource grantsrolesandrole_assignmentstables (WorkOS manages these)Test plan
gram-rbacfeature flag in PostHog🤖 Generated with Claude Code