search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
211 stars 91 forks source link

PR - Issue 50206 - Add Unlock Inactive Accounts option to dsidm CLI #3605

Closed 389-ds-bot closed 4 years ago

389-ds-bot commented 4 years ago

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50549

Description: Port ns-accountstatus.pl, ns-activate.pl and ns-inactivate.pl to lib389 CLI. Add: dsidm account/role entry-status, dsidm account subtree-status, dsidm role lock/unlock Refactor: dsidm account lock/unlock Remove: dsidm account status Also, refactor role.py and idm/account.py accordingly to the CLI requirements.

Resolves: #3265

Reviewed by: ?

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-16 04:07:08

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).

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:17:18

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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:19:19

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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:20:03

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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:21:43

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:23:20

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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:24:35

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:25:12

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:26:00

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:27:16

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-16 04:28:11

Hope these comments help :)

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-19 02:41:47

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!

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-19 02:42:39

1 new commit added

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-19 17:32:12

2 new commits added

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-20 02:38:53

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?

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-20 02:39:45

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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-20 02:41:24

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?

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-20 02:44:18

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-20 02:45:16

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.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-20 02:45:39

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

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-20 21:14:07

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!

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-20 21:14:36

1 new commit added

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-21 02:10:55

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?

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-21 10:46:33

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 :)

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-23 02:39:38

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".

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-23 17:11:40

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...

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-26 01:14:20

Okay, thanks for clarifying that! Ack from me.

It's also worth noting that:

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-26 21:22:38

rebased onto 5287b9ac635a502f3cec9f6ac92f7f847b71d437

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-26 21:23:10

Pull-Request has been merged by droideck

389-ds-bot commented 4 years ago

Comment from spichugi (@droideck) at 2019-08-26 21:29:03

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 - #3622

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

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-08-27 00:28:23

Thanks - and great work as always :)

389-ds-bot commented 4 years ago

Patch 50549.patch