Pureldap and pureber repr fixes #148

GrayAn · 2019-05-13T09:06:24Z

pureldap and pureber minor fixes (mostly string representations):

__repr__ method encoding fixes like showing LDAPResult(resultCode=0, matchedDN='uid=user') instead of LDAPResult(resultCode=0, matchedDN="b'uid=user'");
showing userIdentity, oldPasswd and newPasswd in the LDAPPasswordModifyRequest.__repr__;
LDAPMatchingRuleAssertion.__init__ is now properly initialized with bytes attributes in Python 3;
LDAPExtendedResponse custom tag is now used.

Showing passwords in the LDAPPasswordModifyRequest representation may sound not good but LDAPBindRequest was implemented in the same way.

Tests for __repr__ methods were added to avoid test coverage issues.

LDAPMatchingRuleAssertion and LDAPExtendedResponse fixes are not actually __repr__ fixes but they are rather trivial. Is it OK to have them in this PR?

Contributor Checklist:

I have updated the release notes at docs/source/NEWS.rst
I have updated the automated tests.
All tests pass on your local dev environment. See CONTRIBUTING.rst.

* __repr__ method encoding fixes like showing LDAPResult(resultCode=0, matchedDN='uid=user') instead of LDAPResult(resultCode=0, matchedDN="b'uid=user'"); * showing userIdentity, oldPasswd and newPasswd in the LDAPPasswordModifyRequest.__repr__; * LDAPMatchingRuleAssertion.__init__ is now properly initialized with bytes attributes in Python 3; * LDAPExtendedResponse custom tag is now used.

codecov · 2019-05-13T09:10:56Z

Codecov Report

Merging #148 into master will increase coverage by 0.69%.
The diff coverage is 99.3%.

@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
+ Coverage   92.42%   93.11%   +0.69%     
==========================================
  Files          72       72              
  Lines        9663     9924     +261     
  Branches      929      976      +47     
==========================================
+ Hits         8931     9241     +310     
+ Misses        590      554      -36     
+ Partials      142      129      -13

Impacted Files	Coverage Δ
ldaptor/_encoder.py	`89.18% <100%> (+0.61%)`	⬆️
ldaptor/test/test_pureldap.py	`99.74% <100%> (+0.4%)`	⬆️
ldaptor/protocols/pureber.py	`88.98% <100%> (+0.04%)`	⬆️
ldaptor/protocols/pureldap.py	`94.57% <95%> (+6.07%)`	⬆️

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 ebef037...ab76a87. Read the comment docs.

adiroiban · 2019-05-29T13:12:47Z

not sure why I have not received notifications for this PR.... is this ready for review?

GrayAn · 2019-05-29T13:17:24Z

Yes, I think. Though I don't know what caused codecov errors this time.

adiroiban · 2019-05-29T14:25:03Z

Hi....codecov is failing due to branch coverage... but the HTML view for codecov is not that nice

https://codecov.io/gh/twisted/ldaptor/compare/ebef037bccdaa530de94b6e2afbfda1bd605491a...806ee265a2002c5446e9360dfd8bb57709b372d6/changes#D1-1059

If you run the test locally, you can check build/coverage-html and it should give you more hints about branch coverage, and which branch is not covered

.... but I don't know why test_pureldap.py is not reported as 100%

https://codecov.io/gh/twisted/ldaptor/compare/ebef037bccdaa530de94b6e2afbfda1bd605491a...806ee265a2002c5446e9360dfd8bb57709b372d6/tree/ldaptor/test

the idea is that for the code from tests we want a hard 100% coverage to make sure all the tests are executed and no test is always skipped.

I will take a look at the coverage

adiroiban

Looks good. Only minor comments.

Sorry, I don't have much time to review this.

I don't really use repr and as long as toWire works I think that we can be relaxed in terms of repr.

When I use repr, I want to see the instance id :)
And now that we have toWire maybe we can add id value to repr... but can be done in a later PR

