Make upsertOne2OneConv always take a Conv ID

Brig can determine this ID based on protocol of the conversation or read it from the DB. Inventing this in galley causes more trouble for having two One2One convs for proteus and mls.
wireapp · Feb 19, 2024 · 0e7cade · 0e7cade
1 parent a300688
commit 0e7cade
Show file tree

Hide file tree

Showing 8 changed files with 39 additions and 73 deletions.
diff --git a/libs/wire-api/src/Wire/API/Routes/Internal/Galley.hs b/libs/wire-api/src/Wire/API/Routes/Internal/Galley.hs
@@ -257,7 +257,7 @@ type InternalAPIBase =
                :> "one2one"
                :> "upsert"
                :> ReqBody '[Servant.JSON] UpsertOne2OneConversationRequest
-               :> Post '[Servant.JSON] UpsertOne2OneConversationResponse
+               :> MultiVerb1 'POST '[Servant.JSON] (RespondEmpty 200 "Upsert One2One Policy")
            )
     :<|> IFeatureAPI
     :<|> IFederationAPI

diff --git a/libs/wire-api/src/Wire/API/Routes/Internal/Galley/ConversationsIntra.hs b/libs/wire-api/src/Wire/API/Routes/Internal/Galley/ConversationsIntra.hs
@@ -15,16 +15,9 @@
 -- You should have received a copy of the GNU Affero General Public License along
 -- with this program. If not, see <https://www.gnu.org/licenses/>.
 
-module Wire.API.Routes.Internal.Galley.ConversationsIntra
-  ( DesiredMembership (..),
-    Actor (..),
-    UpsertOne2OneConversationRequest (..),
-    UpsertOne2OneConversationResponse (..),
-  )
-where
+module Wire.API.Routes.Internal.Galley.ConversationsIntra where
 
-import Data.Aeson qualified as A
-import Data.Aeson.Types (FromJSON, ToJSON)
+import Data.Aeson (FromJSON, ToJSON)
 import Data.Id (ConvId, UserId)
 import Data.OpenApi qualified as Swagger
 import Data.Qualified
@@ -60,7 +53,7 @@ data UpsertOne2OneConversationRequest = UpsertOne2OneConversationRequest
     uooRemoteUser :: Remote UserId,
     uooActor :: Actor,
     uooActorDesiredMembership :: DesiredMembership,
-    uooConvId :: Maybe (Qualified ConvId)
+    uooConvId :: Qualified ConvId
   }
   deriving (Show, Generic)
   deriving (FromJSON, ToJSON, Swagger.ToSchema) via Schema UpsertOne2OneConversationRequest
@@ -73,16 +66,4 @@ instance ToSchema UpsertOne2OneConversationRequest where
         <*> (tUntagged . uooRemoteUser) .= field "remote_user" (qTagUnsafe <$> schema)
         <*> uooActor .= field "actor" schema
         <*> uooActorDesiredMembership .= field "actor_desired_membership" schema
-        <*> uooConvId .= optField "conversation_id" (maybeWithDefault A.Null schema)
-
-newtype UpsertOne2OneConversationResponse = UpsertOne2OneConversationResponse
-  { uuorConvId :: Qualified ConvId
-  }
-  deriving (Show, Generic)
-  deriving (FromJSON, ToJSON, Swagger.ToSchema) via Schema UpsertOne2OneConversationResponse
-
-instance ToSchema UpsertOne2OneConversationResponse where
-  schema =
-    object "UpsertOne2OneConversationResponse" $
-      UpsertOne2OneConversationResponse
-        <$> uuorConvId .= field "conversation_id" schema
+        <*> uooConvId .= field "conversation_id" schema
diff --git a/services/brig/src/Brig/API/Connection/Remote.hs b/services/brig/src/Brig/API/Connection/Remote.hs
@@ -37,6 +37,7 @@ import Control.Error.Util ((??))
 import Control.Monad.Trans.Except
 import Data.Id as Id
 import Data.Qualified
+import Galley.Types.Conversations.One2One (one2OneConvId)
 import Imports
 import Network.Wai.Utilities.Error
 import Polysemy
@@ -45,7 +46,7 @@ import Wire.API.Federation.API.Brig
   ( NewConnectionResponse (..),
     RemoteConnectionAction (..),
   )
