Avoid remote calls to get conversation when it is not found locally

CHANGELOG-draft.md

@@ -28,3 +28,4 @@ THIS FILE ACCUMULATES THE RELEASE NOTES FOR THE UPCOMING RELEASE.
 
 * Ensure clients only receive messages meant for them in remote convs (#1739)
 * Federator CA store and client credentials are now automatically reloaded (#1730)
+* Avoid remote calls to get conversation when it is not found locally (#1713)

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

@@ -101,15 +101,18 @@ getConversation zusr cnv = do
   localDomain <- viewFederationDomain
   if qDomain cnv == localDomain
     then getUnqualifiedConversation zusr (qUnqualified cnv)
-    else getRemoteConversation zusr (toRemote cnv)
-
-getRemoteConversation :: UserId -> Remote ConvId -> Galley Public.Conversation
-getRemoteConversation zusr remoteConvId = do
-  conversations <- getRemoteConversations zusr [remoteConvId]
-  case conversations of
-    [] -> throwErrorDescription convNotFound
-    [conv] -> pure conv
-    _convs -> throwM (federationUnexpectedBody "expected one conversation, got multiple")
+    else getRemoteConversation (toRemote cnv)
+  where
+    getRemoteConversation :: Remote ConvId -> Galley Public.Conversation
+    getRemoteConversation remoteConvId = do
+      foundConvs <- Data.remoteConversationIdOf zusr [remoteConvId]
+      unless (remoteConvId `elem` foundConvs) $
+        throwErrorDescription convNotFound
+      conversations <- getRemoteConversations zusr [remoteConvId]
+      case conversations of
+        [] -> throwErrorDescription convNotFound
+        [conv] -> pure conv
+        _convs -> throwM (federationUnexpectedBody "expected one conversation, got multiple")
 
 getRemoteConversations :: UserId -> [Remote ConvId] -> Galley [Public.Conversation]
 getRemoteConversations zusr remoteConvs = do

services/galley/test/integration/API.hs

@@ -153,7 +153,11 @@ tests s =
           test s "fail to add too many members" postTooManyMembersFail,
           test s "add remote members" testAddRemoteMember,
           test s "get conversations/:domain/:cnv - local" testGetQualifiedLocalConv,
+          test s "get conversations/:domain/:cnv - local, not found" testGetQualifiedLocalConvNotFound,
+          test s "get conversations/:domain/:cnv - local, not participating" testGetQualifiedLocalConvNotParticipating,
           test s "get conversations/:domain/:cnv - remote" testGetQualifiedRemoteConv,
+          test s "get conversations/:domain/:cnv - remote, not found" testGetQualifiedRemoteConvNotFound,
+          test s "get conversations/:domain/:cnv - remote, not found on remote" testGetQualifiedRemoteConvNotFoundOnRemote,
           test s "post list-conversations" testListRemoteConvs,
           test s "post conversations/list/v2" testBulkGetQualifiedConvs,
           test s "add non-existing remote members" testAddRemoteMemberFailure,
@@ -1843,6 +1847,24 @@ testGetQualifiedLocalConv = do
     assertEqual "conversation id" convId (cnvQualifiedId conv)
     assertEqual "conversation name" (Just "gossip") (cnvName conv)
 
+testGetQualifiedLocalConvNotFound :: TestM ()
+testGetQualifiedLocalConvNotFound = do
+  alice <- randomUser
+  localDomain <- viewFederationDomain
+  convId <- (`Qualified` localDomain) <$> randomId
+  getConvQualified alice convId !!! do
+    const 404 === statusCode
+    const (Right (Just "no-conversation")) === fmap (view (at "label")) . responseJsonEither @Object
+
+testGetQualifiedLocalConvNotParticipating :: TestM ()
+testGetQualifiedLocalConvNotParticipating = do
+  alice <- randomUser
+  bob <- randomUser
+  convId <- decodeQualifiedConvId <$> postConv bob [] (Just "gossip about alice") [] Nothing Nothing
+  getConvQualified alice convId !!! do
+    const 403 === statusCode
+    const (Just "access-denied") === view (at "label") . responseJsonUnsafe @Object
+
 testGetQualifiedRemoteConv :: TestM ()
 testGetQualifiedRemoteConv = do
   aliceQ <- randomQualifiedUser
@@ -1886,6 +1908,35 @@ testGetQualifiedRemoteConv = do
   -- Alice's membership data stored locally
   liftIO $ assertEqual "conversation" mockConversation conv
 
+testGetQualifiedRemoteConvNotFound :: TestM ()
+testGetQualifiedRemoteConvNotFound = do
+  aliceId <- randomUser
+  let remoteDomain = Domain "far-away.example.com"
+  remoteConvId <- (`Qualified` remoteDomain) <$> randomId
+  -- No need to mock federator as we don't expect a call to be made
+  getConvQualified aliceId remoteConvId !!! do
+    const 404 === statusCode
+    const (Just "no-conversation") === view (at "label") . responseJsonUnsafe @Object
+
+testGetQualifiedRemoteConvNotFoundOnRemote :: TestM ()
+testGetQualifiedRemoteConvNotFoundOnRemote = do
+  aliceQ <- randomQualifiedUser
+  let aliceId = qUnqualified aliceQ
+  bobId <- randomId
+  convId <- randomId
+  let remoteDomain = Domain "far-away.example.com"
+      bobQ = Qualified bobId remoteDomain
+      remoteConvId = Qualified convId remoteDomain
+      aliceAsOtherMember = OtherMember aliceQ Nothing roleNameWireAdmin
+
+  registerRemoteConv remoteConvId bobQ Nothing (Set.fromList [aliceAsOtherMember])
+
+  opts <- view tsGConf
+  void . withTempMockFederator opts remoteDomain (const (GetConversationsResponse [])) $ do
+    getConvQualified aliceId remoteConvId !!! do
+      const 404 === statusCode
+      const (Just "no-conversation") === view (at "label") . responseJsonUnsafe @Object
+
 testListRemoteConvs :: TestM ()
 testListRemoteConvs = do
   -- alice on local domain