search

nemiah / phpFinTS

PHP library to communicate with FinTS/HBCI servers
MIT License
131 stars 40 forks source link

Introduced Flicker Tan handling (only Version 1.4 so far) #357

Closed lukas-staab closed 2 years ago

lukas-staab commented 2 years ago

Introduced Flicker Tan Version 1.4 Used SVG Libary. Let me know if the dependency should be dropped I have added some sample code to the Samples, it will work together with browser.php but it seems cli support was and is missing (no tan files are saved)

Interfacing the TanRequest Challenge Handlers might be a good idea, or generating a class which looks at the given Challenge and tries to guess by length and mime type which challenge it is (flicker, photo,...)

Note: even before my new require the composer.lock was not installable in php 7.1 (without update) therefore i tested and generated the lockfile with php 7.4

lukas-staab commented 2 years ago

see #356 for more details

lukas-staab commented 2 years ago

Happy new year bump

@Philipp91 @nemiah

Let me know If any changes needed :)

Philipp91 commented 2 years ago

Used SVG Libary. Let me know if the dependency should be dropped

Yes, I think it would be better to have this in a separate library (which you are welcome to advertise in the phpFinTS documentation in a few applicable places). Try to make the smallest cut somehow -- hopefully there's a way to reduce the interface between the libraries to one or two strings like it's done here.

The reason I think this would be the right solution is because (by far) not all applications will need this dependency. Even an application that has the requirement to support all banks (including those with Flicker TANs) may well be implemented such that the server serves REST/JSON and not HTML directly, and such that the SVG rendering could be done on the client (which could be an Android app written in Kotlin or a JavaScript client, which might have different or even dynamic values for paramters like $freq and $width). As far as I can tell (correct me if I'm wrong) all those cases wouldn't need/want the server-side SVG rendering.

Also, from an ownership point of view, if you could maintain that flicker rendering library, the expertise for further maintenance and reviewing PRs would be in the right place.

Assuming that this whole thing isn't time-sensitive on your end, I'll revisit this PR once the SVG part has been moved out -- then it's less to read :-)

lukas-staab commented 2 years ago

Try to make the smallest cut somehow -- hopefully there's a way to reduce the interface between the libraries to one or two strings like it's done here.

That should be possible

which might have different or even dynamic values for paramters like $freq and $width). As far as I can tell (correct me if I'm wrong) all those cases wouldn't need/want the server-side SVG rendering.

You have a point, server side rendering might not be the best solution there, but SVGs can as far as I remember be dynamical adapted with css variables or js easily, so rendering server side might still be comfortable - so all the applications do not need to write the same code over and over again.

Assuming that this whole thing isn't time-sensitive on your end, I'll revisit this PR once the SVG part has been moved out -- then it's less to read :-)

Its not really. I am using my code live already, so no issue there. I will think about the interfacing again and come back to you.

One further suggestion: I could also rebuild the part of the svg Lib which I need (it is not really much) so there would not be a dependency, but the (optional) functionality inside fints. What do you think about that?

Philipp91 commented 2 years ago

One further suggestion: I could also rebuild the part of the svg Lib which I need (it is not really much) so there would not be a dependency, but the (optional) functionality inside fints. What do you think about that?

I'm not strongly against, though a separate library would still have the benefit of separate maintenance. Your choice.

nemiah commented 2 years ago

To merge or not to merge? :thinking:

lukas-staab commented 2 years ago

@nemiah I will do some rework and come back to you :)

nemiah commented 2 years ago

ok, great :relaxed:

lukas-staab commented 2 years ago

I am done with my rework. Some hints, which might help for review:

Philipp91 commented 2 years ago

It might be a good Idea to build a Wrapper Class which inputs the TanMode and TanRequest and does the guessing which parser and Renderer should be used for the HhdUc Challenge, worst case by trial and error or with a educated guess by TanModeName (is there a more sophisticated method?)

Good idea. Not a "wrapper" class, just a "helper" class.

According to the specification, the value of this field is what matter. See section II.4.1 on PDF page 47.

lukas-staab commented 2 years ago

I think I addressed all of the review remarks (the helper is not there yet, but in my opinion not urgent for the merge)

nemiah commented 2 years ago

In general I'm all for using already existing extensions. Except when they are too uncommon and nobody has installed them. I can't control where my users install my software and therefore I usually package many dependencies along with my application.

Please remove composer.lock file from the commit and we should be fine?

lukas-staab commented 2 years ago

merging upstream into my fork (I dropped my composer.lock at the merge, so that issue should be fine)

lukas-staab commented 2 years ago

If everything is fine I would suggest a new release (at some point), there have been some PR and there is also a issue about a new release (for the prs/log v3)