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

[Rob Crittenden]

Pavel Zuna wrote:
> 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.

This is a general problem in the framework right now, not specific to 
ACIs. We currently have no easy way to specify "remove this attribute" 
or "remove this attribute value".

In the context of an ACI you could end up with targets you don't want.

ACI's have a couple of different ways to target an ACI. You can set it 
to operate on a whole subtree (ldap:///uid=*, cn=users, ...) or a 
filter, such as being a member of a group 
(targetfilter="(memberOf=cn=bar,cn=groups...).

So if when you created the ACI you had set a list of attributes you 
wanted to apply it to but then changed your mind, there is no way to 
remove them other than to drop the ACI and re-create it. When I 
initially worked on this I decided that is probably what most admins 
would do anyway but I left the mod command as a placeholder just in case 
we got to it.

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

You have a say!

But yes, manipulating ACIs can be dangerous. What I want to avoid is 
making it too easy to grant read access to passwords or overly broad 
write access, for example. What I hope to provide for in the new 
delegation system is enough low-level rights to manage the various 
components (users, groups, hosts, etc) that most of the work can be done 
at the "role" level where you only worry about defining who can do what, 
not what can be done.

I think we may need a way to manage the list of attributes that can be 
operated on which is why I haven't completely dumped the delegation 
plugin yet, but I'm just not sure that we want to maintain both methods.

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090603/7126d968/attachment.bin>