diff --git a/cdip_admin/accounts/admin.py b/cdip_admin/accounts/admin.py index 17148e1e1..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",) + list_display = ("username", "contact_email",) + list_select_related = ("user",) inlines = [ OrganzationMemberInline, @@ -22,9 +23,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/keycloak.py b/cdip_admin/accounts/keycloak.py new file mode 100644 index 000000000..8ff8094a3 --- /dev/null +++ b/cdip_admin/accounts/keycloak.py @@ -0,0 +1,64 @@ +import logging + +import requests + +from cdip_admin import settings +from core.utils import get_admin_access_token + + +KEYCLOAK_SERVER = settings.KEYCLOAK_SERVER +KEYCLOAK_REALM = settings.KEYCLOAK_REALM + +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. + + `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 Keycloak admin access_token.") + return False + + url = KEYCLOAK_ADMIN_API + "users" + headers = { + "authorization": f"{token['token_type']} {token['access_token']}", + "Content-type": "application/json", + } + user_data = { + "email": user["email"], + "firstName": user["first_name"], + "lastName": user["last_name"], + "enabled": True, + } + + try: + response = requests.post( + url=url, headers=headers, json=user_data, timeout=KEYCLOAK_HTTP_TIMEOUT, + ) + except requests.RequestException: + logger.exception("Keycloak add_account network error") + return False + + if 200 <= response.status_code < 300: + logger.info("User created successfully") + return True + if response.status_code == 409: + logger.info(f'Keycloak user {user["email"]} already exists.') + return True + + logger.error(f"Error adding account: {response.status_code}, {response.text}") + return False 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..a22093632 --- /dev/null +++ b/cdip_admin/accounts/migrations/0017_accountprofile_contact_email.py @@ -0,0 +1,21 @@ +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, + help_text='User-controlled contact email, independent of the auth provider email.', + max_length=254, + null=True, + ), + ), + ] diff --git a/cdip_admin/accounts/models.py b/cdip_admin/accounts/models.py index 2aaaf8ac7..e07258236 100644 --- a/cdip_admin/accounts/models.py +++ b/cdip_admin/accounts/models.py @@ -24,6 +24,10 @@ class AccountProfile(models.Model): organizations = models.ManyToManyField( Organization, through=AccountProfileOrganization ) + 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 c23b2c9e0..66877491e 100644 --- a/cdip_admin/accounts/utils.py +++ b/cdip_admin/accounts/utils.py @@ -1,101 +1,46 @@ 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 +from . import keycloak +from .models import AccountProfile, AccountProfileOrganization -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 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.value).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 keycloak.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 @@ -105,10 +50,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/serializers.py b/cdip_admin/api/v2/serializers.py index 8224b696f..9c93ff4ef 100644 --- a/cdip_admin/api/v2/serializers.py +++ b/cdip_admin/api/v2/serializers.py @@ -25,9 +25,29 @@ User = get_user_model() +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() + + 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 +55,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 +75,54 @@ 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): + """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) + 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): + 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 + + 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_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/api/v2/tests/test_users_api.py b/cdip_admin/api/v2/tests/test_users_api.py index cd9d01efe..97b8fe53e 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,125 @@ 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) + + # 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")) + + 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..43ebd12d9 100644 --- a/cdip_admin/api/v2/views.py +++ b/cdip_admin/api/v2/views.py @@ -24,12 +24,19 @@ class UsersView( mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, viewsets.GenericViewSet ): + """Self-service endpoint: always operates on the authenticated user + (``get_object`` returns ``request.user``). Not for admin lookups of + arbitrary users. """ - An endpoint for retrieving 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 diff --git a/cdip_admin/emails/utils.py b/cdip_admin/emails/utils.py index f08d6fa7c..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.utils import get_password_reset_link def send_invite_email(user, organization, is_new_user):