Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On Tue, 2012-06-12 at 14:38 +0200, Petr Viktorin wrote: > On 06/07/2012 11:37 AM, Petr Vobornik wrote: > > On 05/28/2012 04:16 PM, Martin Kosek wrote: > >> On Mon, 2012-05-28 at 15:46 +0200, Petr Vobornik wrote: > >>> On 05/25/2012 09:20 AM, Petr Vobornik wrote: > On 05/16/2012 02:11 PM, Martin Kosek wrote: > > On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote: > >> On 05/16/2012 09:58 AM, Martin Kosek wrote: > >>> On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: > On 05/15/2012 09:55 AM, Martin Kosek wrote: > > On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: > >> The final part of rejecting unknown Command arguments: enable the > >> validation, add tests. > >> Also fix up things that were changed since the previous patches. > >> > >> https://fedorahosted.org/freeipa/ticket/2509 > >>> > >>> 8><-- > >>> > >> > >> Attaching a rebased patch. > >> > > > > Yup, this one is fine. Now, I did not find issues in the patch itself, > > tests are clean. > > > > However, thanks to this new check I found issues in Web UI > > (automember, > > selfservice, delegation screen) which use illegal options and which > > should be fixed before we push your patch: > > > > https://fedorahosted.org/freeipa/ticket/2760 > > > > Martin > > > > I found an issue in automountmap_add_indirect. It complains that 'key' > is unknown option. > >>> > >>> I found another options which were functional and now it complains: > >>> * hbacsvcgroup_find: no_hbacsvc > >>> * hbacsvc_find: not_in_hbacsvcgroup > >>> * same issue in sudo commands and sudo command groups. > >>> > >>> I didn't check all relationships, so it may be broken elsewhere as well. > >>> > >> > >> I don't think this is an error on server side - it never had filter > >> options like these in the modules you referenced (though we may add them > >> as an RFE when needed). > >> > >> When you pass these options in the UI to the server side, its just NOOP > >> - or an error when Petr's patch is applied. > >> > >> Martin > >> > > All issues found in Web UI are fixed. > > > > Updated and rebased patch attached. I did not find any other issue, so ACK, pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On 06/07/2012 11:37 AM, Petr Vobornik wrote:
On 05/28/2012 04:16 PM, Martin Kosek wrote:
On Mon, 2012-05-28 at 15:46 +0200, Petr Vobornik wrote:
On 05/25/2012 09:20 AM, Petr Vobornik wrote:
On 05/16/2012 02:11 PM, Martin Kosek wrote:
On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote:
On 05/16/2012 09:58 AM, Martin Kosek wrote:
On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote:
On 05/15/2012 09:55 AM, Martin Kosek wrote:
On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote:
The final part of rejecting unknown Command arguments: enable the
validation, add tests.
Also fix up things that were changed since the previous patches.
https://fedorahosted.org/freeipa/ticket/2509
8><--
Attaching a rebased patch.
Yup, this one is fine. Now, I did not find issues in the patch itself,
tests are clean.
However, thanks to this new check I found issues in Web UI
(automember,
selfservice, delegation screen) which use illegal options and which
should be fixed before we push your patch:
https://fedorahosted.org/freeipa/ticket/2760
Martin
I found an issue in automountmap_add_indirect. It complains that 'key'
is unknown option.
I found another options which were functional and now it complains:
* hbacsvcgroup_find: no_hbacsvc
* hbacsvc_find: not_in_hbacsvcgroup
* same issue in sudo commands and sudo command groups.
I didn't check all relationships, so it may be broken elsewhere as well.
I don't think this is an error on server side - it never had filter
options like these in the modules you referenced (though we may add them
as an RFE when needed).
When you pass these options in the UI to the server side, its just NOOP
- or an error when Petr's patch is applied.
Martin
All issues found in Web UI are fixed.
Updated and rebased patch attached.
--
Petr³
From 172acc95052fc5c3e14f15dcec6bdaefcd42e303 Mon Sep 17 00:00:00 2001
From: Petr Viktorin
Date: Tue, 17 Apr 2012 12:42:35 -0400
Subject: [PATCH] Fail on unknown Command options
When unknown keyword arguments are passed to a Command, raise an
error instead of ignoring them.
Options used when IPA calls its commands internally are listed
in a new Command attribute called internal_options, and allowed.
Previous patches (0b01751c, c45174d6, c5689e7f) made IPA not use
unknown keyword arguments in its own commands and tests, but since
that some violations were reintroduced in permission_find and tests.
Fix those.
Tests included; both a frontend unittest and a XML-RPC test via the
ping plugin (which was untested previously).
https://fedorahosted.org/freeipa/ticket/2509
---
ipalib/frontend.py | 13 +--
ipalib/plugins/aci.py |2 ++
ipalib/plugins/automount.py |6 +++-
ipalib/plugins/permission.py| 47 +++-
tests/test_cmdline/test_cli.py |6 ++--
tests/test_ipalib/test_frontend.py |5 +++
tests/test_xmlrpc/test_host_plugin.py |2 +-
tests/test_xmlrpc/test_netgroup_plugin.py |6 ++--
tests/test_xmlrpc/test_permission_plugin.py | 12 +++
tests/test_xmlrpc/test_ping_plugin.py | 52 +++
10 files changed, 123 insertions(+), 28 deletions(-)
create mode 100644 tests/test_xmlrpc/test_ping_plugin.py
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 10087ba24bc310a036a22d4c52740825de3184ce..c28fa54ae933fe58833579b3e3aee0e1ad50f198 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -29,7 +29,8 @@
from output import Output, Entry, ListOfEntries
from text import _, ngettext
-from errors import ZeroArgumentError, MaxArgumentError, OverlapError, RequiresRoot, VersionError, RequirementError
+from errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
+RequiresRoot, VersionError, RequirementError, OptionError)
from errors import InvocationError
from constants import TYPE_ERROR
from ipapython.version import API_VERSION
@@ -404,6 +405,8 @@ class Command(HasParam):
output_params = Plugin.finalize_attr('output_params')
has_output_params = tuple()
+internal_options = tuple()
+
msg_summary = None
msg_truncated = _('Results are truncated, try a more specific search')
@@ -520,7 +523,13 @@ def __args_2_params(self, values):
def __options_2_params(self, options):
for name in self.params:
if name in options:
-yield (name, options[name])
+yield (name, options.pop(name))
+# If any options remain, they are either internal or unknown
+unused_keys = set(options).difference(self.internal_options)
+unused_keys.discard('version')
+if unused_keys:
+raise OptionError(_('Unknown option: %(option)s'),
+option=unused_keys.pop())
def args_options_2_entry(self, *args, **options):
"""
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index f7c6039a984c43bc
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On 05/28/2012 04:16 PM, Martin Kosek wrote: On Mon, 2012-05-28 at 15:46 +0200, Petr Vobornik wrote: On 05/25/2012 09:20 AM, Petr Vobornik wrote: On 05/16/2012 02:11 PM, Martin Kosek wrote: On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote: On 05/16/2012 09:58 AM, Martin Kosek wrote: On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: On 05/15/2012 09:55 AM, Martin Kosek wrote: On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: The final part of rejecting unknown Command arguments: enable the validation, add tests. Also fix up things that were changed since the previous patches. https://fedorahosted.org/freeipa/ticket/2509 8><-- Attaching a rebased patch. Yup, this one is fine. Now, I did not find issues in the patch itself, tests are clean. However, thanks to this new check I found issues in Web UI (automember, selfservice, delegation screen) which use illegal options and which should be fixed before we push your patch: https://fedorahosted.org/freeipa/ticket/2760 Martin I found an issue in automountmap_add_indirect. It complains that 'key' is unknown option. I found another options which were functional and now it complains: * hbacsvcgroup_find: no_hbacsvc * hbacsvc_find: not_in_hbacsvcgroup * same issue in sudo commands and sudo command groups. I didn't check all relationships, so it may be broken elsewhere as well. I don't think this is an error on server side - it never had filter options like these in the modules you referenced (though we may add them as an RFE when needed). When you pass these options in the UI to the server side, its just NOOP - or an error when Petr's patch is applied. Martin All issues found in Web UI are fixed. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On Mon, 2012-05-28 at 15:46 +0200, Petr Vobornik wrote: > On 05/25/2012 09:20 AM, Petr Vobornik wrote: > > On 05/16/2012 02:11 PM, Martin Kosek wrote: > >> On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote: > >>> On 05/16/2012 09:58 AM, Martin Kosek wrote: > On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: > > On 05/15/2012 09:55 AM, Martin Kosek wrote: > >> On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: > >>> The final part of rejecting unknown Command arguments: enable the > >>> validation, add tests. > >>> Also fix up things that were changed since the previous patches. > >>> > >>> https://fedorahosted.org/freeipa/ticket/2509 > > 8><-- > > >>> > >>> Attaching a rebased patch. > >>> > >> > >> Yup, this one is fine. Now, I did not find issues in the patch itself, > >> tests are clean. > >> > >> However, thanks to this new check I found issues in Web UI (automember, > >> selfservice, delegation screen) which use illegal options and which > >> should be fixed before we push your patch: > >> > >> https://fedorahosted.org/freeipa/ticket/2760 > >> > >> Martin > >> > > > > I found an issue in automountmap_add_indirect. It complains that 'key' > > is unknown option. > > I found another options which were functional and now it complains: > * hbacsvcgroup_find: no_hbacsvc > * hbacsvc_find: not_in_hbacsvcgroup > * same issue in sudo commands and sudo command groups. > > I didn't check all relationships, so it may be broken elsewhere as well. > I don't think this is an error on server side - it never had filter options like these in the modules you referenced (though we may add them as an RFE when needed). When you pass these options in the UI to the server side, its just NOOP - or an error when Petr's patch is applied. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On 05/25/2012 09:20 AM, Petr Vobornik wrote: On 05/16/2012 02:11 PM, Martin Kosek wrote: On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote: On 05/16/2012 09:58 AM, Martin Kosek wrote: On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: On 05/15/2012 09:55 AM, Martin Kosek wrote: On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: The final part of rejecting unknown Command arguments: enable the validation, add tests. Also fix up things that were changed since the previous patches. https://fedorahosted.org/freeipa/ticket/2509 8><-- Attaching a rebased patch. Yup, this one is fine. Now, I did not find issues in the patch itself, tests are clean. However, thanks to this new check I found issues in Web UI (automember, selfservice, delegation screen) which use illegal options and which should be fixed before we push your patch: https://fedorahosted.org/freeipa/ticket/2760 Martin I found an issue in automountmap_add_indirect. It complains that 'key' is unknown option. I found another options which were functional and now it complains: * hbacsvcgroup_find: no_hbacsvc * hbacsvc_find: not_in_hbacsvcgroup * same issue in sudo commands and sudo command groups. I didn't check all relationships, so it may be broken elsewhere as well. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On 05/25/2012 09:26 AM, Martin Kosek wrote:
On Fri, 2012-05-25 at 09:20 +0200, Petr Vobornik wrote:
On 05/16/2012 02:11 PM, Martin Kosek wrote:
On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote:
On 05/16/2012 09:58 AM, Martin Kosek wrote:
On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote:
On 05/15/2012 09:55 AM, Martin Kosek wrote:
On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote:
The final part of rejecting unknown Command arguments: enable the
validation, add tests.
Also fix up things that were changed since the previous patches.
https://fedorahosted.org/freeipa/ticket/2509
The patch looks OK so far. I just found an error in permission/aci
plugin - --subtree does not work when it matches a result:
# ipa permission-find --subtree=foo
-
0 permissions matched
-
Number of entries returned 0
ipa permission-find
--subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com'
ipa: ERROR: Unknown option: subtree
Attaching fixed patch.
We should not pass **options to aci_show, it is too risky. There may be
other places where we don't use an option-safe approach that we want to
have fixed.
We shouldn't really pass **options to any command; listing everything
explicitly would be much safer. Unfortunately, in a lot of cases where
commands call other commands, it's currently done this way.
Martin
Attaching a rebased patch.
Yup, this one is fine. Now, I did not find issues in the patch itself,
tests are clean.
However, thanks to this new check I found issues in Web UI (automember,
selfservice, delegation screen) which use illegal options and which
should be fixed before we push your patch:
https://fedorahosted.org/freeipa/ticket/2760
Martin
I found an issue in automountmap_add_indirect. It complains that 'key'
is unknown option.
I assume this is a cause and would need to be fixed in Petr3's patch:
847 # Add a submount key
848 self.api.Command['automountkey_add'](
849 location, parentmap, automountkey=key, key=key,
850 automountinformation='-fstype=autofs ldap:%s' %
map)
Martin
Thanks!
Fixed and rebased. I'll test this code path in a separate patch.
--
Petr³
From b0019124b7b4641660dc095a43d0add37da92fa8 Mon Sep 17 00:00:00 2001
From: Petr Viktorin
Date: Tue, 17 Apr 2012 12:42:35 -0400
Subject: [PATCH] Fail on unknown Command options
When unknown keyword arguments are passed to a Command, raise an
error instead of ignoring them.
Options used when IPA calls its commands internally are listed
in a new Command attribute called internal_options, and allowed.
Previous patches (0b01751c, c45174d6, c5689e7f) made IPA not use
unknown keyword arguments in its own commands and tests, but since
that some violations were reintroduced in permission_find and tests.
Fix those.
Tests included; both a frontend unittest and a XML-RPC test via the
ping plugin (which was untested previously).
https://fedorahosted.org/freeipa/ticket/2509
---
ipalib/frontend.py | 13 +--
ipalib/plugins/aci.py |2 ++
ipalib/plugins/automount.py |6 +++-
ipalib/plugins/permission.py| 46 +++-
tests/test_cmdline/test_cli.py |6 ++--
tests/test_ipalib/test_frontend.py |5 +++
tests/test_xmlrpc/test_host_plugin.py |2 +-
tests/test_xmlrpc/test_netgroup_plugin.py |6 ++--
tests/test_xmlrpc/test_permission_plugin.py | 12 +++
tests/test_xmlrpc/test_ping_plugin.py | 52 +++
10 files changed, 122 insertions(+), 28 deletions(-)
create mode 100644 tests/test_xmlrpc/test_ping_plugin.py
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index e8a84eabe0ccb981eabcc6b5d5e89f80c4135fe8..7c63b27ddb41e751692e47ebc334cb699606a74e 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -29,7 +29,8 @@
from output import Output, Entry, ListOfEntries
from text import _, ngettext
-from errors import ZeroArgumentError, MaxArgumentError, OverlapError, RequiresRoot, VersionError, RequirementError
+from errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
+RequiresRoot, VersionError, RequirementError, OptionError)
from errors import InvocationError
from constants import TYPE_ERROR
from ipapython.version import API_VERSION
@@ -404,6 +405,8 @@ class Command(HasParam):
output_params = Plugin.finalize_attr('output_params')
has_output_params = tuple()
+internal_options = tuple()
+
msg_summary = None
msg_truncated = _('Results are truncated, try a more specific search')
@@ -520,7 +523,13 @@ def __args_2_params(self, values):
def __options_2_params(self, options):
for name in self.params:
if name in options:
-yield (name, options[name
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On Fri, 2012-05-25 at 09:20 +0200, Petr Vobornik wrote: > On 05/16/2012 02:11 PM, Martin Kosek wrote: > > On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote: > >> On 05/16/2012 09:58 AM, Martin Kosek wrote: > >>> On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: > On 05/15/2012 09:55 AM, Martin Kosek wrote: > > On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: > >> The final part of rejecting unknown Command arguments: enable the > >> validation, add tests. > >> Also fix up things that were changed since the previous patches. > >> > >> https://fedorahosted.org/freeipa/ticket/2509 > >> > > > > The patch looks OK so far. I just found an error in permission/aci > > plugin - --subtree does not work when it matches a result: > > > > # ipa permission-find --subtree=foo > > - > > 0 permissions matched > > - > > > > Number of entries returned 0 > > > > > > ipa permission-find > > --subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com' > > ipa: ERROR: Unknown option: subtree > > Attaching fixed patch. > > > We should not pass **options to aci_show, it is too risky. There may be > > other places where we don't use an option-safe approach that we want to > > have fixed. > > We shouldn't really pass **options to any command; listing everything > explicitly would be much safer. Unfortunately, in a lot of cases where > commands call other commands, it's currently done this way. > > >>> > >>> > >>> Martin > >>> > >> > >> Attaching a rebased patch. > >> > > > > Yup, this one is fine. Now, I did not find issues in the patch itself, > > tests are clean. > > > > However, thanks to this new check I found issues in Web UI (automember, > > selfservice, delegation screen) which use illegal options and which > > should be fixed before we push your patch: > > > > https://fedorahosted.org/freeipa/ticket/2760 > > > > Martin > > > > I found an issue in automountmap_add_indirect. It complains that 'key' > is unknown option. I assume this is a cause and would need to be fixed in Petr3's patch: 847 # Add a submount key 848 self.api.Command['automountkey_add']( 849 location, parentmap, automountkey=key, key=key, 850 automountinformation='-fstype=autofs ldap:%s' % map) Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On 05/16/2012 02:11 PM, Martin Kosek wrote: On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote: On 05/16/2012 09:58 AM, Martin Kosek wrote: On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: On 05/15/2012 09:55 AM, Martin Kosek wrote: On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: The final part of rejecting unknown Command arguments: enable the validation, add tests. Also fix up things that were changed since the previous patches. https://fedorahosted.org/freeipa/ticket/2509 The patch looks OK so far. I just found an error in permission/aci plugin - --subtree does not work when it matches a result: # ipa permission-find --subtree=foo - 0 permissions matched - Number of entries returned 0 ipa permission-find --subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com' ipa: ERROR: Unknown option: subtree Attaching fixed patch. We should not pass **options to aci_show, it is too risky. There may be other places where we don't use an option-safe approach that we want to have fixed. We shouldn't really pass **options to any command; listing everything explicitly would be much safer. Unfortunately, in a lot of cases where commands call other commands, it's currently done this way. Martin Attaching a rebased patch. Yup, this one is fine. Now, I did not find issues in the patch itself, tests are clean. However, thanks to this new check I found issues in Web UI (automember, selfservice, delegation screen) which use illegal options and which should be fixed before we push your patch: https://fedorahosted.org/freeipa/ticket/2760 Martin I found an issue in automountmap_add_indirect. It complains that 'key' is unknown option. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On Wed, 2012-05-16 at 10:37 +0200, Petr Viktorin wrote: > On 05/16/2012 09:58 AM, Martin Kosek wrote: > > On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: > >> On 05/15/2012 09:55 AM, Martin Kosek wrote: > >>> On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: > The final part of rejecting unknown Command arguments: enable the > validation, add tests. > Also fix up things that were changed since the previous patches. > > https://fedorahosted.org/freeipa/ticket/2509 > > >>> > >>> The patch looks OK so far. I just found an error in permission/aci > >>> plugin - --subtree does not work when it matches a result: > >>> > >>> # ipa permission-find --subtree=foo > >>> - > >>> 0 permissions matched > >>> - > >>> > >>> Number of entries returned 0 > >>> > >>> > >>>ipa permission-find > >>> --subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com' > >>> ipa: ERROR: Unknown option: subtree > >> > >> Attaching fixed patch. > >> > >>> We should not pass **options to aci_show, it is too risky. There may be > >>> other places where we don't use an option-safe approach that we want to > >>> have fixed. > >> > >> We shouldn't really pass **options to any command; listing everything > >> explicitly would be much safer. Unfortunately, in a lot of cases where > >> commands call other commands, it's currently done this way. > >> > > > > There is something wrong with the patch you sent (seen on F16 and F17): > > > > $ git apply ~/freeipa-pviktori-0050-02-Fail-on-unknown-Command-options.patch > > fatal: corrupt patch at line 129 > > > > Martin > > > > Attaching a rebased patch. > Yup, this one is fine. Now, I did not find issues in the patch itself, tests are clean. However, thanks to this new check I found issues in Web UI (automember, selfservice, delegation screen) which use illegal options and which should be fixed before we push your patch: https://fedorahosted.org/freeipa/ticket/2760 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On 05/16/2012 09:58 AM, Martin Kosek wrote:
On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote:
On 05/15/2012 09:55 AM, Martin Kosek wrote:
On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote:
The final part of rejecting unknown Command arguments: enable the
validation, add tests.
Also fix up things that were changed since the previous patches.
https://fedorahosted.org/freeipa/ticket/2509
The patch looks OK so far. I just found an error in permission/aci
plugin - --subtree does not work when it matches a result:
# ipa permission-find --subtree=foo
-
0 permissions matched
-
Number of entries returned 0
ipa permission-find
--subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com'
ipa: ERROR: Unknown option: subtree
Attaching fixed patch.
We should not pass **options to aci_show, it is too risky. There may be
other places where we don't use an option-safe approach that we want to
have fixed.
We shouldn't really pass **options to any command; listing everything
explicitly would be much safer. Unfortunately, in a lot of cases where
commands call other commands, it's currently done this way.
There is something wrong with the patch you sent (seen on F16 and F17):
$ git apply ~/freeipa-pviktori-0050-02-Fail-on-unknown-Command-options.patch
fatal: corrupt patch at line 129
Martin
Attaching a rebased patch.
--
Petr³
From a6d56f34d97795746642b27f6d92b56d6c732109 Mon Sep 17 00:00:00 2001
From: Petr Viktorin
Date: Tue, 17 Apr 2012 12:42:35 -0400
Subject: [PATCH] Fail on unknown Command options
When unknown keyword arguments are passed to a Command, raise an
error instead of ignoring them.
Options used when IPA calls its commands internally are listed
in a new Command attribute called internal_options, and allowed.
Previous patches (0b01751c, c45174d6, c5689e7f) made IPA not use
unknown keyword arguments in its own commands and tests, but since
that some violations were reintroduced in permission_find and tests.
Fix those.
Tests included; both a frontend unittest and a XML-RPC test via the
ping plugin (which was untested previously).
https://fedorahosted.org/freeipa/ticket/2509
---
ipalib/frontend.py | 13 +--
ipalib/plugins/aci.py |2 ++
ipalib/plugins/automount.py |4 +++
ipalib/plugins/permission.py| 51 +++---
tests/test_cmdline/test_cli.py |6 ++--
tests/test_ipalib/test_frontend.py |5 +++
tests/test_xmlrpc/test_host_plugin.py |2 +-
tests/test_xmlrpc/test_netgroup_plugin.py |6 ++--
tests/test_xmlrpc/test_permission_plugin.py | 12 +++
tests/test_xmlrpc/test_ping_plugin.py | 52 +++
10 files changed, 122 insertions(+), 31 deletions(-)
create mode 100644 tests/test_xmlrpc/test_ping_plugin.py
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index e8a84eabe0ccb981eabcc6b5d5e89f80c4135fe8..7c63b27ddb41e751692e47ebc334cb699606a74e 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -29,7 +29,8 @@
from output import Output, Entry, ListOfEntries
from text import _, ngettext
-from errors import ZeroArgumentError, MaxArgumentError, OverlapError, RequiresRoot, VersionError, RequirementError
+from errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
+RequiresRoot, VersionError, RequirementError, OptionError)
from errors import InvocationError
from constants import TYPE_ERROR
from ipapython.version import API_VERSION
@@ -404,6 +405,8 @@ class Command(HasParam):
output_params = Plugin.finalize_attr('output_params')
has_output_params = tuple()
+internal_options = tuple()
+
msg_summary = None
msg_truncated = _('Results are truncated, try a more specific search')
@@ -520,7 +523,13 @@ def __args_2_params(self, values):
def __options_2_params(self, options):
for name in self.params:
if name in options:
-yield (name, options[name])
+yield (name, options.pop(name))
+# If any options remain, they are either internal or unknown
+unused_keys = set(options).difference(self.internal_options)
+unused_keys.discard('version')
+if unused_keys:
+raise OptionError(_('Unknown option: %(option)s'),
+option=unused_keys.pop())
def args_options_2_entry(self, *args, **options):
"""
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index f7c6039a984c43bc639ae51fb652778c8aa1f87f..7a27ce1169401c899400b4cce8a3f9946084c6a2 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -610,6 +610,8 @@ class aci_mod(crud.Update):
takes_options = (_prefix_option,)
+internal_options = ['rename']
+
msg_summary = _('Modified ACI "%(value)s"')
def execute(self, aciname, *
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On Tue, 2012-05-15 at 13:35 +0200, Petr Viktorin wrote: > On 05/15/2012 09:55 AM, Martin Kosek wrote: > > On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: > >> The final part of rejecting unknown Command arguments: enable the > >> validation, add tests. > >> Also fix up things that were changed since the previous patches. > >> > >> https://fedorahosted.org/freeipa/ticket/2509 > >> > > > > The patch looks OK so far. I just found an error in permission/aci > > plugin - --subtree does not work when it matches a result: > > > > # ipa permission-find --subtree=foo > > - > > 0 permissions matched > > - > > > > Number of entries returned 0 > > > > > > ipa permission-find > > --subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com' > > ipa: ERROR: Unknown option: subtree > > Attaching fixed patch. > > > We should not pass **options to aci_show, it is too risky. There may be > > other places where we don't use an option-safe approach that we want to > > have fixed. > > We shouldn't really pass **options to any command; listing everything > explicitly would be much safer. Unfortunately, in a lot of cases where > commands call other commands, it's currently done this way. > There is something wrong with the patch you sent (seen on F16 and F17): $ git apply ~/freeipa-pviktori-0050-02-Fail-on-unknown-Command-options.patch fatal: corrupt patch at line 129 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On 05/15/2012 09:55 AM, Martin Kosek wrote:
On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote:
The final part of rejecting unknown Command arguments: enable the
validation, add tests.
Also fix up things that were changed since the previous patches.
https://fedorahosted.org/freeipa/ticket/2509
The patch looks OK so far. I just found an error in permission/aci
plugin - --subtree does not work when it matches a result:
# ipa permission-find --subtree=foo
-
0 permissions matched
-
Number of entries returned 0
ipa permission-find
--subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com'
ipa: ERROR: Unknown option: subtree
Attaching fixed patch.
We should not pass **options to aci_show, it is too risky. There may be
other places where we don't use an option-safe approach that we want to
have fixed.
We shouldn't really pass **options to any command; listing everything
explicitly would be much safer. Unfortunately, in a lot of cases where
commands call other commands, it's currently done this way.
--
Petr³
From 5b65b401e0b6446b68cf2cce0dacc4079947ec80 Mon Sep 17 00:00:00 2001
From: Petr Viktorin
Date: Tue, 17 Apr 2012 12:42:35 -0400
Subject: [PATCH] Fail on unknown Command options
When unknown keyword arguments are passed to a Command, raise an
error instead of ignoring them.
Options used when IPA calls its commands internally are listed
in a new Command attribute called internal_options, and allowed.
Previous patches (0b01751c, c45174d6, c5689e7f) made IPA not use
unknown keyword arguments in its own commands and tests, but since
that some violations were reintroduced in permission_find and tests.
Fix those.
Tests included; both a frontend unittest and a XML-RPC test via the
ping plugin (which was untested previously).
https://fedorahosted.org/freeipa/ticket/2509
---
ipalib/frontend.py | 13 +--
ipalib/plugins/aci.py |2 ++
ipalib/plugins/automount.py |4 +++
ipalib/plugins/permission.py| 51 +++---
tests/test_cmdline/test_cli.py |6 ++--
tests/test_ipalib/test_frontend.py |5 +++
tests/test_xmlrpc/test_host_plugin.py |2 +-
tests/test_xmlrpc/test_netgroup_plugin.py |6 ++--
tests/test_xmlrpc/test_permission_plugin.py | 12 +++
tests/test_xmlrpc/test_ping_plugin.py | 52 +++
10 files changed, 122 insertions(+), 31 deletions(-)
create mode 100644 tests/test_xmlrpc/test_ping_plugin.py
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index e8a84eabe0ccb981eabcc6b5d5e89f80c4135fe8..7c63b27ddb41e751692e47ebc334cb699606a74e 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -29,7 +29,8 @@
from output import Output, Entry, ListOfEntries
from text import _, ngettext
-from errors import ZeroArgumentError, MaxArgumentError, OverlapError, RequiresRoot, VersionError, RequirementError
+from errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
+RequiresRoot, VersionError, RequirementError, OptionError)
from errors import InvocationError
from constants import TYPE_ERROR
from ipapython.version import API_VERSION
@@ -404,6 +405,8 @@ class Command(HasParam):
output_params = Plugin.finalize_attr('output_params')
has_output_params = tuple()
+internal_options = tuple()
+
msg_summary = None
msg_truncated = _('Results are truncated, try a more specific search')
@@ -520,7 +523,13 @@ def __args_2_params(self, values):
def __options_2_params(self, options):
for name in self.params:
if name in options:
-yield (name, options[name])
+yield (name, options.pop(name))
+# If any options remain, they are either internal or unknown
+unused_keys = set(options).difference(self.internal_options)
+unused_keys.discard('version')
+if unused_keys:
+raise OptionError(_('Unknown option: %(option)s'),
+option=unused_keys.pop())
def args_options_2_entry(self, *args, **options):
"""
diff --git a/ipalib/plugins/aci.py b/ipalib/plugins/aci.py
index f7c6039a984c43bc639ae51fb652778c8aa1f87f..7a27ce1169401c899400b4cce8a3f9946084c6a2 100644
--- a/ipalib/plugins/aci.py
+++ b/ipalib/plugins/aci.py
@@ -610,6 +610,8 @@ class aci_mod(crud.Update):
takes_options = (_prefix_option,)
+internal_options = ['rename']
+
msg_summary = _('Modified ACI "%(value)s"')
def execute(self, aciname, **kw):
diff --git a/ipalib/plugins/automount.py b/ipalib/plugins/automount.py
index ac9c9769f26b31631322e725721ecaf470b80f1c..a42c53b6fcada362a47657c8d6ea3f652240b4d4 100644
--- a/ipalib/plugins/automount.py
+++ b/ipalib/plugins/automount.py
@@ -787,6 +787,8 @@ class automountkey_add(LDAPCreate):
msg_summary = _('Add
Re: [Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
On Mon, 2012-05-14 at 14:47 +0200, Petr Viktorin wrote: > The final part of rejecting unknown Command arguments: enable the > validation, add tests. > Also fix up things that were changed since the previous patches. > > https://fedorahosted.org/freeipa/ticket/2509 > The patch looks OK so far. I just found an error in permission/aci plugin - --subtree does not work when it matches a result: # ipa permission-find --subtree=foo - 0 permissions matched - Number of entries returned 0 ipa permission-find --subtree='ldap:///ipauniqueid=*,cn=hbac,dc=idm,dc=lab,dc=bos,dc=redhat,dc=Com' ipa: ERROR: Unknown option: subtree We should not pass **options to aci_show, it is too risky. There may be other places where we don't use an option-safe approach that we want to have fixed. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
