Include conv creator is only once in notifications sent to remotes

changelog.d/6-federation/ensure-one-creator-member

@@ -0,0 +1 @@
+Ensure that the conversation creator is included only once in notifications sent to remote users

libs/wire-api-federation/src/Wire/API/Federation/API/Galley.hs

@@ -163,8 +163,8 @@ data NewRemoteConversation conv = NewRemoteConversation
     rcCnvAccessRole :: AccessRole,
     -- | The conversation name,
     rcCnvName :: Maybe Text,
-    -- | Members of the conversation
-    rcMembers :: Set OtherMember,
+    -- | Members of the conversation apart from the creator
+    rcNonCreatorMembers :: Set OtherMember,
     rcMessageTimer :: Maybe Milliseconds,
     rcReceiptMode :: Maybe ReceiptMode
   }

libs/wire-api-federation/test/Test/Wire/API/Federation/Golden/GoldenSpec.hs

@@ -25,6 +25,7 @@ import qualified Test.Wire.API.Federation.Golden.LeaveConversationResponse as Le
 import qualified Test.Wire.API.Federation.Golden.MessageSendResponse as MessageSendResponse
 import qualified Test.Wire.API.Federation.Golden.NewConnectionRequest as NewConnectionRequest
 import qualified Test.Wire.API.Federation.Golden.NewConnectionResponse as NewConnectionResponse
+import qualified Test.Wire.API.Federation.Golden.NewRemoteConversation as NewRemoteConversation
 import Test.Wire.API.Federation.Golden.Runner (testObjects)
 
 spec :: Spec
@@ -62,3 +63,7 @@ spec =
         (NewConnectionResponse.testObject_NewConnectionResponse3, "testObject_NewConnectionResponse3.json"),
         (NewConnectionResponse.testObject_NewConnectionResponse4, "testObject_NewConnectionResponse4.json")
       ]
+    testObjects
+      [ (NewRemoteConversation.testObject_NewRemoteConversation1, "testObject_NewRemoteConversation1.json"),
+        (NewRemoteConversation.testObject_NewRemoteConversation2, "testObject_NewRemoteConversation2.json")
+      ]

libs/wire-api-federation/test/Test/Wire/API/Federation/Golden/NewRemoteConversation.hs

@@ -0,0 +1,67 @@
+module Test.Wire.API.Federation.Golden.NewRemoteConversation where
+
+import Data.Domain
+import Data.Id
+import Data.Misc
+import Data.Qualified
+import qualified Data.Set as Set
+import qualified Data.UUID as UUID
+import Imports
+import Wire.API.Conversation
+import Wire.API.Conversation.Role
+import Wire.API.Federation.API.Galley
+import Wire.API.Provider.Service
+
+testObject_NewRemoteConversation1 :: NewRemoteConversation ConvId
+testObject_NewRemoteConversation1 =
+  NewRemoteConversation
+    { rcTime = read "1864-04-12 12:22:43.673 UTC",
+      rcOrigUserId = Id (fromJust (UUID.fromString "eed9dea3-5468-45f8-b562-7ad5de2587d0")),
+      rcCnvId = Id (fromJust (UUID.fromString "d13dbe58-d4e3-450f-9c0c-1e632f548740")),
+      rcCnvType = RegularConv,
+      rcCnvAccess = [InviteAccess, CodeAccess],
+      rcCnvAccessRole = ActivatedAccessRole,
+      rcCnvName = Just "gossip",
+      rcNonCreatorMembers =
+        Set.fromList
+          [ OtherMember
+              { omQualifiedId =
+                  Qualified
+                    (read "50e6fff1-ffbd-4235-bc73-19c093433beb")
+                    (Domain "golden.example.com"),
+                omService = Nothing,
+                omConvRoleName = roleNameWireAdmin
+              },
+            OtherMember
+              { omQualifiedId =
+                  Qualified
+                    (read "6801e49b-918c-4eef-baed-f18522152fca")
+                    (Domain "golden.example.com"),
+                omService =
+                  Just
+                    ( ServiceRef
+                        { _serviceRefId = read "abfe2452-ed22-4f94-b4d4-765b989d7dbb",
+                          _serviceRefProvider = read "11b91f61-917e-489b-a268-60b881d08f06"
+                        }
+                    ),
+                omConvRoleName = roleNameWireMember
+              }
+          ],
+      rcMessageTimer = Just (Ms 1000),
+      rcReceiptMode = Just (ReceiptMode 42)
+    }
+
+testObject_NewRemoteConversation2 :: NewRemoteConversation ConvId
+testObject_NewRemoteConversation2 =
+  NewRemoteConversation
+    { rcTime = read "1864-04-12 12:22:43.673 UTC",
+      rcOrigUserId = Id (fromJust (UUID.fromString "eed9dea3-5468-45f8-b562-7ad5de2587d0")),
+      rcCnvId = Id (fromJust (UUID.fromString "d13dbe58-d4e3-450f-9c0c-1e632f548740")),
+      rcCnvType = One2OneConv,
+      rcCnvAccess = [],
+      rcCnvAccessRole = ActivatedAccessRole,
+      rcCnvName = Nothing,
+      rcNonCreatorMembers = Set.fromList [],
+      rcMessageTimer = Nothing,
+      rcReceiptMode = Nothing
+    }

