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

Mon Sep 29 14:30:04 UTC 2014

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 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140929/2dc834a2/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0279-3-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/20140929/2dc834a2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0278-3-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/20140929/2dc834a2/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0277-3-idviews-Handle-Default-Trust-View-properly-in-the-fr.patch
Type: text/x-patch
Size: 3008 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140929/2dc834a2/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0276-3-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/20140929/2dc834a2/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0273-3-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/20140929/2dc834a2/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0275-3-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/20140929/2dc834a2/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0274-3-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/20140929/2dc834a2/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0272-3-idviews-Add-ipaOriginalUid.patch
Type: text/x-patch
Size: 10765 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140929/2dc834a2/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0268-3-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/20140929/2dc834a2/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0267-3-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/20140929/2dc834a2/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0266-3-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/20140929/2dc834a2/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0265-3-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/20140929/2dc834a2/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0264-3-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/20140929/2dc834a2/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0263-3-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/20140929/2dc834a2/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0261-3-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/20140929/2dc834a2/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0262-3-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/20140929/2dc834a2/attachment-0015.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0270-3-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/20140929/2dc834a2/attachment-0016.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0259-3-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/20140929/2dc834a2/attachment-0017.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0256-3-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/20140929/2dc834a2/attachment-0018.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0257-3-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/20140929/2dc834a2/attachment-0019.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0258-3-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/20140929/2dc834a2/attachment-0020.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0255-3-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/20140929/2dc834a2/attachment-0021.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0253-3-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/20140929/2dc834a2/attachment-0022.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0251-3-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/20140929/2dc834a2/attachment-0023.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0252-3-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/20140929/2dc834a2/attachment-0024.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0249-3-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/20140929/2dc834a2/attachment-0025.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0250-3-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/20140929/2dc834a2/attachment-0026.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0248-3-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/20140929/2dc834a2/attachment-0027.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0247-3-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/20140929/2dc834a2/attachment-0028.bin>