Legacy Lobotomy — Analysis of the Profile Management API

Photo by Andrew Neel on Unsplash

This is the 7th tutorial in the series about refactoring a legacy Django project. To get more ideas regarding the project used for this tutorial or check other tutorials in this series, check the introductory article published earlier.

Legacy Lobotomy — Confident Refactoring of a Django Project

Preface

The project is full of surprises and original ideas on how to shoot yourself in the foot. This makes it interesting for analysis and refactoring, but for the client and the development company, it became the reason of a disaster.

In this and next tutorials, my goal is to discuss the profile management functionality, a crucial component for nearly all systems. This functionality enables a client application to retrieve information about the currently signed-in user, allows users to update profile data, and occasionally, delete the profile. Typically, the backend system offers a set of API endpoints, which may be structured as follows:

GET /auth/user/ – returns profile details of the currently signed-in userPUT /auth/user/ and PATCH /auth/user/ – methods for full and partial update of the user’s personal dataDELETE /auth/user/ – if needed, provides a capability to delete a user’s profile. Depending on the requirements to the system, can provide physical, when all user’s information is completely deleted from the system, and logical, when a user’s status is changed to Deleted or similar and all data remains untouched, ways of deletion.

For this project only GET, PUT and DELETE endpoints are needed. Having such endpoints is the final goal of this and next tutorials. But we need to start with discussing the approach how this problem was solved by the development team and which important security issues were introduced with that implementation.

Origin branch and destination branch

If you want to follow the steps described in this tutorial, you should start from the login-refactoring branch.If you want to only see final code state after all changes from this tutorial are applied, you can check the profile-management-api-analysis branch.

Analysis of the current implementation

When I took a first look at the system, I didn’t pay attention to the UserViewSet class. But later, I noticed one thing that took my attention. Let’s see the source code of this class.

class UserViewSet(viewsets.ModelViewSet):
serializer_class = UserSerializer
queryset = User.objects.all()
authentication_classes = (TokenAuthentication,)
permission_classes = (IsAuthenticated, IsNotSuperuser)

def update(self, request, *args, **kwargs):
data = request.data
instance = self.get_object()
if instance.first_login:
data[‘first_login’] = False
serializer = UserSerializer(instance, data=data, partial=True)
serializer.is_valid(raise_exception=True)
serializer.save()
return Response(serializer.data)

We can see that this viewset class uses theUserSerializer serializer. So, let’s see the code of this class as well, it will help us much to analyze all the problems hidden in this view.

class UserSerializer(serializers.ModelSerializer):
class Meta:
model = User
fields = (
‘pk’,
’email’,
‘first_name’,
‘last_name’,
‘age’,
‘gender’,
‘guardian_email’,
‘accepted_terms_cond’,
‘activity’,
‘total_points’,
‘first_login’,
‘is_superuser’
)

Also, we can see that this viewset uses IsNotSuperuser permission class. This is a custom permission class implemented specifically for this project. Here is its code:

class IsNotSuperuser(permissions.BasePermission):
def has_permission(self, request, view):
return bool(request.user and not request.user.is_superuser)

What can we say after looking at this code? Here is the list of the most important things about it:

UserViewSet inherits ModelViewSet base class which provides all CRUD API methods for the model from the specified queryset. It’s possible to turn off handling of some specific HTTP methods with http_method_names view property. For instance, we can use http_method_names = [‘get’, ‘put’] if we want our viewset handle only GET and PUT methods. As we can see, this property wasn’t defined for this class, which means that such implementation will provide the following endpoints. But for what reason do we need them?
 — GET /users/ – returns a list of users
 — GET /users/<pk>/ – returns a user with a specified primary key pk
 — POST /users/ – creates a new user
 — PUT /users/<pk>/ and PATCH /users/<pk>/ – updates a user with a specified primary key pk
 — DELETE /users/<pk>/ – physically deletes a user with a specified primary key pkBy default, all fields defined in the fields property in the class of serializer. It means that such fields as total_points and is_superuser are also can be changed.permission_classes = (IsAuthenticated, IsNotSuperuser) instruction specifies that only authenticated users are permitted to access the API methods provided by the UserViewSet class, but only if they are not superusers. In essence, all authenticated users, excluding administrators, have the ability to view, create, and update other user profiles. They can downgrade superusers to regular users and vice versa, make themselves administrators, and even delete any user. What???On the instruction data[‘first_login’] = False an exception will be raised, because request.data is readonly dictionary and cannot be modified.

I was confused by having such a viewset in the system, it was clear that it doesn’t look good, but I couldn’t even imagine which purposes it serves for. So I had to check the source code of the mobile app which was the only client of the API, and I found the following code snippets:

const res = await instance.get(`/api/users/${pk}/`, {
headers: {
“Authorization”: `Token ${token}`
}
});const res = await instance.put(`/api/users/${id}/`, profileData, {
headers: {
“Authorization”: `Token ${token}`
}
});const res = await instance.delete(`/api/users/${id}/`, {
headers: {
“Authorization”: `Token ${token}`
}
});

So the app uses these API endpoints to get details of the currently signed-in user and allow them to update and delete the profile. Very interesting, original and brave solution :) Our goal is to get rid of this extremely dangerous solution and replace it with less vulnerable and straightforward approach. Fortunately, now we know that the app needs the backend to support only GET, PUT and DELETE methods.

Let’s modify the code in order to verify that all notices make sense and the system is so vulnerable. This is the first time we need to be able to send requests on behalf of users. Our existing client fixture cannot help us here, so it makes sense to create a new fixture called auth_client which will send requests on behalf of the user created with the user fixture. Also, we may want to send request on behalf of other users. For this purpose we need to create another fixture and call it somehow like auth_client_factory, which would provide capability to pass a user we want to send requests on behalf of. Let’s add the following code into the ~/src/conftest.py file.

