-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
PublickKeyTokenProvider: Fix password update routine with password hash #33898
Conversation
@marcelklehr Do you think this PR could be tested on production server? We have like 600GiB of WAL every 2 days (for a DB that weights less than 200GiB), it puts a real burden on our backup server. |
ed55e2d
to
9a2a28f
Compare
Let's wait till this is merged @ldidry |
Ok, thx. 🙂 |
follow up from #33485 |
ICrypto $crypto, | ||
IConfig $config, | ||
LoggerInterface $logger, | ||
ITimeFactory $time, |
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.
Maybe revert the additional spaces as the last line also doesn't have it ;)
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.
👍
45b8964
to
3b10cdd
Compare
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.
Apart from the comment, please squash the fixups and check the lint failure. Otherwise good to get in from my side 👍
3b10cdd
to
c74766e
Compare
c74766e
to
175ac79
Compare
/rebase |
c00f143
to
441d383
Compare
Cypress failure looks related (500 error on the login) |
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.
Maybe naive, but couldn't we compare the value of decrypted password ?
We cannot decrypt all tokens of a user to update them as we only got the user password when the update is called, not the actual cipher to decrypt the tokens (e.g. the app passwords). |
441d383
to
3c7bae8
Compare
For some reason the migration isn't executed before the cypress tests run. Help is appreciated. |
I had the same issue in another PR. Let me see if I can come up with a generic solution |
#35889 should fix the issue. |
3c7bae8
to
3559da1
Compare
Should be good to merge now |
703a860
to
f2d064a
Compare
Pushed another commit to bump the version and trigger the db upgrade on existing setups. |
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
f2d064a
to
bd6ac28
Compare
Failure unrelated |
This basically breaks Talk integration tests. |
$encryptedPassword = $this->encryptPassword($password, $publicKey); | ||
if ($t->getPassword() !== $encryptedPassword) { | ||
$t->setPassword($encryptedPassword); | ||
if ($t->getPasswordHash() === null || $this->hasher->verify(sha1($password) . $password, $t->getPasswordHash())) { |
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.
Did you maybe mean:
if ($t->getPasswordHash() === null || $this->hasher->verify(sha1($password) . $password, $t->getPasswordHash())) { | |
if ($t->getPasswordHash() === null || !$this->hasher->verify(sha1($password) . $password, $t->getPasswordHash())) { |
So the password is updated everytime it does NOT match the old stored password?
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.
But the actual problem is $this->hasher->verify
takes like ~0.2 seconds
/backport to stable25 |
fixes #33757