libs/wire-api-federation/test/golden/testObject_NewRemoteConversation1.json

@@ -0,0 +1,38 @@
+{
+    "orig_user_id": "eed9dea3-5468-45f8-b562-7ad5de2587d0",
+    "time": "1864-04-12T12:22:43.673Z",
+    "cnv_access": [
+        "invite",
+        "code"
+    ],
+    "non_creator_members": [
+        {
+            "status": 0,
+            "conversation_role": "wire_admin",
+            "qualified_id": {
+                "domain": "golden.example.com",
+                "id": "50e6fff1-ffbd-4235-bc73-19c093433beb"
+            },
+            "id": "50e6fff1-ffbd-4235-bc73-19c093433beb"
+        },
+        {
+            "status": 0,
+            "service": {
+                "id": "abfe2452-ed22-4f94-b4d4-765b989d7dbb",
+                "provider": "11b91f61-917e-489b-a268-60b881d08f06"
+            },
+            "conversation_role": "wire_member",
+            "qualified_id": {
+                "domain": "golden.example.com",
+                "id": "6801e49b-918c-4eef-baed-f18522152fca"
+            },
+            "id": "6801e49b-918c-4eef-baed-f18522152fca"
+        }
+    ],
+    "cnv_access_role": "activated",
+    "cnv_type": 0,
+    "receipt_mode": 42,
+    "message_timer": 1000,
+    "cnv_name": "gossip",
+    "cnv_id": "d13dbe58-d4e3-450f-9c0c-1e632f548740"
+}

libs/wire-api-federation/test/golden/testObject_NewRemoteConversation2.json

@@ -0,0 +1,12 @@
+{
+    "orig_user_id": "eed9dea3-5468-45f8-b562-7ad5de2587d0",
+    "time": "1864-04-12T12:22:43.673Z",
+    "cnv_access": [],
+    "non_creator_members": [],
+    "cnv_access_role": "activated",
+    "cnv_type": 2,
+    "receipt_mode": null,
+    "message_timer": null,
+    "cnv_name": null,
+    "cnv_id": "d13dbe58-d4e3-450f-9c0c-1e632f548740"
+}

libs/wire-api-federation/wire-api-federation.cabal

@@ -4,7 +4,7 @@ cabal-version: 1.12
 --
 -- see: https://github.com/sol/hpack
 --
--- hash: 03f7245b036ccc38819ed5f5654dae8d96b7ec5917b2f898be3305193bc3faf5
+-- hash: 6b4b223086cf1ba879d202fe6b884009e44bff247db6de7a2501d599295b8457
 
 name:           wire-api-federation
 version:        0.1.0
