This comes with a unit test that asserts the correct behaviour and presence of the default objects, and additonally some checks to assert we only push the new data on a 1.4.0 install (IE prevent breaking older tests or unsupported schema issues).

The patch looks good. A question regarding the 'nsSshPublicKey' attribute, I am surprised it is a directoryString shouldn't it be octetString or something like that ?

I have a few questions about the new schema.

do we still use ns as a prefix nsXXX in new schema additions, the netscape reference is no longer needed, why not dsPerson or extPerson as we extend an objectclass.
The new oc, do they really need to be structural ?
why is displayname a MUST?

The code looks good to me, thanks! You have my ack.

Though I think it is important to mention in the docstrings that we should choose the new nsUserAccount and nsUserAccounts only on 1.4.0 and above. It will help to avoid a confusion in the documentation.

376 +    This is the modern and correct userAccount type to choose.
410 +    This is the modern and correct userAccount type to choose.
441 +    This is the classic "user account" style of cn + sn. You should consider
442 +    nsUserAccount instead.
443 +
487 +    This is the classic "user account" style of cn + sn. You should consider
488 +    nsUserAccounts instead.
489 +

Also, I think we can transform these lines into docstring (simple ones, without dirsrvtests/suites format)

561 +def test_install_sample_entries(topology_st):
562 +    # Assert that our entries match.
562 +    """"Assert that our entries match."""
570 +def test_install_aci(topology_st):
571 +    # Assert our default aci's work as expected.
571 +    """Assert our default aci's work as expected."""

I also agree with Ludwig on the naming. We should start dropping "ns" prefix where we can since it really refers to "netscape". I think "ds" is a good prefix, but I'm concerned that its such a obvious choice that customers might already being using these names for their custom schema. For example, if I was a customer and I added a new person objectclass I would probably call it "dsPerson". These conflicts are probably rare, but I wanted to mention it anyway.

I have a few questions about the new schema.
do we still use ns as a prefix nsXXX in new schema additions, the netscape reference is no longer needed, why not dsPerson or extPerson as we extend an objectclass.
The new oc, do they really need to be structural ?
why is displayname a MUST?

To answer both points from @mreynolds and @lkrispen. I have thought about this as well, because I don't like the ns prefix either. The issue is it's so prevalent in the code. So I think we either

• Accept it and just keep consistent (even if not perfect)
• Try to change new names to ds prefix, but then have inconsistent split of ns/ds

I think there really is not right answer here, but I opted to go with consistency over vanity. As much as I would love to make everything "ds" prefix, I think practically as a project we have to wear the NS prefix from now on, we can't avoid it. While not perfect, I think the consistency matters more, and most people don't even think about the origin of it anyway :)

As for "structural". I chose this to parallel the "person" objectClass, which is also structural. I did consider this a lot, thinking about if it should just be aux with top as structural component, but I don't know if that actually works TBH (it probably does, but I have not tested). I considered if this could conflict with other classes that subclass on person, but investigating popular types like radius info and eduPerson, these are all auxtypes. So this won't conflict. I think in a way it shows how aux and structural classes of ldap objects are kinda silly in a way, but it DOES prevent a single aci attack vector related to adding group objectClasses to a person object.

Finally, displayName is a must due to the reasons I put in the mailing list. People should be able to choose how they are viewed and identified as a person that is seperate from the UID or legalName that they have. It's the "identity" part of identity management :). As far as the ldap object goes I can see that from a technical view, having "may displayname, legalname" seems like a technically attractive option due to choice of which field to use, but the issue is that some implementors may choose to use legalName only (think fb real name policy). This goes against what we want! What we want is people to be identified the way they choose, and to do this requires enforcing that a displayName property is ALWAYS present, and that optionally the system may collect a legalName for reasons. It means that there is one correct way to render a person on a login screen, a website or others (displayName), because we guarantee it's there.

I think the final point to remember is that while this is saying "your users should look like this" the current Person schema still remains so we will not affect existing installs that wish to retain their current naming policies. Only that we are demonstrating "here is a modern way to achieve this that is inclusive and respectful to individuals".

I hope that helps!

The patch looks good. A question regarding the 'nsSshPublicKey' attribute, I am surprised it is a directoryString shouldn't it be octetString or something like that ?

Nope, it's a string like:

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCfDs2+iClMFiirY87dTy4/URWQP8wGLGs5uwsNhLzwPYfaQQNjvH7WvFzaO ...

Exactly as it would be in a .pub file in .ssh :)

The code looks good to me, thanks! You have my ack.
Though I think it is important to mention in the docstrings that we should choose the new nsUserAccount and nsUserAccounts only on 1.4.0 and above. It will help to avoid a confusion in the documentation.
376 + This is the modern and correct userAccount type to choose.
410 + This is the modern and correct userAccount type to choose.
441 + This is the classic "user account" style of cn + sn. You should consider
442 + nsUserAccount instead.
443 +
487 + This is the classic "user account" style of cn + sn. You should consider
488 + nsUserAccounts instead.
489 +

Also, I think we can transform these lines into docstring (simple ones, without dirsrvtests/suites format)
561 +def test_install_sample_entries(topology_st):
562 + # Assert that our entries match.
562 + """"Assert that our entries match."""

570 +def test_install_aci(topology_st):
571 + # Assert our default aci's work as expected.
571 + """Assert our default aci's work as expected."""

I agree, good spot. I will make these changes :)

