From 72fd3e6bffe110d99c7ce1746dbd402ba2ec6b6a Mon Sep 17 00:00:00 2001 From: Patsy Date: Tue, 2 Jun 2026 13:31:37 -0600 Subject: [PATCH 1/7] add contact email into account model --- cdip_admin/accounts/admin.py | 7 +++++-- .../0017_accountprofile_contact_email.py | 16 ++++++++++++++++ cdip_admin/accounts/models.py | 1 + 3 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py diff --git a/cdip_admin/accounts/admin.py b/cdip_admin/accounts/admin.py index 17148e1e1..694c0dc49 100644 --- a/cdip_admin/accounts/admin.py +++ b/cdip_admin/accounts/admin.py @@ -9,7 +9,7 @@ class OrganzationMemberInline(admin.TabularInline): @admin.register(AccountProfile) class AccountProfile(admin.ModelAdmin): - list_display = ("username",) + list_display = ("username", "contact_email",) inlines = [ OrganzationMemberInline, @@ -22,9 +22,12 @@ def username(self, obj): search_fields = ( "user__username", "organizations__name", + "contact_email", ) - fieldsets = ((None, {"classes": ("wide",), "fields": (("user",))}),) + fieldsets = ( + (None, {"classes": ("wide",), "fields": ("user", "contact_email")}), + ) @admin.register(EULA) diff --git a/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py b/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py new file mode 100644 index 000000000..1fd8aee67 --- /dev/null +++ b/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py @@ -0,0 +1,16 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('accounts', '0016_alter_eula_options'), + ] + + operations = [ + migrations.AddField( + model_name='accountprofile', + name='contact_email', + field=models.EmailField(blank=True, max_length=254, null=True), + ), + ] diff --git a/cdip_admin/accounts/models.py b/cdip_admin/accounts/models.py index 2aaaf8ac7..184adca76 100644 --- a/cdip_admin/accounts/models.py +++ b/cdip_admin/accounts/models.py @@ -24,6 +24,7 @@ class AccountProfile(models.Model): organizations = models.ManyToManyField( Organization, through=AccountProfileOrganization ) + contact_email = models.EmailField(max_length=254, null=True, blank=True) def __str__(self): return self.user.username From 4ac6ae419366eb7e7e5526b0c3893b5af5145609 Mon Sep 17 00:00:00 2001 From: Patsy Date: Tue, 2 Jun 2026 13:33:55 -0600 Subject: [PATCH 2/7] move keycloak out of utils to have its own module --- cdip_admin/accounts/keycloak.py | 56 +++++++++++++++++++ cdip_admin/accounts/utils.py | 54 ++---------------- .../api/v2/tests/test_organizations_api.py | 4 +- cdip_admin/emails/utils.py | 2 +- 4 files changed, 63 insertions(+), 53 deletions(-) create mode 100644 cdip_admin/accounts/keycloak.py diff --git a/cdip_admin/accounts/keycloak.py b/cdip_admin/accounts/keycloak.py new file mode 100644 index 000000000..68c53cd96 --- /dev/null +++ b/cdip_admin/accounts/keycloak.py @@ -0,0 +1,56 @@ +import logging + +import requests +from django.http import JsonResponse + +from cdip_admin import settings +from core.utils import get_admin_access_token + + +KEYCLOAK_SERVER = settings.KEYCLOAK_SERVER +KEYCLOAK_REALM = settings.KEYCLOAK_REALM +KEYCLOAK_CLIENT = settings.KEYCLOAK_CLIENT_ID + +KEYCLOAK_ADMIN_API = f"{KEYCLOAK_SERVER}/auth/admin/realms/{KEYCLOAK_REALM}/" + +logger = logging.getLogger(__name__) + + +def add_account(user): + + url = KEYCLOAK_ADMIN_API + "users" + + token = get_admin_access_token() + + if not token: + logger.warning("Cannot get a valid access_token.") + response = JsonResponse({"message": "You don't have access to this resource"}) + response.status_code = 403 + return response + + headers = { + "authorization": f"{token['token_type']} {token['access_token']}", + "Content-type": "application/json", + } + # Prepare the payload in the format expected by keycloak + user_data = { + "email": user["email"], + "firstName": user["first_name"], + "lastName": user["last_name"], + "enabled": True # Enable user in keycloak + } + response = requests.post(url=url, headers=headers, json=user_data) + + if response.ok: + logger.info(f"User created successfully") + elif response.status_code == 409: + logger.info(f'Keycloak user {user["email"]} already exists.') + else: + logger.error(f"Error adding account: {response.status_code}], {response.text}") + return False + + return True + + +def get_password_reset_link(user): + return f"{KEYCLOAK_SERVER}/auth/realms/{KEYCLOAK_REALM}/login-actions/reset-credentials?client_id={KEYCLOAK_CLIENT}" diff --git a/cdip_admin/accounts/utils.py b/cdip_admin/accounts/utils.py index c23b2c9e0..798a646d8 100644 --- a/cdip_admin/accounts/utils.py +++ b/cdip_admin/accounts/utils.py @@ -1,61 +1,19 @@ import logging -import requests -from django.http import JsonResponse + from django.contrib.auth.models import User, Group from django.core.exceptions import SuspiciousOperation from django.db.models import Q + from core.enums import DjangoGroups -from cdip_admin import settings -from core.utils import get_admin_access_token from organizations.models import Organization -from .models import AccountProfile, AccountProfileOrganization - -KEYCLOAK_SERVER = settings.KEYCLOAK_SERVER -KEYCLOAK_REALM = settings.KEYCLOAK_REALM -KEYCLOAK_CLIENT = settings.KEYCLOAK_CLIENT_ID +from .keycloak import add_account +from .models import AccountProfile, AccountProfileOrganization -KEYCLOAK_ADMIN_API = f"{KEYCLOAK_SERVER}/auth/admin/realms/{KEYCLOAK_REALM}/" logger = logging.getLogger(__name__) -def add_account(user): - - url = KEYCLOAK_ADMIN_API + "users" - - token = get_admin_access_token() - - if not token: - logger.warning("Cannot get a valid access_token.") - response = JsonResponse({"message": "You don't have access to this resource"}) - response.status_code = 403 - return response - - headers = { - "authorization": f"{token['token_type']} {token['access_token']}", - "Content-type": "application/json", - } - # Prepare the payload in the format expected by keycloak - user_data = { - "email": user["email"], - "firstName": user["first_name"], - "lastName": user["last_name"], - "enabled": True # Enable user in keycloak - } - response = requests.post(url=url, headers=headers, json=user_data) - - if response.ok: - logger.info(f"User created successfully") - elif response.status_code == 409: - logger.info(f'Keycloak user {user["email"]} already exists.') - else: - logger.error(f"Error adding account: {response.status_code}], {response.text}") - return False - - return True - - def add_or_create_user_in_org(org_id, role, user_data): email = user_data["email"] first_name = user_data["first_name"] @@ -105,10 +63,6 @@ def remove_members_from_organization(org_id, profile_ids): return removed_qty -def get_password_reset_link(user): - return f"{KEYCLOAK_SERVER}/auth/realms/{KEYCLOAK_REALM}/login-actions/reset-credentials?client_id={KEYCLOAK_CLIENT}" - - def get_user_organizations_qs(user): # Returns a queryset with the organizations that the user is allowed to see if user.is_superuser: diff --git a/cdip_admin/api/v2/tests/test_organizations_api.py b/cdip_admin/api/v2/tests/test_organizations_api.py index 503c56ad9..92235f811 100644 --- a/cdip_admin/api/v2/tests/test_organizations_api.py +++ b/cdip_admin/api/v2/tests/test_organizations_api.py @@ -253,7 +253,7 @@ def _test_invite_user( api_client.force_authenticate(inviter) # Mock external dependencies - mocker.patch("accounts.utils.add_account", mock_add_account) # mock user creation in Keycloak + mocker.patch("accounts.keycloak.add_account", mock_add_account) # mock user creation in Keycloak mocker.patch("api.v2.views.send_invite_email_task", mock_send_invite_email_task) # mock email sending response = api_client.put( reverse("members-invite", kwargs={"organization_pk": organization.id}), @@ -292,7 +292,7 @@ def _test_cannot_invite_user( api_client.force_authenticate(inviter) # Mock external dependencies - mocker.patch("accounts.utils.add_account", mock_add_account) # mock user creation in Keycloak + mocker.patch("accounts.keycloak.add_account", mock_add_account) # mock user creation in Keycloak mocker.patch("api.v2.views.send_invite_email_task", mock_send_invite_email_task) # mock email sending response = api_client.put( reverse("members-invite", kwargs={"organization_pk": organization.id}), diff --git a/cdip_admin/emails/utils.py b/cdip_admin/emails/utils.py index f08d6fa7c..a3e644fd3 100644 --- a/cdip_admin/emails/utils.py +++ b/cdip_admin/emails/utils.py @@ -1,7 +1,7 @@ from django.template.loader import render_to_string from django.conf import settings from django.core.mail import send_mail -from accounts.utils import get_password_reset_link +from accounts.keycloak import get_password_reset_link def send_invite_email(user, organization, is_new_user): From 142362b1711d2d09622539d34ca88ae79f21b9bb Mon Sep 17 00:00:00 2001 From: Patsy Date: Tue, 2 Jun 2026 13:35:04 -0600 Subject: [PATCH 3/7] add workspaces and role into user details and allow parcial update --- cdip_admin/api/v2/serializers.py | 58 +++++++++ cdip_admin/api/v2/tests/test_users_api.py | 139 +++++++++++++++++++++- cdip_admin/api/v2/urls.py | 1 + cdip_admin/api/v2/views.py | 10 +- 4 files changed, 204 insertions(+), 4 deletions(-) diff --git a/cdip_admin/api/v2/serializers.py b/cdip_admin/api/v2/serializers.py index 8224b696f..7529d125e 100644 --- a/cdip_admin/api/v2/serializers.py +++ b/cdip_admin/api/v2/serializers.py @@ -25,9 +25,21 @@ User = get_user_model() +class UserWorkspaceSerializer(serializers.ModelSerializer): + name = serializers.CharField(source="organization.name") + workspace_id = serializers.UUIDField(source="organization.id") + role = serializers.CharField() + + class Meta: + model = AccountProfileOrganization + fields = ("id", "workspace_id", "name", "role") + + class UserDetailsRetrieveSerializer(serializers.ModelSerializer): full_name = serializers.SerializerMethodField() accepted_eula = serializers.SerializerMethodField() + contact_email = serializers.SerializerMethodField() + workspaces = serializers.SerializerMethodField() class Meta: model = User @@ -35,9 +47,13 @@ class Meta: "id", "username", "email", + "first_name", + "last_name", "full_name", "is_superuser", "accepted_eula", + "contact_email", + "workspaces", ) def get_full_name(self, obj): @@ -51,6 +67,48 @@ def get_accepted_eula(self, obj): else: return agreement.accept + def get_contact_email(self, obj): + profile = getattr(obj, "accountprofile", None) + return profile.contact_email if profile else None + + def get_workspaces(self, obj): + profile = getattr(obj, "accountprofile", None) + if not profile: + return [] + memberships = ( + profile.accountprofileorganization_set + .select_related("organization") + .all() + ) + return UserWorkspaceSerializer(memberships, many=True).data + + +class UserDetailsUpdateSerializer(serializers.Serializer): + USER_FIELDS = ("first_name", "last_name") + + first_name = serializers.CharField(required=False, max_length=150, allow_blank=True) + last_name = serializers.CharField(required=False, max_length=150, allow_blank=True) + contact_email = serializers.EmailField(required=False, allow_null=True) + + def update(self, instance, validated_data): + user_fields = { + k: v for k, v in validated_data.items() if k in self.USER_FIELDS + } + if user_fields: + for k, v in user_fields.items(): + setattr(instance, k, v) + instance.save(update_fields=list(user_fields.keys())) + + if "contact_email" in validated_data: + profile, _ = AccountProfile.objects.get_or_create(user=instance) + profile.contact_email = validated_data["contact_email"] + profile.save(update_fields=["contact_email"]) + + return instance + + def to_representation(self, instance): + return UserDetailsRetrieveSerializer(instance, context=self.context).data + class OrganizationSerializer(serializers.ModelSerializer): role = serializers.SerializerMethodField() diff --git a/cdip_admin/api/v2/tests/test_users_api.py b/cdip_admin/api/v2/tests/test_users_api.py index cd9d01efe..f7f2b5de1 100644 --- a/cdip_admin/api/v2/tests/test_users_api.py +++ b/cdip_admin/api/v2/tests/test_users_api.py @@ -2,6 +2,8 @@ from django.urls import reverse from rest_framework import status +from accounts.models import AccountProfile, AccountProfileOrganization + pytestmark = pytest.mark.django_db @@ -19,9 +21,12 @@ def test_retrieve_user_details_as_superuser(api_client, superuser, eula_v1): assert response_data.get("email") == superuser.email assert "full_name" in response_data assert response_data.get("accepted_eula") == False + # Superuser doesn't have an AccountProfile by default + assert response_data.get("contact_email") is None + assert response_data.get("workspaces") == [] -def test_retrieve_user_details_as_org_admin(api_client, org_admin_user, eula_v1): +def test_retrieve_user_details_as_org_admin(api_client, org_admin_user, organization, eula_v1): api_client.force_authenticate(org_admin_user) response = api_client.get( reverse("user-details") @@ -33,10 +38,21 @@ def test_retrieve_user_details_as_org_admin(api_client, org_admin_user, eula_v1) assert response_data.get("id") == org_admin_user.id assert response_data.get("username") == org_admin_user.username assert response_data.get("email") == org_admin_user.email + assert response_data.get("first_name") == org_admin_user.first_name + assert response_data.get("last_name") == org_admin_user.last_name assert response_data.get("accepted_eula") == False + assert response_data.get("contact_email") is None + workspaces = response_data.get("workspaces") + assert isinstance(workspaces, list) + assert len(workspaces) == 1 + ws = workspaces[0] + assert ws["workspace_id"] == str(organization.id) + assert ws["name"] == organization.name + assert ws["role"] == "admin" + assert "id" in ws -def test_retrieve_user_details_as_org_viewer(api_client, org_viewer_user, eula_v1): +def test_retrieve_user_details_as_org_viewer(api_client, org_viewer_user, organization, eula_v1): api_client.force_authenticate(org_viewer_user) response = api_client.get( reverse("user-details") @@ -49,3 +65,122 @@ def test_retrieve_user_details_as_org_viewer(api_client, org_viewer_user, eula_v assert response_data.get("username") == org_viewer_user.username assert response_data.get("email") == org_viewer_user.email assert response_data.get("accepted_eula") == False + workspaces = response_data.get("workspaces") + assert len(workspaces) == 1 + assert workspaces[0]["role"] == "viewer" + assert workspaces[0]["workspace_id"] == str(organization.id) + + +def test_retrieve_user_details_returns_contact_email_when_set(api_client, org_admin_user, eula_v1): + profile = org_admin_user.accountprofile + profile.contact_email = "caroline.contact@example.org" + profile.save(update_fields=["contact_email"]) + + api_client.force_authenticate(org_admin_user) + response = api_client.get(reverse("user-details")) + + assert response.status_code == status.HTTP_200_OK + assert response.json().get("contact_email") == "caroline.contact@example.org" + + +def test_patch_user_details_updates_user_and_profile_fields(api_client, org_admin_user, eula_v1): + api_client.force_authenticate(org_admin_user) + payload = { + "first_name": "Caro", + "last_name": "Westside", + "contact_email": "caro.westside@example.org", + } + response = api_client.patch(reverse("user-details"), data=payload, format="json") + + assert response.status_code == status.HTTP_200_OK + org_admin_user.refresh_from_db() + assert org_admin_user.first_name == "Caro" + assert org_admin_user.last_name == "Westside" + assert org_admin_user.accountprofile.contact_email == "caro.westside@example.org" + # Response uses the retrieve serializer shape + body = response.json() + assert body["first_name"] == "Caro" + assert body["last_name"] == "Westside" + assert body["contact_email"] == "caro.westside@example.org" + + +def test_patch_user_details_rejects_invalid_email(api_client, org_admin_user, eula_v1): + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("user-details"), + data={"contact_email": "not-an-email"}, + format="json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "contact_email" in response.json() + + +def test_patch_user_details_accepts_null_contact_email(api_client, org_admin_user, eula_v1): + profile = org_admin_user.accountprofile + profile.contact_email = "old@example.org" + profile.save(update_fields=["contact_email"]) + + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("user-details"), + data={"contact_email": None}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + profile.refresh_from_db() + assert profile.contact_email is None + + +def test_patch_user_details_creates_profile_for_superuser(api_client, superuser, eula_v1): + assert not AccountProfile.objects.filter(user=superuser).exists() + + api_client.force_authenticate(superuser) + response = api_client.patch( + reverse("user-details"), + data={"contact_email": "super@example.org"}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + profile = AccountProfile.objects.get(user=superuser) + assert profile.contact_email == "super@example.org" + + +def test_patch_user_details_with_partial_payload(api_client, org_admin_user, eula_v1): + original_email = org_admin_user.accountprofile.contact_email + api_client.force_authenticate(org_admin_user) + response = api_client.patch( + reverse("user-details"), + data={"first_name": "OnlyFirst"}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + org_admin_user.refresh_from_db() + assert org_admin_user.first_name == "OnlyFirst" + # contact_email untouched + assert org_admin_user.accountprofile.contact_email == original_email + + +def test_user_details_requires_authentication(api_client): + response = api_client.get(reverse("user-details")) + assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) + + response = api_client.patch(reverse("user-details"), data={"first_name": "X"}, format="json") + assert response.status_code in (status.HTTP_401_UNAUTHORIZED, status.HTTP_403_FORBIDDEN) + + +def test_retrieve_user_details_does_not_n_plus_1_on_workspaces( + api_client, org_admin_user, other_organization, eula_v1, django_assert_max_num_queries, +): + # Add a second membership; query count must not scale with workspaces count. + AccountProfileOrganization.objects.create( + accountprofile=org_admin_user.accountprofile, + organization=other_organization, + role="admin", + ) + api_client.force_authenticate(org_admin_user) + + with django_assert_max_num_queries(10): + response = api_client.get(reverse("user-details")) + + assert response.status_code == status.HTTP_200_OK + assert len(response.json()["workspaces"]) == 2 diff --git a/cdip_admin/api/v2/urls.py b/cdip_admin/api/v2/urls.py index 6c904bf92..1521f1ab0 100644 --- a/cdip_admin/api/v2/urls.py +++ b/cdip_admin/api/v2/urls.py @@ -47,6 +47,7 @@ view=views.UsersView.as_view( { 'get': 'retrieve', + 'patch': 'partial_update', } ), name="user-details" diff --git a/cdip_admin/api/v2/views.py b/cdip_admin/api/v2/views.py index 784264097..b2b866efa 100644 --- a/cdip_admin/api/v2/views.py +++ b/cdip_admin/api/v2/views.py @@ -24,12 +24,18 @@ class UsersView( mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, viewsets.GenericViewSet ): """ - An endpoint for retrieving the details of the logged-in user + An endpoint for retrieving and updating the details of the logged-in user """ - serializer_class = v2_serializers.UserDetailsRetrieveSerializer + permission_classes = [IsAuthenticated] + + def get_serializer_class(self): + if self.action in ("update", "partial_update"): + return v2_serializers.UserDetailsUpdateSerializer + return v2_serializers.UserDetailsRetrieveSerializer def get_object(self): return self.request.user From 10668a47ae9e3645b4ea62563ce0212acc75f36e Mon Sep 17 00:00:00 2001 From: Patsy Date: Tue, 2 Jun 2026 14:18:10 -0600 Subject: [PATCH 4/7] harden /v2/users/me/ and keycloak module (GUNDI-5347) - UserDetailsUpdateSerializer.update wrapped in transaction.atomic so a failed profile save reverts the user save. - add_account: collapse to a single bool return, add HTTP timeout and RequestException handling; drop dead JsonResponse branch and the unused user param on get_password_reset_link. - add_or_create_user_in_org: cache the org-members Group lookup and clean up control flow. - Admin: rename shadowing ModelAdmin to AccountProfileAdmin and add list_select_related to avoid N+1 on the list view. - AccountProfile.contact_email: add help_text documenting independence from the auth provider email. - Docstrings on UserWorkspaceSerializer, UserDetailsUpdateSerializer and UsersView; comment the query-count budget. - Tests: add a max_num_queries regression test for /v2/users/me/. Co-Authored-By: Claude Opus 4.7 (1M context) --- cdip_admin/accounts/admin.py | 3 +- cdip_admin/accounts/keycloak.py | 47 +++++++++++++++-------- cdip_admin/accounts/models.py | 5 ++- cdip_admin/accounts/utils.py | 47 ++++++++--------------- cdip_admin/api/v2/serializers.py | 38 ++++++++++++------ cdip_admin/api/v2/tests/test_users_api.py | 3 ++ cdip_admin/api/v2/views.py | 5 ++- 7 files changed, 85 insertions(+), 63 deletions(-) diff --git a/cdip_admin/accounts/admin.py b/cdip_admin/accounts/admin.py index 694c0dc49..c067b2e68 100644 --- a/cdip_admin/accounts/admin.py +++ b/cdip_admin/accounts/admin.py @@ -7,9 +7,10 @@ class OrganzationMemberInline(admin.TabularInline): @admin.register(AccountProfile) -class AccountProfile(admin.ModelAdmin): +class AccountProfileAdmin(admin.ModelAdmin): list_display = ("username", "contact_email",) + list_select_related = ("user",) inlines = [ OrganzationMemberInline, diff --git a/cdip_admin/accounts/keycloak.py b/cdip_admin/accounts/keycloak.py index 68c53cd96..09a246236 100644 --- a/cdip_admin/accounts/keycloak.py +++ b/cdip_admin/accounts/keycloak.py @@ -1,7 +1,6 @@ import logging import requests -from django.http import JsonResponse from cdip_admin import settings from core.utils import get_admin_access_token @@ -13,44 +12,58 @@ KEYCLOAK_ADMIN_API = f"{KEYCLOAK_SERVER}/auth/admin/realms/{KEYCLOAK_REALM}/" +# (connect_timeout, read_timeout) for outbound Keycloak admin calls. +# Bounds the time a portal request can spend blocked on Keycloak. +KEYCLOAK_HTTP_TIMEOUT = (5, 10) + logger = logging.getLogger(__name__) def add_account(user): + """Create a user in Keycloak. - url = KEYCLOAK_ADMIN_API + "users" + `user` is a dict with keys: ``email``, ``first_name``, ``last_name``. + Returns ``True`` if the user exists in Keycloak after the call + (newly created, already-present, or any 2xx). Returns ``False`` if + the call could not be made (no admin token) or Keycloak returned an + unexpected error. + """ token = get_admin_access_token() - if not token: - logger.warning("Cannot get a valid access_token.") - response = JsonResponse({"message": "You don't have access to this resource"}) - response.status_code = 403 - return response + logger.warning("Cannot get a valid Keycloak admin access_token.") + return False + url = KEYCLOAK_ADMIN_API + "users" headers = { "authorization": f"{token['token_type']} {token['access_token']}", "Content-type": "application/json", } - # Prepare the payload in the format expected by keycloak user_data = { "email": user["email"], "firstName": user["first_name"], "lastName": user["last_name"], - "enabled": True # Enable user in keycloak + "enabled": True, } - response = requests.post(url=url, headers=headers, json=user_data) + + try: + response = requests.post( + url=url, headers=headers, json=user_data, timeout=KEYCLOAK_HTTP_TIMEOUT, + ) + except requests.RequestException as exc: + logger.error(f"Keycloak add_account network error: {exc}") + return False if response.ok: - logger.info(f"User created successfully") - elif response.status_code == 409: + logger.info("User created successfully") + return True + if response.status_code == 409: logger.info(f'Keycloak user {user["email"]} already exists.') - else: - logger.error(f"Error adding account: {response.status_code}], {response.text}") - return False + return True - return True + logger.error(f"Error adding account: {response.status_code}, {response.text}") + return False -def get_password_reset_link(user): +def get_password_reset_link(): return f"{KEYCLOAK_SERVER}/auth/realms/{KEYCLOAK_REALM}/login-actions/reset-credentials?client_id={KEYCLOAK_CLIENT}" diff --git a/cdip_admin/accounts/models.py b/cdip_admin/accounts/models.py index 184adca76..e07258236 100644 --- a/cdip_admin/accounts/models.py +++ b/cdip_admin/accounts/models.py @@ -24,7 +24,10 @@ class AccountProfile(models.Model): organizations = models.ManyToManyField( Organization, through=AccountProfileOrganization ) - contact_email = models.EmailField(max_length=254, null=True, blank=True) + contact_email = models.EmailField( + max_length=254, null=True, blank=True, + help_text=_("User-controlled contact email, independent of the auth provider email."), + ) def __str__(self): return self.user.username diff --git a/cdip_admin/accounts/utils.py b/cdip_admin/accounts/utils.py index 798a646d8..938bd0001 100644 --- a/cdip_admin/accounts/utils.py +++ b/cdip_admin/accounts/utils.py @@ -18,42 +18,29 @@ def add_or_create_user_in_org(org_id, role, user_data): email = user_data["email"] first_name = user_data["first_name"] last_name = user_data["last_name"] - username = email user_created = False + org_members_group_id = Group.objects.get(name=DjangoGroups.ORGANIZATION_MEMBER).id + try: user = User.objects.get(Q(username=email) | Q(email=email)) - if not user.groups.filter( - name=DjangoGroups.ORGANIZATION_MEMBER.value - ).exists(): - group_id = Group.objects.get( - name=DjangoGroups.ORGANIZATION_MEMBER - ).id - user.groups.add(group_id) + if not user.groups.filter(name=DjangoGroups.ORGANIZATION_MEMBER.value).exists(): + user.groups.add(org_members_group_id) except User.DoesNotExist: - # create keycloak user - response = add_account(user_data) - # create django user - if response: - user = User.objects.create( - email=email, - username=username, - first_name=first_name, - last_name=last_name, - ) - group_id = Group.objects.get( - name=DjangoGroups.ORGANIZATION_MEMBER - ).id - user.groups.add(group_id) - user_created = True - else: + if not add_account(user_data): raise SuspiciousOperation - - account_profile, created = AccountProfile.objects.get_or_create( - user_id=user.id, - ) - apo, created = AccountProfileOrganization.objects.get_or_create( - accountprofile_id=account_profile.id, organization_id=org_id, role=role + user = User.objects.create( + email=email, + username=email, + first_name=first_name, + last_name=last_name, + ) + user.groups.add(org_members_group_id) + user_created = True + + account_profile, _ = AccountProfile.objects.get_or_create(user_id=user.id) + AccountProfileOrganization.objects.get_or_create( + accountprofile_id=account_profile.id, organization_id=org_id, role=role, ) return user, user_created diff --git a/cdip_admin/api/v2/serializers.py b/cdip_admin/api/v2/serializers.py index 7529d125e..9c93ff4ef 100644 --- a/cdip_admin/api/v2/serializers.py +++ b/cdip_admin/api/v2/serializers.py @@ -26,6 +26,14 @@ class UserWorkspaceSerializer(serializers.ModelSerializer): + """One workspace a user belongs to. + + ``id`` is the membership id (``AccountProfileOrganization.id``) and is + what future membership-scoped actions (e.g. leave-workspace) target. + ``workspace_id`` is the organization id and is what the frontend uses + to link to the workspace itself. + """ + name = serializers.CharField(source="organization.name") workspace_id = serializers.UUIDField(source="organization.id") role = serializers.CharField() @@ -84,6 +92,11 @@ def get_workspaces(self, obj): class UserDetailsUpdateSerializer(serializers.Serializer): + """PATCH /v2/users/me/ — splits writes between User (first/last name) + and AccountProfile (contact_email), atomically. Response uses the + retrieve serializer shape so the frontend can refresh from one call. + """ + USER_FIELDS = ("first_name", "last_name") first_name = serializers.CharField(required=False, max_length=150, allow_blank=True) @@ -91,18 +104,19 @@ class UserDetailsUpdateSerializer(serializers.Serializer): contact_email = serializers.EmailField(required=False, allow_null=True) def update(self, instance, validated_data): - user_fields = { - k: v for k, v in validated_data.items() if k in self.USER_FIELDS - } - if user_fields: - for k, v in user_fields.items(): - setattr(instance, k, v) - instance.save(update_fields=list(user_fields.keys())) - - if "contact_email" in validated_data: - profile, _ = AccountProfile.objects.get_or_create(user=instance) - profile.contact_email = validated_data["contact_email"] - profile.save(update_fields=["contact_email"]) + with transaction.atomic(): + user_fields = { + k: v for k, v in validated_data.items() if k in self.USER_FIELDS + } + if user_fields: + for k, v in user_fields.items(): + setattr(instance, k, v) + instance.save(update_fields=list(user_fields.keys())) + + if "contact_email" in validated_data: + profile, _ = AccountProfile.objects.get_or_create(user=instance) + profile.contact_email = validated_data["contact_email"] + profile.save(update_fields=["contact_email"]) return instance diff --git a/cdip_admin/api/v2/tests/test_users_api.py b/cdip_admin/api/v2/tests/test_users_api.py index f7f2b5de1..97b8fe53e 100644 --- a/cdip_admin/api/v2/tests/test_users_api.py +++ b/cdip_admin/api/v2/tests/test_users_api.py @@ -179,6 +179,9 @@ def test_retrieve_user_details_does_not_n_plus_1_on_workspaces( ) api_client.force_authenticate(org_admin_user) + # Budget: ~5 baseline queries (active EULA, UserAgreement, accountprofile, + # memberships+orgs via select_related, plus auth/session). Headroom for + # shared fixtures. Must not scale with the number of workspaces. with django_assert_max_num_queries(10): response = api_client.get(reverse("user-details")) diff --git a/cdip_admin/api/v2/views.py b/cdip_admin/api/v2/views.py index b2b866efa..43ebd12d9 100644 --- a/cdip_admin/api/v2/views.py +++ b/cdip_admin/api/v2/views.py @@ -27,8 +27,9 @@ class UsersView( mixins.UpdateModelMixin, viewsets.GenericViewSet ): - """ - An endpoint for retrieving and updating the details of the logged-in user + """Self-service endpoint: always operates on the authenticated user + (``get_object`` returns ``request.user``). Not for admin lookups of + arbitrary users. """ permission_classes = [IsAuthenticated] From 9359a511ca20b254570a7ce6a1c62a4d5c6e22b7 Mon Sep 17 00:00:00 2001 From: Patsy Date: Tue, 2 Jun 2026 15:32:20 -0600 Subject: [PATCH 5/7] address Copilot review on PR #427 - emails/utils: drop unused get_password_reset_link import. - migration 0017: include help_text on contact_email AddField so it matches the model and makemigrations --check stays clean. - keycloak.add_account: replace response.ok with explicit 2xx range check (response.ok is True for 3xx too, contradicting the docstring). - accounts.utils: use DjangoGroups.ORGANIZATION_MEMBER.value for the group lookup, matching conftest.py and core/permissions.py. Co-Authored-By: Claude Opus 4.7 (1M context) --- cdip_admin/accounts/keycloak.py | 2 +- .../migrations/0017_accountprofile_contact_email.py | 7 ++++++- cdip_admin/accounts/utils.py | 2 +- cdip_admin/emails/utils.py | 1 - 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cdip_admin/accounts/keycloak.py b/cdip_admin/accounts/keycloak.py index 09a246236..6172c9bc3 100644 --- a/cdip_admin/accounts/keycloak.py +++ b/cdip_admin/accounts/keycloak.py @@ -54,7 +54,7 @@ def add_account(user): logger.error(f"Keycloak add_account network error: {exc}") return False - if response.ok: + if 200 <= response.status_code < 300: logger.info("User created successfully") return True if response.status_code == 409: diff --git a/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py b/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py index 1fd8aee67..a22093632 100644 --- a/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py +++ b/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py @@ -11,6 +11,11 @@ class Migration(migrations.Migration): migrations.AddField( model_name='accountprofile', name='contact_email', - field=models.EmailField(blank=True, max_length=254, null=True), + field=models.EmailField( + blank=True, + help_text='User-controlled contact email, independent of the auth provider email.', + max_length=254, + null=True, + ), ), ] diff --git a/cdip_admin/accounts/utils.py b/cdip_admin/accounts/utils.py index 938bd0001..53b7a017c 100644 --- a/cdip_admin/accounts/utils.py +++ b/cdip_admin/accounts/utils.py @@ -19,7 +19,7 @@ def add_or_create_user_in_org(org_id, role, user_data): first_name = user_data["first_name"] last_name = user_data["last_name"] user_created = False - org_members_group_id = Group.objects.get(name=DjangoGroups.ORGANIZATION_MEMBER).id + org_members_group_id = Group.objects.get(name=DjangoGroups.ORGANIZATION_MEMBER.value).id try: user = User.objects.get(Q(username=email) | Q(email=email)) diff --git a/cdip_admin/emails/utils.py b/cdip_admin/emails/utils.py index a3e644fd3..130c56235 100644 --- a/cdip_admin/emails/utils.py +++ b/cdip_admin/emails/utils.py @@ -1,7 +1,6 @@ from django.template.loader import render_to_string from django.conf import settings from django.core.mail import send_mail -from accounts.keycloak import get_password_reset_link def send_invite_email(user, organization, is_new_user): From 1cfcc0d6e04a58cf1a0b80bb2b77ad12527c7866 Mon Sep 17 00:00:00 2001 From: Patsy Date: Thu, 4 Jun 2026 16:42:39 -0600 Subject: [PATCH 6/7] address second Copilot review on PR #427 - accounts/utils: import the keycloak module instead of binding the function at import time. With `from .keycloak import add_account`, patching `accounts.keycloak.add_account` in tests no longer intercepted the call inside `add_or_create_user_in_org`, so the invite tests would issue real outbound HTTP to Keycloak. Switching to `from . import keycloak` and `keycloak.add_account(...)` makes the patch resolve at call-time, restoring test isolation. - keycloak.add_account: use `logger.exception` (preserves traceback) in the RequestException handler, matching the convention used in event_consumers and integrations/tasks. Co-Authored-By: Claude Opus 4.7 (1M context) --- cdip_admin/accounts/keycloak.py | 4 ++-- cdip_admin/accounts/utils.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cdip_admin/accounts/keycloak.py b/cdip_admin/accounts/keycloak.py index 6172c9bc3..55e6c7948 100644 --- a/cdip_admin/accounts/keycloak.py +++ b/cdip_admin/accounts/keycloak.py @@ -50,8 +50,8 @@ def add_account(user): response = requests.post( url=url, headers=headers, json=user_data, timeout=KEYCLOAK_HTTP_TIMEOUT, ) - except requests.RequestException as exc: - logger.error(f"Keycloak add_account network error: {exc}") + except requests.RequestException: + logger.exception("Keycloak add_account network error") return False if 200 <= response.status_code < 300: diff --git a/cdip_admin/accounts/utils.py b/cdip_admin/accounts/utils.py index 53b7a017c..66877491e 100644 --- a/cdip_admin/accounts/utils.py +++ b/cdip_admin/accounts/utils.py @@ -7,7 +7,7 @@ from core.enums import DjangoGroups from organizations.models import Organization -from .keycloak import add_account +from . import keycloak from .models import AccountProfile, AccountProfileOrganization @@ -27,7 +27,7 @@ def add_or_create_user_in_org(org_id, role, user_data): user.groups.add(org_members_group_id) except User.DoesNotExist: - if not add_account(user_data): + if not keycloak.add_account(user_data): raise SuspiciousOperation user = User.objects.create( email=email, From ddce5f968125ed1e22f71bd5cf59c5af29d64342 Mon Sep 17 00:00:00 2001 From: Patsy Date: Mon, 8 Jun 2026 11:27:38 -0600 Subject: [PATCH 7/7] drop dead get_password_reset_link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After removing the unused import in emails/utils.py (commit 9359a51), get_password_reset_link had no callers — and no templates reference a `password_reset_link` context variable either. The invite email tells the user to click "Forgot Password?" in the portal UI; there is no email-embedded reset link. Carrying a Keycloak-specific URL builder into the Auth0 migration is exactly the kind of dead weight we want to leave behind. If a reset link is ever needed again, it should be added via the AuthProvider abstraction at that time. Also drops the now-unused KEYCLOAK_CLIENT constant. Addresses chrisdoehring's review comment on PR #427. Co-Authored-By: Claude Opus 4.7 (1M context) --- cdip_admin/accounts/keycloak.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cdip_admin/accounts/keycloak.py b/cdip_admin/accounts/keycloak.py index 55e6c7948..8ff8094a3 100644 --- a/cdip_admin/accounts/keycloak.py +++ b/cdip_admin/accounts/keycloak.py @@ -8,7 +8,6 @@ KEYCLOAK_SERVER = settings.KEYCLOAK_SERVER KEYCLOAK_REALM = settings.KEYCLOAK_REALM -KEYCLOAK_CLIENT = settings.KEYCLOAK_CLIENT_ID KEYCLOAK_ADMIN_API = f"{KEYCLOAK_SERVER}/auth/admin/realms/{KEYCLOAK_REALM}/" @@ -63,7 +62,3 @@ def add_account(user): logger.error(f"Error adding account: {response.status_code}, {response.text}") return False - - -def get_password_reset_link(): - return f"{KEYCLOAK_SERVER}/auth/realms/{KEYCLOAK_REALM}/login-actions/reset-credentials?client_id={KEYCLOAK_CLIENT}"