@@ -84,6 +84,7 @@ test-suite spec
       Test.Wire.API.Federation.Golden.MessageSendResponse
       Test.Wire.API.Federation.Golden.NewConnectionRequest
       Test.Wire.API.Federation.Golden.NewConnectionResponse
+      Test.Wire.API.Federation.Golden.NewRemoteConversation
       Test.Wire.API.Federation.Golden.Runner
       Test.Wire.API.Federation.GRPC.TypesSpec
       Paths_wire_api_federation

services/galley/src/Galley/API/Federation.hs

@@ -83,7 +83,7 @@ onConversationCreated :: Domain -> NewRemoteConversation ConvId -> Galley ()
 onConversationCreated domain rc = do
   let qrc = fmap (toRemoteUnsafe domain) rc
   loc <- qualifyLocal ()
-  let (localUserIds, _) = partitionQualified loc (map omQualifiedId (toList (rcMembers rc)))
+  let (localUserIds, _) = partitionQualified loc (map omQualifiedId (toList (rcNonCreatorMembers rc)))
 
   addedUserIds <-
     addLocalUsersToRemoteConv
@@ -99,9 +99,9 @@ onConversationCreated domain rc = do
               (const True)
               . omQualifiedId
           )
-          (rcMembers rc)
+          (rcNonCreatorMembers rc)
   -- Make sure to notify only about local users connected to the adder
-  let qrcConnected = qrc {rcMembers = connectedMembers}
+  let qrcConnected = qrc {rcNonCreatorMembers = connectedMembers}
 
   forM_ (fromNewRemoteConversation loc qrcConnected) $ \(mem, c) -> do
     let event =

services/galley/src/Galley/API/Util.hs

@@ -690,7 +690,7 @@ toNewRemoteConversation now localDomain Data.Conversation {..} =
       rcCnvAccess = convAccess,
       rcCnvAccessRole = convAccessRole,
       rcCnvName = convName,
-      rcMembers = toMembers convLocalMembers convRemoteMembers,
+      rcNonCreatorMembers = toMembers (filter (\lm -> lmId lm /= convCreator) convLocalMembers) convRemoteMembers,
       rcMessageTimer = convMessageTimer,
       rcReceiptMode = convReceiptMode
     }
@@ -714,7 +714,7 @@ fromNewRemoteConversation ::
   NewRemoteConversation (Remote ConvId) ->
   [(Public.Member, Public.Conversation)]
 fromNewRemoteConversation loc rc@NewRemoteConversation {..} =
-  let membersView = fmap (second Set.toList) . setHoles $ rcMembers
+  let membersView = fmap (second Set.toList) . setHoles $ rcNonCreatorMembers
       creatorOther =
         OtherMember
           (qUntagged (rcRemoteOrigUserId rc))

services/galley/test/integration/API.hs

