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

Fix/get user timeout #665

Merged
merged 4 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions app/integrations/google_workspace/google_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ def list_groups_with_members(
members_details: bool = True,
groups_filters: list = [],
query: str | None = None,
tolerate_errors: bool = False,
):
"""List all groups in the Google Workspace domain with their members.

Expand All @@ -183,6 +184,7 @@ def list_groups_with_members(
members_details (bool): Include the members details in the response.
groups_filters (list): List of filters to apply to the groups.
query (str): The query to search for groups.
tolerate_errors (bool): Whether to include groups that encountered errors during member detail retrieval.

Returns:
list: A list of group objects with members. Any group without members will not be included.
Expand All @@ -203,16 +205,32 @@ def list_groups_with_members(
groups_with_members = []
for group in groups:
logger.info(f"Getting members for group: {group['email']}")
members = list_group_members(
group["email"], fields="members(email, role, type, status)"
)
try:
members = list_group_members(
group["email"], fields="members(email, role, type, status)"
)
except Exception as e:
logger.warning(f"Error getting members for group {group['email']}: {e}")
continue

if members and members_details:
detailed_members = []
error_occurred = False
for member in members:
logger.info(f"Getting user details for member: {member['email']}")
detailed_members.append(
get_user(member["email"], fields="name, primaryEmail")
)
try:
logger.info(f"Getting user details for member: {member['email']}")
detailed_members.append(
get_user(member["email"], fields="name, primaryEmail")
)
except Exception as e:
logger.warning(
f"Error getting user details for member {member['email']}: {e}"
)
error_occurred = True
if not tolerate_errors:
break
if error_occurred and not tolerate_errors:
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be continue when tolerate_errors instead of not tolerate_errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think you're correct... I'll double check my unit test!

continue
group["members"] = detailed_members
groups_with_members.append(group)
return groups_with_members
62 changes: 32 additions & 30 deletions app/modules/aws/identity_center.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,30 @@
DRY_RUN = True


def synchronize(**kwargs):
def synchronize(
enable_users_sync: bool = True,
enable_user_create: bool = True,
enable_user_delete: bool = False,
enable_groups_sync: bool = True,
enable_membership_create: bool = True,
enable_membership_delete: bool = False,
query: str = "email:aws-*",
pre_processing_filters: list = [],
):
"""Sync the AWS Identity Center with the Google Workspace.

Args:
enable_users_sync (bool): Toggle to sync users.
enable_groups_sync (bool): Toggle to sync groups.
query (str): The query to filter the Google Groups.

enable_users_sync (bool): Enable the synchronization of users. Default is True.
enable_user_create (bool): Enable the creation of users. Default is True.
enable_user_delete (bool): Enable the deletion of users. Default is False.
enable_groups_sync (bool): Enable the synchronization of groups. Default is True.
enable_membership_create (bool): Enable the creation of group memberships. Default is True.
enable_membership_delete (bool): Enable the deletion of group memberships. Default is False.
query (str): The query to search for groups.
pre_processing_filters (list): List of filters to apply to the groups before processing the members.
Returns:
tuple: A tuple containing the users sync status and groups sync status.
"""
enable_users_sync = kwargs.pop("enable_users_sync", True)
enable_user_create = kwargs.pop("enable_user_create", True)
enable_user_delete = kwargs.pop("enable_user_delete", False)
enable_groups_sync = kwargs.pop("enable_groups_sync", True)
enable_membership_create = kwargs.pop("enable_membership_create", True)
enable_membership_delete = kwargs.pop("enable_membership_delete", False)
query = kwargs.pop("query", "email:aws-*")
pre_processing_filters = kwargs.pop("pre_processing_filters", [])

users_sync_status = None
groups_sync_status = None
Expand Down Expand Up @@ -56,7 +61,7 @@ def synchronize(**kwargs):

if enable_users_sync:
users_sync_status = sync_users(
source_users, target_users, enable_user_create, enable_user_delete, **kwargs
source_users, target_users, enable_user_create, enable_user_delete
)
target_users = identity_store.list_users()

Expand All @@ -67,7 +72,6 @@ def synchronize(**kwargs):
target_users,
enable_membership_create,
enable_membership_delete,
**kwargs,
)
logger.info("synchronize:Sync Completed")

Expand All @@ -78,26 +82,25 @@ def synchronize(**kwargs):


def sync_users(
source_users,
target_users,
enable_user_create=True,
enable_user_delete=False,
**kwargs,
source_users: list,
target_users: list,
enable_user_create: bool = True,
enable_user_delete: bool = False,
delete_target_all: bool = False,
):
"""Sync the users in the identity store.

Args:

