fix usage of kundensystemId #471

nemiah · 2025-01-06T08:46:01Z

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

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

nemiah · 2025-01-06T08:49:55Z

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 · 2025-01-06T08:51:11Z

lib/Fhp/FinTs.php

@@ -589,6 +589,8 @@ public function selectTanMode($tanMode, $tanMedium = null)
        }
        $this->selectedTanMode = $tanMode instanceof TanMode ? $tanMode->getId() : $tanMode;
        $this->selectedTanMedium = $tanMedium instanceof TanMedium ? $tanMedium->getName() : $tanMedium;
+
+		$this->kundensystemId = $this->options->kundensystemId;


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

Philipp91 · 2025-01-06T08:52:24Z

lib/Fhp/Options/FinTsOptions.php

+	 * The Kundensystem-Id as returned by the bank and persisted by the application code
+	 * Prevents having to re-authenticate every time on login
+	 * Use DialogInitialization::getKundensystemId() on the return-object of FinTs::login(), to get the new one
+	 * @var string


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

Philipp91 · 2025-01-06T08:55:06Z

lib/Fhp/Options/FinTsOptions.php

+    /**
+	 * The Kundensystem-Id as returned by the bank and persisted by the application code
+	 * Prevents having to re-authenticate every time on login
+	 * Use DialogInitialization::getKundensystemId() on the return-object of FinTs::login(), to get the new one


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 · 2025-01-06T08:55:39Z

lib/Fhp/FinTs.php

@@ -589,6 +589,8 @@ public function selectTanMode($tanMode, $tanMedium = null)
        }
        $this->selectedTanMode = $tanMode instanceof TanMode ? $tanMode->getId() : $tanMode;
        $this->selectedTanMedium = $tanMedium instanceof TanMedium ? $tanMedium->getName() : $tanMedium;
+
+		$this->kundensystemId = $this->options->kundensystemId;


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

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

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 · 2025-01-06T10:04:57Z

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

roben · 2025-01-06T13:32:03Z

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

Philipp91 · 2025-01-06T13:44:38Z

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 · 2025-01-06T20:29:40Z

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 · 2025-01-06T20:38:40Z

lib/Fhp/Options/FinTsOptions.php

+     * This is optional, but it prevents having to re-authenticate every time on login 
+     * if not using a persisted instance with FinTs::persist()
+     * Use DialogInitialization::getKundensystemId() on the return object of FinTs::login(), to get the current Kundensystem-Id
+     * @var string


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

Philipp91 · 2025-01-06T20:49:57Z

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 · 2025-01-06T21:12:08Z

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 · 2025-01-06T21:17:48Z

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 · 2025-01-06T21:38:50Z

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 · 2025-01-09T19:38:34Z

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:

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

Please choose 😁

fix usage of kundensystemId

7cd3ddd

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

nemiah mentioned this pull request Jan 6, 2025

Sparkasse vertrauenswürdiges Gerät #470

Closed

Philipp91 reviewed Jan 6, 2025

View reviewed changes

nemiah added 3 commits January 6, 2025 10:56

fix indentation

03f1b34

fix indentation

515f9b5

fix description

5063003

Philipp91 reviewed Jan 6, 2025

View reviewed changes

nemiah mentioned this pull request Jan 9, 2025

DKB issues since server change #468

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix usage of kundensystemId #471

fix usage of kundensystemId #471

nemiah commented Jan 6, 2025 •

edited

Loading

nemiah commented Jan 6, 2025

Philipp91 left a comment

Philipp91 Jan 6, 2025

Philipp91 Jan 6, 2025

Philipp91 Jan 6, 2025

Philipp91 Jan 6, 2025

nemiah Jan 6, 2025

Philipp91 Jan 6, 2025

nemiah commented Jan 6, 2025

roben commented Jan 6, 2025

Philipp91 commented Jan 6, 2025

nemiah commented Jan 6, 2025 •

edited

Loading

Philipp91 Jan 6, 2025

Philipp91 commented Jan 6, 2025

roben commented Jan 6, 2025

Philipp91 commented Jan 6, 2025

roben commented Jan 6, 2025

nemiah commented Jan 9, 2025

fix usage of kundensystemId #471

Are you sure you want to change the base?

fix usage of kundensystemId #471

Conversation

nemiah commented Jan 6, 2025 • edited Loading

nemiah commented Jan 6, 2025

Philipp91 left a comment

Choose a reason for hiding this comment

Philipp91 Jan 6, 2025

Choose a reason for hiding this comment

Philipp91 Jan 6, 2025

Choose a reason for hiding this comment

Philipp91 Jan 6, 2025

Choose a reason for hiding this comment

Philipp91 Jan 6, 2025

Choose a reason for hiding this comment

nemiah Jan 6, 2025

Choose a reason for hiding this comment

Philipp91 Jan 6, 2025

Choose a reason for hiding this comment

nemiah commented Jan 6, 2025

roben commented Jan 6, 2025

Philipp91 commented Jan 6, 2025

nemiah commented Jan 6, 2025 • edited Loading

Philipp91 Jan 6, 2025

Choose a reason for hiding this comment

Philipp91 commented Jan 6, 2025

roben commented Jan 6, 2025

Philipp91 commented Jan 6, 2025

roben commented Jan 6, 2025

nemiah commented Jan 9, 2025

nemiah commented Jan 6, 2025 •

edited

Loading

nemiah commented Jan 6, 2025 •

edited

Loading