Commit 4d846956 authored by Tyler Struyk's avatar Tyler Struyk
Browse files

ISTWCMS-2267 & ISTWCMS-2268: Added optional prefix to externalauth and rewrite...

ISTWCMS-2267 & ISTWCMS-2268: Added optional prefix to externalauth and rewrite Samlauth to auto-assign on registeration
parent d5215be8
......@@ -22,7 +22,7 @@
"behat/mink": "1.7.x-dev",
"behat/mink-goutte-driver": "~1.2",
"composer/installers": "^1.2",
"cweagans/composer-patches": "^1.6",
"cweagans/composer-patches": "dev-master",
"drupal/allowed_formats": "1.1",
"drupal/autocomplete_deluxe": "dev-8.x-1.x",
"drupal/cancel_button": "1.0",
......@@ -95,8 +95,11 @@
"modules/contrib/{$name}": ["type:drupal-module"]
},
"patches": {
"drupal/samlauth": {
"ISTWCMS-2268: Rewrite the samlauth to allow for autoassigning roles": "patches/modules/samlauth/remove_presave_hook-samlauth-2925171-1.patch"
},
"drupal/externalauth": {
"ISTWCMS-2267: Remove provider prefix to username (saml_)": "patches/modules/externalauth/2798323-externalauth-11.patch"
"ISTWCMS-2267: Remove provider prefix to username (samlauth_)": "patches/modules/externalauth/optional_prefix-externalauth-2798323-14.patch"
}
}
}
......
diff --git a/src/ExternalAuth.php b/src/ExternalAuth.php
index 2811d48..b49e1b3 100644
--- a/src/ExternalAuth.php
+++ b/src/ExternalAuth.php
@@ -91,7 +91,8 @@ public function login($authname, $provider) {
* {@inheritdoc}
*/
public function register($authname, $provider, $account_data = array(), $authmap_data = NULL) {
- $username = $provider . '_' . $authname;
+ $username = $authname;
+
$authmap_event = $this->eventDispatcher->dispatch(ExternalAuthEvents::AUTHMAP_ALTER, new ExternalAuthAuthmapAlterEvent($provider, $authname, $username, $authmap_data));
$entity_storage = $this->entityManager->getStorage('user');
@@ -104,7 +105,6 @@ public function register($authname, $provider, $account_data = array(), $authmap
$account_data = array_merge(
[
'name' => $authmap_event->getUsername(),
- 'init' => $provider . '_' . $authmap_event->getAuthname(),
'status' => 1,
'access' => (int) $_SERVER['REQUEST_TIME'],
],
@@ -158,7 +158,7 @@ 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;
+ $username = $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());
}
diff --git a/config/install/externalauth.settings.yml b/config/install/externalauth.settings.yml
new file mode 100644
index 0000000..67b1129
--- /dev/null
+++ b/config/install/externalauth.settings.yml
@@ -0,0 +1 @@
+prefix: 1
diff --git a/externalauth.info.yml b/externalauth.info.yml
index 4eb4330..ebb3fdc 100644
--- a/externalauth.info.yml
+++ b/externalauth.info.yml
@@ -3,3 +3,4 @@ type: module
description: Helper module to authenticate users using an external site / service and storing identification details
core: 8.x
package: Other
+configuration: externalauth.settings
diff --git a/externalauth.install b/externalauth.install
index c02e013..47d5e30 100644
--- a/externalauth.install
+++ b/externalauth.install
@@ -83,3 +83,13 @@ function externalauth_update_8102() {
$schema->addUniqueKey('authmap', 'authname_provider', array('authname', 'provider'));
$schema->dropIndex('authmap', 'auth_provider');
}
+
+/**
+ * Turn on prefix option by default for backward compatiablity.
+ */
+function externalauth_update_8103() {
+ $config = \Drupal::getContainer()->get('config.factory')
+ ->getEditable('externalauth.settings');
+ $config->set('prefix', 1);
+ $config->save();
+}
diff --git a/externalauth.routing.yml b/externalauth.routing.yml
new file mode 100644
index 0000000..7db1024
--- /dev/null
+++ b/externalauth.routing.yml
@@ -0,0 +1,7 @@
+externalauth.settings:
+ path: '/admin/config/people/externalauth'
+ defaults:
+ _form: '\Drupal\externalauth\Form\ExternalAuthSettingsForm'
+ _title: 'External Auth configuration screen'
+ requirements:
+ _permission: 'administer site configuration'
diff --git a/src/ExternalAuth.php b/src/ExternalAuth.php
index 2811d48..54e56fd 100644
--- a/src/ExternalAuth.php
+++ b/src/ExternalAuth.php
@@ -91,7 +91,9 @@ class ExternalAuth implements ExternalAuthInterface {
* {@inheritdoc}
*/
public function register($authname, $provider, $account_data = array(), $authmap_data = NULL) {
- $username = $provider . '_' . $authname;
+ // Check if should prefix the provider to the username.
+ $prefix = \Drupal::config('externalauth.settings')->get('prefix') ? $provider . '_' : '';
+ $username = $prefix . $authname;
$authmap_event = $this->eventDispatcher->dispatch(ExternalAuthEvents::AUTHMAP_ALTER, new ExternalAuthAuthmapAlterEvent($provider, $authname, $username, $authmap_data));
$entity_storage = $this->entityManager->getStorage('user');
@@ -104,7 +106,7 @@ class ExternalAuth implements ExternalAuthInterface {
$account_data = array_merge(
[
'name' => $authmap_event->getUsername(),
- 'init' => $provider . '_' . $authmap_event->getAuthname(),
+ 'init' => $prefix . $authmap_event->getAuthname(),
'status' => 1,
'access' => (int) $_SERVER['REQUEST_TIME'],
],
@@ -155,10 +157,13 @@ class ExternalAuth implements ExternalAuthInterface {
* {@inheritdoc}
*/
public function linkExistingAccount($authname, $provider, UserInterface $account) {
+ // Check if should prefix the provider to the username.
+ $prefix = \Drupal::config('externalauth.settings')->get('prefix') ? $provider . '_' : '';
+
// 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;
+ $username = $prefix . $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());
}
diff --git a/src/Form/ExternalAuthSettingsForm.php b/src/Form/ExternalAuthSettingsForm.php
new file mode 100644
index 0000000..73b768c
--- /dev/null
+++ b/src/Form/ExternalAuthSettingsForm.php
@@ -0,0 +1,62 @@
+<?php
+
+namespace Drupal\externalauth\Form;
+
+use Drupal\Core\Form\ConfigFormBase;
+use Drupal\Core\Form\FormStateInterface;
+
+/**
+ * Provides a configuration form for externalauth module.
+ */
+class ExternalAuthSettingsForm extends ConfigFormBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getFormId() {
+ return 'externalauth_settings_form';
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function getEditableConfigNames() {
+ return [
+ 'externalauth.settings',
+ ];
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function buildForm(array $form, FormStateInterface $form_state) {
+ $config = $this->config('externalauth.settings');
+
+ $form['externalauth'] = [
+ '#type' => 'fieldset',
+ '#title' => $this->t('Settings'),
+ ];
+
+ $form['externalauth']['prefix'] = [
+ '#type' => 'checkbox',
+ '#title' => $this->t('Prefix user name with their provider'),
+ '#description' => $this->t('If this option is enabled, usernames
+ will be prefix with their provider (i.e. samlauth_username). NOTE:
+ by changing this value, you may have to manual update all existing
+ users.'),
+ '#default_value' => $config->get('prefix'),
+ ];
+ return parent::buildForm($form, $form_state);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function submitForm(array &$form, FormStateInterface $form_state) {
+ parent::submitForm($form, $form_state);
+ $this->config('externalauth.settings')
+ ->set('prefix', $form_state->getValue('prefix'))
+ ->save();
+ }
+
+}
diff --git a/samlauth.module b/samlauth.module
index a7c5857..6ad0c78 100644
--- a/samlauth.module
+++ b/samlauth.module
@@ -58,22 +58,3 @@ function samlauth_check_saml_user($form, FormStateInterface $form_state) {
}
}
-/**
- * Implements hook_user_presave().
- */
-function samlauth_user_presave(UserInterface $account) {
- // Hook into the user creation process from ExternalAuth::register() so that
- // we don't need to save the new user a second time to add our SAML attribute
- // values into the new user object. The way externalauth prefixes account
- // names acts as a recursion stop, in case any called code (e.g. event) saves
- // the account.
- if ($account->isNew() && strpos($account->getAccountName(), 'samlauth_') === 0) {
- // Doublecheck that we're actually processing an ACS request, which we can
- // do by checking the request for presence of a user name attribute.
- /** @var \Drupal\samlauth\SamlService $saml_service */
- $saml_service = \Drupal::service('samlauth.saml');
- if ($saml_service->getAttributeByConfig('user_name_attribute')) {
- $saml_service->synchronizeUserAttributes($account, TRUE);
- }
- }
-}
diff --git a/samlauth.services.yml b/samlauth.services.yml
index d9f24ae..d5d84bf 100644
--- a/samlauth.services.yml
+++ b/samlauth.services.yml
@@ -5,8 +5,13 @@ services:
samlauth.saml:
class: Drupal\samlauth\SamlService
arguments: ['@externalauth.externalauth', '@config.factory', '@entity_type.manager', '@logger.channel.samlauth', '@event_dispatcher', '@user.private_tempstore']
+ samlauth.event_subscriber.user_register:
+ class: Drupal\samlauth\EventSubscriber\UserEventSubscriber
+ arguments: ['@config.factory', '@entity_type.manager', '@typed_data_manager', '@email.validator', '@logger.channel.samlauth']
+ tags:
+ - { name: event_subscriber }
samlauth.event_subscriber.user_sync:
- class: Drupal\samlauth\EventSubscriber\UserSyncEventSubscriber
+ class: Drupal\samlauth\EventSubscriber\UserEventSubscriber
arguments: ['@config.factory', '@entity_type.manager', '@typed_data_manager', '@email.validator', '@logger.channel.samlauth']
tags:
- { name: event_subscriber }
diff --git a/src/Event/SamlauthEvents.php b/src/Event/SamlauthEvents.php
index 16d644a..380ebd8 100644
--- a/src/Event/SamlauthEvents.php
+++ b/src/Event/SamlauthEvents.php
@@ -5,7 +5,7 @@ namespace Drupal\samlauth\Event;
/**
* Defines events for the samlauth module.
*
- * @see \Drupal\samlauth\Event\SamlauthUserSyncEvent
+ * @see \Drupal\samlauth\Event\SamlauthUserEvent
*/
final class SamlauthEvents {
@@ -28,13 +28,33 @@ final class SamlauthEvents {
*/
const USER_LINK = 'samlauth.user_link';
+ /**
+ * Name of the event fired when a user is registered from SAML attributes.
+ *
+ * The event allows modules to synchronize user account values with SAML
+ * attributes passed by the IdP in the authentication response. Basic required
+ * properties (username, email) are already synchronized. The event listener
+ * method receives a \Drupal\samlauth\Event\SamlauthUserRegisterEvent
+ * instance. If it changes the account, it should call the event's
+ * markAccountChanged() method rather than saving the account by itself.
+ *
+ * The event is fired the user is registered using externauth module.
+ *
+ * @Event
+ *
+ * @see \Drupal\samlauth\Event\SamlauthUserRegisterEvent
+ *
+ * @var string
+ */
+ const USER_REGISTER = 'samlauth.user_register';
+
/**
* Name of the event fired when a user is synchronized from SAML attributes.
*
* The event allows modules to synchronize user account values with SAML
* attributes passed by the IdP in the authentication response. Basic required
* properties (username, email) are already synchronized. The event listener
- * method receives a \Drupal\samlauth\Event\SamlauthUserSyncEvent instance. If
+ * method receives a \Drupal\samlauth\Event\SamlauthUserEvent instance. If
* it changes the account, it should call the event's markAccountChanged()
* method rather than saving the account by itself.
*
@@ -44,10 +64,9 @@ final class SamlauthEvents {
*
* @Event
*
- * @see \Drupal\samlauth\Event\SamlauthUserSyncEvent
+ * @see \Drupal\samlauth\Event\SamlauthUserEvent
*
* @var string
*/
const USER_SYNC = 'samlauth.user_sync';
-
}
diff --git a/src/Event/SamlauthUserSyncEvent.php b/src/Event/SamlauthUserEvent.php
similarity index 98%
rename from src/Event/SamlauthUserSyncEvent.php
rename to src/Event/SamlauthUserEvent.php
index 99bae40..92f0f0d 100644
--- a/src/Event/SamlauthUserSyncEvent.php
+++ b/src/Event/SamlauthUserEvent.php
@@ -8,7 +8,7 @@ use Symfony\Component\EventDispatcher\Event;
/**
* Wraps a samlauth user sync event for event listeners.
*/
-class SamlauthUserSyncEvent extends Event {
+class SamlauthUserEvent extends Event {
/**
* The Drupal user account.
diff --git a/src/EventSubscriber/UserSyncEventSubscriber.php b/src/EventSubscriber/UserEventSubscriber.php
similarity index 58%
rename from src/EventSubscriber/UserSyncEventSubscriber.php
rename to src/EventSubscriber/UserEventSubscriber.php
index 1cc495b..6053acb 100644
--- a/src/EventSubscriber/UserSyncEventSubscriber.php
+++ b/src/EventSubscriber/UserEventSubscriber.php
@@ -6,8 +6,10 @@ use Drupal\Core\Config\ConfigFactoryInterface;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Field\BaseFieldDefinition;
use Drupal\Core\TypedData\TypedDataManagerInterface;
+use Drupal\externalauth\Event\ExternalAuthEvents;
+use Drupal\externalauth\Event\ExternalAuthRegisterEvent;
use Drupal\samlauth\Event\SamlauthEvents;
-use Drupal\samlauth\Event\SamlauthUserSyncEvent;
+use Drupal\samlauth\Event\SamlauthUserEvent;
use Egulias\EmailValidator\EmailValidator;
use Psr\Log\LoggerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -21,7 +23,7 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
* account with the same name is found, or continue with a non-renamed account?
* etc.)
*/
-class UserSyncEventSubscriber implements EventSubscriberInterface {
+class UserEventSubscriber implements EventSubscriberInterface {
/**
* The EntityTypeManager service.
@@ -84,25 +86,124 @@ class UserSyncEventSubscriber implements EventSubscriberInterface {
* {@inheritdoc}
*/
public static function getSubscribedEvents() {
+ $events[ExternalAuthEvents::REGISTER][] = ['onUserRegister'];
$events[SamlauthEvents::USER_SYNC][] = ['onUserSync'];
return $events;
}
/**
- * Performs actions to synchronize users with Factory data on login.
+ * Performs actions to new registered users with Factory data on login.
*
- * @param \Drupal\samlauth\Event\SamlauthUserSyncEvent $event
+ * @param \Drupal\externalauth\Event\ExternalAuthRegisterEvent $externalauth_event
* The event.
*/
- public function onUserSync(SamlauthUserSyncEvent $event) {
- // If the account is new, we are in the middle of a user save operation;
- // the current user name is 'samlauth_AUTHNAME' (as set by externalauth) and
+ public function onUserRegister(ExternalAuthRegisterEvent $externalauth_event) {
+
+ // Only register user if we are externalauth module is using this service.
+ if (!$externalauth_event->getProvider() == 'samlauth') {
+ return;
+ }
+
+ $saml_service = \Drupal::service('samlauth.saml');
+ $attributes = $saml_service->getAttributes();
+ $event = new SamlauthUserEvent($externalauth_event->getAccount(), $attributes);
+
+ // The current user name is 'samlauth_AUTHNAME' (as set by externalauth) and
// e-mail is not set yet.
$account = $event->getAccount();
$fatal_errors = [];
+ // Register username.
+ // Get value from the SAML attribute whose name is configured in the
+ // samlauth module.
+ $name = $this->getAttributeByConfig('user_name_attribute', $event);
+ if ($name && $name != $account->getAccountName()) {
+ // Validate the username. This shouldn't be necessary to mitigate
+ // attacks; assuming our SAML setup is correct, noone can insert fake
+ // data here. It protects against SAML attribute misconfigurations.
+ // Invalid names will cancel the login / account creation. The code is
+ // copied from user_validate_name().
+ $definition = BaseFieldDefinition::create('string')->addConstraint('UserName', []);
+ $data = \Drupal::typedDataManager()->create($definition);
+ $data->setValue($name);
+ $violations = $data->validate();
+ if ($violations) {
+ foreach ($violations as $violation) {
+ $fatal_errors[] = $violation->getMessage();
+ }
+ }
+
+ // Check if the username is not already taken by someone else. For new
+ // accounts this can happen if the 'map existing users' setting is off.
+ if (!$fatal_errors) {
+ $account_search = $this->entityTypeManager->getStorage('user')->loadByProperties(array('name' => $name));
+ $existing_account = reset($account_search);
+ if (!$existing_account || $account->id() == $existing_account->id()) {
+ $account->setUsername($name);
+ $event->markAccountChanged();
+ }
+ else {
+ $error = 'An account with the username @username already exists.';
+ if ($account->isNew()) {
+ $fatal_errors[] = t($error, ['@username' => $name]);
+ }
+ else {
+ // We continue and keep the old name. A DSM should be OK here
+ // since login only happens interactively. (And we're ignoring
+ // the law of dependency injection for this.)
+ $error = "Error updating user name from SAML attribute: $error";
+ $this->logger->error($error, ['@username' => $name]);
+ drupal_set_message(t($error, ['@username' => $name]), 'error');
+ }
+ }
+ }
+ }
+
+ // Synchronize e-mail.
+ $mail = $this->getAttributeByConfig('user_mail_attribute', $event);
+ if ($mail) {
+ if ($mail != $account->getEmail()) {
+ // Invalid e-mail cancels the login / account creation just like name.
+ if ($this->emailValidator->isValid($mail)) {
+ $account->setEmail($mail);
+ // externalauth sets init to a non e-mail value so we will fix it.
+ $account->set('init', $mail);
+ $event->markAccountChanged();
+ }
+ else {
+ $fatal_errors[] = t('Invalid e-mail address @mail', ['@mail' => $mail]);
+ }
+ }
+ }
+ else {
+ // We won't allow new accounts with empty e-mail.
+ $fatal_errors[] = t('Email address is not provided in SAML attribute.');
+ }
+
+ if ($fatal_errors) {
+ // Cancel the whole login process and/or account creation.
+ throw new \RuntimeException('Error(s) encountered during SAML attribute synchronization: ' . join(' // ', $fatal_errors));
+ } else {
+ // Allow other module to module user after username and email are set.
+ $dispatcher = \Drupal::service('event_dispatcher');
+ $dispatcher->dispatch(SamlauthEvents::USER_REGISTER, $event);
+ // Save the account information.
+ $account->save();
+ }
+ }
+
+ /**
+ * Performs actions to synchronize users with Factory data on login.
+ *
+ * @param \Drupal\samlauth\Event\SamlauthUserEvent $event
+ * The event.
+ */
+ public function onUserSync(SamlauthUserEvent $event) {
+ $account = $event->getAccount();
+ $fatal_errors = [];
+
// Synchronize username.
- if ($account->isNew() || $this->config->get('sync_name')) {
+ if ($this->config->get('sync_name')) {
// Get value from the SAML attribute whose name is configured in the
// samlauth module.
$name = $this->getAttributeByConfig('user_name_attribute', $event);
@@ -150,18 +251,13 @@ class UserSyncEventSubscriber implements EventSubscriberInterface {
}
// Synchronize e-mail.
- if ($account->isNew() || $this->config->get('sync_mail')) {
+ if ($this->config->get('sync_mail')) {
$mail = $this->getAttributeByConfig('user_mail_attribute', $event);
if ($mail) {
if ($mail != $account->getEmail()) {
// Invalid e-mail cancels the login / account creation just like name.
if ($this->emailValidator->isValid($mail)) {
-
$account->setEmail($mail);
- if ($account->isNew()) {
- // externalauth sets init to a non e-mail value so we will fix it.
- $account->set('init', $mail);
- }
$event->markAccountChanged();
}
else {
@@ -169,10 +265,6 @@ class UserSyncEventSubscriber implements EventSubscriberInterface {
}
}
}
- elseif ($account->isNew()) {
- // We won't allow new accounts with empty e-mail.
- $fatal_errors[] = t('Email address is not provided in SAML attribute.');
- }
}
if ($fatal_errors) {
@@ -189,13 +281,13 @@ class UserSyncEventSubscriber implements EventSubscriberInterface {
* @param string $config_key
* A key in the module's configuration, containing the name of a SAML
* attribute.
- * @param \Drupal\samlauth\Event\SamlauthUserSyncEvent $event
+ * @param \Drupal\samlauth\Event\SamlauthUserEvent $event
* The event, which holds the attributes from the SAML response.
*
* @return mixed|null
* The SAML attribute value; NULL if the attribute value was not found.
*/
- public function getAttributeByConfig($config_key, SamlauthUserSyncEvent $event) {
+ public function getAttributeByConfig($config_key, SamlauthUserEvent $event) {
$attributes = $event->getAttributes();
$attribute_name = $this->config->get($config_key);
return $attribute_name && !empty($attributes[$attribute_name][0]) ? $attributes[$attribute_name][0] : NULL;
diff --git a/src/SamlService.php b/src/SamlService.php
index 666526c..0b07f1b 100644
--- a/src/SamlService.php
+++ b/src/SamlService.php
@@ -9,7 +9,7 @@ use Drupal\Core\Url;
use Drupal\externalauth\ExternalAuth;
use Drupal\samlauth\Event\SamlauthEvents;
use Drupal\samlauth\Event\SamlauthUserLinkEvent;
-use Drupal\samlauth\Event\SamlauthUserSyncEvent;
+use Drupal\samlauth\Event\SamlauthUserEvent;
use Drupal\user\PrivateTempStoreFactory;
use Drupal\user\UserInterface;
use Exception;
@@ -321,7 +321,7 @@ class SamlService {
*/
public function synchronizeUserAttributes(UserInterface $account, $skip_save = FALSE) {
// Dispatch a user_sync event.
- $event = new SamlauthUserSyncEvent($account, $this->getAttributes());
+ $event = new SamlauthUserEvent($account, $this->getAttributes());
$this->eventDispatcher->dispatch(SamlauthEvents::USER_SYNC, $event);
if (!$skip_save && $event->isAccountChanged()) {
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment