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

Add "class" to password_policies #5339

Closed
wants to merge 1 commit into from

Conversation

Rtoax
Copy link

@Rtoax Rtoax commented Nov 23, 2023

For example:

    Password    Class
    1234        1
    1abc        2
    1aBC        3
    1aB!        4

@pep8speaks
Copy link

pep8speaks commented Nov 23, 2023

Hello @Rtoax! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 198:24: E203 whitespace before ':'
Line 198:100: E501 line too long (121 > 99 characters)
Line 199:26: E203 whitespace before ':'
Line 199:100: E501 line too long (122 > 99 characters)

Line 418:100: E501 line too long (109 > 99 characters)

Comment last updated at 2023-11-23 08:19:17 UTC

@github-actions github-actions bot added the f40 label Nov 23, 2023
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

@Rtoax thanks for the PR.

Can you please describe the use case for this new feature? How does the class differs from the quality? Did not go through the code but I would like to see this in the commit message these information.

It's also missing tests; Probably to be inserted at: tests/unit_tests/pyanaconda_tests/modules/common/test_policy.py

Before you put more effort on this give the team a short time to react on the desirability of this feature.

CC @poncovka any thoughts?

For example:

    Password    Class
    1234        1
    1abc        2
    1aBC        3
    1aB!        4

Signed-off-by: Rong Tao <[email protected]>
@Rtoax
Copy link
Author

Rtoax commented Nov 23, 2023

@Rtoax thanks for the PR.

Can you please describe the use case for this new feature? How does the class differs from the quality? Did not go through the code but I would like to see this in the commit message these information.

It's also missing tests; Probably to be inserted at: tests/unit_tests/pyanaconda_tests/modules/common/test_policy.py

Before you put more effort on this give the team a short time to react on the desirability of this feature.

CC @poncovka any thoughts?

This commit just do the same thing like libpwquality's minclass in /etc/security/pwquality.conf

How/What to use/show

image

image

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

I see the libpwquality has a configuration option for the mean class. We are already using this library for validation password quality, and the checks for length are passed through that.

I see minclass is a configurable option in the pwquality library, so it looks like more reasonable to work on that direction.

I see the checks for minlen are on this direction so probably the logic can be stolen from that. How do you see this? Are you satisfied with this solution direction?

@@ -291,13 +291,14 @@ can_change_users = False
#
# quality <NUMBER> The minimum quality score (see libpwquality).
# length <NUMBER> The minimum length of the password.
# class <NUMBER> The minimum class of the password characters.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to reuse the libpwquality class configuration option here:
https://manpages.ubuntu.com/manpages/jammy/man5/pwquality.conf.5.html

See minclass

@KKoukiou KKoukiou marked this pull request as draft January 3, 2024 15:19
@KKoukiou KKoukiou added f41 and removed f40 labels Feb 29, 2024
@KKoukiou
Copy link
Contributor

Closing because of long time inactivity. If you 're still actively interested feel free to ping to reopen.

@KKoukiou KKoukiou closed this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants