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 <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 | 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 = _('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..28e289c134964ba7ad02b8ad5ea4bc4301d98e1a 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -90,6 +90,16 @@
),
)
+def filter_options(options, keys):
+ """Return a dict that includes entries from `options` that are in `keys`
+
+ example:
+ >>> filtered = filter_options({'a': 1, 'b': 2, 'c': 3}, ['a', 'c'])
+ >>> filtered == {'a': 1, 'c': 3}
+ True
+ """
+ return dict((k, options[k]) for k in keys if k in options)
+
class permission(LDAPObject):
"""
Permission object.
@@ -317,10 +327,7 @@ def exc_callback(self, keys, options, exc, call_func, *call_args, **call_kwargs)
opts = copy.copy(options)
# Clear the aci attributes out of the permission entry
for o in self.obj.aci_attributes + ['all', 'raw', 'rights']:
- try:
- del opts[o]
- except:
- pass
+ opts.pop(o, None)
if len(opts) == 0 or aciupdate:
return
raise exc
@@ -338,7 +345,7 @@ def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
cn = options['rename'] # rename finished
- common_options = dict((k, options[k]) for k in ('all', 'raw') if k in options)
+ common_options = filter_options(options, ['all', 'raw'])
result = self.api.Command.permission_show(cn, **common_options)['result']
for r in result:
if not r.startswith('member_'):
@@ -359,10 +366,17 @@ 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" must be passed as "aciname"
+ # to aci-find. Besides that we don't need cn anymore so pop it
+ aciname = options.pop('cn', None)
+
for entry in entries:
(dn, attrs) = entry
try:
- aci = self.api.Command.aci_show(attrs['cn'][0], aciprefix=ACI_PREFIX, **options)['result']
+ common_options = filter_options(options, ['all', 'raw'])
+ aci = self.api.Command.aci_show(attrs['cn'][0],
+ aciprefix=ACI_PREFIX, **common_options)['result']
# copy information from respective ACI to permission entry
for attr in self.obj.aci_attributes:
@@ -375,33 +389,27 @@ def post_callback(self, ldap, entries, truncated, *args, **options):
# aren't already in the list along with their permission info.
opts = copy.copy(options)
+ if aciname:
+ opts['aciname'] = aciname
opts['aciprefix'] = ACI_PREFIX
- try:
- # permission ACI attribute is needed
- del opts['raw']
- except:
- pass
- if 'cn' in options:
- # the attribute for name is difference in acis
- opts['aciname'] = options['cn']
+ # permission ACI attribute is needed
+ opts.pop('raw', None)
aciresults = self.api.Command.aci_find(*args, **opts)
truncated = truncated or aciresults['truncated']
results = aciresults['result']
- if 'cn' in options:
- # there is an option/param overlap if --name is in the
- # search list, we don't need cn anymore so drop it.
- options.pop('cn')
for aci in results:
found = False
if 'permission' in aci:
for entry in entries:
(dn, attrs) = entry
if aci['permission'] == attrs['cn'][0]:
found = True
break
if not found:
- permission = self.api.Command.permission_show(aci['permission'], **options)['result']
+ common_options = filter_options(options, ['all', 'raw'])
+ permission = self.api.Command.permission_show(
+ aci['permission'], **common_options)['result']
dn = permission['dn']
del permission['dn']
if (dn, permission) not in entries:
@@ -416,8 +424,9 @@ class permission_show(LDAPRetrieve):
has_output_params = LDAPRetrieve.has_output_params + output_params
def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
try:
- common_options = dict((k, options[k]) for k in ('all', 'raw') if k in options)
- aci = self.api.Command.aci_show(keys[-1], aciprefix=ACI_PREFIX, **common_options)['result']
+ common_options = filter_options(options, ['all', 'raw'])
+ aci = self.api.Command.aci_show(keys[-1], aciprefix=ACI_PREFIX,
+ **common_options)['result']
for attr in self.obj.aci_attributes:
if attr in aci:
entry_attrs[attr] = aci[attr]
diff --git a/tests/test_cmdline/test_cli.py b/tests/test_cmdline/test_cli.py
index 095577a3b776341d5571f82ecbab55e7e3527f8e..d961f8725b5e4360b6c2298f3fddd3589cbb310d 100644
--- a/tests/test_cmdline/test_cli.py
+++ b/tests/test_cmdline/test_cli.py
@@ -128,8 +128,7 @@ def test_dnsrecord_add(self):
def test_dnsrecord_del_all(self):
try:
self.run_command('dnszone_add', idnsname=u'test-example.com',
- idnssoamname=u'ns.test-example.com',
- admin_email=u'devn...@test-example.com', force=True)
+ idnssoamname=u'ns.test-example.com', force=True)
except errors.NotFound:
raise nose.SkipTest('DNS is not configured')
try:
@@ -162,8 +161,7 @@ def test_dnsrecord_del_all(self):
def test_dnsrecord_del_one_by_one(self):
try:
self.run_command('dnszone_add', idnsname=u'test-example.com',
- idnssoamname=u'ns.test-example.com',
- admin_email=u'devn...@test-example.com', force=True)
+ idnssoamname=u'ns.test-example.com', force=True)
except errors.NotFound:
raise nose.SkipTest('DNS is not configured')
try:
diff --git a/tests/test_ipalib/test_frontend.py b/tests/test_ipalib/test_frontend.py
index b717a43ad4c68940582f901308f4dd4da77834ee..5f7ce65fb29d8eed531d15bf3e3fb094933aa007 100644
--- a/tests/test_ipalib/test_frontend.py
+++ b/tests/test_ipalib/test_frontend.py
@@ -511,6 +511,11 @@ def test_args_options_2_params(self):
assert e.count == 2
assert str(e) == "command 'example' takes at most 2 arguments"
+ # Test that OptionError is raised when an extra option is given:
+ o = self.get_instance()
+ e = raises(errors.OptionError, o.args_options_2_params, bad_option=True)
+ assert e.option == 'bad_option'
+
# Test that OverlapError is raised:
o = self.get_instance(args=('one', 'two'), options=('three', 'four'))
e = raises(errors.OverlapError, o.args_options_2_params,
diff --git a/tests/test_xmlrpc/test_host_plugin.py b/tests/test_xmlrpc/test_host_plugin.py
index 8798168afa71653b64870c77d11a7fa81ec4c952..69ef82e20dafdfed38669ec36c05a5055754b06c 100644
--- a/tests/test_xmlrpc/test_host_plugin.py
+++ b/tests/test_xmlrpc/test_host_plugin.py
@@ -126,7 +126,7 @@ class test_host(Declarative):
command=('host_add', [fqdn1],
dict(
description=u'Test host 1',
- locality=u'Undisclosed location 1',
+ l=u'Undisclosed location 1',
force=True,
),
),
diff --git a/tests/test_xmlrpc/test_netgroup_plugin.py b/tests/test_xmlrpc/test_netgroup_plugin.py
index d51287bcd0174818126131c1bebcb558720a0cc7..951bc77a39d68b9e1d1bfec75551ba550fff5858 100644
--- a/tests/test_xmlrpc/test_netgroup_plugin.py
+++ b/tests/test_xmlrpc/test_netgroup_plugin.py
@@ -726,7 +726,7 @@ class test_netgroup(Declarative):
dict(
- desc='Add duplicatehost %r to netgroup %r' % (host1, netgroup1),
+ desc='Add duplicate host %r to netgroup %r' % (host1, netgroup1),
command=(
'netgroup_add_member', [netgroup1], dict(host=host1)
),
@@ -960,8 +960,8 @@ class test_netgroup(Declarative):
),
dict(
- desc='Search for all netgroups using empty memberuser',
- command=('netgroup_find', [], dict(memberuser=None)),
+ desc='Search for all netgroups using empty member user',
+ command=('netgroup_find', [], dict(user=None)),
expected=dict(
count=2,
truncated=False,
diff --git a/tests/test_xmlrpc/test_permission_plugin.py b/tests/test_xmlrpc/test_permission_plugin.py
index 28db7dc2f588f40d94276db3b3096a9ed5e5db0d..7b71b1513285796fdcdd983d92052f14ea787752 100644
--- a/tests/test_xmlrpc/test_permission_plugin.py
+++ b/tests/test_xmlrpc/test_permission_plugin.py
@@ -551,6 +551,18 @@ class test_permission(Declarative):
dict(
+ desc='Search using nonexistent --subtree',
+ command=('permission_find', [], {'subtree': u'foo'}),
+ expected=dict(
+ count=0,
+ truncated=False,
+ summary=u'0 permissions matched',
+ result=[],
+ ),
+ ),
+
+
+ dict(
desc='Delete %r' % permission1_renamed_ucase,
command=('permission_del', [permission1_renamed_ucase], {}),
expected=dict(
diff --git a/tests/test_xmlrpc/test_ping_plugin.py b/tests/test_xmlrpc/test_ping_plugin.py
new file mode 100644
index 0000000000000000000000000000000000000000..284aed54f314f847f678e7033a4a55bfa2d2fa2f
--- /dev/null
+++ b/tests/test_xmlrpc/test_ping_plugin.py
@@ -0,0 +1,52 @@
+# Authors:
+# Petr Viktorin <pvikt...@redhat.com>
+#
+# Copyright (C) 2012 Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+"""
+Test the `ipalib/plugins/ping.py` module, and XML-RPC in general.
+"""
+
+from ipalib import api, errors, _
+from tests.util import assert_equal, Fuzzy
+from xmlrpc_test import Declarative
+
+
+class test_ping(Declarative):
+
+ tests = [
+ dict(
+ desc='Ping the server',
+ command=('ping', [], {}),
+ expected=dict(
+ summary=Fuzzy('IPA server version .*. API version .*')),
+ ),
+
+ dict(
+ desc='Try to ping with an argument',
+ command=('ping', ['bad_arg'], {}),
+ expected=errors.ZeroArgumentError(name='ping'),
+ ),
+
+ dict(
+ desc='Try to ping with an option',
+ command=('ping', [], dict(bad_arg=True)),
+ expected=errors.OptionError(_('Unknown option: %(option)s'),
+ option='bad_arg'),
+ ),
+
+ ]
--
1.7.10.2
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
