Please, focus on lib389 part for now...
CLI part has some issue that I'll fix tomorrow (but the logic is all in lib389 anyway).

Nitpick but you don't need to update the Copyright dates here because it's from the date of origin, not "last edit" :)

Why not move the dn validity check to the _generic checks so that everything inherits it rather than duplicating it?

Could be worth commenting what this does (I assume it's a subtree lock check via costemplates)

Could it be better to express this as an absolute time IE --inactive-at-time , and then we calc the offset by seconds to that time to do the query? That seems more user friendly, and certainly more applicable to policy and lookups an admin would want to do.

This is a subtree check, so do we really need the other options?

If you return this as a dict, then have the cli layer pass that dict to format status message, you can then benefit from status_json "out of the box" for later webui integration. Remember, don't have the "status" call the presentation/view layer - status JUST checks the status, and then the cli coordinates passing that data to a presentation layer.

Don't do this - if you unlock and unlocked account it's a no-op, not an error. Think about this as a state machine, not as imperative actions.

It's likely an error to make these LDAPsubentries because they then may be hidden incorrectly when an admin doesn't want it. It's better to ldapsubentry on a costemplate container, not the costemplates themself.

I think the placement of get_backend_by_entry is incorrect, because you should look up in the mapping tree which suffix is related. So Ithink you should make this something like mapping_tree.get_rootsuffix_by_entry() instead.

Hope these comments help :)

Great feedback, thank you!

Nitpick but you don't need to update the Copyright dates here because it's from the date of origin, not "last edit" :)

Fixed

Why not move the dn validity check to the _generic checks so that everything inherits it rather than duplicating it?

Nice catch!

