On 09/06/2012 01:13 PM, Tomas Babej wrote:
> On 09/05/2012 01:56 PM, Martin Kosek wrote:
>> On 09/03/2012 05:12 PM, Tomas Babej wrote:
>>> Hi,
>>>
>>> Both selinuxusermap-add and selinuxusermap-mod commands now behave
>>> consistently in not allowing user/host category or user/host members
>>> and HBAC rule being set at the same time. Also adds a bunch of unit
>>> tests that check this behaviour.
>>>
>>> https://fedorahosted.org/freeipa/ticket/2983
>>>
>>> Tomas
>>>
>> I found few issues with this patch:
>>
>> 1) Patch needs a rebase
>>
>> 2) Patch does not expect attributes to be set to None, i.e. to be left empty 
>> or
>> to be deleted, e.g.:
>>
>> # ipa selinuxusermap-add foo --selinuxuser=guest_u:s0 --usercat=all 
>> --hbacrule=
>> ipa: ERROR: HBAC rule and local members cannot both be set
>>
>> # ipa selinuxusermap-add foo --selinuxuser=guest_u:s0 --usercat=all
>> ----------------------------
>> Added SELinux User Map "foo"
>> ----------------------------
>>    Rule name: foo
>>    SELinux User: guest_u:s0
>>    User category: all
>>    Enabled: TRUE
>>
>> # ipa selinuxusermap-mod foo --usercat= --hbacrule=
>> ipa: ERROR: HBAC rule and local members cannot both be set
>>
>> # ipa selinuxusermap-mod foo --usercat=
>> -------------------------------
>> Modified SELinux User Map "foo"
>> -------------------------------
>>    Rule name: foo
>>    SELinux User: guest_u:s0
>>    Enabled: TRUE
>>
>> # ipa selinuxusermap-mod foo --hbacrule=foo
>> -------------------------------
>> Modified SELinux User Map "foo"
>> -------------------------------
>>    Rule name: foo
>>    SELinux User: guest_u:s0
>>    HBAC Rule: foo
>>    Enabled: TRUE
>>
>> # ipa selinuxusermap-mod foo --hbacrule= --usercat=all
>> ipa: ERROR: HBAC rule and local members cannot both be set
>>
>> All these validation failures are not valid.
>>
>> 3) Additionally, I think it would be more readable and less error prone that 
>> if
>> instead of this blob:
>>
>> +        are_local_members_to_be_set  = 'usercategory' in _entry_attrs or \
>> +                                       'hostcategory' in _entry_attrs or \
>> +                                       'memberuser' in _entry_attrs or \
>> +                                       'memberhost' in _entry_attrs
>>
>> You would use something like that:
>>
>> are_local_members_to_be_set  = any(attr in _entry_attrs
>>                                     for attr in ('usercategory',
>>                                                  'hostcategory',
>>                                                  'memberuser',
>>                                                  'memberhost'))
>>
>> Martin
> 1.) Done.
> 2.) Corrected.
> 3.) Fixed.
> 
> Tomas

1) There are some (corner) cases where this approach still does not work:

# ipa selinuxusermap-show foo
  Rule name: foo
  SELinux User: guest_u:s0
  HBAC Rule: foo
  Enabled: TRUE
# ipa selinuxusermap-mod foo --usercat=all --hbacrule=
ipa: ERROR: HBAC rule and local members cannot both be set

HBAC rule attribute is being deleted and user category set, so this should not
be rejected.

2) There are also some styling issues (you can use pep8 tool present in Fedora
to locate them on your own, e.g.:

ipalib/plugins/selinuxusermap.py:247:32: E203 whitespace before ':'
ipalib/plugins/selinuxusermap.py:247:70: E225 missing whitespace around operator
ipalib/plugins/selinuxusermap.py:249:36: E221 multiple spaces before operator
...

Martin

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to