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

[Petr Viktorin]

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().


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.

Also look at the other uses of get() to see if you're doing the right thing.

I've looked at help(pysss_nss_idmap.getnamebysid). Shouldn't you use 
pysss_nss_idmap.NAME_KEY and TYPE_KEY?


0261:
Again you're using get() on MUST attributes.

0262:
ipalib/constants.py - No newline at end of file

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()

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.

In get_dn, why do you resolve only the last key?

In set_anchoruuid_from_dn, an entry is a dict of lists, use either 
single_value or put the dn[0].value in brackets.

0263:

Note that permission_filter_objectclasses are ORed together; I don't 
think you want ipaOverrideAnchor there.

You forgot to regenerate ACI.txt in this patch.

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.

You're misusing get() again.

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.

Instead of:
     anchor.split(':')[-1]
prefer:
     anchor.rpartition(':')[-1]
which does less work.

0267:
resolve_object_to_anchor: For the error message, use 
Object[...].handle_not_found()

0268 looks good

0270:
get_idoverride_dn: Use re.escape() when inserting literal strings in 
regexes.

-- 
Petr³