[Freeipa-devel] [PATCH, 4.1] 0166 updater: enable uid uniqueness plugin for posixAccount objects

[Jan Cholasta]

Dne 21.10.2014 v 13:10 Martin Kosek napsal(a):
> On 10/21/2014 12:41 PM, Alexander Bokovoy wrote:
>> On Tue, 21 Oct 2014, Alexander Bokovoy wrote:
>>> On Tue, 21 Oct 2014, Martin Kosek wrote:
>>>> On 10/21/2014 08:41 AM, Alexander Bokovoy wrote:
>>>>> On Tue, 21 Oct 2014, Martin Kosek wrote:
>>>>>> On 10/20/2014 08:25 PM, Alexander Bokovoy wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> This patch is for ipa-4-1 branch to enable uniqueness plugin for uid
>>>>>>> attribute for entries with objectclass posixAccount.
>>>>>>>
>>>>>>> We don't have uid uniqueness enforced in FreeIPA < 4.1 yet but for
>>>>>>> posixAccounts it worked due to our design of a flat tree: as uid
>>>>>>> attribute is
>>>>>>> part of the DN, renaming user entries
>>>>>>> enforces uniqueness as MODRDN will fail if entry with the same uid
>>>>>>> already exists.
>>>>>>>
>>>>>>> However, it is not enough for ID views -- we should be able to allow
>>>>>>> ID view overrides for the same uid across multiple views and we should
>>>>>>> be able to protect uid uniqueness more generally too.
>>>>>>>
>>>>>>> Implementation is done via update plugin that checks for existing uid
>>>>>>> uniqueness plugin and if it is missing, it will be added. If plugin
>>>>>>> exists, its configuration will be updated.
>>>>>>>
>>>>>>> I haven't added update specific to git master where staging subtree is
>>>>>>> added but I'll do that after FreeIPA 4.1 release as in 4.1 we don't yet
>>>>>>> have the staging subtree. Currently master has broken setup for uid
>>>>>>> uniqueness plugin that doesn't actually work anyway so it will be easier
>>>>>>> to add upgrade over properly configured entry.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/4636
>>>>>>
>>>>>> Hi Alexander,
>>>>>>
>>>>>> Thanks for the patch! However, I am personally not very confident with
>>>>>> merging
>>>>>> it right before 4.1 release, I thought it will be a simple update definition
>>>>>> while this is a complex upgrade script which needs to be properly tested.
>>>>>>
>>>>>> I would rather wait for 4.1.x, especially given it does not block any 4.1
>>>>>> major
>>>>>> feature in any way.
>>>>> I disagree on it for multiple reasons and one of them is that 'a simple
>>>>> update definition' is not right here. Attribute uniqueness plugin
>>>>> supports three different types of setting its own arguments. These types
>>>>> aren't mixable, you have to do switch from one to another. That's why
>>>>> update plugin is the correct approach here.
>>>>>
>>>>> The update plugin I've wrote is very simple by itself.
>>>>
>>>> Ok, reviewing the patch now. I found 2 issues:
>>>>
>>>> 1) It is missing in plugins' Makefile.am so it won't be built properly
>>>>
>>>> 2) Instead of
>>>> +        if len(entries) == 0:
>>>> use
>>>> +        if entries:
>>> You mean 'if not entries:'? Will fix.
>>>
>>>
>>>> 3) I think we should force "uid uniqueness" plugin creation in all cases so
>>>> that we can expect it is there in future and for example do simple updates
>>>> for it.
>>> It does enforce it creating already. The only case that is not handled
>>> right now is the case of already existing multiple uid uniqueness
>>> plugin instances which I explicitly want to handle in git master and
>>> then merge back to 4.1 as a backport -- we only have any chance to
>>> encounter this case with current git master.
>>>
>>>> So if existing active UID uniqueness is there, should we just remove uid from
>>>> it's list of watched attributes, give warning and add our own controlled
>>>> plugin?
>>> No. There could be multiple uid uniqueness plugin instances, looking for
>>> different subtrees, potentially with different top level object classes
>>> and different object class filtering.
>>>
>>>> I am just trying to get rid of clashes with custom user configuration.
>>> I understand that, I'll handle it in git master version. Right now we
>>> didn't set up any of uid uniqueness and chances touching any custom
>>> configuration like that are low, looking into my experience with
>>> existing deployments.
>>>
>>>
>>>> 4) Because of 3), when I enabled "attribute uniqueness" plugin before upgrade,
>>>> it did a strange franken-update and now I have an update plugin with DN
>>>> "cn=attribute uniqueness,cn=plugins,cn=config" but "cn: uid uniqueness: inside.
>>> Good catch. I'll fix this part.
>>>
>>>> (BTW these concerns is where my simplicity expectations came from ;-)
>>> Note that now you realize the whole deal is not that simple as you
>>> thought. ;)
>>>
>>> New patch attached.
>> Missed Makefile.am part.
>>
>
> The
> +        if not entries:
> path works looks ok.
>
> However, I am still concerned about the situation when you detect existing UID
> plugin.
>
> You basically create a new LDAP entry with the same DN and then try to
> overwrite the original one. I do not think this is how the LDAP framework
> works. Petr/Honza, is it?

It will work, but updating the old entry object and calling update_entry 
on it would be nicer.

The difference is that with the current approach, the generated modlist 
will contain MOD_REPLACE for every attribute in the entry, whereas 
updating the original entry will result in a modlist with 
MOD_ADD/MOD_DELETE only for the attributes which were actually modified.

-- 
Jan Cholasta