[Freeipa-devel] [PATCHES] 0473-0477+0497 Managed permission updater, part 1

[Petr Viktorin]

On 03/24/2014 03:43 PM, Martin Kosek wrote:
> On 03/14/2014 04:27 PM, Petr Viktorin wrote:
>> On 03/13/2014 02:01 PM, Petr Viktorin wrote:
>>> On 03/07/2014 10:45 AM, Martin Kosek wrote:
>>>> On 03/05/2014 01:48 PM, Petr Viktorin wrote:
>>>>> On 03/03/2014 04:10 PM, Petr Viktorin wrote:
>>>>>> On 02/28/2014 02:47 PM, Petr Viktorin wrote:
>>>>>>> On 02/28/2014 02:12 PM, Martin Kosek wrote:
>>>>>>>> On 02/26/2014 10:44 AM, Petr Viktorin wrote:
>>>>>>>>> Hello,
>>>>>>>>> Here are a few fixes/improvements, and the first part of a managed
>>>>>>>>> permission
>>>>>>>>> updater.
>>>>>>>>>
>>>>>>>>> The patches should go in this order but don't need to be
>>>>>>>>> ACKed/pushed
>>>>>>>>> all at once.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Design:
>>>>>>>>> http://www.freeipa.org/page/V3/Managed_Read_permissions#Default_Permission_Updater
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Part of the work for: https://fedorahosted.org/freeipa/ticket/3566
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This part is a "preview" of sorts, to get the basic mechanism and
>>>>>>>>> the
>>>>>>>>> metadata
>>>>>>>>> format reviewed before I add all of the default read permissions.
>>>>>>>>> It implements the first section of "Default Permission Updater" in
>>>>>>>>> the design;
>>>>>>>>> "Replacing legacy default permissions" and "Removing the global
>>>>>>>>> anonymous read
>>>>>>>>> ACI" is left for later.
>>>>>>>>> Metadata is added for the netgroup plugin* for starters
>>>>> [...]
>>>>>>>>>
>>>>>>>>
>>>>>>>> 1) 476: Typo in test name:
>>>>>>>>
>>>>>>>> +            desc='Search fo rnonexisting permission with ":" in the
>>>>>>>> name',
>>>>>>>
>>>>>>> Will fix.
>>>>>
>>>>> Fixed
>>>>>
>>>>>>>> 2) 477: do we want to log anything when permission is up to date?
>>>>>>>>
>>>>>>>> +            try:
>>>>>>>> +                ldap.update_entry(entry)
>>>>>>>> +            except errors.EmptyModlist:
>>>>>>>> +                return
>>>>>>>
>>>>>>> I don't think that's needed, after all that's the expected behavior in
>>>>>>> all but the first upgrade.
>>>>>>> But I'll be happy to add it if you think it would be better.
>>>>>
>>>>> I've added a DEBUG message here.
>>>>>
>>>>> [...]
>>>>>>>> 4) I have been quite resilient to the prefixes for the permissions,
>>>>>>>> but it
>>>>>>>> seems it is the easier possible approach to fix conflicts with user
>>>>>>>> permissions
>>>>>>>> without having to check that later for every upgrade or doing more
>>>>>>>> complex
>>>>>>>> stuff like multiple RDNs or different container for system and user
>>>>>>>> permissions.
>>>>>>>>
>>>>>>>> I am now just thinking about the prefixing. Now you use this name:
>>>>>>>>
>>>>>>>> ipa:Read Netgroups
>>>>>>>>
>>>>>>>> Would it be "nicer" to use:
>>>>>>>>
>>>>>>>> IPA:Read Netgroups
>>>>>>>> or
>>>>>>>> IPA: Read Netgroups
>>>>>>>> or even
>>>>>>>> [IPA] Read Netgroups
>>>>>>>> ? :-)
>>>>>>>
>>>>>>> Bikeshedding time!
>>>>>>> Everyone on the list, please chime in!
>>>>>>
>>>>>> Bikeshedding results from today's meeting:
>>>>>>
>>>>>> "ipa: " pviktori++
>>>>>> "System: " mkosek++ simo+ ab++
>>>>>> "Builtin: " simo++ pvo+
>>>>>> "Default:"
>>>>>>
>>>>>> The winner is "System: ", so I'll go and change to that.
>>>>>
>>>>> Done.
>>>>
>>>> Thanks. The patch set looks good now, I just see that the update
>>>> process may
>>>> not be optimal After I run the upgrade second time, it still tries to
>>>> update
>>>> the ACI even though there was no change done to the permission object and
>>>> should thus raise errors.EmptyModlist and skip the ACI update:
>>>>
>>>> 2014-03-07T09:44:34Z INFO Updating managed permissions for netgroup
>>>> 2014-03-07T09:44:34Z INFO Updating managed permission: System: Read
>>>> Netgroups
>>>> 2014-03-07T09:44:34Z DEBUG Updating ACI for managed permission:
>>>> System: Read
>>>> Netgroups
>>>> 2014-03-07T09:44:34Z DEBUG Removing ACI u'(targetattr = "cn ||
>>>> description ||
>>>> hostcategory || ipaenabledflag || ipauniqueid || nisdomainname ||
>>>> usercategory")(targetfilter = "(objectclass=ipanisnetgroup)")(version
>>>> 3.0;acl
>>>> "permission:System: Read Netgroups";allow (read,compare,search) userdn =
>>>> "ldap:///all";)' from
>>>> cn=ng,cn=alt,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
>>>> 2014-03-07T09:44:34Z DEBUG Adding ACI u'(targetattr = "cn ||
>>>> description ||
>>>> hostcategory || ipaenabledflag || ipauniqueid || nisdomainname ||
>>>> usercategory")(targetfilter = "(objectclass=ipanisnetgroup)")(version
>>>> 3.0;acl
>>>> "permission:System: Read Netgroups";allow (read,compare,search) userdn =
>>>> "ldap:///all";)' to
>>>> cn=ng,cn=alt,dc=idm,dc=lab,dc=eng,dc=brq,dc=redhat,dc=com
>>>> 2014-03-07T09:44:34Z INFO No changes to ACI
>>>> 2014-03-07T09:44:34Z INFO Updating managed permission: System: Read
>>>> Netgroup
>>>> Membership
>>>>
>>>> Any idea what could cause this?
>>>
>>> Ah, you're right. The updater tried to remove the 'top' objectclass,
>>> since it found that in the permission but it wasn't in
>>> permission.object_class.
>>> I added 'top' to the list in a new patch.
>>>
>>> There was a minor conflict with master; I'm attaching the whole rebased
>>> patchset for convenience.
>>
>> Rebased again.
>>
>
> I see the last issue I found is fixed - works fine. ACK for all patches after
> you fix another VERSION file conflict.
>
> Thanks,
> Martin
>

Thank you for the review!
Pushed to master: c5e61c85e626da61180f84bc80e294ab0eb3757a


-- 
Petr³