@@ -125,6 +125,7 @@ tests s =
         [ test s "status" status,
           test s "metrics" metrics,
           test s "create conversation" postConvOk,
+          test s "create conversation with remote users" postConvWithRemoteUsersOk,
           test s "get empty conversations" getConvsOk,
           test s "get conversations by ids" getConvsOk2,
           test s "fail to get >500 conversations" getConvsFailMaxSize,
@@ -305,6 +306,73 @@ postConvOk = do
         EdConversation c' -> assertConvEquals cnv c'
         _ -> assertFailure "Unexpected event data"
 
+postConvWithRemoteUsersOk :: TestM ()
+postConvWithRemoteUsersOk = do
+  c <- view tsCannon
+  (alice, qAlice) <- randomUserTuple
+  (alex, qAlex) <- randomUserTuple
+  (amy, qAmy) <- randomUserTuple
+  connectUsers alice (list1 alex [amy])
+  let cDomain = Domain "c.example.com"
+      dDomain = Domain "d.example.com"
+  qChad <- randomQualifiedId cDomain
+  qCharlie <- randomQualifiedId cDomain
+  qDee <- randomQualifiedId dDomain
+  mapM_ (connectWithRemoteUser alice) [qChad, qCharlie, qDee]
+
+  -- Ensure name is within range, max size is 256
+  postConvQualified alice defNewConv {newConvName = Just (T.replicate 257 "a"), newConvQualifiedUsers = [qAlex, qAmy, qChad, qCharlie, qDee]}
+    !!! const 400 === statusCode
+
+  let nameMaxSize = T.replicate 256 "a"
+  WS.bracketR3 c alice alex amy $ \(wsAlice, wsAlex, wsAmy) -> do
+    (rsp, federatedRequests) <-
+      withTempMockFederator (const ()) $
+        postConvQualified alice defNewConv {newConvName = Just nameMaxSize, newConvQualifiedUsers = [qAlex, qAmy, qChad, qCharlie, qDee]}
+          <!! const 201 === statusCode
+    cid <- assertConvQualified rsp RegularConv alice qAlice [qAlex, qAmy, qChad, qCharlie, qDee] (Just nameMaxSize) Nothing
+    cvs <- mapM (convView cid) [alice, alex, amy]
+    liftIO $ mapM_ WS.assertSuccess =<< Async.mapConcurrently (checkWs qAlice) (zip cvs [wsAlice, wsAlex, wsAmy])
+
+    cFedReq <- assertOne $ filter (\r -> F.domain r == domainText cDomain) federatedRequests
+    cFedReqBody <- assertRight $ parseFedReqBody cFedReq
+
+    dFedReq <- assertOne $ filter (\r -> F.domain r == domainText dDomain) federatedRequests
+    dFedReqBody <- assertRight $ parseFedReqBody dFedReq
+
+    liftIO $ do
+      length federatedRequests @?= 2
+
+      FederatedGalley.rcOrigUserId cFedReqBody @?= alice
+      FederatedGalley.rcCnvId cFedReqBody @?= cid
+      FederatedGalley.rcCnvType cFedReqBody @?= RegularConv
+      FederatedGalley.rcCnvAccess cFedReqBody @?= [InviteAccess]
+      FederatedGalley.rcCnvAccessRole cFedReqBody @?= ActivatedAccessRole
+      FederatedGalley.rcCnvName cFedReqBody @?= Just nameMaxSize
+      FederatedGalley.rcNonCreatorMembers cFedReqBody @?= Set.fromList (toOtherMember <$> [qAlex, qAmy, qChad, qCharlie, qDee])
+      FederatedGalley.rcMessageTimer cFedReqBody @?= Nothing
+      FederatedGalley.rcReceiptMode cFedReqBody @?= Nothing
+
+      dFedReqBody @?= cFedReqBody
+  where
+    parseFedReqBody :: FromJSON a => F.FederatedRequest -> Either String a
+    parseFedReqBody fr =
+      case F.request fr of
+        Just r ->
+          (eitherDecode . cs) (F.body r)
+        Nothing -> Left "No request"
+    toOtherMember qid = OtherMember qid Nothing roleNameWireAdmin
+    convView cnv usr = responseJsonUnsafeWithMsg "conversation" <$> getConv usr cnv
+    checkWs qalice (cnv, ws) = WS.awaitMatch (5 # Second) ws $ \n -> do
+      ntfTransient n @?= False
+      let e = List1.head (WS.unpackPayload n)
+      evtConv e @?= cnvQualifiedId cnv
+      evtType e @?= ConvCreate
+      evtFrom e @?= qalice
+      case evtData e of
+        EdConversation c' -> assertConvEquals cnv c'
+        _ -> assertFailure "Unexpected event data"
+
 -- | This test verifies whether a message actually gets sent all the way to
 -- cannon.
 postCryptoMessage1 :: TestM ()
@@ -3065,7 +3133,7 @@ removeUser = do
             FederatedGalley.rcCnvAccess = [],
             FederatedGalley.rcCnvAccessRole = PrivateAccessRole,
             FederatedGalley.rcCnvName = Just "gossip4",
-            FederatedGalley.rcMembers = Set.fromList $ createOtherMember <$> [dee, alice, bob],
+            FederatedGalley.rcNonCreatorMembers = Set.fromList $ createOtherMember <$> [alice, bob],
             FederatedGalley.rcMessageTimer = Nothing,
             FederatedGalley.rcReceiptMode = Nothing
           }

services/galley/test/integration/API/Util.hs

@@ -1257,7 +1257,7 @@ registerRemoteConv convId originUser name othMembers = do
           rcCnvAccess = [],
           rcCnvAccessRole = ActivatedAccessRole,
           rcCnvName = name,
-          rcMembers = othMembers,
+          rcNonCreatorMembers = othMembers,
           rcMessageTimer = Nothing,
           rcReceiptMode = Nothing
         }
