diff --git a/authentication/services/email_link.py b/authentication/services/email_link.py index 1480aea74e..f5ebad0bfa 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,28 @@ 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. + # + # 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"]) - 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..315934ef05 100644 --- a/front_end/src/app/(main)/accounts/social/[provider]/client.tsx +++ b/front_end/src/app/(main)/accounts/social/[provider]/client.tsx @@ -12,6 +12,7 @@ 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"; @@ -38,7 +39,7 @@ const SocialAuthClient: FC = ({ const stash = takeSocialGatedAction(); void (async () => { try { - await exchangeSocialOauthCode( + const { isNew } = await exchangeSocialOauthCode( provider, code, nonce, @@ -47,6 +48,17 @@ 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 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: provider, + 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..675557a56f --- /dev/null +++ b/front_end/src/utils/signup_methods.ts @@ -0,0 +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"; diff --git a/tests/unit/test_auth/test_email_link.py b/tests/unit/test_auth/test_email_link.py index bcff16d41d..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, @@ -184,6 +187,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 +204,83 @@ 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 + + @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) 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 + )