[Freeipa-devel] [PATCHES 247-281] ID views - management part

Tue Sep 30 07:56:15 UTC 2014

Attaching updated patchset with resolved objections from Petr^1 and Petr^3.

(three more patches attached)

On 09/29/2014 04:30 PM, Tomas Babej wrote:
> Thanks for the review.
>
> Sending updated patchset, responses are inline.
>
> On 09/23/2014 10:07 AM, Petr Viktorin wrote:
>> On 09/16/2014 01:21 PM, Tomas Babej wrote:
>>> Petr,
>>>
>>> thanks for the review, your input is, as always, much to the point.
>>>
>>> My responses are inline. Also, I'm attaching a new patchset, rebased on
>>> master, please, have a look at that.
>>>
>>> Most of the patches have at least minor changes, since I rebased
>>> another
>>> ~15 patches into this patchset, and there are additional 9 patches on
>>> top. Patch 254 was deprecated, as we decided to go with more explicit
>>> way of having two separate commands for user and group ID overrides.
>>>
>>> Also, the test suite (around 80 tests) for ID views is attached - patch
>>> 270. It depends on patch 269, sent in separate thread.
>>>
>>> Tomas
>>>
>>> On 09/10/2014 03:10 PM, Petr Viktorin wrote:
>>>> On 08/01/2014 12:30 PM, Tomas Babej wrote:
>>>>> Hi,
>>>>>
>>>>> the following set of patches implements the ID view creation and
>>>>> management of views and ID overrides in IPA.
>>>>>
>>>>> Pending questions:
>>>>> 1.) The patch 253 implements basic managed permissions for ID
>>>>> views and
>>>>> ID overrides. Do we want to have a separate permission for
>>>>> assigning ID
>>>>> views?
>>>>> 2.) Performance: idview-apply command takes ~100 seconds for
>>>>> hostgroups
>>>>> with 1000 member hosts. I am able to lower that by 20-30% using
>>>>> raw ldap
>>>>> calls, is bypassing the framework worth the performance gain?
>>>>> We'll lose
>>>>> the possiblity to hook on exception callbacks.
>>>>>
>>>>> The commands also need more helpful documentation, I am working on
>>>>> that.
>>>>>
>>>>
>>>> Not tested yet, but here are my comments on the patches:
>>>>
>>
>> 0247 looks good
>>
>> In 0248 you deleted a newline at EOF in 71-idviews.update
>>
>> 0249 looks good
>>
>>>>
>>>> 0250:
>>>> With so many names imported from one module, I find it more readable
>>>> to `from ipalib.plugins import baseldap`, and use qualified names
>>>> later. Same in the 2 other patches.
>>>> This is personal preference/tip, feel free to disagree :)
>>>
>>> I was just trying to be consistent here, rest of the IPA plugins
>>> seem to
>>> use non-qualified names for plugins. Even tough the main reason is
>>> probably that thay use evil star imports :) Let's keep it this way
>>> for now.
>>
>> Yes, the rest of the plugins are using star imports; no consistency
>> to be kept here.
>> I think that after these patches nobody will touch it again, so "for
>> now" is basically forever.
>>
>> 0521 looks good
>>
>>>> 0252:
>>
>>>> The pattern_errmsg in idoverride's uid should be expanded to fully
>>>> describe the pattern.
>>>
>>> If we want to expand it, then it should be corrected in user.py plugin
>>> too (I have taken it from there). Still, it's a tradeoff between
>>> conciseness and and exactness, do we really want to specify that user
>>> login may be at most 253 letters long or the $ character may only be
>>> used as terminal one? Or the fact that - can't be used at the
>>> beggining?
>>> This is probably the way it was since forever in user plugin.
>>
>> I think the message should at be free of false positives. Using e.e.
>> "-test-name-" and getting back "may only include letters, numbers, _,
>> -, . and $" would be extremely frustrating.
>>
>> Should I file a ticket?
>>
>> 0253 looks good
>> 0255-0256 looks good
>>
>>>> 0258:
>>>> idview_apply: typo in hostgroup doc, should be "hosts to apply the ID
>>>> view to" and "after running the idview-apply command".
>>>> Typos in output params, should be e.g. "this ID view"
>>>
>>> Fixed the second and third complaint ( IMHO this is clear from context,
>>> but I added "this" to be explicit), I'm not so sure about the first one
>>> though. Can we get a native speaker input on that?
>>
>> Actually both just need an article, doesn't matter much if it's "the"
>> or "this".
>>
>>>>
>>>> By subclassing idview_unapply from idview_apply you're violating
>>>> Liskov's substitution principle. Make a common base class for them.
>>>
>>> Done.
>>
>> In the wrong patch, it seems :)
>>
>>>>
>>>> 0259:
>>>> show_id_overrides: cn is a MUST attribute in ipa*Override; use
>>>> view.single_value['cn'] instead of get(). In enumerate_hosts, is None
>>>> okay if cn is missing?
>>>
>>> This was actually an error, fixed in updated version of patches. New
>>> code uses 'ipaanchoruuid' (which is a MUST) and converts it (with your
>>> objections addressed.)
>>
>> If it's MUST, use simply `single_value['ipaanchoruuid']` instead of
>> get().
>>
> Yes, this is fixed in a later patch.
>
>
>
>>
>> 0260:
>> The get() method on dict-like objects returns None (or a given
>> default) if the item is missing.
>> If sAMAccountName is MUST, use regular dict item access.
>> If sAMAccountName is MAY, then you're not handling the None you might
>> get.
>
> It is a must, fixed in 260.
>
>
>
>>
>> Also look at the other uses of get() to see if you're doing the right
>> thing.
>
> Patch 276 makes sure this does not happen.
>
>
>
>>
>> I've looked at help(pysss_nss_idmap.getnamebysid). Shouldn't you use
>> pysss_nss_idmap.NAME_KEY and TYPE_KEY?
>
> Right. Also, there forgotten debubugging comment somehow lurked in, I
> got that fixed too.
>
>
>>
>>
>> 0261:
>> Again you're using get() on MUST attributes.
>
> Fixed.
>
>>
>> 0262:
>> ipalib/constants.py - No newline at end of file
>
> Fixed.
>
>
>>
>> Is IPA_ANCHOR_PREFIX and SID_ANCHOR_PREFIX to be used outside
>> idviews.py? Do we want it in constants?
>>
>>> +        if anchor.startswith(IPA_ANCHOR_PREFIX):
>>> +            uuid = anchor.split(IPA_ANCHOR_PREFIX)[1].strip()
>
> I thought of constants.py as a global store for all plugin constants.
>
>
>>
>> This is a dangerous idiom to use for removing a prefix, since it will
>> silently give wrong results if the string contains the prefix. Prefer:
>>     anchor[len(IPA_ANCHOR_PREFIX):]
>> Same with SID_ANCHOR_PREFIX below.
>
> This cannot happen unless someone edits the raw LDAP directly, but
> still, you're correct here, better not get used to using/seeing this.
> Fixed in later patch, thanks.
>
>
>
>>
>> In get_dn, why do you resolve only the last key?
>
> You can only pass two arguments to these commands, where the first one
> is the name of the view, which you don't want to resolve.
>
>
>>
>> In set_anchoruuid_from_dn, an entry is a dict of lists, use either
>> single_value or put the dn[0].value in brackets.
>
> This actually needs to stay this way, since LDAPUpdate expects a
> single value, not a list. I added a comment.
>
>
>
>>
>> 0263:
>>
>> Note that permission_filter_objectclasses are ORed together; I don't
>> think you want ipaOverrideAnchor there.
>>
> Right, fixed.
>
>> You forgot to regenerate ACI.txt in this patch.
>>
>
> True, fixed.
>
>> 0264 looks OK
>>
>> 0265:
>>
>> Instead of:
>>
>>> +            objectclass, name_attr = (
>>> +                ('posixaccount', 'uid')
>>> +                if self.override_object == 'user' else
>>> +                ('ipausergroup', 'cn')
>>> +            )
>>
>> prefer:
>>
>>     objectclass, name_attr = {
>>         'user': ('posixaccount', 'uid'),
>>         'group': ('ipausergroup', 'cn'),
>>     }[self.override_object]
>>
>> This generalizes better to more cases, and fails loudly on bad input.
>>
> Fixed.
>
>> You're misusing get() again.
>>
> Fixed.
>
>> 0266:
>>
>> To build strings, prefer formatting:
>>     return '%s%s:%s' % (IPA_ANCHOR_PREFIX, domain, uuid)
>> rather than adding the separator to one of the strings and
>> concatenating.
>>
> Fixed.
>
>> Instead of:
>>     anchor.split(':')[-1]
>> prefer:
>>     anchor.rpartition(':')[-1]
>> which does less work.
>>
> What is an ocean but a multitude of drops :) Fixed.
>
>> 0267:
>> resolve_object_to_anchor: For the error message, use
>> Object[...].handle_not_found()
>>
> Fixed.
>
>> 0268 looks good
>>
>> 0270:
>> get_idoverride_dn: Use re.escape() when inserting literal strings in
>> regexes.
>>
> Fixed, thanks.
>
> -- 
> Tomas Babej
> Associate Software Engineer | Red Hat | Identity Management
> RHCE | Brno Site | IRC: tbabej | freeipa.org 

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0279-4-idviews-Catch-errors-on-unsuccessful-AD-object-looku.patch
Type: text/x-patch
Size: 2025 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0278-4-idviews-Make-sure-the-dict.get-method-is-not-abused-.patch
Type: text/x-patch
Size: 2229 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0277-4-idviews-Handle-Default-Trust-View-properly-in-the-fr.patch
Type: text/x-patch
Size: 3530 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0276-4-idviews-Add-Default-Trust-View-as-part-of-adtrustins.patch
Type: text/x-patch
Size: 3434 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0275-4-idviews-Make-description-optional-for-the-ID-View-ob.patch
Type: text/x-patch
Size: 2826 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0274-4-idviews-Fix-casing-of-ID-Views-to-be-consistent.patch
Type: text/x-patch
Size: 7220 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0273-4-idviews-Update-the-referential-plugin-config-to-watc.patch
Type: text/x-patch
Size: 1739 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0272-4-idviews-Add-ipaOriginalUid.patch
Type: text/x-patch
Size: 10765 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0270-4-ipatests-Add-xmlrpc-tests-for-idviews-plugin.patch
Type: text/x-patch
Size: 49473 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0268-4-idviews-Resolve-anchors-to-object-names-in-idview-sh.patch
Type: text/x-patch
Size: 10987 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0267-4-idviews-Raise-NotFound-errors-if-object-to-override-.patch
Type: text/x-patch
Size: 1541 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0266-4-idviews-Change-format-of-IPA-anchor-to-include-domai.patch
Type: text/x-patch
Size: 2479 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0265-4-idviews-Alter-idoverride-methods-to-work-with-splitt.patch
Type: text/x-patch
Size: 5094 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0264-4-idviews-Split-the-idoverride-commands-into-iduserove.patch
Type: text/x-patch
Size: 17663 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0263-4-idviews-Split-the-idoverride-object-into-iduseroverr.patch
Type: text/x-patch
Size: 6901 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0262-4-idviews-Support-specifying-object-names-instead-of-r.patch
Type: text/x-patch
Size: 7782 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0261-4-trusts-Add-conversion-from-SID-to-object-name.patch
Type: text/x-patch
Size: 3139 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0259-4-idviews-Extend-idview-show-command-to-display-assign.patch
Type: text/x-patch
Size: 8412 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0258-4-idviews-Add-ipa-idview-apply-and-idview-unapply-comm.patch
Type: text/x-patch
Size: 9584 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0257-4-hostgroup-Selected-PEP8-fixes-for-the-hostgroup-plug.patch
Type: text/x-patch
Size: 2851 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0256-4-hostgroup-Remove-redundant-and-star-imports.patch
Type: text/x-patch
Size: 1279 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0020.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0255-4-hostgroup-Add-helper-that-returns-all-members-of-a-h.patch
Type: text/x-patch
Size: 978 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0021.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0253-4-idvies-Add-managed-permissions-for-idview-and-idover.patch
Type: text/x-patch
Size: 4094 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0022.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0252-4-idviews-Create-basic-idview-plugin-structure.patch
Type: text/x-patch
Size: 17680 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0023.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0251-4-ipalib-PEP8-fixes-for-host-plugin.patch
Type: text/x-patch
Size: 6958 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0024.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0250-4-ipalib-Remove-redundant-and-star-imports-from-host-p.patch
Type: text/x-patch
Size: 2623 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0025.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0249-4-idviews-Add-ipaAssignedIDVIew-reference-to-the-host-.patch
Type: text/x-patch
Size: 9760 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0026.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0248-4-idviews-Create-container-for-ID-views-under-cn-accou.patch
Type: text/x-patch
Size: 1875 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0027.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0247-4-idviews-Add-necessary-schema-for-the-ID-views.patch
Type: text/x-patch
Size: 4751 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0028.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0280-4-idviews-Display-the-list-of-hosts-when-using-all.patch
Type: text/x-patch
Size: 1603 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0029.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0282-4-idviews-Create-Default-Trust-View-for-upgraded-serve.patch
Type: text/x-patch
Size: 2542 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0030.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0281-4-idviews-Make-sure-only-regular-IPA-objects-are-allow.patch
Type: text/x-patch
Size: 1614 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/7269d537/attachment-0031.bin>