Skip to content
Open
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
146 changes: 138 additions & 8 deletions openedx/core/djangoapps/user_api/tests/test_views.py
Original file line number Diff line number Diff line change
@@ -1,34 +1,37 @@
"""Tests for the user API at the HTTP request level. """

import json
from unittest.mock import patch

import ddt
import pytest
from django.contrib.auth import get_user_model
from django.test.utils import override_settings
from django.urls import reverse
from opaque_keys.edx.keys import CourseKey
from pytz import common_timezones, common_timezones_set, country_timezones
from rest_framework import status
from rest_framework.exceptions import ValidationError

from common.djangoapps.student.tests.factories import UserFactory
from openedx.core.djangoapps.django_comment_common import models
from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms
from openedx.core.lib.api.test_utils import TEST_API_KEY, ApiTestCase
from openedx.core.lib.time_zone_utils import get_display_time_zone
from xmodule.modulestore.tests.django_utils import (
SharedModuleStoreTestCase, # pylint: disable=wrong-import-order
)
from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # pylint: disable=wrong-import-order
from xmodule.modulestore.tests.factories import CourseFactory # pylint: disable=wrong-import-order

from ..accounts.tests.retirement_helpers import ( # pylint: disable=unused-import
RetirementTestCase, # noqa: F401
fake_requested_retirement, # noqa: F401
setup_retirement_states, # noqa: F401
)
from ..accounts.tests.retirement_helpers import RetirementTestCase # pylint: disable=unused-import; noqa: F401

Check failure on line 24 in openedx/core/djangoapps/user_api/tests/test_views.py

View workflow job for this annotation

GitHub Actions / Quality Others (ubuntu-24.04, 3.12, 20)

ruff (F401)

openedx/core/djangoapps/user_api/tests/test_views.py:24:49: F401 `..accounts.tests.retirement_helpers.RetirementTestCase` imported but unused help: Remove unused import: `..accounts.tests.retirement_helpers.RetirementTestCase`
from ..accounts.tests.retirement_helpers import fake_requested_retirement # pylint: disable=unused-import; noqa: F401

Check failure on line 25 in openedx/core/djangoapps/user_api/tests/test_views.py

View workflow job for this annotation

GitHub Actions / Quality Others (ubuntu-24.04, 3.12, 20)

ruff (F401)

openedx/core/djangoapps/user_api/tests/test_views.py:25:49: F401 `..accounts.tests.retirement_helpers.fake_requested_retirement` imported but unused help: Remove unused import: `..accounts.tests.retirement_helpers.fake_requested_retirement`
from ..accounts.tests.retirement_helpers import setup_retirement_states # pylint: disable=unused-import; noqa: F401

Check failure on line 26 in openedx/core/djangoapps/user_api/tests/test_views.py

View workflow job for this annotation

GitHub Actions / Quality Others (ubuntu-24.04, 3.12, 20)

ruff (F401)

openedx/core/djangoapps/user_api/tests/test_views.py:26:49: F401 `..accounts.tests.retirement_helpers.setup_retirement_states` imported but unused help: Remove unused import: `..accounts.tests.retirement_helpers.setup_retirement_states`
from ..models import UserOrgTag
from ..tests.factories import UserPreferenceFactory

Check failure on line 28 in openedx/core/djangoapps/user_api/tests/test_views.py

View workflow job for this annotation

GitHub Actions / Quality Others (ubuntu-24.04, 3.12, 20)

ruff (I001)

openedx/core/djangoapps/user_api/tests/test_views.py:3:1: I001 Import block is un-sorted or un-formatted help: Organize imports

USER_LIST_URI = "/api/user/v1/users/"
USER_PREFERENCE_LIST_URI = "/api/user/v1/user_prefs/"
ROLE_LIST_URI = "/api/user/v1/forum_roles/Moderator/users/"

User = get_user_model()

