Swiss QR Code iban error

bhoewler commented 5 years ago

Type of issue

[X] Bug
[ ] Question (e.g. about handling/usage)
[X] Request for new feature/improvement

Expected Behavior

Correct checking of iban content.

Current Behavior

iban as example is "CH2609000000857666015" but in fact, content may have letter in some places in string.

Possible Solution (optional)

Use standard iban validator, I found this one using with ISO20022.

public class IBANValidator
{
    public Boolean CheckIBAN(System.String iban)
    {
        if (iban.Length < 4 || iban[0] == ' ' || iban[1] == ' ' || iban[2] == ' ' || iban[3] == ' ') throw new InvalidOperationException();
        var checksum = 0;
        var ibanLength = iban.Length;
        for (int charIndex = 0; charIndex < ibanLength; charIndex++)
        {
            if (iban[charIndex] == ' ') continue;
            int value;
            var c = iban[(charIndex + 4) % ibanLength];
            if ((c >= '0') && (c <= '9'))
            {
                value = c - '0';
            }
            else if ((c >= 'A') && (c <= 'Z'))
            {
                value = c - 'A';
                checksum = (checksum * 10 + (value / 10 + 1)) % 97;
                value %= 10;
            }
            else if ((c >= 'a') && (c <= 'z'))
            {
                value = c - 'a';
                checksum = (checksum * 10 + (value / 10 + 1)) % 97;
                value %= 10;
            }
            else throw new InvalidOperationException();
            checksum = (checksum * 10 + value) % 97;
        }
        return checksum == 1;
    }    
}

Steps to Reproduce (for bugs)

Using iban like "CH1900767000U00121977"

Your Environment

Version used: 1.3.5.0 Compiled from: NuGet

bhoewler commented 5 years ago

Additional source : ISO 13616

https://www.swift.com/sites/default/files/resources/iban_registry.pdf and specialy pages 5 (common format explanation) and 76 (Switzerland : CH2!n5!n12!c) by example.

The original code (2+2+4+7+9=24 pos. but not correct for all countries)

private static bool IsValidIban(string iban)
{
  return Regex.IsMatch(iban.Replace(" ", ""), @"^[a-zA-Z]{2}[0-9]{2}[a-zA-Z0-9]{4}[0-9]{7}([a-zA-Z0-9]?){0,16}$");
}

could be upgrade (or simplified to a minimal check) to (2+2+30=34 max pos, minimum 20)

private static bool IsValidIban(string iban)
{
  return Regex.IsMatch(iban.Replace(" ", ""), @"^[a-zA-Z]{2}[0-9]{2}([a-zA-Z0-9]?){16,30}$");
}

or must be changed as to be equivalent/like of IBAN validator for ISO 20022 standard.

codebude commented 5 years ago

Hi @bhoewler ,

I changed the Regex to the version you suggested and added a checksum check additionally. Also I added some test cases. You can find the changes in https://github.com/codebude/QRCoder/commit/54213f9cc2ca49cced082cb5cbdee83e93a651e4 .

Let me know, if this solves your issue and if I can close it.

bhoewler commented 5 years ago

Hi @codebude , Many thanks, it seems good, will be happy to upgrade by your next NuGet release, then restart testing for implementation.

codebude commented 5 years ago

Thanks for your feedback. Let me know, if you find any problems. The new package will be released this evening.

bhoewler commented 5 years ago

So, I restart testing with my app and the case we use as QrIban/QRR/QrReference (26+1 pos) and all seems ok after some adaptations. To improve, in your test-example, it seems one parameter missing (additionalinformation, after reference) in: PayloadGenerator.SwissQrCode generator = new PayloadGenerator.SwissQrCode(iban, currency, contactGeneral, reference, **null**, null, amount, delay, null, null, null); In code I tried to update (from 1.3.5), it was different and working good before. I checked the image in UBS validator and all is ok. Question : did you plan to have a method for "utlimate Debtor" ? Thanks you so much !

codebude commented 5 years ago

Hi @bhoewler ,

thanks for the feedback. The addionalInformation field was added due to refactoring for Swiss QR code version 2.0. Do you mean you had it to add to your test methods or was it missing in one of my unit tests?

Regarding ultimateDebitor - it is (and was) already there. Check the constructor:

public SwissQrCode(Iban iban, //CdtrInf
                   Currency currency, //Ccy
                   Contact creditor, //Cdtr
                   Reference reference, //RmtInf
                   AdditionalInformation additionalInformation = null, //AddInf
                   Contact debitor = null, //UltmtDbtr
                   decimal? amount = null, //Amt
                   DateTime? requestedDateOfPayment = null, //Obsolete since Swiss QR code v2.0 - my fault. Has to be removed
                   Contact ultimateCreditor = null, //UltmtCdtr
                   string alternativeProcedure1 = null, //AltPmt
                   string alternativeProcedure2 = null //AltPmt
                   )

The parameter "debitor" is the ultimate debitor. The parameter "creditor" is the creditor and "ultimateCreditor" is the ultimate creditor. So you can pass the ultimate debitor via "debitor" parameter an it will also be used.

Or did you mean "ultimate creditor"? Because this can be passed to the constructor, but won't be used in the QR code, because the standard (https://www.paymentstandards.ch/dam/downloads/ig-qr-bill-en.pdf) marks it on page 26 as "X" which means "has to be delivered, but fields must not be filled". I just added the parameter to the constructor so that the method call doesn't change later in future when SIX decides to use the ultimateCreditor.

bhoewler commented 5 years ago

Hello @codebude ,

Oh sorry, not in your unit tests but in the sample to use here : https://github.com/codebude/QRCoder/wiki/Advanced-usage---Payload-generators#317-swissqrcode-iso-20022, it seems one missing parameter: SwissQrCode generator = new SwissQrCode(iban, currency, contactGeneral, reference, null, amount, null, null); I had as example for each line code in my app and from upgrade to 2.0, had an error on convert amout but caused by one missing before … and the new null/use at the end. Another point in the same example, for QRR/QrReference (27 pos.), now we need to use QrIban, begining of page 29 in https://www.paymentstandards.ch/dam/downloads/ig-qr-bill-en.pdf

Sorry 2, will check for ultimateDebtor (Debtor), casue I presume bank will ask for even IBAN, as double check. As now, we don't with BVR/reference cause it goes to "account" (postal system>bank>CAMT).

For Creditor/Debtor, I have to use "unstructured" cause have 4 lines for name/address but don't now where is street/PObox (could be 2/3/4) … + other distinct fileds for PostalCode / City / Country, grrrrrr.

If you have test to do, the SIX validator is very bad (two different tools, v.1 only stable, error when cli-to-go documentation), the UBS tool is better and can do PAIN/XML and QR in the same interface ;-)

Thanks a lot, will continue testing next week.

codebude commented 5 years ago

Sorry for the errors in the Wiki examples. I hadn't updated them so far. But now the example in the wiki should be fine.

For Creditor/Debtor, I have to use "unstructured" cause have 4 lines for name/address but don't now where is street/PObox (could be 2/3/4) … + other distinct fileds for PostalCode / City / Country, grrrrrr.

From my understanding you should use "structured" contact type. In structure you have four lines which will be filled by the Contact-class constructor parameters "name", "street" + "houseNumber", "zipCode" and "city".

If you use unstructured, only "name" and "addressLine1" and "addressLine2" will be used.

bhoewler commented 4 years ago

Hi, I just want to say : congrats and many thanks ! First invoices are out and good paid ... so all works fine till now.

codebude / QRCoder