[Freeipa-devel] [PATCH] 1112 Add service constraint delegation plugin

[Rob Crittenden]

Martin Basti wrote:
> On 20/05/15 18:02, Rob Crittenden wrote:
>> Rob Crittenden wrote:
>>> Rob Crittenden wrote:
>>>> Add a plugin to manage service delegations, like the one allowing the
>>>> HTTP service to obtain an ldap service ticket on behalf of the user.
>>>>
>>>> This does not include impersonation targets, so one cannot yet limit by
>>>> user what tickets can be obtained.
>>>>
>>>> There is also no referential integrity for the memberPrincipal
>>>> attribute
>>>> since it is a string and not a DN. I don't see a way around this that
>>>> isn't either clunky or requires a 389-ds plugin, both of which are
>>>> overkill in this case IMHO.
>>>>
>>>> If you wonder why all the overrides it's because all of this is stored
>>>> in the same container, and membership-like functions are used for a
>>>> non-DN attribute (memberPrincipal).
>>>>
>>>> I used Alexander's patch in the ticket as a jumping off point.
>>>
>>> Removed a couple of hardcoded domain/realm elements in the tests.
>>
>> I must be getting rustly. Forgot to include ACIs. Added now.
>>
>> rob
>>
>>
>>
> Thank you.
>
> I haven't finished review yet, but I have few notes in case you will
> modify the patch.
>
> Please fix following issues:
>
> 1) Patch needs rebase, VERSION conflict
>
> 2)
> +            pattern='^[a-zA-Z0-9_.][ a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.-]?$',
> +            pattern_errmsg='may only include letters, numbers, _, -, .,
> and a space inside',
> +            maxlength=255,
>
> If I count correctly, regexp allows only 254 characters, not 255, and
> this regexp also allows the space at the end of the string.
>
> IMHO '^[a-zA-Z0-9_.]([ a-zA-Z0-9_.-]*[a-zA-Z0-9_.-])?$' would work.

This is a direct copy from many places. I'd file a bug to fix all of 
them I guess.

> 3)
> There are many PEP8 errors, can you fix some of them,?

Is PEP8 a concern? What kinds of errors do we fix? For example, the 
current model for defining options generates a slew of indention errors.

> 4)
> Please use
> except Exception as e: to be compatible with python 3

ok

>
> 5)
> For new files we stared using shorter license header.
> #
> # Copyright (C) 2015  FreeIPA Contributors see COPYING for license
> #

ok

I'll wait and see what falls out of the API review before making any 
real changes.

rob