From b3bd4ccc17e3b6e01ee84b0bd46ca5c95eb989b8 Mon Sep 17 00:00:00 2001 From: Jonathan Klabunde Tomer <125505367+jkt-signal@users.noreply.github.com> Date: Tue, 23 Apr 2024 14:49:04 -0700 Subject: [PATCH] simplify profile auth --- .../textsecuregcm/auth/OptionalAccess.java | 38 +++++++++++-------- .../controllers/ProfileController.java | 36 +++++------------- .../textsecuregcm/util/ProfileHelper.java | 4 +- .../controllers/ProfileControllerTest.java | 11 +----- 4 files changed, 35 insertions(+), 54 deletions(-) diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/auth/OptionalAccess.java b/service/src/main/java/org/whispersystems/textsecuregcm/auth/OptionalAccess.java index 1808d5863..c7387bd3a 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/auth/OptionalAccess.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/auth/OptionalAccess.java @@ -50,25 +50,31 @@ public class OptionalAccess { Optional accessKey, Optional targetAccount) { - if (requestAccount.isPresent() && targetAccount.isPresent() && targetAccount.get().isEnabled()) { + if (requestAccount.isPresent()) { + // Authenticated requests are never unauthorized; if the target exists and is enabled, return OK, otherwise throw not-found. + if (targetAccount.isPresent() && targetAccount.get().isEnabled()) { + return; + } else { + throw new NotFoundException(); + } + } + + // Anything past this point can only be authenticated by an access key. Even when the target + // has unrestricted unidentified access, callers need to supply a fake access key. Likewise, if + // the target account does not exist, we *also* report unauthorized here (*not* not-found, + // since that would provide a free exists check). + if (accessKey.isEmpty() || !targetAccount.map(Account::isEnabled).orElse(false)) { + throw new NotAuthorizedException(Response.Status.UNAUTHORIZED); + } + + // Unrestricted unidentified access does what it says on the tin: we don't check if the key the + // caller provided is right or not. + if (targetAccount.get().isUnrestrictedUnidentifiedAccess()) { return; } - //noinspection ConstantConditions - if (requestAccount.isPresent() && (targetAccount.isEmpty() || (targetAccount.isPresent() && !targetAccount.get().isEnabled()))) { - throw new NotFoundException(); - } - - if (accessKey.isPresent() && targetAccount.isPresent() && targetAccount.get().isEnabled() && targetAccount.get().isUnrestrictedUnidentifiedAccess()) { - return; - } - - if (accessKey.isPresent() && - targetAccount.isPresent() && - targetAccount.get().getUnidentifiedAccessKey().isPresent() && - targetAccount.get().isEnabled() && - MessageDigest.isEqual(accessKey.get().getAccessKey(), targetAccount.get().getUnidentifiedAccessKey().get())) - { + // Otherwise, access is gated by the caller having the unidentified-access key matching the target account. + if (MessageDigest.isEqual(accessKey.get().getAccessKey(), targetAccount.get().getUnidentifiedAccessKey().get())) { return; } diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java index b5b2aab35..b9913fe7e 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/controllers/ProfileController.java @@ -237,7 +237,7 @@ public class ProfileController { throws RateLimitExceededException { final Optional maybeRequester = auth.map(AuthenticatedAccount::getAccount); - final Account targetAccount = verifyPermissionToReceiveAccountIdentityProfile(maybeRequester, accessKey, accountIdentifier); + final Account targetAccount = verifyPermissionToReceiveProfile(maybeRequester, accessKey, accountIdentifier); return buildVersionedProfileResponse(targetAccount, version, @@ -263,7 +263,7 @@ public class ProfileController { } final Optional maybeRequester = auth.map(AuthenticatedAccount::getAccount); - final Account targetAccount = verifyPermissionToReceiveAccountIdentityProfile(maybeRequester, accessKey, accountIdentifier); + final Account targetAccount = verifyPermissionToReceiveProfile(maybeRequester, accessKey, accountIdentifier); final boolean isSelf = maybeRequester.map(requester -> ProfileHelper.isSelfProfileRequest(requester.getUuid(), accountIdentifier)).orElse(false); return buildExpiringProfileKeyCredentialProfileResponse(targetAccount, @@ -290,30 +290,16 @@ public class ProfileController { final Optional maybeRequester = auth.map(AuthenticatedAccount::getAccount); + final Account targetAccount = verifyPermissionToReceiveProfile( + maybeRequester, accessKey.filter(ignored -> identifier.identityType() == IdentityType.ACI), identifier); return switch (identifier.identityType()) { case ACI -> { - final AciServiceIdentifier aciServiceIdentifier = (AciServiceIdentifier) identifier; - - final Account targetAccount = - verifyPermissionToReceiveAccountIdentityProfile(maybeRequester, accessKey, aciServiceIdentifier); - yield buildBaseProfileResponseForAccountIdentity(targetAccount, - maybeRequester.map(requester -> ProfileHelper.isSelfProfileRequest(requester.getUuid(), aciServiceIdentifier)).orElse(false), + maybeRequester.map(requester -> ProfileHelper.isSelfProfileRequest(requester.getUuid(), identifier)).orElse(false), containerRequestContext); } case PNI -> { - final Optional maybeAccountByPni = accountsManager.getByPhoneNumberIdentifier(identifier.uuid()); - - if (maybeRequester.isEmpty()) { - throw new WebApplicationException(Response.Status.UNAUTHORIZED); - } else { - rateLimiters.getProfileLimiter().validate(maybeRequester.get().getUuid()); - } - - OptionalAccess.verify(maybeRequester, Optional.empty(), maybeAccountByPni); - - assert maybeAccountByPni.isPresent(); - yield buildBaseProfileResponseForPhoneNumberIdentity(maybeAccountByPni.get()); + yield buildBaseProfileResponseForPhoneNumberIdentity(targetAccount); } }; } @@ -485,19 +471,15 @@ public class ProfileController { * @throws NotAuthorizedException if the requester is not authorized to receive the target account's profile or if the * requester was not authenticated and did not present an anonymous access key */ - private Account verifyPermissionToReceiveAccountIdentityProfile(final Optional maybeRequester, + private Account verifyPermissionToReceiveProfile(final Optional maybeRequester, final Optional maybeAccessKey, - final AciServiceIdentifier accountIdentifier) throws RateLimitExceededException { - - if (maybeRequester.isEmpty() && maybeAccessKey.isEmpty()) { - throw new WebApplicationException(Response.Status.UNAUTHORIZED); - } + final ServiceIdentifier accountIdentifier) throws RateLimitExceededException { if (maybeRequester.isPresent()) { rateLimiters.getProfileLimiter().validate(maybeRequester.get().getUuid()); } - final Optional maybeTargetAccount = accountsManager.getByAccountIdentifier(accountIdentifier.uuid()); + final Optional maybeTargetAccount = accountsManager.getByServiceIdentifier(accountIdentifier); OptionalAccess.verify(maybeRequester, maybeAccessKey, maybeTargetAccount); assert maybeTargetAccount.isPresent(); diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/util/ProfileHelper.java b/service/src/main/java/org/whispersystems/textsecuregcm/util/ProfileHelper.java index ad9bce015..0c4a81497 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/util/ProfileHelper.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/util/ProfileHelper.java @@ -9,7 +9,7 @@ import org.signal.libsignal.zkgroup.profiles.ProfileKeyCommitment; import org.signal.libsignal.zkgroup.profiles.ProfileKeyCredentialRequest; import org.signal.libsignal.zkgroup.profiles.ServerZkProfileOperations; import org.whispersystems.textsecuregcm.configuration.BadgeConfiguration; -import org.whispersystems.textsecuregcm.identity.AciServiceIdentifier; +import org.whispersystems.textsecuregcm.identity.ServiceIdentifier; import org.whispersystems.textsecuregcm.storage.AccountBadge; import org.whispersystems.textsecuregcm.storage.VersionedProfile; import javax.annotation.Nullable; @@ -81,7 +81,7 @@ public class ProfileHelper { return "profiles/" + Base64.getUrlEncoder().encodeToString(object); } - public static boolean isSelfProfileRequest(@Nullable final UUID requesterUuid, final AciServiceIdentifier targetIdentifier) { + public static boolean isSelfProfileRequest(@Nullable final UUID requesterUuid, final ServiceIdentifier targetIdentifier) { return targetIdentifier.uuid().equals(requesterUuid); } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java index 91b58b59b..8b2589cfa 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/controllers/ProfileControllerTest.java @@ -256,7 +256,6 @@ class ProfileControllerTest { assertThat(profile.getBadges()).hasSize(1).element(0).has(new Condition<>( badge -> "Test Badge".equals(badge.getName()), "has badge with expected name")); - verify(accountsManager).getByAccountIdentifier(AuthHelper.VALID_UUID_TWO); verify(rateLimiter, times(1)).validate(AuthHelper.VALID_UUID); } @@ -287,7 +286,6 @@ class ProfileControllerTest { assertThat(profile.getBadges()).hasSize(1).element(0).has(new Condition<>( badge -> "Test Badge".equals(badge.getName()), "has badge with expected name")); - verify(accountsManager).getByAccountIdentifier(AuthHelper.VALID_UUID_TWO); verify(rateLimiter, never()).validate(AuthHelper.VALID_UUID); } @@ -328,7 +326,6 @@ class ProfileControllerTest { assertThat(profile.isUnrestrictedUnidentifiedAccess()).isFalse(); assertThat(profile.getUnidentifiedAccess()).isNull(); - verify(accountsManager).getByPhoneNumberIdentifier(AuthHelper.VALID_PNI_TWO); verify(rateLimiter, times(1)).validate(AuthHelper.VALID_UUID); } @@ -357,7 +354,6 @@ class ProfileControllerTest { assertThat(response.getStatus()).isEqualTo(401); - verify(accountsManager).getByPhoneNumberIdentifier(AuthHelper.VALID_PNI_TWO); verify(rateLimiter, never()).validate(AuthHelper.VALID_UUID); } @@ -879,9 +875,6 @@ class ProfileControllerTest { assertThat(profile.getBaseProfileResponse().getBadges()).hasSize(1).element(0).has(new Condition<>( badge -> "Test Badge".equals(badge.getName()), "has badge with expected name")); - verify(accountsManager, times(1)).getByAccountIdentifier(eq(AuthHelper.VALID_UUID_TWO)); - verify(profilesManager, times(1)).get(eq(AuthHelper.VALID_UUID_TWO), eq(version)); - verify(rateLimiter, times(1)).validate(AuthHelper.VALID_UUID); } @@ -1122,7 +1115,7 @@ class ProfileControllerTest { final ExpiringProfileKeyCredentialResponse credentialResponse = serverZkProfile.issueExpiringProfileKeyCredential(credentialRequest, new ServiceId.Aci(AuthHelper.VALID_UUID), profileKeyCommitment, expiration); - when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(account)); + when(accountsManager.getByServiceIdentifier(new AciServiceIdentifier(AuthHelper.VALID_UUID))).thenReturn(Optional.of(account)); when(profilesManager.get(AuthHelper.VALID_UUID, version)).thenReturn(Optional.of(versionedProfile)); when(zkProfileOperations.issueExpiringProfileKeyCredential(eq(credentialRequest), eq(new ServiceId.Aci(AuthHelper.VALID_UUID)), eq(profileKeyCommitment), any())) .thenReturn(credentialResponse); @@ -1182,7 +1175,7 @@ class ProfileControllerTest { when(account.isEnabled()).thenReturn(true); when(account.getUnidentifiedAccessKey()).thenReturn(Optional.of(UNIDENTIFIED_ACCESS_KEY)); - when(accountsManager.getByAccountIdentifier(AuthHelper.VALID_UUID)).thenReturn(Optional.of(account)); + when(accountsManager.getByServiceIdentifier(new AciServiceIdentifier(AuthHelper.VALID_UUID))).thenReturn(Optional.of(account)); when(profilesManager.get(AuthHelper.VALID_UUID, version)).thenReturn(Optional.of(versionedProfile)); when(zkProfileOperations.issueExpiringProfileKeyCredential(any(), any(), any(), any())) .thenThrow(new VerificationFailedException());