Hey mate,

I'm not comfortable with the raw use of "Entry" here in the python code. The whole design of lib389 is meant to mean you NEVER need to actually see an Entry because they are a "kinda broken" abstraction that I have always wanted to remove. They actually make python3 / utf8 really hard!

So it would be better to rework the pwpolicy object a bit, maybe @spichugi can help here as he probably knows this second best to myself :)

Thanks! I'll try to be a bit more active on reviews again soon!

As I understand, get_and_set_sasl() is a part of load_config(). And load_config() sometimes can happen before sasl_table is initialized.
It is what I experienced and I got the error in https://pagure.io/389-ds-base/issue/49878
I am not sure how exactly to resolve it though, I need to read more into cockpit-389-ds structure...

"its makes" - typo

It is better to get exact value type with

get_attr_val_utf8_l('nsslapd-suffix')

"l" is for 'lower'

Same

I agree with William, we better use direct python-ldap or new lib389 abstractions which are properly compatible with Python 3.

attrs = {'objectclass': ['top', 'nsContainer'],
              'cn': ['nsPwPolicyContainer']}
ldif = modlist.addModlist(attrs)
self.conn.add_ext_s(basedn, ldif)

And if you'd like to write a stuff for lib389's DSLdapObject, we need to think more about the structure, it'll be not so obvious.
The question is if we are doing it now or later (but I think we have to do it one day or another anyway)

If you try to create a User of Subtree policy and it fails (for example, there is "basedn was not found"), then the policy still will appear in the table. And each time it fails it will add another 'garbage' entry to the list.

Another small thing... The fields under Global Password policy have the default values in them, but the fields under Local password policy are empty. Is it intentional?

If I try to edit the entry, it gives me a strange error -
{'desc': 'No such object', 'matched': 'uid=demo_user,ou=people,dc=example,dc=com'}

Strange - because 'uid=demo_user,ou=people,dc=example,dc=com' is my entry for user pwpolicy

I can probably alter the timing, I'll look into this... Nice investigation

Maybe we should create new wrappers as its confusing how many different ways there are to get/set values in lib389. If we should use only this function, then lets make it clear and have a nice clear name, etc. This should be in a different ticket of course...

I stole this code from somewhere else in lib389, fyi. If we shouldn't use it, then we shouldn't find it elsewhere in the lib389 code ;-)

Anyway I have no problem changing it :-p

@spichugi I'll look into these comments today. Thanks for testing!

@firstyear @spichugi - okay all changes made.

I also fixed the timing of loading HTML pages and loading the configuration settings.

Please review, thanks!!

Hey, there is a magic method on DSLdapObject where if you take any function and call it as "_json" appended, if the function returns a dict, it turns it to JSON.

You should use that insteah of this flag, because it just adds so much more complexity to testing the code.

You shouldn't be doing raw searches here, you should be using a DSLdapObjects class and filtering. This is again, raw entry types that are bad.

This should be a DSLdapObject, not an object, and it should use this as such.

Hey @mreynolds I think you need to rewrite most of pwpolicy.py to use DSLdapObject[s]. The raw search_s and such is not good, and there are already functions for json converstion to make this much easier. I can help with this or @spichugi can, because it really really really will help to make integration with the CLI better and easier because it's all designed to hook into that object models.

Thanks,

As an amendment, there are plenty of objects already in lib389 you can use as examples of how to do this.

Hey @mreynolds I think you need to rewrite most of pwpolicy.py to use DSLdapObject[s]. The raw search_s and such is not good, and there are already functions for json converstion to make this much easier. I can help with this or @spichugi can, because it really really really will help to make integration with the CLI better and easier because it's all designed to hook into that object models.

I couldn't get DsLdapObjs working with local pwp policy structure (as there are several COS entries and containers you have to take into account) not without making a lot of changes to DSLdapObj. Feel free to rerewite it and provide a patch

I couldn't get DsLdapObjs working with local pwp policy structure (as there are several COS entries and containers you have to take into account) not without making a lot of changes to DSLdapObj. Feel free to rewrite it and provide a patch

Let me elaborate on this...

Since local polices consist of many different types of entries, not just one "entry", things like, "list", "get", "set", "create", "delete" from DsLdapObj don't work. So I could use DsLdapObj, but I can only use a fraction of its feature set which really isn't helpful.

Simon and I have already discussed extending DsLDapObj for password policies (which appears to be the corner case of DsLdapObjs - the other remaining features should not run into this), but it is not trivial to extend DsLdapObjs, and we wanted to do it later (1.4.1).

What you don't see going on around here since you left is that things have been very crazy. We have looming deadlines (especially around CLI/UI), non-stop customer escalations (internal and external), and VERY VERY limited dev resources. We don't have the luxury of taking our time and rewriting everything to make it picture perfect.

So for now we have make unpopular decisions so we can meet deadlines (that is our reality). If you really want to help, contribute code, but asking us to completely rewrite a lot of the features that are already working, when there is simply no time to do so, is not helpful or possible. Now we are hoping to revamp a lot of this in 1.4.1 (once time constraints are relaxed), but we need to get 1.4.0 wrapped up first and we are behind on that effort...

Temporary fixes become permanent ones. That's always been the way. Yes I do plan to do more lib389 work again (new workplace allowing), but it will still only be "part-time". There is only so much part-time can do ... :(

