diff --git a/backend/README.md b/backend/README.md index 15f09867..d9394d79 100644 --- a/backend/README.md +++ b/backend/README.md @@ -181,7 +181,7 @@ Every request operates within an organization context: - **Organization (Org)**: Top-level tenant container - **Users**: Regular members with USER role - **Admins**: Organization administrators with ADMIN role -- **Super Admin**: Users with @micropyramid.com email domain have platform-wide access +- **Super Admin**: Users with `is_superuser` set on the user record have platform-wide access. Grant it deliberately (`manage.py createsuperuser` or the Django admin) — it is never inferred from the email address ### Authentication @@ -192,8 +192,9 @@ Authorization: Bearer ``` - Organization ID is embedded in the JWT token (not sent as header) -- Access token lifetime: 1 day -- Refresh token lifetime: 365 days +- Access token lifetime: 1 hour +- Refresh token lifetime: 14 days +- Refresh tokens are single-use: `/api/auth/refresh-token/` blacklists the token you send and returns a replacement, so clients must persist the new `refresh` value from every response ### Middleware diff --git a/backend/common/admin.py b/backend/common/admin.py index 2b4118ba..f4fd4389 100644 --- a/backend/common/admin.py +++ b/backend/common/admin.py @@ -1,6 +1,6 @@ from django.contrib import admin -from common.models import Address, Comment, CommentFiles, SessionToken, User +from common.models import Address, Comment, CommentFiles, User # Register your models here. @@ -8,46 +8,3 @@ admin.site.register(User) admin.site.register(Address) admin.site.register(Comment) admin.site.register(CommentFiles) - - -@admin.register(SessionToken) -class SessionTokenAdmin(admin.ModelAdmin): - list_display = ( - "user", - "token_jti_short", - "is_active", - "expires_at", - "last_used_at", - "created_at", - ) - list_filter = ("is_active", "expires_at", "created_at") - search_fields = ("user__email", "token_jti", "ip_address") - raw_id_fields = ("user",) - readonly_fields = ( - "token_jti", - "refresh_token_jti", - "created_at", - "last_used_at", - "revoked_at", - ) - date_hierarchy = "created_at" - ordering = ("-created_at",) - actions = ["revoke_tokens", "cleanup_expired"] - - def token_jti_short(self, obj): - return f"{obj.token_jti[:16]}..." - - token_jti_short.short_description = "Token JTI" - - def revoke_tokens(self, request, queryset): - for token in queryset: - token.revoke() - self.message_user(request, f"{queryset.count()} tokens revoked successfully.") - - revoke_tokens.short_description = "Revoke selected tokens" - - def cleanup_expired(self, request, queryset): - count, _ = SessionToken.cleanup_expired() - self.message_user(request, f"{count} expired tokens cleaned up.") - - cleanup_expired.short_description = "Cleanup expired tokens" diff --git a/backend/common/migrations/0029_delete_sessiontoken.py b/backend/common/migrations/0029_delete_sessiontoken.py new file mode 100644 index 00000000..9f988d76 --- /dev/null +++ b/backend/common/migrations/0029_delete_sessiontoken.py @@ -0,0 +1,39 @@ +"""Drop the unused `session_token` table. + +`SessionToken` was added as "Phase 3: JWT Token Tracking" but was never wired +up — nothing in any view, middleware, serializer, or task ever created, read, +or revoked a row. Its only consumer was a Django admin page listing a +permanently empty table, which read as a working session-revocation feature +that did not exist. + +Real refresh-token revocation now lives in simplejwt's `token_blacklist` app, +which `OrgAwareTokenRefreshView` writes to on every rotation, so this model has +no remaining purpose. +""" + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('common', '0028_restamp_rls_policies'), + ] + + operations = [ + migrations.RemoveIndex( + model_name='sessiontoken', + name='session_tok_user_id_ce3d01_idx', + ), + migrations.RemoveIndex( + model_name='sessiontoken', + name='session_tok_token_j_a0ab6c_idx', + ), + migrations.RemoveIndex( + model_name='sessiontoken', + name='session_tok_expires_08e0bc_idx', + ), + migrations.DeleteModel( + name='SessionToken', + ), + ] diff --git a/backend/common/models.py b/backend/common/models.py index 7f246cc2..85f57250 100644 --- a/backend/common/models.py +++ b/backend/common/models.py @@ -507,56 +507,6 @@ class APISettings(BaseModel): super().save(*args, **kwargs) -# Phase 3: JWT Token Tracking - - -class SessionToken(BaseModel): - """Track active JWT sessions for security""" - - user = models.ForeignKey( - User, on_delete=models.CASCADE, related_name="session_tokens" - ) - token_jti = models.CharField(max_length=255, unique=True, db_index=True) # JWT ID - refresh_token_jti = models.CharField( - max_length=255, unique=True, db_index=True, null=True, blank=True - ) - ip_address = models.GenericIPAddressField(null=True, blank=True) - user_agent = models.TextField(blank=True, null=True) - expires_at = models.DateTimeField() - is_active = models.BooleanField(default=True) - revoked_at = models.DateTimeField(null=True, blank=True) - last_used_at = models.DateTimeField(auto_now=True) - - class Meta: - verbose_name = "Session Token" - verbose_name_plural = "Session Tokens" - db_table = "session_token" - ordering = ("-created_at",) - indexes = [ - models.Index(fields=["user", "is_active"]), - models.Index(fields=["token_jti"]), - models.Index(fields=["expires_at"]), - ] - - def __str__(self): - return f"{self.user.email} - {self.token_jti[:8]}..." - - def revoke(self): - """Revoke this session token""" - from django.utils import timezone - - self.is_active = False - self.revoked_at = timezone.now() - self.save() - - @classmethod - def cleanup_expired(cls): - """Remove expired tokens (call via cron/celery)""" - from django.utils import timezone - - return cls.objects.filter(expires_at__lt=timezone.now()).delete() - - class MagicLinkToken(models.Model): """One-time magic link tokens for passwordless authentication. diff --git a/backend/common/permissions.py b/backend/common/permissions.py index f70765cd..77088a57 100644 --- a/backend/common/permissions.py +++ b/backend/common/permissions.py @@ -63,15 +63,21 @@ class IsSuperAdmin(permissions.BasePermission): """ Permission class for platform-level super admins. - Super admins are identified by @micropyramid.com email domain. - They have access to admin panel and can manage all organizations. + Super admin is an explicit, deliberately granted flag on the user record + (``User.is_superuser``), never inferred from the email address. Deriving it + from an email domain would hand platform-wide access — every org, every + user — to anyone who can obtain an account at that domain, turning an + ordinary signup into vertical privilege escalation. + + Grant it with ``manage.py createsuperuser``, the Django admin, or another + audited path — not by handing out an email address. """ message = "Super admin access required." def has_permission(self, request, view): - if not request.user or not request.user.is_authenticated: + user = getattr(request, "user", None) + if not user or not user.is_authenticated: return False - # Check for super admin email domain - return request.user.email.endswith("@micropyramid.com") + return bool(user.is_active and user.is_superuser) diff --git a/backend/common/tasks.py b/backend/common/tasks.py index 5cf7bd8a..3be3ee03 100644 --- a/backend/common/tasks.py +++ b/backend/common/tasks.py @@ -395,3 +395,31 @@ def purge_read_notifications(days=NOTIFICATION_PURGE_DAYS): "Purged %s read notifications older than %s days", deleted, days ) return deleted + + +@shared_task +def flush_expired_refresh_tokens(): + """Delete refresh-token bookkeeping rows whose tokens have already expired. + + `OrgAwareTokenRefreshView` rotates refresh tokens, so simplejwt writes one + `OutstandingToken` row per token minted and one `BlacklistedToken` row per + rotation. An expired token is rejected on its `exp` claim regardless of + blacklist state, so those rows stop carrying security value once + `expires_at` passes — and would otherwise grow unbounded, one row per login + and per refresh. Deleting the outstanding row cascades to its blacklist + entry. + + Equivalent to simplejwt's `manage.py flushexpiredtokens`; schedule via + celery-beat (nightly is plenty). Not org-scoped — `expires_at` is intrinsic + to the row, so no RLS context is needed. + + Returns the number of outstanding-token rows removed. + """ + from rest_framework_simplejwt.token_blacklist.models import OutstandingToken + + expired = OutstandingToken.objects.filter(expires_at__lte=timezone.now()) + count = expired.count() + if count: + expired.delete() + logger.info("Flushed %s expired refresh token records", count) + return count diff --git a/backend/common/tests/test_auth.py b/backend/common/tests/test_auth.py index f256b5be..793f1361 100644 --- a/backend/common/tests/test_auth.py +++ b/backend/common/tests/test_auth.py @@ -8,9 +8,11 @@ Run with: pytest common/tests/test_auth.py -v import base64 import json import uuid +from datetime import timedelta from unittest.mock import MagicMock, patch import pytest +from django.utils import timezone from rest_framework import status from rest_framework.exceptions import PermissionDenied @@ -118,6 +120,65 @@ class TestTokenRefreshView: ) assert response.status_code == status.HTTP_403_FORBIDDEN + def test_refresh_rejects_replayed_token( + self, unauthenticated_client, admin_user, org_a, admin_profile + ): + """A refresh token is single-use: replaying it after rotation must fail. + + Without blacklisting, a stolen refresh token keeps minting access + tokens for its full lifetime even after the legitimate user rotates. + """ + token = str( + OrgAwareRefreshToken.for_user_and_org(admin_user, org_a, admin_profile) + ) + + first = unauthenticated_client.post( + self.url, {"refresh": token}, format="json" + ) + assert first.status_code == status.HTTP_200_OK + + replay = unauthenticated_client.post( + self.url, {"refresh": token}, format="json" + ) + assert replay.status_code == status.HTTP_401_UNAUTHORIZED + + def test_refresh_rejects_replayed_token_without_org( + self, unauthenticated_client, admin_user + ): + """The no-org refresh branch must rotate its token too.""" + token = str(OrgAwareRefreshToken.for_user_and_org(admin_user, None)) + + first = unauthenticated_client.post( + self.url, {"refresh": token}, format="json" + ) + assert first.status_code == status.HTTP_200_OK + + replay = unauthenticated_client.post( + self.url, {"refresh": token}, format="json" + ) + assert replay.status_code == status.HTTP_401_UNAUTHORIZED + + def test_refresh_rotated_token_is_usable( + self, unauthenticated_client, admin_user, org_a, admin_profile + ): + """Rotation must hand back a token that works for the next refresh.""" + token = str( + OrgAwareRefreshToken.for_user_and_org(admin_user, org_a, admin_profile) + ) + + first = unauthenticated_client.post( + self.url, {"refresh": token}, format="json" + ) + assert first.status_code == status.HTTP_200_OK + + second = unauthenticated_client.post( + self.url, + {"refresh": first.data["refresh"]}, + format="json", + ) + assert second.status_code == status.HTTP_200_OK + assert second.data["refresh"] != first.data["refresh"] + @pytest.mark.django_db class TestOrgSwitchView: @@ -192,6 +253,104 @@ class TestOrgSwitchView: ) assert response.status_code == status.HTTP_401_UNAUTHORIZED + def test_switch_org_revokes_presented_refresh_token( + self, + admin_client, + unauthenticated_client, + admin_user, + org_a, + admin_profile, + org_b, + ): + """Switching orgs must retire the refresh token the caller was holding. + + The client replaces its token pair on switch, so leaving the old refresh + token live just means a stolen copy keeps working against the old org for + the rest of its 14-day life. + """ + Profile.objects.create(user=admin_user, org=org_b, role="USER", is_active=True) + old_refresh = str( + OrgAwareRefreshToken.for_user_and_org(admin_user, org_a, admin_profile) + ) + + response = admin_client.post( + self.url, + {"org_id": str(org_b.id), "refresh": old_refresh}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + + replay = unauthenticated_client.post( + "/api/auth/refresh-token/", {"refresh": old_refresh}, format="json" + ) + assert replay.status_code == status.HTTP_401_UNAUTHORIZED + + def test_switch_org_returned_refresh_token_is_usable( + self, admin_client, unauthenticated_client, admin_user, org_a, admin_profile, org_b + ): + """Retiring the old token must not strand the caller.""" + Profile.objects.create(user=admin_user, org=org_b, role="USER", is_active=True) + old_refresh = str( + OrgAwareRefreshToken.for_user_and_org(admin_user, org_a, admin_profile) + ) + + response = admin_client.post( + self.url, + {"org_id": str(org_b.id), "refresh": old_refresh}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + + follow_up = unauthenticated_client.post( + "/api/auth/refresh-token/", + {"refresh": response.data["refresh_token"]}, + format="json", + ) + assert follow_up.status_code == status.HTTP_200_OK + + def test_switch_org_without_refresh_token_still_succeeds( + self, admin_client, admin_user, org_b + ): + """Clients that omit `refresh` must keep working. + + Mobile builds already in the wild do not send it; failing their org + switch would be worse than letting one token expire on its own. + """ + Profile.objects.create(user=admin_user, org=org_b, role="USER", is_active=True) + response = admin_client.post( + self.url, {"org_id": str(org_b.id)}, format="json" + ) + assert response.status_code == status.HTTP_200_OK + + def test_switch_org_will_not_revoke_another_users_token( + self, admin_client, unauthenticated_client, admin_user, org_a, org_b + ): + """A caller must not be able to retire someone else's session. + + Without an ownership check, `refresh` becomes a way to forcibly log out + any user whose token you can observe. + """ + Profile.objects.create(user=admin_user, org=org_b, role="USER", is_active=True) + victim = User.objects.create(email="victim@example.com", password="unusable") + victim_profile = Profile.objects.create( + user=victim, org=org_a, role="USER", is_active=True + ) + victim_refresh = str( + OrgAwareRefreshToken.for_user_and_org(victim, org_a, victim_profile) + ) + + response = admin_client.post( + self.url, + {"org_id": str(org_b.id), "refresh": victim_refresh}, + format="json", + ) + assert response.status_code == status.HTTP_200_OK + + still_valid = unauthenticated_client.post( + "/api/auth/refresh-token/", {"refresh": victim_refresh}, format="json" + ) + assert still_valid.status_code == status.HTTP_200_OK + @pytest.mark.django_db class TestProfileDetailView: @@ -217,11 +376,15 @@ class TestProfileDetailView: # --------------------------------------------------------------------------- -def _make_fake_id_token(email, picture="https://photo.example.com/pic.jpg"): +def _make_fake_id_token( + email, picture="https://photo.example.com/pic.jpg", email_verified=True +): """Create a fake JWT-like ID token with the given email in the payload.""" header = base64.urlsafe_b64encode(json.dumps({"alg": "RS256"}).encode()).decode().rstrip("=") payload = base64.urlsafe_b64encode( - json.dumps({"email": email, "picture": picture}).encode() + json.dumps( + {"email": email, "picture": picture, "email_verified": email_verified} + ).encode() ).decode().rstrip("=") signature = base64.urlsafe_b64encode(b"fakesig").decode().rstrip("=") return f"{header}.{payload}.{signature}" @@ -410,6 +573,85 @@ class TestGoogleOAuthCallbackView: ) assert response.status_code == status.HTTP_400_BAD_REQUEST + def _post_code(self, client): + return client.post( + self.url, + { + "code": "authcode", + "code_verifier": "verifier", + "redirect_uri": "http://localhost:3000/callback", + }, + format="json", + ) + + @patch("common.views.auth_views.requests.post") + def test_rejects_unverified_email(self, mock_post, unauthenticated_client): + """An unverified Google email must not create or authenticate a user. + + Without this, a Google account holding an address the owner never + proved control of is enough to become that address in this system. + """ + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "id_token": _make_fake_id_token( + "unverified@example.com", email_verified=False + ), + "access_token": "at", + } + mock_post.return_value = mock_response + + response = self._post_code(unauthenticated_client) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not User.objects.filter(email="unverified@example.com").exists() + + @patch("common.views.auth_views.requests.post") + def test_rejects_missing_email_verified_claim( + self, mock_post, unauthenticated_client + ): + """A response with no email_verified claim must fail closed.""" + header = ( + base64.urlsafe_b64encode(json.dumps({"alg": "RS256"}).encode()) + .decode() + .rstrip("=") + ) + payload = ( + base64.urlsafe_b64encode(json.dumps({"email": "noclaim@example.com"}).encode()) + .decode() + .rstrip("=") + ) + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "id_token": f"{header}.{payload}.sig", + "access_token": "at", + } + mock_post.return_value = mock_response + + response = self._post_code(unauthenticated_client) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not User.objects.filter(email="noclaim@example.com").exists() + + @patch("common.views.auth_views.requests.post") + def test_rejects_deactivated_user(self, mock_post, unauthenticated_client, admin_user): + """A deactivated account must not be able to log back in via Google.""" + admin_user.is_active = False + admin_user.save(update_fields=["is_active"]) + mock_response = MagicMock() + mock_response.status_code = 200 + mock_response.json.return_value = { + "id_token": _make_fake_id_token(admin_user.email), + "access_token": "at", + } + mock_post.return_value = mock_response + + response = self._post_code(unauthenticated_client) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "access_token" not in response.data + # --------------------------------------------------------------------------- # Google ID Token View tests (lines 173-232) @@ -458,6 +700,7 @@ class TestGoogleIdTokenView: """Valid token with new email should create user and return JWT.""" mock_verify.return_value = { "email": "mobileuser@example.com", + "email_verified": True, "picture": "https://photo.example.com/pic.jpg", } response = unauthenticated_client.post( @@ -481,6 +724,7 @@ class TestGoogleIdTokenView: """Existing user should get their organizations in response.""" mock_verify.return_value = { "email": admin_user.email, + "email_verified": True, "picture": "https://photo.example.com/pic.jpg", } response = unauthenticated_client.post( @@ -490,6 +734,59 @@ class TestGoogleIdTokenView: assert len(response.data["organizations"]) == 1 assert response.data["organizations"][0]["id"] == str(org_a.id) + @patch("google.oauth2.id_token.verify_oauth2_token") + @patch("google.auth.transport.requests.Request") + def test_rejects_unverified_email( + self, mock_request_cls, mock_verify, unauthenticated_client + ): + """An unverified Google email must not authenticate on mobile either.""" + mock_verify.return_value = { + "email": "unverified@example.com", + "email_verified": False, + } + + response = unauthenticated_client.post( + self.url, {"idToken": "valid-token"}, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not User.objects.filter(email="unverified@example.com").exists() + + @patch("google.oauth2.id_token.verify_oauth2_token") + @patch("google.auth.transport.requests.Request") + def test_rejects_missing_email_verified_claim( + self, mock_request_cls, mock_verify, unauthenticated_client + ): + """A verified token with no email_verified claim must fail closed.""" + mock_verify.return_value = {"email": "noclaim@example.com"} + + response = unauthenticated_client.post( + self.url, {"idToken": "valid-token"}, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not User.objects.filter(email="noclaim@example.com").exists() + + @patch("google.oauth2.id_token.verify_oauth2_token") + @patch("google.auth.transport.requests.Request") + def test_rejects_deactivated_user( + self, mock_request_cls, mock_verify, unauthenticated_client, admin_user + ): + """A deactivated account must not be able to log back in on mobile.""" + admin_user.is_active = False + admin_user.save(update_fields=["is_active"]) + mock_verify.return_value = { + "email": admin_user.email, + "email_verified": True, + } + + response = unauthenticated_client.post( + self.url, {"idToken": "valid-token"}, format="json" + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "JWTtoken" not in response.data + # --------------------------------------------------------------------------- # Token refresh - User.DoesNotExist edge case (lines 503-504) @@ -515,3 +812,58 @@ class TestTokenRefreshUserNotFound: ) assert response.status_code == status.HTTP_401_UNAUTHORIZED assert "User not found" in response.data["error"] + + +@pytest.mark.django_db +class TestFlushExpiredRefreshTokens: + """Tests for common.tasks.flush_expired_refresh_tokens.""" + + def _outstanding(self, user, jti, expires_at): + from rest_framework_simplejwt.token_blacklist.models import OutstandingToken + + return OutstandingToken.objects.create( + user=user, + jti=jti, + token="not-a-real-token", + created_at=timezone.now() - timedelta(days=30), + expires_at=expires_at, + ) + + def test_deletes_expired_but_keeps_live_records(self, admin_user): + """Rotation bookkeeping must not grow without bound.""" + from rest_framework_simplejwt.token_blacklist.models import OutstandingToken + + from common.tasks import flush_expired_refresh_tokens + + now = timezone.now() + expired = self._outstanding(admin_user, "expired-jti", now - timedelta(days=1)) + live = self._outstanding(admin_user, "live-jti", now + timedelta(days=1)) + + assert flush_expired_refresh_tokens() == 1 + + assert not OutstandingToken.objects.filter(pk=expired.pk).exists() + assert OutstandingToken.objects.filter(pk=live.pk).exists() + + def test_expired_blacklist_entry_is_cascaded(self, admin_user): + """Deleting the outstanding row must take its blacklist entry with it.""" + from rest_framework_simplejwt.token_blacklist.models import ( + BlacklistedToken, + OutstandingToken, + ) + + from common.tasks import flush_expired_refresh_tokens + + expired = self._outstanding( + admin_user, "expired-jti", timezone.now() - timedelta(days=1) + ) + BlacklistedToken.objects.create(token=expired) + + assert flush_expired_refresh_tokens() == 1 + assert not BlacklistedToken.objects.exists() + assert not OutstandingToken.objects.exists() + + def test_returns_zero_when_nothing_expired(self, admin_user): + self._outstanding(admin_user, "live-jti", timezone.now() + timedelta(days=1)) + from common.tasks import flush_expired_refresh_tokens + + assert flush_expired_refresh_tokens() == 0 diff --git a/backend/common/tests/test_magic_link.py b/backend/common/tests/test_magic_link.py index a0686468..3eb2cbc5 100644 --- a/backend/common/tests/test_magic_link.py +++ b/backend/common/tests/test_magic_link.py @@ -204,6 +204,24 @@ class TestMagicLinkVerify: ) assert response2.status_code == status.HTTP_400_BAD_REQUEST + def test_verify_rejects_deactivated_user(self, unauthenticated_client, admin_user): + """A deactivated account must not be able to log itself back in. + + Deactivation is the offboarding lever; if a magic link still mints + tokens for `is_active=False`, revoking access does not actually revoke + access. + """ + admin_user.is_active = False + admin_user.save(update_fields=["is_active"]) + token_obj = self._create_valid_token(email=admin_user.email) + + response = unauthenticated_client.post( + self.url, {"token": token_obj.token}, format="json" + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "access_token" not in response.data + @pytest.mark.django_db class TestMagicLinkRequestCode: @@ -422,3 +440,20 @@ class TestMagicLinkVerifyCode: assert not MagicLinkToken.objects.filter( email="linktok@example.com", is_used=True ).exists() + + def test_verify_code_rejects_deactivated_user( + self, unauthenticated_client, admin_user + ): + """The OTP path must honour deactivation just like the link path.""" + admin_user.is_active = False + admin_user.save(update_fields=["is_active"]) + self._create_code_token(email=admin_user.email, code="222222") + + response = unauthenticated_client.post( + self.url, + {"email": admin_user.email, "code": "222222"}, + format="json", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert "access_token" not in response.data diff --git a/backend/common/tests/test_models.py b/backend/common/tests/test_models.py index 232cbc0f..7a4d148d 100644 --- a/backend/common/tests/test_models.py +++ b/backend/common/tests/test_models.py @@ -29,7 +29,6 @@ from common.models import ( Document, Org, Profile, - SessionToken, Teams, User, ) @@ -195,59 +194,6 @@ class TestDocumentFileType: assert doc.file_type() == ("file", "fa fa-file") -# --------------------------------------------------------------------------- -# SessionToken.revoke() and cleanup_expired() -# --------------------------------------------------------------------------- - - -@pytest.mark.django_db -class TestSessionToken: - """Test SessionToken.revoke() and cleanup_expired() methods.""" - - def _create_token(self, user, **kwargs): - defaults = { - "user": user, - "token_jti": str(uuid.uuid4()), - "refresh_token_jti": str(uuid.uuid4()), - "expires_at": timezone.now() + timezone.timedelta(hours=1), - "is_active": True, - } - defaults.update(kwargs) - return SessionToken.objects.create(**defaults) - - def test_revoke(self, admin_user): - token = self._create_token(admin_user) - assert token.is_active is True - assert token.revoked_at is None - - token.revoke() - token.refresh_from_db() - - assert token.is_active is False - assert token.revoked_at is not None - - def test_cleanup_expired(self, admin_user): - # Create an expired token - expired = self._create_token( - admin_user, - token_jti=str(uuid.uuid4()), - refresh_token_jti=str(uuid.uuid4()), - expires_at=timezone.now() - timezone.timedelta(hours=1), - ) - # Create a valid token - valid = self._create_token( - admin_user, - token_jti=str(uuid.uuid4()), - refresh_token_jti=str(uuid.uuid4()), - expires_at=timezone.now() + timezone.timedelta(hours=1), - ) - - deleted_count, _ = SessionToken.cleanup_expired() - assert deleted_count == 1 - assert not SessionToken.objects.filter(pk=expired.pk).exists() - assert SessionToken.objects.filter(pk=valid.pk).exists() - - # --------------------------------------------------------------------------- # Activity.created_on_arrow # --------------------------------------------------------------------------- diff --git a/backend/common/tests/test_multitenancy.py b/backend/common/tests/test_multitenancy.py index 1c1ff080..4a057000 100644 --- a/backend/common/tests/test_multitenancy.py +++ b/backend/common/tests/test_multitenancy.py @@ -658,27 +658,50 @@ class TestPermissionClasses(MultiTenancyBaseTestCase): perm = IsOrgAdmin() self.assertFalse(perm.has_permission(request, None)) - def test_is_super_admin_with_super_email(self): - """IsSuperAdmin allows micropyramid.com email.""" + def test_is_super_admin_with_explicit_flag(self): + """IsSuperAdmin allows a user explicitly flagged is_superuser.""" from common.permissions import IsSuperAdmin - user = MagicMock() - user.is_authenticated = True - user.email = "admin@micropyramid.com" + self.user_a.is_superuser = True request = MagicMock() - request.user = user + request.user = self.user_a perm = IsSuperAdmin() self.assertTrue(perm.has_permission(request, None)) - def test_is_super_admin_regular_email(self): - """IsSuperAdmin denies non-micropyramid emails.""" + def test_is_super_admin_ignores_email_domain(self): + """An email domain must NEVER confer super admin. + + Granting platform-wide super admin to anyone whose email ends in a + given domain is a privilege-escalation hole: the domain is attacker + controlled input as far as the permission check is concerned. + """ from common.permissions import IsSuperAdmin - user = MagicMock() - user.is_authenticated = True - user.email = "admin@test.com" + self.user_a.email = "attacker@micropyramid.com" + self.user_a.is_superuser = False request = MagicMock() - request.user = user + request.user = self.user_a + perm = IsSuperAdmin() + self.assertFalse(perm.has_permission(request, None)) + + def test_is_super_admin_regular_user(self): + """IsSuperAdmin denies a user without the flag.""" + from common.permissions import IsSuperAdmin + + self.user_a.is_superuser = False + request = MagicMock() + request.user = self.user_a + perm = IsSuperAdmin() + self.assertFalse(perm.has_permission(request, None)) + + def test_is_super_admin_inactive_user(self): + """IsSuperAdmin denies a deactivated super admin.""" + from common.permissions import IsSuperAdmin + + self.user_a.is_superuser = True + self.user_a.is_active = False + request = MagicMock() + request.user = self.user_a perm = IsSuperAdmin() self.assertFalse(perm.has_permission(request, None)) diff --git a/backend/common/views/auth_views.py b/backend/common/views/auth_views.py index 1ed95bf5..df865bf6 100644 --- a/backend/common/views/auth_views.py +++ b/backend/common/views/auth_views.py @@ -22,6 +22,33 @@ from common.serializer import OrgAwareRefreshToken logger = logging.getLogger(__name__) +def _google_email_is_verified(claims): + """Whether Google asserts the caller controls the email in ``claims``. + + Fails closed: a missing ``email_verified`` claim counts as unverified. + Google always sends it alongside the ``email`` scope, so its absence means + the payload is not the shape we think it is. Without this check, a Google + account carrying an address whose owner never proved control of it is enough + to become that address here — which matters because other parts of the + system key off the email string. + """ + verified = claims.get("email_verified") + return verified is True or str(verified).strip().lower() == "true" + + +def _disabled_account_response(): + """403 for a login attempt by a deactivated account. + + Deactivation is the offboarding lever, so every login path must honour it — + otherwise a revoked user simply signs in again. Kept identical across the + Google and magic-link flows so clients can handle one shape. + """ + return Response( + {"error": "User account is disabled"}, + status=status.HTTP_403_FORBIDDEN, + ) + + class GoogleOAuthCallbackView(APIView): """ Handle Google OAuth authorization code exchange with PKCE. @@ -122,6 +149,14 @@ class GoogleOAuthCallbackView(APIView): status=status.HTTP_400_BAD_REQUEST, ) + # Reject before touching the database so an unverified address cannot + # even provision an account. + if not _google_email_is_verified(payload): + return Response( + {"error": "Google account email is not verified"}, + status=status.HTTP_400_BAD_REQUEST, + ) + # Get or create user created = False try: @@ -135,6 +170,9 @@ class GoogleOAuthCallbackView(APIView): ) created = True + if not user.is_active: + return _disabled_account_response() + # Backfill name from Google when the user hasn't set one yet. if not user.name and google_name: user.name = google_name @@ -223,6 +261,14 @@ class GoogleIdTokenView(APIView): status=status.HTTP_400_BAD_REQUEST, ) + # Reject before touching the database so an unverified address cannot + # even provision an account. + if not _google_email_is_verified(idinfo): + return Response( + {"error": "Google account email is not verified"}, + status=status.HTTP_400_BAD_REQUEST, + ) + # Get or create user user, _created = User.objects.get_or_create( email=email, @@ -232,6 +278,10 @@ class GoogleIdTokenView(APIView): "password": make_password(secrets.token_urlsafe(32)), }, ) + + if not user.is_active: + return _disabled_account_response() + # Backfill name from Google for existing users who don't have one. if not user.name and google_name: user.name = google_name @@ -318,6 +368,8 @@ class OrgAwareTokenRefreshView(APIView): }, ) def post(self, request): + from django.db import transaction + from rest_framework_simplejwt.exceptions import TokenError from rest_framework_simplejwt.tokens import RefreshToken as BaseRefreshToken @@ -347,6 +399,8 @@ class OrgAwareTokenRefreshView(APIView): ) # If token has org context, validate membership + org = None + profile = None if org_id: try: profile = Profile.objects.get( @@ -365,13 +419,18 @@ class OrgAwareTokenRefreshView(APIView): status=status.HTTP_403_FORBIDDEN, ) - # Generate new tokens with same org context (include profile for role) + # Rotate: a refresh token is single-use. Blacklisting the presented + # token means a copy stolen via XSS, a proxy, or a log cannot keep + # minting access tokens once the legitimate client has refreshed. + # Both writes share one transaction so a failure can never leave the + # caller tokenless while the old token stays usable. + # `profile` is None when there is no org context, which is exactly + # what for_user_and_org expects. + with transaction.atomic(): + token.blacklist() new_token = OrgAwareRefreshToken.for_user_and_org(user, org, profile) - audit_log.token_refresh(user, org, request) - else: - # No org context - refresh with user info only - new_token = OrgAwareRefreshToken.for_user_and_org(user, None) - audit_log.token_refresh(user, None, request) + + audit_log.token_refresh(user, org, request) return Response( {"access": str(new_token.access_token), "refresh": str(new_token)}, @@ -400,12 +459,52 @@ class OrgSwitchView(APIView): authentication_classes = [JWTAuthentication] permission_classes = [IsAuthenticated] + def _retire_presented_refresh_token(self, request): + """Blacklist the refresh token the caller is switching away from. + + Optional by design: clients already in the wild do not send ``refresh``, + and failing their org switch would be worse than letting one token expire + on its own schedule. When it *is* sent, the old token stops working + immediately instead of staying valid against the previous org for the + rest of its 14-day life. + + The token must belong to the authenticated caller. Without that check + this field would let anyone retire any refresh token they can observe, + turning a convenience parameter into a remote logout for other users. + """ + from rest_framework_simplejwt.exceptions import TokenError + from rest_framework_simplejwt.tokens import RefreshToken as BaseRefreshToken + + presented = request.data.get("refresh") + if not presented: + return + + try: + token = BaseRefreshToken(presented) + except TokenError: + # Expired, malformed, or already blacklisted: nothing left to retire, + # and the switch itself does not depend on it. + return + + if str(token.get("user_id")) != str(request.user.id): + return + + token.blacklist() + @extend_schema( description="Switch to a different organization and get new JWT tokens", request=inline_serializer( name="OrgSwitchRequest", fields={ - "org_id": serializers.UUIDField(help_text="Target organization ID") + "org_id": serializers.UUIDField(help_text="Target organization ID"), + "refresh": serializers.CharField( + required=False, + help_text=( + "Optional. The refresh token being replaced; it is " + "blacklisted so it cannot be reused after the switch. " + "Ignored unless it belongs to the caller." + ), + ), }, ), responses={ @@ -421,6 +520,8 @@ class OrgSwitchView(APIView): }, ) def post(self, request): + from django.db import transaction + from common.audit_log import audit_log org_id = request.data.get("org_id") @@ -455,10 +556,14 @@ class OrgSwitchView(APIView): status=status.HTTP_403_FORBIDDEN, ) - # Generate new tokens with the target org (include profile for role) - token = OrgAwareRefreshToken.for_user_and_org( - request.user, profile.org, profile - ) + # Retire the outgoing refresh token and issue its replacement together, + # so a failure cannot leave the caller without a usable token while the + # old one is already dead. + with transaction.atomic(): + self._retire_presented_refresh_token(request) + token = OrgAwareRefreshToken.for_user_and_org( + request.user, profile.org, profile + ) # Audit log the org switch audit_log.org_switch(request.user, from_org, profile.org, request) @@ -619,6 +724,9 @@ class MagicLinkVerifyView(APIView): ) created = True + if not user.is_active: + return _disabled_account_response() + if created: from common.tasks import send_welcome_email @@ -752,6 +860,9 @@ class MagicLinkVerifyCodeView(APIView): ) created = True + if not user.is_active: + return _disabled_account_response() + if created: from common.tasks import send_welcome_email diff --git a/backend/crm/celery.py b/backend/crm/celery.py index 8a8870d1..aa35493f 100644 --- a/backend/crm/celery.py +++ b/backend/crm/celery.py @@ -69,4 +69,9 @@ app.conf.beat_schedule = { "task": "cases.tasks.auto_stop_stale_timers", "schedule": crontab(minute="*/30"), }, + # Drop rotated/expired refresh token records - daily at 3:30 AM + "flush-expired-refresh-tokens": { + "task": "common.tasks.flush_expired_refresh_tokens", + "schedule": crontab(hour=3, minute=30), + }, } diff --git a/backend/crm/settings.py b/backend/crm/settings.py index db2a9e9f..0ffd25fb 100644 --- a/backend/crm/settings.py +++ b/backend/crm/settings.py @@ -31,6 +31,10 @@ INSTALLED_APPS = [ "django.contrib.staticfiles", "rest_framework", "rest_framework_simplejwt", + # Required for refresh-token rotation: simplejwt only defines + # RefreshToken.blacklist()/check_blacklist() when this app is installed, so + # without it BLACKLIST_AFTER_ROTATION below is silently a no-op. + "rest_framework_simplejwt.token_blacklist", "corsheaders", "django_ses", "drf_spectacular", diff --git a/frontend/src/hooks.server.js b/frontend/src/hooks.server.js index 27fd86f0..3bc8a0a7 100644 --- a/frontend/src/hooks.server.js +++ b/frontend/src/hooks.server.js @@ -75,17 +75,60 @@ function verifyTokenLocally(accessToken) { } /** - * Refresh access token using refresh token - * @param {string} refreshToken - JWT refresh token - * @returns {Promise} New access token or null if refresh failed + * Refreshes currently in flight, keyed by the refresh token being spent. + * Entries are removed as soon as the request settles, so this holds at most one + * entry per user mid-refresh. + * @type {Map>} */ -async function refreshAccessToken(refreshToken) { +const refreshesInFlight = new Map(); + +/** + * Refresh access token using refresh token. + * + * The backend rotates refresh tokens: the one we send is blacklisted server-side + * and a replacement comes back in `refresh`. Callers MUST persist that + * replacement — reusing the old token on the next refresh gets a 401 and logs + * the user out. + * + * Concurrent requests arriving with the same expired access token would + * otherwise each spend the same refresh token, and every one but the winner + * would 401. Requests sharing a refresh token therefore share one round-trip. + * + * @param {string} refreshToken - JWT refresh token + * @returns {Promise<{access: string, refresh?: string}|null>} New tokens or null if refresh failed + */ +function refreshAccessToken(refreshToken) { + const existing = refreshesInFlight.get(refreshToken); + if (existing) { + return existing; + } + + const pending = performTokenRefresh(refreshToken).finally(() => { + refreshesInFlight.delete(refreshToken); + }); + refreshesInFlight.set(refreshToken, pending); + + return pending; +} + +/** + * Perform the actual refresh round-trip. Use refreshAccessToken() instead — + * it deduplicates concurrent callers. + * + * @param {string} refreshToken - JWT refresh token + * @returns {Promise<{access: string, refresh?: string}|null>} New tokens or null if refresh failed + */ +async function performTokenRefresh(refreshToken) { try { const response = await axios.post(`${API_BASE_URL}/auth/refresh-token/`, { refresh: refreshToken }); - return response.data.access; + if (!response.data?.access) { + return null; + } + + return { access: response.data.access, refresh: response.data.refresh }; } catch (error) { console.error('Token refresh failed:', error); return null; @@ -95,16 +138,22 @@ async function refreshAccessToken(refreshToken) { // Profile info is now embedded in JWT - no API call needed /** - * Switch organization and get new tokens with org context + * Switch organization and get new tokens with org context. + * + * Passes the outgoing refresh token so the backend can blacklist it — otherwise + * it stays valid against the previous org for the rest of its lifetime even + * though we replace it in the cookie below. + * * @param {string} accessToken - Current JWT access token * @param {string} orgId - Organization UUID to switch to + * @param {string} [refreshToken] - Refresh token being replaced, to be retired * @returns {Promise} New tokens and org data or null if failed */ -async function switchOrg(accessToken, orgId) { +async function switchOrg(accessToken, orgId, refreshToken) { try { const response = await axios.post( `${API_BASE_URL}/auth/switch-org/`, - { org_id: orgId }, + refreshToken ? { org_id: orgId, refresh: refreshToken } : { org_id: orgId }, { headers: { Authorization: `Bearer ${accessToken}`, @@ -126,7 +175,9 @@ export const handle = sequence(Sentry.sentryHandle(), async function _handle({ e // Get tokens from cookies /** @type {string | undefined} */ let accessToken = event.cookies.get('jwt_access'); - const refreshToken = event.cookies.get('jwt_refresh'); + // Reassigned if we rotate below, so later calls hand on the live token rather + // than the one we just spent. + let refreshToken = event.cookies.get('jwt_refresh'); const orgId = event.cookies.get('org'); /** @type {JWTPayload | null} */ @@ -138,18 +189,31 @@ export const handle = sequence(Sentry.sentryHandle(), async function _handle({ e // If access token expired, try to refresh if (!jwtPayload && refreshToken) { - const newAccessToken = await refreshAccessToken(refreshToken); - if (newAccessToken) { + const refreshed = await refreshAccessToken(refreshToken); + if (refreshed) { // Update cookie with new access token - event.cookies.set('jwt_access', newAccessToken, { + event.cookies.set('jwt_access', refreshed.access, { path: '/', httpOnly: true, sameSite: 'lax', secure: process.env.NODE_ENV === 'production', maxAge: 60 * 60 * 24 // 1 day }); - accessToken = newAccessToken; - jwtPayload = verifyTokenLocally(newAccessToken); + // Persist the rotated refresh token. The one we just spent is + // blacklisted server-side, so keeping it would 401 the next refresh + // and bounce the user to /login an hour later. + if (refreshed.refresh) { + event.cookies.set('jwt_refresh', refreshed.refresh, { + path: '/', + httpOnly: true, + sameSite: 'lax', + secure: process.env.NODE_ENV === 'production', + maxAge: 60 * 60 * 24 * 365 // 1 year + }); + refreshToken = refreshed.refresh; + } + accessToken = refreshed.access; + jwtPayload = verifyTokenLocally(refreshed.access); } else { // Refresh failed, clear cookies event.cookies.delete('jwt_access', { path: '/' }); @@ -195,7 +259,7 @@ export const handle = sequence(Sentry.sentryHandle(), async function _handle({ e }; } else { // Token doesn't have org context, need to switch (1 API call) - const switchResult = await switchOrg(token, orgId); + const switchResult = await switchOrg(token, orgId, refreshToken); if (switchResult) { // Update cookies with new tokens that have org context diff --git a/frontend/src/lib/api.js b/frontend/src/lib/api.js index e8eb3b05..dcaa9a04 100644 --- a/frontend/src/lib/api.js +++ b/frontend/src/lib/api.js @@ -111,10 +111,39 @@ export function getCurrentUser() { } /** - * Refresh the access token using refresh token + * In-flight refresh shared by concurrent callers, or null when idle. + * @type {Promise|null} + */ +let refreshInFlight = null; + +/** + * Refresh the access token using refresh token. + * + * The backend rotates refresh tokens — the token we send is blacklisted and a + * replacement comes back — so concurrent callers must not each spend the same + * one. Whichever request landed second would get a 401 and log the user out, + * so all callers share a single in-flight refresh. + * * @returns {Promise} New access token or null if refresh failed */ async function refreshAccessToken() { + if (refreshInFlight) { + return refreshInFlight; + } + + refreshInFlight = performTokenRefresh().finally(() => { + refreshInFlight = null; + }); + + return refreshInFlight; +} + +/** + * Perform the actual refresh round-trip. Use refreshAccessToken() instead — + * it deduplicates concurrent callers. + * @returns {Promise} New access token or null if refresh failed + */ +async function performTokenRefresh() { const refreshToken = getRefreshToken(); if (!refreshToken) { return null; @@ -131,7 +160,16 @@ async function refreshAccessToken() { if (response.ok) { const data = await response.json(); + if (!data.access) { + clearAuthData(); + return null; + } setInStorage(STORAGE_KEYS.ACCESS_TOKEN, data.access); + // Persist the rotated refresh token. The one we just spent is + // blacklisted server-side, so keeping it would 401 the next refresh. + if (data.refresh) { + setInStorage(STORAGE_KEYS.REFRESH_TOKEN, data.refresh); + } return data.access; } @@ -232,10 +270,16 @@ export const auth = { if (!orgId || !UUID_RE.test(orgId)) { throw new Error('Invalid organization ID format'); } + // Hand over the outgoing refresh token so the backend retires it; we replace + // it below either way, and leaving it live keeps a stolen copy working + // against the previous org. + const outgoingRefresh = getRefreshToken(); /** @type {any} */ const data = await apiRequest('/auth/switch-org/', { method: 'POST', - body: { org_id: orgId } + body: outgoingRefresh + ? { org_id: orgId, refresh: outgoingRefresh } + : { org_id: orgId } }); // Update tokens with new org context diff --git a/frontend/src/routes/(no-layout)/org/+page.server.js b/frontend/src/routes/(no-layout)/org/+page.server.js index 8fd70a9a..aa12a2c1 100644 --- a/frontend/src/routes/(no-layout)/org/+page.server.js +++ b/frontend/src/routes/(no-layout)/org/+page.server.js @@ -77,12 +77,15 @@ export const actions = { } const apiUrl = publicEnv.PUBLIC_DJANGO_API_URL; + // Sent so the backend retires the token we are about to replace below; + // otherwise it stays usable against the previous org until it expires. + const outgoingRefresh = cookies.get('jwt_refresh'); try { // Call switch-org endpoint to get new tokens with org context const response = await axios.post( `${apiUrl}/api/auth/switch-org/`, - { org_id: orgId }, + outgoingRefresh ? { org_id: orgId, refresh: outgoingRefresh } : { org_id: orgId }, { headers: { Authorization: `Bearer ${jwtAccess}`, diff --git a/mobile/lib/services/auth_service.dart b/mobile/lib/services/auth_service.dart index 9775816f..4251b371 100644 --- a/mobile/lib/services/auth_service.dart +++ b/mobile/lib/services/auth_service.dart @@ -346,9 +346,13 @@ class AuthService { try { debugPrint('AuthService: Switching to organization: ${org.name}...'); - // Call switch-org API to get new tokens with org context + // Call switch-org API to get new tokens with org context. The outgoing + // refresh token is sent so the backend blacklists it — we replace it below + // either way, and leaving it live keeps a stolen copy working against the + // previous org until it expires. final response = await _apiService.post(ApiConfig.switchOrg, { 'org_id': org.id, + if (_refreshToken != null) 'refresh': _refreshToken, }, requiresAuth: true); if (!response.success || response.data == null) { @@ -356,9 +360,14 @@ class AuthService { return false; } - // Update tokens from response + // Update tokens from response. The backend has now blacklisted the token + // we sent, so a malformed reply must not clear the field — mirror the + // guard in refreshAccessToken rather than stranding the session. _accessToken = response.data!['access_token'] as String?; - _refreshToken = response.data!['refresh_token'] as String?; + final newRefresh = response.data!['refresh_token'] as String?; + if (newRefresh != null) { + _refreshToken = newRefresh; + } // Update selected organization _selectedOrganization = org;