include group id in signed actions

signalapp · Nov 20, 2024 · 7a47d23 · 7a47d23
1 parent 5074ed4
commit 7a47d23
Show file tree

Hide file tree

Showing 7 changed files with 122 additions and 28 deletions.
diff --git a/src/main/java/org/signal/storageservice/controllers/GroupsController.java b/src/main/java/org/signal/storageservice/controllers/GroupsController.java
@@ -478,7 +478,12 @@ public CompletableFuture<Response> modifyGroup(
         return CompletableFuture.completedFuture(Response.status(Response.Status.CONFLICT).entity(group.get()).build());
       }
 
+      if (!submittedActions.getGroupId().isEmpty()) {
+        throw new BadRequestException("requested actions must not set group id");
+      }
+
       Actions actions = submittedActions.toBuilder()
+                                        .setGroupId(user.getGroupId())
                                         .clearAddMembers()
                                         .addAllAddMembers(groupValidator.validateAddMember(user, inviteLinkPassword, group.get(), submittedActions.getAddMembersList()))
                                         .clearAddMembersPendingProfileKey()

diff --git a/src/main/proto/Groups.proto b/src/main/proto/Groups.proto
@@ -204,6 +204,9 @@ message GroupChange {
     }
 
     bytes sourceUuid = 1;
+    // clients should not provide this value; the server will provide it in the response buffer to ensure the signature is binding to a particular group
+    // if clients set it during a request the server will respond with 400.
+    bytes group_id = 25;
     uint32 version = 2;
 
     repeated AddMemberAction addMembers = 3;
@@ -228,7 +231,7 @@ message GroupChange {
     repeated AddMemberBannedAction add_members_banned = 22;  // change epoch = 4
     repeated DeleteMemberBannedAction delete_members_banned = 23;  // change epoch = 4
     repeated PromoteMemberPendingPniAciProfileKeyAction promote_members_pending_pni_aci_profile_key = 24;  // change epoch = 5
-    // next: 25
+    // next: 26
   }
 
   bytes actions = 1;

diff --git a/src/test/java/org/signal/storageservice/controllers/GroupsControllerBannedMembersTest.java b/src/test/java/org/signal/storageservice/controllers/GroupsControllerBannedMembersTest.java
@@ -139,6 +139,7 @@ public void testModifyGroupBanMember() throws Exception {
     setupGroupsManagerForWrites();
     Response response = modifyGroup(AuthHelper.VALID_USER_AUTH_CREDENTIAL, actionsBuilder);
 
+    actionsBuilder.setGroupId(ByteString.copyFrom(groupPublicParams.getGroupIdentifier().serialize()));
     actionsBuilder.getAddMembersBannedBuilder(0).getAddedBuilder().setTimestamp(clock.millis());
     groupBuilder.setVersion(1).addMembersBannedBuilder().setUserId(validUserThreeId).setTimestamp(clock.millis());
     assertThat(response.getStatus()).isEqualTo(200);
@@ -174,6 +175,7 @@ public void testModifyGroupUnbanMember() throws Exception {
     setupGroupsManagerForWrites();
     Response response = modifyGroup(AuthHelper.VALID_USER_AUTH_CREDENTIAL, actionsBuilder);
 
+    actionsBuilder.setGroupId(ByteString.copyFrom(groupPublicParams.getGroupIdentifier().serialize()));
     groupBuilder.setVersion(1).clearMembersBanned();
     assertThat(response.getStatus()).isEqualTo(200);
     verifyGroupModification(groupBuilder, actionsBuilder, 4, response, validUserId);
@@ -210,6 +212,7 @@ public void testModifyGroupUnbanMemberAsNonAdminWithOpenGroup() throws Exception
     setupGroupsManagerForWrites();
     Response response = modifyGroup(AuthHelper.VALID_USER_TWO_AUTH_CREDENTIAL, actionsBuilder);
 
+    actionsBuilder.setGroupId(ByteString.copyFrom(groupPublicParams.getGroupIdentifier().serialize()));
     groupBuilder.setVersion(1).clearMembersBanned();
     assertThat(response.getStatus()).isEqualTo(200);
     verifyGroupModification(groupBuilder, actionsBuilder, 4, response, validUserTwoId);

diff --git a/src/test/java/org/signal/storageservice/controllers/GroupsControllerInviteLinkTest.java b/src/test/java/org/signal/storageservice/controllers/GroupsControllerInviteLinkTest.java
@@ -84,9 +84,10 @@ void testModifyAddFromInviteLinkAccessControl() throws Exception {
             .isEqualTo(group);
 
     assertThat(signedChange).isEqualTo(changeCaptor.getValue());
+    assertThat(GroupChange.Actions.parseFrom(signedChange.getActions()).getGroupId()).isEqualTo(ByteString.copyFrom(groupPublicParams.getGroupIdentifier().serialize()));
     assertThat(GroupChange.Actions.parseFrom(signedChange.getActions()).getVersion()).isEqualTo(1);
     assertThat(GroupChange.Actions.parseFrom(signedChange.getActions()).getSourceUuid()).isEqualTo(ByteString.copyFrom(validUserPresentation.getUuidCiphertext().serialize()));
-    assertThat(GroupChange.Actions.parseFrom(signedChange.getActions()).toBuilder().clearSourceUuid().build()).isEqualTo(groupChange);
+    assertThat(GroupChange.Actions.parseFrom(signedChange.getActions()).toBuilder().clearGroupId().clearSourceUuid().build()).isEqualTo(groupChange);
 
     AuthHelper.GROUPS_SERVER_KEY.getPublicParams().verifySignature(signedChange.getActions().toByteArray(),
                                                                    new NotarySignature(signedChange.getServerSignature().toByteArray()));

diff --git a/...st/java/org/signal/storageservice/controllers/GroupsControllerPhoneNumberPrivacyTest.java b/...st/java/org/signal/storageservice/controllers/GroupsControllerPhoneNumberPrivacyTest.java
@@ -2,6 +2,7 @@
 
 import javax.ws.rs.client.Entity;
 import javax.ws.rs.core.Response;
+import com.google.protobuf.ByteString;
 import org.junit.jupiter.api.Test;
 import org.signal.storageservice.providers.ProtocolBufferMediaType;
 import org.signal.storageservice.storage.protos.groups.Group;
@@ -41,6 +42,7 @@ void testRejectInvitationFromPni() throws Exception {
         .header("Authorization", AuthHelper.getAuthHeader(groupSecretParams, AuthHelper.VALID_USER_THREE_AUTH_CREDENTIAL))
         .method("PATCH", Entity.entity(actionsBuilder.build().toByteArray(), ProtocolBufferMediaType.APPLICATION_PROTOBUF))) {
 
+      actionsBuilder.setGroupId(ByteString.copyFrom(groupPublicParams.getGroupIdentifier().serialize()));
       verifyGroupModification(groupBuilder, actionsBuilder, 0, response, validUserThreePniId);
     }
   }