Latest issued Vaccination DCC 2/2 does not have highest priority

vaubaehn commented 3 years ago

Avoid duplicates

[x] Bug is not mentioned in the FAQ
[ ] Bug is specific for Android only, for general issues / questions that apply to iOS and Android please raise them in the documentation repository - I'm not sure how sorting is done in iOS. If the issue is similar, this issue could be transferred to documention repo.
[x] Bug is not already reported in another issue

Technical details

Device name: LG G4 (LGH-815)
Android version: Android 6.0
App version: 2.6.1

Describe the bug

Most users will only import 1 or 2 vaccination certificates into CWA, with either dose 2/2, or dose 1/2 together with dose 2/2. But there are some use cases, when users are importing additionally a re-issued VaccDCC (2/2) as a 3rd certificate at a later point in time. Either just because they can do it 🤷‍♂️ , or they need to: when the Netherlands decided to apply business rules schema v1.3.0 to their validation checks, but German VaccDCCs were issued compliant to schema v1.0.0 before July 1st, and users want to be on safe side in border control and get their 2nd VacDCC re-issued (online, pharmacy). See https://github.com/corona-warn-app/cwa-documentation/issues/671 . The 3rd VaccDCC (2/2) with a newer date of issue and later date of expiry can be imported into CWA without problems, but at least in my case, the 3rd VaccDCC is not taken into account as certificate with highest priority, but sorted as the 2nd VaccDCC in the list of all DCCs. This may lead to confusion upon presenting QR code to gate keepers, even more as there is currently no date of issue/expiry date displayed (you need to check certID by comparing with paper printouts/PDFs to find out...)

Steps to reproduce the issue

Import a vaccination DCC (2/2) that was issued before July 1st, 2021.
Import another vaccination DCC (2/2) that was issued somewhat after July 1st, 2021.
Check validity of both certificates against Netherland's business rules.
Find that the one with highest priority (automatically activated) in the list fails, while the second one passes - at least in my case. So, the first one is the old one, and the second one the new one.

Expected behaviour

The (vacc)DCC (2/2) with latest date of issue/the longest expiry date should be the vaccDCC with the highest priority.

Possible Fix

Apply another rule 3a that accounts for expiry date here: https://github.com/corona-warn-app/cwa-app-android/pull/3811

Additional context

I don't know if this would also be necessary for recovery certificates...

Internal Tracking ID: EXPOSUREAPP-8756

fynngodau commented 3 years ago

no date of issue/expiry date displayed

Expiry date will be displayed in CWA 2.7.

MikeMcC399 commented 3 years ago

@vaubaehn

I am also seeing CWA 2.6.1 selecting an older 2/2 certificate. They are both from PDFs and the older one has the QR code on the left of the text, the newer one has the QR code on the right of the text. They were both issued in July and they both pass the Netherlands test, so the impact of selecting the older certificate is not significant for me. Nevertheless it would be good to find out why the older one is being preferred.

MikeMcC399 commented 3 years ago

@vaubaehn

I did some more tests and both CWA and CovPass used the first scanned certificate as the default. If I scan the older one first, then that is default. If I scan the newer one first, then that becomes default.

D/PersonCertificatesExtensionsKt: Rule 3 match (Series-completing Vaccination Certificate > 14 days)

It seems that since the certificates have the same vaccination date, then the first one encountered in the list is preferred, with no other comparison taking place.

