Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expose /conversations/{cnv}/members/v2 for federation backends #1543

Merged
merged 4 commits into from
Jun 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions charts/brig/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ data:
host: gundeck
port: 8080

{{- if .enableFederator }}
# TODO remove this
federator:
host: federator
Expand All @@ -54,6 +55,7 @@ data:
federatorInternal:
host: federator
port: 8080
{{- end }}

{{- with .aws }}
aws:
Expand Down
1 change: 1 addition & 0 deletions charts/brig/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ config:
# -- If set to false, 'dynamoDBEndpoint' _must_ be set.
randomPrekeys: true
useSES: true
enableFederator: false # keep enableFederator default in sync with galley chart's config.enableFederator as well as wire-server chart's tag.federator
emailSMS:
general:
templateBranding:
Expand Down
2 changes: 2 additions & 0 deletions charts/galley/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@ data:
host: spar
port: 8080

{{- if .enableFederator }}
federator:
host: federator
port: 8080
{{- end }}

{{- if (.journal) }}
journal:
Expand Down
1 change: 1 addition & 0 deletions charts/galley/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ config:
cassandra:
host: aws-cassandra
replicaCount: 3
enableFederator: false # keep enableFederator default in sync with brig chart's config.enableFederator as well as wire-server chart's tag.federator
settings:
maxTeamSize: 500
maxConvSize: 500
Expand Down
2 changes: 1 addition & 1 deletion charts/wire-server/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ tags:
team-settings: false
account-pages: false
legalhold: false
federator: false
federator: false # see also galley.config.enableFederator and brig.config.enableFederator
sftd: false
4 changes: 3 additions & 1 deletion hack/helm_vars/wire-server/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ tags:
cannon: true
cargohold: true
spar: true
federator: true
federator: true # also see galley.config.enableFederator and brig.config.enableFederator
proxy: false
webapp: false
team-settings: false
Expand Down Expand Up @@ -53,6 +53,7 @@ brig:
sessionTokenTimeout: 20
accessTokenTimeout: 30
providerTokenTimeout: 60
enableFederator: true # keep in sync with galley.config.enableFederator and tags.federator!
optSettings:
setActivationTimeout: 5
# keep this in sync with brigSettingsTeamInvitationTimeout in spar/templates/tests/configmap.yaml
Expand Down Expand Up @@ -140,6 +141,7 @@ galley:
cassandra:
host: cassandra-ephemeral
replicaCount: 1
enableFederator: true # keep in sync with brig.config.enableFederator and tags.federator!
settings:
maxConvAndTeamSize: 16
maxTeamSize: 32
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
47 changes: 41 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,42 @@ 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) can happen due to a
-- misconfiguration of federator. That should give a 500.
-- Port 1 should always be wrong hopefully.
let federatorUnavailable :: Opts = opts & optFederator ?~ Endpoint "127.0.0.1" 1
withSettingsOverrides federatorUnavailable $ do
postQualifiedMembers' g alice (remoteBob :| []) convId !!! do
const 500 === statusCode
-- FUTUREWORK: this should be a federation-not-available
-- but the error is hidden inside a server-error, confusingly.
-- separate task see cryptpad
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not refer to internal documents in public facing code?

--
-- Error {code = Status {statusCode = 500, statusMessage = "Internal
-- Server Error"}, label = "server-error", message =
-- "{\"code\":500,\"message\":\"Local federator not available:
-- Network.Socket.connect: <socket: 146>: does not exist (Connection
-- refused)Host: \\\"127.0.0.1\\\" Port:
-- 1\",\"label\":\"federation-not-available\"}"}
const (Just "server-error") === 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