fix usage of kundensystemId

[nemiah]

kundensystemId can now be set on FinTsOptions and will be used on next login to prevent having to re-authenticate:

$options->url = $url;
$options->bankCode = $blz;
…
$options->kundensystemId = $kundensystemId; //get $kundensystemId by calling DialogInitialization::getKundensystemId() on the return-object of FinTs::login()

See #458 and #453 and #470

[nemiah]

Any comments? ☺️

[Philipp91]

An alternative solution would be to introduce a third persist() tier, which includes minimal plus the kundensystemId but not the large blobs.

Relatedly: Why are you opposed to persisting the whole instance? I vaguely recall the specification advocating for that.

[Philipp91]

Wrong indentation (also in the other file), please run the auto-formatter (cs-fix?)

[Philipp91]

Make it clear by the type here and in the explanation above that this is optional. And give it a null default value.

[Philipp91]

Nit: "return object" without dash (compound nouns in English don't use dash)

My actual question: Why does it say "the new one"? Isn't that the "only" one?

I see two potential misunderstandings (of a library user) here:

• Am I supposed to run login() every time and then get the kundensystemId?
• After getting the kundensystemId, do I need to put it into FinTsOptions right away? And then what, create a new FinTs object right away?

Probably it would help to explain how this interacts with persistence, namely that it's meant to be stored in the database and be used the next time a FinTs instance is created, in a new PHP run. And that it's redundant if full persist() is used.

[Philipp91]

Why does this happen within selectTanMode() and not in the same place where the persist() stuff is read back?

[nemiah]

Because it works here, no other reason 🤷‍♀️

[Philipp91]

I'd move it into the new() function around line 88, and either put it into the else branch (i.e. only run it when $persistedInstance === null) or into an if ($this->options->kundensystemId === null).

(Without that if protection, it will break any user that doesn't populate the field, because it will overwrite the restored kundensystemId with null -- in fact I'm surprised no tests are breaking.)

[nemiah]

Ok, fixed the indentation and clarified the description as far as I understood.

[roben]

Great idea to put this in the options. And please don't add another persist tier. It's confusing enough as it is.

[Philipp91]

I just checked the persist() implementation, and actually even the "minimal" version of it already contains the Kundensystem-ID.

So I guess you're only running into the original issue #470 because you're not using persist() at all?

[nemiah]

I'm using persist() to save the session until the TAN is available. I had no reason to save it longer, until the Sparkasse got too annoying…

Great idea to put this in the options. And please don't add another persist tier. It's confusing enough as it is.

Yes, I think so, too. It fits nicely with very few new lines of code and there is already a getKundensystemId-method in the return object of login(). I only have to remember the Kundensystem-ID and not a whole serialized PHP object.

[Philipp91]

     * @var string|null
     */
    public $kundensystemId = null;

[Philipp91]

Great idea to put this in the options.

While I ultimately don't care, I'd still like to point out that it doesn't square with the original design of that class. As per the documentation, it's meant to be "Configuration options for the connection to the bank." In particular, you could share a single such object across multiple users. Some applications might have a database table where they store all the supported banks plus their parameters in the form of a JSON-serialized FinTsOptions object. The addition of the user-specific kundensystemId would now require such applications to augment the object before passing it into new(). So this change essentially reinterprets it as a parameter object of the new() function (except that it doesn't include the credentials and persistedInstance parameters, so it's not complete either).

Conceptually, the Kundensystem ID is like a session token. So it's sensitive authentication data and would perhaps fit better into Credentials, though I'm not sure whether that would cause other issues.

[roben]

What about adding it as an optional third parameter to selectTanMode? That's exactly where and when it needs to be set, anyways.

But here is another caveat: If you restored a persisted TAN request, the kundensystemId may already be set and different. In that case, it must not be overridden but extracted to update the old id.

This is how I currently have implemented it (simplified version, with accessors from my fork):

$this->fints->selectTanMode($tan_mode, $media);
// restore persisted system id, see https://github.com/nemiah/phpFinTS/issues/453
// must not be set for get_supported_tan_modes() (which must be called before init_account)
// If id is already set it was restored from a persisted instance (see constructor) during a TAN request and will be updated in $this->persist_state().
if (null === $this->fints->getKundensystemId() && null !== $system_id) {
	$this->fints->setKundensystemId($system_id);
}

[Philipp91]

What about adding it as an optional third parameter to selectTanMode? That's exactly where and when it needs to be set, anyways.

Why is that the right place and not the constructor?

The constructor solution seems to fit the lifecycle better (it's restoring state from a previous lifetime of the FinTs object and has nothing to do with TAN mode selection) and in your code you could get rid of the null === $this->fints->getKundensystemId() check and the corresponding explanation too (see my else-based proposal above).

[roben]

Why is that the right place and not the constructor?

It just feels a bit too explicit / naked there next to the three current more generic parameters. But after considering everything it may be the best place to put it.

[nemiah]

As I ultimately care little about the specific implementation and more about the outcome, this discussion is too complicated for me. Therefore I propose the following solutions:

1. someone with more interest in the implementation details creates a new PR with the functionality and I'll close this one
2. I'll merge my PR and someone with more interest in said implementation details fixes it to their liking in another PR
3. I'll merge my PR and we leave it at that

Please choose 😁

[Philipp91]

tl;dr: Feel free to submit, perhaps with style nits addressed. Be aware that this approach (not this code) might break once banks start rotating session tokens (something we need to address also on the library side, see #458), at which point this approach will need additional (and uglier-looking) amendments both in the library and application code (!) or otherwise users will get logged out sporadically (hard to debug). I believe this is by design and the correct design is use persist() every time you stop using a FinTs instance and restore it the next time, which works today and wouldn't need any adjustments even when #458 comes around.

---

Looks like it's not (1.) ... Actually I tried coming up with a cleaner design, but it's very difficult because, at least in my mind, this functionality shouldn't exist in the first place. Thus I struggle to find "the right" place for it. To me, it's still something that should be covered by persist() and restore in the constructor.

I only have to remember the Kundensystem-ID and not a whole serialized PHP object.

It's not like you have to remember it in your head though. It's just a slightly longer string that your app moves into a database and back.

For what it's worth, I find it easier to convey to library users: "Please call $fints->persist() on the old instance and take care to pass the returned value to the FinTs() constructor for the new instance -- that way, the state is retained and the end user won't be logged out."

---

$options->kundensystemId = $kundensystemId; //get $kundensystemId by calling DialogInitialization::getKundensystemId() on the return-object of FinTs::login()

This may look like a working solution today. But firstly, it's more difficult to explain to library users. Basically "Please create an instance, then log in, take the returned object and call a bunch of functions on it, then store that in your DB. And when you create an instance in the future, pass that value to the options struct."

Secondly, it will stop working once #458 hits. It's an open bug and we haven't had to implement it because banks haven't implemented it on their side yet. Some banks may never choose to. But basically it allows banks to rotate the session token, similar to how OAuth does it with a refresh token. This means that the Kundensystem-ID can expire and a reasonable amount of time before that happens, the server can inform the client, which can transparently get a new Kundensystem-ID without requiring separate authentication from the end user. That way, if the old Kundensystem-ID ever leaks, it will hopefully have been invalidated by then.

Your application (in the form I imagine it to be implemented based on the accessors you're adding in this PR) will only ever update the Kundensystem-ID in its database when a login happens, and then re-supply that to the options in the constructor forever. That will effectively log the user out and force them to reauthenticate as soon as the first bank follows through with this.

If we continue the design started by this PR, these could be the solutions to that new problem:

1. Add a new FinTs::getKundensystemId() (or make the field public; I believe that has been proposed as PR or hack somewhere already) and ask every application to read that field after every action they execute and to store it in the DB (at least in case it differs). That way, the app will get the updates. But it's quite cumbersome for the app code. And any application developer who forgets this will log out their users after some timeout, perhaps every couple weeks, or for some banks every couple days. It's very difficult to debug this kind of thing.
2. Pass a callback onNewKundensystemId() through FinTsOptions or so, which allows the library to inform the application about an updated token, so that the application can store it in the DB. Also relatively clumsy and personally I'm not a fan of callbacks. But at least we could force library users to give us that callback, so it's less likely for them to forget to implement it.

---

Let's contrast this with persist() and restore: That already works today for anyone who uses it. It will transparently continue to work, no matter how the library gets developed further in terms of how it handles the Kundensystem-ID or anything else for that matter. We can implement #458 without any application code needing adjustments in terms of persist(). And anyone who doesn't use it is free to do so, they are simply logged out every time, which makes it easier to notice and debug.

---

If you still want to submit this because it's what makes your application work (and potentially others' too), even if it's only until it breaks when banks change their implemention, then I don't mind if you go ahead. As long as we have a proper way to use the library and it's supported and the one we recommend (e.g. demonstrate in example code), it hopefully won't cause trouble. You could consider fixing the two nit comments from above and adjusting the indentation from tabs to spaces (cs-fix should yell actually?).

[Philipp91]

You should probably also wire it up here, so that people don't accidentally leak their session tokens when posting logs on GitHub.

[nemiah]

@Philipp91 I value you taking so much time and writing all of that up and therefore I'll add another option. I'll just leave it in my code until it breaks. If someone else wants it, it's a few lines of code.