On Wed, Nov 28, 2012 at 04:54:45PM +0100, Pavel Březina wrote:
On 11/28/2012 11:15 AM, Sumit Bose wrote:
>On Tue, Nov 20, 2012 at 11:18:22AM +0100, Pavel Březina wrote:
>>On 11/19/2012 04:59 PM, Pavel Březina wrote:
>>>https://fedorahosted.org/sssd/ticket/1638
>>
>>Self nack.
>>
>>>- if (pwd_exp_warning == 0 ||
>>>- difftime(now + pwd_exp_warning, ppolicy->expire) >
0.0) {
>>>+ if (pwd_exp_warning != 0 &&
>>>+ difftime(pwd_exp_warning, ppolicy->expire) > 0.0) {
>>> goto done;
>>> }
>>
>>We don't need to test if pwd_exp_warning != 0, because if it is 0 then
>>difftime returns value <= 0 and the condition fails.
>>
>>New patch is attached.
>>
>
>> From 3bb970535dd1d12016ed8ba431039b843be9da7c Mon Sep 17 00:00:00 2001
>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>Date: Mon, 19 Nov 2012 16:52:36 +0100
>>Subject: [PATCH] warn user if password is about to expire
>>
>>https://fedorahosted.org/sssd/ticket/1638
>>
>>If pwd_exp_warning == 0, expiry warning should be printed if it is
>>returned by server.
>>
>>If pwd_exp_warning > 0, expiry warning should be printed only if
>>the password will expire in time <= pwd_exp_warning.
>>
>>ppolicy->expiry contains period in seconds after which the password
>>expires. Not the exact timestamp. Thus we should not add 'now' to
>>pwd_exp_warning.
>>---
>> src/providers/ldap/ldap_auth.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>>diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
>>index
32a2e04ea959a3cc81b88f5b2b19575c813e8adf..04fd24ffa4708dce53fb937e92efbbc102bef97c 100644
>>--- a/src/providers/ldap/ldap_auth.c
>>+++ b/src/providers/ldap/ldap_auth.c
>>@@ -212,7 +212,6 @@ static errno_t check_pwexpire_ldap(struct pam_data *pd,
>> if (ppolicy->grace > 0 || ppolicy->expire > 0) {
>> uint32_t *data;
>> uint32_t *ptr;
>>- time_t now = time(NULL);
>> int ret;
>>
>> if (pwd_exp_warning < 0) {
>>@@ -231,8 +230,7 @@ static errno_t check_pwexpire_ldap(struct pam_data *pd,
>> ptr++;
>> *ptr = ppolicy->grace;
>> } else if (ppolicy->expire > 0) {
>>- if (pwd_exp_warning == 0 ||
>>- difftime(now + pwd_exp_warning, ppolicy->expire) > 0.0) {
>>+ if (difftime(pwd_exp_warning, ppolicy->expire) > 0.0) {
>> goto done;
>> }
>> *ptr = SSS_PAM_USER_INFO_EXPIRE_WARN;
>
>I haven't tested the patch yet, but I would recommend to drop difftime()
>because pwd_exp_warning and ppolicy->expire are integers and just using
>'<' or '>' would be easier and better to read.
>
>That said I think the current logic is flawed. As you said of
>pwd_exp_warning is 0 difftime returns value <= 0 and the expire time is
>added to the response. But if pwd_exp_warning is positive
>ppolicy->expire must be larger than pwd_exp_warning to be added to the
>response. This means the warning is only printed if it is still longer
>than the time given in pwd_expiration_warning.
>
>bye,
>Sumit
You are correct sir. Thank you. Looks like I forgot to test the option.
I'm sending a new patch. I don't have time to test it myself right now,
but I don't want to delay 1.9.3 if with any luck it is going to be
released today.
I'll try it tomorrow if it's still pending, though I'm quite
confident it will work.
Patch looks good, applies to master and compiles without warning. I
didn't had a chance to test it either so I'll give a tentative ACK to
not delay 1.9.3. I'll try to do some testing later today.
bye,
Sumit