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.