Attach response controls to exceptions #251

somay · 2018-11-16T08:08:06Z

Fixes: #208

exception.args[0]['ctrls'] is a list of the form of (oid: str, criticality: int, berval: bytes).

I've choose ctrls instead of controls as the key of the dictionary because the other keys are also abbreviated like desc, info.

codecov · 2018-11-16T08:15:10Z

Codecov Report

Merging #251 into master will increase coverage by 13.16%.
The diff coverage is 61.9%.

@@             Coverage Diff             @@
##           master     #251       +/-   ##
===========================================
+ Coverage   57.91%   71.08%   +13.16%     
===========================================
  Files          49       49               
  Lines        5910     4824     -1086     
  Branches      815      815               
===========================================
+ Hits         3423     3429        +6     
+ Misses       2157     1064     -1093     
- Partials      330      331        +1

Impacted Files	Coverage Δ
Lib/slapdtest/_slapdtest.py	`85.61% <100%> (+27.5%)`	⬆️
Modules/constants.c	`48.68% <100%> (+2.85%)`	⬆️
Modules/LDAPObject.c	`66.93% <33.33%> (+0.22%)`	⬆️
Lib/ldap/modlist.py	`84.61% <0%> (+1.59%)`	⬆️
Lib/ldap/asyncsearch.py	`28.82% <0%> (+4.4%)`	⬆️
Lib/ldap/controls/sessiontrack.py	`65.21% <0%> (+5.21%)`	⬆️
Lib/ldap/schema/tokenizer.py	`97.87% <0%> (+5.87%)`	⬆️
Lib/ldap/schema/models.py	`71.07% <0%> (+6.46%)`	⬆️
Lib/ldap/cidict.py	`69.23% <0%> (+7.16%)`	⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ce471e...9ed502d. Read the comment docs.

encukou · 2018-11-20T13:18:51Z

Lib/slapdtest/_slapdtest.py

+    :py:class:`PPolicyEnabledSlapdObject` as the slapd controller.
+    """
+
+    server_class = PPolicyEnabledSlapdObject


Is this necessary? PPolicyEnabledSlapdTestCase needs to be subclassed; the subclass can set its server_class directly.

encukou · 2018-11-20T13:21:28Z

Lib/slapdtest/_slapdtest.py

@@ -596,3 +623,43 @@ def setUpClass(cls):
    @classmethod
    def tearDownClass(cls):
        cls.server.stop()
+
+
+class PPolicyEnabledSlapdObject(SlapdObject):


Do you think this class will be needed for more than just one test? If not, would it be better to define the class just in the file for that test?

encukou · 2018-11-20T13:22:53Z

Lib/slapdtest/_slapdtest.py

@@ -187,6 +191,9 @@ class SlapdObject(object):
        'core.schema',
    )

+    modules = ()
+    overlays = ()


Since these attributes can be overridden in subclasses, they should be documented.

encukou · 2018-11-20T13:25:47Z

Lib/slapdtest/_slapdtest.py

+        ldap_conn.simple_bind_s(who or self.server.root_dn, cred or self.server.root_pw)
+        return ldap_conn
+
+    def _make_ldap_object(self, **kwargs):


If subclasses are supposed to call this, it should be documented.
Perhaps it would be better to add an overridable bind=True class attribute?

encukou · 2018-11-20T13:28:55Z

Modules/constants.c

@@ -104,6 +104,11 @@ LDAPerror(LDAP *l, char *msg)
            ldap_memfree(matched);
        }

+        if (pyctrls != NULL) {
+            PyDict_SetItemString(info, "ctrls", pyctrls);
+            /* Py_XDECREF(pyctrls) must be called on caller side */


Needing to call Py_XDECREF(pyctrls) is implied by Python's calling convention – it's a borrowed reference. No need to point it out here.

encukou · 2018-11-20T13:33:31Z

Tests/t_ldapobject.py

+
+        controls = cm.exception.args[0]['ctrls']
+        pp = ldap.controls.DecodeControlTuples(controls)[0]
+        self.assertEqual(pp.error, 1)  # error == 1 means AccountLockout


Should we have a constant for AccountLockout somewhere?

Could you also check len(ldap.controls.DecodeControlTuples(controls))?

encukou · 2018-11-20T13:36:56Z

Tests/t_ldapobject.py

+        # Firstly cause a bind failure to lock out the account
+        with self.assertRaises(ldap.INVALID_CREDENTIALS):
+            wrong_password = 'wrong' + password
+            ldap_conn.simple_bind_s(user_dn, wrong_password)


Could you also check here that ctrls is empty?

checked.
Thank you for quick and detailed review.

somay · 2018-11-22T08:19:38Z

I haven't finished yet.
I'll comment explicitly when I'm ready for your review.

mistotebe · 2019-05-02T10:17:34Z

I have started related work on this and #177 in https://github.com/mistotebe/python-ldap/tree/exceptions, @somay could you have a look and see if these two could be reconciled (I'm happy to do that)?

tiran · 2019-05-02T12:10:54Z

Petr and I are at PyCon until end of next week. It may take two weeks until we are able to look into your PR.

I noticed some unrelated improvements like a memory leak fix and some changes to slapdtest. Could you please file separate bugs and PRs for these? Thanks

mistotebe · 2019-05-02T14:50:46Z

@tiran sure, hope you're having fun, either of you attending this year's LDAPCon?

The linked branch is a work in progress and I've been rewriting it along the way, I guess I can split things at some point when they've settled.

Also, do you think you'll have some time to moderate the email I sent to the mailing list?

somay · 2019-05-21T01:45:32Z

@mistotebe sorry for long inactivity.
I'm also grateful if you would handle this issue in addition to your one.
Actually, I have little time to finish this PR (sorry) because I will quit my current LDAP related job soon.
Feel free to reuse my code modification and documentation.
I will try to review your draft in this week, although the scope of your PR seems much wider than mine.

encukou · 2020-06-05T19:25:06Z

Thanks to @mistotebe's work in #288, response controls will be available under ctrls.

Attach response controls to exceptions

57ed6ae

encukou reviewed Nov 20, 2018

View reviewed changes

SOMA Yuki added 4 commits November 21, 2018 15:04

remove PPolicyEnabledSlapdTestCase

c76f087

avoid to export PPolicyEnabledSlapdObject

cc7ef3b

trim a redundant comment

0144bdb

Test: assert len(ctrls) & remove a magic number

a0ba399

somay force-pushed the issue208-controls-in-failed-responses branch from acafa3a to a0ba399 Compare November 21, 2018 07:13

SOMA Yuki added 4 commits November 21, 2018 16:56

Doc: delete about slapdtest.PPolicyEnabled...

2b5d1ee

Doc: slapdtest: modules and overlay attributes

a616cae

delete _make_ldap_object()

e8648f2

Test: check also empty control list

9ed502d

encukou added the wip label Jan 7, 2019

encukou added the help wanted label Jan 29, 2020

encukou closed this Jun 5, 2020

tiran added this to the 3.3 milestone Jun 5, 2020

Attach response controls to exceptions #251

Attach response controls to exceptions #251

Uh oh!

Conversation

somay commented Nov 16, 2018

Uh oh!

codecov bot commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somay commented Nov 22, 2018

Uh oh!

mistotebe commented May 2, 2019

Uh oh!

tiran commented May 2, 2019

Uh oh!

mistotebe commented May 2, 2019

Uh oh!

somay commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

encukou commented Jun 5, 2020

Uh oh!

Uh oh!

codecov bot commented Nov 16, 2018 •

edited

Loading

somay commented May 21, 2019 •

edited

Loading