-import Wire.API.Routes.Internal.Galley.ConversationsIntra (Actor (..), DesiredMembership (..), UpsertOne2OneConversationRequest (..), UpsertOne2OneConversationResponse (uuorConvId))
+import Wire.API.Routes.Internal.Galley.ConversationsIntra
 import Wire.API.Routes.Public.Util (ResponseForExistedCreated (..))
 import Wire.API.User
 import Wire.NotificationSubsystem
@@ -111,20 +112,20 @@ updateOne2OneConv ::
   Local UserId ->
   Maybe ConnId ->
   Remote UserId ->
-  Maybe (Qualified ConvId) ->
+  Qualified ConvId ->
   Relation ->
   Actor ->
-  (AppT r) (Qualified ConvId)
-updateOne2OneConv lUsr _mbConn remoteUser mbConvId rel actor = do
+  (AppT r) ()
+updateOne2OneConv lUsr _mbConn remoteUser convId rel actor = do
   let request =
         UpsertOne2OneConversationRequest
           { uooLocalUser = lUsr,
             uooRemoteUser = remoteUser,
             uooActor = actor,
             uooActorDesiredMembership = desiredMembership actor rel,
-            uooConvId = mbConvId
+            uooConvId = convId
           }
-  uuorConvId <$> wrapHttp (Intra.upsertOne2OneConversation request)
+  void $ wrapHttp (Intra.upsertOne2OneConversation request)
   where
     desiredMembership :: Actor -> Relation -> DesiredMembership
     desiredMembership a r =
@@ -162,7 +163,9 @@ transitionTo self _ _ Nothing Nothing _ =
   throwE (InvalidTransition (tUnqualified self))
 transitionTo self mzcon other Nothing (Just rel) actor = lift $ do
   -- update 1-1 connection
-  qcnv <- updateOne2OneConv self mzcon other Nothing rel actor
+  let proteusConv = (one2OneConvId BaseProtocolProteusTag (tUntagged self) (tUntagged other))
+  updateOne2OneConv self mzcon other proteusConv rel actor
+  -- void $ updateOne2OneConv self mzcon other BaseProtocolMLSTag  rel actor
 
   -- create connection
   connection <-
@@ -171,15 +174,19 @@ transitionTo self mzcon other Nothing (Just rel) actor = lift $ do
         self
         (tUntagged other)
         (relationWithHistory rel)
-        qcnv
+        proteusConv
 
   -- send event
   pushEvent self mzcon connection
   pure (Created connection, True)
 transitionTo _self _zcon _other (Just connection) Nothing _actor = pure (Existed connection, False)
 transitionTo self mzcon other (Just connection) (Just rel) actor = lift $ do
   -- update 1-1 conversation
-  void $ updateOne2OneConv self Nothing other (ucConvId connection) rel actor
+  let convId =
+        fromMaybe
+          (one2OneConvId BaseProtocolProteusTag (tUntagged self) (tUntagged other))
+          $ ucConvId connection
+  void $ updateOne2OneConv self Nothing other convId rel actor
 
   -- update connection
   connection' <- wrapClient $ Data.updateConnection connection (relationWithHistory rel)

diff --git a/services/brig/src/Brig/IO/Intra.hs b/services/brig/src/Brig/IO/Intra.hs
@@ -95,7 +95,7 @@ import Wire.API.Event.Conversation (Connect (Connect))
 import Wire.API.Federation.API.Brig
 import Wire.API.Federation.Error
 import Wire.API.Properties
-import Wire.API.Routes.Internal.Galley.ConversationsIntra (UpsertOne2OneConversationRequest, UpsertOne2OneConversationResponse)
+import Wire.API.Routes.Internal.Galley.ConversationsIntra
 import Wire.API.Routes.Internal.Galley.TeamsIntra (GuardLegalholdPolicyConflicts (GuardLegalholdPolicyConflicts))
 import Wire.API.Team.LegalHold (LegalholdProtectee)
 import Wire.API.Team.Member qualified as Team
@@ -717,11 +717,11 @@ upsertOne2OneConversation ::
     HasRequestId m
   ) =>
   UpsertOne2OneConversationRequest ->
-  m UpsertOne2OneConversationResponse
+  m ()
 upsertOne2OneConversation urequest = do
   response <- galleyRequest POST req
   case Bilge.statusCode response of
-    200 -> decodeBody "galley" response
+    200 -> pure ()
     _ -> throwM internalServerError
   where
     req =

diff --git a/services/galley/src/Galley/API/One2One.hs b/services/galley/src/Galley/API/One2One.hs
@@ -58,17 +58,8 @@ iUpsertOne2OneConversation ::
     Member MemberStore r
   ) =>
   UpsertOne2OneConversationRequest ->