Thanks!

docs/source/NEWS.rst

ldaptor/protocols/pureldap.py

adiroiban · 2019-06-16T08:25:10Z

ldaptor/protocols/pureldap.py

+        if self.oldPasswd is not None:
+            l.append('oldPasswd={}'.format(repr(self.oldPasswd)))
+        if self.newPasswd is not None:
+            l.append('newPasswd={}'.format(repr(self.newPasswd)))


I don't know why this is TODO... but I think than unless there is an explicit request, is better to leave the password outside of the repr... is ok to have userIdentity... but maybe use a private member self._userIdentity

GrayAn · 2019-06-18T15:24:57Z

Great, thank you for the review! I am going to change the __repr__ method for password containing classes and add a repr converter helper. It will take some time though, I will return to my PC in the end of the month :)

now show passwords as asterisks

Python version

GrayAn · 2019-07-01T07:29:38Z

Greetings! Fixed the issues with the password representations and repr_converter. Is it ok?

adiroiban

Looks good. Thanks.
Only minor comments.

adiroiban · 2019-07-01T07:51:47Z

ldaptor/protocols/pureldap.py

        l = []
        l.append('version=%d' % self.version)
-        l.append('dn=%s' % repr(converter(self.dn)))
-        l.append('auth=%s' % repr(converter(self.auth)))
+        l.append('dn=%s' % repr(repr_converter(self.dn)))


sorry for spotting this only now.

but shouldn't rep(self.dn) already return a repr converted value? ... otherwise just use l.append('dn=%s' % repr_converter(self.dn)) as repr_converter should return the same value as repr.

Not in all cases unfortunately. In Python 3 it may look as dn="b'uid=user'" if dn was specified as byte string. repr_converter is used to make it look like dn="uid=user".
repr function is also needed, without it the result may look like dn=uid=user.

omg :) ok. thanks for the comment
this py3/py2 thing is ugly.

I think that this can be merge as it is and have a release

adiroiban · 2019-07-01T07:53:42Z

ldaptor/protocols/pureldap.py

-class LDAPPasswordModifyRequest_oldPasswd(BEROctetString):
+class LDAPPasswordModifyRequest_passwd(BEROctetString):
+    def __repr__(self):
+        value = '*' * len(self.value)


Minor comment:
For security freaks.... just use a single asterisks not to reveal the length of the password and empty if password was not set.

But I am ok with this implementation.. better than plain text :)

adiroiban · 2019-07-01T08:05:33Z

codecov/project/tests look ok... it reports a missing empty line https://codecov.io/gh/twisted/ldaptor/compare/ebef037bccdaa530de94b6e2afbfda1bd605491a...ab76a873dc2f7370edc696aa68a8c6030dafaea5/src/ldaptor/test/test_pureldap.py?before=ldaptor/test/test_pureldap.py#L33

looks like a bug in coverage or codecov

GrayAn · 2019-07-01T11:53:34Z

Thank you! Can you help me with the last problem? This PR cannot be merged automatically so GitHub suggested to make it via command line. The last line in the given instructions is git push origin master. This action cannot be performed as this branch is protected:

remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: 2 of 4 required status checks are failing.
To github.com:twisted/ldaptor.git
 ! [remote rejected] master -> master (protected branch hook declined)

How to properly merge it to master?

adiroiban · 2019-07-01T14:37:44Z

Done... by using my admin powers... but I have also used the to disable the requirement on code coverage.

GrayAn · 2019-07-01T14:43:33Z

Thanks! A possible release has been discussed in the old pull request. Maybe the time has come?

psi29a · 2019-07-04T15:11:03Z

@GrayAn if you or @adiroiban can put the effort into making a PR for a new release, I'd be happy to tag and upload to pypi

GrayAn · 2019-07-08T07:11:09Z

