Skip to content
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

Update Okta Module to use Models #1243

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update Okta Module to use Models #1243

wants to merge 1 commit into from

Conversation

csanders-git
Copy link

No description provided.

Copy link
Contributor

@achantavy achantavy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Getting rid of awssaml - we need to keep this.
  2. 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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
email: PropertyRef = PropertyRef("email")
email: PropertyRef = PropertyRef("email", extra_index=True)

@@ -0,0 +1,212 @@
from dataclasses import dataclass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's look at

CREATE INDEX IF NOT EXISTS FOR (n:OktaOrganization) ON (n.id);
CREATE INDEX IF NOT EXISTS FOR (n:OktaOrganization) ON (n.lastupdated);
CREATE INDEX IF NOT EXISTS FOR (n:OktaUser) ON (n.id);
CREATE INDEX IF NOT EXISTS FOR (n:OktaUser) ON (n.email);
CREATE INDEX IF NOT EXISTS FOR (n:OktaUser) ON (n.lastupdated);
CREATE INDEX IF NOT EXISTS FOR (n:OktaGroup) ON (n.id);
CREATE INDEX IF NOT EXISTS FOR (n:OktaGroup) ON (n.name);
CREATE INDEX IF NOT EXISTS FOR (n:OktaGroup) ON (n.lastupdated);
CREATE INDEX IF NOT EXISTS FOR (n:OktaApplication) ON (n.id);
CREATE INDEX IF NOT EXISTS FOR (n:OktaApplication) ON (n.lastupdated);
CREATE INDEX IF NOT EXISTS FOR (n:OktaUserFactor) ON (n.id);
CREATE INDEX IF NOT EXISTS FOR (n:OktaUserFactor) ON (n.lastupdated);
CREATE INDEX IF NOT EXISTS FOR (n:OktaTrustedOrigin) ON (n.id);
CREATE INDEX IF NOT EXISTS FOR (n:OktaTrustedOrigin) ON (n.lastupdated);
CREATE INDEX IF NOT EXISTS FOR (n:OktaAdministrationRole) ON (n.id);
CREATE INDEX IF NOT EXISTS FOR (n:OktaAdministrationRole) ON (n.lastupdated);

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The datatype is OktaGroup. Double checking but is this comment correct?

cartography/intel/okta/groups.py Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's clean this up

okta_client, neo4j_session, common_job_parameters
)

# TODO: Deprecate this while we determine a method of making it more generic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker: This is still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants