Handle Galley errors in federation RPCs

changelog.d/6-federation/handle-leave-conversation-errors

@@ -0,0 +1 @@
+Errors when leaving a conversation are now correctly handled instead of resulting in a generic federation error.

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

@@ -45,7 +45,6 @@ import Wire.API.Federation.Client (FederationClientFailure, FederatorClient)
 import Wire.API.Federation.Domain (OriginDomainHeader)
 import qualified Wire.API.Federation.GRPC.Types as Proto
 import Wire.API.Message (MessageNotSent, MessageSendingStatus, PostOtrResponse, Priority)
-import Wire.API.Routes.Public.Galley.Responses (RemoveFromConversationError)
 import Wire.API.User.Client (UserClientMap)
 import Wire.API.Util.Aeson (CustomEncoded (..))
 
@@ -214,6 +213,16 @@ data LeaveConversationRequest = LeaveConversationRequest
   deriving stock (Generic, Eq, Show)
   deriving (ToJSON, FromJSON) via (CustomEncoded LeaveConversationRequest)
 
+-- | Error outcomes of the leave-conversation RPC.
+data RemoveFromConversationError
+  = RemoveFromConversationErrorRemovalNotAllowed
+  | RemoveFromConversationErrorNotFound
+  | RemoveFromConversationErrorUnchanged
+  deriving stock (Eq, Show, Generic)
+  deriving
+    (ToJSON, FromJSON)
+    via (CustomEncoded RemoveFromConversationError)
+
 -- Note: this is parametric in the conversation type to allow it to be used
 -- both for conversations with a fixed known domain (e.g. as the argument of the
 -- federation RPC), and for conversations with an arbitrary Qualified or Remote id

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

@@ -47,10 +47,6 @@ spec =
       [ (LeaveConversationResponse.testObject_LeaveConversationResponse1, "testObject_LeaveConversationResponse1.json"),
         (LeaveConversationResponse.testObject_LeaveConversationResponse2, "testObject_LeaveConversationResponse2.json"),
         (LeaveConversationResponse.testObject_LeaveConversationResponse3, "testObject_LeaveConversationResponse3.json"),
-        (LeaveConversationResponse.testObject_LeaveConversationResponse4, "testObject_LeaveConversationResponse4.json"),
-        (LeaveConversationResponse.testObject_LeaveConversationResponse5, "testObject_LeaveConversationResponse5.json"),
-        (LeaveConversationResponse.testObject_LeaveConversationResponse6, "testObject_LeaveConversationResponse6.json"),
-        (LeaveConversationResponse.testObject_LeaveConversationResponse7, "testObject_LeaveConversationResponse7.json"),
         (LeaveConversationResponse.testObject_LeaveConversationResponse8, "testObject_LeaveConversationResponse8.json")
       ]
     testObjects

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

@@ -1,8 +1,7 @@
 module Test.Wire.API.Federation.Golden.LeaveConversationResponse where
 
 import Imports
-import Wire.API.Federation.API.Galley (LeaveConversationResponse (LeaveConversationResponse))
-import Wire.API.Routes.Public.Galley.Responses (RemoveFromConversationError (..))
+import Wire.API.Federation.API.Galley
 
 testObject_LeaveConversationResponse1 :: LeaveConversationResponse
 testObject_LeaveConversationResponse1 = LeaveConversationResponse $ Right ()
@@ -13,17 +12,5 @@ testObject_LeaveConversationResponse2 = LeaveConversationResponse $ Left RemoveF
 testObject_LeaveConversationResponse3 :: LeaveConversationResponse
 testObject_LeaveConversationResponse3 = LeaveConversationResponse $ Left RemoveFromConversationErrorNotFound
 
-testObject_LeaveConversationResponse4 :: LeaveConversationResponse
-testObject_LeaveConversationResponse4 = LeaveConversationResponse $ Left RemoveFromConversationErrorCustomRolesNotSupported
-
-testObject_LeaveConversationResponse5 :: LeaveConversationResponse
-testObject_LeaveConversationResponse5 = LeaveConversationResponse $ Left RemoveFromConversationErrorSelfConv
-
-testObject_LeaveConversationResponse6 :: LeaveConversationResponse
-testObject_LeaveConversationResponse6 = LeaveConversationResponse $ Left RemoveFromConversationErrorOne2OneConv
-
-testObject_LeaveConversationResponse7 :: LeaveConversationResponse
-testObject_LeaveConversationResponse7 = LeaveConversationResponse $ Left RemoveFromConversationErrorConnectConv
-
 testObject_LeaveConversationResponse8 :: LeaveConversationResponse
 testObject_LeaveConversationResponse8 = LeaveConversationResponse $ Left RemoveFromConversationErrorUnchanged

libs/wire-api/src/Wire/API/ErrorDescription.hs

@@ -294,12 +294,6 @@ type MissingLegalholdConsent =
     "Failed to connect to a user or to invite a user to a group because somebody \
     \is under legalhold and somebody else has not granted consent."
 
-type CustomRolesNotSupported =
-  ErrorDescription
-    400
-    "bad-request"
-    "Custom roles not supported"
-
 type InvalidOp desc =
   ErrorDescription
     403

libs/wire-api/src/Wire/API/Routes/Public/Galley.hs

