diff --git a/openedx/core/djangoapps/user_api/accounts/permissions.py b/openedx/core/djangoapps/user_api/accounts/permissions.py index 691df5629a..459f0129fb 100644 --- a/openedx/core/djangoapps/user_api/accounts/permissions.py +++ b/openedx/core/djangoapps/user_api/accounts/permissions.py @@ -2,7 +2,6 @@ Permissions classes for User accounts API views. """ - from django.conf import settings from rest_framework import permissions @@ -12,6 +11,7 @@ class CanDeactivateUser(permissions.BasePermission): Grants access to AccountDeactivationView if the requesting user is a superuser or has the explicit permission to deactivate a User account. """ + def has_permission(self, request, view): return request.user.has_perm('student.can_deactivate_users') @@ -22,6 +22,7 @@ class CanRetireUser(permissions.BasePermission): a superuser, the RETIREMENT_SERVICE_USERNAME, or has the explicit permission to retire a User account. """ + def has_permission(self, request, view): return request.user.has_perm('accounts.can_retire_user') @@ -30,5 +31,18 @@ class CanReplaceUsername(permissions.BasePermission): """ Grants access to the Username Replacement API for the service user. """ + def has_permission(self, request, view): return request.user.username == getattr(settings, "USERNAME_REPLACEMENT_WORKER", False) + + +class CanGetAccountInfoUsingId(permissions.BasePermission): + """ + Grants access to AccountViewSet if the requesting user is a superuser/staff + and reqesting to get account info based on LMS User id + """ + + def has_permission(self, request, view): + return request.GET.get('lms_user_id') is None or ( + request.GET.get('lms_user_id') and (request.user.is_staff or request.user.is_superuser) + ) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index 9a91d20969..75ee16ffe4 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -2,7 +2,6 @@ Test cases to cover Accounts-related behaviors of the User API application """ - import datetime import hashlib import json @@ -15,15 +14,16 @@ from django.conf import settings from django.test.testcases import TransactionTestCase from django.test.utils import override_settings from django.urls import reverse +from rest_framework import status from rest_framework.test import APIClient, APITestCase +from common.djangoapps.student.models import PendingEmailChange, UserProfile +from common.djangoapps.student.tests.factories import TEST_PASSWORD, RegistrationFactory, UserFactory from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user from openedx.core.djangoapps.user_api.accounts import ACCOUNT_VISIBILITY_PREF_KEY from openedx.core.djangoapps.user_api.models import UserPreference from openedx.core.djangoapps.user_api.preferences.api import set_user_preference from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms -from common.djangoapps.student.models import PendingEmailChange, UserProfile -from common.djangoapps.student.tests.factories import TEST_PASSWORD, UserFactory, RegistrationFactory from .. import ALL_USERS_VISIBILITY, CUSTOM_VISIBILITY, PRIVATE_VISIBILITY @@ -94,7 +94,7 @@ class UserAPITestCase(APITestCase): """ Helper method for sending a GET to the server. Verifies the expected status and returns the response. """ - url = self.url + '?' + query_parameters if query_parameters else self.url # pylint: disable=no-member + url = self.url + '?' + query_parameters if query_parameters else self.url # pylint: disable=no-member response = client.get(url) assert expected_status == response.status_code return response @@ -386,6 +386,34 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): response = self.send_get(client, query_parameters=f'email={self.user.email}') self._verify_full_account_response(response) + def test_successful_get_account_by_user_id(self): + """ + Test that request using lms user id by a staff user successfully retrieves Account Info. + """ + api_client = "staff_client" + user = "staff_user" + client = self.login_client(api_client, user) + self.create_mock_profile(self.user) + set_user_preference(self.user, ACCOUNT_VISIBILITY_PREF_KEY, PRIVATE_VISIBILITY) + + response = self.send_get(client, query_parameters=f'lms_user_id={self.user.id}') + self._verify_full_account_response(response) + + def test_unsuccessful_get_account_by_user_id(self): + """ + Test that requesting using lms user id by a normal user fails to retrieve Account Info. + """ + api_client = "client" + user = "user" + client = self.login_client(api_client, user) + self.create_mock_profile(self.user) + set_user_preference(self.user, ACCOUNT_VISIBILITY_PREF_KEY, PRIVATE_VISIBILITY) + + response = self.send_get( + client, query_parameters=f'lms_user_id={self.user.id}', expected_status=status.HTTP_403_FORBIDDEN + ) + assert response.data.get('detail') == 'You do not have permission to perform this action.' + def test_search_emails(self): client = self.login_client('staff_client', 'staff_user') json_data = {'emails': [self.user.email]} @@ -460,6 +488,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): """ Test the return from GET based on user visibility setting. """ + def verify_fields_visible_to_all_users(response): """ Confirms that private fields are private, and public/shareable fields are public/shareable @@ -688,11 +717,11 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): assert expected_user_message == error_response.data['field_errors'][field]['user_message'] - assert "Value '{value}' is not valid for field '{field}': {messages}"\ - .format(value=fails_validation_value, - field=field, - messages=[developer_validation_message]) ==\ - error_response.data['field_errors'][field]['developer_message'] + assert "Value '{value}' is not valid for field '{field}': {messages}".format( + value=fails_validation_value, + field=field, + messages=[developer_validation_message] + ) == error_response.data['field_errors'][field]['developer_message'] elif field != "account_privacy": # If there are no values that would fail validation, then empty string should be supported; @@ -720,8 +749,9 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): Internal helper to check the error messages returned """ assert 'This field is not editable via this API' == data['field_errors'][field_name]['developer_message'] - assert "The '{}' field cannot be edited."\ - .format(field_name) == data['field_errors'][field_name]['user_message'] + assert "The '{}' field cannot be edited.".format( + field_name + ) == data['field_errors'][field_name]['user_message'] for field_name in ["username", "date_joined", "is_active", "profile_image", "requires_parental_consent"]: response = self.send_patch(client, {field_name: "will_error", "gender": "o"}, expected_status=400) @@ -765,6 +795,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): """ Test the metadata stored when changing the name field. """ + def get_name_change_info(expected_entries): """ Internal method to encapsulate the retrieval of old names used @@ -799,7 +830,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): # Now change the name again and verify meta information. self.send_patch(self.client, {"name": "Donald Duck"}) name_change_info = get_name_change_info(2) - verify_change_info(name_change_info[0], old_name, self.user.username, "Donald Duck",) + verify_change_info(name_change_info[0], old_name, self.user.username, "Donald Duck", ) verify_change_info(name_change_info[1], "Mickey Mouse", self.user.username, "Donald Duck") @mock.patch.dict( @@ -850,7 +881,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): # Try changing to an invalid email to make sure error messages are appropriately returned. error_response = self.send_patch(client, {"email": bad_email}, expected_status=400) field_errors = error_response.data["field_errors"] - assert "Error thrown from validate_new_email: 'Valid e-mail address required.'" ==\ + assert "Error thrown from validate_new_email: 'Valid e-mail address required.'" == \ field_errors['email']['developer_message'] assert 'Valid e-mail address required.' == field_errors['email']['user_message'] @@ -919,7 +950,7 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): client = self.login_client("client", "user") response = self.send_patch(client, {"language_proficiencies": patch_value}, expected_status=400) assert response.data['field_errors']['language_proficiencies']['developer_message'] == \ - f"Value '{patch_value}' is not valid for field 'language_proficiencies': {expected_error_message}" + f"Value '{patch_value}' is not valid for field 'language_proficiencies': {expected_error_message}" @mock.patch('openedx.core.djangoapps.user_api.accounts.serializers.AccountUserSerializer.save') def test_patch_serializer_save_fails(self, serializer_save): @@ -941,9 +972,9 @@ class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): """ self.client.login(username=self.user.username, password=TEST_PASSWORD) response = self.send_get(self.client) - assert response.data['profile_image'] ==\ - {'has_image': False, - 'image_url_full': 'http://testserver/static/default_50.png', + assert response.data['profile_image'] == \ + {'has_image': False, + 'image_url_full': 'http://testserver/static/default_50.png', 'image_url_small': 'http://testserver/static/default_10.png'} @ddt.data( diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index beb250e5f4..7aed9e2b98 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -5,15 +5,12 @@ For additional information and historical context, see: https://openedx.atlassian.net/wiki/display/TNL/User+API """ - import datetime import logging import uuid from functools import wraps import pytz -from rest_framework.exceptions import UnsupportedMediaType - from consent.models import DataSharingConsent from django.apps import apps from django.conf import settings @@ -31,6 +28,7 @@ from integrated_channels.degreed.models import DegreedLearnerDataTransmissionAud from integrated_channels.sap_success_factors.models import SapSuccessFactorsLearnerDataTransmissionAudit from rest_framework import permissions, status from rest_framework.authentication import SessionAuthentication +from rest_framework.exceptions import UnsupportedMediaType from rest_framework.parsers import JSONParser from rest_framework.response import Response from rest_framework.serializers import ValidationError @@ -41,13 +39,11 @@ from wiki.models.pluginbase import RevisionPluginRevision from common.djangoapps.entitlements.models import CourseEntitlement from common.djangoapps.student.models import ( # lint-amnesty, pylint: disable=unused-import - AccountRecovery, CourseEnrollmentAllowed, LoginFailures, ManualEnrollmentAudit, PendingEmailChange, PendingNameChange, - Registration, User, UserProfile, get_potentially_retired_user_by_username, @@ -78,7 +74,7 @@ from ..models import ( UserRetirementStatus ) from .api import get_account_settings, update_account_settings -from .permissions import CanDeactivateUser, CanReplaceUsername, CanRetireUser +from .permissions import CanDeactivateUser, CanGetAccountInfoUsingId, CanReplaceUsername, CanRetireUser from .serializers import ( PendingNameChangeSerializer, UserRetirementPartnerReportSerializer, @@ -114,6 +110,7 @@ def request_requires_username(function): Requires that a ``username`` key containing a truthy value exists in the ``request.data`` attribute of the decorated function. """ + @wraps(function) def wrapper(self, request): # pylint: disable=missing-docstring username = request.data.get('username', None) @@ -123,6 +120,7 @@ def request_requires_username(function): data={'message': 'The user was not specified.'} ) return function(self, request) + return wrapper @@ -293,7 +291,7 @@ class AccountViewSet(ViewSet): authentication_classes = ( JwtAuthentication, BearerAuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser ) - permission_classes = (permissions.IsAuthenticated,) + permission_classes = (permissions.IsAuthenticated, CanGetAccountInfoUsingId) parser_classes = (JSONParser, MergePatchParser,) def get(self, request): @@ -306,9 +304,11 @@ class AccountViewSet(ViewSet): """ GET /api/user/v1/accounts?username={username1,username2} GET /api/user/v1/accounts?email={user_email} + GET /api/user/v1/accounts?lms_user_id={lms_user_id} (Staff Only) """ usernames = request.GET.get('username') user_email = request.GET.get('email') + lms_user_id = request.GET.get('lms_user_id') search_usernames = [] if usernames: @@ -320,9 +320,16 @@ class AccountViewSet(ViewSet): except (UserNotFound, User.DoesNotExist): return Response(status=status.HTTP_404_NOT_FOUND) search_usernames = [user.username] + elif lms_user_id: + try: + user = User.objects.get(id=lms_user_id) + except (UserNotFound, User.DoesNotExist): + return Response(status=status.HTTP_404_NOT_FOUND) + search_usernames = [user.username] try: account_settings = get_account_settings( - request, search_usernames, view=request.query_params.get('view')) + request, search_usernames, view=request.query_params.get('view') + ) except UserNotFound: return Response(status=status.HTTP_404_NOT_FOUND) @@ -459,7 +466,7 @@ class AccountDeactivationView(APIView): Account deactivation viewset. Currently only supports POST requests. Only admins can deactivate accounts. """ - authentication_classes = (JwtAuthentication, ) + authentication_classes = (JwtAuthentication,) permission_classes = (permissions.IsAuthenticated, CanDeactivateUser) def post(self, request, username): @@ -505,8 +512,8 @@ class DeactivateLogoutView(APIView): - Log the user out - Create a row in the retirement table for that user """ - authentication_classes = (JwtAuthentication, SessionAuthentication, ) - permission_classes = (permissions.IsAuthenticated, ) + authentication_classes = (JwtAuthentication, SessionAuthentication,) + permission_classes = (permissions.IsAuthenticated,) def post(self, request): """ @@ -1208,7 +1215,7 @@ class UsernameReplacementView(APIView): This API will be called first, before calling the APIs in other services as this one handles the checks on the usernames provided. """ - authentication_classes = (JwtAuthentication, ) + authentication_classes = (JwtAuthentication,) permission_classes = (permissions.IsAuthenticated, CanReplaceUsername) def post(self, request):