class UserAPITestCase(ApiTestCase):
"""
Expand Down Expand Up @@ -649,3 +652,130 @@
assert len(results) == len(common_timezones)
for time_zone_info in results:
self._assert_time_zone_is_valid(time_zone_info)


@override_settings(ENABLE_AUTHN_REGISTER_HIBP_POLICY=False)
@ddt.ddt
class TestUserModifyAPI(ApiTestCase):
"""Test cases covering the user modification API"""

PATH = "/api/user/v1/modify"

DATA = {
"name": "Test User",
"username": "testuser",
"password": "Password1234",
"email": "e@mail.com",
"terms_of_service": "true",
"honor_code": "true",
}

def setUp(self):
"""Create a test user and log in with that user"""
super().setUp()

self.test_user = UserFactory.create(
username="user",
email="user@example.com",
password="pass",
is_staff=True,
is_superuser=True,
)
self.client.login(username="user", password="pass")

@override_settings(ENFORCE_SAFE_SESSIONS=False)
def test_create_new_user_success(self):
"""Test creating a user with valid information"""
response = self.client.post(self.PATH, self.DATA)
assert response.status_code == status.HTTP_201_CREATED
created_user = User.objects.get(username=self.DATA["username"])
assert response.json() == {
"user_id": created_user.id,
"username": created_user.username,
}

@ddt.data("username", "password", "email", "terms_of_service", "honor_code")
def test_create_new_user_error_missing_info(self, missing_field):
"""Test creating a user with missing required information"""
data = self.DATA.copy()
data.pop(missing_field)
response = self.client.post(self.PATH, data)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "error" in response.json()
assert missing_field in str(response.json()["error"])

def test_create_new_user_error_invalid_attribute(self):
"""Test creating a user with an invalid attribute"""
data = self.DATA.copy()
data["email"] = "invalid-email"
response = self.client.post(self.PATH, data)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "error" in response.json()
assert "email" in str(response.json()["error"])

@ddt.data(
("email", "user@example.com", "existing account"),
("username", "user", "already exists"),
)
@ddt.unpack
def test_create_new_user_error_already_exists(self, field, value, error):
"""Test creating a user with an invalid attribute"""
data = self.DATA.copy()
data[field] = value
response = self.client.post(self.PATH, data)
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "error" in response.json()
assert error in str(response.json()["error"])

def test_patch_user_success(self):
"""Test updating a user with a valid lookup field"""
user = UserFactory.create(
username="patch-user",
email="patch@example.com",
)

response = self.client.patch(
self.PATH,
data=json.dumps({"email": user.email, "first_name": "Updated Name"}),
content_type="application/json",
)

assert response.status_code == status.HTTP_200_OK
assert response.json() == {"user_id": user.id, "username": user.username}
user = User.objects.get(id=user.id)
assert user.first_name == "Updated Name"

def test_patch_user_not_found(self):
"""Test patch returns 404 when no user matches the lookup fields"""
response = self.client.patch(
self.PATH,
data=json.dumps(
{"email": "missing@example.com", "first_name": "Updated Name"}
),
content_type="application/json",
)

assert response.status_code == status.HTTP_404_NOT_FOUND
assert response.json() == {"error": "User not found."}

@patch("openedx.core.djangoapps.user_api.views.UserSerializer.update")
def test_patch_user_validation_error(self, serializer_update):
"""Test serializer validation errors are returned with status 400"""
user = UserFactory.create(
username="test-user",
email="patch-error@example.com",
)
serializer_update.side_effect = ValidationError(
{"email": ["Invalid email address."]}
)

response = self.client.patch(
self.PATH,
data=json.dumps({"email": user.email, "first_name": "Updated Name"}),
content_type="application/json",
)

assert response.status_code == status.HTTP_400_BAD_REQUEST
assert response.json() == {
"error": {"email": ["Invalid email address."]}
}
3 changes: 3 additions & 0 deletions openedx/core/djangoapps/user_api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,9 @@
path('v1/accounts/replace_usernames/', UsernameReplacementView.as_view(),
name='username_replacement'
),
path('v1/modify', user_api_views.UserModifyView.as_view(),
name='user_account_modify'
),
re_path(
fr'^v1/preferences/{settings.USERNAME_PATTERN}$',
PreferencesView.as_view(),
Expand Down
138 changes: 135 additions & 3 deletions openedx/core/djangoapps/user_api/views.py
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
"""HTTP end-points for the User API. """

from django.contrib.auth.models import User # pylint: disable=imported-auth-user
from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db import transaction
from django.http import HttpResponse
from django.utils.decorators import method_decorator
from django.views.decorators.csrf import ensure_csrf_cookie
from django_filters.rest_framework import DjangoFilterBackend
from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication
from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser
from opaque_keys import InvalidKeyError
from opaque_keys.edx import locator
from opaque_keys.edx.keys import CourseKey
from rest_framework import generics, status, viewsets
from rest_framework.exceptions import ParseError
from rest_framework.permissions import IsAuthenticated
from rest_framework.exceptions import ParseError, ValidationError
from rest_framework.permissions import IsAdminUser, IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView

from common.djangoapps.student.helpers import AccountValidationError
from openedx.core.djangoapps.django_comment_common.models import Role
from openedx.core.djangoapps.user_api.models import UserPreference
from openedx.core.djangoapps.user_api.preferences.api import get_country_time_zones, update_email_opt_in
Expand All @@ -22,6 +27,8 @@
UserPreferenceSerializer,
UserSerializer,
)
from openedx.core.djangoapps.user_authn.views.register import create_account_with_params
from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser
from openedx.core.lib.api.permissions import ApiKeyHeaderPermission
from openedx.core.lib.api.view_utils import require_post_params

Expand Down Expand Up @@ -161,3 +168,128 @@ class CountryTimeZoneListView(generics.ListAPIView):
def get_queryset(self):
country_code = self.request.GET.get("country_code", None)
return get_country_time_zones(country_code)


class UserModifyView(APIView):
"""
**Use Cases**

