dev/drupal#54 Remove hook_user_login, fixes the masquerade module

[mlutfy]

The hook_user_login implementation was added by Torrance during the initial port to Drupal8. The Drupal7 implementation does not implement this hook.

It causes problems with the Drupal 'masquerade' module, causing the user to be logged out when attempting to masquerade. This is because the synchronizeUser() function resets the PHP session.

More details here:
https://lab.civicrm.org/dev/drupal/issues/54#note_23652

I tested the following use-cases:

• As a Drupal admin, I created a new Drupal user, then logged-in with that user. The contact+user were in sync.
• As a CRM admin, with the Actions -> Create CMS account, I created a Drupal account for a user, then logged-in with that account. Also OK.

Looking on the initial commit (29b7571), there doesn't seem to be a reason for the hook, so it feels safe to remove.

[mlutfy]

On the first (normal) login, the CiviCRM session stores the userID and ufID, but when the user masquerades and synchronizeUser gets called again, the old session ufID doesn't match anymore and so it does a hard reset of the session (causing a logout).

Using $session->reset(FALSE) does a soft reset of the CiviCRM-specific session bits, so that the call to synchronize then re-updates the ufID and whatever is required.

Finally, I tried to reduce the scope of this fix, in case it causes regressions (my first attempt had broken the dashlets). I would have use \Drupal::service('masquerade')->isMasquerading(), but for some reason it did not work and I could not figure out why.

[mlutfy]

This might not be necessary, but the other functions do it, so I figured it was safer. I didn't want to create a new wrapper function in src/Civicrm.php just for getSession or equivalent (as was done for the synchronize function)

[monishdeb]

just to reduce LOC

function civicrm_user_login(AccountInterface $account) {
  $civicrm = \Drupal::service('civicrm');
  $civicrm->initialize();

  if (\Drupal::hasService('masquerade')) {
    // In theory this is harmless whether we masquerade or not, but for now narrowing
    // the scope, in case it causes regressions.
    // Also, there is a isMasquerading() function, but it does not seem to be
    // initialized at this point (it always returns false).
    \CRM_Core_Session::singleton()->reset(FALSE);
  }
  $civicrm->synchronizeUser($account);
}

[mlutfy]

👍 fixed

[monishdeb]

thanks :)

[hansrossel]

It works!

[mlutfy]

I did another force-push to address another odd behaviour encountered with the Dashboard dashlets.

[stesi561]

Is there some reason that the behaviour: " logged out when attempting to masquerade" would not be observed? I've just being testing this on a pretty vanilla d8 site 8.7.6 civicrm 5.16.2, masquerade 8.x-2.0-beta2 with and without this PR - I can masquerade through to another user - but am unable to unmasquerade (clicking the link results in a log out). Am I missing something basic here?

[mlutfy]

@stesi561 Maybe relevant: in most of my test-cases, the Drupal frontpage is the CiviCRM dashboard (/civicrm/dashboard).

Just to double-check: when testing, were you accessing CiviCRM before & after masquerading? I suppose the bug might not trigger if you masquerade immediately after the initial login (since the CiviCRM session will not have been initialized yet).

[stesi561]

Hmm. Yes okay - I can replicate the problem behaviour consistently now.

However applying the above patch doesn't seem to resolve?

Steps to reproduce:
Apply the patch,
clear caches wipe templates_c,
log in
Navigate to /civicrm
Masquerade as another user
This seems to succeed
Then navigate to another page - you are now logged out.

I've upgraded the site I was using to test on to civicrm 5.17.4, and drupal 8.7.7 and tested again.

This is on a drupal site installed using roundearth composer

[stesi561]

Any suggestions on further debugging that I can do to investigate this further?

[jitendrapurohit]

looks like $user variable is not defined in this function?

[mlutfy]

👍 yikes, indeed. Although it produced a notice, but it was still giving the correct behaviour because getBestUFID accepts null.

I fixed the code nonetheless and tested to make sure that fixing it did not introduce a regression.

[mlutfy]

I tried re-testing this issue after: civicrm/civicrm-core#16177 - thinking it would fix the issue, but I'm still encountering it.

[GValFr35]

I tested civicrm/civicrm-core#16177 in combination with this change :

function civicrm_user_login(AccountInterface $account) {
  $civicrm = \Drupal::service('civicrm');
  $civicrm->initialize();

  if (\Drupal::hasService('masquerade')) {
    // In theory this is harmless whether we masquerade or not, but for now narrowing
    // the scope, in case it causes regressions.
    // Also, there is a isMasquerading() function, but it does not seem to be
    // initialized at this point (it always returns false).
    \CRM_Core_Session::singleton()->reset(FALSE);
  }
  $civicrm->synchronizeUser($account);
}

and that seems to fix the issue on my side without regression (ie dashboard ...).

[GValFr35]

Although I prefer to patch the CRM_Core_Session::reset() function as follows:

  public function reset($all = 1) {
    $this->initialize();
    // to make certain we clear it, first initialize it to empty
    $this->_session[$this->_key] = [];
    unset($this->_session[$this->_key]);

    if ($all) {
      unset($this->_session);
    }
  }

But yes, it must be fully tested with other CMS...

[mlutfy]

@GValFr35 Thanks for your input. I updated my PR using your code. Cleaner and works well.

Since this has been open for a while, and I'm not the only one running into this problem, I would be tempted to merge if @GValFr35 validates the latest edit.

(I only patches in the civicrm_user_login, because I would very much prefer to stay away from doing changes to the core session code.)

[albionbrown]

I can also say that @GValFr35's patch also works for me too, using Drupal 8.9.x and CiviCRM 5.39.0. Is there any news on when this could get merged?

[demeritcowboy]

Since two people have confirmed recently it works for masquerade, I think I'm ok to merge this since the first thing synchronizeUser does is call initialize, so there should be no change if masquerade is not installed. And just doing a quick test like that nothing obvious comes up.

[jptillman]

I've been using the last patch via composer in production since it was posted. We rely on it.