-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add typing for Directory Sync events #282
Changes from 8 commits
9b4f7a9
692b5cb
44826fa
75578f4
622036f
49bb7ba
7e98632
d6a137f
548dc53
1565d73
5d88c9f
3ce4d32
88f0ec1
b5c2a6b
bd9b1f4
6c14e8a
bd8020c
3fea3d1
6025c77
e8bb412
7d82288
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 |
---|---|---|
@@ -0,0 +1,71 @@ | ||
from typing import Dict, Any, List | ||
from enum import Enum | ||
|
||
JsonDict = Dict[str, Any] | ||
|
||
|
||
class DirectoryType(Enum): | ||
AZURE_SCIM_v2 = "azure scim v2.0" | ||
BAMBOO_HR = "bamboohr" | ||
BREATHE_HR = "breathe hr" | ||
CEZANNE_HR = "cezanne hr" | ||
CYBERARK_SCIM_v2 = "cyperark scim v2.0" | ||
FOURTH_HR = "fourth hr" | ||
GENERIC_SCIM_v2 = "generic scim v2.0" | ||
GOOGLE = "gsuite directory" | ||
GUSTO = "gusto" | ||
HIBOB = "hibob" | ||
JUMPCLOUD_SCIM_v2 = "jump cloud scim v2.0" | ||
OKTA_SCIM_v2 = "okta scim v2.0" | ||
ONELOGIN_SCIM_v2 = "onelogin scim v2.0" | ||
PEOPLE_HR = "people hr" | ||
PERSONIO = "personio" | ||
PINGFEDERATE_SCIM_v2 = "pingfederate scim v2.0" | ||
RIPPLING_SCIM_v2 = "rippling v2.0" | ||
SFTP = "sftp" | ||
SFTP_WORKDAY = "sftp workday" | ||
WORKDAY = "workday" | ||
|
||
|
||
class DirectoryState(Enum): | ||
ACTIVE = "active" | ||
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. we never show 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. I don't believe there's ever an directory event emitted with 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. Correct 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. I don't see where we exclude validating directories: https://github.com/workos/workos/blob/1b4c7a6a83b49ec3a605ff4f2f5dd17848e6085d/packages/api-services/src/directories/directories.service.ts#L322 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. Via the directory API we definitely return other directory states, but the type is specifically event-scoped where we'd only have active or deleting directories. If we want to enumerate all states, we could extract this enum from the event context. 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. So, is the idea that we are only typing the events and not the dsync APIs? 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. Ideally we should type all the things, but I think that's worth a separate PR. 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. Sure. But I'm don't think we should release the SDK until both the Dsync events and API resources are typed. 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. Yep, agreed. 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. Ended up adding some light typing for the APIs using existing objects. The list endpoints could use some better typing, but would require a size-able rework of the |
||
DELETING = "deleting" | ||
|
||
|
||
class OrganizationDomain: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.id: str = attributes["id"] | ||
self.domain: str = attributes["domain"] | ||
|
||
|
||
class DirectoryEvent: | ||
def __init__(self) -> None: | ||
self.id: str = attributes["id"] | ||
self.name: str = attributes["name"] | ||
self.type: DirectoryType = DirectoryType(attributes["type"]) | ||
self.state: DirectoryState = DirectoryState.ACTIVE | ||
self.domains: List[OrganizationDomain] = [] | ||
for domain in attributes["domains"]: | ||
self.domains.push(OrganizationDomain(domain)) | ||
self.organization_id: str = attributes["organization_id"] | ||
self.created_at: str = attributes["created_at"] | ||
self.updated_at: str = attributes["updated_at"] | ||
self.external_key: str = attributes["external_key"] | ||
# always 'directory' for this event | ||
self.object: str = attributes["object"] | ||
mattgd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class DirectoryActivatedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.event: str = "dsync.activated" | ||
self.id: str = attributes["id"] | ||
self.created_at = attributes["created_at"] | ||
self.data: DirectoryEvent = DirectoryEvent(attributes["data"]) | ||
|
||
|
||
class DirectoryDeletedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.event: str = "dsync.deleted" | ||
self.id: str = attributes["id"] | ||
self.created_at = attributes["created_at"] | ||
self.data: DirectoryEvent = DirectoryEvent(attributes["data"]) | ||
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. We can't use this same type here. The directory deleted event is not the same shape as the directory activated event: https://workos.com/docs/events/directory-sync It uses the directory type, not the legacy directory type with extra fields: https://github.com/workos/workos/blob/e95130bf78e01b202c7e7417adf36135cfc5f033/packages/api-json-models/src/events/directory-deleted.ts#L9 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. Good catch, I'll update. Is 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. Per the events JSON models, it is accurate. As for "here to stay", I think it contains fields that are no longer necessary. But I'm dubious about the dsync team prioritizing an effort to remove the unnecessary fields. So, likely here to stay for a while. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
from typing import Dict, Any | ||
|
||
JsonDict = Dict[str, Any] | ||
|
||
|
||
class DirectoryGroupEvent: | ||
def __init__(self) -> None: | ||
self.id: str = attributes["id"] | ||
self.name: str = attributes["name"] | ||
self.idp_id: str = attributes["idp_id"] | ||
self.directory_id: str = attributes["directory_id"] | ||
self.organization_id: str = attributes["organization_id"] | ||
self.created_at: str = attributes["created_at"] | ||
self.updated_at: str = attributes["updated_at"] | ||
self.raw_attributes: JsonDict = attributes.get("raw_attributes", {}) | ||
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. @PavanKulkarni didn't we stop emitting 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. Let me confirm this in LD. 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. yes only 3 teams still depend on the flag. That being said we still send |
||
self.previous_attributes: JsonDict = attributes.get("previous_attributes", {}) | ||
# always 'directory_group' for this event | ||
self.object: str = attributes["object"] | ||
|
||
|
||
class DirectoryGroupCreatedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.event: str = "dsync.group.created" | ||
self.id: str = attributes["id"] | ||
self.created_at: str = attributes["created_at"] | ||
self.data: DirectoryGroupEvent = DirectoryGroupEvent(attributes["data"]) | ||
|
||
|
||
class DirectoryGroupDeletedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.event: str = "dsync.group.deleted" | ||
self.id: str = attributes["id"] | ||
self.created_at: str = attributes["created_at"] | ||
self.data: DirectoryGroupEvent = DirectoryGroupEvent(attributes["data"]) | ||
|
||
|
||
class DirectoryGroupUpdatedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.event: str = "dsync.group.updated" | ||
self.id: str = attributes["id"] | ||
self.created_at: str = attributes["created_at"] | ||
self.data: DirectoryGroupEvent = DirectoryGroupEvent(attributes["data"]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
from typing import Dict, Any, Optional, List | ||
from enum import Enum | ||
|
||
JsonDict = Dict[str, Any] | ||
|
||
|
||
class DirectoryUserRole: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.slug: str = attributes["slug"] | ||
|
||
|
||
class DirectoryUserEmail: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.type: Optional[str] = attributes.get("type") | ||
self.value: Optional[str] = attributes.get("value") | ||
mattgd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.primary: Optional[bool] = attributes.get("primary") | ||
|
||
|
||
class DirectoryUserState(Enum): | ||
ACTIVE = "active" | ||
INACTIVE = "inactive" | ||
|
||
|
||
class DirectoryUserEvent: | ||
def __init__(self) -> None: | ||
self.id: str = attributes["id"] | ||
self.name: str = attributes["name"] | ||
mattgd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.idp_id: str = attributes["idp_id"] | ||
self.directory_id: str = attributes["directory_id"] | ||
self.organization_id: str = attributes["organization_id"] | ||
self.first_name: Optional[str] = attributes.get("first_name") | ||
self.last_name: Optional[str] = attributes.get("last_name") | ||
self.job_title: Optional[str] = attributes.get("job_title") | ||
self.emails: List[DirectoryUserEmail] = [] | ||
for email in attributes.get("emails"): | ||
self.emails.push(DirectoryUserEmail(email)) | ||
self.username: Optional[str] = attributes.get("username") | ||
self.state: DirectoryUserState = DirectoryUserState(attributes["state"]) | ||
self.custom_attributes: JsonDict = attributes.get("custom_attributes", {}) | ||
self.raw_attributes: JsonDict = attributes.get("raw_attributes", {}) | ||
self.created_at: str = attributes["created_at"] | ||
self.updated_at: str = attributes["updated_at"] | ||
self.role: Optional[DirectoryUserRole] = ( | ||
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. Is this actually optional anymore? 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. I didn't think so, but it's still optional in the API event schema, so will keep as-is until the API is updated. 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. It's the API resource that led me to ask this question. The role here always exists when returning a directory user: https://github.com/workos/workos/blob/e95130bf78e01b202c7e7417adf36135cfc5f033/packages/api/src/directory-users/directory-users.controller.ts#L340 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. It's optional! We don't return it on non-scim/Google users 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. 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. @ameesha but in the code I referenced I see that the serializer accepts 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. yeah the role is always grabbed but in the serializer (line i sent), we only use it for scim/google dirs 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. nothing cares about whether the role is relevant in the directory type or not but the serializer. it's the only* thing that checks that logic (cause we create and set roles across the board). `* there are other checks for directory type so we can end recalculations early but if we removed it, nothing would really change |
||
DirectoryUserRole(attributes["role"]) if attributes.get("role") else None | ||
) | ||
self.previous_attributes: JsonDict = attributes.get("previous_attributes", {}) | ||
# always 'directory_user' for this event | ||
self.object: str = attributes["object"] | ||
|
||
|
||
class DirectoryUserCreatedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.id: str = attributes["id"] | ||
self.event: str = "dsync.user.created" | ||
self.created_at: str = attributes["created_at"] | ||
self.data: DirectoryUserEvent = DirectoryUserEvent(attributes["data"]) | ||
|
||
|
||
class DirectoryUserDeletedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.id: str = attributes["id"] | ||
self.event: str = "dsync.user.deleted" | ||
self.created_at: str = attributes["created_at"] | ||
self.data: DirectoryUserEvent = DirectoryUserEvent(attributes["data"]) | ||
|
||
|
||
class DirectoryUserUpdatedEvent: | ||
def __init__(self, attributes: JsonDict) -> None: | ||
self.id: str = attributes["id"] | ||
self.event: str = "dsync.user.updated" | ||
self.created_at: str = attributes["created_at"] | ||
self.data: DirectoryUserEvent = DirectoryUserEvent(attributes["data"]) |
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 need S3 or plain Rippling here, or are those totally deprecated?
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 S3, Rippling (non-SCIM) and Gusto are completely deprecated
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, so we can remove Gusto from this list also.
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.
According to mode, these are the directory types that actually exist in produiction:
We don't actually have any Personio directories, which is no big deal.
But we do have Rippling directories (non-SCIM) in the DB. That's not reflected here.
We don't actually have any "untyped" directories in prod anymore, so we don't need to worry about this
pending
state: https://github.com/workos/workos/blob/1b4c7a6a83b49ec3a605ff4f2f5dd17848e6085d/packages/api/src/directories/directories.controller.ts#L63We should have a KTLO item to clean that up.
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 was for the events list so we wouldn't have events with these types anymore.
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.
Nvm catching up to see we're doing API as well now lol