@psi29a I'm not familiar with the release process but I found these pull requests (140 and 141). Do I need to just update the package version in the init.py and NEWS.rst files?

adiroiban · 2019-07-08T09:12:53Z

yes. a new PR is required to update the version and the release notes in NEWS.rst

maybe we can also set up automatic pypi releases from travis like (https://github.com/twisted/twistedchecker/pull/108/files) but if @psi29a is ok with manual upload. that is not required.

Thanks!

adiroiban approved these changes Jun 16, 2019

View reviewed changes

GrayAn added 2 commits July 1, 2019 10:04

LDAPBindRequest and LDAPPasswordModifyRequest string representations

d529144

now show passwords as asterisks

repr_converter helper for choosing proper string type for the

ab76a87

Python version

adiroiban approved these changes Jul 1, 2019

View reviewed changes

adiroiban merged commit 6ed55a3 into twisted:master Jul 1, 2019

GrayAn deleted the repr branch July 1, 2019 14:39

kumar35959 mentioned this pull request Jul 3, 2019

Do not use unicode globally, as some upstream tests does not support unicode foxpass/ldaptor#5

Merged

kumar35959 added a commit to foxpass/ldaptor that referenced this pull request Jul 3, 2019

Port twisted#148 to foxpass/ldaptor

32cc431

kumar35959 mentioned this pull request Jul 3, 2019

Port https://github.com/twisted/ldaptor/pull/148 to foxpass/ldaptor foxpass/ldaptor#6

Merged

aren pushed a commit to foxpass/ldaptor that referenced this pull request Jul 3, 2019

Port twisted#148 to foxpass/ldaptor (#6)

9f2f78d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pureldap and pureber repr fixes #148

Pureldap and pureber repr fixes #148

GrayAn commented May 13, 2019

codecov bot commented May 13, 2019 •

edited

Loading

adiroiban commented May 29, 2019

GrayAn commented May 29, 2019

adiroiban commented May 29, 2019

adiroiban left a comment •

edited

Loading

adiroiban Jun 16, 2019

GrayAn commented Jun 18, 2019

GrayAn commented Jul 1, 2019

adiroiban left a comment

adiroiban Jul 1, 2019

GrayAn Jul 1, 2019

adiroiban Jul 1, 2019

adiroiban Jul 1, 2019

adiroiban commented Jul 1, 2019

GrayAn commented Jul 1, 2019

adiroiban commented Jul 1, 2019

GrayAn commented Jul 1, 2019

psi29a commented Jul 4, 2019

GrayAn commented Jul 8, 2019

adiroiban commented Jul 8, 2019

Pureldap and pureber repr fixes #148

Pureldap and pureber repr fixes #148

Conversation

GrayAn commented May 13, 2019

Contributor Checklist:

codecov bot commented May 13, 2019 • edited Loading

Codecov Report

adiroiban commented May 29, 2019

GrayAn commented May 29, 2019

adiroiban commented May 29, 2019

adiroiban left a comment • edited Loading

Choose a reason for hiding this comment

adiroiban Jun 16, 2019

Choose a reason for hiding this comment

GrayAn commented Jun 18, 2019

GrayAn commented Jul 1, 2019

adiroiban left a comment

Choose a reason for hiding this comment

adiroiban Jul 1, 2019

Choose a reason for hiding this comment

GrayAn Jul 1, 2019

Choose a reason for hiding this comment

adiroiban Jul 1, 2019

Choose a reason for hiding this comment

adiroiban Jul 1, 2019

Choose a reason for hiding this comment

adiroiban commented Jul 1, 2019

GrayAn commented Jul 1, 2019

adiroiban commented Jul 1, 2019

GrayAn commented Jul 1, 2019

psi29a commented Jul 4, 2019

GrayAn commented Jul 8, 2019

adiroiban commented Jul 8, 2019

codecov bot commented May 13, 2019 •

edited

Loading

adiroiban left a comment •

edited

Loading