This corresponds to internal bz:

https://bugzilla.redhat.com/show_bug.cgi?id=1464505

Hey @mreynolds you know this code better than I do. Could the fault be that we are not setting need_pw during grace logins? I think sending this back always isn't the logic we intend here.

My understanding of this issue is that in any case, when we are in grace periods, any grace login should return the control password expired, since it's exactly the case. In the code, we check that password is expired, then we want to see if it's a grace login. Only in that case we return to client side without error but the password was expired and we should send the control back to the client application.

@mreynolds Do you mind checking this patch for @gparente ?? Thanks mate :)

@gparente I've investigated your patch, and it is good. However, the way new_new_passwd() was used was complicated, and required the caller to add the controls. So I cleaned all of this up and simplified it. Here is the new patch:

Mark,

in fact, Thierry has shown in the password policy draft, that this control is not "password expired" but "password should be changed".

That changes completely semantics and I think it should not be sent during grace period.

I think this should be closed as "not a bug".

Regards,

German.

Mark,
in fact, Thierry has shown in the password policy draft, that this control is not "password expired" but "password should be changed".
That changes completely semantics and I think it should not be sent during grace period.
I think this should be closed as "not a bug".

https://tools.ietf.org/html/draft-vchu-ldap-pwd-policy-00

Fell free to close it then, but this also means that we have not been using it correctly for a long time (according to this draft). More importantly we never stated that we support that draft either.

Anyway, we don't comply with the draft as of today, so we either fix our current implementation to follow this draft (and potentially break existing clients that expect these controls), or we continue doing it our own way. In the later case I see no problem returning "our" expired control during the grace period.

Perhaps in DS 1.4.0 (RHEL 8) we can discuss this and look into if we should be complying with the draft. We should also consider what other ldap vendors do as well.

I agree with you mark. We are not committed to the ietf draft even if our current implement of this specific control is following it.

IMHO we need to document this control and potentially extend its use like @gparente proposed.
DS 1.4.0 looks good to me

I agree with you mark. We are not committed to the ietf draft even if our current implement of this specific control is following it.
IMHO we need to document this control and potentially extend its use like @gparente proposed.
DS 1.4.0 looks good to me

Sounds good, milestone adjusted. Perhaps the bugzilla should be closed in the mean time @gparente ?

Update: There is inconsistency in how the control is interpreted according to different sources:

http://www.alvestrand.no/objectid/2.16.840.1.113730.3.4.4.html

So I think the patch/fix is still valid, and adds value.

Please review patch...

@mreynolds . The fix looks good.
This is extending the meaning of that control and a doc bug should be opened.

This is mentioned in the deployment guide:

https://access.redhat.com/documentation/en-us/red_hat_directory_server/10/html/deployment_guide/Designing_a_Secure_Directory-Designing_a_Password_Policy#Password_Policy_Attributes-Grace_Login_Limit

It states that the password is "expired", but the login is allowed. So this enforces the idea that we should be sending the expired control. Also, there is no mention in any of our docs about password policy controls at all, so I will get a doc bug filed to cover this topic.

02d76b6..fbd32c4 master -> master

84109ee..5173add 389-ds-base-1.3.6 -> 389-ds-base-1.3.6

CI Test

Revised CI Test to add another test

I am okay with a code, so only minor issues:

We import docstrings to Polarion, so they should be in some exact format. There is an example:

118 +    """ Test for expiration control when password must be changed because an
119 +    admin reset the password

I am not sure if the additional whitespace in the beginning of the string will be processed correctly (and it probably won't look consistent)

122 +    :feature: Test the password policy expiration control after password reset

The :feature: is not mandatory field anymore, I've removed it from create_test.py.

123 +    :setup: Configure password policy

Setup usually contains the fixtures. Also you already have the :step: with "Configure password policy". So the :setup: will look like "Standalone instance, a user for testing"

124 +    :steps:
125 +        1. Configure password policy and reset password as admin
126 +        2. Bind, and check for expired control withthe proper error code "2"
127 +    :expectedresults:
128 +        1. The EXPIRED control is returned, and we the expected error code

For the Polarion we need 1-to-1 steps-expectedresults mapping. So it will look like:

124 +    :steps:
125 +        1. Configure password policy
125 +        2. Reset password as admin
126 +        3. Bind, and check for expired control with the proper error code "2"
127 +    :expectedresults:
128 +        1. Password policy should be configured successfully
128 +        2. Password should be successfully reset
129 +        3. The EXPIRED control is returned, and we have the expected error code

And one more thing. Could you please add more logging? I think we can use comments "# password expired" as the logs - "Wait 6 second for the password to be expired".

Thanks for the comments, new patch is attached.

Thanks!

CI Test

6d9e91f..d72a226 master -> master

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

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.