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?= <pbrez...@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 > -- > 1.7.11.7 > > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
