Add support for X-ORIGIN in schema element class ObjectClass #247

droideck · 2018-10-08T16:08:18Z

389 Directory Server uses X-ORIGIN ObjectClass attribute for determining if the ObjectClass is 'user defined' or not. 389 Directory Server UI currently depends on it.
Also, X-ORIGIN is already supported for AttributeType schema element. So it makes sense to make it consistent across these two schema elements.

codecov · 2018-10-25T15:13:12Z

Codecov Report

Merging #247 into master will increase coverage by 13.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master     #247       +/-   ##
===========================================
+ Coverage   57.91%   70.93%   +13.01%     
===========================================
  Files          49       49               
  Lines        5910     4816     -1094     
  Branches      815      812        -3     
===========================================
- Hits         3423     3416        -7     
+ Misses       2157     1066     -1091     
- Partials      330      334        +4

Impacted Files	Coverage Δ
Lib/ldap/schema/models.py	`71.27% <100%> (+6.66%)`	⬆️
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/cidict.py	`69.23% <0%> (+7.16%)`	⬆️
Lib/ldap/compat.py	`66.03% <0%> (+11.35%)`	⬆️
Lib/ldap/controls/sss.py	`39.65% <0%> (+14.09%)`	⬆️
Lib/ldap/filter.py	`65.62% <0%> (+14.4%)`	⬆️
... and 23 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...9c48d3e. Read the comment docs.

encukou · 2018-10-25T15:14:08Z

I checked how the attribute looks on the object, and all looks fine. I suggest adding this extra test. Could you quickly review it?

tiran

The doc string of ObjectClass needs to be updated, too.

droideck · 2018-10-25T16:05:13Z

AttributeType also doesn't have info about 'X-ORIGIN' in its docstring. Should I update it too?

Test looks good to me.

encukou · 2018-10-25T16:40:12Z

On Thu, Oct 25, 2018, 18:05 Simon Pichugin ***@***.***> wrote: AttributeType also doesn't have info about 'X-ORIGIN' in its docstring. Should I update it too?

That would be great.

droideck · 2018-11-06T09:06:46Z

I've added the docstrings.

One more issue popped up. It is described here -
https://pagure.io/389-ds-base/pull-request/50005#comment-67932

So should I implement it? Are you okay with it?
And should I do it in this PR?

encukou · 2018-11-13T11:49:33Z

As I see it, the problem with X-ATTR is that it's not really standardized anywhere. Is that right?
Does any other server use it as a multi-valued attribute, or is this 389-DS specific behavior?

droideck · 2018-11-26T01:12:16Z

As I see it, the problem with X-ATTR is that it's not really standardized anywhere. Is that right?

Yes, it's right.

Does any other server use it as a multi-valued attribute, or is this 389-DS specific behavior?

At least, Oracle's Directory Server uses it the same way we are. :)

Anyway, if we'll add the multi-value support it wouldn't hurt the existing functionality.
The same way we have NAME now.
It will be NAME ( 'nsAIMid' 'nscpaimscreenname' ) for multi-values and NAME 'cosspecifier' for single-valued.

Also, currently, if we have X-ORIGIN ( 'my objectclass' 'user defined' ), the existing python-ldap implementation will just print the first value, which is wrong.

droideck · 2018-11-27T17:02:00Z

I've added the functionality I mentioned before and a test for it (also, I used FreeIPA LDIFs which have multi-valued X-ORIGIN, btw).

Please, check.

droideck · 2019-01-03T12:49:05Z

Are there any objections? :)
Or we can merge? The tests pass.

encukou · 2019-01-03T14:45:55Z

Tests pass, but there's standard to verify that the behavior is correct. I can't find anything to suggest that there's agreement between LDAP servers on what X-ORIGIN actually means.
Is there such a standard or some other neutral documentation that could be referenced here?

vashirov · 2019-01-03T15:57:36Z

LDAP does not define any standard schema extensions, but many directory servers accept any properly-formatted extension as a means of annotating the schema element. For example, one of the most commonly-used extension types is “X-ORIGIN”, which is typically used to indicate the source of the associated schema element, and the string “X-ORIGIN ‘RFC 4519’” might be used to indicate that the associated schema element is defined in RFC 4519.

as seen on
[1] https://ldap.com/schema-element-extensions/
[2] https://ldapwiki.com/wiki/LDAP%20Schema%20Element%20Extensions

Another usage example in LDAP server implementation:
https://github.com/ForgeRock/opendj-community-edition/blob/master/resource/schema/00-core.ldif

encukou

OK, let's put it in. @tiran, if you could confirm this is OK, that would be great.

I'm missing a test that setting x_origin works, and a test of the behavior of setting x_origin (to values like None, list, tuple, or single string).

Let me know if I should write it.

encukou · 2019-01-03T16:03:48Z

Lib/ldap/schema/models.py

@@ -126,6 +126,9 @@ class ObjectClass(SchemaElement):
  sup
    This list of strings contains NAMEs or OIDs of object classes
    this object class is derived from
