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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to