Nir Soffer has uploaded a new change for review. Change subject: issci: Fix equality and hash when username or password are None ......................................................................
issci: Fix equality and hash when username or password are None When fetching iscsi session info, username of password can be None, but iscsi.ChapCredentials was not handling these values correctly in __eq__ and __hash__. In particular, we used to compare password.value and include it in the hash. This would raise AttributeError when password is None. Now all combinations are handled properly; we compare password objects, using ProtectedPassword.__eq__, and include hash(password), using ProtectedPassword.__hash__. This issue was hidden because practically username is None when password is None, and the equality test fail before we try to compare None.value. The hash issue is hidden because we never use __hash__ for ChapCredentials. So this is mainly a correctness fix that should not effect current code, but it may be needed for fixing the related bug. Change-Id: I95bda5d07506ecbfb76ec5397717fe05b53f97ec Relates-To: https://bugzilla.redhat.com/1279485 Signed-off-by: Nir Soffer <[email protected]> --- M lib/vdsm/password.py M tests/iscsiTests.py M vdsm/storage/iscsi.py 3 files changed, 46 insertions(+), 19 deletions(-) git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/80/48480/1 diff --git a/lib/vdsm/password.py b/lib/vdsm/password.py index 9e0756d..336bb44 100644 --- a/lib/vdsm/password.py +++ b/lib/vdsm/password.py @@ -39,6 +39,9 @@ def __repr__(self): return repr(str(self)) + def __hash__(self): + return hash((self.__class__, self.value)) + def protect_passwords(obj): """ diff --git a/tests/iscsiTests.py b/tests/iscsiTests.py index 5ef895a..43ff970 100644 --- a/tests/iscsiTests.py +++ b/tests/iscsiTests.py @@ -66,16 +66,22 @@ @expandPermutations class TestChapCredentialsEquality(TestCaseBase): - def test_eq_equal(self): - c1 = iscsi.ChapCredentials("username", ProtectedPassword("password")) - c2 = iscsi.ChapCredentials("username", ProtectedPassword("password")) + @permutations([ + (None, None), + (None, "password"), + ("username", None), + ("usernae", "password"), + ]) + def test_eq_equal(self, username, password): + c1 = iscsi.ChapCredentials(username, protected(password)) + c2 = iscsi.ChapCredentials(username, protected(password)) self.assertTrue(c1 == c2, "%s should equal %s" % (c1, c2)) def test_eq_subclass(self): class Subclass(iscsi.ChapCredentials): pass - c1 = iscsi.ChapCredentials("username", ProtectedPassword("password")) - c2 = Subclass("username", ProtectedPassword("password")) + c1 = iscsi.ChapCredentials("username", protected("password")) + c2 = Subclass("username", protected("password")) self.assertFalse(c1 == c2, "%s should not equal %s" % (c1, c2)) @permutations([ @@ -83,29 +89,41 @@ ("a", "b", "a", "a"), ]) def test_eq_different(self, user1, user2, pass1, pass2): - c1 = iscsi.ChapCredentials(user1, ProtectedPassword(pass1)) - c2 = iscsi.ChapCredentials(user2, ProtectedPassword(pass2)) + c1 = iscsi.ChapCredentials(user1, protected(pass1)) + c2 = iscsi.ChapCredentials(user2, protected(pass2)) self.assertFalse(c1 == c2, "%s should not equal %s" % (c1, c2)) - def test_ne_equal(self): - c1 = iscsi.ChapCredentials("username", ProtectedPassword("password")) - c2 = iscsi.ChapCredentials("username", ProtectedPassword("password")) + @permutations([ + (None, None), + (None, "password"), + ("username", None), + ("usernae", "password"), + ]) + def test_ne_equal(self, username, password): + c1 = iscsi.ChapCredentials(username, protected(password)) + c2 = iscsi.ChapCredentials(username, protected(password)) self.assertFalse(c1 != c2, "%s should equal %s" % (c1, c2)) @expandPermutations class TestChapCredentialsHash(TestCaseBase): - def test_equal_same_hash(self): - c1 = iscsi.ChapCredentials("username", ProtectedPassword("password")) - c2 = iscsi.ChapCredentials("username", ProtectedPassword("password")) + @permutations([ + (None, None), + (None, "password"), + ("username", None), + ("usernae", "password"), + ]) + def test_equal_same_hash(self, username, password): + c1 = iscsi.ChapCredentials(username, protected(password)) + c2 = iscsi.ChapCredentials(username, protected(password)) self.assertEqual(hash(c1), hash(c2)) def test_subclass_different_hash(self): class Subclass(iscsi.ChapCredentials): pass - c1 = iscsi.ChapCredentials("username", ProtectedPassword("password")) - c2 = Subclass("username", ProtectedPassword("password")) + c1 = iscsi.ChapCredentials("username", protected("password")) + c2 = Subclass("username", protected("password")) self.assertNotEqual(hash(c1), hash(c2)) @permutations([ @@ -113,6 +131,12 @@ ("a", "b", "a", "a"), ]) def test_not_equal_different_hash(self, user1, user2, pass1, pass2): - c1 = iscsi.ChapCredentials(user1, ProtectedPassword(pass1)) - c2 = iscsi.ChapCredentials(user2, ProtectedPassword(pass2)) + c1 = iscsi.ChapCredentials(user1, protected(pass1)) + c2 = iscsi.ChapCredentials(user2, protected(pass2)) self.assertNotEqual(hash(c1), hash(c2)) + + +def protected(password): + if password is None: + return None + return ProtectedPassword(password) diff --git a/vdsm/storage/iscsi.py b/vdsm/storage/iscsi.py index fce4e9b..58a88b7 100644 --- a/vdsm/storage/iscsi.py +++ b/vdsm/storage/iscsi.py @@ -302,13 +302,13 @@ def __eq__(self, other): return (self.__class__ == other.__class__ and self.username == other.username and - self.password.value == other.password.value) + self.password == other.password) def __ne__(self, other): return not self == other def __hash__(self): - return hash((self.__class__, self.username, self.password.value)) + return hash((self.__class__, self.username, hash(self.password))) # Technically there are a lot more interface properties but VDSM doesn't -- To view, visit https://gerrit.ovirt.org/48480 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I95bda5d07506ecbfb76ec5397717fe05b53f97ec Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
