Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1018943

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1018944

Shadow Account Support Design - http://www.port389.org/docs/389ds/design/shadow-account-support.html

dirsrvtests/tickets/ticket548_test.py I would change the asserts a little bit.

{{{
def check_shadow_attr_value(entry, attr_type, expected, dn):
if entry.hasAttr(attr_type):
actual = entry.getValue(attr_type)
if int(actual) == expected:
log.info('%s of entry %s has expected value %s' % (attr_type, dn, actual))
assert True
else:
log.fatal('%s %s of entry %s does not have expected value %s' % (attr_type, actual, dn, expected))
assert False
else:
log.fatal('entry %s does not have %s attr' % (dn, attr_type))
assert False

}}}

I have built and tested the code, and it looks good to me. Ack from me.

About '''dirsrvtests/tickets/ticket548_test.py''':

This code surely would work and do its job.

But if we want to make troubleshooting easy and more "pytestic", I think, we should:
- move '''def test_ticket548_final''' to topology fixture like this:

{{{
def fin():
standalone.delete()
request.addfinalizer(fin)
}}}
- make "main" block like this:

{{{
if name == 'main':
# Run isolated
# -s for DEBUG mode
CURRENT_FILE = os.path.realpath(file)
pytest.main("-s %s" % CURRENT_FILE)
}}}

The rest looks good for me, Thank you.

git patch file (master) -- CI test; revised based upon the comments by William and Simon (Thanks!!)
0002-Ticket-548-CI-test-added-test-cases-for-ticket-548.patch

This looks good, one question.

Should these be switched:

2903    #if 0 
2904        /*  
2905         * shadowInactive - the number of days of inactivity allowed for the user. 
2906         * Password Policy does not have the corresponding parameter. 
2907         */ 
2908        shadowval = 0; 
2909        bv.bv_val = slapi_ch_smprintf("%ld", shadowval); 
2910        bv.bv_len = strlen(bv.bv_val); 
2911        slapi_entry_attr_replace(e, "shadowInactive", bvals); 
2912        slapi_ch_free_string(&bv.bv_val); 
2913    #endif 
2914     
2915        /* shadowFlag - not currently in use. */ 
2916        bv.bv_val = slapi_ch_smprintf("%d", 0); 
2917        bv.bv_len = strlen(bv.bv_val); 
2918        slapi_entry_attr_replace(e, "shadowFlag", bvals); 
2919        slapi_ch_free_string(&bv.bv_val); 
2920     
2921        LDAPDebug0Args(LDAP_DEBUG_TRACE, "<-- add_shadow_password_attrs\n"); 
2922        return; 
2923    }

If shadowFlag is not used, shouldn't that be inside the #if/#end block instead of shadowInactive? Just checking.

Replying to [comment:10 mreynolds]:

If shadowFlag is not used, shouldn't that be inside the #if/#end block instead of shadowInactive? Just checking.

Thanks, Mark. Yeah, you are right regarding shadowFlag. I thought it could be safer to set 0. But looking at the real case, the field is blank. So, there is not reason to set 0 to the flag. I'm updating the patch...
dirsrv:!!:12345::::::

About shadowInactive, It indicates the number of days of inactivity allowed for the user once the password is expired. I could not find the corresponding password policy attribute to auto-fill the value. Please note that you have no problem to set the value manually. The function add_shadow_ext_password_attrs fills some shadow values before sending the result to the client.

git patch file (master) -- revised following the comment by Mark (Thanks!)
0001-Ticket-548-RFE-Allow-AD-password-sync-to-update-shad.patch

Thank you for your comments, Mark. Revised. Could you please review it one more time? Thanks!

Thanks, ack

Thank you for reviewing the patch, Mark!

Pushed to master:
97946bd..cb209be master -> master
commit 17f3624
commit cb209be

Failure case:
1. Add 10k entries [0000 - 9999]
{{{
dn: uid=test####,dc=example,dc=com
objectClass: top
objectClass: person
objectClass: organizationalPerson
objectClass: inetOrgPerson
objectClass: posixAccount
objectClass: shadowAccount
cn: guest#
sn: guest#
uid: guest#
givenname: givenname#
description: description#
userPassword: password####
mail: uid#
uidnumber: #
gidnumber: #
shadowMin: 0
shadowMax: 99999
shadowInactive: 30
shadowWarning: 7
homeDirectory: /home/uid#
}}}

1. Configure subtree password policy, e.g., at the suffix level "dc=example,dc=com"

2. This command line crashes the server.
   {{{
   ldclt -h localhost -p 389 -e bindeach,bindonly -D uid=testXXXX,dc=example,dc=com -w passwordXXXX \
   -e randombinddn,randombinddnlow=0,randombinddnhigh=9999 -e esearch \
   -f "(&(objectClass=posixAccount)(uid=test11)(uidNumber=)(gidNumber=*))"
   }}}