Could be worth commenting what this does (I assume it's a subtree lock check via costemplates)

Some info is in help section of CLI. And the main logic is described in account.py. CLI is only for representation so I don't see much reason for detailed comments about under the hood logic at both places...

Could it be better to express this as an absolute time IE --inactive-at-time , and then we calc the offset by seconds to that time to do the query? That seems more user friendly, and certainly more applicable to policy and lookups an admin would want to do.

After given more thoughts, I think you are right. It makes sense to have it as a date even though we have accountInactivityLimit attribute specified in seconds. Fixed.

This is a subtree check, so do we really need the other options?

I left one (could be useful some times) and sub options (which is the default).

If you return this as a dict, then have the cli layer pass that dict to format status message, you can then benefit from status_json "out of the box" for later webui integration. Remember, don't have the "status" call the presentation/view layer - status JUST checks the status, and then the cli coordinates passing that data to a presentation layer.

Yeah, I pass return it as a dict and we can work with it later (if we'll add it to WebUI one day)

Don't do this - if you unlock and unlocked account it's a no-op, not an error. Think about this as a state machine, not as imperative actions.

I agree, and as I see it, we should support both - 'ensure_lock' and 'lock'. The second could be useful in case we want to support a Python coding style when the exception processing is a very important part of the logic.
The same way we have ensure_state and create, ensure_removed and remove, etc.

It's likely an error to make these LDAPsubentries because they then may be hidden incorrectly when an admin doesn't want it. It's better to ldapsubentry on a costemplate container, not the costemplates themself.

LDAPsubentry objectclass is inherited by nsRoleDefinition by design. I've fixed the search objectclasses in MANY though and included LDAPsubentry there.

I think the placement of get_backend_by_entry is incorrect, because you should look up in the mapping tree which suffix is related. So Ithink you should make this something like mapping_tree.get_rootsuffix_by_entry() instead.

Yeah, agree. Also, there were some import issues in Backend. I moved it as you suggested. Thanks!

This will do like ... 6 searches? I think it does a search for each get_attr_val because I never implemented entry caching. So you could consider using a "batch" get_attrs_vals_utf8 instead to get them all in a single pass, then lower-case as needed client side?

What we we don't have permission to read this /I think You'll get an exception ... .

I think this should be on a subclass of Roles like ... nsDisabledRole or something like that? It's not applicable to "all roles" is it?

I'm concerned you are using a string value here for status checking, rather than a sturcture type like an enum that has a unicode() fn for display. Change status to return an enum, then have __unicode on it for presentation, and this should check that.

ALso, consider not checking the status when you call lock, just do an ensure_present or whatever, to guarantee the attr exists, and then move on. This is especially important for accounts that want to lock, but don't have cos access in the status, so you are tying locking to status checks, when really they are seperate.

I'm still a bit worried you are confusing the presentation (strings) with structured apis with enum, and where that should be. Also where our logic is, because access controls exist .... it may be really valuable to have tests for this feature to really be able to explore some of these edg-ier cases.

(But otherwise, great work, like this is looking better, sorry I'm just perfectionist about this :)

This will do like ... 6 searches? I think it does a search for each get_attr_val because I never implemented entry caching. So you could consider using a "batch" get_attrs_vals_utf8 instead to get them all in a single pass, then lower-case as needed client side?

Fixed

What we we don't have permission to read this /I think You'll get an exception ... .

Added log message and and extended the statuses

I think this should be on a subclass of Roles like ... nsDisabledRole or something like that? It's not applicable to "all roles" is it?

We can disable any role - Filter, Managed or Nested. If I understood your question correctly...

I'm concerned you are using a string value here for status checking, rather than a sturcture type like an enum that has a unicode() fn for display. Change status to return an enum, then have __unicode on it for presentation, and this should check that.
ALso, consider not checking the status when you call lock, just do an ensure_present or whatever, to guarantee the attr exists, and then move on. This is especially important for accounts that want to lock, but don't have cos access in the status, so you are tying locking to status checks, when really they are seperate.

Yeah, I was thinking about it but then switched to another task and missed. Thanks for bringing it up!

My comment is about this - def lock on filtered roles - filtered roles is more than just "locking accounts" so I think lock here seems like it could be too broad or be used in places it shouldn't. This function isn't about filtered roles, it seems to be ... more?

My comment is about this - def lock on filtered roles - filtered roles is more than just "locking accounts" so I think lock here seems like it could be too broad or be used in places it shouldn't. This function isn't about filtered roles, it seems to be ... more?

Yeah, I had the same thought... I agree it is more than that.

Also, I checked the legacy tools and they allow to lock and monitor (check the status) of Filtered roles too. And it takes an effect. I wasn't able to bind with the user (which affected by filtered role) after that.

Maybe we can log a warning to the user that he tries to lock a FilteredRole?

Also, I haven't found any docs or examples about what should be done additionally with the role (like acis?). So if you have anything, please, share :)

I try to avoid roles as much as possible tbh, I think they are a difficult way to manage things.

What do you mean "lock a filteredrole"?

I think that the thing with this is the aci's are about the aci's on the user because to add to a role, you mod the user, the role just then directs cos/internal parts. So the aci's are "can the nsRoleDn attr be modified".

I try to avoid roles as much as possible tbh, I think they are a difficult way to manage things.
What do you mean "lock a nsRoleFilter"?

For example, we have a nsFilteredRoleDefinition with nsRoleFilter: (postalCode=66666) or even more complex filter which defines some unity of entries.

Then the administrator wants to lock these users.

He can do like this:

dsconf ldap://localhost:389 -D "cn=Directory Manager" -w password -b dc=example,dc=com role lock cn=postalcode_filter_role,cn=roles,dc=example,dc=com

And it will lock all entries that have postalCode=66666.

I think that the thing with this is the aci's are about the aci's on the user because to add to a role, you mod the user, the role just then directs cos/internal parts. So the aci's are "can the nsRoleDn attr be modified".

So in that case, when the administrator has some restricting ACI's regarding nsRoleDn he shouldn't use the role-lock feature by design. And he will get an error.

Some people though use roles and role inactivation feature (and they don't use ACI's that contradict with the feature). And for them, I think, we should have the option...

Okay, thanks for clarifying that! Ack from me.

It's also worth noting that:

• you should have tests for this ;)
• there IS a cli test framework to help you test cli specific parts, and you could consider using it to help test the greater framework.

Okay, thanks for clarifying that! Ack from me.
It's also worth noting that:

you should have tests for this ;)
there IS a cli test framework to help you test cli specific parts, and you could consider using it to help test the greater framework.

Thank you!
I've tested the feature a lot during the development and the rewriting but the test are needed, I full agree.

I've created the issue - https://pagure.io/389-ds-base/issue/50566

I'll switch to that as soon as I'll sort out other priorities or someone else can take this low hanging frute :)

Thanks - and great work as always :)

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/3605

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.