Leave domain empty in cookies, but account for clients with old cookies.

deploy/services-demo/conf/brig.demo-docker.yaml

@@ -102,7 +102,6 @@ optSettings:
   setActivationTimeout: 1209600     # 1 day
   setTeamInvitationTimeout: 1814400 # 21 days
   setUserMaxConnections: 1000
-  setCookieDomain: brig
   setCookieInsecure: false
   setUserCookieRenewAge: 1209600 # 14 days
   setUserCookieLimit: 32

deploy/services-demo/conf/brig.demo.yaml

@@ -102,7 +102,6 @@ optSettings:
   setActivationTimeout: 1209600     # 1 day
   setTeamInvitationTimeout: 1814400 # 21 days
   setUserMaxConnections: 1000
-  setCookieDomain: localhost
   setCookieInsecure: false
   setUserCookieRenewAge: 1209600 # 14 days
   setUserCookieLimit: 32

deploy/services-demo/conf/nginz/nginx.conf

@@ -290,6 +290,11 @@ http {
       proxy_pass http://galley;
     }
 
+    location ~* ^/teams/([^/]*)/legalhold(.*) {
+      include common_response_with_zauth.conf;
+      proxy_pass http://galley;
+    }
+
     # Gundeck Endpoints
 
     rewrite ^/api-docs/push  /push/api-docs?base_url=http://127.0.0.1:8080/ break;

services/brig/brig.integration.yaml

@@ -135,7 +135,6 @@ optSettings:
   setNexmo: test/resources/nexmo-credentials.yaml
   # setStomp: test/resources/stomp-credentials.yaml
   setUserMaxConnections: 16
-  setCookieDomain: 127.0.0.1
   setCookieInsecure: true
   setUserCookieRenewAge: 2
   setUserCookieLimit: 5

services/brig/src/Brig/Options.hs

@@ -411,9 +411,6 @@ data Settings = Settings
     -- | Max. number of permanent clients per user
     setUserMaxPermClients :: !(Maybe Int),
     -- | The domain to restrict cookies to
-    setCookieDomain :: !Text,
-    -- | Whether to allow plain HTTP transmission
-    --   of cookies (for testing purposes only)
     setCookieInsecure :: !Bool,
     -- | Minimum age of a user cookie before
     --   it is renewed during token refresh

services/brig/src/Brig/Provider/API.hs

