[Freeipa-devel] [PATCHES] Fix bug where List parameters were always cloned with keywords parsed from name. + Remove unused reference to old LDAP backend in join plugin. + Make delegation plugin consistent with plugin2 and use new Crud methods. + Fix DS ACI parsing. + Add ACI plugin port to new LDAP backend.

[Pavel Zuna]

Rob Crittenden wrote:
> Pavel Zuna wrote:
>> Patch 0001: Fix bug where List parameters were always cloned with 
>> keywords parsed from name.
>>
>> For example, required List options were always cloned as required in 
>> crud.Search.
>>
>>
>> Patch 0002: Remove unused reference to old LDAP backend in join plugin.
>>
>>
>> Patch 0003: Make delegation plugin consistent with plugin2 and use new 
>> Crud methods.
>>
>> The delegation plugin doesn't do anything at this point, but I think 
>> we should get rid of the old Crud methods.
>>
>>
>> Patch 0004: Fix DS ACI parsing.
>>
>> Parsing of ACI strings was faulty and didn't want to make friends with 
>> unicode.
>>
>>
>> Patch 0005: Add ACI plugin port to new LDAP backend.
>>
>> It's more of a complete rewrite actually. I dropped the showall 
>> command, use aci2-find with no parameters instead.
>>
>>
>> Pavel
> 
> As a separate issue to the team, we have all been doing multiple patches 
> per e-mail lately. I've found this to be difficult to review from time 
> to time, particularly when one part of the patch gets nacked but the 
> rest are ok (but perhaps related to the nack patch). Do we want to go 
> back to 1-patch-per-e-mail?
Fine by me.

> Patch 1-4 look fine. Pushed.
> 
> I think the delegation plugin is going to go away though. It just does 
> the "grant write on these attributes for group A to group B". The aci 
> plugin can do much more (and is more dangerous too).
> 
> Patch 5 nack.
> 
> I think we really need a ListEnum but that's another story :-)
Actually, it might be another story, but it's related to the next issue. 
ListEnum would validate its values automatically. I was thinking about that and 
even tried to implement it, but then gave up, because sub-classing from both 
List and Enum was a bit tricky.

> Instead of normalize I think this should use a validate function for the 
> permission types and not continue if unknown values are used. This 
> silently drops unknown values so could have unknown side-effects.
The problem is that the List type overrides Param._validate_scalar and performs 
no validation of values at all (validate functions are not called). The only way 
around this I can think of now is to raise ValidationError in the current 
normalize function... or rewrite the List type.

> You're missing the dn argument when setting the target filter:
> 
> +    if 'memberof' in kw:
> +        (dn, entry_attrs) = api.Command['group2_show'](kw['memberof'])
> +        a.set_target_filter('memberOf=%s')
Ok.

> The reason I didn't implement mod is that we don't currently have a way 
> to say "remove this thing". So if you had an ACI that limited access by 
> attributes there is no way to remove that. I suppose something is better 
> than teasing with a command that is not implemented at all though.
I'm not sure I understand what you mean exactly, but my knowledge of ACIs and DS 
in general is limited. What I make of it is, that it's impossible to delete ACIs 
in certain cases. Because what I actually do in the -mod command is deleting the 
old ACI and creating a new one to take its place.

> I'm still not sure I want to ship this with as a general-use plugin. 
> I've used it to create the current set of ACIs but it is not for the 
> faint of heart, that's for sure.
I can see your point and it's not my decision to make anyway. I think that most 
admins will rather manipulate ACIs manually for the sake of safety.

> rob

Pavel