Fix getTanModes if allowed tan mode not in all tan modes

[roben]

After finally receiving a https url for Commerzbank this was a final fix to get it working.

[nemiah]

Any comments on this one? @ampaze @Philipp91

[ampaze]

I wonder, how can one allowed tan mode not be included in the list of all tan modes?

[roben]

You'd have to ask the CoBa for that. Anyways, that is what they sent and the old code crashed with an error.

[Philipp91]

Could you please provide the concrete BPD and allowedTanModes that you received from the bank? Even if it's not fixable otherwise and we need to merge this patch, it's good to document here (and perhaps also in the commit message) why we did that, in case someone stumbles upon this in the future. It's an oddness that requires some justification for future programmers not to simply remove it again.

[roben]

Sorry for the late reply. Getting this data would be rather complicated because I'd have to ask a customer for access to his system and then mess around with his actual bank account.

Maybe the relevant part of our error report suffices to document it:

Error: Undefined array key 999

Backtrace:
[...]
library/Fhp/FinTs.php => error_function:534
?? => Fhp\{closure}:??
library/Fhp/FinTs.php => array_map:535
library/PicoErp/Hbci/Client.php => getTanModes:94
[...]

As you can see, just calling getTanModes() leads to a PHP error.

[Philipp91]

999 is a special value. I believe you want to use NoPsd2TanMode as shown in the README.

[roben]

I am not sure, because the customer was properly challenged with an image based TAN afterwards. According to the README, this tan mode is only needed for ING (which we already process this way).

Anyways, calling getTanModes() should not lead to a PHP error (as in error, not an Exception) because the bank did not return sane data. That's what this MR is for.

[Philipp91]

Ok fair enough.

[Philipp91]

Ah I guess the bank adds 999 to the list of TAN modes it supports. So an alternative implementation would be if ($tanModeId === TanMode::SINGLE_STEP_ID) continue;. What do you think? It would be more focused and more defensive coding, i.e. it would still allow us to discover different weird situation that warrant investigation. (In my experience, it's always better to surface errors than to swallow them.)

[roben]

That should be fine. We don't know if they also send something else, though, but that will be clear when this change is implemented. Sorry for the long delays between my replies here, I have a rather large backlog currently.

[DerSpezialist]

Hey there, is there a time frame when this fix will be implemented? I'm trying to get my bank statements from Commerzbank, but i'm currently running in to this issue.

Thanks in advance!

[nemiah]

Are we fine with this one?

[Philipp91]

Yes