Add metrics for registration lock flow

This commit is contained in:
Katherine Yen 2023-03-09 09:07:21 -08:00 committed by GitHub
parent c06313dd2e
commit 46fef4082c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 157 additions and 39 deletions

View File

@ -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 doesnt 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<Long> 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(),

View File

@ -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<String> registrationLock;
@ -34,15 +43,28 @@ public class StoredRegistrationLock {
return registrationLock.isPresent() && registrationLockSalt.isPresent();
}
public boolean isPresent() {
return hasLockAndSalt();
}
public StoredRegistrationLock(Optional<String> registrationLock, Optional<String> 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) {

View File

@ -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<Account> 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);

View File

@ -91,7 +91,8 @@ public class AccountControllerV2 {
final Optional<Account> 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),

View File

@ -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)) {

View File

@ -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<Arguments> 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")
);
}

View File

@ -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<String> registrationLock, final Optional<String> salt, final long lastSeen,
final StoredRegistrationLock.Status expectedStatus) {
final StoredRegistrationLock storedLock = new StoredRegistrationLock(registrationLock, salt, lastSeen);
assertEquals(expectedStatus, storedLock.getStatus());
}
private static Stream<Arguments> 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)
);
}
}

View File

@ -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);

View File

@ -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")

View File

@ -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")