@@ -1347,6 +1347,54 @@ assertConvWithRole r t c s us n mt role = do
       _ -> return ()
   return cId
 
+assertConvQualified ::
+  HasCallStack =>
+  Response (Maybe Lazy.ByteString) ->
+  ConvType ->
+  UserId ->
+  Qualified UserId ->
+  [Qualified UserId] ->
+  Maybe Text ->
+  Maybe Milliseconds ->
+  TestM ConvId
+assertConvQualified r t c s us n mt = assertConvQualifiedWithRole r t c s us n mt roleNameWireAdmin
+
+assertConvQualifiedWithRole ::
+  HasCallStack =>
+  Response (Maybe Lazy.ByteString) ->
+  ConvType ->
+  UserId ->
+  Qualified UserId ->
+  [Qualified UserId] ->
+  Maybe Text ->
+  Maybe Milliseconds ->
+  RoleName ->
+  TestM ConvId
+assertConvQualifiedWithRole r t c s us n mt role = do
+  cId <- fromBS $ getHeader' "Location" r
+  let cnv = responseJsonMaybe @Conversation r
+  let _self = cmSelf . cnvMembers <$> cnv
+  let others = cmOthers . cnvMembers <$> cnv
+  liftIO $ do
+    assertEqual "id" (Just cId) (qUnqualified . cnvQualifiedId <$> cnv)
+    assertEqual "name" n (cnv >>= cnvName)
+    assertEqual "type" (Just t) (cnvType <$> cnv)
+    assertEqual "creator" (Just c) (cnvCreator <$> cnv)
+    assertEqual "message_timer" (Just mt) (cnvMessageTimer <$> cnv)
+    assertEqual "self" (Just s) (memId <$> _self)
+    assertEqual "others" (Just . Set.fromList $ us) (Set.fromList . map omQualifiedId . toList <$> others)
+    assertEqual "creator is always and admin" (Just roleNameWireAdmin) (memConvRoleName <$> _self)
+    assertBool "others role" (all (== role) $ maybe (error "Cannot be null") (map omConvRoleName . toList) others)
+    assertBool "otr muted ref not empty" (isNothing (memOtrMutedRef =<< _self))
+    assertBool "otr archived not false" (Just False == (memOtrArchived <$> _self))
+    assertBool "otr archived ref not empty" (isNothing (memOtrArchivedRef =<< _self))
+    case t of
+      SelfConv -> assertEqual "access" (Just privateAccess) (cnvAccess <$> cnv)
+      ConnectConv -> assertEqual "access" (Just privateAccess) (cnvAccess <$> cnv)
+      One2OneConv -> assertEqual "access" (Just privateAccess) (cnvAccess <$> cnv)
+      _ -> return ()
+  return cId
+
 wsAssertOtr :: Qualified ConvId -> Qualified UserId -> ClientId -> ClientId -> Text -> Notification -> IO ()
 wsAssertOtr = wsAssertOtr' "data"