@pytest.fixture
def auth_client(user, auth_client_factory):
return auth_client_factory(user)

@pytest.fixture
def auth_client_factory():
def auth_client(user):
from rest_framework.authtoken.models import Token
from rest_framework.test import APIClient

# create a token if it doesn’t exist yet
Token.objects.get_or_create(user=user)

# APIClient allows us to pass any HTTP attributes which will be used with all
# further requests sent by the instance of the APIClient
return APIClient(HTTP_AUTHORIZATION=f’Token {user.auth_token.key}’)

return auth_client

Now we can add a new file test_users_viewset.py into the ~/src/users/tests/api/ directory with the following content.

import pytest
from django.contrib.auth import get_user_model
from rest_framework import status

User = get_user_model()

class TestUsersViewSet:
@pytest.mark.django_db
def test_any_authenticated_user_can_see_the_list_of_all_users_in_the_system(self, auth_client, user, user_factory):
another_user = user_factory()

response = auth_client.get(‘/api/users/’)
returned_users = {item[‘pk’] for item in response.json()}

assert response.status_code == status.HTTP_200_OK
assert returned_users == {user.pk, another_user.pk}

@pytest.mark.django_db
def test_any_authenticated_user_can_see_details_of_any_user_in_the_system(self, auth_client, user_factory):
another_user = user_factory()

response = auth_client.get(f’/api/users/{another_user.pk}/’)

assert response.status_code == status.HTTP_200_OK
assert response.json()[‘pk’] == another_user.pk

@pytest.mark.django_db
def test_any_authenticated_user_can_make_themselves_a_superuser(self, auth_client_factory, user_factory):
# first_login=False is used to work around a bug with attempt to change request data
user = user_factory(is_superuser=False, first_login=False)
auth_client = auth_client_factory(user)
response = auth_client.patch(f’/api/users/{user.pk}/’, {‘is_superuser’: True})

user.refresh_from_db()

assert response.status_code == status.HTTP_200_OK
assert response.json()[‘is_superuser’] assert user.is_superuser

@pytest.mark.django_db
def test_any_authenticated_user_can_make_another_user_a_superuser(self, auth_client, user_factory):
# first_login=False is used to work around a bug with attempt to change request data
another_user = user_factory(is_superuser=False, first_login=False)
response = auth_client.patch(f’/api/users/{another_user.pk}/’, {‘is_superuser’: True})

another_user.refresh_from_db()

assert response.status_code == status.HTTP_200_OK

# verify that another user is already superuser
assert response.json()[‘is_superuser’] assert another_user.is_superuser

@pytest.mark.django_db
def test_any_authenticated_user_can_delete_another_user(self, auth_client, user_factory):
another_user = user_factory()

response = auth_client.delete(f’/api/users/{another_user.pk}/’)

assert response.status_code == status.HTTP_204_NO_CONTENT

# verify that an attempt to read the entry again causes the User.DoesNotExist exception
# that means that this user no longer exists
with pytest.raises(User.DoesNotExist):
another_user.refresh_from_db()

If we run the tests, they all should pass. It proves that the system has the following vulnerabilities:

Any authenticated user can see the list of all users in the systemAny authenticated user can see details of any user in the systemAny authenticated user can make themselves a superuserAny authenticated user can make another user a superuserAny authenticated user can delete another user

Should superusers really be disallowed to manage their profile in the app?

Well, it’s a good question. Based on the application code, superusers should be able to only use the chat from the app. All other features are hidden for them. In order to use the app, a user needs to be added to a team. If we run the project and open the admin panel, we will see there sections “Admins” and “Users”. If we try to add a new admin and a new user, we will see that we can (and have to) specify a team for regular users only, but it is not possible for admins. So it seems like it’s really should work this way, and we have to preserve such a restriction for profile management and other functionalities. But the system should allow the app to fetch details of a superuser. Otherwise, the request will fail with 403 Forbidden HTTP error and the app will not have a chance to allow a superuser to access the allowed features. So at least GET method should be allowed for all users. Let’s keep the rest methods not available for superusers, because this functionality is also hidden for them in the app.

Conclusion

In this tutorial we’ve discovered very important issues which system has. Having such vulnerabilities on production is very critical for the whole system because they allow an attacker to do everything with the registered users’ data. In the next tutorial I will demonstrate how to get rid of these issues and an easier way to implement this functionality.

Legacy Lobotomy — Analysis of the Profile Management API was originally published in Level Up Coding on Medium, where people are continuing the conversation by highlighting and responding to this story.

​ Level Up Coding – Medium

about Infinite Loop Digital

We support businesses by identifying requirements and helping clients integrate AI seamlessly into their operations.

Gartner
Gartner Digital Workplace Summit Generative Al

GenAI sessions:

  • 4 Use Cases for Generative AI and ChatGPT in the Digital Workplace
  • How the Power of Generative AI Will Transform Knowledge Management
  • The Perils and Promises of Microsoft 365 Copilot
  • How to Be the Generative AI Champion Your CIO and Organization Need
  • How to Shift Organizational Culture Today to Embrace Generative AI Tomorrow
  • Mitigate the Risks of Generative AI by Enhancing Your Information Governance
  • Cultivate Essential Skills for Collaborating With Artificial Intelligence
  • Ask the Expert: Microsoft 365 Copilot
  • Generative AI Across Digital Workplace Markets
10 – 11 June 2024

London, U.K.