Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/enable decoupled tan submission #142

Merged

Conversation

JuliusFreudenberger
Copy link
Contributor

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"

This is required for example for TAN mode 923 called pushTAN 2.0 by Sparkasse.
This mode requires a confirmation in the pushTAN app but no TAN to be entered.
The implementation presents the same tan-challenge screen without the text input. Instead of submitting a TAN, the decoupled submission is checked through the fints-library.

Refs bnw#131
This enables the selection of the new pushTAN 2.0 authentication by Sparkasse.

Refs bnw#131
@bnw
Copy link
Owner

bnw commented Sep 8, 2024

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 🤞

@bnw bnw merged commit 775119f into bnw:master Sep 8, 2024
@JuliusFreudenberger JuliusFreudenberger deleted the fix/enable-decoupled-tan-submission branch September 11, 2024 08:42
@JuliusFreudenberger
Copy link
Contributor Author

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
Copy link
Owner

bnw commented Sep 11, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

bnw commented Sep 11, 2024

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants