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 pvikt...@redhat.com
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
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/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 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/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 pvikt...@redhat.com
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
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
[Freeipa-devel] [PATCH] 0050 Fail on unknown Command options
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
--
Petr³
From 64496d35b5483b8b237282dd157388f10e72beda Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
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 | 17 +-
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_ping_plugin.py | 52 +
9 files changed, 89 insertions(+), 18 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 b0be26f5c44e51d91af3bdf7d0a94c0a14f8fe4a..8476f1826561f8f00522b0b19f057d493bec2611 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 = _('Added automount key %(value)s')
+internal_options = ['description', 'add_operation']
+
def pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
options.pop('add_operation', None)
options.pop('description', None)
@@ -910,6 +912,8 @@ class automountkey_mod(LDAPUpdate):
msg_summary = _('Modified automount key %(value)s')
+internal_options = ['newautomountkey']
+
takes_options = LDAPUpdate.takes_options + (
IA5Str('newautomountinformation?',
cli_name='newinfo',
diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index ff38f852da9a6d133ae0b4c7a77c74fcd4a23913..c532e16f4e29066708ac2c0d6ee1d4de204cb631 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -359,10 +359,16 @@ class permission_find(LDAPSearch):
def post_callback(self, ldap, entries, truncated, *args, **options):
if options.pop('pkey_only', False):
return
+
+# There is an option/param overlap: cn
