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
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
<img alt="0001-Ticket-49370-local-password-policies-should-use-the-.patch" src="/389-ds-base/issue/raw/files/0df9672144d4292371ad05d722e9f444756e2f4536248ef984234663114044bc-0001-Ticket-49370-local-password-policies-should-use-the-.patch" />
Metadata Update from @mreynolds: - Custom field reviewstatus adjusted to review (was: None)
Metadata Update from @firstyear: - Custom field reviewstatus adjusted to ack (was: review)
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)
I missed the default "on/off" settings :-(
This addresses that:
<img alt="0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch" src="/389-ds-base/issue/raw/files/d69272b079e9f3d52b74bd35f3168b576c2777a1125c8c9e50ddaa8706a6e35e-0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch" />
Metadata Update from @mreynolds: - Custom field reviewstatus adjusted to review (was: ack)
Metadata Update from @mreynolds: - Issue status updated to: Open (was: Closed)
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)
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?
Metadata Update from @mreynolds: - Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1465600, https://bugzilla.redhat.com/show_bug.cgi?id=1535515 (was: https://bugzilla.redhat.com/show_bug.cgi?id=1465600)
Actually I will create the extra functions, and I want to improve the CI tests anyway.
Revised patch with CI test
<img alt="0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch" src="/389-ds-base/issue/raw/files/1487a0f5a5664a56a03fc9d685fa66e8effaab30136fbf0f5287c74a3ba6cb99-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.
<img alt="0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch" src="/389-ds-base/issue/raw/files/30e560a081d62ab2461b667d9533585f571c7c85aa66fb340171c2d4d35f042b-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!
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
<img alt="0001-Ticket-49370-Add-all-the-password-policy-defaults-to.patch" src="/389-ds-base/issue/raw/files/5219ba61036b7e7cfc04070d0328663e4da57507c0408d86b2f045743e360c0d-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)
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)
<img alt="0001-Ticket-49370-Crash-when-using-a-global-and-local-pw-.patch" src="/389-ds-base/issue/raw/files/f5d011cc871fd8b3ff31ba20f3e52ca972fa47a308150779cbd7f847e8fdfc32-0001-Ticket-49370-Crash-when-using-a-global-and-local-pw-.patch" />
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...
<img alt="0001-Ticket-49370-Crash-when-using-a-global-and-local-pw.patch" src="/389-ds-base/issue/raw/files/ec8d0b1251ac472bf06d640a918e33222ca44321e0ec81b0a01865bd901d1979-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)
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.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: fixed)
Log in to comment on this ticket.