@@ -1045,7 +1045,7 @@ setProviderCookie t r = do
       Cookie.def
         { Cookie.setCookieName = "zprovider",
           Cookie.setCookieValue = toByteString' t,
-          Cookie.setCookieDomain = Just $ Text.encodeUtf8 . setCookieDomain $ s,
+          Cookie.setCookieDomain = Nothing,
           Cookie.setCookiePath = Just "/provider",
           Cookie.setCookieExpires = Just (ZAuth.tokenExpiresUTC t),
           Cookie.setCookieSecure = not (setCookieInsecure s),

services/brig/src/Brig/User/API/Auth.hs

@@ -43,7 +43,7 @@ import Network.HTTP.Types.Status
 import Network.Wai (Response)
 import Network.Wai.Predicate
 import qualified Network.Wai.Predicate as P
-import Network.Wai.Predicate.Request
+import qualified Network.Wai.Predicate.Request as R
 import Network.Wai.Routing
 import Network.Wai.Utilities.Error ((!>>))
 import Network.Wai.Utilities.Request (JsonRequest, jsonRequest)
@@ -225,12 +225,12 @@ legalHoldLogin l = do
   let typ = PersistentCookie -- Session cookie isn't a supported use case here
   Auth.legalHoldLogin l typ !>> legalHoldLoginError
 
-logoutH :: JSON ::: Maybe (Either ZAuth.UserToken ZAuth.LegalHoldUserToken) ::: Maybe (Either ZAuth.AccessToken ZAuth.LegalHoldAccessToken) -> Handler Response
+logoutH :: JSON ::: Maybe (Either [ZAuth.UserToken] [ZAuth.LegalHoldUserToken]) ::: Maybe (Either ZAuth.AccessToken ZAuth.LegalHoldAccessToken) -> Handler Response
 logoutH (_ ::: ut ::: at) = empty <$ logout ut at
 
 -- TODO: add legalhold test checking cookies are revoked (/access/logout is called) when legalhold device is deleted.
 logout ::
-  Maybe (Either ZAuth.UserToken ZAuth.LegalHoldUserToken) ->
+  Maybe (Either [ZAuth.UserToken] [ZAuth.LegalHoldUserToken]) ->
   Maybe (Either ZAuth.AccessToken ZAuth.LegalHoldAccessToken) ->
   Handler ()
 logout Nothing Nothing = throwStd authMissingCookieAndToken
@@ -256,7 +256,7 @@ rmCookies :: UserId -> Public.RemoveCookies -> Handler ()
 rmCookies uid (Public.RemoveCookies pw lls ids) = do
   Auth.revokeAccess uid pw ids lls !>> authError
 
-renewH :: JSON ::: Maybe (Either ZAuth.UserToken ZAuth.LegalHoldUserToken) ::: Maybe (Either ZAuth.AccessToken ZAuth.LegalHoldAccessToken) -> Handler Response
+renewH :: JSON ::: Maybe (Either [ZAuth.UserToken] [ZAuth.LegalHoldUserToken]) ::: Maybe (Either ZAuth.AccessToken ZAuth.LegalHoldAccessToken) -> Handler Response
 renewH (_ ::: ut ::: at) = lift . either tokenResponse tokenResponse =<< renew ut at
 
 -- | renew access for either:
@@ -265,21 +265,21 @@ renewH (_ ::: ut ::: at) = lift . either tokenResponse tokenResponse =<< renew u
 --
 -- Other combinations of provided inputs will cause an error to be raised.
 renew ::
-  Maybe (Either ZAuth.UserToken ZAuth.LegalHoldUserToken) ->
+  Maybe (Either [ZAuth.UserToken] [ZAuth.LegalHoldUserToken]) ->
   Maybe (Either ZAuth.AccessToken ZAuth.LegalHoldAccessToken) ->
   Handler (Either (Auth.Access ZAuth.User) (Auth.Access ZAuth.LegalHoldUser))
 renew = \case
   Nothing ->
     const $ throwStd authMissingCookie
-  (Just (Left userToken)) ->
+  (Just (Left userTokens)) ->
     -- normal UserToken, so we want a normal AccessToken
-    fmap Left . renewAccess userToken <=< matchingOrNone leftToMaybe
-  (Just (Right legalholdUserToken)) ->
+    fmap Left . renewAccess userTokens <=< matchingOrNone leftToMaybe
+  (Just (Right legalholdUserTokens)) ->
     -- LegalholdUserToken, so we want a LegalholdAccessToken
-    fmap Right . renewAccess legalholdUserToken <=< matchingOrNone rightToMaybe
+    fmap Right . renewAccess legalholdUserTokens <=< matchingOrNone rightToMaybe
   where
-    renewAccess ut mat =
-      Auth.renewAccess ut mat !>> zauthError
+    renewAccess uts mat =
+      Auth.renewAccess uts mat !>> zauthError
     matchingOrNone :: (a -> Maybe b) -> Maybe a -> Handler (Maybe b)
     matchingOrNone matching = traverse $ \accessToken ->
       case matching accessToken of
@@ -292,24 +292,30 @@ renew = \case
 -- | A predicate that captures user and access tokens for a request handler.
 tokenRequest ::
   forall r.
-  (HasCookies r, HasHeaders r, HasQuery r) =>
+  (R.HasCookies r, R.HasHeaders r, R.HasQuery r) =>
   Predicate
     r
     P.Error
-    ( Maybe (Either ZAuth.UserToken ZAuth.LegalHoldUserToken)
+    ( Maybe (Either [ZAuth.UserToken] [ZAuth.LegalHoldUserToken])
         ::: Maybe (Either ZAuth.AccessToken ZAuth.LegalHoldAccessToken)
     )
 tokenRequest = opt (userToken ||| legalHoldUserToken) .&. opt (accessToken ||| legalHoldAccessToken)
   where
-    userToken = cookieErr @ZAuth.User <$> cookie "zuid"
-    legalHoldUserToken = cookieErr @ZAuth.LegalHoldUser <$> cookie "zuid"
+    userToken = cookieErr @ZAuth.User <$> cookies "zuid"
+    legalHoldUserToken = cookieErr @ZAuth.LegalHoldUser <$> cookies "zuid"
     accessToken = parse @ZAuth.Access <$> (tokenHeader .|. tokenQuery)
     legalHoldAccessToken = parse @ZAuth.LegalHoldAccess <$> (tokenHeader .|. tokenQuery)
+    --
+    tokenHeader :: r -> Result P.Error ByteString
     tokenHeader = bearer <$> header "authorization"
+    --
+    tokenQuery :: r -> Result P.Error ByteString
     tokenQuery = query "access_token"
-    cookieErr :: ZAuth.UserTokenLike u => Result P.Error (ZAuth.Token u) -> Result P.Error (ZAuth.Token u)
+    --
+    cookieErr :: ZAuth.UserTokenLike u => Result P.Error [ZAuth.Token u] -> Result P.Error [ZAuth.Token u]
     cookieErr x@Okay {} = x
     cookieErr (Fail x) = Fail (setMessage "Invalid user token" (P.setStatus status403 x))
+    --
     -- Extract the access token from the Authorization header.
     bearer :: Result P.Error ByteString -> Result P.Error ByteString
     bearer (Fail x) = Fail x
@@ -323,6 +329,7 @@ tokenRequest = opt (userToken ||| legalHoldUserToken) .&. opt (accessToken ||| l
                     TypeError
                     (setMessage "Invalid authorization scheme" (err status403))
                 )
+    --
     -- Parse the access token
     parse :: ZAuth.AccessTokenLike a => Result P.Error ByteString -> Result P.Error (ZAuth.Token a)
     parse (Fail x) = Fail x
@@ -338,3 +345,28 @@ tokenRequest = opt (userToken ||| legalHoldUserToken) .&. opt (accessToken ||| l
 tokenResponse :: ZAuth.UserTokenLike u => Auth.Access u -> AppIO Response
 tokenResponse (Auth.Access t Nothing) = pure $ json t
 tokenResponse (Auth.Access t (Just c)) = Auth.setResponseCookie c (json t)
+
+-- Utilities
+-- Internal: These functions are nearly copies verbatim from the original
+-- project: https://gitlab.com/twittner/wai-predicates/-/blob/develop/src/Network/Wai/Predicate.hs#L106-112
+-- I will still make an upstream PR but would not like to block this PR because of
+-- it. Main difference: the original stops after finding the first valid cookie which
+-- is a problem if clients send more than 1 cookie and one of them happens to be invalid
+-- We should also be dropping this in favor of servant which will make this redundant
+cookies :: (R.HasCookies r, FromByteString a) => ByteString -> Predicate r P.Error [a]
+cookies k r =
+  case R.lookupCookie k r of
+    [] -> Fail . addLabel "cookie" $ notAvailable k
+    cc ->
+      maybe
+        (Fail . addLabel "cookie" . typeError k $ "Failed to get zuid cookies")
+        return
+        (traverse fromByteString cc)
+
+notAvailable :: ByteString -> P.Error
+notAvailable k = e400 & setReason NotAvailable . setSource k
+{-# INLINE notAvailable #-}
+
+typeError :: ByteString -> ByteString -> P.Error
+typeError k m = e400 & setReason TypeError . setSource k . setMessage m
+{-# INLINE typeError #-}

services/brig/src/Brig/User/Auth.hs

@@ -152,18 +152,18 @@ withRetryLimit action uid = do
       BudgetExhausted ttl -> throwE . LoginBlocked . RetryAfter . floor $ ttl
       BudgetedValue () _ -> pure ()
 
-logout :: ZAuth.TokenPair u a => ZAuth.Token u -> ZAuth.Token a -> ExceptT ZAuth.Failure AppIO ()
-logout ut at = do
-  (u, ck) <- validateTokens ut (Just at)
+logout :: ZAuth.TokenPair u a => [ZAuth.Token u] -> ZAuth.Token a -> ExceptT ZAuth.Failure AppIO ()
+logout uts at = do
+  (u, ck) <- validateTokens uts (Just at)
   lift $ revokeCookies u [cookieId ck] []
 
 renewAccess ::
   ZAuth.TokenPair u a =>
-  ZAuth.Token u ->
+  [ZAuth.Token u] ->
   Maybe (ZAuth.Token a) ->
   ExceptT ZAuth.Failure AppIO (Access u)
-renewAccess ut at = do
-  (uid, ck) <- validateTokens ut at
+renewAccess uts at = do
+  (uid, ck) <- validateTokens uts at
   Log.debug $ field "user" (toByteString uid) . field "action" (Log.val "User.renewAccess")
   catchSuspendInactiveUser uid ZAuth.Expired
   ck' <- lift $ nextCookie ck
@@ -252,12 +252,34 @@ isPendingActivation ident = case ident of
             Just SSOIdentity {} -> False -- sso-created users are activated immediately.
             Nothing -> True
 
+-- | Validate a list of (User/LH) tokens potentially with an associated access token.
+--   If there are multiple valid cookies, we try all of them. When an access token is
+--   given, we perform the usual checks.
+--   If multiple cookies are given and several are valid, we return the first valid one.
 validateTokens ::
+  ZAuth.TokenPair u a =>
+  [ZAuth.Token u] -> -- FUTUREWORK: This should be a NonEmpty
+  Maybe (ZAuth.Token a) ->
+  ExceptT ZAuth.Failure AppIO (UserId, Cookie (ZAuth.Token u))
+validateTokens [] _ = throwE ZAuth.Invalid
+validateTokens (ut : []) at = validateToken ut at
+validateTokens uts at = do
+  tokens <- forM uts $ \ut -> lift $ runExceptT (validateToken ut at)
+  parseResults tokens
+  where
+    -- FUTUREWORK: There is surely a better way to do this
+    parseResults :: [Either ZAuth.Failure (UserId, Cookie (ZAuth.Token u))] -> ExceptT ZAuth.Failure AppIO (UserId, Cookie (ZAuth.Token u))
+    parseResults res = case (lefts res, rights res) of
+      (_, (suc : _)) -> return suc
+      ((e : _), _) -> throwE e
+      _ -> throwE ZAuth.Invalid -- Impossible with NonEmpty
+
+validateToken ::
   ZAuth.TokenPair u a =>
   ZAuth.Token u ->
   Maybe (ZAuth.Token a) ->
   ExceptT ZAuth.Failure AppIO (UserId, Cookie (ZAuth.Token u))
-validateTokens ut at = do
+validateToken ut at = do
   unless (maybe True ((ZAuth.userTokenOf ut ==) . ZAuth.accessTokenOf) at) $
     throwE ZAuth.Invalid
   ExceptT (ZAuth.validateToken ut)

services/brig/src/Brig/User/Auth/Cookie.hs

@@ -52,7 +52,6 @@ import Data.Id
 import qualified Data.List as List
 import qualified Data.Metrics as Metrics
 import Data.Proxy
-import Data.Text.Encoding (encodeUtf8)
 import Data.Time.Clock
 import Imports
 import Network.Wai (Response)
@@ -235,7 +234,7 @@ setResponseCookie c r = do
       WebCookie.def
         { WebCookie.setCookieName = "zuid",
           WebCookie.setCookieValue = toByteString' (cookieValue c),
-          WebCookie.setCookieDomain = Just $ encodeUtf8 . setCookieDomain $ s,
+          WebCookie.setCookieDomain = Nothing,
           WebCookie.setCookiePath = Just "/access",
           WebCookie.setCookieExpires =
             if cookieType c == PersistentCookie

services/brig/test/integration/API/User/Auth.hs

@@ -36,10 +36,9 @@ import Brig.Types.User.Auth
 import qualified Brig.Types.User.Auth as Auth
 import Brig.ZAuth (ZAuth, runZAuth)
 import qualified Brig.ZAuth as ZAuth
-import Control.Lens (set, (^.), (^?))
+import Control.Lens (set, (^.))
 import Control.Retry
 import Data.Aeson
-import Data.Aeson.Lens
 import qualified Data.ByteString as BS
 import Data.ByteString.Conversion
 import qualified Data.ByteString.Lazy as Lazy
@@ -48,7 +47,6 @@ import Data.Id
 import Data.Misc (PlainTextPassword (..))
 import Data.Proxy
 import qualified Data.Text as Text
-import Data.Text.Encoding (encodeUtf8)
 import qualified Data.Text.Lazy as Lazy
 import Data.Time.Clock
 import qualified Data.UUID.V4 as UUID
@@ -96,7 +94,8 @@ tests conf m z b g n =
           testGroup
             "nginz"
             [ test m "nginz-login" (testNginz b n),
-              test m "nginz-legalhold-login" (testNginzLegalHold b g n)
+              test m "nginz-legalhold-login" (testNginzLegalHold b g n),
+              test m "nginz-login-legacy-cookies" (testNginzLegacyCookies b n)
             ]
         ],
       testGroup
@@ -146,7 +145,7 @@ testNginz b n = do
   let Just email = userEmail u
   -- Login with email
   rs <-
-    login b (defEmailLogin email) PersistentCookie
+    login n (defEmailLogin email) PersistentCookie
       <!! const 200 === statusCode
   let c = decodeCookie rs
       t = decodeToken rs
@@ -167,13 +166,27 @@ testNginz b n = do
 testNginzLegalHold :: Brig -> Galley -> Nginz -> Http ()
 testNginzLegalHold b g n = do
   -- create team user Alice
-  (alice, tid) <- createUserWithTeam b
+  (alice, tid) <- createUserWithTeam' b
   putLegalHoldEnabled tid TeamFeatureEnabled g -- enable it for this team
-  rs <-
-    legalHoldLogin b (LegalHoldLogin alice (Just defPassword) Nothing) PersistentCookie
-      <!! const 200 === statusCode
-  let c = decodeCookie rs
-      t = decodeToken' @ZAuth.LegalHoldAccess rs
+  (c, t) <- do
+    -- we need to get the cookie domain from a login through nginz.  otherwise, if brig and
+    -- nginz are running on different hosts, no cookie will be presented in the later requests
+    -- to nginz in this test.  for simplicity, we use the internal end-point for
+    -- authenticating an LH dev, and then steal the domain from a cookie obtained via user
+    -- login.
+    rsUsr <- do
+      let Just email = userEmail alice
+      login n (defEmailLogin email) PersistentCookie
+        <!! const 200 === statusCode
+    rsLhDev <-
+      legalHoldLogin b (LegalHoldLogin (userId alice) (Just defPassword) Nothing) PersistentCookie
+        <!! const 200 === statusCode
+    let t = decodeToken' @ZAuth.LegalHoldAccess rsLhDev
+        c = cLhDev {cookie_domain = cookie_domain cUsr}
+        cLhDev = decodeCookie rsLhDev
+        cUsr = decodeCookie rsUsr
+    pure (c, t)
+
   -- ensure nginz allows passing legalhold cookies / tokens through to /access
   post (n . path "/access" . cookie c . header "Authorization" ("Bearer " <> (toByteString' t))) !!! do
     const 200 === statusCode
@@ -183,6 +196,33 @@ testNginzLegalHold b g n = do
   -- ensure legal hold tokens can fetch notifications
   get (n . path "/notifications" . header "Authorization" ("Bearer " <> (toByteString' t))) !!! const 200 === statusCode
 
+-- | Corner case for 'testNginz': when upgrading a wire backend from the old behavior (setting
+-- cookie domain to eg. @*.wire.com@) to the new behavior (leaving cookie domain empty,
+-- effectively setting it to the backend host), clients may start sending two cookies for a
+-- while: because the domains differ, new ones will not overwrite old ones locally (it was seen
+-- on different browsers) although the cookie does get overriden in the DB). This should be handled
+-- gracefully (ie., one invalid cookie should just be ignored if up to two cookies with label @"zuid"@ are present).
+--
+-- In this test, we actually don't use different domains (testing that correctly is actually pretty
+-- complex) - instead, we simply set 2 cookies with the same name and different values and http-client
+-- will replicate the situation: we have 2 cookies, one of them is incorrect (but must parse correctly!)
+-- and the other is valid.
+testNginzLegacyCookies :: Brig -> Nginz -> Http ()
+testNginzLegacyCookies b n = do
+  u <- randomUser b
+  let Just email = userEmail u
+      dologin :: HasCallStack => Http ResponseLBS
+      dologin = login n (defEmailLogin email) PersistentCookie <!! const 200 === statusCode
+  badCookie1 <- (\c -> c {cookie_value = "SKsjKQbiqxuEugGMWVbq02fNEA7QFdNmTiSa1Y0YMgaEP5tWl3nYHWlIrM5F8Tt7Cfn2Of738C7oeiY8xzPHAA==.v=1.k=1.d=1.t=u.l=.u=13da31b4-c6bb-4561-8fed-07e728fa6cc5.r=f844b420"}) . decodeCookie <$> dologin
+  goodCookie <- decodeCookie <$> dologin
+  badCookie2 <- (\c -> c {cookie_value = "SKsjKQbiqxuEugGMWVbq02fNEA7QFdNmTiSa1Y0YMgaEP5tWl3nYHWlIrM5F8Tt7Cfn2Of738C7oeiY8xzPHAA==.v=1.k=1.d=1.t=u.l=.u=13da31b4-c6bb-4561-8fed-07e728fa6cc5.r=f844b420"}) . decodeCookie <$> dologin
+
+  post (n . path "/access" . cookie goodCookie) !!! const 200 === statusCode
+  post (n . path "/access" . cookie badCookie1) !!! const 403 === statusCode
+  post (n . path "/access" . cookie badCookie2) !!! const 403 === statusCode
+  -- Sending both cookies should always work, regardless of the order
+  post (n . path "/access" . cookie badCookie1 . cookie goodCookie . cookie badCookie2) !!! const 200 === statusCode
+
 -------------------------------------------------------------------------------
 -- Login
 
@@ -849,18 +889,6 @@ prepareLegalHoldUser brig galley = do
   putLegalHoldEnabled tid TeamFeatureEnabled galley
   return uid
 
-decodeCookie :: HasCallStack => Response a -> Http.Cookie
-decodeCookie = fromMaybe (error "missing zuid cookie") . getCookie "zuid"
-
-decodeToken :: HasCallStack => Response (Maybe Lazy.ByteString) -> ZAuth.AccessToken
-decodeToken = decodeToken' @ZAuth.Access
-
-decodeToken' :: (HasCallStack, ZAuth.AccessTokenLike a) => Response (Maybe Lazy.ByteString) -> ZAuth.Token a
-decodeToken' r = fromMaybe (error "invalid access_token") $ do
-  x <- responseBody r
-  t <- x ^? key "access_token" . _String
-  fromByteString (encodeUtf8 t)
-
 getCookieId :: forall u. (HasCallStack, ZAuth.UserTokenLike u) => Http.Cookie -> CookieId
 getCookieId c =
   maybe