Skip to content

Create Login Action in separate function call #475

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thoresuenert
Copy link

@thoresuenert thoresuenert commented Mar 28, 2025

Discussion started in #474 , because we PRed from the wrong branch we had to open this PR.

The Example:
In the Samples/browser.php example the flow is shared between front and backend code and have the assumption that a tan is only required once during login.

If a tan is required the flow of actions is stopped and restarted where we left.

Our Goal here is a flow that fits all actions:
We want update a SEPA Account:

  1. Login
  2. GetBalance
  3. GetNewTransaction
  4. Logout/Close Connection

We assume that each step can require a tanRequest flow, so the process can stop on each step and have to restart with a tan (coupled or decoupled). We want to treat every situation the same:

  1. POST Request /account/refresh to start the Process
  2. Handle Exception: tan is needed, show user interaction for all tan requests a like
  3. Continue with POST Request /account/refresh

Our Implementation

// $action is the action from the last call which needed a tan, or the action from the current step:
   $action = session()->pull('fints.action', $bank_connection->fints->createLoginAction());
// if the $action is from the last call and needs a tan and is not from this step:
// skip this step and continue with the next step
    if (! $action instanceof DialogInitialization) {
        return;
    }
// execute the action for the first time or try again with a tan
    $bank_connection->execute($action, $tan = request()->input('tan'));
// if the action needs a tan, fire an expcetion and show the tanRequest to the user
    if ($action->needsTan()) {
        $this->handleTanRequiredException($action);
    }

This is our idea of an implementation of the stop/restart mechanism the tanRequest needs.

Extract login action creation into single function to make it accessible for own login interaction
@thoresuenert
Copy link
Author

          Calling
        $this->requireTanMode();
        $this->ensureSynchronized();
        $this->messageNumber = 1;

inside of createLoginAction seems wrong to me.

I am not sure I understand the problem this PR is trying to solve.
If you continue after a TAN request, you don't need a login, otherwise you can always login, execute your actions and logout.

Originally posted by @ampaze in #474 (comment)

Should every execute of an action call $this->requireTanMode(); and $this->ensureSynchronized(); because in the mean time things can change?

@Philipp91
Copy link
Contributor

I suggest you do this (not literally, but structurally):

function yourUserWantsToLogIn($credentials) {
  $action = $fints->login($credentials);
  commonTanHandling($action);
}

function yourUserWantsTheBalance() {
  $action = createBalanceAction();
  $fints->execute($action);
  commonTanHandling($action);
}

function commonTanHandling($action) {
  // The things you have above, without execute() calls and perhaps restructured a bit.
}

Note how the TAN handling needs to be implemented only in one place, no matter whether it's a login or a GetBalance or something else that requires the TAN.

@ampaze
Copy link
Contributor

ampaze commented Mar 31, 2025

Should every execute of an action call $this->requireTanMode(); and $this->ensureSynchronized(); because in the mean time things can change?

No, I meant that these lines of code should be called when you want to login, not when you create the login action.

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.

4 participants