git patch file (master) -- additional fix for the crash bug caused by ldclt.
0001-Ticket-548-RFE-Allow-AD-password-sync-to-update-shad.2.patch

Okay, I have spent a bit of time reviewing and testing this patch. I'm happy with it, as it does prevent a server deadlock. It also passes asan during the ldclt too.

Ack from me.

Reviewed by William (Thank you!!)

Pushed to master:
45c7165..dd90d19 master -> master
commit dd90d19

Thanks to William for his testing and input. I agree with him this scenario should be supported without removing shadowMin, shadowMax, etc. from the entry.

{{{
Should the values of ShadowMin, ShadowMax, etc be update on a password policy change?

I was testing 548 and trying to work out COS, and I noticed this:

Global PW policy:

userPassword::
e1NTSEF9MjgxM1BidGNYN05hMzlJQ1FiOUVRNDg5OVFUTDVXbTF6cDhlUlE9PQ=
=
shadowMin: 1
shadowMax: 10
shadowWarning: 3

Get cos and pwdpolicysubentry working

passwordMustChange: on
passwordExp: on
passwordMinAge: 172800
passwordMaxAge: 1728000
passwordWarning: 518400
passwordChange: on
passwordStorageScheme: clear

Change password

ldapvi -b dc=example,dc=com -D 'cn=Directory Manager' -h localhost '(uid=user3)' userPassword

Password is now changed

userPassword:: cGFzc3dvcmQ=
shadowMin: 1
shadowMax: 10
shadowWarning: 3

Delete shadow* and research:

userPassword:: cGFzc3dvcmQ=
shadowMin: 2
shadowMax: 20
shadowWarning: 6

So, should the shadow values be getting checked / updated on a password
change based on the fact the users PW policy may have changed in the
interim? If we are applying the new policy for the hashing algo, we
probably should be updating the shadow bits too ....
}}}

git patch file (master) -- additional fix for #comment:19
0001-Ticket-548-RFE-Allow-AD-password-sync-to-update-shad.3.patch

git patch file (master) -- additional test for #comment:19
0002-Ticket-548-CI-test-added-test-cases-for-ticket-548.2.patch

Updated test case that checks for shadow updates on pw change.
0001-Ticket-548-CI-test-added-test-cases-for-ticket-548.patch

I'm going to nack 0002-Ticket-548-CI-test-added-test-cases-for-ticket-548.2.patch in favor of the patch I just uploaded that covers more use cases.

I'm going to ack 0001-Ticket-548-RFE-Allow-AD-password-sync-to-update-shad.3.patch​ as this corrects the shadow* update issue that nhosoi and I discovered.

If nhosoi is happy with the CI test patch, I think it's good to commit.

Replying to [comment:21 firstyear]:

I'm going to nack 0002-Ticket-548-CI-test-added-test-cases-for-ticket-548.2.patch in favor of the patch I just uploaded that covers more use cases.

I'm going to ack 0001-Ticket-548-RFE-Allow-AD-password-sync-to-update-shad.3.patch​ as this corrects the shadow* update issue that nhosoi and I discovered.

If nhosoi is happy with the CI test patch, I think it's good to commit.

Yes, I'm happy with Attachment 0001-Ticket-548-CI-test-added-test-cases-for-ticket-548.patch​.

It successfully ran with the proposed patch. Please go ahead and push it.

Thanks, William!

commit 7a16ce9cfe291f75dae09b4e41f57b9836dc1a1f
commit 0a629c015fff4078a1f3d0c83363b9b7005fe5b1

Total 16 (delta 13), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
be8c063..097239c master -> master

Hello, Orion.

Sorry to ask this question after long time since you filed this ticket.

We finally implemented this shadow account support with a small addition, in which we are setting the shadow attributes such as shadowMax, shadowMin, etc. by calculating the values from the values in the password policy.

In the effort, this ticket was filed by a community member who is using the shadow account.
https://fedorahosted.org/389/ticket/48833

To solve this issue, we have 2 proposals.
1. If an entry already contains shadow attributes, they are not touched even if the values are not inconsistent with the ones in the password policy.
2. Even if an entry contains shadow attributes, make them in sync with the values from the password policy. But the default values of the password policy would be replaced by the ones to match the shadow account's default values. (For instance, 99999 days for the password max age.)

Do you have any preference? Your input would be greatly appreciated.

Well, after all this time, I've stopped using the shadowAccount attributes in 389. So I'm afraid I don't really have a preference.

Replying to [comment:25 orion]:

Well, after all this time, I've stopped using the shadowAccount attributes in 389. So I'm afraid I don't really have a preference.

Hello Orion,

I'm so happy to hear from you!

That's perfectly fine. We wanted to make sure our decision does not break your system.

Thanks!

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

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.