@@ -39,7 +39,6 @@ import Wire.API.Event.Conversation
 import Wire.API.Message
 import Wire.API.Routes.MultiVerb
 import Wire.API.Routes.Public
-import Wire.API.Routes.Public.Galley.Responses
 import Wire.API.Routes.Public.Util
 import Wire.API.Routes.QualifiedCapture
 import Wire.API.ServantProto (Proto, RawProto)
@@ -72,6 +71,15 @@ type ConversationVerb =
 
 type ConvUpdateResponses = UpdateResponses "Conversation unchanged" "Conversation updated" Event
 
+type RemoveFromConversationVerb =
+  MultiVerb
+    'DELETE
+    '[JSON]
+    '[ RespondEmpty 204 "No change",
+       Respond 200 "Member removed" Event
+     ]
+    (Maybe Event)
+
 data Api routes = Api
   { -- Conversations
 
@@ -258,31 +266,27 @@ data Api routes = Api
         :- Summary "Remove a member from a conversation (deprecated)"
         :> ZLocalUser
         :> ZConn
+        :> CanThrow ConvNotFound
+        :> CanThrow (InvalidOp "Invalid operation")
         :> "conversations"
         :> Capture' '[Description "Conversation ID"] "cnv" ConvId
         :> "members"
         :> Capture' '[Description "Target User ID"] "usr" UserId
-        :> MultiVerb
-             'DELETE
-             '[JSON]
-             RemoveFromConversationHTTPResponse
-             RemoveFromConversationResponse,
+        :> RemoveFromConversationVerb,
     -- This endpoint can lead to the following events being sent:
     -- - MemberLeave event to members
     removeMember ::
       routes
         :- Summary "Remove a member from a conversation"
         :> ZLocalUser
         :> ZConn
+        :> CanThrow ConvNotFound
+        :> CanThrow (InvalidOp "Invalid operation")
         :> "conversations"
         :> QualifiedCapture' '[Description "Conversation ID"] "cnv" ConvId
         :> "members"
         :> QualifiedCapture' '[Description "Target User ID"] "usr" UserId
-        :> MultiVerb
-             'DELETE
-             '[JSON]
-             RemoveFromConversationHTTPResponse
-             RemoveFromConversationResponse,
+        :> RemoveFromConversationVerb,
     -- This endpoint can lead to the following events being sent:
     -- - MemberStateUpdate event to members
     updateOtherMemberUnqualified ::

libs/wire-api/wire-api.cabal

@@ -4,7 +4,7 @@ cabal-version: 1.12
 --
 -- see: https://github.com/sol/hpack
 --
--- hash: f7a6c4eafbaab04b23ffb79fcde736cd8d0894f91c6ef1eace2de5b98d9a47e5
+-- hash: fff87c53181cb0f2cd21a3da254e2a9a5dbae2ee00ea67ac124be1593a640e5e
 
 name:           wire-api
 version:        0.1.0
@@ -58,7 +58,6 @@ library
       Wire.API.Routes.Public
       Wire.API.Routes.Public.Brig
       Wire.API.Routes.Public.Galley
-      Wire.API.Routes.Public.Galley.Responses
       Wire.API.Routes.Public.LegalHold
       Wire.API.Routes.Public.Spar
       Wire.API.Routes.Public.Util

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

@@ -63,7 +63,6 @@ import Wire.API.Event.Conversation
 import Wire.API.Federation.API.Common (EmptyResponse (..))
 import qualified Wire.API.Federation.API.Galley as F
 import Wire.API.Routes.Internal.Brig.Connection
-import Wire.API.Routes.Public.Galley.Responses
 import Wire.API.ServantProto
 import Wire.API.User.Client (userClientMap)
 
@@ -232,12 +231,9 @@ addLocalUsersToRemoteConv remoteConvId qAdder localUsers = do
   E.createMembersInRemoteConversation remoteConvId connectedList
   pure connected
 
--- FUTUREWORK: actually return errors as part of the response instead of throwing
 leaveConversation ::
   Members
     '[ ConversationStore,
-       Error ActionError,
-       Error ConversationError,
        Error InvalidInput,
        ExternalAccess,
        FederatorAccess,
@@ -255,12 +251,23 @@ leaveConversation requestingDomain lc = do
   lcnv <- qualifyLocal (F.lcConvId lc)
   fmap F.LeaveConversationResponse
     . runError
-    . mapError @NoChanges (const RemoveFromConversationErrorUnchanged)
+    . mapError handleNoChanges
+    . mapError handleConvError
+    . mapError handleActionError
     . void
     . updateLocalConversation lcnv leaver Nothing
     . ConversationLeave
     . pure
     $ leaver
+  where
+    handleConvError :: ConversationError -> F.RemoveFromConversationError
+    handleConvError _ = F.RemoveFromConversationErrorNotFound
+
+    handleActionError :: ActionError -> F.RemoveFromConversationError
+    handleActionError _ = F.RemoveFromConversationErrorRemovalNotAllowed
+
+    handleNoChanges :: NoChanges -> F.RemoveFromConversationError
+    handleNoChanges _ = F.RemoveFromConversationErrorUnchanged
 
 -- FUTUREWORK: report errors to the originating backend
 -- FUTUREWORK: error handling for missing / mismatched clients