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.

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

Triggering RememberMe's loginFail() when token cannot be created #27297

Merged
merged 1 commit into from May 27, 2018

Conversation

Projects
None yet
6 participants
@weaverryan
Member

weaverryan commented May 17, 2018

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no (but minor behavior change)
Deprecations? no->
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR not needed

This is an edge-case bug fix. If, for example, someone tampers with the remember me cookie, and so it is invalid, this causes the ->autoLogin() call to throw an AuthenticationException. But, this did not call the loginFail() method.

Honestly, I'm not sure if the old or new behavior is correct. But, we should discuss and merge or close.

if (!$this->catchExceptions) {
throw $e;
}
return;
}

This comment has been minimized.

@weaverryan

weaverryan May 17, 2018

Member

Look at the rest of this method for full context. The new code basically tries to treat a failure from autoLogin just like the failure below. I only didn't move the call into the try-catch, so that we could have slightly different log messages.

@fabpot

fabpot approved these changes May 18, 2018

@iltar

This comment has been minimized.

Contributor

iltar commented May 18, 2018

As a remember me doesn't have an actual authentication attempt, but only refreshes, I think it's safe to assume that all behavior should be consistent for a refresh. If that means that in this scenario the loginfail was not triggered, it probably should be.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone May 18, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented May 27, 2018

merging into 2.8 as 2.7 is not maintained anymore.

@fabpot fabpot changed the base branch from 2.7 to 2.8 May 27, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented May 27, 2018

Thank you @weaverryan.

@fabpot fabpot merged commit e3412e6 into symfony:2.8 May 27, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 27, 2018

bug #27297 Triggering RememberMe's loginFail() when token cannot be c…
…reated (weaverryan)

This PR was submitted for the 2.7 branch but it was merged into the 2.8 branch instead (closes #27297).

Discussion
----------

Triggering RememberMe's loginFail() when token cannot be created

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no (but minor behavior change)
| Deprecations? | no->
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | not needed

This is an edge-case bug fix. If, for example, someone tampers with the remember me cookie, and so it is invalid, this causes the `->autoLogin()` call to throw an `AuthenticationException`. But, this did not call the `loginFail()` method.

Honestly, I'm not sure if the old or new behavior is correct. But, we should discuss and merge or close.

Commits
-------

e3412e6 Triggering RememberMe's loginFail() when token cannot be created
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment