[Freeipa-devel] [PATCH 0015] Add support for managing user auth types

[Petr Viktorin]

On 09/18/2013 09:51 PM, Nathaniel McCallum wrote:
> On Mon, 2013-09-09 at 15:35 +0200, Petr Viktorin wrote:
>> On 09/05/2013 06:04 AM, Nathaniel McCallum wrote:
>>> patch attached
>>
>> Thanks, some comments below.
>>
>> Git complains about trailing whitespace in the patch, please strip it.
>
> Fixed.
>
>>> freeipa-npmccallum-0015-Add-support-for-managing-user-auth-types.patch
>>>
>>>
>>> >From 757436ccc431d26a3e62de830dad0b107a6c48ff Mon Sep 17 00:00:00 2001
>>> From: Nathaniel McCallum<npmccallum at redhat.com>
>>> Date: Wed, 4 Sep 2013 23:35:36 -0400
>>> Subject: [PATCH] Add support for managing user auth types
>>>
>>> https://fedorahosted.org/freeipa/ticket/3368
>>> ---
>>>    ipalib/plugins/config.py | 16 ++++++++++++++++
>>>    ipalib/plugins/user.py   | 32 ++++++++++++++++++++++----------
>>>    2 files changed, 38 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/ipalib/plugins/config.py b/ipalib/plugins/config.py
>>> index b9cf05016bf80cd48134cca5a50cdca7db423ca9..692ca22db70eb9a81a49eab6dc1e23284c8a9946 100644
>>> @@ -210,6 +218,14 @@ class config_mod(LDAPUpdate):
>>>
>>>        def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
>>>            assert isinstance(dn, DN)
>>> +
>>> +        if 'ipauserauthtype' in entry_attrs:
>>> +            if 'objectclass' not in entry_attrs:
>>> +                (_dn, _entry_attrs) = ldap.get_entry(dn, ['objectclass'])
>>> +                entry_attrs['objectclass'] = _entry_attrs['objectclass']
>>> +            if 'ipauserauthtypeclass' not in entry_attrs['objectclass']:
>>> +                entry_attrs['objectclass'].append('ipauserauthtypeclass')
>>
>> Shouldn't we rather add ipaUserAuthType to the ipaGuiConfig objectclass?
>
> No. The ipaUserAuthTypeClass is generic and will be used several places.
>
>> If not, we should still update ipaConfig on IPA update update rather
>> than here; install/updates/50-ipaconfig.update would be a good place.
>
> Fixed.
>
>>> diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
>>> index 471981f48204209753eda2fb994d4c653dca0fa2..02f62120d281a873dfd9c21e1b855b112cca05a4 100644
>> [...]
>>> @@ -633,14 +640,19 @@ class user_mod(LDAPUpdate):
>>>                entry_attrs['userpassword'] = ipa_generate_password(user_pwdchars)
>>>                # save the password so it can be displayed in post_callback
>>>                setattr(context, 'randompassword', entry_attrs['userpassword'])
>>> +
>>> +        if 'objectclass' not in entry_attrs:
>>> +            (_dn, _entry_attrs) = ldap.get_entry(dn, ['objectclass'])
>>> +            entry_attrs['objectclass'] = _entry_attrs['objectclass']
>>
>> The framework is forcing some pretty ugly code here.
>> I've filed https://fedorahosted.org/freeipa/ticket/3914 to simplify this
>> in the future.
>>
>>
>> Just a note, it's no longer necessary to use (_dn, _entry_attrs) here;
>> ldap.get_entry() now returns a dict-like entry directly so you can use:
>>
>>       _entry = ldap.get_entry(dn, ['objectclass'])
>>       entry_attrs['objectclass'] = _entry['objectclass']
>>
>> In fact, unpacking the entry into a tuple returns the DN and the entry
>> object itself. This:
>>       (dn, entry) = ldap.get_entry(...)
>> is exactly equivalent to:
>>       entry = ldap.get_entry(...)
>>       dn = entry.dn
>> but the former is deprecated.
>
> Fixed.

Great, we're getting close!

Please send patches in `git format-patch` style (they include commit info).
Also, please bump the API revision in VERSION since API.txt was changed.

When adding the objectclass in user, it is possible that the user 
doesn't exist. You should call handle_not_found in this case so the 
appropriate error message is generated.
I ended up doing this for testing, squash in the patch if you want.

There's another test failure when trying to rename a manager user. I 
didn't investigate in detail why that happens.

I'm attaching the tests I used, do they look OK?

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0281-Add-tests-for-user-auth-type-management.patch
Type: text/x-patch
Size: 5027 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130923/a20bf0b0/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: squash-Add-support-for-managing-user-auth-types.patch
Type: text/x-patch
Size: 1090 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130923/a20bf0b0/attachment-0001.bin>