From b8c975338eeae0150056a6ab67987de035981c40 Mon Sep 17 00:00:00 2001 From: jschaul Date: Thu, 15 Sep 2022 20:12:22 +0200 Subject: [PATCH 1/9] log randomPrekey state of things --- services/brig/src/Brig/App.hs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/services/brig/src/Brig/App.hs b/services/brig/src/Brig/App.hs index fb600c55cb..925078a3af 100644 --- a/services/brig/src/Brig/App.hs +++ b/services/brig/src/Brig/App.hs @@ -232,8 +232,12 @@ newEnv o = do SqsQueue q -> SqsQueue <$> AWS.getQueueUrl (aws ^. AWS.amazonkaEnv) q mSFTEnv <- mapM Calling.mkSFTEnv $ Opt.sft o prekeyLocalLock <- case Opt.randomPrekeys o of - Just True -> Just <$> newMVar () - _ -> pure Nothing + Just True -> do + Log.warn lgr $ Log.msg (Log.val "randomPrekeys: active") + Just <$> newMVar () + _ -> do + Log.warn lgr $ Log.msg (Log.val "randomPrekeys: not active; using dynamoDB instead.") + pure Nothing kpLock <- newMVar () pure $! Env From 624348197514ef1e234f3a057a31322f547dd697 Mon Sep 17 00:00:00 2001 From: jschaul Date: Fri, 16 Sep 2022 09:34:58 +0200 Subject: [PATCH 2/9] Add an integration test to ensure list of prekeys does not become empty. This test should also run for the dynamoDB case, to ensure our implementation isn't buggy (it probably isn't, but to check for regressions) --- .../brig/test/integration/API/User/Client.hs | 50 +++++++++++++++++++ services/brig/test/integration/Util.hs | 8 ++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/services/brig/test/integration/API/User/Client.hs b/services/brig/test/integration/API/User/Client.hs index d3171c6ee4..b53e679aef 100644 --- a/services/brig/test/integration/API/User/Client.hs +++ b/services/brig/test/integration/API/User/Client.hs @@ -50,6 +50,7 @@ import Data.Text.Ascii (AsciiChars (validate)) import qualified Data.Vector as Vec import Imports import qualified Network.Wai.Utilities.Error as Error +import qualified System.Logger as Log import Test.QuickCheck (arbitrary, generate) import Test.Tasty hiding (Timeout) import Test.Tasty.Cannon hiding (Cannon) @@ -103,6 +104,7 @@ tests _cl _at opts p db b c g = test p "get /clients - 200" $ testListClients b, test p "get /clients/:client/prekeys - 200" $ testListPrekeyIds b, test p "post /clients - 400" $ testTooManyClients opts b, + test p "client/prekeys not empty" $ testPrekeysNotEmptyRandomPrekeys opts b, test p "delete /clients/:client - 200 (pwd)" $ testRemoveClient True b c, test p "delete /clients/:client - 200 (no pwd)" $ testRemoveClient False b c, test p "delete /clients/:client - 400 (short pwd)" $ testRemoveClientShortPwd b, @@ -689,6 +691,54 @@ testTooManyClients opts brig = do const (Just "too-many-clients") === fmap Error.label . responseJsonMaybe const (Just "application/json") === getHeader "Content-Type" +-- Ensure that the list of prekeys for a user does not become empty, and the +-- last resort prekey keeps being returned if it's the only key left. +-- Test with featureFlag randomPrekeys=true +testPrekeysNotEmptyRandomPrekeys :: Opt.Opts -> Brig -> Http () +testPrekeysNotEmptyRandomPrekeys opts brig = do + -- Run the test for randomPrekeys (not dynamoDB locking) + let newOpts = opts {Opt.randomPrekeys = Just True} + ensurePrekeysNotEmpty newOpts brig + +-- TODO: re-enable fake dynamoDB for integration tests in charts to also test +-- with that feature flag. + +-- Ensure that the list of prekeys for a user does not become empty, and the +-- last resort prekey keeps being returned if it's the only key left. +-- Test with featureFlag randomPrekeys=false +-- testPrekeysNotEmptyRandomPrekeys :: Opt.Opts -> Brig -> Http () +-- testPrekeysNotEmptyRandomPrekeys opts brig = do +-- -- Run the test for behaviour using dynamoDB locking +-- let newOpts = opts {Opt.randomPrekeys = Just False} +-- ensurePrekeysNotEmpty newOpts brig + +-- Ensure that the list of prekeys for a user does not become empty, and the +-- last resort prekey keeps being returned if it's the only key left. +-- Test with featureFlag randomPrekeys unset (empty) +-- testPrekeysNotEmptyRandomPrekeys :: Opt.Opts -> Brig -> Http () +-- testPrekeysNotEmptyRandomPrekeys opts brig = do +-- -- Run the test for behaviour using dynamoDB locking +-- let newOpts = opts {Opt.randomPrekeys = Nothing} +-- ensurePrekeysNotEmpty newOpts brig + +ensurePrekeysNotEmpty :: Opt.Opts -> Brig -> Http () +ensurePrekeysNotEmpty opts brig = withSettingsOverrides opts $ do + lgr <- Log.new Log.defSettings + uid <- userId <$> randomUser brig + -- Create a client with 1 regular prekey and 1 last resort prekey + c <- responseJsonError =<< addClient brig uid (defNewClient PermanentClientType [somePrekeys !! 10] (someLastPrekeys !! 10)) + -- Claim the first regular one + _rs1 <- getPreKey brig uid uid (clientId c) responseJsonMaybe rs2 + liftIO $ assertEqual "last prekey rs2" (Just lastPrekeyId) pId2 + liftIO $ Log.warn lgr (Log.msg (Log.val "First claim of last resort successful, claim again...")) + -- Claim again; this should (again) give the last resort one + rs3 <- getPreKey brig uid uid (clientId c) responseJsonMaybe rs3 + liftIO $ assertEqual "last prekey rs3" (Just lastPrekeyId) pId3 + -- @END -- The testRemoveClient test conforms to the following testing standards: diff --git a/services/brig/test/integration/Util.hs b/services/brig/test/integration/Util.hs index 37cfac2e5e..9e2b99dc8d 100644 --- a/services/brig/test/integration/Util.hs +++ b/services/brig/test/integration/Util.hs @@ -687,7 +687,13 @@ defNewClientWithVerificationCode mbCode ty pks lpk = newClientVerificationCode = mbCode } -getPreKey :: Brig -> UserId -> UserId -> ClientId -> Http ResponseLBS +getPreKey :: + (MonadIO m, MonadCatch m, MonadFail m, MonadHttp m, HasCallStack) => + Brig -> + UserId -> + UserId -> + ClientId -> + m ResponseLBS getPreKey brig zusr u c = get $ apiVersion "v1" From c218034442d09b16fadafd70aff773ff3017112a Mon Sep 17 00:00:00 2001 From: jschaul Date: Mon, 19 Sep 2022 11:41:39 +0200 Subject: [PATCH 3/9] add another test for last prekey validation --- libs/wire-api/src/Wire/API/User/Client/Prekey.hs | 6 ++++++ services/brig/test/integration/API/User/Client.hs | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/libs/wire-api/src/Wire/API/User/Client/Prekey.hs b/libs/wire-api/src/Wire/API/User/Client/Prekey.hs index 8b29e54afa..cf084c65d8 100644 --- a/libs/wire-api/src/Wire/API/User/Client/Prekey.hs +++ b/libs/wire-api/src/Wire/API/User/Client/Prekey.hs @@ -25,6 +25,7 @@ module Wire.API.User.Client.Prekey LastPrekey, lastPrekey, unpackLastPrekey, + fakeLastPrekey, lastPrekeyId, PrekeyBundle (..), ClientPrekey (..), @@ -101,6 +102,11 @@ lastPrekeyId = PrekeyId maxBound lastPrekey :: Text -> LastPrekey lastPrekey = LastPrekey . Prekey lastPrekeyId +-- for tests only +-- This fake last prekey has the wrong prekeyId +fakeLastPrekey :: LastPrekey +fakeLastPrekey = LastPrekey $ Prekey (PrekeyId 7) "pQABAQcCoQBYIDXdN8VlKb5lbgPmoDPLPyqNIEyShG4oT/DlW0peRRZUA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=" + -------------------------------------------------------------------------------- -- PrekeyBundle diff --git a/services/brig/test/integration/API/User/Client.hs b/services/brig/test/integration/API/User/Client.hs index b53e679aef..17c2ba94b4 100644 --- a/services/brig/test/integration/API/User/Client.hs +++ b/services/brig/test/integration/API/User/Client.hs @@ -105,6 +105,7 @@ tests _cl _at opts p db b c g = test p "get /clients/:client/prekeys - 200" $ testListPrekeyIds b, test p "post /clients - 400" $ testTooManyClients opts b, test p "client/prekeys not empty" $ testPrekeysNotEmptyRandomPrekeys opts b, + test p "lastprekeys not bogus" $ testRegularPrekeysCannotBeSentAsLastPrekeys opts b, test p "delete /clients/:client - 200 (pwd)" $ testRemoveClient True b c, test p "delete /clients/:client - 200 (no pwd)" $ testRemoveClient False b c, test p "delete /clients/:client - 400 (short pwd)" $ testRemoveClientShortPwd b, @@ -739,6 +740,12 @@ ensurePrekeysNotEmpty opts brig = withSettingsOverrides opts $ do let pId3 = prekeyId . prekeyData <$> responseJsonMaybe rs3 liftIO $ assertEqual "last prekey rs3" (Just lastPrekeyId) pId3 +testRegularPrekeysCannotBeSentAsLastPrekeys :: Opt.Opts -> Brig -> Http () +testRegularPrekeysCannotBeSentAsLastPrekeys opts brig = do + uid <- userId <$> randomUser brig + -- The parser should reject a normal prekey in the lastPrekey field + addClient brig uid (defNewClient PermanentClientType [head somePrekeys] fakeLastPrekey) !!! const 400 === statusCode + -- @END -- The testRemoveClient test conforms to the following testing standards: From 84f026289d1abf96e80766ea0ebc292a44c0cebd Mon Sep 17 00:00:00 2001 From: jschaul Date: Mon, 19 Sep 2022 13:56:40 +0200 Subject: [PATCH 4/9] Add one more test, fix warning from previous test --- .../brig/test/integration/API/User/Client.hs | 29 +++++++++++++++++-- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/services/brig/test/integration/API/User/Client.hs b/services/brig/test/integration/API/User/Client.hs index 17c2ba94b4..7657b8cb00 100644 --- a/services/brig/test/integration/API/User/Client.hs +++ b/services/brig/test/integration/API/User/Client.hs @@ -105,7 +105,8 @@ tests _cl _at opts p db b c g = test p "get /clients/:client/prekeys - 200" $ testListPrekeyIds b, test p "post /clients - 400" $ testTooManyClients opts b, test p "client/prekeys not empty" $ testPrekeysNotEmptyRandomPrekeys opts b, - test p "lastprekeys not bogus" $ testRegularPrekeysCannotBeSentAsLastPrekeys opts b, + test p "lastprekeys not bogus" $ testRegularPrekeysCannotBeSentAsLastPrekeys b, + test p "lastprekeys not bogus during update" $ testRegularPrekeysCannotBeSentAsLastPrekeysDuringUpdate b, test p "delete /clients/:client - 200 (pwd)" $ testRemoveClient True b c, test p "delete /clients/:client - 200 (no pwd)" $ testRemoveClient False b c, test p "delete /clients/:client - 400 (short pwd)" $ testRemoveClientShortPwd b, @@ -740,12 +741,34 @@ ensurePrekeysNotEmpty opts brig = withSettingsOverrides opts $ do let pId3 = prekeyId . prekeyData <$> responseJsonMaybe rs3 liftIO $ assertEqual "last prekey rs3" (Just lastPrekeyId) pId3 -testRegularPrekeysCannotBeSentAsLastPrekeys :: Opt.Opts -> Brig -> Http () -testRegularPrekeysCannotBeSentAsLastPrekeys opts brig = do +testRegularPrekeysCannotBeSentAsLastPrekeys :: Brig -> Http () +testRegularPrekeysCannotBeSentAsLastPrekeys brig = do uid <- userId <$> randomUser brig -- The parser should reject a normal prekey in the lastPrekey field addClient brig uid (defNewClient PermanentClientType [head somePrekeys] fakeLastPrekey) !!! const 400 === statusCode +testRegularPrekeysCannotBeSentAsLastPrekeysDuringUpdate :: Brig -> Http () +testRegularPrekeysCannotBeSentAsLastPrekeysDuringUpdate brig = do + uid <- userId <$> randomUser brig + c <- responseJsonError =<< addClient brig uid (defNewClient PermanentClientType [head somePrekeys] (someLastPrekeys !! 11)) Date: Mon, 19 Sep 2022 14:10:50 +0200 Subject: [PATCH 5/9] logging on startup: log using info level, not warning --- services/brig/src/Brig/App.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/brig/src/Brig/App.hs b/services/brig/src/Brig/App.hs index 925078a3af..50742a9faf 100644 --- a/services/brig/src/Brig/App.hs +++ b/services/brig/src/Brig/App.hs @@ -233,10 +233,10 @@ newEnv o = do mSFTEnv <- mapM Calling.mkSFTEnv $ Opt.sft o prekeyLocalLock <- case Opt.randomPrekeys o of Just True -> do - Log.warn lgr $ Log.msg (Log.val "randomPrekeys: active") + Log.info lgr $ Log.msg (Log.val "randomPrekeys: active") Just <$> newMVar () _ -> do - Log.warn lgr $ Log.msg (Log.val "randomPrekeys: not active; using dynamoDB instead.") + Log.info lgr $ Log.msg (Log.val "randomPrekeys: not active; using dynamoDB instead.") pure Nothing kpLock <- newMVar () pure $! From 0d54889e6fb606624f072a6d0395d446322bf20c Mon Sep 17 00:00:00 2001 From: jschaul Date: Mon, 19 Sep 2022 14:49:11 +0200 Subject: [PATCH 6/9] fixup: expected status code is 201, not 200 --- services/brig/test/integration/API/User/Client.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/brig/test/integration/API/User/Client.hs b/services/brig/test/integration/API/User/Client.hs index 7657b8cb00..1dc3e4455d 100644 --- a/services/brig/test/integration/API/User/Client.hs +++ b/services/brig/test/integration/API/User/Client.hs @@ -750,7 +750,7 @@ testRegularPrekeysCannotBeSentAsLastPrekeys brig = do testRegularPrekeysCannotBeSentAsLastPrekeysDuringUpdate :: Brig -> Http () testRegularPrekeysCannotBeSentAsLastPrekeysDuringUpdate brig = do uid <- userId <$> randomUser brig - c <- responseJsonError =<< addClient brig uid (defNewClient PermanentClientType [head somePrekeys] (someLastPrekeys !! 11)) Date: Thu, 19 Jan 2023 14:04:04 +0100 Subject: [PATCH 7/9] remove commented out code --- .../brig/test/integration/API/User/Client.hs | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/services/brig/test/integration/API/User/Client.hs b/services/brig/test/integration/API/User/Client.hs index 1dc3e4455d..f9c4899118 100644 --- a/services/brig/test/integration/API/User/Client.hs +++ b/services/brig/test/integration/API/User/Client.hs @@ -702,27 +702,6 @@ testPrekeysNotEmptyRandomPrekeys opts brig = do let newOpts = opts {Opt.randomPrekeys = Just True} ensurePrekeysNotEmpty newOpts brig --- TODO: re-enable fake dynamoDB for integration tests in charts to also test --- with that feature flag. - --- Ensure that the list of prekeys for a user does not become empty, and the --- last resort prekey keeps being returned if it's the only key left. --- Test with featureFlag randomPrekeys=false --- testPrekeysNotEmptyRandomPrekeys :: Opt.Opts -> Brig -> Http () --- testPrekeysNotEmptyRandomPrekeys opts brig = do --- -- Run the test for behaviour using dynamoDB locking --- let newOpts = opts {Opt.randomPrekeys = Just False} --- ensurePrekeysNotEmpty newOpts brig - --- Ensure that the list of prekeys for a user does not become empty, and the --- last resort prekey keeps being returned if it's the only key left. --- Test with featureFlag randomPrekeys unset (empty) --- testPrekeysNotEmptyRandomPrekeys :: Opt.Opts -> Brig -> Http () --- testPrekeysNotEmptyRandomPrekeys opts brig = do --- -- Run the test for behaviour using dynamoDB locking --- let newOpts = opts {Opt.randomPrekeys = Nothing} --- ensurePrekeysNotEmpty newOpts brig - ensurePrekeysNotEmpty :: Opt.Opts -> Brig -> Http () ensurePrekeysNotEmpty opts brig = withSettingsOverrides opts $ do lgr <- Log.new Log.defSettings From b9a341b0c0045dc53a631405b9dfed6c8a1de669 Mon Sep 17 00:00:00 2001 From: jschaul Date: Thu, 19 Jan 2023 14:07:21 +0100 Subject: [PATCH 8/9] changelog --- changelog.d/5-internal/pr-2694 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/5-internal/pr-2694 diff --git a/changelog.d/5-internal/pr-2694 b/changelog.d/5-internal/pr-2694 new file mode 100644 index 0000000000..7bd6984179 --- /dev/null +++ b/changelog.d/5-internal/pr-2694 @@ -0,0 +1 @@ +Add two integration tests arounds last prekeys From b9ae325e86596b9e928249d0ae1766a5468a98ac Mon Sep 17 00:00:00 2001 From: jschaul Date: Thu, 19 Jan 2023 14:20:42 +0100 Subject: [PATCH 9/9] linter? make format doesn't work; or local/CI don't agree --- services/brig/test/integration/API/User/Client.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/brig/test/integration/API/User/Client.hs b/services/brig/test/integration/API/User/Client.hs index f9c4899118..77ccdb1e2f 100644 --- a/services/brig/test/integration/API/User/Client.hs +++ b/services/brig/test/integration/API/User/Client.hs @@ -746,7 +746,7 @@ testRegularPrekeysCannotBeSentAsLastPrekeysDuringUpdate brig = do . body (RequestBodyLBS $ encode update) ) !!! const 400 - === statusCode + === statusCode -- @END