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

[Martin Basti]

On 27/05/15 19:27, Rob Crittenden wrote:
> 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.
I cannot find the same regexp in current code, there are only patterns 
without space, so the space issue is only in this patch.

Otherwise I agree to file a ticket to fix the length issue.
>
>> 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.
We try to keep PEP8 in new code, mainly indentation, blank lines, too 
long lines.
Yes in test definitions and option definitions, is better to keep the 
same style, but other parts of code should be PEP8.

For example these should be fixed
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:37:13: E225 
missing whitespace around operator
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:39:1: E302 
expected 2 blank lines, found 1
./ipatests/test_xmlrpc/test_serviceconstraint_plugin.py:42:1: E302 
expected 2 blank lines, found 1

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

Martin^2

-- 
Martin Basti