The patch looks good. A question regarding the 'nsSshPublicKey' attribute, I am surprised it is a directoryString shouldn't it be octetString or something like that ?

Nope, it's a string like:
ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCfDs2+iClMFiirY87dTy4/URWQP8wGLGs5uwsNhLzwPYfaQQNjvH7WvFzaO ...

Exactly as it would be in a .pub file in .ssh :)

It is not a string, you see the base64 encoding of an octet string - and it is base64 encoded because it is NOT a string. We already have SshPublicKey, why do we need a nsSshPublicKey ?

about ns prefix, structural oc and displayname

ns prefix. do we need a prefix at all. If we want to introduce new attribute types and objectclasses do we need to prefix existent names or could we come up with new, reasonable names ?

structural. I only want to make sure that we introduce new schema and demo example we should only have ONE structural oc per entry. We are a bit sloppy in accepting multiple, but if we creat new entries we should be compliant

displayname. on one hand you say: "People should be able to choose how they are viewed and identified", but on the other hand you want to enforce a very specific deployment and I think it is not up to us to decide this.

The patch looks good. A question regarding the 'nsSshPublicKey' attribute, I am surprised it is a directoryString shouldn't it be octetString or something like that ?
Nope, it's a string like:
ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCfDs2+iClMFiirY87dTy4/URWQP8wGLGs5uwsNhLzwPYfaQQNjvH7WvFzaO ...
Exactly as it would be in a .pub file in .ssh :)

It is not a string, you see the base64 encoding of an octet string - and it is base64 encoded because it is NOT a string. We already have SshPublicKey, why do we need a nsSshPublicKey ?

We don't own the OID, and it may conflict with existing deployments ...

It is a string, absolutely is a string. it's "ssh-rsa ". And sssd and others expect exactly this format else it won't work.

about ns prefix, structural oc and displayname

ns prefix. do we need a prefix at all. If we want to introduce new attribute types and objectclasses do we need to prefix existent names or could we come up with new, reasonable names ?

We can, but the issue is "what are those names" and "will the conflict". I decided to be consistent with all our other types and avoid the possible conflicts.

structural. I only want to make sure that we introduce new schema and demo example we should only have ONE structural oc per entry. We are a bit sloppy in accepting multiple, but if we creat new entries we should be compliant

Okay I'll remove structule from nsorgperson, and make it aux.

displayname. on one hand you say: "People should be able to choose how they are viewed and identified", but on the other hand you want to enforce a very specific deployment and I think it is not up to us to decide this.

Right now we have a "person" which is cn/sn, which isn't right either. And this is enforced at a schema level of the person class.

So yes, I'm proposing an opinionated change, that says "displayName/legalName" as the pair, but this has been widely discussed and supported by various communities, especially when it comes to women's rights and needing to have different displaynames to avoid abuse and violence. If a technical system says "yes actually respect how people want to be named" then people will follow that. There is a number of points about this on the 389-devel list, along with broader support from people who are very active in this space and the issues.

As well people don't need to use this, they are absolutely able to continue to use Person or whatever else they choose. We aren't forcing them to use the new types, but we are demonstrating a "better way" to represent people (note I'm not changing the UserAccount type, only adding a parallel type).

• regarding the syntax, if nsSshPublicKey stores the string "ssh-rsa " I think directoryString is a good choice.

• regarding the mandatory displayname, I have no strong opinion. The only thing is that moving a MAY attribute to MUST creates schema issue. So if there is a chance that 'displayname' is preferable as mandatory it needs to be in the MUST list from the begining.

I don't have a strong opinion about the prefix, but one argument for a new prefix is that it would distinguish the "new/modern" schema from the "old". But like I said I am fine with "ns".

I don't have a strong opinion about the prefix, but one argument for a new prefix is that it would distinguish the "new/modern" schema from the "old". But like I said I am fine with "ns".

This is also a good point, but I still think I prefer consistency over distinguishing. Admins don't look at "what's new and what's old" they look at "what's in 389-ds" I think. So I think I'll continue to opt for consistency.

regarding the mandatory displayname, I have no strong opinion. The only thing is that moving a MAY attribute to MUST creates schema issue. So if there is a chance that 'displayname' is preferable as mandatory it needs to be in the MUST list from the begining.

Yes, I would like to keep this as MUST. :)

I'll update this with @spichugi's comments later and update the patch,

I don't have a strong opinion about the prefix, but one argument for a new prefix is that it would distinguish the "new/modern" schema from the "old". But like I said I am fine with "ns".

This is also a good point, but I still think I prefer consistency over distinguishing. Admins don't look at "what's new and what's old" they look at "what's in 389-ds" I think. So I think I'll continue to opt for consistency.

That 's fine, and we can't completely get rid of "netscape" from 389 anyway - it's simply in its "bones" (e.g. ns-slapd).

Yeah, I think so .... We have to live with our history,

So now, is there any other objection? I need to update the patch with @spichugi's comments still ...

ack

Updated patch with @spichugi 's comments, and @lkrispens recommendation about auxiliary. Rebased and tested.

CliSuite 70 passed in 174.96 seconds
Basic and replication suites pass

Given @mreynolds ack, and all the positive comments I think this is okay to merge. Any other concerns we can still resolve as we haven't released yet.

commit b086d41607628635e242a8a80631dfe259fcc75f
To ssh://git@pagure.io/389-ds-base.git
7b03c31..b086d41 master -> master

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/2484

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.