From 46fef4082cf2abce374b8c90d2b8141d33f50b7c Mon Sep 17 00:00:00 2001 From: Katherine Yen Date: Thu, 9 Mar 2023 09:07:21 -0800 Subject: [PATCH] Add metrics for registration lock flow --- .../RegistrationLockVerificationManager.java | 78 +++++++++++++++---- .../auth/StoredRegistrationLock.java | 30 ++++++- .../controllers/AccountController.java | 8 +- .../controllers/AccountControllerV2.java | 3 +- .../controllers/RegistrationController.java | 3 +- ...gistrationLockVerificationManagerTest.java | 27 ++++--- .../auth/StoredRegistrationLockTest.java | 35 +++++++++ .../controllers/AccountControllerTest.java | 8 +- .../controllers/AccountControllerV2Test.java | 2 +- .../RegistrationControllerTest.java | 2 +- 10 files changed, 157 insertions(+), 39 deletions(-) create mode 100644 service/src/test/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLockTest.java diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java b/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java index d2446fd85..b68887ad8 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManager.java @@ -11,23 +11,39 @@ import com.google.common.annotations.VisibleForTesting; import javax.annotation.Nullable; import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Response; +import io.micrometer.core.instrument.DistributionSummary; +import io.micrometer.core.instrument.Metrics; +import io.micrometer.core.instrument.Tag; +import io.micrometer.core.instrument.Tags; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; +import org.whispersystems.textsecuregcm.entities.PhoneVerificationRequest; import org.whispersystems.textsecuregcm.entities.RegistrationLockFailure; import org.whispersystems.textsecuregcm.limits.RateLimiters; +import org.whispersystems.textsecuregcm.metrics.UserAgentTagUtil; import org.whispersystems.textsecuregcm.push.ClientPresenceManager; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.util.Util; +import java.time.Duration; +import java.time.Instant; public class RegistrationLockVerificationManager { + public enum Flow { + REGISTRATION, + CHANGE_NUMBER + } @VisibleForTesting public static final int FAILURE_HTTP_STATUS = 423; - private static final String LOCKED_ACCOUNT_COUNTER_NAME = - name(RegistrationLockVerificationManager.class, "lockedAccount"); - private static final String LOCK_REASON_TAG_NAME = "lockReason"; + private static final String EXPIRED_REGISTRATION_LOCK_COUNTER_NAME = + name(RegistrationLockVerificationManager.class, "expiredRegistrationLock"); + private static final String REQUIRED_REGISTRATION_LOCK_COUNTER_NAME = + name(RegistrationLockVerificationManager.class, "requiredRegistrationLock"); private static final String ALREADY_LOCKED_TAG_NAME = "alreadyLocked"; + private static final String REGISTRATION_LOCK_VERIFICATION_FLOW_TAG_NAME = "flow"; + private static final String REGISTRATION_LOCK_MATCHES_TAG_NAME = "registrationLockMatches"; + private static final String PHONE_VERIFICATION_TYPE_TAG_NAME = "phoneVerificationType"; private final AccountsManager accounts; @@ -52,15 +68,29 @@ public class RegistrationLockVerificationManager { * @throws RateLimitExceededException * @throws WebApplicationException */ - public void verifyRegistrationLock(final Account account, @Nullable final String clientRegistrationLock) - throws RateLimitExceededException, WebApplicationException { + public void verifyRegistrationLock(final Account account, @Nullable final String clientRegistrationLock, + final String userAgent, + final Flow flow, + final PhoneVerificationRequest.VerificationType phoneVerificationType + ) throws RateLimitExceededException, WebApplicationException { + + final Tags expiredTags = Tags.of(UserAgentTagUtil.getPlatformTag(userAgent), + Tag.of(REGISTRATION_LOCK_VERIFICATION_FLOW_TAG_NAME, flow.name()), + Tag.of(PHONE_VERIFICATION_TYPE_TAG_NAME, phoneVerificationType.name()) + ); final StoredRegistrationLock existingRegistrationLock = account.getRegistrationLock(); - final ExternalServiceCredentials existingBackupCredentials = - backupServiceCredentialGenerator.generateForUuid(account.getUuid()); - if (!existingRegistrationLock.requiresClientRegistrationLock()) { - return; + switch (existingRegistrationLock.getStatus()) { + case EXPIRED: + Metrics.counter(EXPIRED_REGISTRATION_LOCK_COUNTER_NAME, expiredTags).increment(); + return; + case ABSENT: + return; + case REQUIRED: + break; + default: + throw new RuntimeException("Unexpected status: " + existingRegistrationLock.getStatus()); } if (!Util.isEmpty(clientRegistrationLock)) { @@ -68,18 +98,34 @@ public class RegistrationLockVerificationManager { } final String phoneNumber = account.getNumber(); + final boolean registrationLockMatches = existingRegistrationLock.verify(clientRegistrationLock); + final boolean alreadyLocked = account.hasLockedCredentials(); - if (!existingRegistrationLock.verify(clientRegistrationLock)) { + final Tags additionalTags = expiredTags.and( + REGISTRATION_LOCK_MATCHES_TAG_NAME, Boolean.toString(registrationLockMatches), + ALREADY_LOCKED_TAG_NAME, Boolean.toString(alreadyLocked) + ); + + Metrics.counter(REQUIRED_REGISTRATION_LOCK_COUNTER_NAME, additionalTags).increment(); + + final DistributionSummary registrationLockIdleDays = DistributionSummary + .builder(name(RegistrationLockVerificationManager.class, "registrationLockIdleDays")) + .tags(additionalTags) + .publishPercentiles(0.75, 0.95, 0.99, 0.999) + .distributionStatisticExpiry(Duration.ofHours(2)) + .register(Metrics.globalRegistry); + + final Instant accountLastSeen = Instant.ofEpochMilli(account.getLastSeen()); + final Duration timeSinceLastSeen = Duration.between(accountLastSeen, Instant.now()); + + registrationLockIdleDays.record(timeSinceLastSeen.toDays()); + + if (!registrationLockMatches) { // At this point, the client verified ownership of the phone number but doesn’t have the reglock PIN. // Freezing the existing account credentials will definitively start the reglock timeout. // Until the timeout, the current reglock can still be supplied, // along with phone number verification, to restore access. /* - boolean alreadyLocked = existingAccount.hasLockedCredentials(); - Metrics.counter(LOCKED_ACCOUNT_COUNTER_NAME, - LOCK_REASON_TAG_NAME, "verifiedNumberFailedReglock", - ALREADY_LOCKED_TAG_NAME, Boolean.toString(alreadyLocked)) - .increment(); final Account updatedAccount; if (!alreadyLocked) { @@ -91,6 +137,8 @@ public class RegistrationLockVerificationManager { List deviceIds = updatedAccount.getDevices().stream().map(Device::getId).toList(); clientPresenceManager.disconnectAllPresences(updatedAccount.getUuid(), deviceIds); */ + final ExternalServiceCredentials existingBackupCredentials = + backupServiceCredentialGenerator.generateForUuid(account.getUuid()); throw new WebApplicationException(Response.status(FAILURE_HTTP_STATUS) .entity(new RegistrationLockFailure(existingRegistrationLock.getTimeRemaining(), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLock.java b/service/src/main/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLock.java index 96f8f7057..bf469e727 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLock.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLock.java @@ -6,6 +6,7 @@ package org.whispersystems.textsecuregcm.auth; import com.google.common.annotations.VisibleForTesting; +import java.time.Duration; import java.util.Optional; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; @@ -13,6 +14,14 @@ import org.whispersystems.textsecuregcm.util.Util; @SuppressWarnings("OptionalUsedAsFieldOrParameterType") public class StoredRegistrationLock { + public enum Status { + REQUIRED, + EXPIRED, + ABSENT + } + + @VisibleForTesting + static final Duration REGISTRATION_LOCK_EXPIRATION_DAYS = Duration.ofDays(7); private final Optional registrationLock; @@ -34,15 +43,28 @@ public class StoredRegistrationLock { return registrationLock.isPresent() && registrationLockSalt.isPresent(); } + public boolean isPresent() { + return hasLockAndSalt(); + } + public StoredRegistrationLock(Optional registrationLock, Optional registrationLockSalt, long lastSeen) { this.registrationLock = registrationLock; this.registrationLockSalt = registrationLockSalt; this.lastSeen = lastSeen; } - public boolean requiresClientRegistrationLock() { - boolean hasTimeRemaining = getTimeRemaining() >= 0; - return hasLockAndSalt() && hasTimeRemaining; + private boolean hasTimeRemaining() { + return getTimeRemaining() >= 0; + } + + public Status getStatus() { + if (!isPresent()) { + return Status.ABSENT; + } + if (hasTimeRemaining()) { + return Status.REQUIRED; + } + return Status.EXPIRED; } public boolean needsFailureCredentials() { @@ -50,7 +72,7 @@ public class StoredRegistrationLock { } public long getTimeRemaining() { - return TimeUnit.DAYS.toMillis(7) - timeSinceLastSeen(); + return REGISTRATION_LOCK_EXPIRATION_DAYS.toMillis() - timeSinceLastSeen(); } public boolean verify(@Nullable String clientRegistrationLock) { diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java index 6d0b081af..c593f093e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountController.java @@ -78,6 +78,7 @@ import org.whispersystems.textsecuregcm.entities.ConfirmUsernameHashRequest; import org.whispersystems.textsecuregcm.entities.DeviceName; import org.whispersystems.textsecuregcm.entities.GcmRegistrationId; import org.whispersystems.textsecuregcm.entities.MismatchedDevices; +import org.whispersystems.textsecuregcm.entities.PhoneVerificationRequest; import org.whispersystems.textsecuregcm.entities.RegistrationLock; import org.whispersystems.textsecuregcm.entities.ReserveUsernameHashRequest; import org.whispersystems.textsecuregcm.entities.ReserveUsernameHashResponse; @@ -409,7 +410,9 @@ public class AccountController { if (existingAccount.isPresent()) { registrationLockVerificationManager.verifyRegistrationLock(existingAccount.get(), - accountAttributes.getRegistrationLock()); + accountAttributes.getRegistrationLock(), + userAgent, RegistrationLockVerificationManager.Flow.REGISTRATION, + PhoneVerificationRequest.VerificationType.SESSION); } if (availableForTransfer.orElse(false) && existingAccount.map(Account::isTransferSupported).orElse(false)) { @@ -471,7 +474,8 @@ public class AccountController { final Optional existingAccount = accounts.getByE164(number); if (existingAccount.isPresent()) { - registrationLockVerificationManager.verifyRegistrationLock(existingAccount.get(), request.registrationLock()); + registrationLockVerificationManager.verifyRegistrationLock(existingAccount.get(), request.registrationLock(), + userAgent, RegistrationLockVerificationManager.Flow.CHANGE_NUMBER, PhoneVerificationRequest.VerificationType.SESSION); } rateLimiters.getVerifyLimiter().clear(number); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java index ff22b02e9..c5f3f8509 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2.java @@ -91,7 +91,8 @@ public class AccountControllerV2 { final Optional existingAccount = accountsManager.getByE164(number); if (existingAccount.isPresent()) { - registrationLockVerificationManager.verifyRegistrationLock(existingAccount.get(), request.registrationLock()); + registrationLockVerificationManager.verifyRegistrationLock(existingAccount.get(), request.registrationLock(), + userAgent, RegistrationLockVerificationManager.Flow.CHANGE_NUMBER, verificationType); } Metrics.counter(CHANGE_NUMBER_COUNTER_NAME, Tags.of(UserAgentTagUtil.getPlatformTag(userAgent), diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java index 9f71cfb14..6feded00e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/RegistrationController.java @@ -102,7 +102,8 @@ public class RegistrationController { if (existingAccount.isPresent()) { registrationLockVerificationManager.verifyRegistrationLock(existingAccount.get(), - registrationRequest.accountAttributes().getRegistrationLock()); + registrationRequest.accountAttributes().getRegistrationLock(), + userAgent, RegistrationLockVerificationManager.Flow.REGISTRATION, verificationType); } if (!registrationRequest.skipDeviceTransfer() && existingAccount.map(Account::isTransferSupported).orElse(false)) { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java index 28d4a9257..e4f5528f1 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/RegistrationLockVerificationManagerTest.java @@ -26,6 +26,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; import org.whispersystems.textsecuregcm.controllers.RateLimitExceededException; +import org.whispersystems.textsecuregcm.entities.PhoneVerificationRequest; import org.whispersystems.textsecuregcm.limits.RateLimiter; import org.whispersystems.textsecuregcm.limits.RateLimiters; import org.whispersystems.textsecuregcm.push.ClientPresenceManager; @@ -65,7 +66,7 @@ class RegistrationLockVerificationManagerTest { @EnumSource void testErrors(RegistrationLockError error) throws Exception { - when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(true); + when(existingRegistrationLock.getStatus()).thenReturn(StoredRegistrationLock.Status.REQUIRED); final String submittedRegistrationLock = "reglock"; @@ -89,29 +90,35 @@ class RegistrationLockVerificationManagerTest { }; final Exception e = assertThrows(exceptionType.first(), () -> - registrationLockVerificationManager.verifyRegistrationLock(account, submittedRegistrationLock)); + registrationLockVerificationManager.verifyRegistrationLock(account, submittedRegistrationLock, + "Signal-Android/4.68.3", RegistrationLockVerificationManager.Flow.REGISTRATION, + PhoneVerificationRequest.VerificationType.SESSION)); exceptionType.second().accept(e); } @ParameterizedTest @MethodSource - void testSuccess(final boolean requiresClientRegistrationLock, @Nullable final String submittedRegistrationLock) { + void testSuccess(final StoredRegistrationLock.Status status, @Nullable final String submittedRegistrationLock) { - when(existingRegistrationLock.requiresClientRegistrationLock()) - .thenReturn(requiresClientRegistrationLock); + when(existingRegistrationLock.getStatus()) + .thenReturn(status); when(existingRegistrationLock.verify(submittedRegistrationLock)).thenReturn(true); assertDoesNotThrow( - () -> registrationLockVerificationManager.verifyRegistrationLock(account, submittedRegistrationLock)); + () -> registrationLockVerificationManager.verifyRegistrationLock(account, submittedRegistrationLock, + "Signal-Android/4.68.3", RegistrationLockVerificationManager.Flow.REGISTRATION, + PhoneVerificationRequest.VerificationType.SESSION)); } static Stream testSuccess() { return Stream.of( - Arguments.of(false, null), - Arguments.of(true, null), - Arguments.of(false, "reglock"), - Arguments.of(true, "reglock") + Arguments.of(StoredRegistrationLock.Status.ABSENT, null), + Arguments.of(StoredRegistrationLock.Status.EXPIRED, null), + Arguments.of(StoredRegistrationLock.Status.REQUIRED, null), + Arguments.of(StoredRegistrationLock.Status.ABSENT, "reglock"), + Arguments.of(StoredRegistrationLock.Status.EXPIRED, "reglock"), + Arguments.of(StoredRegistrationLock.Status.REQUIRED, "reglock") ); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLockTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLockTest.java new file mode 100644 index 000000000..2538d8b25 --- /dev/null +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/StoredRegistrationLockTest.java @@ -0,0 +1,35 @@ +package org.whispersystems.textsecuregcm.auth; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import javax.swing.text.html.Option; +import java.time.Duration; +import java.util.Optional; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; +import static org.whispersystems.textsecuregcm.auth.StoredRegistrationLock.REGISTRATION_LOCK_EXPIRATION_DAYS; + +public class StoredRegistrationLockTest { + @ParameterizedTest + @MethodSource + void getStatus(final Optional registrationLock, final Optional salt, final long lastSeen, + final StoredRegistrationLock.Status expectedStatus) { + final StoredRegistrationLock storedLock = new StoredRegistrationLock(registrationLock, salt, lastSeen); + + assertEquals(expectedStatus, storedLock.getStatus()); + } + + private static Stream getStatus() { + return Stream.of( + Arguments.of(Optional.of("registrationLock"), Optional.of("salt"), System.currentTimeMillis() - Duration.ofDays(1).toMillis(), StoredRegistrationLock.Status.REQUIRED), + Arguments.of(Optional.empty(), Optional.empty(), 0L, StoredRegistrationLock.Status.ABSENT), + Arguments.of(Optional.of("registrationLock"), Optional.of("salt"), System.currentTimeMillis() - REGISTRATION_LOCK_EXPIRATION_DAYS.toMillis(), StoredRegistrationLock.Status.EXPIRED) + ); + } + +} diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java index cf9d09232..54c5520ab 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerTest.java @@ -1448,7 +1448,7 @@ class AccountControllerTest { .thenReturn(CompletableFuture.completedFuture(true)); final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); - when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(false); + when(existingRegistrationLock.getStatus()).thenReturn(StoredRegistrationLock.Status.ABSENT); final Account existingAccount = mock(Account.class); when(existingAccount.getNumber()).thenReturn(number); @@ -1482,7 +1482,7 @@ class AccountControllerTest { .thenReturn(CompletableFuture.completedFuture(true)); final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); - when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(true); + when(existingRegistrationLock.getStatus()).thenReturn(StoredRegistrationLock.Status.REQUIRED); final UUID existingUuid = UUID.randomUUID(); final Account existingAccount = mock(Account.class); @@ -1521,7 +1521,7 @@ class AccountControllerTest { .thenReturn(CompletableFuture.completedFuture(true)); final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); - when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(true); + when(existingRegistrationLock.getStatus()).thenReturn(StoredRegistrationLock.Status.REQUIRED); when(existingRegistrationLock.verify(anyString())).thenReturn(false); UUID existingUuid = UUID.randomUUID(); @@ -1561,7 +1561,7 @@ class AccountControllerTest { .thenReturn(CompletableFuture.completedFuture(true)); final StoredRegistrationLock existingRegistrationLock = mock(StoredRegistrationLock.class); - when(existingRegistrationLock.requiresClientRegistrationLock()).thenReturn(true); + when(existingRegistrationLock.getStatus()).thenReturn(StoredRegistrationLock.Status.REQUIRED); when(existingRegistrationLock.verify(reglock)).thenReturn(true); final Account existingAccount = mock(Account.class); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java index b2c458413..3bc4df53a 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/AccountControllerV2Test.java @@ -295,7 +295,7 @@ class AccountControllerV2Test { case RATE_LIMITED -> new RateLimitExceededException(null, true); }; doThrow(e) - .when(registrationLockVerificationManager).verifyRegistrationLock(any(), any()); + .when(registrationLockVerificationManager).verifyRegistrationLock(any(), any(), any(), any(), any()); final Invocation.Builder request = resources.getJerseyTest() .target("/v2/accounts/number") diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java index f3b05dc84..c820eec3c 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/RegistrationControllerTest.java @@ -265,7 +265,7 @@ class RegistrationControllerTest { case RATE_LIMITED -> new RateLimitExceededException(null, true); }; doThrow(e) - .when(registrationLockVerificationManager).verifyRegistrationLock(any(), any()); + .when(registrationLockVerificationManager).verifyRegistrationLock(any(), any(), any(), any(), any()); final Invocation.Builder request = resources.getJerseyTest() .target("/v1/registration")