On 11/19/2012 04:59 PM, Pavel Březina wrote:
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.
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:
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@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
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:
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@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.
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:
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@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
On 11/28/2012 05:39 PM, Sumit Bose wrote:
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:
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@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
Thanks, I just tried it and it works as expected.
On Thu, Nov 29, 2012 at 12:55:27PM +0100, Pavel Březina wrote:
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
Thanks, I just tried it and it works as expected.
Thank you. Pushed to master and sssd-1-9
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Sumit Bose