source_users (list): A list of users from the source system.
target_users (list): A list of users in the identity store.
enable_user_create (bool): Enable creation of users.
enable_user_delete (bool): Enable deletion of users.
delete_target_all (bool): Mark all target users for deletion.
enable_user_create (bool): Enable creation of users. Default is True.
enable_user_delete (bool): Enable deletion of users. Default is False.
delete_target_all (bool): Mark all target users for deletion. Default is False.

Returns:
tuple: A tuple containing the users created and deleted.
"""
delete_target_all = kwargs.get("delete_target_all", False)

if delete_target_all:
users_to_delete = target_users
Expand Down Expand Up @@ -150,12 +153,11 @@ def sync_users(


def sync_groups(
source_groups,
target_groups,
target_users,
enable_membership_create=True,
enable_membership_delete=False,
**kwargs,
source_groups: list,
target_groups: list,
target_users: list,
enable_membership_create: bool = True,
enable_membership_delete: bool = False,
):
"""Sync the groups in the identity store.

Expand Down
1 change: 1 addition & 0 deletions app/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ google-api-core==2.19.2
google-auth==2.33.0
httpx==0.27.2
itsdangerous==2.2.0
pandas==2.2.3
PyJWT==2.9.0
PyYAML!=6.0.0,!=5.4.0,!=5.4.1
python-dotenv==0.21.1
Expand Down
84 changes: 84 additions & 0 deletions app/tests/integrations/google_workspace/test_google_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,33 @@ def test_list_groups_with_members_filtered(
assert mock_get_user.call_count == 2


@patch("integrations.google_workspace.google_directory.list_groups")
@patch("integrations.google_workspace.google_directory.list_group_members")
@patch("integrations.google_workspace.google_directory.get_user")
def test_list_groups_with_members_error_in_list_group_members(
mock_get_user,
mock_list_group_members,
mock_list_groups,
google_groups,
google_group_members,
google_users,
google_groups_w_users,
):
groups = google_groups(2)
group_members = [Exception("Error fetching group members"), google_group_members(2)]
users = google_users(2)

mock_list_groups.return_value = groups
mock_list_group_members.side_effect = group_members
mock_get_user.side_effect = users

# Only the second group should be processed
expected_groups_with_users = [groups[1]]
expected_groups_with_users[0]["members"] = users

assert google_directory.list_groups_with_members() == expected_groups_with_users


@patch("integrations.google_workspace.google_directory.list_groups")
@patch("integrations.google_workspace.google_directory.list_group_members")
@patch("integrations.google_workspace.google_directory.get_user")
Expand Down Expand Up @@ -437,6 +464,63 @@ def test_list_groups_with_members_without_members_enabled(
mock_get_user.assert_not_called()


@patch("integrations.google_workspace.google_directory.list_groups")
@patch("integrations.google_workspace.google_directory.list_group_members")
@patch("integrations.google_workspace.google_directory.get_user")
def test_list_groups_with_members_error_in_get_user(
mock_get_user,
mock_list_group_members,
mock_list_groups,
google_groups,
google_group_members,
google_users,
):
groups = google_groups(2)
group_members = [google_group_members(2), google_group_members(2)]
users = [Exception("Error fetching user details"), google_users(1)[0]]

mock_list_groups.return_value = groups
mock_list_group_members.side_effect = group_members
mock_get_user.side_effect = users

# No groups should be processed due to error in get_user
assert google_directory.list_groups_with_members() == []


@patch("integrations.google_workspace.google_directory.list_groups")
@patch("integrations.google_workspace.google_directory.list_group_members")
@patch("integrations.google_workspace.google_directory.get_user")
def test_list_groups_with_members_tolerate_errors(
mock_get_user,
mock_list_group_members,
mock_list_groups,
google_groups,
google_group_members,
google_users,
):
groups = google_groups(2)
group_members = [google_group_members(2), google_group_members(2)]
users = [
Exception("Error fetching user details"),
google_users(1)[0],
Exception("Error fetching user details"),
google_users(1)[0],
]

mock_list_groups.return_value = groups
mock_list_group_members.side_effect = group_members
mock_get_user.side_effect = users

# Expected result should include both groups, with the second group having one member
expected_groups_with_users = [groups[0], groups[1]]
expected_groups_with_users[0]["members"] = []
expected_groups_with_users[1]["members"] = [google_users(1)[0]]

result = google_directory.list_groups_with_members(tolerate_errors=True)

assert result == expected_groups_with_users


@patch("integrations.google_workspace.google_directory.list_groups")
def test_list_groups_with_members_skips_when_no_groups(mock_list_groups):
mock_list_groups.return_value = []
Expand Down
Loading