-  Sem r UpsertOne2OneConversationResponse
+  Sem r ()
 iUpsertOne2OneConversation UpsertOne2OneConversationRequest {..} = do
-  let convId =
-        fromMaybe
-          ( one2OneConvId
-              BaseProtocolProteusTag
-              (tUntagged uooLocalUser)
-              (tUntagged uooRemoteUser)
-          )
-          uooConvId
-
   let dolocal :: Local ConvId -> Sem r ()
       dolocal lconvId = do
         mbConv <- getConversation (tUnqualified lconvId)
@@ -111,5 +102,5 @@ iUpsertOne2OneConversation UpsertOne2OneConversationRequest {..} = do
             deleteMembersInRemoteConversation rconvId [tUnqualified uooLocalUser]
           (RemoteActor, _) -> pure ()
 
-  foldQualified uooLocalUser dolocal doremote convId
-  pure (UpsertOne2OneConversationResponse convId)
+  foldQualified uooLocalUser dolocal doremote uooConvId
+  pure ()
diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs
@@ -3694,16 +3694,11 @@ testAllOne2OneConversationRequests = do
 testOne2OneConversationRequest :: Bool -> Actor -> DesiredMembership -> TestM ()
 testOne2OneConversationRequest shouldBeLocal actor desired = do
   alice <- qTagUnsafe <$> randomQualifiedUser
-  (bob, expectedConvId) <- generateRemoteAndConvId shouldBeLocal alice
+  (bob, convId) <- generateRemoteAndConvId shouldBeLocal alice
 
-  convId <- do
-    let req = UpsertOne2OneConversationRequest alice bob actor desired Nothing
-    res <-
-      iUpsertOne2OneConversation req
-        <!! statusCode === const 200
-    uuorConvId <$> responseJsonError res
-
-  liftIO $ convId @?= expectedConvId
+  do
+    let req = UpsertOne2OneConversationRequest alice bob actor desired convId
+    iUpsertOne2OneConversation req !!! statusCode === const 200
 
   if shouldBeLocal
     then

diff --git a/services/galley/test/integration/API/Federation.hs b/services/galley/test/integration/API/Federation.hs
@@ -120,12 +120,9 @@ getConversationsAllFound = do
               uooRemoteUser = rAlice,
               uooActor = LocalActor,
               uooActorDesiredMembership = Included,
-              uooConvId = Just cnv1Id
+              uooConvId = cnv1Id
             }
-    UpsertOne2OneConversationResponse cnv1IdReturned <-
-      responseJsonError
-        =<< iUpsertOne2OneConversation createO2O
-    liftIO $ assertEqual "Mismatch in the generated conversation ID" cnv1IdReturned cnv1Id
+    iUpsertOne2OneConversation createO2O !!! const 200 === statusCode
 
   do
     convs <-

diff --git a/services/galley/test/integration/API/Util.hs b/services/galley/test/integration/API/Util.hs
@@ -2895,23 +2895,18 @@ iUpsertOne2OneConversation req = do
 
 createOne2OneConvWithRemote :: HasCallStack => Local UserId -> Remote UserId -> TestM ()
 createOne2OneConvWithRemote localUser remoteUser = do
-  let mkRequest actor mConvId =
+  let convId = one2OneConvId BaseProtocolProteusTag (tUntagged localUser) (tUntagged remoteUser)
+      mkRequest actor =
         UpsertOne2OneConversationRequest
           { uooLocalUser = localUser,
             uooRemoteUser = remoteUser,
             uooActor = actor,
             uooActorDesiredMembership = Included,
-            uooConvId = mConvId
+            uooConvId = convId
           }
-  ooConvId <-
-    fmap uuorConvId
-      . responseJsonError
-      =<< iUpsertOne2OneConversation (mkRequest LocalActor Nothing)
-        <!! const 200
-          === statusCode
-  iUpsertOne2OneConversation (mkRequest RemoteActor (Just ooConvId))
-    !!! const 200
-      === statusCode
+
+  iUpsertOne2OneConversation (mkRequest LocalActor) !!! const 200 === statusCode
+  iUpsertOne2OneConversation (mkRequest RemoteActor) !!! const 200 === statusCode
 
 generateRemoteAndConvId :: Bool -> Local UserId -> TestM (Remote UserId, Qualified ConvId)
 generateRemoteAndConvId = generateRemoteAndConvIdWithDomain (Domain "far-away.example.com")