FS-1517 Partial success on fetch prekeys by lepsa · Pull Request #3108 · wireapp/wire-server

lepsa · 2023-02-28T09:02:34Z

Adding a new version of the list-prekeys routes that can return partial successes, listing qualified users that they weren't able to list.

Checklist

[:heavy_check_mark:] Add a new entry in an appropriate subdirectory of changelog.d
[:heavy_check_mark:] Read and follow the PR guidelines

Changes

I've updates /users/list-prekeys to include a new field, which required the current structure to be moved down into a new key. The bulk of the data has not changed, it just under a new field to support the new failed_to_list field that is populated on partial success.
New tests added, old tests updated to reflect their max API version, and a new test added that checks for failed_to_list

Adding a new version of the list-prekeys routes that can return partial successes, listing qualified users that they weren't able to list.

mdimjasevic

In the tests you've added I'd expect not checking for local users' prekeys, but for remote ones. Let me know if I'm missing something!

changelog.d/1-api-changes/fs-1517

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

libs/wire-api/src/Wire/API/User/Client.hs

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

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

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

…ial-success-on-endpoint-to-fetch-prekeys-foo

lepsa · 2023-03-23T07:38:14Z

I've also updated some of the handler names so that the V3 is on the old handlers.

mdimjasevic · 2023-03-27T09:15:02Z

