Skip to content

Commit 12e265f

Browse files
eager-signaleager-signal
committed
Refine Profiles gRPC errors and documentation
1 parent 90c27f6 commit 12e265f

6 files changed

Lines changed: 119 additions & 20 deletions

File tree

‎service/src/main/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcHelper.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ static Optional<GetVersionedProfileResult> getVersionedProfile(final Account acc
7272
} else {
7373
responseBuilder.setDataEtag(DataEtag.newBuilder()
7474
.setData(ByteString.copyFrom(p.data()))
75-
.setEtag(ByteString.copyFrom(p.dataHash()))
75+
.setEtagSha256(ByteString.copyFrom(p.dataHash()))
7676
.build());
7777
}
7878
});
@@ -110,7 +110,7 @@ static Optional<GetVersionedProfileResult> getVersionedProfile(final Account acc
110110
} else {
111111
responseBuilder.setPaymentAddressDataEtag(DataEtag.newBuilder()
112112
.setData(ByteString.copyFrom(paymentAddress.get()))
113-
.setEtag(ByteString.copyFrom(
113+
.setEtagSha256(ByteString.copyFrom(
114114
profile.map(VersionedProfile::paymentAddressHash).orElseGet(() -> hash(paymentAddress.get()))))
115115
.build());
116116
}

‎service/src/main/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcService.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ public SetProfileResponse setProfile(final SetProfileRequest request) {
114114
: account.getCurrentProfileVersion().get().equals(expectedCurrentVersionHex);
115115

116116
if (!currentVersionMatchesExpected) {
117-
return SetProfileResponse.newBuilder().setWriteConflict(FailedPrecondition.newBuilder()
117+
return SetProfileResponse.newBuilder().setExpectedVersionWriteConflict(FailedPrecondition.newBuilder()
118118
.setDescription("current and expected profile versions must match")
119119
.build()).build();
120120
}
@@ -170,7 +170,7 @@ yield new AvatarData(currentAvatar, Optional.of(updateAvatarObjectName),
170170

171171
} catch (WriteConflictException _) {
172172
return SetProfileResponse.newBuilder()
173-
.setWriteConflict(FailedPrecondition.newBuilder()
173+
.setExpectedDataWriteConflict(FailedPrecondition.newBuilder()
174174
.setDescription("current and expected data hash mismatch")
175175
.build())
176176
.build();
@@ -189,7 +189,7 @@ yield new AvatarData(currentAvatar, Optional.of(updateAvatarObjectName),
189189

190190
} catch (final WriteConflictException _) {
191191
return SetProfileResponse.newBuilder()
192-
.setWriteConflict(FailedPrecondition.newBuilder()
192+
.setExpectedVersionWriteConflict(FailedPrecondition.newBuilder()
193193
.setDescription("current and expected version mismatch")
194194
.build())
195195
.build();

‎service/src/main/java/org/whispersystems/textsecuregcm/storage/ProfilesManager.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ public void setV1(UUID uuid, VersionedProfileV1 versionedProfile) {
104104
/// Transactionally sets v1 and v2 Profile data in DynamoDB.
105105
///
106106
/// Note: writes to the Redis cache are not transactional, and a failed DynamoDB write may leave incorrect data in Redis
107+
/// @throws WriteConflictException if the expected data hash does not match the stored data hash
107108
public void set(UUID uuid, VersionedProfileV1 versionedProfileV1, VersionedProfile versionedProfile, @Nullable byte[] expectedCurrentDataHash) throws WriteConflictException {
108109

109110
final TransactWriteItem v1TransactWriteItem = profilesV1.getTransactWriteItem(uuid, versionedProfileV1);

‎service/src/main/proto/org/signal/chat/profile.proto‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,10 @@ message SetProfileRequest {
8686
//
8787
// Optional, if there is no current profile version for the account.
8888
bytes expected_current_version = 4 [(require.exactlySize) = 0, (require.exactlySize) = 32];
89+
8990
// The ciphertext of the MobileCoin wallet ID on the profile.
9091
bytes payment_address = 5 [(require.exactlySize) = 0, (require.exactlySize) = 582];
92+
9193
// A list of badge IDs associated with the profile.
9294
repeated string badge_ids = 6;
9395

@@ -128,8 +130,15 @@ message SetProfileResult {
128130
message SetProfileResponse {
129131
oneof response {
130132
SetProfileResult result = 1;
131-
errors.FailedPrecondition write_conflict = 2 [(tag.reason) = "write_conflict"];
133+
// The current data hash did not match the request's expectation, indicating
134+
// another device on the account may have written an update the caller does not know about.
135+
errors.FailedPrecondition expected_data_write_conflict = 2 [(tag.reason) = "expected_data_write_conflict"];
136+
// Payments are not permitted in the account's region, based on its phone number.
137+
// The request should be retried without `payment_address.
132138
PaymentsForbiddenInRegion payments_forbidden_in_region = 3 [(tag.reason) = "payments_forbidden_in_region"];
139+
// The current version did not match the request's expectation, indicating
140+
// another device on the account may have created a new version the caller does not know about.
141+
errors.FailedPrecondition expected_version_write_conflict = 4 [(tag.reason) = "expected_version_write_conflict"];
133142

134143
// Because this is a temporary field during the migration, it has the highest
135144
// field number without serialization overhead. This is purely aesthetic.
@@ -167,7 +176,10 @@ message DataEtag {
167176
bytes data = 1;
168177
// An entity tag for `data`. This may be used in subsequent requests for
169178
// this profile version to optimize bandwidth.
170-
bytes etag = 2;
179+
//
180+
// Clients may validate that the value has been correctly calculated by deriving
181+
// the SHA-256 digest of `data`.
182+
bytes etag_sha256 = 2;
171183
}
172184

173185

@@ -301,13 +313,6 @@ message GetExpiringProfileKeyCredentialResult {
301313
bytes profile_key_credential = 1;
302314
}
303315

304-
message GetExpiringProfileKeyCredentialResponse {
305-
oneof response {
306-
GetExpiringProfileKeyCredentialResult result = 1;
307-
errors.NotFound not_found = 2 [(tag.reason) = "not_found"];
308-
}
309-
}
310-
311316
message GetExpiringProfileKeyCredentialAnonymousResponse {
312317
oneof response {
313318
GetExpiringProfileKeyCredentialResult result = 1;

‎service/src/test/java/org/whispersystems/textsecuregcm/grpc/ProfileAnonymousGrpcServiceTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ void getVersionedProfile(final byte[] requestVersion,
404404
if (expectResponseHasPaymentAddress) {
405405
expectedResultBuilder.setPaymentAddressDataEtag(DataEtag.newBuilder()
406406
.setData(ByteString.copyFrom(paymentAddress))
407-
.setEtag(ByteString.copyFrom(ProfileGrpcHelper.hash(paymentAddress)))
407+
.setEtagSha256(ByteString.copyFrom(ProfileGrpcHelper.hash(paymentAddress)))
408408
.build());
409409
}
410410

@@ -456,10 +456,10 @@ void getVersionedProfileV2() {
456456
final GetVersionedProfileResult result = response.getResult();
457457
assertTrue(result.hasDataEtag());
458458
assertEquals(ByteString.copyFrom(data), result.getDataEtag().getData());
459-
assertEquals(ByteString.copyFrom(v2Profile.dataHash()), result.getDataEtag().getEtag());
459+
assertEquals(ByteString.copyFrom(v2Profile.dataHash()), result.getDataEtag().getEtagSha256());
460460
assertTrue(result.hasPaymentAddressDataEtag());
461461
assertEquals(ByteString.copyFrom(paymentAddress), result.getPaymentAddressDataEtag().getData());
462-
assertEquals(ByteString.copyFrom(Objects.requireNonNull(v2Profile.paymentAddressHash())), result.getPaymentAddressDataEtag().getEtag());
462+
assertEquals(ByteString.copyFrom(Objects.requireNonNull(v2Profile.paymentAddressHash())), result.getPaymentAddressDataEtag().getEtagSha256());
463463
assertFalse(result.getDataEtagMatched());
464464
assertFalse(result.getPaymentAddressEtagMatched());
465465
assertFalse(result.hasV1Response());

‎service/src/test/java/org/whispersystems/textsecuregcm/grpc/ProfileGrpcServiceTest.java‎

Lines changed: 96 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,99 @@ void setProfile() throws Exception {
265265
assertThat(v1Profile.phoneNumberSharing()).isEqualTo(validPhoneNumberSharing);
266266
}
267267

268+
@Test
269+
void setProfileExpectedDataWriteConflict() throws Exception {
270+
final byte[] commitment = new ProfileKey(new byte[32]).getCommitment(new ServiceId.Aci(AUTHENTICATED_ACI)).serialize();
271+
final byte[] validAboutEmoji = new byte[60];
272+
final byte[] validAbout = new byte[540];
273+
final byte[] validPaymentAddress = new byte[582];
274+
final byte[] validPhoneNumberSharing = new byte[29];
275+
276+
doThrow(WriteConflictException.class)
277+
.when(profilesManager).set(any(), any(), any(), any());
278+
279+
final SetProfileRequest request = SetProfileRequest.newBuilder()
280+
.setVersion(ByteString.copyFrom(VERSION))
281+
.setData(ByteString.copyFrom(VALID_DATA))
282+
.setCommitment(ByteString.copyFrom(commitment)) // after v1 -> v2 migration, commitment can be null if expectedDataHash is present
283+
.setPaymentAddress(ByteString.copyFrom(validPaymentAddress))
284+
.setV1Request(V1_REQUEST.toBuilder()
285+
.setAvatarChange(AvatarChange.AVATAR_CHANGE_UNCHANGED)
286+
.setAboutEmoji(ByteString.copyFrom(validAboutEmoji))
287+
.setAbout(ByteString.copyFrom(validAbout))
288+
.setPhoneNumberSharing(ByteString.copyFrom(validPhoneNumberSharing))
289+
)
290+
.build();
291+
292+
final SetProfileResponse response = authenticatedServiceStub().setProfile(request);
293+
294+
assertTrue(response.hasExpectedDataWriteConflict());
295+
verify(accountsManager, never()).updateCurrentProfileVersion(any(), any(), any(), any());
296+
}
297+
298+
@Test
299+
void setProfileExpectedVersionWriteConflictBeforeUpdates() throws Exception {
300+
final byte[] commitment = new ProfileKey(new byte[32]).getCommitment(new ServiceId.Aci(AUTHENTICATED_ACI)).serialize();
301+
final byte[] validAboutEmoji = new byte[60];
302+
final byte[] validAbout = new byte[540];
303+
final byte[] validPaymentAddress = new byte[582];
304+
final byte[] validPhoneNumberSharing = new byte[29];
305+
306+
when(account.getCurrentProfileVersion()).thenReturn(
307+
Optional.of(HexFormat.of().formatHex(TestRandomUtil.nextBytes(32))));
308+
309+
final SetProfileRequest request = SetProfileRequest.newBuilder()
310+
.setVersion(ByteString.copyFrom(VERSION))
311+
.setData(ByteString.copyFrom(VALID_DATA))
312+
.setCommitment(ByteString.copyFrom(commitment)) // after v1 -> v2 migration, commitment can be null if expectedDataHash is present
313+
.setPaymentAddress(ByteString.copyFrom(validPaymentAddress))
314+
.setExpectedCurrentVersion(ByteString.copyFrom(VERSION))
315+
.setV1Request(V1_REQUEST.toBuilder()
316+
.setAvatarChange(AvatarChange.AVATAR_CHANGE_UNCHANGED)
317+
.setAboutEmoji(ByteString.copyFrom(validAboutEmoji))
318+
.setAbout(ByteString.copyFrom(validAbout))
319+
.setPhoneNumberSharing(ByteString.copyFrom(validPhoneNumberSharing))
320+
)
321+
.build();
322+
323+
final SetProfileResponse response = authenticatedServiceStub().setProfile(request);
324+
325+
assertTrue(response.hasExpectedVersionWriteConflict());
326+
verify(profilesManager, never()).set(any(), any(), any(), any());
327+
verify(accountsManager, never()).updateCurrentProfileVersion(any(), any(), any(), any());
328+
}
329+
330+
@Test
331+
void setProfileExpectedVersionWriteConflict() throws Exception {
332+
final byte[] commitment = new ProfileKey(new byte[32]).getCommitment(new ServiceId.Aci(AUTHENTICATED_ACI)).serialize();
333+
final byte[] validAboutEmoji = new byte[60];
334+
final byte[] validAbout = new byte[540];
335+
final byte[] validPaymentAddress = new byte[582];
336+
final byte[] validPhoneNumberSharing = new byte[29];
337+
338+
doThrow(WriteConflictException.class)
339+
.when(accountsManager).updateCurrentProfileVersion(any(), any(), any(), any());
340+
341+
final SetProfileRequest request = SetProfileRequest.newBuilder()
342+
.setVersion(ByteString.copyFrom(VERSION))
343+
.setData(ByteString.copyFrom(VALID_DATA))
344+
.setCommitment(ByteString.copyFrom(commitment)) // after v1 -> v2 migration, commitment can be null if expectedDataHash is present
345+
.setPaymentAddress(ByteString.copyFrom(validPaymentAddress))
346+
.setV1Request(V1_REQUEST.toBuilder()
347+
.setAvatarChange(AvatarChange.AVATAR_CHANGE_UNCHANGED)
348+
.setAboutEmoji(ByteString.copyFrom(validAboutEmoji))
349+
.setAbout(ByteString.copyFrom(validAbout))
350+
.setPhoneNumberSharing(ByteString.copyFrom(validPhoneNumberSharing))
351+
)
352+
.build();
353+
354+
final SetProfileResponse response = authenticatedServiceStub().setProfile(request);
355+
356+
assertTrue(response.hasExpectedVersionWriteConflict());
357+
verify(profilesManager).set(any(), any(), any(), any());
358+
verify(accountsManager).updateCurrentProfileVersion(any(), any(), any(), any());
359+
}
360+
268361
@Test
269362
void setProfileWithoutCapability() {
270363
when(account.hasCapability(DeviceCapability.PROFILES_V2)).thenReturn(false);
@@ -640,7 +733,7 @@ void getVersionedProfile(final byte[] requestVersion, @Nullable final byte[] acc
640733
if (expectResponseHasPaymentAddress) {
641734
expectedResultBuilder.setPaymentAddressDataEtag(
642735
DataEtag.newBuilder().setData(ByteString.copyFrom(paymentAddress))
643-
.setEtag(ByteString.copyFrom(ProfileGrpcHelper.hash(paymentAddress)))
736+
.setEtagSha256(ByteString.copyFrom(ProfileGrpcHelper.hash(paymentAddress)))
644737
.build());
645738

646739
}
@@ -693,10 +786,10 @@ void getVersionedProfileV2() {
693786

694787
assertTrue(result.hasDataEtag());
695788
assertEquals(ByteString.copyFrom(data), result.getDataEtag().getData());
696-
assertEquals(ByteString.copyFrom(v2Profile.dataHash()), result.getDataEtag().getEtag());
789+
assertEquals(ByteString.copyFrom(v2Profile.dataHash()), result.getDataEtag().getEtagSha256());
697790
assertTrue(result.hasPaymentAddressDataEtag());
698791
assertEquals(ByteString.copyFrom(paymentAddress), result.getPaymentAddressDataEtag().getData());
699-
assertEquals(ByteString.copyFrom(v2Profile.paymentAddressHash()), result.getPaymentAddressDataEtag().getEtag());
792+
assertEquals(ByteString.copyFrom(v2Profile.paymentAddressHash()), result.getPaymentAddressDataEtag().getEtagSha256());
700793
assertFalse(result.getDataEtagMatched());
701794
assertFalse(result.getPaymentAddressEtagMatched());
702795
assertFalse(result.hasV1Response());

0 commit comments

Comments
 (0)