diff --git a/config/install/openid_connect.settings.yml b/config/install/openid_connect.settings.yml index 5217e2b..2c9d885 100644 --- a/config/install/openid_connect.settings.yml +++ b/config/install/openid_connect.settings.yml @@ -2,5 +2,6 @@ always_save_userinfo: true connect_existing_users: false override_registration_settings: false user_login_display: 'hidden' +redirect_login: 'user' userinfo_mappings: timezone: zoneinfo diff --git a/config/schema/openid_connect.schema.yml b/config/schema/openid_connect.schema.yml index 2040c3d..3e4aa75 100644 --- a/config/schema/openid_connect.schema.yml +++ b/config/schema/openid_connect.schema.yml @@ -14,6 +14,9 @@ openid_connect.settings: user_login_display: type: string label: 'Show external providers in user login form' + redirect_login: + type: string + label: 'Redirect on login' userinfo_mappings: type: sequence label: 'User claims mapping' diff --git a/openid_connect.install b/openid_connect.install index fb5dee9..e2d3619 100644 --- a/openid_connect.install +++ b/openid_connect.install @@ -188,3 +188,14 @@ function openid_connect_update_8201() { $client->save(); } } + +/** + * Add redirect_login to module settings. + */ +function openid_connect_update_8202() { + $config_factory = \Drupal::configFactory(); + + $config = $config_factory->getEditable('openid_connect.settings'); + $config->set('redirect_login', 'user'); + $config->save(TRUE); +} diff --git a/openid_connect.services.yml b/openid_connect.services.yml index e115054..e9322f9 100644 --- a/openid_connect.services.yml +++ b/openid_connect.services.yml @@ -21,7 +21,7 @@ services: openid_connect.session: class: Drupal\openid_connect\OpenIDConnectSession - arguments: ['@path.current', '@request_stack'] + arguments: ['@config.factory', '@redirect.destination'] openid_connect.autodiscover: class: Drupal\openid_connect\OpenIDConnectAutoDiscover diff --git a/src/Controller/OpenIDConnectRedirectController.php b/src/Controller/OpenIDConnectRedirectController.php index dc3d513..3a86d29 100644 --- a/src/Controller/OpenIDConnectRedirectController.php +++ b/src/Controller/OpenIDConnectRedirectController.php @@ -210,14 +210,11 @@ class OpenIDConnectRedirectController extends ControllerBase implements AccessIn } } - // It's possible to set 'options' in the redirect destination. - if (is_array($destination)) { - $query = !empty($destination[1]['query']) ? '?' . $destination[1]['query'] : ''; - $redirect = Url::fromUri('internal:/' . ltrim($destination[0], '/') . $query)->toString(); - } - else { - $redirect = Url::fromUri('internal:/' . ltrim($destination, '/'))->toString(); - } + // The destination parameter should be a prepared uri and include any query + // parameters or fragments already. + // + // @see \Drupal\openid_connect\OpenIDConnectSession::saveDestination() + $redirect = Url::fromUri('internal:/' . ltrim($destination, '/'))->toString(); return new RedirectResponse($redirect); } diff --git a/src/Form/OpenIDConnectSettingsForm.php b/src/Form/OpenIDConnectSettingsForm.php index 23ac205..36224ad 100644 --- a/src/Form/OpenIDConnectSettingsForm.php +++ b/src/Form/OpenIDConnectSettingsForm.php @@ -134,6 +134,18 @@ class OpenIDConnectSettingsForm extends ConfigFormBase implements ContainerInjec '#default_value' => $settings->get('user_login_display'), ]; + $form['redirects'] = [ + '#title' => $this->t('Redirects'), + '#type' => 'fieldset', + ]; + + $form['redirects']['redirect_login'] = [ + '#title' => $this->t('Login'), + '#type' => 'textfield', + '#description' => $this->t('Path to redirect to on client login'), + '#default_value' => $settings->get('redirect_login'), + ]; + $form['userinfo_mappings'] = [ '#title' => $this->t('User claims mapping'), '#type' => 'fieldset', @@ -178,6 +190,7 @@ class OpenIDConnectSettingsForm extends ConfigFormBase implements ContainerInjec ->set('connect_existing_users', $form_state->getValue('connect_existing_users')) ->set('override_registration_settings', $form_state->getValue('override_registration_settings')) ->set('user_login_display', $form_state->getValue('user_login_display')) + ->set('redirect_login', $form_state->getValue('redirect_login')) ->set('userinfo_mappings', array_filter($form_state->getValue('userinfo_mappings'))) ->save(); } diff --git a/src/OpenIDConnectSession.php b/src/OpenIDConnectSession.php index b177472..7aad880 100644 --- a/src/OpenIDConnectSession.php +++ b/src/OpenIDConnectSession.php @@ -2,8 +2,8 @@ namespace Drupal\openid_connect; -use Drupal\Core\Path\CurrentPathStack; -use Symfony\Component\HttpFoundation\RequestStack; +use Drupal\Core\Config\ConfigFactory; +use Drupal\Core\Routing\RedirectDestination; /** * Session service of the OpenID Connect module. @@ -11,30 +11,30 @@ use Symfony\Component\HttpFoundation\RequestStack; class OpenIDConnectSession { /** - * The current path. + * Drupal\Core\Config\ConfigFactory definition. * - * @var \Drupal\Core\Path\CurrentPathStack + * @var \Drupal\Core\Config\ConfigFactory */ - protected $currentPath; + protected $configFactory; /** - * The request stack. + * The redirect destination service. * - * @var \Symfony\Component\HttpFoundation\RequestStack + * @var \Drupal\Core\Routing\RedirectDestination */ - protected $requestStack; + protected $redirectDestination; /** * Construct an instance of the OpenID Connect session service. * - * @param \Drupal\Core\Path\CurrentPathStack $current_path - * The current path. - * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack - * The request stack. + * @param \Drupal\Core\Config\ConfigFactory $config_factory + * The configuration factory. + * @param \Drupal\Core\Routing\RedirectDestination $redirect_destination + * The redirect destination service. */ - public function __construct(CurrentPathStack $current_path, RequestStack $request_stack) { - $this->currentPath = $current_path; - $this->requestStack = $request_stack; + public function __construct(ConfigFactory $config_factory, RedirectDestination $redirect_destination) { + $this->configFactory = $config_factory; + $this->redirectDestination = $redirect_destination; } /** @@ -43,21 +43,23 @@ class OpenIDConnectSession { * @todo Evaluate, whether we can now use the user.private_tempstore instead * of the global $_SESSION variable, as https://www.drupal.org/node/2743931 * has been applied to 8.5+ core. + * + * @see \Drupal\openid_connect\Controller\OpenIDConnectRedirectController::authenticate() */ public function saveDestination() { - $current_path = $this->currentPath->getPath(); - $path = ($current_path == '/user/login') ? '/user' : $current_path; - - // The destination could contain query parameters. Ensure that they are - // preserved. - $query = $this->requestStack->getCurrentRequest()->getQueryString(); - - $_SESSION['openid_connect_destination'] = [ - $path, - [ - 'query' => $query, - ], - ]; + // If the current request includes a 'destination' query parameter we'll use + // that in the redirection. Otherwise use the current request path and + // query. + $destination = ltrim($this->redirectDestination->get(), '/'); + + // Don't redirect to user/login. In this case redirect to the user profile. + if (strpos($destination, 'user/login') === 0) { + $config_factory = \Drupal::configFactory(); + $redirect_login = $config_factory->get('openid_connect.settings')->get('redirect_login'); + $destination = $redirect_login ?: 'user'; + } + + $_SESSION['openid_connect_destination'] = $destination; } } diff --git a/tests/src/Unit/OpenIdConnectSessionTest.php b/tests/src/Unit/OpenIdConnectSessionTest.php index 7b5d9b5..4505cd0 100644 --- a/tests/src/Unit/OpenIdConnectSessionTest.php +++ b/tests/src/Unit/OpenIdConnectSessionTest.php @@ -4,11 +4,9 @@ declare(strict_types = 1); namespace Drupal\Tests\openid_connect\Unit; -use Drupal\Core\Path\CurrentPathStack; +use Drupal\Core\Routing\RedirectDestination; use Drupal\openid_connect\OpenIDConnectSession; use Drupal\Tests\UnitTestCase; -use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\RequestStack; /** * @coversDefaultClass \Drupal\openid_connect\OpenIDConnectSession @@ -32,18 +30,11 @@ class OpenIdConnectSessionTest extends UnitTestCase { const TEST_QUERY = 'sport=baseball&team=reds'; /** - * A mock of the current_path service. + * A mock of the redirect.destination service. * * @var \PHPUnit\Framework\MockObject\MockObject */ - protected $currentPath; - - /** - * A mock of the requestStack method for testing. - * - * @var \PHPUnit\Framework\MockObject\MockObject - */ - protected $requestStack; + protected $redirectDestination; /** * {@inheritdoc} @@ -51,20 +42,8 @@ class OpenIdConnectSessionTest extends UnitTestCase { protected function setUp(): void { parent::setUp(); - // Mock the currentPath service. - $this->currentPath = $this->createMock(CurrentPathStack::class); - - // Mock the Request class that is returned by RequestStack class. - $request = $this->createMock(Request::class); - $request->expects($this->once()) - ->method('getQueryString') - ->willReturn('sport=baseball&team=reds'); - - // Mock the requestStack service. - $this->requestStack = $this->createMock(RequestStack::class); - $this->requestStack->expects($this->once()) - ->method('getCurrentRequest') - ->willReturn($request); + // Mock the 'redirect.destination' service. + $this->redirectDestination = $this->createMock(RedirectDestination::class); } /** @@ -77,13 +56,14 @@ class OpenIdConnectSessionTest extends UnitTestCase { self::TEST_QUERY ); - // Mock the getPath method for the current path service. - $this->currentPath->expects($this->once()) - ->method('getPath') - ->willReturn(self::TEST_PATH); + $destination = self::TEST_PATH . '?' . self::TEST_QUERY; + // Mock the get method for the 'redirect.destination' service. + $this->redirectDestination->expects($this->once()) + ->method('get') + ->willReturn($destination); // Create a new OpenIDConnectSession class. - $session = new OpenIDConnectSession($this->currentPath, $this->requestStack); + $session = new OpenIDConnectSession($this->redirectDestination); // Call the saveDestination() method. $session->saveDestination(); @@ -98,17 +78,16 @@ class OpenIdConnectSessionTest extends UnitTestCase { public function testSaveDestinationUserPath(): void { // Setup our expected results. $expectedSession = $this->getExpectedSessionArray( - '/user', - self::TEST_QUERY + 'user' ); - // Mock the getPath method with the user path. - $this->currentPath->expects($this->once()) - ->method('getPath') + // Mock the get method with the user login path. + $this->redirectDestination->expects($this->once()) + ->method('get') ->willReturn(self::TEST_USER_PATH); // Create a class to test with. - $session = new OpenIDConnectSession($this->currentPath, $this->requestStack); + $session = new OpenIDConnectSession($this->redirectDestination); // Call the saveDestination method. $session->saveDestination(); @@ -128,14 +107,15 @@ class OpenIdConnectSessionTest extends UnitTestCase { * @return array * The expected session array. */ - private function getExpectedSessionArray(string $path, string $queryString): array { + private function getExpectedSessionArray(string $path, string $queryString = ''): array { + + $destination = $path; + if ($queryString) { + $destination .= '?' . $queryString; + } + return [ - 'openid_connect_destination' => [ - $path, - [ - 'query' => $queryString, - ], - ], + 'openid_connect_destination' => ltrim($destination, '/'), ]; }