search

skwasjer / IbanNet

C# .NET IBAN validator, parser, builder and generator
Apache License 2.0
119 stars 31 forks source link

Lowercased country code fails with InvalidStructureResult #24

Closed Hoffs closed 3 years ago

Hoffs commented 4 years ago

I have been looking at this for a while and can't find concrete information which way it should work. Right now I can see that the validation in StructureValidator tries to match first symbols with expected country code. As I see, because country code is lowercased it will not be equals to expected country code which is always uppercased:

https://github.com/skwasjer/IbanNet/blob/39f9b9adf2b5158903d1a738ee4f4f6cc2372a85/src/IbanNet/Validation/StructureValidator.cs#L18

But in other places, like Mod97 validation or finding iban country it uses upper cased ones, which makes it pass: https://github.com/skwasjer/IbanNet/blob/39f9b9adf2b5158903d1a738ee4f4f6cc2372a85/src/IbanNet/Validation/Rules/IsValidCountryCodeRule.cs#L43-L52 https://github.com/skwasjer/IbanNet/blob/39f9b9adf2b5158903d1a738ee4f4f6cc2372a85/src/IbanNet/CheckDigits/Calculators/Mod97CheckDigitsCalculator.cs#L47-L50

So I wonder, is this valid and expected behavior?

skwasjer commented 4 years ago

The check digits calculation is correctly ignoring case, as is the country code rule. I think this is a remnant of older implementation where IbanNet was built around Swift registry specifically, but was refactored to support multiple providers. This specific country code check should actually be made a 'test' and injected as a StructureSegmentTest, this would solve the problem and allow Swift provider to still be strict but allow more relaxed providers to pass this check.

That said, using strict validation, it would still fail when you'd have an IBAN with lowercase further up in the IBAN (for countries that require upper case), so the next debate would be, should we allow this at all? This is also why there is a loose validation mode. Alternatively, the SwiftStructureValidationFactory could be opened up a bit for extensibility/configurability.

So in short, yes, it is expected behavior, or at least, lets call it originally as intended. The entire IBAN could perhaps be uppercased to solve the problem, but I have to investigate if that breaks the spec.