ipatests: Check maxlife error message where minlife > maxlife specified #6098

mrizwan93 · 2021-11-22T08:36:11Z

When minlife > maxlife specified on commandline, it says:
"ipa: ERROR: invalid 'maxlife': Maximum password life must be
greater than minimum."

But when minlife == maxlife specfied, It works.
This test check that error message says what exactly it does.

related: https://pagure.io/freeipa/issue/9038

Signed-off-by: Mohammad Rizwan myusuf@redhat.com

mrizwan93 · 2021-11-22T08:37:19Z

depends on #6086

amore17 · 2021-11-22T09:43:37Z

ipatests/test_integration/test_pwpolicy.py

@@ -261,3 +261,35 @@ def test_minlength_add(self):
        )
        assert result.returncode != 0
        assert 'minlength' in result.stderr_text
+
+    def test_maxlife(self):
+        """Check maxlife error message where minlife > maxlife specified


If its checking only error text then i would suggest you to add this test
in test_xmlrpc/test_pwpolicy_plugin.py

+1 to that suggestion. Integration tests are much slower.

stanislavlevin · 2021-11-30T07:02:41Z

ipatests/test_xmlrpc/test_pwpolicy_plugin.py

+        try:
+            entry = api.Command['pwpolicy_mod'](krbminpwdlife=25,
+                                                krbmaxpwdlife=1)['result']
+        except errors.ValidationError as e:
+            assert ("Maximum password life must be equal to "
+                    "or greater than the minimum.") in str(e)


Is it an expected mandatory exception?
https://docs.pytest.org/en/latest/how-to/assert.html#assertions-about-expected-exceptions

Yes, see https://github.com/freeipa/freeipa/pull/6086/files#diff-fcbdf956172aafe4f6c1e64b54988af95c6bd35770c9a1e83e056e72e3114d21R492

let me rephrase myself.
if this is an expected exception then it should be raised always. Currently, the test assumes that the exception is optional. For example, try except else construction or Pytest's pytest.raises (the link above) can be used.

Missed your point :)
Updated.

example:

freeipa/ipatests/test_xmlrpc/test_plugins_plugin.py

Lines 56 to 60 in 637412a

with pytest.raises(errors.OptionError) as e:

self.run_plugins(

nonexistentoption="nonexistentoption", server=server

)

assert "Unknown option: nonexistentoption" in str(e.value)

mrizwan93 · 2021-12-07T08:11:53Z

@stanislavlevin @rcritten Could you look at this please?

stanislavlevin · 2021-12-07T08:50:54Z

Hi, @mrizwan93.

I think it's a good idea to extend your tests to check validate_lifetime of ipaserver/plugins/pwpolicy.py, not only the one case (pwpolicy_mod). pwpolicy_add is affected as well
validate_lifetime for pwpolicy_mod has additional conditions. Please, cover them with tests.

mrizwan93 · 2022-02-15T09:12:37Z

/azp run

azure-pipelines · 2022-02-15T09:12:50Z

Azure Pipelines successfully started running 1 pipeline(s).

mrizwan93 · 2022-02-22T10:21:50Z

/azp run

azure-pipelines · 2022-02-22T10:22:03Z

Azure Pipelines successfully started running 1 pipeline(s).

flo-renaud

Hi @mrizwan93
please find an inline comment

flo-renaud · 2022-03-10T15:40:09Z

ipatests/test_xmlrpc/test_pwpolicy_plugin.py

+
+        # when minlife is -1
+        pwpolicy_cmd('pwpolicy_mod', -1)
+        pwpolicy_cmd('pwpolicy_add', -1, test_group101, pwpolicyadd=True)


The cospriority argument is missing: the method expects cmd, minlife, cospriority=None, pwpolicy_group=None, pwpolicyadd=False.

flo-renaud · 2022-03-11T08:52:56Z

ipatests/test_xmlrpc/test_pwpolicy_plugin.py

@@ -186,28 +282,32 @@ def test_b_pwpolicy_add(self):
    def test_c_pwpolicy_find(self):
        """Test that password policies are sorted and reported properly"""
        result = api.Command['pwpolicy_find']()['result']
-        assert len(result) == 4
+        assert len(result) == 6


Instead of modifying test_c_pwpolicy_find and test_c_pwpolicy_find_pkey_only because there are additional policies, you can simply delete the pwpolicies created in test_maxlife_pwpolicy at the end of test_maxlife_pwpolicy.

flo-renaud · 2022-03-14T09:12:37Z

ipatests/test_xmlrpc/test_pwpolicy_plugin.py

@@ -31,6 +31,49 @@
                                              Declarative)


+def pwpolicy_cmd(
+        cmd, minlife, cospriority=None,
+        pwpolicy_group=None, pwpolicyadd=False):


The method arguments are redundant: there is no need for pwpolicyadd=True/False as the cmd arg already defines either add or mod. I would remove the pwpolicyadd arg and replace if pwpolicyadd: with if cmd == "pwpolicy_add":

flo-renaud · 2022-03-14T09:17:05Z

ipatests/test_xmlrpc/test_pwpolicy_plugin.py

+def pwpolicy_cmd(
+        cmd, minlife, cospriority=None,
+        pwpolicy_group=None, pwpolicyadd=False):
+    if pwpolicyadd:


Add a method docstring: either create a new pwpolicy with max life = 1 day or modify the existing global pwpolicy.

flo-renaud

Hi @mrizwan93
I only have minor nitpicks, please find them inline.

flo-renaud · 2022-03-15T09:04:32Z

ipatests/test_xmlrpc/test_pwpolicy_plugin.py

+    """Helper method to add or modify the password policy
+
+    :param cmd: Either pwpolicy_add or pwpolicy_mod
+    :pram minlife: minimum life for the password to exist


