Redirect after login not correct when using base_path

I wonder if this is related to another issue I'm seeing in multilingual sites, where there is a URL prefix being used for all languages (i.e. / is never a resource path, but /en or /fr is)?

This also sounded to me like it was related to that 'multilingual' issue. So I hope it has been solved in the 3.x release, by #2848809: Login/logout fails with non-default language being active with the URL prefix negotiation.

If not, feel free to reopen.

I'm re-opening as it's still an issue, even for non base_path sites it ends up with 2 slashes after login.

samlauth/src/Controller/SamlController.php:342
SamlController::getUrlFromDestination

We workaround it currently by embedding a destination parameter and strip base_path and leading slash as this is what SAMLauth expects and appends base_url in our login link:

    // Strip base_path and leading slash as this is what SAMLauth expects
    // and appends base_url.
    // @see samlauth/src/Controller/SamlController.php:342
    $destination = \Drupal::destination()->get();
    $destination = preg_replace('#^' . $GLOBALS['base_path'] . '#', '', $destination);

    $form['actions']['submit'] = [
      '#type' => 'link',
      '#title' => $this->t('SAML Login'),
      '#url' => Url::fromRoute('samlauth.saml_controller_login'),
      '#options' => [
        'query' => ['destination' => $destination],
      ],
    ];

I just realized this was fixed on the 2.x branch but it is broken on the 3.x branch.

https://git.drupalcode.org/issue/samlauth-2939159/-/blob/2939159-redirec...

#2670118: Make getPostLoginDestination / getPostLogoutDestination configurable
#3041903: Issues with destination parameter

Attached is a debug step through of the proposed change in action.

We just recently started encountering the double leading slash issue that @joelpittet mentions above. We use SAMLAuth along with the https://www.drupal.org/project/require_login module to force all visitors to login to our Intranet.

My MR above had a side-effect, apparently there are cases where the URL is missing the first slash resulting in:

CRITICAL

Message:   
InvalidArgumentException encountered while initiating SAML login: The user-entered string 'home' must begin with a '/', '?', or '#'. in Drupal\Core\Url::fromUserInput() (line 216 of core/lib/Drupal/Core/Url.php).
Time:   
2023-10-26T12:31:32.458774-07:00
Channel:   
samlauth
Context:   
exception:   
{
    "class": "InvalidArgumentException",
    "message": "The user-entered string 'home' must begin with a '/', '?', or '#'.",
    "code": 0,
    "file": "core/lib/Drupal/Core/Url.php:216",
    "trace": [...

I'll update the code to mitigate against that possibility (though it might be on our end).

I added the ltrim() that core uses in various parts

See prior art:

core/lib/Drupal/Core/Form/ConfirmFormHelper.php
core/modules/migrate/src/Plugin/migrate/process/MenuLinkParent.php
core/modules/views/src/Plugin/views/display/DisplayPluginBase.php
core/modules/views/src/Plugin/views/field/FieldPluginBase.php

Needs something else... tbd

Any ideas on how to move this forward? It's more of a nuisance, but is still there, lingering...

Sorry @timwood, I haven't got a chance to dig into what went wrong with my last MR.

Adding another related issue #2418219: Deprecate destination URLs that don't include the base path as I dig into this further. Specifically #2418219-22: Deprecate destination URLs that don't include the base path from @claudiu.cristea

Or better keep the current behavior: if the destination starts with a slash, it has the base path prefix. No slash, it's the internal path or alias.

@timwood This latest solution seems to check all the boxes. I check if there is a base_path and strip it out only when the destination URL is starts with a starting / (which might be always), then the full URL built has a base_path included (can't seem to get around this without manually building it)

Applied patch and ran login / logout tests with many different urls and the patch seems to be working correctly without adding any errors or issues.

Looks good to me too. Committing to 3.x