Skip to content

Commit

Permalink
fix: do keycloak changes inside a transaction
Browse files Browse the repository at this point in the history
In case Keycloak returns an error we need to be
able to revert.

Refs: HP-1787
  • Loading branch information
charn committed Feb 14, 2024
1 parent 3c1ff87 commit 860a140
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 23 deletions.
10 changes: 5 additions & 5 deletions profiles/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -1260,7 +1260,7 @@ def mutate_and_get_payload(cls, root, info, **input):
if sensitive_data:
update_sensitivedata(profile, sensitive_data)

profile_updated.send(sender=profile.__class__, instance=profile)
profile_updated.send(sender=profile.__class__, instance=profile)

return UpdateMyProfileMutation(profile=profile)

Expand Down Expand Up @@ -1336,7 +1336,7 @@ def mutate_and_get_payload(cls, root, info, **input):
if sensitive_data:
update_sensitivedata(profile, sensitive_data)

profile_updated.send(sender=profile.__class__, instance=profile)
profile_updated.send(sender=profile.__class__, instance=profile)

return UpdateProfileMutation(profile=profile)

Expand Down Expand Up @@ -1369,9 +1369,9 @@ def mutate_and_get_payload(cls, root, info, **input):
profile_to_claim.save()
profile_to_claim.claim_tokens.all().delete()

profile_updated.send(
sender=profile_to_claim.__class__, instance=profile_to_claim
)
profile_updated.send(
sender=profile_to_claim.__class__, instance=profile_to_claim
)

return ClaimProfileMutation(profile=profile_to_claim)

Expand Down
12 changes: 8 additions & 4 deletions profiles/tests/test_gql_update_my_profile_mutation.py
Original file line number Diff line number Diff line change
Expand Up @@ -589,19 +589,22 @@ def test_remove_all_emails_if_they_are_not_primary(user_gql_client):
assert executed["data"] == expected_data


def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_produced(
def test_when_keycloak_returns_conflict_on_update_changes_are_reverted(
user_gql_client, keycloak_setup, mocker
):
"""Correct error code is produced and local changes are reverted."""
profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user)
email = profile.emails.first()
primary_email = profile.get_primary_email()
original_email = primary_email.email
new_email = f"new-{primary_email.email}"

mocker.patch.object(
keycloak.KeycloakAdminClient,
"get_user",
return_value={
"firstName": profile.first_name,
"lastName": profile.last_name,
"email": "[email protected]",
"email": primary_email.email,
},
)

Expand All @@ -612,13 +615,14 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
)

email_updates = [
{"id": to_global_id("EmailNode", email.id), "email": "[email protected]"}
{"id": to_global_id("EmailNode", primary_email.id), "email": new_email}
]
executed = user_gql_client.execute(
EMAILS_MUTATION, variables={"profileInput": {"updateEmails": email_updates}}
)

assert_match_error_code(executed, "DATA_CONFLICT_ERROR")
assert profile.get_primary_email().email == original_email


PHONES_MUTATION = """
Expand Down
14 changes: 9 additions & 5 deletions profiles/tests/test_gql_update_profile_mutation.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,11 +272,14 @@ def test_changing_an_email_address_marks_it_unverified(user_gql_client, service)
assert executed["data"] == expected_data


def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_produced(
def test_when_keycloak_returns_conflict_on_update_changes_are_reverted(
user_gql_client, service, keycloak_setup, mocker
):
"""Correct error code is produced and local changes are reverted."""
profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user)
email = profile.emails.first()
primary_email = profile.get_primary_email()
original_email = primary_email.email
new_email = f"new-{primary_email.email}"

setup_profile_and_staff_user_to_service(profile, user_gql_client.user, service)

Expand All @@ -286,7 +289,7 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
return_value={
"firstName": profile.first_name,
"lastName": profile.last_name,
"email": "[email protected]",
"email": primary_email.email,
},
)

Expand All @@ -301,8 +304,8 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
"id": to_global_id("ProfileNode", profile.id),
"updateEmails": [
{
"id": to_global_id("EmailNode", email.id),
"email": "[email protected]",
"id": to_global_id("EmailNode", primary_email.id),
"email": new_email,
}
],
}
Expand All @@ -312,6 +315,7 @@ def test_when_keycloak_returns_conflict_on_update_then_correct_error_code_is_pro
)

assert_match_error_code(executed, "DATA_CONFLICT_ERROR")
assert profile.get_primary_email().email == original_email


class TestProfileInputValidation(ExistingProfileInputValidationBase):
Expand Down
12 changes: 3 additions & 9 deletions profiles/tests/test_profile_changes_to_keycloak.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ def test_do_nothing_if_user_is_not_found_in_keycloak(mocker):
mocked_update_user = mocker.patch.object(
keycloak.KeycloakAdminClient, "update_user"
)

profile = ProfileFactory()

profile_updated.send(sender=profile.__class__, instance=profile)
Expand All @@ -48,7 +47,6 @@ def test_changed_names_are_sent_to_keycloak(mocker):
"lastName": "New last name",
"email": None,
}

mocker.patch.object(
keycloak.KeycloakAdminClient,
"get_user",
Expand All @@ -57,11 +55,11 @@ def test_changed_names_are_sent_to_keycloak(mocker):
mocked_update_user = mocker.patch.object(
keycloak.KeycloakAdminClient, "update_user"
)

profile = ProfileFactory(
first_name=new_values["firstName"], last_name=new_values["lastName"]
)
user_id = profile.user.uuid

profile_updated.send(sender=profile.__class__, instance=profile)

mocked_update_user.assert_called_once_with(user_id, new_values)
Expand All @@ -77,7 +75,6 @@ def test_changing_email_causes_it_to_be_marked_unverified(
"email": "[email protected]",
"emailVerified": False,
}

mocker.patch.object(
keycloak.KeycloakAdminClient,
"get_user",
Expand All @@ -99,12 +96,12 @@ def test_changing_email_causes_it_to_be_marked_unverified(
else Exception("send_verify_email failed")
),
)

profile = ProfileFactory(
first_name=new_values["firstName"], last_name=new_values["lastName"]
)
profile.emails.create(email=new_values["email"], primary=True)
user_id = profile.user.uuid

profile_updated.send(sender=profile.__class__, instance=profile)

mocked_update_user.assert_called_once_with(user_id, new_values)
Expand All @@ -113,23 +110,21 @@ def test_changing_email_causes_it_to_be_marked_unverified(

def test_if_there_are_no_changes_then_nothing_is_sent_to_keycloak(mocker):
values = {"firstName": "First name", "lastName": "Last name"}

mocker.patch.object(keycloak.KeycloakAdminClient, "get_user", return_value=values)
mocked_update_user = mocker.patch.object(
keycloak.KeycloakAdminClient, "update_user"
)

profile = ProfileFactory(
first_name=values["firstName"], last_name=values["lastName"]
)

profile_updated.send(sender=profile.__class__, instance=profile)

mocked_update_user.assert_not_called()


def test_when_update_causes_a_conflict_then_data_conflict_error_is_raised(mocker):
profile = ProfileWithPrimaryEmailFactory()

mocker.patch.object(
keycloak.KeycloakAdminClient,
"get_user",
Expand All @@ -139,7 +134,6 @@ def test_when_update_causes_a_conflict_then_data_conflict_error_is_raised(mocker
"email": "[email protected]",
},
)

mocker.patch.object(
keycloak.KeycloakAdminClient,
"update_user",
Expand Down

0 comments on commit 860a140

Please sign in to comment.