typo pram
minlife: The minimum amount of time (in hours) that must pass between two password change operations.

flo-renaud · 2022-03-15T09:05:48Z

ipatests/test_xmlrpc/test_pwpolicy_plugin.py

+        cmd, minlife, cospriority=None,
+        pwpolicy_group=None):
+    """Helper method to add or modify the password policy
+


The add method harcodes the max life to 1 day.

When minlife > maxlife specified on commandline, it says: "ipa: ERROR: invalid 'maxlife': Maximum password life must be greater than minimum." But when minlife == maxlife specfied, It works. This test check that error message says what exactly it does related: https://pagure.io/freeipa/issue/9038 Signed-off-by: Mohammad Rizwan <myusuf@redhat.com>

mrizwan93 · 2022-03-15T11:38:02Z

/azp run

azure-pipelines · 2022-03-15T11:38:14Z

Azure Pipelines successfully started running 1 pipeline(s).

abbra · 2022-03-16T10:17:45Z

master:

5a909cf ipatests: Check maxlife error message where minlife > maxlife specified

mrizwan93 added the WIP Work in progress - not ready yet for review label Nov 22, 2021

mrizwan93 added ipa-4-9 Mark for backport to ipa 4.9 and removed WIP Work in progress - not ready yet for review labels Nov 22, 2021

mrizwan93 force-pushed the maxlife-test-6086 branch from 1fe6803 to cb585e5 Compare November 22, 2021 09:05

mrizwan93 added the needs review Pull Request is waiting for a review label Nov 22, 2021

amore17 reviewed Nov 22, 2021

View reviewed changes

mrizwan93 force-pushed the maxlife-test-6086 branch from cb585e5 to 80637c9 Compare November 22, 2021 10:42

mrizwan93 force-pushed the maxlife-test-6086 branch 3 times, most recently from 5ea4bc9 to 0f6d4a3 Compare November 30, 2021 06:39

stanislavlevin reviewed Nov 30, 2021

View reviewed changes

mrizwan93 force-pushed the maxlife-test-6086 branch 2 times, most recently from e5c9396 to b8224fe Compare December 1, 2021 09:45

mrizwan93 force-pushed the maxlife-test-6086 branch 9 times, most recently from 4994191 to 5b1ed80 Compare February 1, 2022 13:34

mrizwan93 force-pushed the maxlife-test-6086 branch 2 times, most recently from 5848671 to f8d4bee Compare February 15, 2022 04:23

mrizwan93 force-pushed the maxlife-test-6086 branch 7 times, most recently from a620feb to 9939cca Compare February 23, 2022 13:31

flo-renaud reviewed Mar 10, 2022

View reviewed changes

mrizwan93 force-pushed the maxlife-test-6086 branch 2 times, most recently from 5519ca8 to eb75cf2 Compare March 11, 2022 06:01

flo-renaud reviewed Mar 11, 2022

View reviewed changes

flo-renaud self-assigned this Mar 11, 2022

mrizwan93 force-pushed the maxlife-test-6086 branch from eb75cf2 to 48fba47 Compare March 11, 2022 11:04

flo-renaud reviewed Mar 14, 2022

View reviewed changes

mrizwan93 force-pushed the maxlife-test-6086 branch 2 times, most recently from dee6722 to 8edfeb6 Compare March 14, 2022 09:43

flo-renaud reviewed Mar 15, 2022

View reviewed changes

mrizwan93 force-pushed the maxlife-test-6086 branch 2 times, most recently from d6e8f46 to cc61243 Compare March 15, 2022 09:52

mrizwan93 force-pushed the maxlife-test-6086 branch from cc61243 to 0026f65 Compare March 15, 2022 09:53

flo-renaud added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Mar 15, 2022

abbra added the pushed Pull Request has already been pushed label Mar 16, 2022

abbra closed this Mar 16, 2022

abbra mentioned this pull request Mar 16, 2022

[Backport][ipa-4-9] ipatests: Check maxlife error message where minlife > maxlife specified #6215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipatests: Check maxlife error message where minlife > maxlife specified #6098

ipatests: Check maxlife error message where minlife > maxlife specified #6098

mrizwan93 commented Nov 22, 2021

mrizwan93 commented Nov 22, 2021

amore17 Nov 22, 2021

fcami Nov 22, 2021

rcritten Dec 1, 2021

stanislavlevin Nov 30, 2021

mrizwan93 Nov 30, 2021

stanislavlevin Nov 30, 2021 •

edited

mrizwan93 Nov 30, 2021

stanislavlevin Nov 30, 2021

mrizwan93 commented Dec 7, 2021

stanislavlevin commented Dec 7, 2021

mrizwan93 commented Feb 15, 2022

azure-pipelines bot commented Feb 15, 2022

mrizwan93 commented Feb 22, 2022

azure-pipelines bot commented Feb 22, 2022

flo-renaud left a comment

flo-renaud Mar 10, 2022

flo-renaud Mar 11, 2022

flo-renaud Mar 14, 2022

flo-renaud Mar 14, 2022

flo-renaud left a comment

flo-renaud Mar 15, 2022

flo-renaud Mar 15, 2022

mrizwan93 commented Mar 15, 2022

azure-pipelines bot commented Mar 15, 2022

abbra commented Mar 16, 2022

	with pytest.raises(errors.OptionError) as e:
	self.run_plugins(
	nonexistentoption="nonexistentoption", server=server
	)
	assert "Unknown option: nonexistentoption" in str(e.value)

ipatests: Check maxlife error message where minlife > maxlife specified #6098

ipatests: Check maxlife error message where minlife > maxlife specified #6098

Conversation

mrizwan93 commented Nov 22, 2021

mrizwan93 commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanislavlevin Nov 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrizwan93 commented Dec 7, 2021

stanislavlevin commented Dec 7, 2021

mrizwan93 commented Feb 15, 2022

azure-pipelines bot commented Feb 15, 2022

mrizwan93 commented Feb 22, 2022

azure-pipelines bot commented Feb 22, 2022

flo-renaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flo-renaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrizwan93 commented Mar 15, 2022

azure-pipelines bot commented Mar 15, 2022

abbra commented Mar 16, 2022

stanislavlevin Nov 30, 2021 •

edited