-
Notifications
You must be signed in to change notification settings - Fork 3
Mount IT contacts on the Organizations service #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,21 @@ export const operationHints: Record<string, OperationHint> = { | |
| name: 'list_authorized_applications', | ||
| }, | ||
|
|
||
| // -- IT contacts (mounted on ItContacts) -------------------------------------- | ||
| // Drop the `organization` prefix the path would otherwise produce, and name | ||
| // the invite/revoke sub-resources after the action. | ||
| 'GET /organizations/{organization_id}/it_contacts': { name: 'list_it_contacts' }, | ||
| 'POST /organizations/{organization_id}/it_contacts': { name: 'create_it_contact' }, | ||
| 'DELETE /organizations/{organization_id}/it_contacts/{contact_id}': { | ||
| name: 'delete_it_contact', | ||
| }, | ||
| 'POST /organizations/{organization_id}/it_contacts/{contact_id}/invite': { | ||
| name: 'invite_it_contact', | ||
| }, | ||
| 'POST /organizations/{organization_id}/it_contacts/{contact_id}/revoke': { | ||
| name: 'revoke_it_contact', | ||
| }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the change above, what do the SDK methods end up looking like?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Node (other langs get the same names in their own casing): await workos.organizations.listItContacts({ organizationId });
await workos.organizations.createItContact({ organizationId, email });
await workos.organizations.deleteItContact({ organizationId, contactId });
await workos.organizations.inviteItContact({ organizationId, contactId, intents: ['sso'] });
await workos.organizations.revokeItContact({ organizationId, contactId });Without the hints they'd be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do we do for organization domains currently?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Organization domains are the other shape: the API itself is top-level, not nested — IT contacts can't follow that without changing the shipped API paths, which is the option we ruled out above. The closest nested precedent is Groups ( |
||
|
|
||
| // -- External ID lookups (not derivable from path) ---------------------------- | ||
| 'GET /organizations/external_id/{external_id}': { name: 'get_organization_by_external_id' }, | ||
| 'GET /user_management/users/external_id/{external_id}': { name: 'get_user_by_external_id' }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this as a top-level mount if we created this as a subresource? Should we create it as a top-level resource in the API instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mount only changes the generated SDK surface, not the API — the HTTP paths stay
/organizations/{organization_id}/it_contactseither way.Precedent for keeping it top-level: Groups is exactly the same shape (
/organizations/{organizationId}/groups,.../groups/{groupId}/organization-memberships) and is mounted as its ownGroupsservice, withorganizationIdpassed as an option —workos.groups.listGroups({ organizationId }). Org API keys and authorized applications go the other way (mounted ontoApiKeys/Organizations), which is why the policy needs an explicit rule here. IT contacts have their own object type (it_contact), their own lifecycle and their own actions (invite/revoke), so they read more like Groups than likeorganizations.listAuthorizedApplications, and mounting them onOrganizationswould giveworkos.organizations.listItContacts(...)alongside unrelated org CRUD. jonatas confirmedworkos.itContacts.*as the desired surface.Making it a top-level resource in the API (
/it_contacts?organization_id=...) is a bigger, separate call — it would break the shipped endpoints, and no SDK exposes them yet so nothing forces it now. Happy to switch this toOrganizations(or hold pending an API change) if you'd rather; that's just editing these two policy entries and regenerating workos/workos-node#1681.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More-so what I'm saying is it seems like we want it to be both a top-level resource and a sub-resource, but feels like we should choose one.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... It needs to be a subresource of organization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it — the inconsistency you're pointing at is real: API says subresource, this policy says top-level.
Two consistent options:
Organizations→workos.organizations.listItContacts({ organizationId }). Two-line change here plus a regen of feat: Add IT Contacts API workos-node#1681./it_contactswithorganization_id) and keep this mount. That's a breaking change to already-shipped endpoints, so it needs its own decision — though no SDK exposes IT contacts yet, so the blast radius is limited to direct API users.Worth noting Groups sits in the same "inconsistent" spot today (
/organizations/{organizationId}/groupsmounted as top-levelGroups), so option 1 makes IT contacts stricter than the closest existing precedent. I don't have a stake either way — @jonatascastro12 asked forworkos.itContacts.*, so I'll follow whichever you two land on. Tell me which and I'll push it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the API as-is and mount on
OrganizationsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — option 1 is pushed here (
OrganizationsItContacts: 'Organizations', commit 7739c4b), and workos/workos-node#1681 now exposesworkos.organizations.listItContacts/createItContact/deleteItContact/inviteItContact/revokeItContact({ organizationId, ... }). CI green on both.