[Freeipa-devel] [PATCH] 14 ipa permission-add does not fail if using invalid attribute

[Ondrej Hamada]

On 02/28/2012 09:57 PM, Rob Crittenden wrote:
> Ondrej Hamada wrote:
>> On 02/27/2012 03:22 PM, Rob Crittenden wrote:
>>> Ondrej Hamada wrote:
>>>> When adding or modifying permission with both type and attributes
>>>> specified, check whether the attributes are allowed for specified 
>>>> type.
>>>> In case of disallowed attributes the InvalidSyntax error is raised.
>>>>
>>>> New tests were also added to the unit-tests.
>>>>
>>>> https://fedorahosted.org/freeipa/ticket/2293
>>>>
>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>
>>> NACK. You should use obj.object_class_config to determine if the
>>> default list of objectclasses comes from LDAP.
>>>
>>> I think that may be it, otherwise the patch reads ok.
>>>
>>> I'm very glad to see unit tests!
>>>
>>> rob
>> Corrected
>>
>
> Sorry, found a couple of more things I should have found the first 
> review.
>
> Please use the dn module to construct dn_ipaconfig. Or you can also 
> get the DN on-the-fly since the config object using get_dn().
>
> Probably just as safe to call: if obj.object_class_config: ... rather 
> than hasattr. I suppose its just a style thing.
Done.
>
> I wonder if ObjectclassViolation is a better exception. SyntaxError 
> means the data type is wrong, not that it isn't allowed.
I agree that it makes more sense and I've updated the patch that way, 
but the documentation says: "permission operation fails with schema 
syntax errors" - maybe we should also update the documentation.
>
> rob
>
>


-- 
Regards,

Ondrej Hamada
FreeIPA team
jabber: ohama at jabbim.cz
IRC: ohamada

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ohamada-14-3-Validate-attributes-in-permission-add.patch
Type: text/x-patch
Size: 6624 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120229/bc49e608/attachment.bin>