Skip to content

feat: step2-org-roles#514

Open
swaroopAkkineniWorkos wants to merge 15 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step2-org-roles
Open

feat: step2-org-roles#514
swaroopAkkineniWorkos wants to merge 15 commits intoENT-5353-base-fga-for-go-sdkfrom
ENT-5353-step2-org-roles

Conversation

@swaroopAkkineniWorkos
Copy link
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

linear bot commented Mar 6, 2026

@swaroopAkkineniWorkos swaroopAkkineniWorkos changed the title org-roles feat: step2-org-roles Mar 6, 2026
@swaroopAkkineniWorkos
Copy link
Contributor Author

@greptile review

@swaroopAkkineniWorkos
Copy link
Contributor Author

@greptile re-review

@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
Swaroop Akkineni added 2 commits March 16, 2026 12:48
@workos workos deleted a comment from greptile-apps bot Mar 16, 2026
@swaroopAkkineniWorkos
Copy link
Contributor Author

@greptile rereview plz

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

@greptile re-review

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

@greptile re-review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 16, 2026

Greptile Summary

This PR (step 2 of a multi-step FGA feature) adds organization role CRUD operations to the pkg/authorization package and removes the previously-added environment role package-level wrappers from authorization.go. URL path parameters are correctly encoded with url.PathEscape, and both client-level and package-level tests provide solid coverage of the five new operations.

Key observations:

  • SetOrganizationRolePermissionsOpts, AddOrganizationRolePermissionOpts, and RemoveOrganizationRolePermissionOpts are exported in client.go but have no corresponding client methods or package-level wrappers, shipping an incomplete API surface.
  • A large number of other type definitions (env role types, permission types, resource types, membership types, etc.) remain in client.go with no backing implementations — appropriate if they are scaffolding for later steps, but currently exported dead code.
  • Tests in authorization_test.go mutate the global DefaultClient variable without t.Parallel() guards; sequential today, but a latent data-race risk if parallelism is added later.

Confidence Score: 3/5

  • Safe to merge for the five implemented org role operations, but ships exported types with no backing implementations that may confuse SDK consumers.
  • The five org role CRUD methods are correctly implemented with proper URL encoding and are well-tested. However, org role permission opts types are exported with no methods behind them, and a large volume of other type scaffolding is also shipped without implementations, reducing the overall quality score.
  • pkg/authorization/client.go — the orphaned org role permission opts types (lines 321–340) and the broader question of whether all the unimplemented type definitions are intentional scaffolding or an oversight.

Important Files Changed

Filename Overview
pkg/authorization/client.go Adds five org role client methods (Create, List, Get, Update, Delete) with correct url.PathEscape usage, but retains a large number of type definitions with no corresponding client methods — including env role types removed from the public API and org role permission opts (SetOrganizationRolePermissionsOpts, AddOrganizationRolePermissionOpts, RemoveOrganizationRolePermissionOpts) that have no implementations.
pkg/authorization/authorization.go Removes environment role package-level wrappers and retains only the five org role wrappers (Create, List, Get, Update, Delete), which correctly delegate to the corresponding client methods.
pkg/authorization/client_test.go Well-structured client-level tests with path verification, request body capture, multiple sub-cases per operation, and HTTP error handling — good coverage for all five implemented org role methods.
pkg/authorization/authorization_test.go Package-level wrapper tests that mutate the global DefaultClient between tests; safe for sequential execution but would be a data race if tests are ever marked parallel. Coverage is otherwise adequate for the five org role operations.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant authorization.go as Package API
    participant Client as Client (client.go)
    participant WorkOS as WorkOS API

    Caller->>Package API: CreateOrganizationRole(ctx, opts)
    Package API->>Client: DefaultClient.CreateOrganizationRole(ctx, opts)
    Client->>Client: once.Do(init)
    Client->>WorkOS: POST /authorization/organizations/{orgId}/roles
    WorkOS-->>Client: 200 OrganizationRole JSON
    Client-->>Package API: OrganizationRole, nil
    Package API-->>Caller: OrganizationRole, nil

    Caller->>Package API: ListOrganizationRoles(ctx, opts)
    Package API->>Client: DefaultClient.ListOrganizationRoles(ctx, opts)
    Client->>WorkOS: GET /authorization/organizations/{orgId}/roles
    WorkOS-->>Client: 200 ListOrganizationRolesResponse JSON
    Client-->>Caller: ListOrganizationRolesResponse, nil

    Caller->>Package API: GetOrganizationRole(ctx, opts)
    Package API->>Client: DefaultClient.GetOrganizationRole(ctx, opts)
    Client->>WorkOS: GET /authorization/organizations/{orgId}/roles/{slug}
    WorkOS-->>Client: 200 OrganizationRole JSON
    Client-->>Caller: OrganizationRole, nil

    Caller->>Package API: UpdateOrganizationRole(ctx, opts)
    Package API->>Client: DefaultClient.UpdateOrganizationRole(ctx, opts)
    Client->>WorkOS: PATCH /authorization/organizations/{orgId}/roles/{slug}
    WorkOS-->>Client: 200 OrganizationRole JSON
    Client-->>Caller: OrganizationRole, nil

    Caller->>Package API: DeleteOrganizationRole(ctx, opts)
    Package API->>Client: DefaultClient.DeleteOrganizationRole(ctx, opts)
    Client->>WorkOS: DELETE /authorization/organizations/{orgId}/roles/{slug}
    WorkOS-->>Client: 204 No Content
    Client-->>Caller: nil
Loading

Last reviewed commit: 4d41ad1

@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 atainter and removed request for a team March 16, 2026 21:00
Swaroop Akkineni added 3 commits March 16, 2026 17:05
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.

1 participant