CatimaLoyalty / Android

Catima, a Loyalty Card & Ticket Manager for Android
https://catima.app
GNU General Public License v3.0
894 stars 150 forks source link

QR code generated by Catima cannot be read by Verify Ontario #506

Open toolstack opened 3 years ago

toolstack commented 3 years ago

Smart Health Cards standard for COVID vaccination QR receipts are used in several Canadian provinces, but Catima generates a QR code that looks very different from the source QR code and cannot be read by the Verify Ontario app.

For example here is a sample QR code that looks similar to the official QR codes created by Ontario:

sample-qr-code

And what Catima generates after reading in the data:

Screenshot_20211017-133542

Note that this is valid QR code, and can be read by the shc-extractor script from which the sample data is taken, but Verify Ontario cannot read it.

It looks like the official QR code is a 4x4 pattern while Catima generates a 6x6 code. Catima is also not alone in generating this 6x6 style QR code from the data, another QR app I tried does the same thing.

TheLastProject commented 3 years ago

I first thought Catima might use another error correction level, but that isn't it. They're both L.

This issue sounds extremely similar to #244.

Looking at this commit, it seems that we may be able to fix this after next ZXING release: https://github.com/zxing/zxing/commit/128775149b7d1a432d7ba05464739d549dbcd2b8

While honestly I believe it is a bug in the Verify Ontaria app if it can't read this, Catima should indeed still try to output the same as was given as input and that currently isn't happening.

toolstack commented 3 years ago

While honestly I believe it is a bug in the Verify Ontaria app if it can't read this, Catima should indeed still try to output the same as was given as input and that currently isn't happening.

Agreed on both points :smile:

It looks like zxing is in matinanace mode, no "active" development, so it may be a while before a release is done (though that commit was just done today, so mabye?). If they don't release something shortly any thoughts on pulling in the patched version before the release is complete?

toolstack commented 3 years ago

If they don't release something shortly any thoughts on pulling in the patched version before the release is complete?

I can confirm that using the current snapshot of ZXing, along with adding the QR_COMPACT hint to the Catima code, that the QR code is generated in a 4x4 matrix and is read correctly by the Verify Ontario app.

TheLastProject commented 3 years ago

I don't really want to use a snapshot version, because zxing is extremely important and we can't risk breakage there, but I would accept a PR that starts implementing recognizing the type of scanned code and saves that to generate the exact same code type.

But make sure to talk to @gschwaer if you want to take that on, because this month is Hacktoberfest so I don't want to risk passing up on the person who found such an issue first.

I probably won't merge anything until zxing releases something new, but I would mark it hacktoberfest-accepted as it is useful preparation work

toolstack commented 3 years ago

Would there be any case in which you don't want the smallest possible QR code?

If not, just adding the QR_COMPACT hint to all QR code generation would seem like the easiest fix. It's always going to return a valid QR code.

I don't see any obvious API from xzing to return what kind of QR code is decoded, just the data seems to be returned.

The only other thought I'd have if using it as default isn't tenable, would be to add a new barcode type to Catima, something like "QR Code Compact" which would use the hint, but be user selectable.

TheLastProject commented 3 years ago

Would there be any case in which you don't want the smallest possible QR code?

Yes, I don't want to suddenly start using a different type of QR code because it will confuse users.

Besides that, I have no clue if this compacting does anything for the error correction level.

I don't see any obvious API from xzing to return what kind of QR code is decoded, just the data seems to be returned.

I thought @gschwaer implemented this for Code128 in https://github.com/zxing/zxing/pull/1411, but it seems now like they only implemented the creating part, not the returning-what-was-scanned part, unless I'm misunderstanding the code :/

The only other thought I'd have if using it as default isn't tenable, would be to add a new barcode type to Catima, something like "QR Code Compact" which would use the hint, but be user selectable.

I do want something like this in general, but standardized because we already identified both QR and CODE 128 has multiple valid variants. In general (with proper readers), it also shouldn't matter, so this should perhaps be less in-your-face than regular different code types to not overload the user with options.

toolstack commented 3 years ago

I do want something like this in general, but standardized because we already identified both QR and CODE 128 has multiple valid variants. In general

Perhaps a third drop down on the "Barcode" tab when editing called "Subtype" or "Barcode Options". For QR it could be "Normal"/"None" (aka current behaviour) and "Compact".

TheLastProject commented 3 years ago

Okay, hearing this discussed more... lets just go for a regular list entry after all