+  x-origin
+    This string contains the X-ORIGIN text which is typically used to indicate
+    the source of the associated schema element. It can a list of strings


Suggested change

the source of the associated schema element. It can a list of strings

the source of the associated schema element. It should be a tuple of strings.

Agree. But it has a small typo - "be be"

Also, should we change other docstrings then for the consistency? For example, 'may' also should be a tuple but the docstring states 'This list of strings':

may This list of strings contains NAMEs or OIDs of additional attributes an entry of the object class may have

I can add one more commit here for this

I guess "sequence" is the exact thing we want here.
Perhaps write it out as "sequence (list/tuple)", to aid people who don't actively know that definition.

Actually, list wouldn't work because the SchemaElement class explicitly requires a 'tuple'.

class SchemaElement: def key_list(self,key,values,sep=' ',quoted=0): assert type(values)==tuple,TypeError("values has to be a tuple, was %r" % values)

Lib/ldap/schema/models.py

droideck · 2019-01-03T16:44:52Z

I'm missing a test that setting x_origin works, and a test of the behavior of setting x_origin (to values like None, list, tuple, or single string).

Sure.
I've searched for any examples in other tests but it looks to me that we don't test 'set' operation of any particular attributes (like self.names = d['NAME'], self.desc = d['DESC'][0], self.must = d['MUST'], etc.)

So if I missed something and it's there (or you have tools for that) feel free point me to it :)
Thanks

encukou · 2019-01-03T16:55:37Z

Well, new features need tests, even if old ones don't have them.
Right now I don't know how they would be written, but I can look into it tomorrow (and, in the process, actually write the tests).

droideck · 2019-01-03T16:57:17Z

Well, new features need tests, even if old ones don't have them.
Right now I don't know how they would be written, but I can look into it tomorrow (and, in the process, actually write the tests).

Sure, I agree. Thank you very much!

The sequences are always tuples, not lists. String attributes are None when missing.

encukou · 2019-01-04T17:39:06Z

Can I ask for a quick check of these?

droideck · 2019-01-04T21:41:04Z

Looks great! Thank you!

encukou

Cross-reviewed by me and @droideck

Add support for X-ORIGIN in schema element class ObjectClass

7c22697

encukou requested a review from tiran October 25, 2018 14:25

Add a test for X-ORIGIN

ebc12da

tiran requested changes Oct 25, 2018

View reviewed changes

Add docstrings for X-ORIGIN

9f5a578

Add multi-value support to X-ORIGIN attribute

7d1359c

encukou reviewed Jan 3, 2019

View reviewed changes

encukou added 5 commits January 4, 2019 16:20

Test the stringification of X-ORIGIN in schema attributes

cd35f1c

Test behavior of setting x_origin

c186345

Fix x_origin (not x-origin) attribute name in docs

2dc4ebd

Fix and reword docs of schema elements

7b76b00

The sequences are always tuples, not lists. String attributes are None when missing.

Test types and values of ObjectClass and AttributeType attributes

9c48d3e

encukou approved these changes Jan 7, 2019

View reviewed changes

encukou merged commit bdf7820 into python-ldap:master Jan 7, 2019

This was referenced Sep 13, 2020

PR - Issue 49928 - Fix various small WebUI schema issues 389ds/389-ds-base#3025

Closed

PR - Ticket 50004 - lib389 - improve X-ORIGIN schema parsing 389ds/389-ds-base#3064

Closed

	the source of the associated schema element. It can a list of strings
	the source of the associated schema element. It should be a tuple of strings.

Add support for X-ORIGIN in schema element class ObjectClass #247

Add support for X-ORIGIN in schema element class ObjectClass #247

Uh oh!

Conversation

droideck commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

encukou commented Oct 25, 2018

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

droideck commented Oct 25, 2018

Uh oh!

encukou commented Oct 25, 2018 via email

Uh oh!

droideck commented Nov 6, 2018

Uh oh!

encukou commented Nov 13, 2018

Uh oh!

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

Uh oh!

Uh oh!

droideck commented Nov 27, 2018

Uh oh!

droideck commented Jan 3, 2019

Uh oh!

encukou commented Jan 3, 2019

Uh oh!

vashirov commented Jan 3, 2019

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

encukou Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droideck Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

droideck Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

encukou Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

droideck Jan 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

droideck commented Jan 3, 2019

Uh oh!

encukou commented Jan 3, 2019

Uh oh!

droideck commented Jan 3, 2019

Uh oh!

encukou commented Jan 4, 2019

Uh oh!

droideck commented Jan 4, 2019

Uh oh!

encukou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

droideck commented Oct 8, 2018 •

edited

Loading

codecov bot commented Oct 25, 2018 •

edited

Loading

droideck commented Nov 26, 2018 •

edited

Loading

encukou Jan 3, 2019 •

edited

Loading

droideck Jan 3, 2019 •

edited

Loading