Skip to content

feat: step9 membership queries#521

Open
swaroopAkkineniWorkos wants to merge 13 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step9-membership-queries
Open

feat: step9 membership queries#521
swaroopAkkineniWorkos wants to merge 13 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step9-membership-queries

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

@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile re-review

@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 15, 2026 12:37
@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 15, 2026 13:03
@workos workos deleted a comment from greptile-apps bot Mar 15, 2026
@swaroopAkkineniWorkos
Copy link
Copy Markdown
Contributor Author

@greptile re-review

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

@greptile re-review

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

@greptile re-review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR implements three new membership-query methods on the authorization.ClientListResourcesForMembership, ListMembershipsForResource, and ListMembershipsForResourceByExternalId — together with the supporting types (MembershipStatus, Assignment, AuthorizationOrganizationMembership, and the corresponding Opts/Response structs). These are the first fully-working method implementations in the package; all prior methods are stubs returning ErrNotImplemented.

Notable issues found:

  • Missing path-parameter validation (logic): None of the three new methods guard against empty values for the IDs they embed in the URL path (OrganizationMembershipId, ResourceId, OrganizationId, ResourceTypeSlug, ExternalId). An empty value passes through url.PathEscape silently and produces a malformed double-slash URL (e.g. /authorization/resources//organization_memberships), leading to a confusing 404 or unexpected API error rather than a clear client-side validation error. The existing APIKey check pattern should be extended to cover these fields.

  • PermissionSlug missing omitempty (style): Unlike every other optional query parameter (Limit, Before, After, Order, Assignment), the PermissionSlug field in all three new Opts structs carries no omitempty annotation, causing permission_slug= to be sent unconditionally even when the caller omits it.

  • The comprehensive table-driven tests in client_test.go cover the happy path and error scenarios well. The simpler smoke tests in authorization_test.go depend on the package-level wrapper functions (e.g. ListResourcesForMembership(ctx, opts)) that are expected to live in a separate, unmodified file — compilation will fail if those wrappers do not already exist.

Confidence Score: 3/5

  • Safe to merge after adding path-parameter validation guards to prevent malformed API requests.
  • The core HTTP machinery and response deserialization are correct and well-tested. The main concern is the absence of empty-value checks for required URL path segments across all three new methods, which can silently produce malformed requests at runtime. The missing omitempty on PermissionSlug is a lower-severity style issue. No security or data-integrity risks identified.
  • pkg/authorization/client.go — specifically the three new method implementations and the Opts struct definitions for PermissionSlug tagging.

Important Files Changed

Filename Overview
pkg/authorization/client.go Adds three fully-implemented methods (ListResourcesForMembership, ListMembershipsForResource, ListMembershipsForResourceByExternalId) along with their request/response types. Missing validation for required URL path parameters in all three methods could produce malformed URLs at runtime.
pkg/authorization/client_test.go Comprehensive table-driven tests for the three new client methods, covering pagination, ordering, cursor, assignment filter, and error scenarios. Test helper functions (jsonResponseHandler, noContentHandler, newAuthorizationTestClient) are also defined here and shared with authorization_test.go.
pkg/authorization/authorization_test.go Smoke tests exercising the three new methods through the package-level DefaultClient wrappers. Tests rely on jsonResponseHandler defined in client_test.go; package-level wrapper functions (ListResourcesForMembership, ListMembershipsForResource, ListMembershipsForResourceByExternalId) are expected to exist in a separate file not modified in this PR.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Client
    participant WorkOSAPI

    note over Client: ListResourcesForMembership
    Caller->>Client: ListResourcesForMembership(ctx, opts)
    Client->>Client: once.Do(init) — lazy setup
    Client->>Client: validate APIKey
    Client->>Client: set default Limit + Order
    Client->>WorkOSAPI: GET /authorization/organization_memberships/{id}/resources?permission_slug=...&limit=...&order=...&[parent_resource_id=...]
    WorkOSAPI-->>Client: ListAuthorizationResourcesResponse
    Client-->>Caller: response, err

    note over Client: ListMembershipsForResource
    Caller->>Client: ListMembershipsForResource(ctx, opts)
    Client->>Client: once.Do(init)
    Client->>Client: validate APIKey
    Client->>Client: set default Limit + Order
    Client->>WorkOSAPI: GET /authorization/resources/{resource_id}/organization_memberships?permission_slug=...&assignment=...
    WorkOSAPI-->>Client: ListAuthorizationOrganizationMembershipsResponse
    Client-->>Caller: response, err

    note over Client: ListMembershipsForResourceByExternalId
    Caller->>Client: ListMembershipsForResourceByExternalId(ctx, opts)
    Client->>Client: once.Do(init)
    Client->>Client: validate APIKey
    Client->>Client: set default Limit + Order
    Client->>WorkOSAPI: GET /authorization/organizations/{org_id}/resources/{type_slug}/{ext_id}/organization_memberships?permission_slug=...
    WorkOSAPI-->>Client: ListAuthorizationOrganizationMembershipsResponse
    Client-->>Caller: response, err
Loading

Comments Outside Diff (2)

  1. pkg/authorization/client.go, line 725-729 (link)

    Missing validation for required path parameters

    ListResourcesForMembership constructs the URL path using opts.OrganizationMembershipId without validating it is non-empty. If the caller passes an empty OrganizationMembershipId, url.PathEscape("") returns "", producing a malformed path like /authorization/organization_memberships//resources. This will either 404, match an unexpected route, or return a confusing error.

    The same issue exists in:

    • ListMembershipsForResource (pkg/authorization/client.go:787-791) — opts.ResourceId is not checked
    • ListMembershipsForResourceByExternalId (pkg/authorization/client.go:843-848) — opts.OrganizationId, opts.ResourceTypeSlug, and opts.ExternalId are not checked

    Consider adding explicit guards similar to the existing APIKey check:

    if opts.OrganizationMembershipId == "" {
        return ListAuthorizationResourcesResponse{}, errors.New("authorization: OrganizationMembershipId is required")
    }
  2. pkg/authorization/client.go, line 473-481 (link)

    PermissionSlug missing omitempty tag

    PermissionSlug in ListResourcesForMembershipOpts (and the same field in ListMembershipsForResourceOpts at line 494 and ListMembershipsForResourceByExternalIdOpts at line 507) is declared without omitempty:

    PermissionSlug string `url:"permission_slug"`

    This means permission_slug= is always appended to the request URL, even when the caller does not provide a value. Every other optional query field (Limit, Before, After, Order, Assignment) correctly uses omitempty. If permission_slug can be absent (i.e., the API does not require it), the tag should be:

    If it is truly required by the API, consider adding an explicit empty-value guard on the client side so callers get a clear error instead of an ambiguous API response.

Last reviewed commit: 65b8385

@swaroopAkkineniWorkos swaroopAkkineniWorkos marked this pull request as ready for review March 16, 2026 21:01
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested a review from a team as a code owner March 16, 2026 21:01
@swaroopAkkineniWorkos swaroopAkkineniWorkos requested review from amygdalama and removed request for a team March 16, 2026 21:01
}

if opts.Order == "" {
opts.Order = common.Desc
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment in other PR about default order

c.once.Do(c.init)
return ListAuthorizationResourcesResponse{}, errors.New("not implemented")

if c.APIKey == "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this method have this but others in different PRs don't?

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