So "QR Code (Compact)". And also we will use "Code 128 (A)" and "Code 128 (B)" and stuff. Simple and clear it's a subtype.

Making these selectable in the dropdown would be a good first step, probably with a new DB field called "BARCODE_SUBTYPE" which is null by default, for easier compat with zxing internals

gschwaer commented 3 years ago

I thought @gschwaer implemented this for Code128 in zxing/zxing#1411, but it seems now like they only implemented the creating part, not the returning-what-was-scanned part, unless I'm misunderstanding the code :/

Yes. If I remember correctly, the issue is that zxing has no support built in for returning more than the decoded data, the main QR code type, and the raw data. Also Code-128 can actually use mixed encoding, so not just A or B, but for example A and then at letter 6 switch to B and then after letter 10 back to A. So to find a way to express this is already complicated. But since for Catima, the actual subcode is of interest, we can parse the raw bytes and thus find out, if a single subtype was used and if so which one. For the QR code generation we can then force the selected subtype via the patch I implemented. That is the plan.

So "QR Code (Compact)". And also we will use "Code 128 (A)" and "Code 128 (B)" and stuff. Simple and clear it's a subtype.

Sounds good. Just keep in mind that there should maybe be a fallback type in case a mixed encoding is encountered.

TheLastProject commented 3 years ago

zxing has no support built in for returning more than the decoded data, the main QR code type, and the raw data [...] But since for Catima, the actual subcode is of interest, we can parse the raw bytes and thus find out, if a single subtype was used and if so which one.

Okay, that makes sense. What is this "raw data" like?

gschwaer commented 3 years ago

If I remember correctly you call OneDReader.decode(bitmap) at some point. This should return a Result, which contains a member called byte[] rawBytes. Those raw bytes can be checked for the codes here which specify the subcode. If more than one of them are present it is of mixed subtype. Not sure if it is possible that none is present. Something like (pseudo python code):

CODE_C = 99
CODE_B = 100
CODE_A = 101
MIXED = 42

result = OneDReader.decode()
subcode = None
for byte in result.rawBytes:
    if byte in (CODE_A, CODE_B, CODE_C):
        if subcode is not None:
            subcode = MIXED
            break
        subcode = byte
if subcode == None:
    raise MaybeSomeError('No subcode type found.')
TheLastProject commented 3 years ago

Hmm, I wonder if we can move this code into zxing, as it seems weird for Catima to read internals itself instead of ask zxing. But thank you for the info, that helps :)

gschwaer commented 3 years ago

That is true, but it is also kind of a special case that is only really interesting when you want to restore the QR code 1-to-1 and use Code128. I think the chances of the zxing maintainers accepting such a feature are slim considering zxing is in maintenance mode only.

TheLastProject commented 3 years ago

Well, of course we would want a generic method for Code128/QR/whatever. But I do realize getting the other code in already took quite a while yeah

Altonss commented 2 years ago

A new version of zxing has been released integrating the desired changes :)

TheLastProject commented 2 years ago

Okay, now we just need to:

  1. Create a new database table to store the EncodingHints in (as there may be more than 1 EncodingHints per barcode)
  2. Save the relevant EncodingHints during scanning
  3. Load the relevant EncodingHints during DB load of a barcode

Still gonna be a fair bit of code, but should be doable.

TheLastProject commented 2 years ago

Thinking about this more, it probably makes more sense to just implement only the EncodingHints we know what to do with as new colums in the LoyaltyCardDbIds table. They should then be nullable (null = unset).

Altonss commented 2 years ago

Thinking about this more, it probably makes more sense to just implement only the EncodingHints we know what to do with as new colums in the LoyaltyCardDbIds table. They should then be nullable (null = unset).

So which encoding hints would be relevant to add? I'm thinking about picking up this task as I already have some experience with writing database changes in Catima, but this might be a bit of work ^^

Edit: If I understood correctly, the both relevant encoding hints for the moment would be FORCE_CODE_SET and QR_COMPACT, am I right?

TheLastProject commented 2 years ago

For this specific issue only QR_COMPACT seems necessary, but we'll generally need to support more hints.

I think this will be easier to do if we refactor the whole CatimaBarcode class first. I started with this in the fix/barcodeRefactor branch but I haven't had time to work more on this

Altonss commented 2 years ago

For this specific issue only QR_COMPACT seems necessary, but we'll generally need to support more hints.

I think this will be easier to do if we refactor the whole CatimaBarcode class first. I started with this in the fix/barcodeRefactor branch but I haven't had time to work more on this

Thanks, I will have a look at this :)