There are ways to make this work with dsldapobjects. It just requires some planning and time. I actually think it will be quite simple to achieve. The things you say as in "get" "set" "create", that's not all dsldapobject is either.

Yes I understand that you feel stressed that you don't have time available to you at the moment. That also isn't an excuse to rush IMO. That means that the business needs to properly support you and your work and allow the resources you need. Especially a premium product like DS, I do not accept that we can ever rush the work we do. If Red Hat is not willing to invest properly in their people, that is their issue. I do not believe that their internal issues should be allowed to affect the high standards we hold ourselves to in this project's upstream and open source components.

It is on reflection that I see that a contributing factor to my own issues was the very fact that I felt the team was under-resourced, and that I had to achieve "everything" because of "deadlines". Contributing in that environment while also isolated, unable to receive the support I required certainly helped lead to the events that followed. If you have this issue, you need to communicate that to people in the team, and in your management chain. Perhaps you release with a partial feature set on day 1? Perhaps the release date needs to be pushed back? I personally failed because when I asked for help (and did not get it) I stopped asking. Please don't be like me. Please always and continually push up on people to get the help you need.

You have always been the person who would not allow me to take shortcuts in my work - be it whitespacing or more. Please consider this my request that you do not take shortcuts either.

So to summarise: upstream is not Red Hat. Red Hat's management need to take responsibility for their failings. Do not fall into the same trap that I did. Temporary fixes are permanent. Our upstream is about quality, not quantity. Take care of yourself first and foremost. Take pride in your work being the best possible.

Thanks,

I think DSLdapObject is a very nice feature and it really can help people with writing something cool using lib389 API.

A small note that is not directly related to the question:

As I see, DSLdapObject is a helper abstraction. It can help us to write a better code but we 
shouldn't over complicated it too. So sometimes it is okay to use bare python-ldap for some 
operations, IMO, so we can do some calls that are very close to the ones used in C.
I understand that we still use Entry() object while doing 'search_s' but I think it is minor 
and it is better if we just check the attribute value as Mark has done here.
Alternative (if we don't have DSLdapObject) is python-ldap "search_result_entries[0][1]['nnslapd-myattr']"
which is kinda tedious.

We can implement CLI tools either with bare python-ldap, either with lib389 DSLdapObject abstraction. We just should use the one that suits the task best. The lib389 DSLdapObject is preferred, of course. But...
Currently, we don't have PwPolicy DSLDapObject expansion so we should use direct python-ldap calls (as you can see, it gives not much code, it is pretty straightforward and it's easy to read). It will do the job and we still will get to the deadlines.

Writing PwPolicy DSLDapObject module is a separate task we can create. It should include CLI tools upgrade. I agree we should care about it so we should plan it for the future release that is not too far (like Mark proposed - 1.4.1 will be a good shot)

P.S. I am going to review the PR and test it now...

It has the bytes value. We should convert it to UTF-8 (ensure_str) in order self.conn.delete_s(policy_dn) to work

I still have the errors with the local policy removal...

Failed to delete local password policy {'desc': 'No such object', 'matched': 
'cn=nsPwPolicyContainer,ou=People,dc=example,dc=com'}

But it does partial removal because I can't go to 'edit/view' option after this:

Could not find password policy for entry: ou=People,dc=example,dc=com Error: {'desc': 'No such object', 'matched': 'dc=example,dc=com'}

I think DSLdapObject is a very nice feature and it really can help people with writing something cool using lib389 API.
A small note that is not directly related to the question:
As I see, DSLdapObject is a helper abstraction. It can help us to write a better code but we
shouldn't over complicated it too. So sometimes it is okay to use bare python-ldap for some
operations, IMO, so we can do some calls that are very close to the ones used in C.

The issue is that you will hit issues where you begin to reimplement the same things (modify, changes, encoding), you will have to integrate it to the CLI (which is all design to hook dsldapobjects), and you will need to continually extent custom things.

It's not just there because it's pretty, there are good, fundamental reasons to use dsldapobjects. The Entry object that is shimmed in via DirSrv is a really bad abstraction. It makes a lot of assumptions and has caused me many problems, especially with moving to python 3.

It seems like it's "easy" to just drop in your search_s here and modify_s there, but really, long term maintence you end up with 100 bespoke, custom objects, and they all have different behaviours and no consistent interface. You reinvent the same wheel over and over. Now you have to custom shim in each one to the cli, you have an unclear model of the data and it's interactions. It becomes harder to test and utilise with existing modules.

I can not strongly stress enough that this is about doing the right thing to prevent more tech debt from being created. That's why I'm taking a strong stance on this matter.

                                  I still have the errors with the local policy removal...

Failed to delete local password policy {'desc': 'No such object', 'matched':
'cn=nsPwPolicyContainer,ou=People,dc=example,dc=com'}

It works for me, please explain what you doing so I can reproduce it. Thanks!

@firstyear, we're going to get everything work with this current patch (although it is working for me with the latest rebase) and merge it, but then right away @spichugi is going to make the conversion to DsLdapObject.

It works for me, please explain what you doing so I can reproduce it. Thanks!

Does it pass for User local policy deletion too? Because it is okay for Subtree for me now, but the User deletion fails.

I guess it was failing for Subtree because I did fix 'ensure_str' issue locally on the machine and it got messy...

@spichugi - patch is ready for you

Works for me! Thank you! Ack

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 pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/2935

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.