diff --git a/pom.xml b/pom.xml index 3170fd286..0381945a0 100644 --- a/pom.xml +++ b/pom.xml @@ -363,6 +363,12 @@ junit-jupiter-api test + + org.junit-pioneer + junit-pioneer + 1.9.1 + test + diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java b/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java index e8d68842b..6cbb02605 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticator.java @@ -27,9 +27,11 @@ import org.whispersystems.textsecuregcm.util.Util; public class BaseAccountAuthenticator { private static final String AUTHENTICATION_COUNTER_NAME = name(BaseAccountAuthenticator.class, "authentication"); + private static final String ENABLED_NOT_REQUIRED_AUTHENTICATION_COUNTER_NAME = name(BaseAccountAuthenticator.class, + "enabledNotRequiredAuthentication"); private static final String AUTHENTICATION_SUCCEEDED_TAG_NAME = "succeeded"; private static final String AUTHENTICATION_FAILURE_REASON_TAG_NAME = "reason"; - private static final String AUTHENTICATION_ENABLED_REQUIRED_TAG_NAME = "enabledRequired"; + private static final String ENABLED_TAG_NAME = "enabled"; private static final String AUTHENTICATION_HAS_STORY_CAPABILITY = "hasStoryCapability"; private static final String STORY_ADOPTION_COUNTER_NAME = name(BaseAccountAuthenticator.class, "storyAdoption"); @@ -37,6 +39,9 @@ public class BaseAccountAuthenticator { private static final String DAYS_SINCE_LAST_SEEN_DISTRIBUTION_NAME = name(BaseAccountAuthenticator.class, "daysSinceLastSeen"); private static final String IS_PRIMARY_DEVICE_TAG = "isPrimary"; + @VisibleForTesting + static final char DEVICE_ID_SEPARATOR = '.'; + private final AccountsManager accountsManager; private final Clock clock; @@ -54,7 +59,7 @@ public class BaseAccountAuthenticator { final String identifier; final long deviceId; - final int deviceIdSeparatorIndex = basicUsername.indexOf('.'); + final int deviceIdSeparatorIndex = basicUsername.indexOf(DEVICE_ID_SEPARATOR); if (deviceIdSeparatorIndex == -1) { identifier = basicUsername; @@ -99,15 +104,23 @@ public class BaseAccountAuthenticator { } if (enabledRequired) { - if (!device.get().isEnabled()) { + final boolean deviceDisabled = !device.get().isEnabled(); + if (deviceDisabled) { failureReason = "deviceDisabled"; - return Optional.empty(); } - if (!account.get().isEnabled()) { + final boolean accountDisabled = !account.get().isEnabled(); + if (accountDisabled) { failureReason = "accountDisabled"; + } + if (accountDisabled || deviceDisabled) { return Optional.empty(); } + } else { + Metrics.counter(ENABLED_NOT_REQUIRED_AUTHENTICATION_COUNTER_NAME, + ENABLED_TAG_NAME, String.valueOf(device.get().isEnabled() && account.get().isEnabled()), + IS_PRIMARY_DEVICE_TAG, String.valueOf(device.get().isMaster())) + .increment(); } AuthenticationCredentials deviceAuthenticationCredentials = device.get().getAuthenticationCredentials(); @@ -130,8 +143,7 @@ public class BaseAccountAuthenticator { return Optional.empty(); } finally { Tags tags = Tags.of( - AUTHENTICATION_SUCCEEDED_TAG_NAME, String.valueOf(succeeded), - AUTHENTICATION_ENABLED_REQUIRED_TAG_NAME, String.valueOf(enabledRequired)); + AUTHENTICATION_SUCCEEDED_TAG_NAME, String.valueOf(succeeded)); if (StringUtils.isNotBlank(failureReason)) { tags = tags.and(AUTHENTICATION_FAILURE_REASON_TAG_NAME, failureReason); diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java index e26cb0214..e369b531b 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/auth/BaseAccountAuthenticatorTest.java @@ -19,7 +19,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import io.dropwizard.auth.basic.BasicCredentials; -import java.time.Clock; import java.time.Instant; import java.util.List; import java.util.Optional; @@ -31,7 +30,7 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; -import org.whispersystems.textsecuregcm.experiment.ExperimentEnrollmentManager; +import org.junitpioneer.jupiter.cartesian.CartesianTest; import org.whispersystems.textsecuregcm.storage.Account; import org.whispersystems.textsecuregcm.storage.AccountsManager; import org.whispersystems.textsecuregcm.storage.Device; @@ -208,37 +207,46 @@ class BaseAccountAuthenticatorTest { verify(accountsManager, never()).updateDeviceAuthentication(any(), any(), any()); } - @ParameterizedTest - @ValueSource(booleans = {true, false}) - void testAuthenticateEnabledRequired(final boolean enabledRequired) { + @CartesianTest + void testAuthenticateEnabledRequired( + @CartesianTest.Values(booleans = {true, false}) final boolean enabledRequired, + @CartesianTest.Values(booleans = {true, false}) final boolean accountEnabled, + @CartesianTest.Values(booleans = {true, false}) final boolean deviceEnabled, + @CartesianTest.Values(booleans = {true, false}) final boolean authenticatedDeviceIsPrimary) { final UUID uuid = UUID.randomUUID(); - final long deviceId = 1; + final long deviceId = authenticatedDeviceIsPrimary ? 1 : 2; final String password = "12345"; final Account account = mock(Account.class); - final Device device = mock(Device.class); + final Device authenticatedDevice = mock(Device.class); final AuthenticationCredentials credentials = mock(AuthenticationCredentials.class); clock.unpin(); when(accountsManager.getByAccountIdentifier(uuid)).thenReturn(Optional.of(account)); when(account.getUuid()).thenReturn(uuid); - when(account.getDevice(deviceId)).thenReturn(Optional.of(device)); - when(account.isEnabled()).thenReturn(false); - when(device.getId()).thenReturn(deviceId); - when(device.isEnabled()).thenReturn(false); - when(device.getAuthenticationCredentials()).thenReturn(credentials); + when(account.getDevice(deviceId)).thenReturn(Optional.of(authenticatedDevice)); + when(account.isEnabled()).thenReturn(accountEnabled); + when(authenticatedDevice.getId()).thenReturn(deviceId); + when(authenticatedDevice.isEnabled()).thenReturn(deviceEnabled); + when(authenticatedDevice.getAuthenticationCredentials()).thenReturn(credentials); when(credentials.verify(password)).thenReturn(true); when(credentials.getVersion()).thenReturn(AuthenticationCredentials.CURRENT_VERSION); + final String identifier; + if (authenticatedDeviceIsPrimary) { + identifier = uuid.toString(); + } else { + identifier = uuid.toString() + BaseAccountAuthenticator.DEVICE_ID_SEPARATOR + deviceId; + } final Optional maybeAuthenticatedAccount = - baseAccountAuthenticator.authenticate(new BasicCredentials(uuid.toString(), password), enabledRequired); + baseAccountAuthenticator.authenticate(new BasicCredentials(identifier, password), enabledRequired); - if (enabledRequired) { + if (enabledRequired && !(accountEnabled && deviceEnabled)) { assertThat(maybeAuthenticatedAccount).isEmpty(); } else { assertThat(maybeAuthenticatedAccount).isPresent(); assertThat(maybeAuthenticatedAccount.get().getAccount().getUuid()).isEqualTo(uuid); - assertThat(maybeAuthenticatedAccount.get().getAuthenticatedDevice()).isEqualTo(device); + assertThat(maybeAuthenticatedAccount.get().getAuthenticatedDevice()).isEqualTo(authenticatedDevice); } }