Problem/Motivation
The saveDestination method of the OpenIDConnectSession class trusts that the get method of the redirect.destination service is returning the destination query parameter. But if there is no query parameter, it returns the current destination url instead.
I think we should use the redirect_login configuration if there is no query parameter, and override it only if this query parameter is present.
Steps to reproduce
Set a login redirection on the configuration form /admin/config/people/openid-connect/settings, login without the destination query parameter, and you will not be redirected to the provided login redirection.
Proposed resolution
Check the destination query parameter from the current request and use the redirect_login service only if the the parameter is present. Otherwise, use the login_redirection config. If no query destination nor login_redirection config is present, fallback to the user profile.
Remaining tasks
None.
User interface changes
None.
API changes
Change the saveDestination method's logic to fit the proposed resolution.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | interdiff_2-4.txt | 24 KB | sahil.goyal |
| #4 | 3375888-4.patch | 12.77 KB | sahil.goyal |
| #4 | 3375888-before.png | 371.94 KB | sahil.goyal |
| check-destination-query-parameter-before-using-the-destination-service.patch | 12.38 KB | chinaskino |
Comments
Comment #2
chinaskino commentedComment #3
henry tran commentedHi @Chinaskino
I applied your patch and navigated to /admin/config/people/openid-connect/settings
There is an error:
Can you please help?
I'm using:
Drupal core: 10.1
openid_connect: 3.0@alpha
Thank you.
The website encountered an unexpected error. Please try again later.
ArgumentCountError: Too few arguments to function Drupal\openid_connect\OpenIDConnectSession::__construct(), 4 passed in /app/docroot/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 and exactly 5 expected in Drupal\openid_connect\OpenIDConnectSession->__construct() (line 67 of modules/contrib/openid_connect/src/OpenIDConnectSession.php).
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 440)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters() (Line: 237)
Drupal\Component\DependencyInjection\Container->createService() (Line: 177)
Drupal\Component\DependencyInjection\Container->get() (Line: 79)
Drupal\openid_connect\Form\OpenIDConnectSettingsForm::create() (Line: 28)
Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (Line: 48)
Drupal\Core\Controller\HtmlFormController->getFormObject() (Line: 58)
Drupal\Core\Controller\FormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 583)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 166)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 58)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 270)
Drupal\shield\ShieldMiddleware->bypass() (Line: 137)
Drupal\shield\ShieldMiddleware->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 704)
Drupal\Core\DrupalKernel->handle() (Line: 19)
Comment #4
sahil.goyal commentedI have addressed the concern in #3 and updated the patch provided by @Chinaskino.