Cleanup TfaTrustedBrowser Plugin save logic

Shreya_th → made their first commit to this issue’s fork.

I have created fix for the patch, please review the MR #50

@Shreya_th I'm not sure what your MR was trying to accomplish? It doesn't appear to be related to this issues fault.

Regarding the root fault of this, yes the changes in !MR50 should stop the error, this originates from adding strict type hints in 📌 Add return, property, and parameter type hints to code Fixed , however there is a wider scope question, is this property even necessary?

  public function submitForm(array $form, FormStateInterface &$form_state): void {
    $trust_browser = $form_state->getValue('trust_browser');
    if (!empty($trust_browser)) {
      $this->setTrusted($this->generateBrowserId(), $this->getAgent());
    }
  }


  public function finalize(): void {
    if ($this->trustBrowser) {
      $name = $this->getAgent();
      $this->setTrusted($this->generateBrowserId(), $name);
    }
  }

$this->trustBrowser is never going to be anything but FALSE, it is never set during our login operations. Even if it were set to a non-false value it is attempting to duplicate actions that are done previously.

This holds true back to the 8.x-1.x branch as well, so we have been doing it this way for a long time.

The significant question of this is, should this be done in finalize() or submitForm(), and was this part of a conversion process that was never completed, or a portion of our API in 8.x-1.x that just didn't develop into being needed. I have not yet had time to look through the commit logs to determine what issues were involved in this code being created in this way.

We should find those answers first before we choose our solution as that will determine what fix we need.

Related commit: ebef2da3a9d4cee63f18fa0e874e2be0bef9745d
Reverting the relevant plugin change appears to be functional against current dev appears to work. Unknown why they were changed.

TfaLoginInterface::class does not actually list the finalize() method on interface so in that regard its an optional call.

Since the finalize() in TfaTrustedBrowser always requires that a user confirm if they wish to add the browser this isn't a scenario where we can make the call without the form.

We have already proceed through form validation by the time submitForm() is called so there is no logical reason that I can see that it needs to be handled in a separate step, especially since its handled as part of the same EntryForm::submitForm() call.

Given this, I would suggest we:

• Remove the finalize method from TfaTrustedBrower
• Remove the $trustBrowser property

We will likely want a followup call to discuss the finalize() method call in EntryForm, either it should be added to the TfaLoginInterface if we want it to continue existing, or it should be removed.

I would suggest first lets fix the current issue ASAP by committing changes in the current changes in MR , and work on making the proper fix as sites break due to this issue.

I would suggest first lets fix the current issue ASAP by committing changes in the current changes in MR , and work on making the proper fix as sites break due to this issue.

I'm generally accepting of expedited fixes when the larger fix will take significant amount of time and effort and that the expedited fix has minimal negative impact. While this MR as it exists appears to have minimal(no) negative impact the issue itself does not appear as if it would take significant time or effort to resolve.

As this issue stands currently it is just going to be a removal of a property, and removal of a method. I'm not even expecting there to be any test code to cleanup since the testing branch is currently passing. The comments in #8 should be a very quick fix to implement (quick glance it appears we just need to delete 19 lines of code).

Additionally this currently only impacts the dev branch of 2.x. However this also means it should not be impacting production sites as 2.x is still in early alpha, and due to open work on 📌 SA-CONTRIB-2023-030 and 2.x Active the 2.x branch (and alpha2 release is also code that should not be used anywhere (sites should be on 8.x-1.x.)

cmlara → closed merge request !49

Automatically closed - issue fixed for 2 weeks with no activity.