Skip to content

feat: step8 role assignments#520

Open
swaroopAkkineniWorkos wants to merge 14 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step8-role-assignments
Open

feat: step8 role assignments#520
swaroopAkkineniWorkos wants to merge 14 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step8-role-assignments

Conversation

@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor

Description

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

@linear
Copy link
Copy Markdown

linear bot commented Mar 6, 2026

@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile review

@workos workos deleted a comment from greptile-apps bot Mar 10, 2026
@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile re-review plz

@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile re-review

Swaroop Akkineni added 2 commits March 14, 2026 22:50
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile re-review plz

@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile re-review plz

@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile re-review plz

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 15, 2026

Greptile Summary

This PR implements the four previously-stubbed role assignment methods (ListRoleAssignments, AssignRole, RemoveRole, RemoveRoleAssignment) in the FGA authorization client, along with comprehensive unit tests. It also renames the Resource field to ResourceIdentifier in AssignRoleOpts and RemoveRoleOpts for clarity, and adds url:"-" struct tags to path-parameter fields across several opts structs to prevent them from accidentally leaking into URL query strings when query.Values() is called.

  • ListRoleAssignments correctly builds the URL from OrganizationMembershipId, applies default limit (10) and order (desc) before encoding query parameters, and decodes the paginated response.
  • AssignRole manually constructs the JSON body by merging role_slug with the output of ResourceIdentifier.resourceIdentifierParams(), supporting both ResourceIdentifierById and ResourceIdentifierByExternalId variants.
  • RemoveRole uses the same body-building strategy as AssignRole but issues an HTTP DELETE with a body — intentional for this API's design where role removal is identified by slug and resource rather than a stable ID.
  • RemoveRoleAssignment issues a body-less DELETE to the role assignment's specific URL path.
  • The url:"-" tag additions are a correctness fix: without them, query.Values() could encode path-parameter fields into the query string for list operations.

Confidence Score: 4/5

  • Safe to merge — implementations follow established patterns and tests cover the key paths; only minor test-quality issues found.
  • All four method implementations are consistent with the existing codebase patterns. The url:"-" tag additions correctly prevent path parameters from being serialized into query strings. Tests are thorough, covering response deserialization, pagination parameters, and HTTP error handling. The only issues identified are a test that uses both Before and After cursor parameters simultaneously (which is typically invalid in cursor-based pagination and could mask real bugs), and some test subtests that capture capturedRawQuery without asserting on it.
  • No files require special attention beyond the minor test quality issues in pkg/authorization/client_test.go.

Important Files Changed

Filename Overview
pkg/authorization/client.go Implements the four previously-stubbed role assignment methods (ListRoleAssignments, AssignRole, RemoveRole, RemoveRoleAssignment) and renames ResourceResourceIdentifier in AssignRoleOpts/RemoveRoleOpts. Also adds url:"-" tags to path-parameter fields in several opts structs to prevent them from appearing in encoded query strings. Implementation follows existing patterns correctly.
pkg/authorization/client_test.go Adds comprehensive table-driven tests for the four new methods using a reusable test client helper. Covers happy paths, pagination parameters, and HTTP error cases. Minor issues: a "passes all parameters" subtest uses both Before and After cursors simultaneously (invalid in cursor-based pagination), and several subtests capture capturedRawQuery without asserting on it.
pkg/authorization/authorization_test.go Adds package-level integration-style tests for the four role assignment operations using the package DefaultClient. Covers both ResourceIdentifierById and ResourceIdentifierByExternalId variants. Clean and straightforward.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Client
    participant WorkOS API

    Caller->>Client: ListRoleAssignments(opts)
    Client->>Client: Apply defaults (limit=10, order=desc)
    Client->>WorkOS API: GET /authorization/organization_memberships/{id}/role_assignments?limit=&order=&...
    WorkOS API-->>Client: 200 ListRoleAssignmentsResponse
    Client-->>Caller: ListRoleAssignmentsResponse, nil

    Caller->>Client: AssignRole(opts)
    Client->>Client: Build body {role_slug, resource_id | resource_external_id+resource_type_slug}
    Client->>WorkOS API: POST /authorization/organization_memberships/{id}/role_assignments
    WorkOS API-->>Client: 200 RoleAssignment
    Client-->>Caller: RoleAssignment, nil

    Caller->>Client: RemoveRole(opts)
    Client->>Client: Build body {role_slug, resource_id | resource_external_id+resource_type_slug}
    Client->>WorkOS API: DELETE /authorization/organization_memberships/{id}/role_assignments (with body)
    WorkOS API-->>Client: 204 No Content
    Client-->>Caller: nil

    Caller->>Client: RemoveRoleAssignment(opts)
    Client->>WorkOS API: DELETE /authorization/organization_memberships/{id}/role_assignments/{ra_id}
    WorkOS API-->>Client: 204 No Content
    Client-->>Caller: nil
Loading

Comments Outside Diff (1)

  1. pkg/authorization/client_test.go, line 741-756 (link)

    Simultaneous Before and After cursors in "passes all parameters" test

    Setting both Before and After cursor parameters simultaneously in a cursor-based pagination test represents invalid API usage. In cursor-based pagination, before and after are mutually exclusive — using both at the same time is undefined behavior and most API implementations will either return an error or silently ignore one of them.

    This test will always pass against the mock server (which blindly echos back the fixture response), but it doesn't verify real API behavior and sets a confusing precedent for users reading the tests as documentation.

    Consider splitting this into two focused subtests:

    t.Run("passes before cursor with other params", func(t *testing.T) {
        // Limit + Before + Order
    })
    
    t.Run("passes after cursor with other params", func(t *testing.T) {
        // Limit + After + Order
    })

Last reviewed commit: 76e7c7c

@swaroopAkkineniWorkos swaroopAkkineniWorkos marked this pull request as ready for review March 16, 2026 21:00
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested a review from a team as a code owner March 16, 2026 21:00
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested review from mattgd and removed request for a team March 16, 2026 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants