Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] bug #10242 Missing checkPreAuth from RememberMeAuthenticationProvider#11058
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why does your mock throws an exception, if, then, you do not assert anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@jeremy-derusse expectation is defined with the@expectedException annotation.
However, I wouldn't use$userChecker->expects($this->once()), but$userChecker->expects($this->any()).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@jakzal checkPreAuth must be called once in this test, as muche as checkPostAuth in the next test case
I just see that the exeption thrown in the postcheck test is mocked : i will do the same here to be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
right, I missed the annotation. sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
test case is edited to be consistent with postcheck test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Acutally, mocking exceptions is not consistent with rest of Symfony. I'd avoid this. See#10621
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I will fix that ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done (real exceptions instances are thrown)
odolbeau commentedJun 5, 2014
👍 |
MattKetmo commentedJun 30, 2014
+1 to merge this fix asap (which could be seen as a security issue) |
cvasseur commentedJul 1, 2014
+1 |
cmodijk commentedJul 8, 2014
Any updates on this? |
glutamatt commentedJul 9, 2014
Nothing :'( |
gredin commentedJul 30, 2014
👍 Fix an issue I had. |
odolbeau commentedSep 2, 2014
Any update? |
c7nj7n commentedSep 2, 2014
+1 |
MattKetmo commentedSep 17, 2014
Ping@stof for the review (I don't know who's the best member from thecore team in charge of the security component) I don't think this could be seen as asecurity issue (well… sort of), but it's could be quite critical in some case and the PR stands for a few month now. Thanks. |
dschenk commentedSep 19, 2014
+1 I think its a pretty severe security issue. If a user gets disabled he can still login with the remember-me cookie. If such a user is an admin (and there were certain reasons to disable the account), he can wreak havoc on the system... |
30a34b0 todb99801Comparedb99801 to510218fComparefabpot commentedSep 24, 2014
Looks good to me. |
fabpot commentedSep 24, 2014
Thank you@glutamatt. |
…AuthenticationProvider (glutamatt)This PR was submitted for the 2.4 branch but it was merged into the 2.3 branch instead (closes#11058).Discussion----------[Security] bug#10242 Missing checkPreAuth from RememberMeAuthenticationProvider| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#10242| License | MIT[Security] fixed missing call to UserChecker::checkPreAuthedit : after the discution with@hellomedia , i replaced postcheck with precheckglutamatt@e0730e0#commitcomment-6580764Commits-------a38d1cd bug#10242 Missing checkPreAuth from RememberMeAuthenticationProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO, adding the preAuth check is good, but postAuth should not be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it's something i was thinking about, but i followed this pr :#9902
and prefered to stay consistent with.
postauth check if credentials are fresh enough (if i'm not wrong), i focused on triger critical checks performed by preAuth checks
it's a matter of remeberme feature ... I can't decide this alone
Anyway, thanks for your remark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am the one who suggested@glutamatt to remove it.
As discussed here :glutamatt@e0730e0
PostCheck only checks for credentials expiration. I would think that could - but does not have to - prevent the user from logging in. It could just be a suggestion to change a password after X months.
Maybe I am wrong, but if it should always prevent from loging in, why is there 2 separate checks, pre-check and post-check ? Why not everything in the same global check ?
[Security] fixed missing call to UserChecker::checkPreAuth
edit : after the discution with@hellomedia , i replaced postcheck with precheck
glutamatt@e0730e0#commitcomment-6580764