Perhaps there should be an FAQ article advising users to remove older certificates which have been reissued for correctional purposes? (See https://github.com/corona-warn-app/cwa-website/issues/1555 for notes on terminology.)

vaubaehn commented 3 years ago

@MikeMcC399

It seems that since the certificates have the same vaccination date, then the first one encountered in the list is preferred, with no other comparison taking place.

Yes, this seems to be the case according to comments in code: https://github.com/corona-warn-app/cwa-app-android/blob/fc7c1bd7377a9db04cb50abc06588c1d93400041/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/covidcertificate/person/core/PersonCertificatesExtensions.kt#L66-L85

Perhaps there should be an FAQ article advising users to remove older certificates which have been reissued for correctional purposes? (See corona-warn-app/cwa-website#1555 for notes on terminology.)

This is a good idea. Ideally, the sorting algorithm could be enhanced in a future release. I guess there are already many people out there who have a re-issued 2/2 VaccDCC.

MikeMcC399 commented 3 years ago

@vaubaehn This issue should also be reported to https://github.com/Digitaler-Impfnachweis/covpass-android/issues, since the CovPass App has exactly the same issue. Perhaps we should wait first for CWA dev feedback?

vaubaehn commented 3 years ago

@MikeMcC399 Or we could ask @wkornewald whether we open an issue in https://github.com/Digitaler-Impfnachweis/covpass-android/issues or if they want to observe progress from here?

MikeMcC399 commented 3 years ago

@vaubaehn I wouldn't want to assume that a particular person responds for CovPass App so I just went ahead and opened https://github.com/Digitaler-Impfnachweis/covpass-android/issues/57. The two issues, for the two different apps, can be processed in parallel now.

Edit: The CovPass team has acknowledged the issue in https://github.com/Digitaler-Impfnachweis/covpass-android/issues/57#issuecomment-889637319.

Now waiting for bug acknowledgement from the CWA Open Source Team and assignment of an internal ID.

MikeMcC399 commented 3 years ago

I/DccQrCodeValidator in the censored error report log file will give you the certificate's version number I/DccQrCodeValidator: Extracted data from QR code is VaccinationCertificateQRCode(qrCode=<censor-collision/>"},"ver":"1.3.0"}, kid=xxxxxxxxxx)) when it is added into the app.

If you connect via Android Studio, then Logcat will show the full parsing of the certificate data under I/DccQrCodeValidator when the certificate is added, including version, issuedAt, expiresAt and dt.

The schema version is defined in https://github.com/ehn-dcc-development/ehn-dcc-schema.

Jo-Achim commented 3 years ago

[OT] Perhaps on this occasion, if vaccination certificates of several people have been scanned into CWA, their sorting / order could be revised. Thanks. Please refer: https://github.com/corona-warn-app/cwa-wishlist/issues/598#issue-948906728 [/OT]

dsarkar commented 3 years ago

@MikeMcC399 @vaubaehn Thanks. Internal Tracking ID: EXPOSUREAPP-8756

Corona-Warn-App Open Source Team

Jo-Achim commented 3 years ago

The sorting order of the scanned vaccination certificates is unfortunate even if there are multiple complete vaccination certificates:

Screenshot_20210809-102339_Corona-Warn_2

See: https://github.com/corona-warn-app/cwa-documentation/issues/679#issuecomment-895154648.

"Last scanned = highest priority" would be an idea.

Jo-Achim commented 3 years ago

Workaround for sorting EU-Certificates (CWA 2.6.1 and CovPass 1.28.7): https://github.com/corona-warn-app/cwa-documentation/issues/679#issuecomment-896324471.

MikeMcC399 commented 3 years ago

@Jo-Achim If you have scanned in multiple certificates for a single vaccination, I would simply remove any certificate which you do not want used. I can't see any advantage to storing more than one certificate for the same person and the same vaccination.

Jo-Achim commented 3 years ago

@MikeMcC399 yes, I understand that and it is certainly a simple possibility.

But the initial issue here, for example, was "Latest issued Vaccination DCC 2/2 does not have highest priority" - the disadvantages have already been mentioned - against the background that, for whatever reasons, users scanned several EU certificates for the same person and vaccination.

A "for whatever reason" shouldn't be, but it could be that with a further development of the certificates not every country always keeps pace, the downward compatibility might not be given, ... - and therefore an older certificate could be helpful.

In this sense, I also understood your post https://github.com/Digitaler-Impfweise/covpass-android/issues/57#issue-956015931 under "Possible Fix":

2. If both the older and the re-issued certificate are stored, then when deciding on the "Aktuell verwendetes Zertifikat" the newer vaccination certificate should be chosen.

MikeMcC399 commented 3 years ago

@Jo-Achim Keeping multiple certificates for the same person / vaccination and with different schema versions for the purpose of providing a workaround for schema compatibility issues with other country's business rules is an aspect that I had not considered.

I don't think that would be a workaround though, because only one of the certificates for a given person is chosen as "Currently used certificate" at any time. The business rule check only runs against that one certificate.

Your apparent desire to check against different schema versions stored for the same person / vaccination would need to be a separate enhancement request.

Jo-Achim commented 3 years ago

@MikeMcC399

I don't think that would be a workaround though, because only one of the certificates for a given person is chosen as "Currently used certificate" at any time. The business rule check only runs against that one certificate.

Well, just in case, there is still the option of selecting the certificate to be used. Because after selecting the certificate for the person, you can scroll down to select another vaccination certificate and "check validity" in the CWA.

Your apparent desire to check against different schema versions stored for the same person / vaccination would need to be a separate enhancement request.

Done in: https://github.com/corona-warn-app/cwa-wishlist/issues/611#issue-966565025.

MikeMcC399 commented 3 years ago

@Jo-Achim

Well, just in case, there is still the option of selecting the certificate to be used. Because after selecting the certificate for the person, you can scroll down to select another vaccination certificate and "check validity" in the CWA.

Thanks for pointing that out. I hadn't noticed that ability!

MikeMcC399 commented 3 years ago

Link back to related issue https://github.com/corona-warn-app/cwa-app-android/issues/4000.

MikeMcC399 commented 3 years ago

With PR #4123 merged into release/2.11.x this should be fixed in the next release. 👍🏻

marcauberer commented 2 years ago

@vaubaehn can you confirm the fix? Can we close this one?

MikeMcC399 commented 2 years ago

@vaubaehn I can't exactly carry out your repro steps with 2/2 certificates because I only have 2/2 certificates and they both use schema 1.3.0. Also, due to the fact that NL rules have been fixed (as you yourself confirmed in https://github.com/corona-warn-app/cwa-documentation/issues/671#issuecomment-899683456) the repro steps would not actually show up the problem regarding the choice of certificate any more.

I can however scan these two different 1.3.0 2/2 certificates in CWA Android 2.11.2 in the order older then newer certificate, and the newer certificate is selected as "Currently used certificate".

So I do believe that this issue is resolved.

vaubaehn commented 2 years ago

Hi @MikeMcC399 and @marcauberer , meanwhile I have a phone at hand where CWA 2.11.2 arrived, and where I could check schema 1.0.0 and 1.3.0 certificates. CWA now sorts certificates like intended, and I'm closing here.

Thanks to everyone involved! 🍻

corona-warn-app / cwa-app-android