Great idea, ack from me,

e9dd75d..fbc3556 master -> master

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

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

This addresses that:

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 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

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.

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

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

Thank you! Looks great!

b086d41..c8b388b master -> master

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

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...

Thank Mark, you have my ack

f95b8e7..d86e0f9 master -> master

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

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2429

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.