Update Okta Module to use Models #1243
Conversation
There was a problem hiding this comment.
Thanks for taking on the data model refactor and using the new Okta async APIs (and getting rid of a long-standing "miss last page" bug).
I gave this a first pass and my main blockers are:
- Getting rid of awssaml - we need to keep this.
- Okta group role transform efficiency: the list of group roles can be huge so there is a lot of wasted work here. Let's come up with a faster and cleaner way of doing this.
Since this is a big PR, we might need a few rounds. In future for faster merging it'd be better to make refactors true refactors with no new added functionality.
| @@ -1,31 +1,31 @@ | |||
| from setuptools import find_packages | |||
| from setuptools import setup | |||
|
|
|||
| __version__ = '0.81.0' | |||
| __version__ = "0.81.0" | |||
There was a problem hiding this comment.
Let's not include these changes since it's not directly related to this PR.
| id: PropertyRef = PropertyRef("id") | ||
| lastupdated: PropertyRef = PropertyRef("lastupdated", set_in_kwargs=True) | ||
| login: PropertyRef = PropertyRef("login") | ||
| email: PropertyRef = PropertyRef("email") |
There was a problem hiding this comment.
| email: PropertyRef = PropertyRef("email") | |
| email: PropertyRef = PropertyRef("email", extra_index=True) |
| @@ -0,0 +1,212 @@ | |||
| from dataclasses import dataclass | |||
There was a problem hiding this comment.
Let's look at https://github.com/lyft/cartography/blob/2fa2cb0aa54b74844d9711c8bd30f759bf315f9f/cartography/data/indexes.cypher#L217-L232
and add extra_index=True to each PropertyRef where needed, and then we can remove the line from indexes.cypher.
| class OktaApplicationNodeProperties(CartographyNodeProperties): | ||
| id: PropertyRef = PropertyRef("id") | ||
| lastupdated: PropertyRef = PropertyRef("lastupdated", set_in_kwargs=True) | ||
| # TODO: Find out when this is used |
There was a problem hiding this comment.
Nit: Can we remove the TODO here and replace it with a GitHub issue instead?
| last_updated: PropertyRef = PropertyRef("last_updated") | ||
| licensing_seat_count: PropertyRef = PropertyRef("licensing_seat_count") | ||
| name: PropertyRef = PropertyRef("name") | ||
| # TODO: Figure out profile |
There was a problem hiding this comment.
Nit: TODOs to GH issues.
| ) -> List[Dict[str, Any]]: | ||
| """ | ||
| Convert a list of Okta group roles into a format for Neo4j | ||
| :param okta_group_roles: List of Okta group roles |
There was a problem hiding this comment.
The datatype is OktaGroup. Double checking but is this comment correct?
| match_user = {**group_props, "user_id": group_member.id} | ||
| transformed_groups.append(match_user) | ||
| # Check to see if this group has any matching group roles | ||
| # TODO: Converting this into a hashmap would make perform better |
There was a problem hiding this comment.
This feels like a lot of wasted work since the group-role list can be very big. Let's take some time to think about restructuring.
| ) | ||
|
|
||
|
|
||
| # TODO: Authenticator's enrolled per user |
| okta_client, neo4j_session, common_job_parameters | ||
| ) | ||
|
|
||
| # TODO: Deprecate this while we determine a method of making it more generic |
There was a problem hiding this comment.
Blocker: This is still needed.
No description provided.