Creates a user with email and username, or updates user information by
email or username.

**Example Requests**

POST /api/user/v1/modify/

PATCH /api/user/v1/modify/

**Example POST Response**

If the request is successful, an HTTP 201 "Created" response is
returned along with the user id and username, e.g.:

{
"user_id": 5,
"username": "newuser"
}

"""

authentication_classes = (
JwtAuthentication,
BearerAuthenticationAllowInactiveUser,
SessionAuthenticationAllowInactiveUser,
)
permission_classes = (IsAdminUser,)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure what permissions we want here, but just flagging to double check since this api is pretty powerful - it can modify arbitrary users.

Also not sure how this fits with the new RBAC system?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was developed before the new RBCA system and will need further changes to adapt to it. But as you can see, it isn't being used by any of the user_api django app views, so it all needs updating, which falls outside the scope of the work.


@method_decorator(transaction.non_atomic_requests)
def dispatch(self, request, *args, **kwargs):
"""
Decorate with `non_atomic_requests` to work on newer versions of platform.
"""
return super().dispatch(request, *args, **kwargs)
Comment on lines +205 to +210

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think applying this to patch can result in issues. If the password updates but the user doesn't you'll end with a partial update.


def post(self, request):
"""
Create a user with email and username.
"""
try:
user = create_account_with_params(request, request.data)
except (
AccountValidationError,
ValueError,
ValidationError,
DjangoValidationError,
) as e:
if isinstance(e, ValidationError):
message = e.detail
else:
message = str(e)
return Response(
data={"error": message},
status=status.HTTP_400_BAD_REQUEST,
)

return Response(
data={"user_id": user.id, "username": user.username},
status=status.HTTP_201_CREATED,
)

def patch(self, request):
"""
Update user information by email or username.
"""
Comment thread
viadanna marked this conversation as resolved.
try:
user = self._get_user_by_email_or_username(request)
if not user:
return Response(
data={"error": "User not found."},
status=status.HTTP_404_NOT_FOUND,
)

data = request.data.copy()
# Remove email and username from the data to prevent changing them
data.pop("email", None)
data.pop("username", None)
# Remove password from the data and handle it separately
password = data.pop("password", None)
if password:
user.set_password(password)
user = UserSerializer().update(user, data)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be better to validate this data before saving. i.e construct the serialiser from the data, check is_valid, and then save.

except (
AccountValidationError,
ValueError,
ValidationError,
DjangoValidationError,
) as e:
if isinstance(e, ValidationError):
message = e.detail
else:
message = str(e)
return Response(
data={"error": message},
status=status.HTTP_400_BAD_REQUEST,
)
return Response(
data={"user_id": user.id, "username": user.username},
status=status.HTTP_200_OK,
)

@staticmethod
def _get_user_by_email_or_username(request):
"""
Get a user by email and/or username.

Returns None if one doesn't exist.
"""

email = request.data.get("email")
username = request.data.get("username")
if not email and not username:
raise ValidationError("email or username must be specified")
query = {}
if email:
query["email"] = email
if username:
query["username"] = username
return User.objects.filter(**query).first()
Comment on lines +278 to +295

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps you can use the existing get_user_by_username_or_email function from common.djangoapps.student.models.user.

Loading