Conversation
swaroopAkkineniWorkos
changed the title
|
@greptile review |
Greptile SummaryThis PR introduces the foundational scaffolding for the FGA (Fine-Grained Authorization) Go SDK package ( Key concerns found during review:
Confidence Score: 2/5
Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class Client {
+APIKey string
+HTTPClient retryablehttp.HttpClient
+Endpoint string
+JSONEncode func
-once sync.Once
+init()
+CreateEnvironmentRole()
+ListEnvironmentRoles()
+GetEnvironmentRole()
+UpdateEnvironmentRole()
+CreateOrganizationRole()
+ListOrganizationRoles()
+GetOrganizationRole()
+UpdateOrganizationRole()
+DeleteOrganizationRole()
+SetEnvironmentRolePermissions()
+AddEnvironmentRolePermission()
+SetOrganizationRolePermissions()
+AddOrganizationRolePermission()
+RemoveOrganizationRolePermission()
+CreatePermission()
+ListPermissions()
+GetPermission()
+UpdatePermission()
+DeletePermission()
+GetResource()
+CreateResource()
+UpdateResource()
+DeleteResource()
+ListResources()
+GetResourceByExternalId()
+UpdateResourceByExternalId()
+DeleteResourceByExternalId()
+Check()
+ListRoleAssignments()
+AssignRole()
+RemoveRole()
+RemoveRoleAssignment()
+ListResourcesForMembership()
+ListMembershipsForResource()
+ListMembershipsForResourceByExternalId()
}
class ResourceIdentifier {
<<interface>>
+resourceIdentifierParams() map
}
class ResourceIdentifierById {
+ResourceId string
}
class ResourceIdentifierByExternalId {
+ResourceExternalId string
+ResourceTypeSlug string
}
class ParentResourceIdentifier {
<<interface>>
+parentResourceIdentifierParams() map
}
class ParentResourceIdentifierById {
+ParentResourceId string
}
class ParentResourceIdentifierByExternalId {
+ParentResourceExternalId string
+ParentResourceTypeSlug string
}
class EnvironmentRole {
+Id string
+Slug string
+Permissions []string
}
class OrganizationRole {
+Id string
+Slug string
+Permissions []string
}
class Permission {
+Id string
+Slug string
}
class AuthorizationResource {
+Id string
+ExternalId string
+OrganizationId string
}
class RoleAssignment {
+Id string
+Role RoleAssignmentRole
+Resource RoleAssignmentResource
}
class AuthorizationCheckResult {
+Authorized bool
}
ResourceIdentifier <|.. ResourceIdentifierById
ResourceIdentifier <|.. ResourceIdentifierByExternalId
ParentResourceIdentifier <|.. ParentResourceIdentifierById
ParentResourceIdentifier <|.. ParentResourceIdentifierByExternalId
Client ..> EnvironmentRole : returns
Client ..> OrganizationRole : returns
Client ..> Permission : returns
Client ..> AuthorizationResource : returns
Client ..> RoleAssignment : returns
Client ..> AuthorizationCheckResult : returns
Client ..> ResourceIdentifier : uses
Client ..> ParentResourceIdentifier : uses
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // GetEnvironmentRole gets an environment role by slug. | ||
| func (c *Client) GetEnvironmentRole(ctx context.Context, opts GetEnvironmentRoleOpts) (EnvironmentRole, error) { | ||
| c.once.Do(c.init) | ||
| return EnvironmentRole{}, errors.New("not implemented") | ||
| } | ||
|
|
||
| // UpdateEnvironmentRole updates an environment role. | ||
| func (c *Client) UpdateEnvironmentRole(ctx context.Context, opts UpdateEnvironmentRoleOpts) (EnvironmentRole, error) { | ||
| c.once.Do(c.init) |
There was a problem hiding this comment.
Missing ListMetadata in environment and organization role list responses
ListEnvironmentRolesResponse and ListOrganizationRolesResponse do not include pagination metadata, while all other list responses (ListPermissionsResponse, ListAuthorizationResourcesResponse, ListRoleAssignmentsResponse, ListAuthorizationOrganizationMembershipsResponse) consistently include ListMetadata:
type ListEnvironmentRolesResponse struct {
Data []EnvironmentRole `json:"data"`
ListMetadata common.ListMetadata `json:"list_metadata"`
}
type ListOrganizationRolesResponse struct {
Data []OrganizationRole `json:"data"`
ListMetadata common.ListMetadata `json:"list_metadata"`
}If these APIs support pagination (which is likely given the pattern used elsewhere), callers will have no way to iterate through pages. This could be intentional if the API returns all results in a single call, but it's worth confirming.
| func (c *Client) AddEnvironmentRolePermission(ctx context.Context, opts AddEnvironmentRolePermissionOpts) (EnvironmentRole, error) { | ||
| c.once.Do(c.init) | ||
| return EnvironmentRole{}, errors.New("not implemented") | ||
| } | ||
|
|
||
| // SetOrganizationRolePermissions sets permissions for an organization role. |
There was a problem hiding this comment.
Inconsistent omitempty on Description in update opts
Across all update opts structs (UpdateEnvironmentRoleOpts, UpdateOrganizationRoleOpts, UpdatePermissionOpts, UpdateAuthorizationResourceOpts, UpdateResourceByExternalIdOpts), Name uses omitempty but Description does not:
Name *string `json:"name,omitempty"`
Description *string `json:"description"` // always serialized, even when nil → JSON nullWhen Description is nil, it will be serialized as null in the JSON body, which will actively clear the description on the server side. This may be intentional (to allow callers to explicitly clear a description), but it creates an asymmetry: callers who omit Description will unintentionally wipe it from the resource.
A more explicit pattern would be to either document this behaviour clearly, or use a sentinel/wrapper type to distinguish "not set" from "explicitly cleared". This same pattern appears at the Description field of all five update opts types.
|
|
||
| // DeletePermission deletes a permission. | ||
| func (c *Client) DeletePermission(ctx context.Context, opts DeletePermissionOpts) error { | ||
| c.once.Do(c.init) | ||
| return errors.New("not implemented") | ||
| } | ||
|
|
||
| // GetResource gets a resource by Id. | ||
| func (c *Client) GetResource(ctx context.Context, opts GetAuthorizationResourceOpts) (AuthorizationResource, error) { | ||
| c.once.Do(c.init) | ||
| return AuthorizationResource{}, errors.New("not implemented") | ||
| } | ||
|
|
||
| // CreateResource creates a new resource. | ||
| func (c *Client) CreateResource(ctx context.Context, opts CreateAuthorizationResourceOpts) (AuthorizationResource, error) { | ||
| c.once.Do(c.init) | ||
| return AuthorizationResource{}, errors.New("not implemented") | ||
| } | ||
|
|
||
| // UpdateResource updates a resource. | ||
| func (c *Client) UpdateResource(ctx context.Context, opts UpdateAuthorizationResourceOpts) (AuthorizationResource, error) { | ||
| c.once.Do(c.init) | ||
| return AuthorizationResource{}, errors.New("not implemented") | ||
| } | ||
|
|
||
| // DeleteResource deletes a resource. |
There was a problem hiding this comment.
Missing RemoveEnvironmentRolePermission method
There is an asymmetry between environment and organization role permission management:
| Operation | Environment Role | Organization Role |
|---|---|---|
| Set (replace all) | ✅ SetEnvironmentRolePermissions |
✅ SetOrganizationRolePermissions |
| Add (one) | ✅ AddEnvironmentRolePermission |
✅ AddOrganizationRolePermission |
| Remove (one) | ❌ missing | ✅ RemoveOrganizationRolePermission |
If the WorkOS FGA API supports removing individual permissions from environment roles, a corresponding RemoveEnvironmentRolePermission function and opts type should be added here to keep the API surface consistent. If this is intentionally omitted (e.g., the API doesn't support it at the environment scope), a brief comment explaining the asymmetry would help future maintainers.
| // GetPermissionOpts contains the options for getting a permission. | ||
| type GetPermissionOpts struct { | ||
| Slug string `json:"-"` | ||
| } | ||
|
|
||
| // UpdatePermissionOpts contains the options for updating a permission. | ||
| type UpdatePermissionOpts struct { | ||
| Slug string `json:"-"` | ||
| Name *string `json:"name,omitempty"` | ||
| Description *string `json:"description"` | ||
| } | ||
|
|
||
| // DeletePermissionOpts contains the options for deleting a permission. | ||
| type DeletePermissionOpts struct { | ||
| Slug string `json:"-"` | ||
| } | ||
|
|
There was a problem hiding this comment.
sync.Once prevents re-initialization after first method call
The sync.Once pattern used here means init() fires exactly once — on the first method call. This creates a subtle footgun: if HTTPClient or Endpoint are customized on the Client struct after the first method call, the changes will be silently ignored because init() has already run and the once can never be reset.
For the DefaultClient pattern (where SetAPIKey is called before use), this is fine. However, for consumers who construct a custom Client{...} and then call a method before setting all fields, it can lead to hard-to-debug issues.
Consider following the pattern used elsewhere in the workos-go SDK to verify this is consistent, or document the expected initialization order explicitly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Add reusable path segment constants matching the Python SDK pattern for authorization API endpoints.
swaroopAkkineniWorkos
changed the title
1 participant
Uh oh!
There was an error while loading. Please reload this page.