Re: [Freeipa-devel] [PATCH] 996 fix unit tests
On Fri, 2012-03-23 at 23:05 +0100, Ondrej Hamada wrote: On 03/23/2012 08:12 PM, Rob Crittenden wrote: A few unit tests were failing due to new type enforcement and comman support. Unit tests are passing 100% for me with this. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel You were faster. Works for me. ACK Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0031 Add missing BuildRequires
On Fri, 2012-03-23 at 21:59 +0200, Alexander Bokovoy wrote: On Fri, 23 Mar 2012, Petr Viktorin wrote: Since our build process runs pylint, we need all Python dependencies installed at RPM creation time. This adds python-lxml and python-pyasn1 to BuildRequires. https://fedorahosted.org/freeipa/ticket/2538 Including patches for both master and ipa-2-2. ACK. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 995 update min version of 389-ds-base
On Fri, 2012-03-23 at 16:36 -0400, Rob Crittenden wrote: Petr Viktorin wrote: On 03/23/2012 08:11 PM, Rob Crittenden wrote: Rob Crittenden wrote: Set new minimum version of 389-ds-base to fix db corruption during upgrade problem. This patch is against 2.2. Corrected changelog entry. You've attached a different patch. Sigh, fixed now. TGIF. rob ACK. Rebaed for ipa-2-2/master branch and pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 66] Replace broken i18n shell test with Python test
On 03/23/2012 10:33 PM, John Dennis wrote: Attached is is a new patch addressing Petr's review comment as well as some sample output for illustration purposes (I suggest you review the example.txt). Rather than wading through a log of dialog in the previous emails let me address Petr's issues and how I addressed them. The option parsing is totally reworked (see example.txt). Not only is optparse being used, but the options have been simplified and appear in groups. The script can do a variety of different things so you must specify the mode you want it to run in. I don't see an obvious candidate for a default mode, plus most users will run it as a make target anyway obviating the need to specify any command line options. I logically divided the validation checks, one for checking the English source strings (pot file) and one for checking the translations (po files). These represent two of the three possible modes of operations. Petr's has said several times that he would prefer the English sources strings be checked by other tests. I presume he is referring to the unit tests, but we do not have any such unit test nor are we likely to (at least not for string validation). The reason is you have to have available the set of strings from the sources which have been marked for translation. It's a somewhat complicated process to produce that set of strings and that logic are part of the install/po area, namely the generation of the pot file. This is the only logical place I can see for performing tests and/or validation on (translatable) source strings. I do not envision the validation ever being part of a unit test, it is functionally very different. There is a good argument for generating a new pot file and validating it on every code check-in. This patch does not do that, instead it's done when the pot file is generated manually, but it would be trival to add that feature once this framework is in place. However a good case could be made for the gettext test currently implemented in test_i18n.py being invoked as a unit test but there would be some structural work needed to make that happen (e.g. assuring the test mo file is generated and pointed to and calling things through IPA Gettext classes and having the framework run in the context of that locale). But we've never had a failure in that area yet so I'd be inclined to defer that at the moment, open a new ticket to integrate the gettext test in test_i18n.py with the test_text.py in the unit tests and consider that a separate work item so as not let it interfere with getting this committed. Being able to run the gettext test in the translation directory is valuable none-the-less. It would be better if it were fully integrated with our Gettext classes in text.py but that is best left as another task. By dividing the validation checks between the pot file and po files I think I addressed the majority of Petr's concerns over the validation of source strings. Petr also alluded Transifex errors as if Transifex would somehow catch the translation problems, but no such error checking exists (actually I'm considering giving this code to upstream Transifex, they're entirely Python based and they might find parts of it useful) But wait there's more ... Some of the validations were producing false positives because the validator could not determine whether some uses of $ and % were valid or mangled syntax. So I added a pedantic flag. It's off by default and skips checking for errors which are unlikely to be present and are difficult to check. You can run the validation with --pedantic as a safeguard and interpret the results. I thought this was preferred to removing the ability to perform the checks whatsoever. I also added a --show-strings flag so along with a error message and location it will output the offending string(s). This if off by default for brevity purposes but is useful for those having to make fixes or just to better understand the nature of the error. I tried to make the error output a bit less verbose with better formatting. I updated .gitignore to ignore the generated test files. Great! Just two issues now. According to the style guide, we do allow %s in a string iff it only appears once. The reason for named substitutions is that the word order can be changed, providing context is just secondary. Why does the checker report messages with a single %s as errors? You've removed test_lang from Makefile.in, but test and .PHONY still mention it. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0027 Use valid argument names in tests
On 03/14/2012 05:02 PM, Petr Viktorin wrote:
This patch depends on my patch 0024 (but I can rebase if it needs to be
pushed earlied).
It fixes some of the test bugs that would be found by a fix for
https://fedorahosted.org/freeipa/ticket/2509 (Unknown Command arguments
are allowed (and ignored)).
As you can see it's quite easy, without validation, to use a wrong name
for an argument.
Unfortunately, some of our plugins (automount, dns, delegation,
permission, selfservice) take advantage of the current sloppy
validation, so they'll need some untangling for 3.x to make them more
robust and testable. For now I'm just touching the tests, so we don't
get new bugs in production code.
Rebased onto current master
--
Petr³
From c4b811c84b9427e7754a46467c49c6b67a96b130 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Wed, 14 Mar 2012 11:33:41 -0400
Subject: [PATCH] Use valid argument names in tests
Some of our tests used unintended extra options, or options with
misspelled, wrongly copy-pasted or otherwise bad names. These are
ignored, so the intended argument was treated as missing. The test
itself can still pass but may be rendered ineffective or fragile.
This only fixes those of such errors that appear in the test suite.
Fixing code in the framework and actual rejecting of unknown
arguments is deferred for later (ticket #2509).
---
tests/test_xmlrpc/test_automember_plugin.py |5 +
tests/test_xmlrpc/test_delegation_plugin.py |2 +-
tests/test_xmlrpc/test_hbac_plugin.py |6 +++---
tests/test_xmlrpc/test_hbactest_plugin.py |2 +-
tests/test_xmlrpc/test_host_plugin.py |2 +-
tests/test_xmlrpc/test_pwpolicy.py |2 +-
tests/test_xmlrpc/test_role_plugin.py |2 +-
tests/test_xmlrpc/test_sudorule_plugin.py |6 +++---
8 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/tests/test_xmlrpc/test_automember_plugin.py b/tests/test_xmlrpc/test_automember_plugin.py
index 1241bd669c597c1363b86c4f378472031c3e6682..265c3af861b0bfd5ffa102e5fbb3a81857292996 100644
--- a/tests/test_xmlrpc/test_automember_plugin.py
+++ b/tests/test_xmlrpc/test_automember_plugin.py
@@ -718,10 +718,7 @@ class test_automember(Declarative):
dict(
desc='Retrieve default automember group for groups',
command=(
-'automember_default_group_show', [], dict(
-type=u'group',
-automemberdefaultgroup=defaultgroup1,
-)
+'automember_default_group_show', [], dict(type=u'group')
),
expected=dict(
result=dict(
diff --git a/tests/test_xmlrpc/test_delegation_plugin.py b/tests/test_xmlrpc/test_delegation_plugin.py
index 5030f8bc26e869c0553d331c290e481a94514c9e..c4473b9d41d0ef4f728209eaeaf00055aef727ac 100644
--- a/tests/test_xmlrpc/test_delegation_plugin.py
+++ b/tests/test_xmlrpc/test_delegation_plugin.py
@@ -45,7 +45,7 @@ class test_delegation(Declarative):
dict(
desc='Try to update non-existent %r' % delegation1,
-command=('delegation_mod', [delegation1], dict(description=u'Foo')),
+command=('delegation_mod', [delegation1], dict(group=u'admins')),
expected=errors.NotFound(reason='no such entry'),
),
diff --git a/tests/test_xmlrpc/test_hbac_plugin.py b/tests/test_xmlrpc/test_hbac_plugin.py
index bbaf9d25b5b1bca524bc4a02a16c484d6e0ebc99..58265dc07c729fbaca0111807d511b0e906fc095 100644
--- a/tests/test_xmlrpc/test_hbac_plugin.py
+++ b/tests/test_xmlrpc/test_hbac_plugin.py
@@ -123,7 +123,7 @@ def test_6_hbacrule_find(self):
Test searching for HBAC rules using `xmlrpc.hbacrule_find`.
ret = api.Command['hbacrule_find'](
-name=self.rule_name, accessruletype=self.rule_type,
+cn=self.rule_name, accessruletype=self.rule_type,
description=self.rule_desc_mod
)
assert ret['truncated'] is False
@@ -155,7 +155,7 @@ def test_7_hbacrule_init_testing_data(self):
self.test_sourcehostgroup, description=u'desc'
)
self.failsafe_add(api.Object.hbacsvc,
-self.test_service, description=u'desc', force=True
+self.test_service, description=u'desc',
)
def test_8_hbacrule_add_user(self):
@@ -424,7 +424,7 @@ def test_f_hbacrule_exclusiveuser(self):
api.Command['hbacrule_mod'](self.rule_name, usercategory=u'all')
try:
-api.Command['hbacrule_add_user'](self.rule_name, users=u'admin')
+api.Command['hbacrule_add_user'](self.rule_name, user=u'admin')
finally:
api.Command['hbacrule_mod'](self.rule_name, usercategory=u'')
diff --git a/tests/test_xmlrpc/test_hbactest_plugin.py b/tests/test_xmlrpc/test_hbactest_plugin.py
index 5d829b14c4fa6e7d96db36b64afe38a171096142..bc12e8974dc4591afa3b4a5f9e68df9ed32e2231 100644
---
Re: [Freeipa-devel] [PATCH] 994 set nsslapd-minssf-exclude-rootdse
On Thu, 2012-03-22 at 17:21 -0400, Rob Crittenden wrote: If minssf is set in configuration and this is not set then clients won't be able to detect the available namingContexts, defaultNamingContext, capabilities, etc. This was requested by the SSSD team. rob ACK. Works fine - RootDSE is not accessible even with lower SSF than minSSF. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 994 set nsslapd-minssf-exclude-rootdse
On Mon, 2012-03-26 at 14:29 +0200, Martin Kosek wrote: On Thu, 2012-03-22 at 17:21 -0400, Rob Crittenden wrote: If minssf is set in configuration and this is not set then clients won't be able to detect the available namingContexts, defaultNamingContext, capabilities, etc. This was requested by the SSSD team. rob ACK. Works fine - RootDSE is not accessible even with lower SSF than minSSF. Martin ... and pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 994 set nsslapd-minssf-exclude-rootdse
On Mon, 2012-03-26 at 14:29 +0200, Martin Kosek wrote: On Thu, 2012-03-22 at 17:21 -0400, Rob Crittenden wrote: If minssf is set in configuration and this is not set then clients won't be able to detect the available namingContexts, defaultNamingContext, capabilities, etc. This was requested by the SSSD team. rob ACK. Works fine - RootDSE is not accessible even with lower SSF than I hope you meant *now* :-) minSSF. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 994 set nsslapd-minssf-exclude-rootdse
On Mon, 2012-03-26 at 08:37 -0400, Simo Sorce wrote: On Mon, 2012-03-26 at 14:29 +0200, Martin Kosek wrote: On Thu, 2012-03-22 at 17:21 -0400, Rob Crittenden wrote: If minssf is set in configuration and this is not set then clients won't be able to detect the available namingContexts, defaultNamingContext, capabilities, etc. This was requested by the SSSD team. rob ACK. Works fine - RootDSE is not accessible even with lower SSF than I hope you meant *now* :-) The answer is yes of course - I should type slower :-) minSSF. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 72 Fix uses of O=REALM instead of the configured certificate subject base
https://fedorahosted.org/freeipa/ticket/2521
Honza
--
Jan Cholasta
From 8c078285b4703f3ddb991665ec4a548b44a3e97d Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Mon, 26 Mar 2012 07:11:41 -0400
Subject: [PATCH] Fix uses of O=REALM instead of the configured certificate
subject base.
ticket 2521
---
ipalib/x509.py | 14 ++
tests/test_xmlrpc/test_cert.py |4 +++-
tests/test_xmlrpc/test_host_plugin.py|8
tests/test_xmlrpc/test_service_plugin.py |4 ++--
tests/test_xmlrpc/xmlrpc_test.py |2 +-
5 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/ipalib/x509.py b/ipalib/x509.py
index 04e1b94..06b1aad 100644
--- a/ipalib/x509.py
+++ b/ipalib/x509.py
@@ -42,15 +42,21 @@ from ipalib import api
from ipalib import _
from ipalib import util
from ipalib import errors
+from ipalib.dn import DN
PEM = 0
DER = 1
PEM_REGEX = re.compile(r'(?=-BEGIN CERTIFICATE-).*?(?=-END CERTIFICATE-)', re.DOTALL)
-def valid_issuer(issuer, realm):
-return issuer in ('CN=%s Certificate Authority' % realm,
- 'CN=Certificate Authority,O=%s' % realm,)
+def subject_base():
+return DN(api.Command['config_show']()['result']['ipacertificatesubjectbase'][0])
+
+def valid_issuer(issuer):
+if api.env.ra_plugin == 'dogtag':
+return DN(issuer) == DN(('CN', 'Certificate Authority'), subject_base())
+else:
+return DN(issuer) == DN(('CN', '%s Certificate Authority' % api.env.realm))
def strip_header(pem):
@@ -209,7 +215,7 @@ def verify_cert_subject(ldap, hostname, dercert):
issuer = str(nsscert.issuer)
# Handle both supported forms of issuer, from selfsign and dogtag.
-if (not valid_issuer(issuer, api.env.realm)):
+if (not valid_issuer(issuer)):
raise errors.CertificateOperationError(error=_('Issuer %(issuer)s does not match the expected issuer') % \
{'issuer' : issuer})
diff --git a/tests/test_xmlrpc/test_cert.py b/tests/test_xmlrpc/test_cert.py
index 253373a..90809ab 100644
--- a/tests/test_xmlrpc/test_cert.py
+++ b/tests/test_xmlrpc/test_cert.py
@@ -28,6 +28,7 @@ from nose.tools import assert_raises # pylint: disable=E0611
from xmlrpc_test import XMLRPC_test, assert_attr_equal
from ipalib import api
from ipalib import errors
+from ipalib import x509
import tempfile
from ipapython import ipautil
import nose
@@ -74,6 +75,8 @@ class test_cert(XMLRPC_test):
# Create our temporary NSS database
self.run_certutil([-N, -f, self.pwname])
+self.subject = DN(('CN', self.host_fqdn), x509.subject_base())
+
def tearDown(self):
super(test_cert, self).tearDown()
shutil.rmtree(self.reqdir, ignore_errors=True)
@@ -95,7 +98,6 @@ class test_cert(XMLRPC_test):
host_fqdn = u'ipatestcert.%s' % api.env.domain
service_princ = u'test/%s@%s' % (host_fqdn, api.env.realm)
-subject = DN(('CN',host_fqdn),('O',api.env.realm))
def test_1_cert_add(self):
diff --git a/tests/test_xmlrpc/test_host_plugin.py b/tests/test_xmlrpc/test_host_plugin.py
index 5e49e2b..2d691df 100644
--- a/tests/test_xmlrpc/test_host_plugin.py
+++ b/tests/test_xmlrpc/test_host_plugin.py
@@ -252,7 +252,7 @@ class test_host(Declarative):
valid_not_before=fuzzy_date,
valid_not_after=fuzzy_date,
subject=lambda x: DN(x) == \
-DN(('CN',api.env.host),('O',api.env.realm)),
+DN(('CN',api.env.host),x509.subject_base()),
serial_number=fuzzy_digits,
serial_number_hex=fuzzy_hex,
md5_fingerprint=fuzzy_hash,
@@ -284,7 +284,7 @@ class test_host(Declarative):
valid_not_before=fuzzy_date,
valid_not_after=fuzzy_date,
subject=lambda x: DN(x) == \
-DN(('CN',api.env.host),('O',api.env.realm)),
+DN(('CN',api.env.host),x509.subject_base()),
serial_number=fuzzy_digits,
serial_number_hex=fuzzy_hex,
md5_fingerprint=fuzzy_hash,
@@ -483,7 +483,7 @@ class test_host(Declarative):
valid_not_before=fuzzy_date,
valid_not_after=fuzzy_date,
subject=lambda x: DN(x) == \
-DN(('CN',api.env.host),('O',api.env.realm)),
+DN(('CN',api.env.host),x509.subject_base()),
serial_number=fuzzy_digits,
serial_number_hex=fuzzy_hex,
md5_fingerprint=fuzzy_hash,
@@ -513,7 +513,7 @@ class test_host(Declarative):
valid_not_before=fuzzy_date,
valid_not_after=fuzzy_date,
subject=lambda x: DN(x) == \
-
Re: [Freeipa-devel] [PATCH] 72 Fix uses of O=REALM instead of the configured certificate subject base
Jan Cholasta wrote: https://fedorahosted.org/freeipa/ticket/2521 Honza You can still set a custom subject base for selfsign installations so you need a special case in valid_issuer(). I wonder if this comparison should be case insensitive too. It may also be an optimization to cache the base in subject_base(). It can't change after install time so it should be valid the entire lifetime of the server. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 42-3 Add CleanRUV Task to ipa-replica-manage del
On Wed, 2012-02-29 at 07:10 +, JR Aquino wrote:
On Feb 28, 2012, at 10:44 AM, JR Aquino wrote:
On Feb 24, 2012, at 3:09 PM, JR Aquino wrote:
ipa-replica-manage del causes tombstone entries to remain in 389 DS. This
has proven to be problematic.
We can automatically perform the cleanup task at the deletion time to
minimize orphans and ghosts in the directory.
This patch runs the cleanruv action on all masters following a delete.
https://fedorahosted.org/freeipa/ticket/2303
Rebased and corrected patch attached
NACK
Testing turned up a missed case for handling an exception during --force.
Corrected patch attached
Hello JR,
I reviewed and tested your patch and have few comments.
1) It needs rebasing, otherwise indentation of this block is weird:
+# Get list of Masters
+dn = 'cn=masters,cn=ipa,cn=etc,%s' % thisrepl.suffix
+res = thisrepl.conn.search_s(dn, ldap.SCOPE_ONELEVEL)
+master_names = []
+for entry in res:
+master_names.append(entry.cn)
+
if force_del:
-dn = 'cn=masters,cn=ipa,cn=etc,%s' % thisrepl.suffix
-res = thisrepl.conn.search_s(dn, ldap.SCOPE_ONELEVEL)
-replica_names = []
-for entry in res:
-replica_names.append(entry.cn)
+replica_names = master_names
We don't need to set replica_names in the for loop (i.e. for each
master). master_names array creation could be created with a simpler
construct:
master_names = [ entry.cn for entry in res ]
2) Error message improvement
When I tried to remove replica agreement and one of the replicas
(vm-062.idm.lab.bos.redhat.com) was down, Clean RUV task on this replica
failed as expected but I never get a message about which replica it was:
# ipa-replica-manage del vm-071.idm.lab.bos.redhat.com
Deleting a master is irreversible.
To reconnect to the remote master you will need to prepare a new replica file
and re-install.
Continue to delete? [no]: y
Deleted replication agreement from 'vm-022.idm.lab.bos.redhat.com' to
'vm-071.idm.lab.bos.redhat.com'
Deleted replication agreement from 'vm-115.idm.lab.bos.redhat.com' to
'vm-071.idm.lab.bos.redhat.com'
Cleaned RUV entry for 'vm-071.idm.lab.bos.redhat.com' on
'vm-022.idm.lab.bos.redhat.com'
Cleaned RUV entry for 'vm-071.idm.lab.bos.redhat.com' on
'vm-115.idm.lab.bos.redhat.com'
{'desc': Can't contact LDAP server}
Please refer to http://directory.fedoraproject.org/wiki/Howto:CLEANRUV for
manual cleanup.
I just got:
{'desc': Can't contact LDAP server}
Please refer to http://directory.fedoraproject.org/wiki/Howto:CLEANRUV for
manual cleanup.
I would expect a more explaining error, something like:
Error: Can't contact LDAP server
Failed to clean RUV entry for 'vm-071.idm.lab.bos.redhat.com' on
'vm-062.idm.lab.bos.redhat.com':
Please refer to http://directory.fedoraproject.org/wiki/Howto:CLEANRUV for
manual cleanup.
3) Redundant exception
I this this try..except statement is redundant as you don't perform
anything in the except part:
+try:
+repl1 = replication.ReplicationManager(realm, replica1,
dirman_passwd)
+
+replica_dn = repl1.replica_dn()
+
+attr = 'CLEANRUV%s' % delrepl_id
+mod = [(ldap.MOD_REPLACE, 'nsds5task', attr)]
+repl1.conn.modify_s(replica_dn, mod)
+
+except Exception, e:
+raise e
4) Bad tombstone replication
When I started the replica that was down during RUV cleanup, tombstone
was replicated again to all replicas. This would mean that manual
CleanRUV would have to be run on _all_ replicas again and not just the
one that was down. Are we OK with this? If yes, then it would be nice to
mention this information in the error message.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 72 Fix uses of O=REALM instead of the configured certificate subject base
On 26.3.2012 16:15, Rob Crittenden wrote: Jan Cholasta wrote: https://fedorahosted.org/freeipa/ticket/2521 Honza You can still set a custom subject base for selfsign installations so you need a special case in valid_issuer(). For selfsign installations, the issuer is always CN=REALM Certificate Authority, no matter what is set in the subject base, so no special case is needed. I wonder if this comparison should be case insensitive too. I think the DN class already takes care of this. It may also be an optimization to cache the base in subject_base(). It can't change after install time so it should be valid the entire lifetime of the server. What if someone does $ ipa config-mod --setattr ipacertificatesubjectbase='O=Something' ? rob Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 72 Fix uses of O=REALM instead of the configured certificate subject base
On 03/26/2012 11:28 AM, Jan Cholasta wrote: On 26.3.2012 16:15, Rob Crittenden wrote: Jan Cholasta wrote: https://fedorahosted.org/freeipa/ticket/2521 Honza You can still set a custom subject base for selfsign installations so you need a special case in valid_issuer(). For selfsign installations, the issuer is always CN=REALM Certificate Authority, no matter what is set in the subject base, so no special case is needed. I wonder if this comparison should be case insensitive too. I think the DN class already takes care of this. It may also be an optimization to cache the base in subject_base(). It can't change after install time so it should be valid the entire lifetime of the server. What if someone does $ ipa config-mod --setattr ipacertificatesubjectbase='O=Something' ? :: [ LOG] :: ipaconfig-mod_setattr ipacertificatesubjectbase positive :: [ PASS ] :: Set ipapwdexpadvnotify to OU=Bogus :: [ PASS ] :: ipacertificatesubjectbase successfully changed. :: [ LOG] :: Duration: 3s :: [ LOG] :: Assertions: 2 good, 0 bad :: [ PASS ] :: RESULT: ipaconfig-mod_setattr ipacertificatesubjectbase positive It works ... should we be getting an error?? rob Honza -- Jenny Galipeau jgali...@redhat.com Principal Software QA Engineer Red Hat, Inc. Security Engineering Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 66] Replace broken i18n shell test with Python test
On 03/26/2012 04:24 AM, Petr Viktorin wrote: Great! Just two issues now. According to the style guide, we do allow %s in a string iff it only appears once. The reason for named substitutions is that the word order can be changed, providing context is just secondary. Why does the checker report messages with a single %s as errors? Good catch, I should have known better, I believe I was the one who wrote that section in the style guide, so I should have known better (or remembered better). Fixed. You've removed test_lang from Makefile.in, but test and .PHONY still mention it. Also fixed. Updated patch attached. -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ From 0d74736954efca3bdafa1967f1f6f97c43528fa8 Mon Sep 17 00:00:00 2001 From: John Dennis jden...@redhat.com Date: Fri, 23 Mar 2012 01:44:04 -0400 Subject: [PATCH 66-3] Replace broken i18n shell test with Python test Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit We had been using shell scripts and sed to test our translations. But trying to edit pot and po files with sed is nearly impossible because the file format can vary significantly and the sed editing was failing and gettext tools were complaining about our test strategy. We had been using a Python script (test_i18n.py) to perform the actual test after using shell, sed, and gettext tools to create the files. There is a Python library (polib) which can read/write/edit pot/po/mo files (used internally by Transifex, our translation portal). The strategy now is to do everything in Python (in test_i18n.py). This is easier, more robust and allows us to do more things. * add python-polib to BuildRequires * Remove the logic for creating the test lang from Makefile.in and replace it with calls to test_i18n.py * add argument parsing, usage, configuration parameters, etc. to test_i18n.py to make it easier to use and configurable. * add function to generate a test po and mo file. It also writes the files and creates the test directory structure. * Took the existing validate code and refactored it into validation function. It used to just pick one string and test it, now it iterates over all strings and all plural forms. * Validate anonymous Python format substitutions in pot file * added support for plural forms. * Add pot po file validation for variable substitution * In install/po subdir you can now do: $ make test $ make validate-pot $ make validate-po * The options for running test_i18n.py are: $ ./test_i18n.py --help Usage: test_i18n.py --test-gettext test_i18n.py --create-test test_i18n.py --validate-pot [pot_file1, ...] test_i18n.py --validate-po po_file1 [po_file2, ...] Options: -h, --helpshow this help message and exit -s, --show-stringsshow the offending string when an error is detected --pedanticbe aggressive when validating -v, --verbose be informative --traceback print the traceback when an exception occurs Operational Mode: You must select one these modes to run in -g, --test-gettext create the test translation file(s) and exercise them -c, --create-test create the test translation file(s) -P, --validate-pot validate pot file(s) -p, --validate-po validate po file(s) Run Time Parameters: These may be used to modify the run time defaults --test-lang=TEST_LANG test po file uses this as it's basename (default=test) --lang=LANG lang used for locale, MUST be a valid lang (default=xh_ZA) --domain=DOMAIN translation domain used during test (default=ipa) --locale=LOCALE locale used during test (default=test_locale) --pot-file=POT_FILE default pot file, used when validating pot file or generating test po and mo files (default=ipa.pot) --- .gitignore |2 + freeipa.spec.in |1 + install/po/Makefile.in | 22 +- install/po/test_i18n.py | 620 ++- 4 files changed, 577 insertions(+), 68 deletions(-) diff --git a/.gitignore b/.gitignore index 14f9e9b..d20747c 100644 --- a/.gitignore +++ b/.gitignore @@ -46,6 +46,8 @@ install/config.status install/install-sh install/missing install/stamp-h1 +install/po/test.po +install/po/test_locale/xh_ZA/LC_MESSAGES/ipa.mo install/ui/test/results ipa-client/COPYING ipa-client/ChangeLog diff --git a/freeipa.spec.in b/freeipa.spec.in index 2db0f83..2259edf 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -72,6 +72,7 @@ BuildRequires: python-kerberos BuildRequires: python-rhsm BuildRequires: pyOpenSSL BuildRequires: pylint +BuildRequires: python-polib BuildRequires: libipa_hbac-python BuildRequires: python-memcached BuildRequires: sssd = 1.8.0 diff --git a/install/po/Makefile.in b/install/po/Makefile.in index d267bea..88d1a09 100644 ---
Re: [Freeipa-devel] [PATCH 66] Replace broken i18n shell test with Python test
On 03/26/2012 05:49 PM, John Dennis wrote: On 03/26/2012 04:24 AM, Petr Viktorin wrote: Great! Just two issues now. According to the style guide, we do allow %s in a string iff it only appears once. The reason for named substitutions is that the word order can be changed, providing context is just secondary. Why does the checker report messages with a single %s as errors? Good catch, I should have known better, I believe I was the one who wrote that section in the style guide, so I should have known better (or remembered better). Fixed. You've removed test_lang from Makefile.in, but test and .PHONY still mention it. Also fixed. Updated patch attached. Just a one-liner: The docstring of validate_anonymous_substitutions isn't up to par – there's a missing quote, extra “a”, and the '%s occurred' has no context. I suggest: We do not permit multiple anonymous substitutions in translation strings (e.g. '%s') because they do not allow translators to reorder the wording. Instead keyword substitutions should be used when there are more than one. ACK if that's fixed -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 72 Fix uses of O=REALM instead of the configured certificate subject base
Jenny Galipeau wrote: On 03/26/2012 11:28 AM, Jan Cholasta wrote: On 26.3.2012 16:15, Rob Crittenden wrote: Jan Cholasta wrote: https://fedorahosted.org/freeipa/ticket/2521 Honza You can still set a custom subject base for selfsign installations so you need a special case in valid_issuer(). For selfsign installations, the issuer is always CN=REALM Certificate Authority, no matter what is set in the subject base, so no special case is needed. I wonder if this comparison should be case insensitive too. I think the DN class already takes care of this. It may also be an optimization to cache the base in subject_base(). It can't change after install time so it should be valid the entire lifetime of the server. What if someone does $ ipa config-mod --setattr ipacertificatesubjectbase='O=Something' ? :: [ LOG] :: ipaconfig-mod_setattr ipacertificatesubjectbase positive :: [ PASS ] :: Set ipapwdexpadvnotify to OU=Bogus :: [ PASS ] :: ipacertificatesubjectbase successfully changed. :: [ LOG] :: Duration: 3s :: [ LOG] :: Assertions: 2 good, 0 bad :: [ PASS ] :: RESULT: ipaconfig-mod_setattr ipacertificatesubjectbase positive It works ... should we be getting an error?? Yes, it should fail. I thought there was already a bug open on it, though maybe we just removed the option from -mod. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 72 Fix uses of O=REALM instead of the configured certificate subject base
On 03/26/2012 01:40 PM, Rob Crittenden wrote: Jenny Galipeau wrote: On 03/26/2012 11:28 AM, Jan Cholasta wrote: On 26.3.2012 16:15, Rob Crittenden wrote: Jan Cholasta wrote: https://fedorahosted.org/freeipa/ticket/2521 Honza You can still set a custom subject base for selfsign installations so you need a special case in valid_issuer(). For selfsign installations, the issuer is always CN=REALM Certificate Authority, no matter what is set in the subject base, so no special case is needed. I wonder if this comparison should be case insensitive too. I think the DN class already takes care of this. It may also be an optimization to cache the base in subject_base(). It can't change after install time so it should be valid the entire lifetime of the server. What if someone does $ ipa config-mod --setattr ipacertificatesubjectbase='O=Something' ? :: [ LOG] :: ipaconfig-mod_setattr ipacertificatesubjectbase positive :: [ PASS ] :: Set ipapwdexpadvnotify to OU=Bogus :: [ PASS ] :: ipacertificatesubjectbase successfully changed. :: [ LOG] :: Duration: 3s :: [ LOG] :: Assertions: 2 good, 0 bad :: [ PASS ] :: RESULT: ipaconfig-mod_setattr ipacertificatesubjectbase positive It works ... should we be getting an error?? Yes, it should fail. I thought there was already a bug open on it, though maybe we just removed the option from -mod. rob Okay, I will log a bug. Will we are on this subject ... :-) Why is there option --certificate for host-add and service-add? You can not add a certificate if it doesn't match the expected issuer and assume you are doing that by checking issuer DN, based on the errror message. You can't add a service unless the host exists ... I am just not seeing the use case(s) for this! -- Jenny Galipeau jgali...@redhat.com Principal Software QA Engineer Red Hat, Inc. Security Engineering Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0029 Check expected error messages in tests
Petr Viktorin wrote: On 03/20/2012 01:39 PM, Petr Viktorin wrote: This patch adds checking error messages, not just types, to the XML-RPC tests. The checking is still somewhat hackish, since XML-RPC doesn't give us structured error info, but it should protect against regressions on issues like whether we put name or cli_name in a ValidationError. https://fedorahosted.org/freeipa/ticket/2549 Updated and rebased to current master. NACK automember wrongly was testing for non-existent users rather than automember rules but those should still be tested IMHO, perhaps with both types. There is also some inconsistency. In host you use substitution to set the hostname in the error: '%s: host not found' % fqdn1 but in others (group, hostgroup for example) the name is hardcoded. I also noticed that some reasons are unicode and others are not. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 72 Fix uses of O=REALM instead of the configured certificate subject base
Jan Cholasta wrote: On 26.3.2012 16:15, Rob Crittenden wrote: Jan Cholasta wrote: https://fedorahosted.org/freeipa/ticket/2521 Honza You can still set a custom subject base for selfsign installations so you need a special case in valid_issuer(). For selfsign installations, the issuer is always CN=REALM Certificate Authority, no matter what is set in the subject base, so no special case is needed. I wonder if this comparison should be case insensitive too. I think the DN class already takes care of this. It may also be an optimization to cache the base in subject_base(). It can't change after install time so it should be valid the entire lifetime of the server. What if someone does $ ipa config-mod --setattr ipacertificatesubjectbase='O=Something' Ok, you're right about the issuer and DN case insensitivity, so we're good there. I think that caching is still a good idea. We'll handle the immutable subjectbase as a separate problem. This is really pretty minor and isn't a show stopper, you just have to revert it and things work again. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 15 Confusing default user groups
Ondrej Hamada wrote:
On 03/19/2012 05:25 PM, Martin Kosek wrote:
On Tue, 2012-03-06 at 19:07 +0100, Ondrej Hamada wrote:
https://fedorahosted.org/freeipa/ticket/2354
There was added '(fallback)' string in the automember plugin labels
referring to automember default groups to point out, that the users are
already members of default group specified in IPA config, thus the
default group specified in automember will be additional one - a
fallback group.
Hm, looks ok. Though I would also like some second opinion for this
change. I think naming it simply Fallback Group would be better, but
we cannot change the API at this stage and rename the parameter. So this
change is a good compromise so far, IMO.
I found few issues though:
1) The label of default group parameter in automember has not been
updated, i.e. the following command still shows the old name:
# ipa automember-default-group-show --type=group
Default Group:
cn=editors,cn=groups,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
2) I think we could fix few issues in docstrings since we touch these
strings anyway:
a) Typo in doc
- label=_('Default Group'),
- doc=_('Default group for entires to land'),
+ label=_('Default (fallback) Group'),
+ doc=_('Default (fallback) group for entires to land'),
b) Non-translatable strings:
- entry_attrs['automemberdefaultgroup'] = u'No default group
set'
+ entry_attrs['automemberdefaultgroup'] = u'No default
(fallback) group set'
- entry_attrs['automemberdefaultgroup'] = u'No default group
set'
+ entry_attrs['automemberdefaultgroup'] = u'No default
(fallback) group set'
Martin
fixed
Ondra
Petr, related to handling in the UI, do you look for the string No
default group set' or just look for a string that isn't a dn?
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 66] Replace broken i18n shell test with Python test
On 03/26/2012 04:34 PM, John Dennis wrote: On 03/26/2012 01:11 PM, Petr Viktorin wrote: Just a one-liner: The docstring of validate_anonymous_substitutions isn't up to par – there's a missing quote, extra “a”, and the '%s occurred' has no context. I suggest: We do not permit multiple anonymous substitutions in translation strings (e.g. '%s') because they do not allow translators to reorder the wording. Instead keyword substitutions should be used when there are more than one. ACK if that's fixed Done, patch attached. Oh, I also meant to add that I've updated the unittest tests/test_ipalib/test_text.py to use the updated test_i18n.py code to exercise the GettextFactory and NGettextFactory classes in our framework. I'll open a new ticket for that along with a patch. With the above the unit tests will exercise at least a portion of translation pipeline in the framework. -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0025-26 Test improvements
Petr Viktorin wrote:
Patch 25 fixes errors I found by running pylint on the testsuite. They
were in code that was unused, either by error or because it only runs on
errors.
Patch 26 adds a test for the batch plugin.
In patch 25 the second test_internal_error should really be
test_unauthorized_error. I think that is a clearer name. Otherwise looks
good.
Patch 26 needs a very minor rebase to fix an error caused by improved
error code handling:
expected = Fuzzy(uinvalid 'gidnumber'.*, type 'unicode', None)
got = uinvalid 'gid': Gettext('must be an integer', domain='ipa',
localedir=None)
I tested this:
diff --git a/tests/test_xmlrpc/test_batch_plugin.py
b/tests/test_xmlrpc/test_bat
ch_plugin.py
index e4280ed..d69bfd9 100644
--- a/tests/test_xmlrpc/test_batch_plugin.py
+++ b/tests/test_xmlrpc/test_batch_plugin.py
@@ -186,7 +186,7 @@ class test_batch(Declarative):
dict(error=u'params' is required),
dict(error=u'givenname' is required),
dict(error=u'description' is required),
-dict(error=Fuzzy(uinvalid 'gidnumber'.*)),
+dict(error=Fuzzy(uinvalid 'gid'.*)),
),
),
),
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 236 Amend permissions for new DNS attributes
Martin Kosek wrote: New features in bind-dyndb-ldap and IPA DNS plugin pulled new attributes and objectclasses. ACIs and permissions need to be updated to allow users with appropriate permissions update these attributes in LDAP. This patch updates the ACI for DNS record updates and adds one new permission to update global DNS configuration. https://fedorahosted.org/freeipa/ticket/2510 ACK, pushed to master and ipa-2-2 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 237 Improve user awareness about dnsconfig
Martin Kosek wrote: Global DNS configuration is a nice tool to maintain a common DNS settings stored in LDAP which are then used for all enrolled IPA servers. However, the settings stored in LDAP override local settings in named.conf on DNS servers. This patch adds more information about global DNS configuration options in install scripts and DNS module help. https://fedorahosted.org/freeipa/ticket/2525 ACK, pushed to master and ipa-2-2 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