At first I thought this test failure in Brig is unrelated, but given the involved type, I figure it is related:

    claim multi-prekey bundle:                                                               FAIL

      Exception: Assertions failed:

       2: Just (QualifiedUserClientPrekeyMap {getQualifiedUserClientPrekeyMap = QualifiedUserClientMap {qualifiedUserClientMap = fromList [(Domain {_domainText = "federation-test-helper.test-gdcq96hqza57-fed2.svc.cluster.local"},fromList [(1b650b15-1ab5-4ba4-b6a2-483d81cd9a50,fromList [(ClientId {client = "bb7d4da95e4053b9"},Just (Prekey {prekeyId = PrekeyId {keyId = 9}, prekeyKey = "pQABAQkCoQBYIFcAnXdx0M1Q1hoDDfgMK9r+Zchn8YlVHHaQwQYhRk1dA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "c2cf27807bc11004"},Just (Prekey {prekeyId = PrekeyId {keyId = 8}, prekeyKey = "pQABAQgCoQBYIJH1ewvIVV3yGqQvdr/QM9HARzMgo5ksOTRyKEuN2aZzA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "c7b81ec8f06d822c"},Just (Prekey {prekeyId = PrekeyId {keyId = 7}, prekeyKey = "pQABAQcCoQBYIDXdN8VlKb5lbgPmoDPLPyqNIEyShG4oT/DlW0peRRZUA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "d15d5572afc9dad5"},Just (Prekey {prekeyId = PrekeyId {keyId = 6}, prekeyKey = "pQABAQYCoQBYIFXUkVftE7kK22waAzhOjOmJVex3EBTU8RHZFx2o1Ed8A6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="}))])]),(Domain {_domainText = "federation-test-helper.test-gdcq96hqza57.svc.cluster.local"},fromList [(01004631-66e6-4753-93ca-477a97a25fbb,fromList [(ClientId {client = "1fe08975b9978fbe"},Just (Prekey {prekeyId = PrekeyId {keyId = 5}, prekeyKey = "pQABAQUCoQBYIHnafNR4Gh3ID71lYzToewEVag4EKskDFq+gaeraOlSJA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "550d8c614fd20299"},Just (Prekey {prekeyId = PrekeyId {keyId = 1}, prekeyKey = "pQABAQECoQBYIOjl7hw0D8YRNqkkBQETCxyr7/ywE/2R5RWcUPM+GJACA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "6496061963ba8ec3"},Just (Prekey {prekeyId = PrekeyId {keyId = 3}, prekeyKey = "pQABAQMCoQBYIEjdt+YWd3lHmG8pamULLMubAMZw556IO8kW7s1MLFytA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "ea035ddd6d9647d5"},Just (Prekey {prekeyId = PrekeyId {keyId = 4}, prekeyKey = "pQABAQQCoQBYIPIaOA3Xqfk4Lh2/pU88Owd2eW5eplHpywr+Mx4QGyiMA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "ea2b0e1c2342feb8"},Just (Prekey {prekeyId = PrekeyId {keyId = 2}, prekeyKey = "pQABAQICoQBYIGoXawUQWQ9ZW+MXhvuo9ALOBUjLff8S5VdAokN29C1OA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="}))])])]}}) =/= Nothing

      

      Response was:

      

      Response {responseStatus = Status {statusCode = 200, statusMessage = "OK"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Mon, 27 Mar 2023 08:46:51 GMT"),("Server","Warp/3.3.23"),("Content-Encoding","gzip"),("Content-Type","application/json;charset=utf-8"),("Vary","Accept-Encoding")], responseBody = Just "{\"qualified_user_client_prekeys\":{\"federation-test-helper.test-gdcq96hqza57-fed2.svc.cluster.local\":{\"1b650b15-1ab5-4ba4-b6a2-483d81cd9a50\":{\"bb7d4da95e4053b9\":{\"id\":9,\"key\":\"pQABAQkCoQBYIFcAnXdx0M1Q1hoDDfgMK9r+Zchn8YlVHHaQwQYhRk1dA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"c2cf27807bc11004\":{\"id\":8,\"key\":\"pQABAQgCoQBYIJH1ewvIVV3yGqQvdr/QM9HARzMgo5ksOTRyKEuN2aZzA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"c7b81ec8f06d822c\":{\"id\":7,\"key\":\"pQABAQcCoQBYIDXdN8VlKb5lbgPmoDPLPyqNIEyShG4oT/DlW0peRRZUA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"d15d5572afc9dad5\":{\"id\":6,\"key\":\"pQABAQYCoQBYIFXUkVftE7kK22waAzhOjOmJVex3EBTU8RHZFx2o1Ed8A6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"}}},\"federation-test-helper.test-gdcq96hqza57.svc.cluster.local\":{\"01004631-66e6-4753-93ca-477a97a25fbb\":{\"1fe08975b9978fbe\":{\"id\":5,\"key\":\"pQABAQUCoQBYIHnafNR4Gh3ID71lYzToewEVag4EKskDFq+gaeraOlSJA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"550d8c614fd20299\":{\"id\":1,\"key\":\"pQABAQECoQBYIOjl7hw0D8YRNqkkBQETCxyr7/ywE/2R5RWcUPM+GJACA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"6496061963ba8ec3\":{\"id\":3,\"key\":\"pQABAQMCoQBYIEjdt+YWd3lHmG8pamULLMubAMZw556IO8kW7s1MLFytA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"ea035ddd6d9647d5\":{\"id\":4,\"key\":\"pQABAQQCoQBYIPIaOA3Xqfk4Lh2/pU88Owd2eW5eplHpywr+Mx4QGyiMA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"ea2b0e1c2342feb8\":{\"id\":2,\"key\":\"pQABAQICoQBYIGoXawUQWQ9ZW+MXhvuo9ALOBUjLff8S5VdAokN29C1OA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"}}}}}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}

      CallStack (from HasCallStack):

        error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-1CeiEQekzBEdjo334Kowc:Bilge.Assert

        <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-1CeiEQekzBEdjo334Kowc:Bilge.Assert

        !!!, called at test/integration/Federation/End2end.hs:259:5 in main:Federation.End2end

      Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&/claim multi-prekey bundle/' to rerun this test only.

mdimjasevic

I believe there are things that have to be updated in the integration tests. The application code looks good now.

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

mdimjasevic · 2023-03-27T09:16:48Z

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

+          ( Right $
+              QualifiedUserClientPrekeyMapV4
+                { qualifiedUserClientPrekeys = QualifiedUserClientMap Map.empty,
+                  failedToList = pure [Qualified uid1 domain]


As mentioned above, it seems wrong to me to qualify a local user uid1 with a (fake) remote domain.

…ial-success-on-endpoint-to-fetch-prekeys

mdimjasevic · 2023-03-30T11:44:33Z

At first I thought this test failure in Brig is unrelated, but given the involved type, I figure it is related:
    claim multi-prekey bundle:                                                               FAIL

The same test still fails in the CI:

    claim multi-prekey bundle:                                                               FAIL

      Exception: Assertions failed:

       2: Just (QualifiedUserClientPrekeyMap {getQualifiedUserClientPrekeyMap = QualifiedUserClientMap {qualifiedUserClientMap = fromList [(Domain {_domainText = "federation-test-helper.test-y37s0mkld547-fed2.svc.cluster.local"},fromList [(e6c6b2d2-2c30-4e19-833b-8cb64995c73f,fromList [(ClientId {client = "bb7d4da95e4053b9"},Just (Prekey {prekeyId = PrekeyId {keyId = 9}, prekeyKey = "pQABAQkCoQBYIFcAnXdx0M1Q1hoDDfgMK9r+Zchn8YlVHHaQwQYhRk1dA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "c2cf27807bc11004"},Just (Prekey {prekeyId = PrekeyId {keyId = 8}, prekeyKey = "pQABAQgCoQBYIJH1ewvIVV3yGqQvdr/QM9HARzMgo5ksOTRyKEuN2aZzA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "c7b81ec8f06d822c"},Just (Prekey {prekeyId = PrekeyId {keyId = 7}, prekeyKey = "pQABAQcCoQBYIDXdN8VlKb5lbgPmoDPLPyqNIEyShG4oT/DlW0peRRZUA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "d15d5572afc9dad5"},Just (Prekey {prekeyId = PrekeyId {keyId = 6}, prekeyKey = "pQABAQYCoQBYIFXUkVftE7kK22waAzhOjOmJVex3EBTU8RHZFx2o1Ed8A6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="}))])]),(Domain {_domainText = "federation-test-helper.test-y37s0mkld547.svc.cluster.local"},fromList [(8baa832a-75c0-4f96-9280-c96d34bdcc9b,fromList [(ClientId {client = "1fe08975b9978fbe"},Just (Prekey {prekeyId = PrekeyId {keyId = 5}, prekeyKey = "pQABAQUCoQBYIHnafNR4Gh3ID71lYzToewEVag4EKskDFq+gaeraOlSJA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "550d8c614fd20299"},Just (Prekey {prekeyId = PrekeyId {keyId = 1}, prekeyKey = "pQABAQECoQBYIOjl7hw0D8YRNqkkBQETCxyr7/ywE/2R5RWcUPM+GJACA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "6496061963ba8ec3"},Just (Prekey {prekeyId = PrekeyId {keyId = 3}, prekeyKey = "pQABAQMCoQBYIEjdt+YWd3lHmG8pamULLMubAMZw556IO8kW7s1MLFytA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "ea035ddd6d9647d5"},Just (Prekey {prekeyId = PrekeyId {keyId = 4}, prekeyKey = "pQABAQQCoQBYIPIaOA3Xqfk4Lh2/pU88Owd2eW5eplHpywr+Mx4QGyiMA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="})),(ClientId {client = "ea2b0e1c2342feb8"},Just (Prekey {prekeyId = PrekeyId {keyId = 2}, prekeyKey = "pQABAQICoQBYIGoXawUQWQ9ZW+MXhvuo9ALOBUjLff8S5VdAokN29C1OA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY="}))])])]}}) =/= Nothing

      

      Response was:

      

      Response {responseStatus = Status {statusCode = 200, statusMessage = "OK"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Thu, 30 Mar 2023 05:12:13 GMT"),("Server","Warp/3.3.23"),("Content-Encoding","gzip"),("Content-Type","application/json;charset=utf-8"),("Vary","Accept-Encoding")], responseBody = Just "{\"qualified_user_client_prekeys\":{\"federation-test-helper.test-y37s0mkld547-fed2.svc.cluster.local\":{\"e6c6b2d2-2c30-4e19-833b-8cb64995c73f\":{\"bb7d4da95e4053b9\":{\"id\":9,\"key\":\"pQABAQkCoQBYIFcAnXdx0M1Q1hoDDfgMK9r+Zchn8YlVHHaQwQYhRk1dA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"c2cf27807bc11004\":{\"id\":8,\"key\":\"pQABAQgCoQBYIJH1ewvIVV3yGqQvdr/QM9HARzMgo5ksOTRyKEuN2aZzA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"c7b81ec8f06d822c\":{\"id\":7,\"key\":\"pQABAQcCoQBYIDXdN8VlKb5lbgPmoDPLPyqNIEyShG4oT/DlW0peRRZUA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"d15d5572afc9dad5\":{\"id\":6,\"key\":\"pQABAQYCoQBYIFXUkVftE7kK22waAzhOjOmJVex3EBTU8RHZFx2o1Ed8A6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"}}},\"federation-test-helper.test-y37s0mkld547.svc.cluster.local\":{\"8baa832a-75c0-4f96-9280-c96d34bdcc9b\":{\"1fe08975b9978fbe\":{\"id\":5,\"key\":\"pQABAQUCoQBYIHnafNR4Gh3ID71lYzToewEVag4EKskDFq+gaeraOlSJA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"550d8c614fd20299\":{\"id\":1,\"key\":\"pQABAQECoQBYIOjl7hw0D8YRNqkkBQETCxyr7/ywE/2R5RWcUPM+GJACA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"6496061963ba8ec3\":{\"id\":3,\"key\":\"pQABAQMCoQBYIEjdt+YWd3lHmG8pamULLMubAMZw556IO8kW7s1MLFytA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"ea035ddd6d9647d5\":{\"id\":4,\"key\":\"pQABAQQCoQBYIPIaOA3Xqfk4Lh2/pU88Owd2eW5eplHpywr+Mx4QGyiMA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"},\"ea2b0e1c2342feb8\":{\"id\":2,\"key\":\"pQABAQICoQBYIGoXawUQWQ9ZW+MXhvuo9ALOBUjLff8S5VdAokN29C1OA6EAoQBYILLf1TIwSB62q69Ojs/X1tzJ+dYHNAw4QbW/7TC5vSZqBPY=\"}}}}}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}

      CallStack (from HasCallStack):

        error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-L77WXGV80XFJMqJLnvaA1h:Bilge.Assert

        <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-L77WXGV80XFJMqJLnvaA1h:Bilge.Assert

        !!!, called at test/integration/Federation/End2end.hs:259:5 in main:Federation.End2end

      Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&/claim multi-prekey bundle/' to rerun this test only.

lepsa · 2023-04-03T08:48:58Z

Local testing is showing green across the board. Can we re-run the CI tests?

…ial-success-on-endpoint-to-fetch-prekeys

mdimjasevic

In tests, I didn't get how you can authenticate with a fake remote user. This shouldn't work, but perhaps our test infrastructure is lacking.

I think I mentioned it before, but in the tests I don't see how you test prekey fetching from an unreachable backend. It could be I am missing it so can you please point me to it?

mdimjasevic · 2023-04-05T08:30:06Z

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

+          . paths ["users", "list-prekeys"]
+          . contentJson
+          . body (RequestBodyLBS $ encode userClients1)
+          . zUser (qUnqualified uid2)


User uid2 is a remote user. The way Wire is designed is that users/clients always talk directly and exclusively with their own backend, and never with a remote backend. Therefore, given that uid2 is remote, the user shouldn't be making a request to this backend. I'm not sure how this could result in a 200 response as I'd expect a 404 or something like that. Can you please clarify this for me?

mdimjasevic · 2023-04-05T09:10:24Z

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

+        . paths ["users", "list-prekeys"]
+        . contentJson
+        . body (RequestBodyLBS $ encode userClients2)
+        . zUser (qUnqualified uid2)


Same as above.

FS-1517 Partial success on fetch prekeys

0d0bb98

Adding a new version of the list-prekeys routes that can return partial successes, listing qualified users that they weren't able to list.

elland added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 13, 2023

HI CI

491f612

mdimjasevic reviewed Mar 20, 2023

View reviewed changes

lepsa added 4 commits March 21, 2023 14:20

FS-1517: Updating based on PR feedback before merging in changes

6f547e5

Merge remote-tracking branch 'origin/develop' into fs-1517-allow-part…

9ac243a

…ial-success-on-endpoint-to-fetch-prekeys-foo

FS-1517. Merging upstream changes, updating and fixing new tests

4a50171

FS-1517: Updating tests and pulling out some common code

17246a6

mdimjasevic reviewed Mar 27, 2023

View reviewed changes

lepsa added 3 commits March 30, 2023 14:00

FS-1517 Updating tests based on feedback

f015fe7

Merge remote-tracking branch 'origin/develop' into fs-1517-allow-part…

9ebbefd

…ial-success-on-endpoint-to-fetch-prekeys

FS-1517: PR formatting

a9af020

Merge remote-tracking branch 'origin/develop' into fs-1517-allow-part…

133cf11

…ial-success-on-endpoint-to-fetch-prekeys

mdimjasevic reviewed Apr 5, 2023

View reviewed changes

mdimjasevic approved these changes Apr 11, 2023

View reviewed changes

mdimjasevic merged commit 6fe8477 into wireapp:develop Apr 11, 2023

zebot mentioned this pull request Apr 17, 2023

Release 2023-04-17 - (expected chart version 4.35.0) #3230

Merged

Comments

Conversation

lepsa commented Feb 28, 2023

Checklist

Changes

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lepsa commented Mar 23, 2023

Uh oh!

mdimjasevic commented Mar 27, 2023

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdimjasevic Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

mdimjasevic commented Mar 30, 2023

Uh oh!

lepsa commented Apr 3, 2023

Uh oh!

mdimjasevic left a comment

Choose a reason for hiding this comment

Uh oh!

mdimjasevic Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

mdimjasevic Apr 5, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants