8000 Delete cookies from the TokenProvider that is no longer in use by TerjeBr · Pull Request #6055 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Delete cookies from the TokenProvider that is no longer in use #6055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 24, 2012
Merged

Delete cookies from the TokenProvider that is no longer in use #6055

merged 1 commit into from
Nov 24, 2012

Conversation

TerjeBr
Copy link
@TerjeBr TerjeBr commented Nov 18, 2012

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Todo: -
License of the code: MIT

When the user logs in, or login fails for some reason, the old "remember me" cookie should be deleted from the TokenProvider if you are using the PersistentTokenBasedRememberMeServices.

As the code is now, the token is only deleted on logout.

Tobion
Tobion reviewed Nov 18, 2012
View reviewed changes
src/Symfony/Component/Security/Http/RememberMe/PersistentTokenBasedRememberMeServices.php
@@ -63,10 +63,12 @@ public function setTokenProvider(TokenProviderInterface $tokenProvider)
/**
* {@inheritDoc}
*/
public function logout(Request $request, Response $response, TokenInterface $token)
public function cancelCookie(Request $request)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be protected

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have fixed that now.

@TerjeBr
Copy link
Author
TerjeBr commented Nov 20, 2012

So, anything else that needs to be done before this is merged?

fabpot
fabpot reviewed Nov 21, 2012
View reviewed changes
src/Symfony/Component/Security/Http/RememberMe/AbstractRememberMeServices.php
@@ -172,6 +172,9 @@ public function logout(Request $request, Response $response, TokenInterface $tok
*/
final public function loginSuccess(Request $request, Response $response, TokenInterface $token)
{
// Make sure any old remember-me cookies are canceled
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canceled should be cancelled.

@TerjeBr
Copy link
Author
TerjeBr commented Nov 21, 2012

Ok, I have corrected the typo in the comment and squashed the commit.

@schmittjoh
Copy link
Contributor

btw, canceled (more American) and cancelled (more British) are both
correct English forms.

On Wed, Nov 21, 2012 at 11:30 AM, Terje Bråten notifications@github.comwrote:

Ok, I have corrected the typo in the comment and squashed the commit.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6055#issuecomment-10592112.

@schmittjoh
Copy link
Contributor

As a side-note have you verified that this does not break the cookie theft protection?

@TerjeBr
Copy link
Author
TerjeBr commented Nov 21, 2012

Yes, cookie theft protection is still there and is functioning well.

@TerjeBr
Copy link
Author
TerjeBr commented Nov 21, 2012

I am using this together with the DoctrineTokenProvider in issue #6057 in my own project and done some extensive testing on it.

@TerjeBr
Copy link
Author
TerjeBr commented Nov 23, 2012

Is this ready to be merged now?

fabpot added a commit that referenced this pull request Nov 24, 2012
@fabpot
merged branch TerjeBr/persistent-token-provider (PR #6055)
d5ff238 
This PR was merged into the master branch.

Commits
-------

d1b5093 Try to make sure cookies get deleted from the TokenProvider when no longer in use

Discussion
----------

Delete cookies from the TokenProvider that is no longer in use

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Todo: -
License of the code: MIT

When the user logs in, or login fails for some reason, the old "remember me" cookie should be deleted from the TokenProvider if you are using the PersistentTokenBasedRememberMeServices.

As the code is now, the token is only deleted on logout.

---------------------------------------------------------------------------

by TerjeBr at 2012-11-20T13:45:54Z

So, anything else that needs to be done before this is merged?

---------------------------------------------------------------------------

by TerjeBr at 2012-11-21T10:30:53Z

Ok, I have corrected the typo in the comment and squashed the commit.

---------------------------------------------------------------------------

by schmittjoh at 2012-11-21T10:36:29Z

btw, ``canceled`` (more American) and ``cancelled`` (more British) are both
correct English forms.

On Wed, Nov 21, 2012 at 11:30 AM, Terje Bråten <notifications@github.com>wrote:

> Ok, I have corrected the typo in the comment and squashed the commit.
>
> —
> Reply to this email directly or view it on GitHub<#6055 (comment)>.
>
>

---------------------------------------------------------------------------

by schmittjoh at 2012-11-21T10:40:24Z

As a side-note have you verified that this does not break the cookie theft protection?

---------------------------------------------------------------------------

by TerjeBr at 2012-11-21T10:51:10Z

Yes, cookie theft protection is still there and is functioning well.

---------------------------------------------------------------------------

by TerjeBr at 2012-11-21T11:14:04Z

I am using this together with the DoctrineTokenProvider in issue #6057 in my own project and done some extensive testing on it.

---------------------------------------------------------------------------

by TerjeBr at 2012-11-23T10:30:34Z

Is this ready to be merged now?
@fabpot fabpot merged commit d1b5093 into symfony:master Nov 24, 2012
@TerjeBr
Copy link
Author
TerjeBr commented Nov 24, 2012

Thank you for merging this.

@nmariani
Copy link

I try to implement the Remember Me functionality but I can't make it work.
Are you sure that this PR did not break the basic functionality.

  1. The cancelCookie method is always called in the AbstractRememberMeServices::loginSuccess so a blank cookie (value null) is always added in request attributes for each login.
  2. Then the method TokenBasedRememberMeServiceson::LoginSuccess is called and add a new "RememberMe" cookie in the response headers.
  3. And finally the Symfony\Component\Security\Http\RememberMe\ResponseListener::onKernelResponse is called, detect the blank cookie in the request attributes and replace the "RememberMe" cookie above with the blank one.
  4. As a result the response headers contain a cookie with a value set to NULL

Do I miss something or am I doing something wrong ?

@nmariani
Copy link

When I comment the lines of code added in this PR, everything works as expected.

@schmittjoh
Copy link
Contributor

I was skeptical when I saw the PR, but I had no time to test it.

I guess Fabien has though?

On Wed, Nov 28, 2012 at 5:09 PM, Nathanaël Mariani <notifications@github.com

wrote:

When I comment the lines of code added in this PR, everything works as
expected.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6055#issuecomment-10808369.

@TerjeBr
Copy link
Author
TerjeBr commented Nov 28, 2012

@nmariani
In PersistentTokenBasedRememberMeServices.php at line 136 there is

$request->attributes->remove(self::COOKIE_ATTR_NAME);

That should remove the blank cookie so the ResponseListener do not set it, as you say it does in point 3.

@TerjeBr
Copy link
Author
TerjeBr commented Nov 28, 2012

OK, I see now.
The above mentioned line should be moved into "AbstractRememberMeServices.php", because other RememberMeServices that is not PersistentTokenBased will also inherit from the class AbstractRememberMeServices, and they do not contain the line that removes the "set cookie attribute" from the request.

How shall I do the fix? Is it best that I amend this pull request, or is it better that I start a new pull request since this one is already merged?

@fabpot
Copy link
Member
fabpot commented Nov 29, 2012

@TerjeBr Open a new PR please, thanks.

@TerjeBr TerjeBr mentioned this pull request Nov 29, 2012
Merged
fabpot added a commit that referenced this pull request Dec 2, 2012
@fabpot
merged branch TerjeBr/persistent-token-provider (PR #6149)
150a138 
This PR was merged into the master branch.

Commits
-------

373be62 Bugfix for creating cookie on loginSuccess in AbstractRememberMeServices

Discussion
----------

Bugfix for creating cookie on loginSuccess in AbstractRememberMeServices

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #6055
Todo: -
License of the code: MIT

By a mistake setting of new cookies did not work for other RememberMe services than PersistentTokenBasedRememberMeServices

---------------------------------------------------------------------------

by TerjeBr at 2012-12-01T17:28:08Z

Ping.  Any feedback on this?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TerjeBr @schmittjoh @nmariani @fabpot @pborreli @Tobion
0