Skip to content
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

EZP-31462: Removed password hash types break login #141

Merged
merged 4 commits into from
Feb 16, 2021

Conversation

glye
Copy link
Member

@glye glye commented Dec 2, 2020

Question Answer
JIRA issue EZP-31462
Type bug
Target eZ Platform version v3.2+
BC breaks no
Doc needed yes

The previous fixes are incomplete. When an invalid hash type is detected, we expire the password and ask the user to enter a new password. But when we save this new password, we don't update the hash type, which means the password isn't saved, and the user ends up in a redirect loop.

However, cache can mask the issue. If you test by manually changing hash type to an invalid one in the db, you have to clear caches before you go through the test steps.

This fix catches when the type is invalid, and sets it to the default type in that case.

DOC: Once fixed, I suggest the relevant doc should mention that you have to upgrade to at least version x for this to work.
https://github.com/ezsystems/developer-documentation/blob/03a7cc0ea04f3e6bf7ebf22a8bea53840123196e/docs/releases/ez_platform_v3.0_deprecations.md#password-hashes

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ezsystems/php-dev-team).

@glye
Copy link
Member Author

glye commented Dec 3, 2020

Fixup: Get the default hash type from the password hash service, rather than hardcoding it.

NB: It's not at all certain that the final fix will look like this. It may be the wrong place to fix it.

@sonarcloud
Copy link

sonarcloud bot commented Dec 3, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kaff kaff requested a review from alongosz December 3, 2020 13:29
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Could be, but I'm not gonna accept this w/o either integration or unit tests.

@glye
Copy link
Member Author

glye commented Dec 8, 2020

Fair point. Set as work in progress, and I've asked for Eng help on it. I need to focus on the growing security issue list, and this isn't security. Maybe @kaff will be able to look at this later? 🏃💨

@alongosz alongosz changed the base branch from 1.1 to 1.2 February 10, 2021 13:24
@alongosz alongosz force-pushed the ezp-31462_removed_password_hash_types_break_login branch from cee5500 to 2944284 Compare February 10, 2021 13:34
@alongosz alongosz force-pushed the ezp-31462_removed_password_hash_types_break_login branch from 2944284 to 9947249 Compare February 10, 2021 15:50
@alongosz alongosz requested review from mikadamczyk, adamwojs, ciastektk, Nattfarinn and webhdx and removed request for kaff February 11, 2021 01:18
eZ/Publish/Core/Repository/UserService.php Outdated Show resolved Hide resolved
Co-Authored-By: Paweł Niedzielski <Steveb-p@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented Feb 11, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@micszo micszo self-assigned this Feb 16, 2021
Copy link
Member

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Retested successfully.
QA Approved on Ibexa Experience v3.2.4 with diff.

@micszo micszo removed their assignment Feb 16, 2021
@alongosz alongosz merged commit f0ed375 into 1.2 Feb 16, 2021
@alongosz alongosz deleted the ezp-31462_removed_password_hash_types_break_login branch February 16, 2021 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants