search

bnw / firefly-iii-fints-importer

Import financial transactions from you FinTS enabled bank into Firefly III.
GNU Affero General Public License v3.0
151 stars 23 forks source link

Fix/enable decoupled tan submission #142

Closed JuliusFreudenberger closed 3 weeks ago

JuliusFreudenberger commented 1 month ago

Since I now also encounter the problem described in #131 I went ahead and tried to implement a solution. This is my first time writing PHP code, so please forgive me for doing things unusually.

This adds the basic functionality for decoupled TAN checks, presented by phpFinTs. I could confirm this working with my Sparkasse bank account.

However, this implementation has a caveat: Confirming the import in the pushTAN app is necessary every time an import is started. Also this adds an entry to the list of saved devices in device management (Geräteverwaltung). This is also an issue in phpFinTs: nemiah/phpFinTS#453, where it is described as an issue with the "kundensystemId"

bnw commented 3 weeks ago

Hey, thanks a lot for your contribution :) Code looks good. I can't test the code path myself, but let's hope for the best 🤞

JuliusFreudenberger commented 3 weeks ago

I tried it out using the docker container now, but I get the following error:

 Fatal error: Uncaught Fhp\Protocol\UnexpectedResponseException: Got neither 3956 nor HITAN with tanProzess=2 in /vendor/nemiah/php-fints/lib/Fhp/FinTs.php:490 Stack trace: #0 /app/TanHandler.php(40): Fhp\FinTs->checkDecoupledSubmission(Object(Fhp\Protocol\DialogInitialization)) #1 /app/TanHandler.php(31): App\TanHandler->create_or_continue_action() #2 /app/Login.php(21): App\TanHandler->__construct(Object(Closure), 'login-action', Object(Symfony\Component\HttpFoundation\Session\Session), Object(Twig\Environment), Object(Fhp\FinTs), Object(App\Step), Object(Symfony\Component\HttpFoundation\Request)) #3 /app/index.php(59): App\StepFunction\Login() #4 {main} thrown in /vendor/nemiah/php-fints/lib/Fhp/FinTs.php on line 490

I did not get this in my development environment, so I guess it has something to do with an older version of phpFinTs? At least to the commit reference in composer.lock points to a commit made more than 2 years ago.

bnw commented 3 weeks ago

I have reverted the PR. If you like, you can submit another PR that also includes an update of the phpFinTs library. Does this library update come with other API changes for which the importer needs to be adapted?

JuliusFreudenberger commented 3 weeks ago

Sadly I could not find changelogs for phpFinTs.. What certainly is included is a drop of support for PHP7.

I have no experience in dependency management in PHP. I guess, running composer update updates all dependencies, so there could be even more side effects. As I set up my dev environment, I used the most recent versions with composer update and importing works for me. Of course, I cannot test all other banks.

Would you accept another PR with an updated composer.lock with all updated dependencies or should a less invasive route be taken?

bnw commented 3 weeks ago

Ok, we can do another 🤞-PR. Please try to update as few packages as possible tho. See e.g. https://stackoverflow.com/a/16740711

Thanks a lot for your work! :)

JuliusFreudenberger commented 3 weeks ago

I will have a look into that, thanks for the hint! I'll open another PR when I'm ready.