#49370 local password policies should use the same defaults as the global policy
Closed: fixed 2 years ago Opened 2 years ago by mreynolds.

Issue Description

When we create a local password policy we do not use any defaults like what we do with the global policy. They should be consistent.


Metadata Update from @mreynolds:
- Issue assigned to mreynolds

2 years ago

Metadata Update from @mreynolds:
- Custom field component adjusted to None
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to None
- Custom field type adjusted to None
- Custom field version adjusted to None
- Issue set to the milestone: 1.3.6.0

2 years ago

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to review (was: None)

2 years ago

Metadata Update from @firstyear:
- Custom field reviewstatus adjusted to ack (was: review)

2 years ago

Great idea, ack from me,

e9dd75d..fbc3556 master -> master

c520fb7..5a2b673 389-ds-base-1.3.6 -> 389-ds-base-1.3.6

Metadata Update from @mreynolds:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1465600
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

2 years ago

I missed the default "on/off" settings :-(

This addresses that:

0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to review (was: ack)

2 years ago

Metadata Update from @mreynolds:
- Issue status updated to: Open (was: Closed)

2 years ago

The patch looks good but looks similar from initialization done in FrontendConfig_init.
Would it make sense to have a common initialization pw_policy function called from both FrontendConfig_init and new_passwdPolicy ?

Metadata Update from @tbordaz:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

2 years ago

Metadata Update from @mreynolds:
- Issue status updated to: Open (was: Closed)

2 years ago

The patch looks good but looks similar from initialization done in FrontendConfig_init.
Would it make sense to have a common initialization pw_policy function called from both FrontendConfig_init and new_passwdPolicy ?

The issue is the on/offs in frontend init also update local variables as well.

init_pw_unlock = cfg->pw_policy.pw_unlock = LDAP_ON;

I would need to create multiple init functions for all the cases. In my opinion it complicates things. Agree? Or disagree?

Actually I will create the extra functions, and I want to improve the CI tests anyway.

Revised patch with CI test

0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch

The test looks good! Some issues though...
passw_policy fixture fails:

    topo.standalone.subtreePwdPolicy(subtree, {'passwordchange': 'on',
                                               'passwordCheckSyntax': 'on',
                                               'passwordLockout': 'on',
                                               'passwordResetFailureCount': '3',
                                               'passwordLockoutDuration': '3',
>                                                  'passwordMaxFailure': '2'})

dirsrvtests/tests/suites/password/regression_test.py:55:
E         TypeError: ('expected a byte string in the list', 'o')

So eventually we need to add a new class like this nsPwPolicyContainer(nsContainer) that will have at least one main method "setup subtree password policy with this set of parameters {}" and some additional methods.

But if we are in hurry now with the BZ, I'd say we can fix byte issues in the existing method and add the class later. Just use ensure_bytes on arguments in add_s and modify_s methods.

Another small thing.
Instead of USER_DN, you can use existing test_user fixture because it already returns UserAccount object. So you can 'bind' with it. For DM we have DirectoryManager object.

test_user.bind(PASSWORD)
dm = DirectoryManager(inst)
dm.bind() # it has PW_DM by default

And you don't need wrappers like this because the failure will happen we will have nice stack trace anyway.

116 +    try:
117 +        topo.standalone.simple_bind_s(USER_DN, PASSWORD)
118 +    except ldap.LDAPError as e:
119 +        log.fatal("Failed to login after reset time: " + str(e))
120 +        raise e

Here:

106 +    log.info('Verify account is locked')
107 +    try:
108 +        topo.standalone.simple_bind_s(USER_DN, PASSWORD)
109 +    except ldap.CONSTRAINT_VIOLATION:
110 +        log.info("Account is now locked")

You just log the info. Probably it will be better to use some assert statement if we really expect it. 'with pytest.raises(ldap.CONSTRAINT_VIOLATION)' is a good one!

Revised patch attach.

@spichugi The directory manager object does not work for test cases as it opens a new connection, but it does not bind on the existing one, but i was able to include your other suggestions.

0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch

I'm happy with the C code.

The python I think is only python 2 compatible. As well, pytest.raises is better than:

try:
    thing
except e:
    pass

Given it's an existing test, I think perhaps we may just accept it "as is" and the fix it later instead?

@mreynolds thanks Mark for this nice clean up.
I have a doubt about the patch.

  • pw_storagescheme this field was part of your previous patch but it is no longer present in your second patch. Is it a copy/paste issue or intentional ?

  • pw_track_update_time this field was set to fe password policy in the previous patch. Now calling pwpolicy_init_defaults it is set to OFF.

Ok, what I mean is to have something like this:

log.info("Verify user can bind...")
test_user.bind(PASSWORD)

log.info('Test passwordUnlock default - user should be able to reset password after lockout')
for i in range(0,2):
    with pytest.raises(ldap.INVALID_CREDENTIALS):
        test_user.bind("bad-password")

log.info('Verify account is locked')
with pytest.raises(ldap.CONSTRAINT_VIOLATION):
    test_user.bind(PASSWORD)

log.info('Wait for lockout duration...')
time.sleep(4)

log.info('Check if user can now bind with correct password')
test_user.bind(PASSWORD)

test_user.bind() will return a new connection and the topology.standalone will still have DM bind (so we dont need to rebind fot the clean up)

@mreynolds thanks Mark for this nice clean up.
I have a doubt about the patch.

pw_storagescheme this field was part of your previous patch but it is no longer present in your second patch. Is it a copy/paste issue or intentional ?

I think it was a copy and pate error - nice catch!

pw_track_update_time this field was set to fe password policy in the previous patch. Now calling pwpolicy_init_defaults it is set to OFF.

It was always OFF by default. Previously we pulled in the global value,. but it should use the default.

CI test adjusted, and fixed the pw storage scheme issue reported by @tbordaz

0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch

The C part is good to me. you have my ack

Metadata Update from @spichugi:
- Custom field reviewstatus adjusted to ack (was: review)

2 years ago

Thank you! Looks great!

b086d41..c8b388b master -> master

0584617..400e6eb 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

2 years ago

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to review (was: ack)

2 years ago

The patch looks good. You might check pwscheme_name before calling pw_name2scheme.

@tbordaz Nice catch, it might be rare that the name would be NULL, but it's possible...

0001-Ticket-49370-Crash-when-using-a-global-and-local-pw.patch

Thank Mark, you have my ack

Metadata Update from @tbordaz:
- Custom field reviewstatus adjusted to ack (was: review)

2 years ago

f95b8e7..d86e0f9 master -> master

873d48c..49291b7 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

Login to comment on this ticket.