From 1bb4ff9ed2313fb3c2bd1418258c5bcec557b6a5 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcritten@redhat.com>
Date: Thu, 21 Jul 2022 09:28:46 -0400
Subject: [PATCH] Disabling gracelimit does not prevent LDAP binds

Originally the code treated 0 as disabled. This was
changed during the review process to -1 but one remnant
was missed effetively allowing gracelimit 0 to also mean
disabled.

Add explicit tests for testing with gracelimit = 0 and
gracelimit = -1.

Also remove some extranous "str(self.master.domain.basedn)"
lines from some of the tests.

Fixes: https://pagure.io/freeipa/issue/9206

Signed-off-by: Rob Crittenden <rcritten@redhat.com>
Reviewed-By: Francisco Trivino <ftrivino@redhat.com>
---
 .../ipa-graceperiod/ipa_graceperiod.c         |  2 +-
 ipatests/test_integration/test_pwpolicy.py    | 55 ++++++++++++++++++-
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-graceperiod/ipa_graceperiod.c b/daemons/ipa-slapi-plugins/ipa-graceperiod/ipa_graceperiod.c
index a3f57cb4bd7a2a66d70fae98cca0f62a8f0c017f..345e1dee7d163167373ca82dedb1e827f0e1bc8c 100644
--- a/daemons/ipa-slapi-plugins/ipa-graceperiod/ipa_graceperiod.c
+++ b/daemons/ipa-slapi-plugins/ipa-graceperiod/ipa_graceperiod.c
@@ -479,7 +479,7 @@ static int ipagraceperiod_preop(Slapi_PBlock *pb)
         if (pwresponse_requested) {
             slapi_pwpolicy_make_response_control(pb, -1, grace_limit - grace_user_time , -1);
         }
-    } else if ((grace_limit > 0) && (grace_user_time >= grace_limit)) {
+    } else if (grace_user_time >= grace_limit) {
         LOG_TRACE("%s password is expired and out of grace limit\n", dn);
         errstr = "Password is expired.\n";
         ret = LDAP_INVALID_CREDENTIALS;
diff --git a/ipatests/test_integration/test_pwpolicy.py b/ipatests/test_integration/test_pwpolicy.py
index 6d66982848ac5a0061b47d30fad022be055c93e4..41d6e9070a90c2bde7b3182ad6ecf1a923bba203 100644
--- a/ipatests/test_integration/test_pwpolicy.py
+++ b/ipatests/test_integration/test_pwpolicy.py
@@ -36,7 +36,7 @@ class TestPWPolicy(IntegrationTest):
         cls.master.run_command(['ipa', 'group-add-member', POLICY,
                                 '--users', USER])
         cls.master.run_command(['ipa', 'pwpolicy-add', POLICY,
-                                '--priority', '1'])
+                                '--priority', '1', '--gracelimit', '-1'])
         cls.master.run_command(['ipa', 'passwd', USER],
                                stdin_text='{password}\n{password}\n'.format(
                                password=PASSWORD
@@ -265,7 +265,6 @@ class TestPWPolicy(IntegrationTest):
 
     def test_graceperiod_expired(self):
         """Test the LDAP bind grace period"""
-        str(self.master.domain.basedn)
         dn = "uid={user},cn=users,cn=accounts,{base_dn}".format(
              user=USER, base_dn=str(self.master.domain.basedn))
 
@@ -308,7 +307,6 @@ class TestPWPolicy(IntegrationTest):
 
     def test_graceperiod_not_replicated(self):
         """Test that the grace period is reset on password reset"""
-        str(self.master.domain.basedn)
         dn = "uid={user},cn=users,cn=accounts,{base_dn}".format(
              user=USER, base_dn=str(self.master.domain.basedn))
 
@@ -341,3 +339,54 @@ class TestPWPolicy(IntegrationTest):
         )
         assert 'passwordgraceusertime: 0' in result.stdout_text.lower()
         self.reset_password(self.master)
+
+    def test_graceperiod_zero(self):
+        """Test the LDAP bind with zero grace period"""
+        dn = "uid={user},cn=users,cn=accounts,{base_dn}".format(
+             user=USER, base_dn=str(self.master.domain.basedn))
+
+        self.master.run_command(
+            ["ipa", "pwpolicy-mod", POLICY, "--gracelimit", "0", ],
+        )
+
+        # Resetting the password will mark it as expired
+        self.reset_password(self.master)
+
+        # Now grace is done and binds should fail.
+        result = self.master.run_command(
+            ["ldapsearch", "-e", "ppolicy", "-D", dn,
+             "-w", PASSWORD, "-b", dn], raiseonerr=False
+        )
+        assert result.returncode == 49
+
+        assert 'Password is expired' in result.stderr_text
+        assert 'Password expired, 0 grace logins remain' in result.stderr_text
+
+    def test_graceperiod_disabled(self):
+        """Test the LDAP bind with grace period disabled (-1)"""
+        str(self.master.domain.basedn)
+        dn = "uid={user},cn=users,cn=accounts,{base_dn}".format(
+             user=USER, base_dn=str(self.master.domain.basedn))
+
+        # This can fail if gracelimit is already -1 so ignore it
+        self.master.run_command(
+            ["ipa", "pwpolicy-mod", POLICY, "--gracelimit", "-1",],
+            raiseonerr=False,
+        )
+
+        # Ensure the password is expired
+        self.reset_password(self.master)
+
+        result = self.kinit_as_user(self.master, PASSWORD, PASSWORD)
+
+        for _i in range(0, 10):
+            result = self.master.run_command(
+                ["ldapsearch", "-e", "ppolicy", "-D", dn,
+                 "-w", PASSWORD, "-b", dn]
+            )
+
+        # With graceperiod disabled it should not increment
+        result = tasks.ldapsearch_dm(
+            self.master, dn, ['passwordgraceusertime',],
+        )
+        assert 'passwordgraceusertime: 0' in result.stdout_text.lower()
-- 
2.37.2