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

Increase minimum password length for admin user #39319

Open
wants to merge 1 commit into
base: 2.4-develop
Choose a base branch
from

Conversation

torhoehn
Copy link
Contributor

@torhoehn torhoehn commented Nov 2, 2024

Description (*)

PCI 4.0 requires a minimal password length of 12 characters, so it has to be changed from 7 to 12.

Manual testing scenarios (*)

  1. Change password of an existing admin account.
  2. It should fail to change the password if less than 12 characters are used.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Increase minimum password length for admin user #39327: Increase minimum password length for admin user

Copy link

m2-assistant bot commented Nov 2, 2024

Hi @torhoehn. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@m2-github-services m2-github-services added Partner: Basecom partners-contribution Pull Request is created by Magento Partner labels Nov 2, 2024
@torhoehn torhoehn changed the title increase minimum password length Increase minimum password length Nov 2, 2024
@Morgy93
Copy link
Member

Morgy93 commented Nov 2, 2024

This line also requires modification: Line 688 in validation.js.

Update: There’s an additional line that needs attention in the same file: Line 694.

@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 2, 2024

This one needs to be changed as well: https://github.com/magento/magento2/blob/2.4-develop/lib/web/mage/validation.js#L688

Thanks for pointing out, I changed that file as well.

I will leave this PR as simple as it is, to hopefully get it merged fast. I will create another PR one on top of it, to make the values configurable (magento/community-features#333).

@sprankhub
Copy link
Member

@torhoehn, you also need to update the error message mentioned by @Morgy93 earlier.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 4, 2024

I agree that it needs to be improved for admin users, but should it also be enforced for clients?

@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 4, 2024

@torhoehn, you also need to update the error message mentioned by @Morgy93 earlier.

@sprankhub Ah sorry, I will change that as well.

I agree that it needs to be improved for admin users, but should it also be enforced for clients?

@ihor-sviziev I will check that. But to keep it as simple as possible I would like to create another PR for that, because I don't know how many changes will be needed for that.

@torhoehn torhoehn changed the title Increase minimum password length Increase minimum password length for admin user Nov 4, 2024
@sprankhub sprankhub removed their assignment Nov 4, 2024
@engcom-Hotel
Copy link
Contributor

@magento create issue

@engcom-Hotel engcom-Hotel added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Nov 5, 2024
@torhoehn
Copy link
Contributor Author

torhoehn commented Nov 7, 2024

@magento run all tests

@Priyakshic Priyakshic added the Project: Community Picked PRs upvoted by the community label Nov 13, 2024
@engcom-Hotel
Copy link
Contributor

@magento run all tests

Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

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

Hello @torhoehn,

Thanks for the collaboration!

I think we can cover this change with an automated test, so please do the needful and also please fix the failed tests.

Thanks

@torhoehn
Copy link
Contributor Author

@magento run all tests

@torhoehn
Copy link
Contributor Author

@magento run all tests

@torhoehn
Copy link
Contributor Author

@magento run all tests

@ihor-sviziev
Copy link
Contributor

@torhoehn

@ihor-sviziev I will check that. But to keep it as simple as possible I would like to create another PR for that, because I don't know how many changes will be needed for that.

Does it mean, that these changes are applied only for admin users, right? If so - then fine.
I just wonder about asking for a secure passwords for clients who buy on the websites - such a change here might significantly reduce conversion.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Nov 19, 2024

@torhoehn, I just double-checked - it is affecting the only admin users, so it does make sense.

@magento run all tests

@engcom-Hotel
Copy link
Contributor

@magento run Database Compare, Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests, Static Tests, Unit Tests, WebAPI Tests

@engcom-Hotel
Copy link
Contributor

@magento run Static Tests

@engcom-Hotel
Copy link
Contributor

@magento run Database Compare

Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

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

Hello @torhoehn,

Thanks for the collaboration!

The changes seems good to me but please fix the failed static tests. Other failed tests seems flaky to me.

Thanks

@engcom-Hotel
Copy link
Contributor

Hello @torhoehn,

This is part of the latest Adobe Copyright guidelines and it is now covered by static test analysis. So from now on we need to update the copyright as below in the updated files:

Copyright [year code created] Adobe 
All Rights Reserved.

Please do the needful.

Thanks

@torhoehn
Copy link
Contributor Author

@magento run all tests

@hostep
Copy link
Contributor

hostep commented Nov 22, 2024

@torhoehn: the year in the copyright header should be the year when the file was first created, so 2024 is wrong, you should check the git history for each file to find the correct year. For example, the file app/code/Magento/User/Model/UserValidationRules.php got created in 2015, lib/web/mage/validation.js in 2014, ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Partner: Basecom partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: needs update Project: Community Picked PRs upvoted by the community
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

[Issue] Increase minimum password length for admin user
8 participants