[Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option

[Rob Crittenden]

JR Aquino wrote:
> On Jun 14, 2011, at 11:06 AM, Rob Crittenden wrote:
>
>> JR Aquino wrote:
>>> On Jun 10, 2011, at 3:11 PM, JR Aquino wrote:
>>>
>>>> On Jun 9, 2011, at 10:24 AM, Rob Crittenden wrote:
>>>>
>>>>> JR Aquino wrote:
>>>>>> https://fedorahosted.org/freeipa/ticket/1277
>>>>>>
>>>>>> Raise DuplicateEntry Error when adding a duplicate sudo option
>>>>>
>>>>> nack, this will still fail if no ipasudoopt is passed in.
>>>>>
>>>>> Also, is this case-sensitive?
>>>>
>>>> Yes, it is case sensitive (Example: sudoOption: env_keep+=SSH_AUTH_SOCK)
>>>>
>>>> Here is an adjusted patch to account for no ipasudoopt as well as an empty space.
>>>>
>>>> <freeipa-jraquino-0029-Raise-DuplicateEntry-Error-when-adding-a-duplicate.patch>
>>>
>>>
>>> Minor correction: Addressed the 1 character change needed to address #1276
>>>
>>> Added notes to indicate this patch fixes:
>>> #1276 (Removed option from Sudo rule message is displayed even when the given option doesn't exist.)
>>> #1277 (Added option to Sudo rule message is displayed even when the given option already exists.)
>>> #1308 (Internal error while removing sudorule option without "--sudooption")
>>>
>>
>> NACK
>>
>> $ ipa sudorule-add test
>> ----------------------
>> Added sudo rule "test"
>> ----------------------
>>   Rule name: test
>>   Enabled: TRUE
>> $ ipa sudorule-remove-option test --sudooption=foo
>> -----------------------
>> sudorule-remove-option:
>> -----------------------
>>   Rule name: test
>> ipa: ERROR: KeyError: 'ipasudoopt'
>> Traceback (most recent call last):
>>   File "/home/rcrit/redhat/freeipa-master/ipalib/cli.py", line 1141, in run
>>     sys.exit(api.Backend.cli.run(argv))
>>   File "/home/rcrit/redhat/freeipa-master/ipalib/cli.py", line 965, in run
>>     rv = cmd.output_for_cli(self.api.Backend.textui, result, *args, **options)
>>   File "/home/rcrit/redhat/freeipa-master/ipalib/plugins/sudorule.py", line 675, in output_for_cli
>>     textui.print_attribute('Sudo Options', result['result']['ipasudoopt'])
>> KeyError: 'ipasudoopt'
>> ipa: ERROR: an internal error has occurred
>>
>> Is this legal?
>>
>> $ ipa sudorule-add-option test --sudooption=foo
>> --------------------
>> sudorule-add-option:
>> --------------------
>>   Rule name: test
>>   Sudo Options: foo
>> $ ipa sudorule-add-option test --sudooption=foo
>> ipa: ERROR: This entry already exists
>> $ ipa sudorule-add-option test --sudooption=FOO
>> --------------------
>> sudorule-add-option:
>> --------------------
>>   Rule name: test
>>   Sudo Options: foo
>>   Sudo Options: FOO
>
> This is legal ^ Or if you like double negatives, this is not illegal.
>
> However, the only options that will be respected are listed: http://www.gratisoft.us/sudo/man/1.8.1/sudoers.man.html in the SUDOERS OPTIONS section. Some of the values can be singular like:
> "sudoOption: !authenticate" which will allow you to run sudo without a password or "sudoOption: iolog_dir=/var/log/sudo-playback"
>
>>
>> I also noticed that ipasudoopt doesn't have a label and isn't shown in the rule by default.
>
> Here is a corrected patch to address the KeyError and the display issue.
>

A minor issue and a question.

The minor issue is you changed a couple of options from optional to 
mandatory, which is fine, but we need to bump up the minor version in 
VERSION (older clients otherwise could not send the string and blow 
things up).

The question is, should we raise EmptyModList() when removing an option 
that doesn't exist or NotFound(reason=_())? I think the second might be 
more explanatory but might be harder for handle in scripts (how would 
you distinguish between entry not found and option not found)?

rob