[Security] bug #10242 Missing checkPreAuth from RememberMeAuthenticationProvider #11058
Conversation
| $userChecker = $this->getMock('Symfony\Component\Security\Core\User\UserCheckerInterface'); | ||
| $userChecker->expects($this->once()) | ||
| ->method('checkPreAuth') | ||
| ->will($this->throwException(new DisabledException())) |
There was a problem hiding this comment.
Why does your mock throws an exception, if, then, you do not assert anything?
There was a problem hiding this comment.
@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.
@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.
right, I missed the annotation. sorry
There was a problem hiding this comment.
test case is edited to be consistent with postcheck test
There was a problem hiding this comment.
Acutally, mocking exceptions is not consistent with rest of Symfony. I'd avoid this. See #10621
There was a problem hiding this comment.
I will fix that ...
There was a problem hiding this comment.
Done (real exceptions instances are thrown)
|
👍 |
|
+1 to merge this fix asap (which could be seen as a security issue) |
|
+1 |
|
Any updates on this? |
|
Nothing :'( |
|
👍 Fix an issue I had. |
|
Any update? |
|
+1 |
|
Ping @stof for the review (I don't know who's the best member from the core team in charge of the security component) I don't think this could be seen as a security issue (well… sort of), but it's could be quite critical in some case and the PR stands for a few month now. Thanks. |
|
+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 to
db99801
Compare
db99801 to
510218f
Compare
|
Looks good to me. |
|
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::checkPreAuth edit : after the discution with @hellomedia , i replaced postcheck with precheck glutamatt@e0730e0#commitcomment-6580764 Commits ------- a38d1cd bug #10242 Missing checkPreAuth from RememberMeAuthenticationProvider
| @@ -50,7 +50,7 @@ public function authenticate(TokenInterface $token) | |||
| } | |||
|
|
|||
| $user = $token->getUser(); | |||
| $this->userChecker->checkPostAuth($user); | |||
| $this->userChecker->checkPreAuth($user); | |||
There was a problem hiding this comment.
IMO, adding the preAuth check is good, but postAuth should not be removed
There was a problem hiding this comment.
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.
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