search

cds-snc / covid-alert-server

Exposure Notification: Diagnosis Server implementation / Notification d’exposition : Mise en œuvre du serveur de diagnostic
Apache License 2.0
298 stars 31 forks source link

Bundle option does not return keys uploaded today #323

Closed uhengart closed 3 years ago

uhengart commented 3 years ago

There are two options to retrieve keys from the server: 1) the keys uploaded on a particular day or 2) the entire bundle of keys currently available. There is an inconsistency between the two options. Using the first option, it is possible to download the keys uploaded so far today. However, the bundle from the second option does not contain the keys uploaded so far today.

To illustrate, these are the time windows from the two export.bin's with the downloaded keys as of today (2020-10-16 UTC): First option: 2020-10-16 00:00:00 UTC - 2020-10-17 00:00:00 UTC Second option: 2020-10-01 00:00:00 UTC - 2020-10-16 00:00:00 UTC

My understanding is that the problem is with the second option since the keys uploaded today should be retrievable today. Either these keys were used yesterday (or earlier) or they were used earlier today but a new key was set when they were retrieved by the app for uploading.

There's a similar inconsistency at the beginning of the bundle period, just reversed.

The fix is to change

endDate := timemath.CurrentDateNumber() - 1

to

endDate := timemath.CurrentDateNumber()

in covid-alert-server/pkg/server/retrieve.go, line 84.

Also, the time window allowed by the two options is too large, even after the fix is applied. In the example above, the time window for the bundle would start on 2020-10-02 after the fix is applied but it really should start only on 2020-10-03 (and end on 2020-10-17). That said, this is less of a problem since the keys from 2020-10-02 (or earlier) are already gone in the database.

CalvinRodo commented 3 years ago

Thanks for the issue , I'll take a look when I get the chance to see what impact this could potentially have on the mobile app.

maxneuvians commented 3 years ago

Hi @uhengart, thank you for your interest in the server and app!

This is currently by design.

The 14 day back bundle is only downloaded once by the app when the app is installed and then ideally never again. The initial implementation of the app saw a mechanism where you would iterate through the past 14 days and download a bundle for each day. The problem was that Google and Apple have rate limited API calls to the underlying EN framework (one is at 15 calls per day, the other at 21). So what would happen is that you would install the app, download each day bundle individually, and exhaust your API call limit after the first install. Any subsequent calls to the EN framework would error out. The solution here was to create one back bundle to force only one API call for the previous keys, allowing you to make subsequent calls for current day keys.

To your point about the date logic - early in the development we wanted to retain the ability to limit downloads to back days only and also avoid missing any keys due to off by one errors. We should at some point refactor the code there.

Let me know if you have any other questions!

uhengart commented 3 years ago

Makes sense. Thanks for the explanation!