Allow nsaccountlock to be searched in user-find commands

[redhatrises]

This patch provides the ability to search and find users who are enabled/disabled in ipa user-find command without breaking API compatibility.

[MartinBasti]

Hello,

thank you for PR!

I have a few comments:

• Why user-show needs --nsaccountlock option?
• Could be this done by changing flags instead of overriding get_options? IMO it is compatible

diff --git a/ipaserver/plugins/user.py b/ipaserver/plugins/user.py
index 0194f1b..3df2723 100644
--- a/ipaserver/plugins/user.py
+++ b/ipaserver/plugins/user.py
@@ -371,7 +371,7 @@ class user(baseuser):
     takes_params = baseuser.takes_params + (
         Bool('nsaccountlock?',
             label=_('Account disabled'),
-            flags=['no_option'],
+            flags=['no_create', 'no_update'],
         ),
         Bool('preserved?',
             label=_('Preserved user'),

Adding @HonzaCholasta to make sure that changing options in this way is compatible

[redhatrises]

Why user-show needs --nsaccountlock option?

I didn't want to limit it to user-find. However, it looks like adding the option is actually pointless as that information is in the output already. I can fix that.

Could be this done by changing flags instead of overriding get_options? IMO it is compatible

@MartinBasti sure. Not sure where we are with ABI/API compatibility issues which is why I didn't use the overriding get_options. I guess we will see what @HonzaCholasta says.

[HonzaCholasta]

Replacing flags=['no_option'] with flags=['no_create', 'no_update'] is not backward compatible - the no_option flag only hides the option in the CLI, but no_create / no_update would completely remove it from user_add / user_mod.

So, @redhatrises's approach is OK, although I would rather remove the no_option flag in user.takes_options and add it back in user_add.get_options() and user_mod.get_options().

Also, now that the options is visible in CLI, you should set cli_name='disabled' on it, so that we have a --disabled option rather than --nsaccountlock option in ipa user-find.

[redhatrises]

@MartinBasti I believe that this is ready for your review.

[MartinBasti]

LGTM, I'll test it later

[MartinBasti]

@pvomacka IMO this may deserve webUI part too

[MartinBasti]

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/a930ec824da0337109d646ab3acb495dc1b6ba63

[MartinBasti]

I found "not-sure-if" bug, nsaccountlock is not always specified (admin has it and any user after user-enable, that's why I didn't catch it during testing of PR) in LDAP tree, so search user-find --disabled=false returns only admin adn user that were explicitly enabled.

IMHO this is unexpected behavior for users, however expected from IPA framework POW and LDAP POW.
What could we do to improve UX? Maybe on client side we should allow --disabled only as flag to prevent users to search in enabled users and get corrupted results.

[MartinBasti]

Or we can modify search filter on server to cover this case, but it won't be nice

[redhatrises]

@MartinBasti sorry for the late reply, but yes, this is a bug. If 'nsaccountlock' doesn't exist, it should return as Account disabled = False. I know this PR is already closed, but should be add 'nsaccountlock' on ipa user-add?

[MartinBasti]

@redhatrises IMO for new users we can always create that attribute in LDAP, that should limit bad behavior. I wouldn't add it to user-add, usually you wants to create an enabled user, for disabled you can use stage-user. I hope that activating of stage user creates this attribute in LDAP as well.

However this need a discussion, if it is a proper approach is the right.

BTW you can open a new PR we shouldn't continue here.

[HonzaCholasta]

No, it's not the right approach. This is an issue in the framework and that's where it needs to be fixed - in the framework - rather than working around the issue in every plugin which hits it.

[abbra]

nsaccountlock is an operational attribute, not a normal one. I don't like it being created all the time. You have to request it explicitly if you want to show status of users, not invent a mechanism to always add it.

[redhatrises]

Thanks guys. So can this be fixed in pre_callback or post_callback in user_find, or am I looking elsewhere? (Not super familiar with all of the IPA framework)

[abbra]

Yes, you can add nsaccountlock attribute retrieval in the pre_callback and process it in the post_callback. nsaccountlock is an operational attribute so it needs to be requested explicitly.

[HonzaCholasta]

@abbra, the issue is not that the attribute is not requested (it is in fast always requested in user commands), it is that when the attribute is not set on a user entry (that's right, the attribute is not operational in 389 DS), the entry will not be returned in ipa user-find --disabled=0, which might be surprising to the user.

@redhatrises, the framework fix would be to update LDAPSearch.get_attr_filter() to handle the "search for the default value" case, off the top of my head it should be something like this:

    def get_attr_filter(self, ldap, **options):
        """
        Returns a MATCH_ALL filter containing all required attributes from the
        options
        """
        search_kw = self.args_options_2_entry(**options)
        search_kw['objectclass'] = self.obj.object_class
        default_kw = self.get_default(**options)
        filters = []
        for name, value in search_kw.items():
            flt = ldap.make_filter_from_attr(name, value, ldap.MATCH_ALL)
            if name in default_kw and value == default_kw[name]:
                # default value search, check also for non-present attribute
                flt = ldap.combine_filters([flt, '(!({}=*))'.format(name)])
            filters.append(flt)
        return ldap.combine_filters(filters, ldap.MATCH_ALL)

[abbra]

The nsaccountlock is virtual attribute in 389-ds:

attributeTypes: ( 2.16.840.1.113730.3.1.610 NAME 'nsAccountLock' 
   DESC 'Operational attribute for Account Inactivation' SYNTAX 1.3.6.1.4.1.1466.115.121.1.15
   USAGE directoryOperation X-ORIGIN 'Netscape Directory Server' )

Notice USAGE directoryOperation in the attribute definition. It is treated as a virtual one everywhere in the code but nothing sets it. It is supposed to be set via nsRole and CoS template. See ns-activate.pl/ns-inactivate.pl/ns-accountstatus.pl in 389-ds for external manipulation of it.

[HonzaCholasta]

I see, I assumed that it's not operational because it's not always set. I stand corrected, but this information does not change anything in respect to the default value search issue.

[abbra]

You are correct in the fact that the search filter need to be modified to allow matching entries without nsAccountLock attribute set.