http://cgit.drupalcode.org/simplesamlphp_auth/tree/src/Controller/Simple...

CommentFileSizeAuthor
#8 2904061-8.patch4.05 KBmikejw
#2 2904061.patch3.33 KBmattgill
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mattgill created an issue. See original summary.

mattgill’s picture

mattgill CreditAttribution: mattgill as a volunteer commented
FileSize
2904061.patch3.33 KB

Contributing patch to convert cookie methods to using the Symfony mechanism.

snufkin’s picture

snufkin CreditAttribution: snufkin at Cheppers commented

Overall I love this, I am really happy to add more standardised, framework dependent API. Without fully digging into the code I have one question:

+++ b/src/Controller/SimplesamlphpAuthController.php
@@ -202,17 +205,61 @@ class SimplesamlphpAuthController extends ControllerBase implements ContainerInj
+        $this->revokeReturnToCookie($response);

The original removal of the cookie on line 206 is outside of the $redirect conditional, but here we are adding it to the $redirect context. Are we losing any functionality as a result of this, or there are no cases when we need to unset the cookie when there are no redirects?

mattgill’s picture

mattgill CreditAttribution: mattgill as a volunteer commented

Thanks!
I had another look through, my understanding of the code is that:

Originally, if the redirect cookie was set,

  1. Set the redirect variable
  2. Unset the redirect cookie

And in the patch, it does the same, but instead of unsettling the cookie at that point, waits until inside the redirect conditional (where we have the response object available). Because we still set the redirect variable, we can say that we wouldn't loose any functionality (the cookie is just removed slightly later in the execution).

Hope that makes sense!!

mattgill’s picture

mattgill CreditAttribution: mattgill as a volunteer commented

Also, if you get a moment, can you check line 152:
http://cgit.drupalcode.org/simplesamlphp_auth/tree/src/Controller/Simple...

I believe
$request->request->get('ReturnTo')

should be replaced with
$request->get('ReturnTo')

if you agree, let me know and I'll submit another patch :)

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented

@mattgill, re #4, it's better to be explicit so I wouldn't replace it, leave it as $request->request->get('ReturnTo')

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
Status: Needs review » Closed (duplicate)

It looks like this may have been resolved in another way in the dev branch.

mikejw’s picture

mikejw CreditAttribution: mikejw at University of Adelaide commented
Status: Closed (duplicate) » Needs review
FileSize
2904061-8.patch4.05 KB

I was getting errors thrown about setting headers - maybe due to something introduced in recent versions of Drupal? Anyhow, here is a patch based on the other patch in this thread and working against the latest dev.