From 9953c9d61fd94d5695a0dbebb75c916187461ca4 Mon Sep 17 00:00:00 2001 From: Atakan Seckin Date: Sat, 5 Sep 2026 10:00:58 +0200 Subject: [PATCH 1/2] Count a registration however the account was made The register event had one call site, the password form, so the metric never meant what its name says. Magic-link signups went uncounted, and Google signups never counted at all - that callback carried no analytics whatever, from long before the email-capture work. Both new paths need the same thing the password form gets for free: a way to tell a signup from a sign-in, because unlike the signup endpoint they also serve returning users. The email-link account exists before the link is ever opened, so the verify call reports whether it was the one that completed the signup - first activation, and this flow was what created the account. That second half matters: a password signup still waiting on its confirmation email can be activated by a link too, and it was already counted when the form was submitted. Google needs nothing computed. social_core already stamps is_new on the user its pipeline returns, from our own associate_by_email and create_user steps; the response just never carried it. Each event now names the route that produced it, the password form included - otherwise the one path that was always counted would be the one that cannot be identified. Note this redefines an existing metric rather than adding one: the register count steps up when this ships, and comparisons across that point are not like for like. Co-Authored-By: Claude Opus 5 --- authentication/services/email_link.py | 20 +++++++++++--- authentication/views/email_link.py | 16 +++++++++--- authentication/views/social.py | 7 +++++ .../email/components/email_link_verify.tsx | 12 +++++++++ front_end/src/app/(main)/accounts/actions.ts | 6 +++-- .../accounts/social/[provider]/actions.ts | 2 ++ .../accounts/social/[provider]/client.tsx | 13 +++++++++- front_end/src/components/auth/signup.tsx | 2 ++ .../src/services/api/auth/auth.server.ts | 6 ++++- front_end/src/types/auth.ts | 7 +++++ front_end/src/utils/signup_methods.ts | 7 +++++ tests/unit/test_auth/test_email_link.py | 26 +++++++++++++++++++ tests/unit/test_auth/test_social_pipeline.py | 22 ++++++++++++++++ 13 files changed, 136 insertions(+), 10 deletions(-) create mode 100644 front_end/src/utils/signup_methods.ts diff --git a/authentication/services/email_link.py b/authentication/services/email_link.py index 1480aea74e..c9d19ff3c7 100644 --- a/authentication/services/email_link.py +++ b/authentication/services/email_link.py @@ -27,11 +27,15 @@ def token_timeout(self) -> int: email_link_token_generator = EmailLinkTokenGenerator() +# Recorded in User.metadata when this flow creates the account +SIGNUP_METHOD_EMAIL_LINK = "email_link" -def verify_email_link_auth(user_id: int, token: str) -> User: + +def verify_email_link_auth(user_id: int, token: str) -> tuple[User, bool]: """ Validates an email-link token, activates the user when applicable and - returns them. One generic error for every failure mode (anti-enumeration). + returns them along with whether this call completed their signup. One + generic error for every failure mode (anti-enumeration). """ user = User.objects.select_for_update().filter(pk=user_id).first() @@ -44,11 +48,21 @@ def verify_email_link_auth(user_id: int, token: str) -> User: logger.info(f"email_link verify rejected: user_id={user_id}") raise ValidationError({"token": ["Link is invalid or expired"]}) + # Read before activating: check_can_activate goes false the moment we do, + # and get_tokens_for_user stamps last_login straight after. Also require + # that this flow created the account - a signup-form account still waiting + # on its confirmation email can be activated by a link too, and that signup + # was already counted when the form was submitted. + is_new = user.check_can_activate() and ( + (user.metadata or {}).get("signup_details", {}).get("method") + == SIGNUP_METHOD_EMAIL_LINK + ) + if user.check_can_activate(): user.is_active = True user.save(update_fields=["is_active"]) - return user + return user, is_new def send_email_link_auth_email(user: User, redirect_url: str | None) -> None: diff --git a/authentication/views/email_link.py b/authentication/views/email_link.py index a196f415cd..ac4957bceb 100644 --- a/authentication/views/email_link.py +++ b/authentication/views/email_link.py @@ -9,6 +9,7 @@ from authentication.serializers import ConfirmationTokenSerializer from authentication.services.common import get_tokens_for_user from authentication.services.email_link import ( + SIGNUP_METHOD_EMAIL_LINK, send_email_link_auth_email, verify_email_link_auth, ) @@ -64,7 +65,10 @@ def email_link_request_api_view(request): # while signed out, rather than switching layouts under them. interface_type=User.InterfaceType.CONSUMER_VIEW, metadata={ - "signup_details": {"method": "email_link", "action_type": action_type} + "signup_details": { + "method": SIGNUP_METHOD_EMAIL_LINK, + "action_type": action_type, + } }, ) @@ -95,9 +99,15 @@ def email_link_verify_api_view(request): # One transaction so the token check and the last_login write that consumes it # cannot interleave with a concurrent request holding the same token. with transaction.atomic(): - user = verify_email_link_auth(**serializer.validated_data) + user, is_new = verify_email_link_auth(**serializer.validated_data) tokens = get_tokens_for_user(user) apply_pending_action(user) - return Response({"tokens": tokens, "user": UserPrivateSerializer(user).data}) + return Response( + { + "tokens": tokens, + "user": UserPrivateSerializer(user).data, + "is_new": is_new, + } + ) diff --git a/authentication/views/social.py b/authentication/views/social.py index 25e408c062..85c02bad48 100644 --- a/authentication/views/social.py +++ b/authentication/views/social.py @@ -48,10 +48,17 @@ def social_providers_api_view(request): class SocialCodeAuth(SocialTokenOnlyAuthView): class TokenSerializer(serializers.Serializer): tokens = serializers.SerializerMethodField() + is_new = serializers.SerializerMethodField() def get_tokens(self, obj: User): return get_tokens_for_user(obj) + def get_is_new(self, obj: User) -> bool: + # social_core stamps this on the user the pipeline returns, from + # whichever of our steps claimed or created the account. In memory + # only, so it is true just on the request that created them. + return getattr(obj, "is_new", False) + serializer_class = TokenSerializer authentication_classes = (JWTAuthentication,) diff --git a/front_end/src/app/(auth-flow)/auth/email/components/email_link_verify.tsx b/front_end/src/app/(auth-flow)/auth/email/components/email_link_verify.tsx index 7e2a7ee251..a55eab27ba 100644 --- a/front_end/src/app/(auth-flow)/auth/email/components/email_link_verify.tsx +++ b/front_end/src/app/(auth-flow)/auth/email/components/email_link_verify.tsx @@ -21,6 +21,7 @@ import { sendAnalyticsEvent } from "@/utils/analytics"; import cn from "@/utils/core/cn"; import { withConfirmedEvent } from "@/utils/email_link_confirmation"; import { ensureRelativeRedirect } from "@/utils/navigation"; +import { SIGNUP_METHOD_EMAIL_LINK } from "@/utils/signup_methods"; type Props = { userId: string; @@ -113,6 +114,17 @@ const EmailLinkVerify: FC = ({ userId, token, redirectUrl }) => { sameDevice: !!pending, }); + // The account is created when the address is submitted, so only the + // verification that completes the signup counts - a returning user + // asking for a sign-in link arrives here too. + if (result.isNew) { + sendAnalyticsEvent("register", { + method: SIGNUP_METHOD_EMAIL_LINK, + trigger: appliedTrigger, + surface: pending?.surface, + }); + } + // Arriving by magic link means exploring, not enrolling: skip the // forecaster tutorial for good rather than interrupting the action the // user actually came to complete. Persisted so it holds on every device. diff --git a/front_end/src/app/(main)/accounts/actions.ts b/front_end/src/app/(main)/accounts/actions.ts index d226847629..68f0e2a76c 100644 --- a/front_end/src/app/(main)/accounts/actions.ts +++ b/front_end/src/app/(main)/accounts/actions.ts @@ -263,7 +263,9 @@ export async function requestEmailLinkAction(params: { export async function verifyEmailLinkAction( userId: string, token: string -): Promise<{ user: CurrentUser } | { errors: ApiErrorPayload }> { +): Promise< + { user: CurrentUser; isNew: boolean } | { errors: ApiErrorPayload } +> { try { const response = await ServerAuthApi.verifyEmailLink(userId, token); @@ -274,7 +276,7 @@ export async function verifyEmailLinkAction( await LanguageService.setLocaleCookie(response.user.language); } - return { user: response.user }; + return { user: response.user, isNew: response.is_new }; } catch (err: unknown) { return { errors: ApiError.isApiError(err) diff --git a/front_end/src/app/(main)/accounts/social/[provider]/actions.ts b/front_end/src/app/(main)/accounts/social/[provider]/actions.ts index e1b01ab9f7..2e3e4a303b 100644 --- a/front_end/src/app/(main)/accounts/social/[provider]/actions.ts +++ b/front_end/src/app/(main)/accounts/social/[provider]/actions.ts @@ -36,4 +36,6 @@ export async function exchangeSocialOauthCode( const authManager = await getAuthCookieManager(); authManager.setAuthTokens(response.tokens); } + + return { isNew: !!response?.is_new }; } diff --git a/front_end/src/app/(main)/accounts/social/[provider]/client.tsx b/front_end/src/app/(main)/accounts/social/[provider]/client.tsx index 4d970ee715..dd0b8217cc 100644 --- a/front_end/src/app/(main)/accounts/social/[provider]/client.tsx +++ b/front_end/src/app/(main)/accounts/social/[provider]/client.tsx @@ -12,9 +12,11 @@ import { } from "@/components/email_capture/pending_store"; import LoadingIndicator from "@/components/ui/loading_indicator"; import { SocialProviderType } from "@/types/auth"; +import { sendAnalyticsEvent } from "@/utils/analytics"; import { rotateCsrfToken } from "@/utils/csrf"; import { withConfirmedEvent } from "@/utils/email_link_confirmation"; import { EMAIL_CAPTURE_SIGNUP_SOURCE } from "@/utils/gated_actions"; +import { SIGNUP_METHOD_GOOGLE } from "@/utils/signup_methods"; type Props = { provider: SocialProviderType; @@ -38,7 +40,7 @@ const SocialAuthClient: FC = ({ const stash = takeSocialGatedAction(); void (async () => { try { - await exchangeSocialOauthCode( + const { isNew } = await exchangeSocialOauthCode( provider, code, nonce, @@ -47,6 +49,15 @@ const SocialAuthClient: FC = ({ // decides whether a brand-new account starts in the consumer view stash ? EMAIL_CAPTURE_SIGNUP_SOURCE : null ); + // Only the exchange that created the account is a registration; an + // existing user signing in with Google lands here too. + if (isNew) { + sendAnalyticsEvent("register", { + method: SIGNUP_METHOD_GOOGLE, + fromEmailCapture: !!stash, + }); + } + // Invalidate the nonce now that it has served its purpose (and been // logged as a `state` param) — bounds any replay to the flow duration. rotateCsrfToken(); diff --git a/front_end/src/components/auth/signup.tsx b/front_end/src/components/auth/signup.tsx index 70c494d69b..6ad5c81000 100644 --- a/front_end/src/components/auth/signup.tsx +++ b/front_end/src/components/auth/signup.tsx @@ -28,6 +28,7 @@ import { useServerAction } from "@/hooks/use_server_action"; import { AppTheme } from "@/types/theme"; import { CurrentUser } from "@/types/users"; import { sendAnalyticsEvent } from "@/utils/analytics"; +import { SIGNUP_METHOD_PASSWORD } from "@/utils/signup_methods"; import usePostLoginActionHandler from "./hooks/usePostLoginActionHandler"; @@ -109,6 +110,7 @@ export const SignupForm: FC<{ turnstileRef.current?.reset(); } else { sendAnalyticsEvent("register", { + method: SIGNUP_METHOD_PASSWORD, event_category: new URLSearchParams(window.location.search).toString(), signupPath: redirectLocation, }); diff --git a/front_end/src/services/api/auth/auth.server.ts b/front_end/src/services/api/auth/auth.server.ts index 38d7a9ef28..f91d8d4f45 100644 --- a/front_end/src/services/api/auth/auth.server.ts +++ b/front_end/src/services/api/auth/auth.server.ts @@ -3,6 +3,7 @@ import "server-only"; import { ApiService } from "@/services/api/api_service"; import { AuthResponse, + EmailLinkVerifyResponse, AuthTokens, SignUpResponse, SocialAuthResponse, @@ -138,7 +139,10 @@ class ServerAuthApiClass extends ApiService { } async verifyEmailLink(userId: string, token: string) { - return this.post( + return this.post< + EmailLinkVerifyResponse, + { user_id: string; token: string } + >( "/auth/email-link/verify/", { user_id: userId, token }, {}, diff --git a/front_end/src/types/auth.ts b/front_end/src/types/auth.ts index 03a74f5870..6261c3135b 100644 --- a/front_end/src/types/auth.ts +++ b/front_end/src/types/auth.ts @@ -12,6 +12,8 @@ export type AuthTokens = { export type SocialAuthResponse = { tokens: AuthTokens; + /** True only on the request that created the account. */ + is_new: boolean; }; export type SocialProviderType = "facebook" | "google-oauth2"; @@ -26,6 +28,11 @@ export type AuthResponse = { user: CurrentUser; }; +export type EmailLinkVerifyResponse = AuthResponse & { + /** True only when this call completed a signup rather than a sign-in. */ + is_new: boolean; +}; + export type SignUpResponse = { tokens: AuthTokens | null; user: CurrentUser; diff --git a/front_end/src/utils/signup_methods.ts b/front_end/src/utils/signup_methods.ts new file mode 100644 index 0000000000..66404c2e92 --- /dev/null +++ b/front_end/src/utils/signup_methods.ts @@ -0,0 +1,7 @@ +/** + * How an account came into existence, sent as the `method` property on the + * `register` analytics event so registrations can be split by route. + */ +export const SIGNUP_METHOD_EMAIL_LINK = "email_link"; +export const SIGNUP_METHOD_PASSWORD = "password"; +export const SIGNUP_METHOD_GOOGLE = "google-oauth2"; diff --git a/tests/unit/test_auth/test_email_link.py b/tests/unit/test_auth/test_email_link.py index bcff16d41d..d2d7b5556c 100644 --- a/tests/unit/test_auth/test_email_link.py +++ b/tests/unit/test_auth/test_email_link.py @@ -184,6 +184,7 @@ def test_new_user_journey_vote_applied(self, anon_client, user1, mocker): assert response.data["tokens"]["access"] assert response.data["tokens"]["refresh"] assert response.data["user"]["id"] == user.id + assert response.data["is_new"] is True assert "action_result" not in response.data user.refresh_from_db() assert user.is_active @@ -200,9 +201,34 @@ def test_existing_active_user_signs_in(self, anon_client, user1, mocker): ) assert response.status_code == 200 + assert response.data["is_new"] is False user1.refresh_from_db() assert user1.last_login + def test_unconfirmed_signup_activated_by_link_is_not_new(self, anon_client, mocker): + """ + A signup-form account still waiting on its confirmation email can be + activated by a link, but that signup was already counted when the form + was submitted - counting it again here would double it. + """ + mocker.patch("authentication.views.email_link.send_email_link_auth_email") + user = User.objects.create_user( + username="formsignup", + email="formsignup@example.com", + password="pw", + is_active=False, + ) + token = email_link_token_generator.make_token(user) + + response = anon_client.post( + self.url, {"user_id": user.id, "token": token}, format="json" + ) + + assert response.status_code == 200 + assert response.data["is_new"] is False + user.refresh_from_db() + assert user.is_active + def test_single_use(self, anon_client, user1): token = email_link_token_generator.make_token(user1) diff --git a/tests/unit/test_auth/test_social_pipeline.py b/tests/unit/test_auth/test_social_pipeline.py index 062dfa2169..8ca59c728f 100644 --- a/tests/unit/test_auth/test_social_pipeline.py +++ b/tests/unit/test_auth/test_social_pipeline.py @@ -6,6 +6,7 @@ from rest_framework.exceptions import ValidationError from authentication.social_pipeline import associate_by_email, create_user +from authentication.views.social import SocialCodeAuth from tests.unit.test_users.factories import factory_user from users.models import User @@ -111,3 +112,24 @@ def test_pipeline_does_not_run_the_username_deriving_step(self): "social_core.pipeline.user.get_username" not in settings.SOCIAL_AUTH_PIPELINE ) + + +class TestSocialAuthResponse: + """ + social_core stamps is_new on the user the pipeline returns. The response has + to carry it through, or the client cannot tell a signup from a sign-in and + every Google login would count as a registration. + """ + + def test_reports_a_signup(self): + user = factory_user() + user.is_new = True + + assert SocialCodeAuth.TokenSerializer(instance=user).data["is_new"] is True + + def test_reports_a_sign_in(self): + # No attribute at all, which is what an existing account arrives with + assert ( + SocialCodeAuth.TokenSerializer(instance=factory_user()).data["is_new"] + is False + ) From b40cb61312b0ee13426fda2766a800850e798ceb Mon Sep 17 00:00:00 2001 From: Atakan Seckin Date: Mon, 7 Sep 2026 10:43:50 +0200 Subject: [PATCH 2/2] Stop assuming metadata is shaped, and name the provider that signed them up Review feedback, both valid. metadata is free-form JSON that staff edit by hand, and the model says as much: structure is not enforced. Walking two levels of it with .get assumed both were objects, so a hand-edited string, number or list took the whole sign-in down with an AttributeError. Anything that is not the expected shape now simply fails to match. The register event also hardcoded Google, while the callback route accepts any provider and Facebook is configured on the backend already - only the button is missing. Reporting the provider itself is both correct today and correct if another one is ever offered, and it needs no constant of its own since the provider name is the method. The malformed-metadata cases run against the service rather than the endpoint: serializing a user whose metadata is not an object fails separately in get_max_bots, which walks it the same unguarded way. That one is not this branch's to fix. Co-Authored-By: Claude Opus 5 --- authentication/services/email_link.py | 13 +++-- .../accounts/social/[provider]/client.tsx | 7 +-- front_end/src/utils/signup_methods.ts | 4 +- tests/unit/test_auth/test_email_link.py | 52 +++++++++++++++++++ 4 files changed, 69 insertions(+), 7 deletions(-) diff --git a/authentication/services/email_link.py b/authentication/services/email_link.py index c9d19ff3c7..f5ebad0bfa 100644 --- a/authentication/services/email_link.py +++ b/authentication/services/email_link.py @@ -53,11 +53,18 @@ def verify_email_link_auth(user_id: int, token: str) -> tuple[User, bool]: # that this flow created the account - a signup-form account still waiting # on its confirmation email can be activated by a link too, and that signup # was already counted when the form was submitted. - is_new = user.check_can_activate() and ( - (user.metadata or {}).get("signup_details", {}).get("method") - == SIGNUP_METHOD_EMAIL_LINK + # + # metadata is free-form JSON that staff can edit by hand, so nothing + # guarantees either level is an object. Anything else simply does not match. + metadata = user.metadata if isinstance(user.metadata, dict) else {} + signup_details = metadata.get("signup_details") + created_by_email_link = ( + isinstance(signup_details, dict) + and signup_details.get("method") == SIGNUP_METHOD_EMAIL_LINK ) + is_new = user.check_can_activate() and created_by_email_link + if user.check_can_activate(): user.is_active = True user.save(update_fields=["is_active"]) diff --git a/front_end/src/app/(main)/accounts/social/[provider]/client.tsx b/front_end/src/app/(main)/accounts/social/[provider]/client.tsx index dd0b8217cc..315934ef05 100644 --- a/front_end/src/app/(main)/accounts/social/[provider]/client.tsx +++ b/front_end/src/app/(main)/accounts/social/[provider]/client.tsx @@ -16,7 +16,6 @@ import { sendAnalyticsEvent } from "@/utils/analytics"; import { rotateCsrfToken } from "@/utils/csrf"; import { withConfirmedEvent } from "@/utils/email_link_confirmation"; import { EMAIL_CAPTURE_SIGNUP_SOURCE } from "@/utils/gated_actions"; -import { SIGNUP_METHOD_GOOGLE } from "@/utils/signup_methods"; type Props = { provider: SocialProviderType; @@ -50,10 +49,12 @@ const SocialAuthClient: FC = ({ stash ? EMAIL_CAPTURE_SIGNUP_SOURCE : null ); // Only the exchange that created the account is a registration; an - // existing user signing in with Google lands here too. + // existing user signing in with a provider lands here too. The provider + // name is the method, so this stays right if another one is enabled - + // Facebook is configured on the backend already, just not offered. if (isNew) { sendAnalyticsEvent("register", { - method: SIGNUP_METHOD_GOOGLE, + method: provider, fromEmailCapture: !!stash, }); } diff --git a/front_end/src/utils/signup_methods.ts b/front_end/src/utils/signup_methods.ts index 66404c2e92..675557a56f 100644 --- a/front_end/src/utils/signup_methods.ts +++ b/front_end/src/utils/signup_methods.ts @@ -1,7 +1,9 @@ /** * How an account came into existence, sent as the `method` property on the * `register` analytics event so registrations can be split by route. + * + * Social signups report the provider name itself rather than a constant here, + * so a newly enabled provider is labelled correctly without another edit. */ export const SIGNUP_METHOD_EMAIL_LINK = "email_link"; export const SIGNUP_METHOD_PASSWORD = "password"; -export const SIGNUP_METHOD_GOOGLE = "google-oauth2"; diff --git a/tests/unit/test_auth/test_email_link.py b/tests/unit/test_auth/test_email_link.py index d2d7b5556c..7f46b0e648 100644 --- a/tests/unit/test_auth/test_email_link.py +++ b/tests/unit/test_auth/test_email_link.py @@ -1,13 +1,16 @@ import datetime import re +import pytest from django.contrib.auth.tokens import default_token_generator from django.utils import timezone from rest_framework.reverse import reverse from authentication.services.email_link import ( + SIGNUP_METHOD_EMAIL_LINK, EmailLinkTokenGenerator, email_link_token_generator, + verify_email_link_auth, ) from authentication.services.gated_actions import ( pop_pending_action, @@ -229,6 +232,55 @@ def test_unconfirmed_signup_activated_by_link_is_not_new(self, anon_client, mock user.refresh_from_db() assert user.is_active + @pytest.mark.parametrize( + "metadata", + [ + "not-an-object", + 42, + ["signup_details"], + {"signup_details": "not-an-object"}, + {"signup_details": None}, + {"signup_details": {"method": "something_else"}}, + ], + ) + def test_malformed_metadata_is_not_a_signup(self, metadata): + """ + metadata is free-form JSON that staff can edit by hand, so the signup + method lookup must not assume either level is an object - it used to + raise AttributeError. Exercised against the service rather than the + endpoint because serializing a user with non-object metadata fails + separately, in get_max_bots, which this branch does not touch. + """ + user = User.objects.create_user( + username="oddmetadata", + email="oddmetadata@example.com", + password=None, + is_active=False, + metadata=metadata, + ) + token = email_link_token_generator.make_token(user) + + verified, is_new = verify_email_link_auth(user.id, token) + + assert verified.id == user.id + assert is_new is False + verified.refresh_from_db() + assert verified.is_active + + def test_metadata_marking_this_flow_is_a_signup(self): + user = User.objects.create_user( + username="linksignup", + email="linksignup@example.com", + password=None, + is_active=False, + metadata={"signup_details": {"method": SIGNUP_METHOD_EMAIL_LINK}}, + ) + token = email_link_token_generator.make_token(user) + + _, is_new = verify_email_link_auth(user.id, token) + + assert is_new is True + def test_single_use(self, anon_client, user1): token = email_link_token_generator.make_token(user1)