Nir Soffer has posted comments on this change. Change subject: securable: refactor the securable implementation ......................................................................
Patch Set 6: (8 comments) http://gerrit.ovirt.org/#/c/22115/6/tests/main.py File tests/main.py: Line 36: Line 37: def __init__(self): Line 38: self.secured = False Line 39: Line 40: def __is_secure__(self): __issecured__ or __secured__ is more Pythonic here. If you go an define "special" method, make it look like other special methods. Line 41: return self.secured Line 42: Line 43: def securedMethod(self): Line 44: "securedMethod docstring" Line 59: pass Line 60: Line 61: def assertUnsecured(self, secureObject): Line 62: self.assertRaises(SecureError, secureObject.securedMethod) Line 63: secureObject.unsecuredMethod() I would separate this to two tests - one test for rasing when calling secure method, and other other for not raising when calling unsecure method. This way, when a test fails, you know exactly what is broken by the test name. Line 64: Line 65: def assertSecured(self, secureObject): Line 66: secureObject.securedMethod() Line 67: secureObject.unsecuredMethod() Line 83: self.assertUnsecured(secureObject) Line 84: Line 85: def testSecurityOverride(self): Line 86: secureObject = TestSecurable.MySecureClass() Line 87: secureObject.securedMethod(__securityOverride=True) I would simply reuse the same constant: securedObject.secureMethod(__secured__=True) Line 88: Line 89: def testDocstringWrapping(self): Line 90: secureObject = TestSecurable.MySecureClass() Line 91: http://gerrit.ovirt.org/#/c/22115/6/vdsm/storage/securable.py File vdsm/storage/securable.py: Line 21: from functools import wraps Line 22: Line 23: OVERRIDE_ARG = "__securityOverride" Line 24: SECURE_FIELD = "__secured__" Line 25: SECURE_METHOD_NAME = "__is_secure__" We can one constant for everything: SECURED = __secured__ Line 26: Line 27: Line 28: class SecureError(RuntimeError): Line 29: pass Line 44: Line 45: for name, value in cls.__dict__.iteritems(): Line 46: # Skipping non callable attributes, special methods (including Line 47: # SECURE_METHOD_NAME) and unsecured methods. Line 48: if (not callable(value) or not getattr(value, SECURE_FIELD, True) If we simplify the constants: getattr(value, SECURED, True) Line 49: or name.startswith("__")): Line 50: continue Line 51: setattr(cls, name, _secure_method(value)) Line 52: Line 58: Line 59: This decorator is used to mark the methods (of a @secured class) that Line 60: are not subject to the __is_secure__ execution check. Line 61: """ Line 62: setattr(f, SECURE_FIELD, False) If we simplify the constants: setattr(f, SECURED, False) Line 63: return f Line 64: Line 65: Line 66: def _secure_method(method): Line 65: Line 66: def _secure_method(method): Line 67: @wraps(method) Line 68: def wrapper(self, *args, **kwargs): Line 69: override = kwargs.pop(OVERRIDE_ARG, False) If we unify the constants: override = kwargs.pop(SECURED, False) Line 70: Line 71: if not (getattr(self, SECURE_METHOD_NAME)() is True Line 72: or override is True): Line 73: raise SecureError("Secured object is not in safe state") Line 69: override = kwargs.pop(OVERRIDE_ARG, False) Line 70: Line 71: if not (getattr(self, SECURE_METHOD_NAME)() is True Line 72: or override is True): Line 73: raise SecureError("Secured object is not in safe state") First, we don't want to test for True, we care about the truth-fullness of the value, not the type. Second, it is more clear to do this in two steps: is_secured = getattr(self, SECURED)() if not (is_secured or override): raise ... But why do we care about is_secured if we override the value? This explain better our intent: secured = kwargs.pop(SECURED, False) or getattr(self, SECURED)() if not secured: raise ... Line 74: Line 75: return method(self, *args, **kwargs) Line 76: -- To view, visit http://gerrit.ovirt.org/22115 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id5a8be7536748481746795e27701fbecc7c3318c Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
