Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
JR Aquino wrote: On Jun 16, 2011, at 8:01 AM, Rob Crittenden wrote: JR Aquino wrote: On Jun 15, 2011, at 8:03 AM, Rob Crittenden wrote: 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). Is there a rule of thumb or document that details when this is appropriate? 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 As per IRC conversation: Added new Exception: AttrValueNotFound Incremented minor version in VERSION Adjusted API 1276 (Raise AttrValueNotFound when trying to remove a non-existent option from Sudo rule) 1277 (Raise DuplicateEntry Error when adding a duplicate sudo option) 1308 (Make sudooption a required option for sudorule_remove_option) This is very close, found a couple more issues: I don't think I was very clear in what to update in VERSION, you want it to look like this: diff --git a/VERSION b/VERSION index 6cbf732..e31f0d0 100644 --- a/VERSION +++ b/VERSION @@ -79,4 +79,4 @@ IPA_DATA_VERSION=2010061412 # # IPA_API_VERSION_MAJOR=2 -IPA_API_VERSION_MINOR=5 +IPA_API_VERSION_MINOR=6 Two tests are failing. One is failing because externalhost is returned as a tuple (rather than not at all). The second because sudorule_remove_option has changed the type of data being returned. rob Ok, the VERSION issue is resolved, and the ipasudoopt test issue is solved. I have created: https://fedorahosted.org/freeipa/ticket/1339 to address the externalhost tuple as it is separate from the sudo options effort. ack, pushed to master ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
On Jun 16, 2011, at 8:01 AM, Rob Crittenden wrote: > JR Aquino wrote: >> On Jun 15, 2011, at 8:03 AM, Rob Crittenden wrote: >> >>> 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). >> >> Is there a rule of thumb or document that details when this is appropriate? >> >> >>> 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 >> >> >> As per IRC conversation: >> Added new Exception: AttrValueNotFound >> Incremented minor version in VERSION >> Adjusted API >> 1276 (Raise AttrValueNotFound when trying to remove a non-existent option >> from Sudo rule) >> 1277 (Raise DuplicateEntry Error when adding a duplicate sudo option) >> 1308 (Make sudooption a required option for sudorule_remove_option) >> > > This is very close, found a couple more issues: > > I don't think I was very clear in what to update in VERSION, you want it to > look like this: > > diff --git a/VERSION b/VERSION > index 6cbf732..e31f0d0 100644 > --- a/VERSION > +++ b/VERSION > @@ -79,4 +79,4 @@ IPA_DATA_VERSION=2010061412 > # # > > IPA_API_VERSION_MAJOR=2 > -IPA_API_VERSION_MINOR=5 > +IPA_API_VERSION_MINOR=6 > > Two tests are failing. One is failing because externalhost is returned as a > tuple (rather than not at all). The second because sudorule_remove_option has > changed the type of data being returned. > > rob Ok, the VERSION issue is resolved, and the ipasudoopt test issue is solved. I have created: https://fedorahosted.org/freeipa/ticket/1339 to address the externalhost tuple as it is separate from the sudo options effort. binL1EGmvO8nL.bin Description: freeipa-jraquino-0029-Raise-DuplicateEntry-Error-when-adding-a-duplicate.patch ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
JR Aquino wrote: On Jun 15, 2011, at 8:03 AM, Rob Crittenden wrote: 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). Is there a rule of thumb or document that details when this is appropriate? 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 As per IRC conversation: Added new Exception: AttrValueNotFound Incremented minor version in VERSION Adjusted API 1276 (Raise AttrValueNotFound when trying to remove a non-existent option from Sudo rule) 1277 (Raise DuplicateEntry Error when adding a duplicate sudo option) 1308 (Make sudooption a required option for sudorule_remove_option) This is very close, found a couple more issues: I don't think I was very clear in what to update in VERSION, you want it to look like this: diff --git a/VERSION b/VERSION index 6cbf732..e31f0d0 100644 --- a/VERSION +++ b/VERSION @@ -79,4 +79,4 @@ IPA_DATA_VERSION=2010061412 # # IPA_API_VERSION_MAJOR=2 -IPA_API_VERSION_MINOR=5 +IPA_API_VERSION_MINOR=6 Two tests are failing. One is failing because externalhost is returned as a tuple (rather than not at all). The second because sudorule_remove_option has changed the type of data being returned. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
On Jun 15, 2011, at 8:03 AM, Rob Crittenden wrote: > 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). Is there a rule of thumb or document that details when this is appropriate? > 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 As per IRC conversation: Added new Exception: AttrValueNotFound Incremented minor version in VERSION Adjusted API 1276 (Raise AttrValueNotFound when trying to remove a non-existent option from Sudo rule) 1277 (Raise DuplicateEntry Error when adding a duplicate sudo option) 1308 (Make sudooption a required option for sudorule_remove_option) binr2ad1uNgGK.bin Description: freeipa-jraquino-0029-Raise-DuplicateEntry-Error-when-adding-a-duplicate.patch ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
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.
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
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
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.
>>>
>>>
>>
>>
>> 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.
binXCkevccW1o.bin
Description: freeipa-jraquino-0029-Raise-DuplicateEntry-Error-when-adding-a-duplicate.patch
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
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.
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
I also noticed that ipasudoopt doesn't have a label and isn't shown in
the rule by default.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
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. > > 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") binJhSIDaySFB.bin Description: freeipa-jraquino-0029-Raise-DuplicateEntry-Error-when-adding-a-duplicate.patch ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
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. binlpJMvoMsfZ.bin Description: freeipa-jraquino-0029-Raise-DuplicateEntry-Error-when-adding-a-duplicate.patch ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 29 Raise DuplicateEntry Error when adding a duplicate sudo option
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? rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
