Successful login with IDP but the user is still considered as anonymous

Having the same issue, this bug appeared after we had enabled the samesite=strict cookie policy.

Took sime time.... but I think I have managed to find a solution to the problem. I got inspiration from the \Drupal\comment\Controller\CommentController::commentPermalink().

Also, I am not sure what BC promise this module follows, but since Drupal's BC promise also does not cover page controllers, I dared to perform a bigger clean-up on the SimplesamlphpAuthController.php besides the logic fixes that I have made.

Hopefully \Drupal::service('page_cache_kill_switch')->trigger(); is not needed anymore, which is a two edged sword...

Please review.

CS fixes.

Hm, that's quite a patch to review. BC is one thing, but it''s quite hard to understand what's just refactoring/improvements and what the actual changes are.

Also, not surprised that samesite strict is causing problems, surprised that anything works with that ;)

Yeah, I know... :)

The actual change is:

+        // A simple redirection would not be enough here. We have to carry over
+        // the session initiated by external auth to the sub-request.  Without
+        // that the screen would look like as if the user would not have been
+        // logged in successfully, although it did.
+        $redirect_request = Request::create($destination->getGeneratedUrl(), 'GET', $request->query->all(), $request->cookies->all(), [], $request->server->all());
+        if ($request->hasSession()) {
+          $redirect_request->setSession($request->getSession());
+        }
+        $response = $this->httpKernel->handle($redirect_request, HttpKernelInterface::SUB_REQUEST);
+        if ($response instanceof CacheableResponseInterface) {
+          $response->addCacheableDependency($destination);
         }

All the other fixes just make the logic more simpler and the code more easier to read and maintain. This is the reason why this one patch could possibly resolve 2-3 other issues in the queue as well.

I also bumped into some early rendering issues - all Drupal devs' favorite one - this is the reason why this is a method and not a property:

+  /**
+   * The default response returned by the authentication controller.
+   *
+   * @return \Symfony\Component\HttpFoundation\Response
+   *   The response.
+   */
+  private function defaultReponse() {
     return $this->redirect('user.login');
   }

and I also thought about PHP >= 7.3 compatibility as you can see - if I have spent hours on this controller :)

(although I must admit that part of the code may require some fine tuning, like parsing the domain and leveraging the samesite option in case of PHP >= 7.3)

Also, not surprised that samesite strict is causing problems, surprised that anything works with that ;)

Update: a few months ago we had to change Strict to LAX because everything was working...except when a user open a one time login link in Gmail, Outlook online, etc, s/he could not use it... 🙄 (thanks for the implementation of the one-time login flow)

How can we can we make a step forward with the review of this patch? Are there any active maintainers here?

Maybe PHP 7.3 compatibility bridge could be moved to a new patch since PHP 7.2 is EOL in 2 days and the changed syntax of setrawcookie() should cause problems to everyone.

I also bumped into some early rendering issues - all Drupal devs' favorite one - this is the reason why this is a method and not a property:

It seems there is an another way around to eliminate early rendering issues, although since my added workaround in #4 seems to be working, I am just leaving this here: https://www.drupal.org/project/drupal/issues/2638686#comment-13958570

I am not sure whether this was your intention @mxr576, but testing e.g. with https://example.com/saml_login?ReturnTo=https://example.com/admin/group renders the contents of /admin/group at the URL https://example.com/saml_login?ReturnTo=https%3A%2F%2Fexample.com%2Fadmi...

- and when I don't have an explicit ReturnTo query parameter defined, I get the site front page at the same URL. This is a bit confusing at least to me. A better way, if possible, would IMO be to redirect the user to the page instead of showing the rendered content of the page at the saml_login route.

I came across this issue with a similar problem. When my Drupal site is tweaked to only use `SameSite` cookies, this module starts to fail to work for me. It seems to me like the module should have a setting to allow users to turn on the samesite cookie protection when it's setting the cookies...? Following some of the comments, I couldn't quite understand how this relates to redirection...?

I think the comment in #5 may have been asking for a simpler patch that can just fix the issue, or at least that's what I would think this issue needs. Then another issue can be opened up to do some of the refactoring, which can be reviewed independently of this issue. That's what I would suggest here as the best approach to get a patch merged in...

@kekkis you are right, as far I as I remember that is not an expected side effect, but I do not remember having that issue on the project where the original issue was fixed. The key problem that I tried to solve is not loosing cookie data in given circumstances when redirection happens from the IDP to Drupal.