Now that #2798323: Make provider prefix to username optional is in we don't need ExternalAuthSubscriber anymore.

CommentFileSizeAuthor
#8 3083149-8.patch2.51 KBbkosborne
#4 3083149-4.interdiff.txt318 bytesclaudiu.cristea
#4 3083149-4.patch2.29 KBclaudiu.cristea
#3 3083149-3.patch2.2 KBclaudiu.cristea

Comments

claudiu.cristea created an issue. See original summary.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

👍 indeed. We should be able to drop it and indicate that we require at least version 1.2 of external auth

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review
StatusFileSize
new 3083149-3.patch2.2 KB

Here we go.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 3083149-4.patch2.29 KB
new 3083149-4.interdiff.txt318 bytes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Postponed

Hm... I just found that we cannot remove this subscriber yet as "provider prefix" is used also here \Drupal\externalauth\ExternalAuth::linkExistingAccount():

  public function linkExistingAccount($authname, $provider, UserInterface $account) {
    // If a mapping (for the same provider) to this account already exists, we
    // silently skip saving this auth mapping.
    if (!$this->authmap->get($account->id(), $provider)) {
      $username = $provider . '_' . $authname;
      $authmap_event = $this->eventDispatcher->dispatch(ExternalAuthEvents::AUTHMAP_ALTER, new ExternalAuthAuthmapAlterEvent($provider, $authname, $username, NULL));
      $this->authmap->save($account, $provider, $authmap_event->getAuthname(), $authmap_event->getData());
    }
  }

We need to open a ticket upstream and fix that method too. But this time looks harder :(

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Hmm, while you're correct that it's also used there, I think it's not as hard to fix as you think.

The username doesn't really need to be dealt with at all when linking an existing account, because the username is not used. It's just created so that it can be passed to the AUTHMAP_ALTER event which requires a username.

I think that we should petition to have the linkExistingAccount method changed to this:

  /**
   * {@inheritdoc}
   */
  public function linkExistingAccount($authname, $provider, UserInterface $account) {
    // If a mapping (for the same provider) to this account already exists, we
    // silently skip saving this auth mapping.
    if (!$this->authmap->get($account->id(), $provider)) {
      $authmap_event = $this->eventDispatcher->dispatch(ExternalAuthEvents::AUTHMAP_ALTER, new ExternalAuthAuthmapAlterEvent($provider, $authname, $account->getAccountName(), NULL));
      $this->authmap->save($account, $provider, $authmap_event->getAuthname(), $authmap_event->getData());
    }
  }
bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Related issues: +#3104478: Clean up linkExistingAccount method so it doesn't needless prefix username with provider name
bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Postponed » Needs review
StatusFileSize
new 3083149-8.patch2.51 KB

This adds the dependency to the info file.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Needs review » Fixed

  • bkosborne committed 2d0dc1c on 8.x-1.x 
    Issue #3083149 by claudiu.cristea, bkosborne: Drop...

Status: Fixed » Closed (fixed)

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