From 0f23a449c7f745cb507760a27ae83524d9c230d7 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 8 Mar 2023 13:50:57 +0100 Subject: [PATCH] add unique idx on user column in mobileapp authtoken table (#1482) # Which issue(s) this PR fixes Solves the (rare) issue where a user could potentially have > 1 mobileapp auth token, leading to 500 errors when trying to interact w/ the authtoken (ex. disconnect a mobile app from a user's profile): ```shell 2023-03-07 10:12:13 source=engine:app google_trace_id=e14bf933d634068a48caf093ce43c7f5/5550677047491218352 logger=django.request Internal Server Error: /api/internal/v1/users/U6WJ3BRLM1TR7/unlink_backend Traceback (most recent call last): File "/usr/local/lib/python3.9/site-packages/django/core/handlers/exception.py", line 47, in inner response = get_response(request) File "/usr/local/lib/python3.9/site-packages/django/core/handlers/base.py", line 181, in _get_response response = wrapped_callback(request, *callback_args, **callback_kwargs) File "/usr/local/lib/python3.9/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view return view_func(*args, **kwargs) File "/usr/local/lib/python3.9/site-packages/rest_framework/viewsets.py", line 125, in view return self.dispatch(request, *args, **kwargs) File "/usr/local/lib/python3.9/site-packages/rest_framework/views.py", line 509, in dispatch response = self.handle_exception(exc) File "/usr/local/lib/python3.9/site-packages/rest_framework/views.py", line 469, in handle_exception self.raise_uncaught_exception(exc) File "/usr/local/lib/python3.9/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception raise exc File "/usr/local/lib/python3.9/site-packages/rest_framework/views.py", line 506, in dispatch response = handler(request, *args, **kwargs) File "/etc/app/apps/api/views/user.py", line 453, in unlink_backend backend.unlink_user(user) File "/etc/app/apps/mobile_app/backend.py", line 34, in unlink_user token = MobileAppAuthToken.objects.get(user=user) File "/usr/local/lib/python3.9/site-packages/django/db/models/manager.py", line 85, in manager_method return getattr(self.get_queryset(), name)(*args, **kwargs) File "/usr/local/lib/python3.9/site-packages/django/db/models/query.py", line 439, in get raise self.model.MultipleObjectsReturned( apps.mobile_app.models.MobileAppAuthToken.MultipleObjectsReturned: get() returned more than one MobileAppAuthToken -- it returned 2! ``` ## Checklist - [x] Tests updated - [ ] Documentation added (N/A) - [x] `CHANGELOG.md` updated --- CHANGELOG.md | 2 + .../0002_alter_mobileappauthtoken_user.py | 29 +++++++ engine/apps/mobile_app/models.py | 4 +- .../tests/test_mobile_app_auth_token.py | 77 +++++++++++++++++++ engine/conftest.py | 23 ++++++ 5 files changed, 132 insertions(+), 3 deletions(-) create mode 100644 engine/apps/mobile_app/migrations/0002_alter_mobileappauthtoken_user.py create mode 100644 engine/apps/mobile_app/tests/test_mobile_app_auth_token.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 205455c6..e974fd9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Prohibit creating & updating past overrides ([1474](https://github.com/grafana/oncall/pull/1474)) +- Add unique index on `user_id` column in `mobile_app_mobileappauthtoken` table to avoid allowing a user + to have more than one mobile app auth token at a time ([1482](https://github.com/grafana/oncall/pull/1482)) ## v1.1.33 (2023-03-07) diff --git a/engine/apps/mobile_app/migrations/0002_alter_mobileappauthtoken_user.py b/engine/apps/mobile_app/migrations/0002_alter_mobileappauthtoken_user.py new file mode 100644 index 00000000..4efa17eb --- /dev/null +++ b/engine/apps/mobile_app/migrations/0002_alter_mobileappauthtoken_user.py @@ -0,0 +1,29 @@ +from django.db import migrations, models +import django.db.models.deletion + + +def delete_user_duplicate_mobileappauthtokens(apps, _): + MobileAppAuthToken = apps.get_model('mobile_app', 'MobileAppAuthToken') + + # start w/ the oldest mobile app auth tokens (ORDER BY id ASC) + # and if we find any newer tokens, delete the earlier ones (ie. `row` variable) + for row in MobileAppAuthToken.objects.all().order_by('id'): + if MobileAppAuthToken.objects.filter(user_id=row.user_id).count() > 1: + row.delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('user_management', '0008_organization_is_grafana_incident_enabled'), + ('mobile_app', '0001_initial'), + ] + + operations = [ + migrations.RunPython(delete_user_duplicate_mobileappauthtokens, migrations.RunPython.noop), + migrations.AlterField( + model_name='mobileappauthtoken', + name='user', + field=models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='user_management.user'), + ), + ] diff --git a/engine/apps/mobile_app/models.py b/engine/apps/mobile_app/models.py index 7ad107dd..08d2c1f2 100644 --- a/engine/apps/mobile_app/models.py +++ b/engine/apps/mobile_app/models.py @@ -51,9 +51,7 @@ class MobileAppVerificationToken(BaseAuthToken): class MobileAppAuthToken(BaseAuthToken): - user = models.ForeignKey( - to=User, null=False, blank=False, related_name="mobile_app_auth_tokens", on_delete=models.CASCADE - ) + user = models.OneToOneField(to=User, null=False, blank=False, on_delete=models.CASCADE) organization = models.ForeignKey( to=Organization, null=False, blank=False, related_name="mobile_app_auth_tokens", on_delete=models.CASCADE ) diff --git a/engine/apps/mobile_app/tests/test_mobile_app_auth_token.py b/engine/apps/mobile_app/tests/test_mobile_app_auth_token.py new file mode 100644 index 00000000..af92a8bd --- /dev/null +++ b/engine/apps/mobile_app/tests/test_mobile_app_auth_token.py @@ -0,0 +1,77 @@ +import pytest +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient + +from apps.mobile_app.models import MobileAppAuthToken + + +@pytest.mark.django_db +def test_mobile_app_auth_token( + make_organization_and_user_with_mobile_app_verification_token, +): + organization, user, verification_token = make_organization_and_user_with_mobile_app_verification_token() + + client = APIClient() + url = reverse("mobile_app:auth_token") + + response = client.post(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + response = client.post(url, HTTP_AUTHORIZATION=verification_token) + assert response.status_code == status.HTTP_201_CREATED + + original_auth_token_id = response.data["id"] + original_auth_token = response.data["token"] + original_auth_token_created_at = response.data["created_at"] + + assert original_auth_token_id is not None + assert original_auth_token is not None + assert original_auth_token_created_at is not None + + # we can fetch the token + response = client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + response = client.get(url, HTTP_AUTHORIZATION=verification_token) + assert response.status_code == status.HTTP_200_OK + + assert response.data["token_id"] == original_auth_token_id + assert response.data["user_id"] == user.id + assert response.data["organization_id"] == organization.id + assert response.data["created_at"] == original_auth_token_created_at + assert response.data["revoked_at"] is None + + # can only ever have one mobile app auth token.. old one gets deleted if we try + # creating a new one + response = client.post(url, HTTP_AUTHORIZATION=verification_token) + assert response.status_code == status.HTTP_201_CREATED + + new_auth_token_id = response.data["id"] + new_auth_token = response.data["token"] + new_auth_token_created_at = response.data["created_at"] + + assert new_auth_token_id is not None + assert new_auth_token is not None + assert new_auth_token_created_at is not None + + assert new_auth_token_id != original_auth_token_id + assert new_auth_token != original_auth_token + assert new_auth_token_created_at != original_auth_token_created_at + + assert MobileAppAuthToken.objects.filter(user=user).count() == 1 + + # we can delete the token + response = client.delete(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + + response = client.delete(url, HTTP_AUTHORIZATION=verification_token) + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert MobileAppAuthToken.objects.filter(user=user).count() == 0 + + response = client.delete(url, HTTP_AUTHORIZATION=verification_token) + assert response.status_code == status.HTTP_404_NOT_FOUND + + response = client.get(url, HTTP_AUTHORIZATION=verification_token) + assert response.status_code == status.HTTP_404_NOT_FOUND diff --git a/engine/conftest.py b/engine/conftest.py index 7c58208a..5d15c8c2 100644 --- a/engine/conftest.py +++ b/engine/conftest.py @@ -55,6 +55,7 @@ from apps.base.tests.factories import ( ) from apps.email.tests.factories import EmailMessageFactory from apps.heartbeat.tests.factories import IntegrationHeartBeatFactory +from apps.mobile_app.models import MobileAppVerificationToken from apps.schedules.tests.factories import ( CustomOnCallShiftFactory, OnCallScheduleCalendarFactory, @@ -175,6 +176,14 @@ def make_token_for_organization(): return _make_token_for_organization +@pytest.fixture +def make_mobile_app_verification_token_for_user(): + def _make_mobile_app_verification_token_for_user(user, organization): + return MobileAppVerificationToken.create_auth_token(user, organization) + + return _make_mobile_app_verification_token_for_user + + @pytest.fixture def make_public_api_token(): def _make_public_api_token(user, organization, name="test_api_token"): @@ -643,6 +652,20 @@ def make_organization_and_user_with_plugin_token(make_organization_and_user, mak return _make_organization_and_user_with_plugin_token +@pytest.fixture() +def make_organization_and_user_with_mobile_app_verification_token( + make_organization_and_user, make_mobile_app_verification_token_for_user +): + def _make_organization_and_user_with_mobile_app_verification_token( + role: typing.Optional[LegacyAccessControlRole] = None, + ): + organization, user = make_organization_and_user(role) + _, token = make_mobile_app_verification_token_for_user(user, organization) + return organization, user, token + + return _make_organization_and_user_with_mobile_app_verification_token + + @pytest.fixture() def mock_send_user_notification_signal(monkeypatch): def mocked_send_signal(*args, **kwargs):