Skip to content

Commit

Permalink
expose adding remote members to a conversation for (test-only) backen…
Browse files Browse the repository at this point in the history
…ds explicitly configured to allow federation
  • Loading branch information
jschaul committed May 27, 2021
1 parent 86edb37 commit 6e78733
Show file tree
Hide file tree
Showing 6 changed files with 50 additions and 18 deletions.
12 changes: 10 additions & 2 deletions libs/wire-api-federation/src/Wire/API/Federation/Error.hs
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,16 @@ federationRpcError msg =
federationUnavailable :: Text -> Wai.Error
federationUnavailable err =
Wai.Error
HTTP.status500
"federation-not-available"
-- FUTUREWORK: This can be changed back to a
-- status500 / "federation-not-available"
-- if we have a way of actually stating that federation should be turned
-- off. Right now, helm charts configure federator inside galley/brig by
-- default, but don't deploy federator by default. This is not ideal, as
-- clients can attempt to make requests involving remote users but then
-- would get a 500 which isn't expected - they may assume that they should
-- retry the request.
HTTP.status400
"federation-not-enabled"
("Local federator not available: " <> LT.fromStrict err)

federationRemoteError :: Proto.OutwardError -> Wai.Error
Expand Down
3 changes: 1 addition & 2 deletions libs/wire-api/src/Wire/API/Routes/Public/Galley.hs
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,9 @@ data Api routes = Api
:> UVerb 'POST '[Servant.JSON] ConversationResponses,
addMembersToConversationV2 ::
routes
:- Summary "Add qualified members to an existing conversation: WIP, inaccessible for clients until ready"
:- Summary "Add qualified members to an existing conversation: WIP, events not propagated yet."
:> ZUser
:> ZConn
:> "i" -- FUTUREWORK: remove this /i/ once it's ready. See comment on 'Update.addMembers'
:> "conversations"
:> Capture "cnv" ConvId
:> "members"
Expand Down
8 changes: 6 additions & 2 deletions services/brig/test/integration/Federation/End2end.hs
Original file line number Diff line number Diff line change
Expand Up @@ -227,11 +227,15 @@ testAddRemoteUsersToLocalConv brig1 galley1 brig2 = do
let invite = InviteQualified (userQualifiedId bob :| []) roleNameWireAdmin
post
( galley1
-- FUTUREWORK: use an endpoint without /i/ once it's ready.
. paths ["i", "conversations", toByteString' convId, "members", "v2"]
. paths ["conversations", toByteString' convId, "members", "v2"]
. zUser (userId alice)
. zConn "conn"
. header "Z-Type" "access"
. json invite
)
!!! (const 200 === statusCode)

-- FUTUREWORK: check the happy path case as implementation of these things progresses:
-- - conversation can be queried and shows members (galley1)
-- - conversation can be queried and shows members (galley2 via qualified get conversation endpoint)
-- - this (qualified) convId pops up for both alice (on galley1) and bob (on galley2) when they request their own conversations ( GET /conversations )
5 changes: 1 addition & 4 deletions services/galley/src/Galley/API/Update.hs
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,7 @@ mapUpdateToServant :: UpdateResult -> Galley (Union UpdateResponses)
mapUpdateToServant (Updated e) = Servant.respond $ WithStatus @200 e
mapUpdateToServant Unchanged = Servant.respond NoContent

-- FUTUREWORK(federation): Before 'addMembers' in its current form can be made
-- public by exposing 'addMembersToConversationV2' (currently hidden using /i/
-- prefix), i.e. by allowing remote members to be actually added in any environment,
-- we need the following checks/implementation:
-- FUTUREWORK(federation): we need the following checks/implementation:
-- - (1) [DONE] Remote qualified users must exist before they can be added (a
-- call to the respective backend should be made): Avoid clients making up random
-- Ids, and increase the chances that the updateConversationMembership call
Expand Down
37 changes: 31 additions & 6 deletions services/galley/test/integration/API.hs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import Bilge hiding (timeout)
import Bilge.Assert
import Brig.Types
import qualified Control.Concurrent.Async as Async
import Control.Lens (at, view, (^.))
import Control.Lens (at, view, (.~), (?~), (^.))
import Data.Aeson hiding (json)
import Data.ByteString.Conversion
import qualified Data.Code as Code
Expand All @@ -50,6 +50,7 @@ import Data.Range
import qualified Data.Set as Set
import qualified Data.Text as T
import qualified Data.Text.Ascii as Ascii
import Galley.Options (Opts, optFederator)
import Galley.Types hiding (InternalMember (..))
import Galley.Types.Conversations.Roles
import qualified Galley.Types.Teams as Teams
Expand All @@ -62,6 +63,7 @@ import qualified Test.Tasty.Cannon as WS
import Test.Tasty.HUnit
import TestHelpers
import TestSetup
import Util.Options (Endpoint (Endpoint))
import Wire.API.Conversation.Member (Member (..))
import Wire.API.User.Client (UserClientPrekeyMap, getUserClientPrekeyMap)

Expand Down Expand Up @@ -116,6 +118,7 @@ tests s =
test s "add non-existing remote members" testAddRemoteMemberFailure,
test s "add deleted remote members" testAddDeletedRemoteUser,
test s "add remote members on invalid domain" testAddRemoteMemberInvalidDomain,
test s "add remote members when federation isn't enabled" testAddRemoteMemberFederationDisabled,
test s "remove members" deleteMembersOk,
test s "fail to remove members from self conv." deleteMembersFailSelf,
test s "fail to remove members from 1:1 conv." deleteMembersFailO2O,
Expand Down Expand Up @@ -867,11 +870,7 @@ leaveConnectConversation = do
let c = fromMaybe (error "invalid connect conversation") (cnvId <$> responseJsonUnsafe bdy)
deleteMember alice alice c !!! const 403 === statusCode

-- This test adds a non existent remote user to a local conversation and expects
-- a 200. This is of course not correct. When we implement a remote call, we
-- must mock it by mocking the federator and expecting a successful response
-- from the remote. Additionally, another test must be added to deal with error
-- scenarios of federation.
-- FUTUREWORK: Add more tests for scenarios of federation.
-- See also the comment in Galley.API.Update.addMembers for some other checks that are necessary.
testAddRemoteMember :: TestM ()
testAddRemoteMember = do
Expand Down Expand Up @@ -953,6 +952,32 @@ testAddRemoteMemberInvalidDomain = do
postQualifiedMembers alice (remoteBob :| []) convId
!!! const 422 === statusCode

-- This test is a safeguard to ensure adding remote members will fail
-- on environments where federation isn't configured (such as our production as of May 2021)
testAddRemoteMemberFederationDisabled :: TestM ()
testAddRemoteMemberFederationDisabled = do
g <- view tsGalley
alice <- randomUser
remoteBob <- flip Qualified (Domain "some-remote-backend.example.com") <$> randomId
convId <- decodeConvId <$> postConv alice [] (Just "remote gossip") [] Nothing Nothing
opts <- view tsGConf
-- federator endpoint not configured is equivalent to federation being disabled
-- This is the case on staging/production in May 2021.
let federatorNotConfigured :: Opts = opts & optFederator .~ Nothing
withSettingsOverrides federatorNotConfigured $ do
postQualifiedMembers' g alice (remoteBob :| []) convId !!! do
const 400 === statusCode
const (Just "federation-not-enabled") === fmap label . responseJsonUnsafe
-- federator endpoint being configured in brig and/or galley, but not being
-- available (i.e. no service listing on that IP/port) is the case for
-- kubernetes-based deployments that do not explicitly enable the federator
-- service.
let federatorUnavailable :: Opts = opts & optFederator ?~ Endpoint "127.0.0.1" 7000
withSettingsOverrides federatorUnavailable $ do
postQualifiedMembers' g alice (remoteBob :| []) convId !!! do
const 400 === statusCode
const (Just "federation-not-enabled") === fmap label . responseJsonUnsafe

postMembersOk :: TestM ()
postMembersOk = do
alice <- randomUser
Expand Down
3 changes: 1 addition & 2 deletions services/galley/test/integration/API/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -706,8 +706,7 @@ postQualifiedMembers' g zusr invitees conv = do
let invite = Public.InviteQualified invitees roleNameWireAdmin
post $
g
-- FUTUREWORK: use an endpoint without /i/ once it's ready.
. paths ["i", "conversations", toByteString' conv, "members", "v2"]
. paths ["conversations", toByteString' conv, "members", "v2"]
. zUser zusr
. zConn "conn"
. zType "access"
Expand Down

0 comments on commit 6e78733

Please sign in to comment.