Re: [Freeipa-devel] [PATCH 0010] Sort policies numerically in pwpolicy-find

2012-08-31 Thread Tomas Babej

On 08/31/2012 07:08 PM, Rob Crittenden wrote:

Tomas Babej wrote:

Hi,

this is a fairly simple one-liner.

https://fedorahosted.org/freeipa/ticket/3039

Tomas


Looks good. Can you add a unit test so we don't have a regression on 
this?


thanks

rob



I tweaked one of the existing unit tests to test the expected behaviour, 
so that we are not cluttered with tests checking the same thing.


Tomas
>From dc0b2d3054b2ac7e1b7ac3dfd18e6db95112ceb8 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Fri, 31 Aug 2012 05:29:32 -0400
Subject: [PATCH] Sort policies numerically in pwpolicy-find

Password policies in pwpolicy-find are now sorted in the expected
numerical manner. Also tweaks one of the unit tests so that it
tests this behaviour.

https://fedorahosted.org/freeipa/ticket/3039
---
 ipalib/plugins/pwpolicy.py| 2 +-
 tests/test_xmlrpc/test_pwpolicy_plugin.py | 6 --
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/ipalib/plugins/pwpolicy.py b/ipalib/plugins/pwpolicy.py
index 33c8329f70f054132134bd91a65f1a6fabf5d1c7..5ae07c40d70109468463d9a3997873e9ce8b1061 100644
--- a/ipalib/plugins/pwpolicy.py
+++ b/ipalib/plugins/pwpolicy.py
@@ -481,7 +481,7 @@ class pwpolicy_find(LDAPSearch):
 # policies with higher priority (lower number) will be at the
 # beginning of the list
 try:
-cospriority = entry[1]['cospriority'][0]
+cospriority = int(entry[1]['cospriority'][0])
 except KeyError:
 # if cospriority is not present in the entry, rather return 0
 # than crash
diff --git a/tests/test_xmlrpc/test_pwpolicy_plugin.py b/tests/test_xmlrpc/test_pwpolicy_plugin.py
index 4f57c23fd169b69972b26531c86126c416a6aaa6..3b482ce2dd7413b9968ac6046295315110768d03 100644
--- a/tests/test_xmlrpc/test_pwpolicy_plugin.py
+++ b/tests/test_xmlrpc/test_pwpolicy_plugin.py
@@ -39,7 +39,7 @@ class test_pwpolicy(XMLRPC_test):
 user = u'testuser12'
 kw = {'cospriority': 1, 'krbminpwdlife': 30, 'krbmaxpwdlife': 40, 'krbpwdhistorylength': 5, 'krbpwdminlength': 6 }
 kw2 = {'cospriority': 2, 'krbminpwdlife': 40, 'krbmaxpwdlife': 60, 'krbpwdhistorylength': 8, 'krbpwdminlength': 9 }
-kw3 = {'cospriority': 3, 'krbminpwdlife': 50, 'krbmaxpwdlife': 30, 'krbpwdhistorylength': 3, 'krbpwdminlength': 4 }
+kw3 = {'cospriority': 10, 'krbminpwdlife': 50, 'krbmaxpwdlife': 30, 'krbpwdhistorylength': 3, 'krbpwdminlength': 4 }
 global_policy = u'global_policy'
 
 def test_1_pwpolicy_add(self):
@@ -177,12 +177,14 @@ class test_pwpolicy(XMLRPC_test):
 assert_attr_equal(entry, 'krbmaxpwdlife', '30')
 assert_attr_equal(entry, 'krbpwdhistorylength', '3')
 assert_attr_equal(entry, 'krbpwdminlength', '4')
-assert_attr_equal(entry, 'cospriority', '3')
+assert_attr_equal(entry, 'cospriority', '10')
 
 def test_c_pwpolicy_find(self):
 """Test that password policies are sorted and reported properly"""
 result = api.Command['pwpolicy_find']()['result']
 assert len(result) == 4
+
+# Test that policies are sorted in numerical order
 assert result[0]['cn'] == (self.group,)
 assert result[1]['cn'] == (self.group2,)
 assert result[2]['cn'] == (self.group3,)
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH] 1052 add version to prepared replica files

2012-08-31 Thread Rob Crittenden
When installing a replica in an upgrade situation we want to be sure we 
install the same version or higher. This will have to bake a bit until 
the next full version of IPA but the idea is to prevent installing a 
newer replica file on an older server.


To test this you need to rip apart a prepared file and tweak the version 
forward or backward.


To do this, do something like:

# gpg -d replica-info-pitbull.example.com.gpg | tar xf -
# edit realm_info/realm_info
# tar cf replica-info-pitbull.example.com realm_info
# gpg --batch --homedir `pwd`/.gnupg --passphrase-fd 0 --yes --no-tty -o 
replica-info-pitbull.example.com.gpg -c replica-info-pitbull.example.com



rob
>From cf1998b2341a72b5b1a24317f64ad8976fb02bb9 Mon Sep 17 00:00:00 2001
From: Rob Crittenden 
Date: Wed, 29 Aug 2012 11:32:03 -0400
Subject: [PATCH] Add version to replica prepare file, prevent installing to
 older version

---
 install/tools/ipa-replica-install   |  4 
 install/tools/ipa-replica-prepare   |  1 +
 install/tools/man/ipa-replica-install.1 |  2 ++
 install/tools/man/ipa-replica-prepare.1 | 14 --
 ipaserver/install/installutils.py   |  7 ++-
 5 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index d7baf9c05794d95472091059cb96c54cf00bfc41..41e1ef575889ce81da5ce939095e88da44d33ed3 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -328,6 +328,10 @@ def main():
 
 config = ReplicaConfig()
 read_replica_info(dir, config)
+root_logger.debug('Installing replica file with version %d (0 means no version in prepared file).' % config.version)
+if config.version and config.version > version.NUM_VERSION:
+root_logger.error('A replica file from a newer release (%d) cannot be installed on an older version (%d)' % (config.version, version.NUM_VERSION))
+sys.exit(1)
 config.dirman_password = dirman_password
 try:
 host = get_host_name(options.no_host_dns)
diff --git a/install/tools/ipa-replica-prepare b/install/tools/ipa-replica-prepare
index 3578488264564b6335033f87a62cd4e1e7f8b719..d1ffe4e2e1d0eee3571713b24cc9d57d59acaa24 100755
--- a/install/tools/ipa-replica-prepare
+++ b/install/tools/ipa-replica-prepare
@@ -207,6 +207,7 @@ def save_config(dir, realm_name, host_name,
 config.set("realm", "domain_name", domain_name)
 config.set("realm", "destination_host", dest_host)
 config.set("realm", "subject_base", str(subject_base))
+config.set("realm", "version", str(version.NUM_VERSION))
 fd = open(dir + "/realm_info", "w")
 config.write(fd)
 
diff --git a/install/tools/man/ipa-replica-install.1 b/install/tools/man/ipa-replica-install.1
index 1a0f89a410b3409adbc790b114d14fee8e9e1952..3f4459727aafbe96280ab1abd12f113386b07921 100644
--- a/install/tools/man/ipa-replica-install.1
+++ b/install/tools/man/ipa-replica-install.1
@@ -29,6 +29,8 @@ The replica_file is created using the ipa\-replica\-prepare utility.
 If the installation fails you may need to run ipa\-server\-install \-\-uninstall before running ipa\-replica\-install again.
 
 The installation will fail if the host you are installing the replica on exists as a host in IPA or an existing replication agreement exists (for example, from a previously failed installation).
+
+A replica should only be installed on the same or higher version of IPA on the remote system.
 .SH "OPTIONS"
 .SS "BASIC OPTIONS"
 .TP
diff --git a/install/tools/man/ipa-replica-prepare.1 b/install/tools/man/ipa-replica-prepare.1
index f30ed10c17cfde9e54c3ce9556a8d03671398227..8e1e60a25628432bf380e7af1d2d2dac9abf8c8a 100644
--- a/install/tools/man/ipa-replica-prepare.1
+++ b/install/tools/man/ipa-replica-prepare.1
@@ -1,21 +1,21 @@
 .\" A man page for ipa-replica-prepare
 .\" Copyright (C) 2008 Red Hat, Inc.
-.\" 
+.\"
 .\" 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 .
-.\" 
+.\"
 .\" Author: Rob Crittenden 
-.\" 
+.\"
 .TH "ipa-replica-prepare" "1" "Mar 14 2008" "FreeIPA" "FreeIPA Manual Pages"
 .SH "NAME"
 ipa\-replica\-prepare \- Create an IPA replica file
@@ -28,9 +28,11 @@ A replica can only be created on an IPA server installed with ipa\-server\-insta
 
 You must provide the fully\-qualified hostname of the machine you want to install the replica on and a host\-specific replica_f

[Freeipa-devel] [PATCH] 1051 Fix CS replica management

2012-08-31 Thread Rob Crittenden
The naming in CS replication agreements is different from IPA 
agreements, we have to live with what the create. The master side should 
be on the local side, replica1, not the remote. This required reversing 
a few master variables.


Pass in the force flag to del_link.

Do a better job of finding the agreements on each side.

This should be ipa-csreplica-manage more in line with ipa-replica-manage.

rob
>From 18a5b9841f1214048efac82c36397c82c69b31fa Mon Sep 17 00:00:00 2001
From: Rob Crittenden 
Date: Thu, 30 Aug 2012 16:24:10 -0400
Subject: [PATCH] Fix CS replication management.

The master side should be on the local side, replica1, not the
remote. This required reversing a few master variables.

Pass in the force flag to del_link.

Do a better job of finding the agreements on each side.

https://fedorahosted.org/freeipa/ticket/2858
---
 install/tools/ipa-csreplica-manage | 56 ++
 ipaserver/install/replication.py   | 10 +++
 2 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/install/tools/ipa-csreplica-manage b/install/tools/ipa-csreplica-manage
index 6eefe8d6d3c3a942d6bef6a68f7725cef9138e0a..459752e961d89d1162d9aeb4900ac5bde3df2d6b 100755
--- a/install/tools/ipa-csreplica-manage
+++ b/install/tools/ipa-csreplica-manage
@@ -27,6 +27,7 @@ from ipapython.ipa_log_manager import *
 
 from ipapython import ipautil
 from ipaserver.install import replication, installutils
+from ipaserver.install.cainstance import PKI_INSTANCE_NAME
 from ipaserver import ipaldap
 from ipapython import version
 from ipalib import api, errors, util
@@ -80,7 +81,6 @@ class CSReplicationManager(replication.ReplicationManager):
 """
 dn = None
 cn = None
-instance_name = 'pki-ca'
 
 # if master is not None we know what dn to return:
 if master is not None:
@@ -88,14 +88,14 @@ class CSReplicationManager(replication.ReplicationManager):
 name = "master"
 else:
 name = "clone"
-cn="%sAgreement1-%s-%s" % (name, hostname, instance_name)
+cn="%sAgreement1-%s-%s" % (name, hostname, PKI_INSTANCE_NAME)
 dn = DN(('cn', cn), self.replica_dn())
 return (cn, dn)
 
 for host in self.hostnames:
 for master in ["master", "clone"]:
 try:
-cn="%sAgreement1-%s-%s" % (master, host, instance_name)
+cn="%sAgreement1-%s-%s" % (master, host, PKI_INSTANCE_NAME)
 dn = DN(('cn', cn), self.replica_dn())
 self.conn.getEntry(dn, ldap.SCOPE_BASE)
 return (cn, dn)
@@ -175,7 +175,7 @@ def list_replicas(realm, host, replica, dirman_passwd, verbose):
 peers[ent.getValue('cn')] = ['CA not configured', '']
 
 except Exception, e:
-sys.exit("Failed to get data from '%s': %s" % (host, convert_error(e)))
+sys.exit("Failed to get data from '%s' while trying to list replicas: %s" % (host, convert_error(e)))
 finally:
 conn.unbind_s()
 
@@ -204,18 +204,36 @@ def del_link(realm, replica1, replica2, dirman_passwd, force=False):
 repl1 = CSReplicationManager(realm, replica1, dirman_passwd, PORT, True)
 
 repl1.hostnames = [replica1, replica2]
-type1 = repl1.get_agreement_type(replica2)
 
-repl_list = repl1.find_ipa_replication_agreements()
+repl_list = repl1.find_replication_agreements()
 if not force and len(repl_list) <= 1:
 print "Cannot remove the last replication link of '%s'" % replica1
 print "Please use the 'del' command to remove it from the domain"
 sys.exit(1)
 
-except ldap.NO_SUCH_OBJECT:
-sys.exit("'%s' has no replication agreement for '%s'" % (replica1, replica2))
-except errors.NotFound:
-sys.exit("'%s' has no replication agreement for '%s'" % (replica1, replica2))
+replica1_master = False
+replica2_master = False
+
+(master_cn, master_dn) = repl1.agreement_dn(replica2, True)
+(clone_cn, clone_dn) = repl1.agreement_dn(replica1, False)
+
+# Determine which side is the master
+for e in repl_list:
+if (e.dn['cn'] == clone_cn and 
+e.getValue('nsDS5ReplicaHost') == replica2):
+replica1_master = False
+replica2_master = True
+break
+elif (e.dn['cn'] == master_cn):
+replica1_master = True
+replica2_master = False
+break
+
+if not replica1_master and not replica2_master:
+sys.exit("'%s' has no replication agreement for '%s'" % (replica1, replica2))
+
+repl1.hostnames = [replica1, replica2]
+
 except ldap.SERVER_DOWN, e:
 sys.exit("Unable to connect to %s: %s" % (ipautil.format_netloc(replica1, PORT), convert_error(e)))
 except Exception, e:
@@ -247,7 +265,7 @@ def

Re: [Freeipa-devel] [PATCH] 1050 prevent replica orphans

2012-08-31 Thread Rob Crittenden

Rob Crittenden wrote:

It was possible use ipa-replica-manage connect/disconnect/del to end up
orphaning or or more IPA masters. This is an attempt to catch and
prevent that case.

I tested with this topology, trying to delete B.

A <-> B <-> C

I got here by creating B and C from A, connecting B to C then deleting
the link from A to B, so it went from A -> B and A -> C to the above.

What I do is look up the servers that the delete candidate host has
connections to and see if we're the last link.

I added an escape clause if there are only two masters.

rob


Oh, this relies on my cleanruv patch 1031.

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCH] 1050 prevent replica orphans

2012-08-31 Thread Rob Crittenden
It was possible use ipa-replica-manage connect/disconnect/del to end up 
orphaning or or more IPA masters. This is an attempt to catch and 
prevent that case.


I tested with this topology, trying to delete B.

A <-> B <-> C

I got here by creating B and C from A, connecting B to C then deleting 
the link from A to B, so it went from A -> B and A -> C to the above.


What I do is look up the servers that the delete candidate host has 
connections to and see if we're the last link.


I added an escape clause if there are only two masters.

rob
>From 9260967f3ef9df090df86e9705596b7cb4d322b6 Mon Sep 17 00:00:00 2001
From: Rob Crittenden 
Date: Fri, 31 Aug 2012 11:56:58 -0400
Subject: [PATCH] When deleting a master, try to prevent orphaning other
 servers.

If you have a replication topology like A <-> B <-> C and you try
to delete server B that will leave A and C orphaned. It may also
prevent re-installation of a new master on B because the cn=masters
entry for it probably still exists on at least one of the other masters.

Check on each master that it connects to to ensure that it isn't the
last link, and fail if it is. If any of the masters are not up then
warn that this could be a bad thing but let the user continue if
they want.

Document how to remove a cn=masters entry in the man page.

https://fedorahosted.org/freeipa/ticket/2797
---
 install/tools/ipa-replica-manage   | 57 ++
 install/tools/man/ipa-replica-manage.1 | 12 +++
 2 files changed, 69 insertions(+)

diff --git a/install/tools/ipa-replica-manage b/install/tools/ipa-replica-manage
index 5023d9a8d761943515c8753237db0ed5c42ab813..21e2868042c9adfbce5ff9ca5ebf8690ab5a76a6 100755
--- a/install/tools/ipa-replica-manage
+++ b/install/tools/ipa-replica-manage
@@ -373,6 +373,48 @@ def clean_ruv(realm, ruv, options):
 thisrepl.cleanallruv(ruv)
 print "Cleanup task created"
 
+def check_last_link(delrepl, realm, dirman_passwd):
+"""
+We don't want to orphan a server when deleting another one. If you have
+A topology that looks like this:
+
+ A B
+ | |
+ | |
+ | |
+ C D
+
+If we try to delete host D it will orphan host B.
+
+What we need to do is if the master being deleted has only a single
+agreement, connect to that master and make sure it has agreements with
+more than just this master.
+
+@delrepl: a ReplicationManager object of the master being deleted
+
+returns: hostname of orphaned server or None
+"""
+replica_names = delrepl.find_ipa_replication_agreements()
+
+orphaned = []
+# Connect to each remote server and see what agreements it has
+for replica in replica_names:
+try:
+repl = replication.ReplicationManager(realm, replica, dirman_passwd)
+except ldap.SERVER_DOWN, e:
+print "Unable to validate that '%s' will not be orphaned." % replica
+if not ipautil.user_input("Continue to delete?", False):
+sys.exit("Aborted")
+continue
+names = repl.find_ipa_replication_agreements()
+if len(names) == 1 and names[0] == delrepl.hostname:
+orphaned.append(replica)
+
+if len(orphaned):
+return ', '.join(orphaned)
+else:
+return None
+
 def del_master(realm, hostname, options):
 
 force_del = False
@@ -426,6 +468,21 @@ def del_master(realm, hostname, options):
 if not ipautil.user_input("Continue to delete?", False):
 sys.exit("Deletion aborted")
 
+# Check for orphans
+masters_dn = DN(('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'), ipautil.realm_to_suffix(realm))
+try:
+masters = delrepl.conn.getList(masters_dn, ldap.SCOPE_ONELEVEL)
+except:
+sys.exit("Failed read master data from '%s': %s" % (delrepl.hostname, str(e)))
+# This only applies if we have more than 2 IPA servers, otherwise there is
+# no chance of an orphan.
+if len(masters) > 2:
+orphaned_server = check_last_link(delrepl, realm, options.dirman_passwd)
+if orphaned_server is not None:
+print "Deleting this server will orphan '%s'. " % orphaned_server
+print "You will need to reconfigure your replication topology to delete this server."
+sys.exit(1)
+
 # 4. Remove each agreement
 for r in replica_names:
 try:
diff --git a/install/tools/man/ipa-replica-manage.1 b/install/tools/man/ipa-replica-manage.1
index 4a1c489f33591ff6ac98fe7f9a16ebb6a52ee28a..3eeadd8d6f5af61d9890994f7cadf3acfdc2f3e0 100644
--- a/install/tools/man/ipa-replica-manage.1
+++ b/install/tools/man/ipa-replica-manage.1
@@ -59,6 +59,18 @@ Each IPA master server has a unique replication ID. This ID is used by 389\-ds\-
 When a master is removed, all other masters need to remove its replication ID from the list of masters. Normally this occurs automatically when a master is deleted with ipa\

Re: [Freeipa-devel] [PATCH 0010] Sort policies numerically in pwpolicy-find

2012-08-31 Thread Rob Crittenden

Tomas Babej wrote:

Hi,

this is a fairly simple one-liner.

https://fedorahosted.org/freeipa/ticket/3039

Tomas


Looks good. Can you add a unit test so we don't have a regression on this?

thanks

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCH 0010] Sort policies numerically in pwpolicy-find

2012-08-31 Thread Tomas Babej

Hi,

this is a fairly simple one-liner.

https://fedorahosted.org/freeipa/ticket/3039

Tomas
>From fd68588f8fbd28c942042fe8fb55bc3bef90e345 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Fri, 31 Aug 2012 05:29:32 -0400
Subject: [PATCH] Sort policies numerically in pwpolicy-find

https://fedorahosted.org/freeipa/ticket/3039
---
 ipalib/plugins/pwpolicy.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipalib/plugins/pwpolicy.py b/ipalib/plugins/pwpolicy.py
index 33c8329f70f054132134bd91a65f1a6fabf5d1c7..5ae07c40d70109468463d9a3997873e9ce8b1061 100644
--- a/ipalib/plugins/pwpolicy.py
+++ b/ipalib/plugins/pwpolicy.py
@@ -481,7 +481,7 @@ class pwpolicy_find(LDAPSearch):
 # policies with higher priority (lower number) will be at the
 # beginning of the list
 try:
-cospriority = entry[1]['cospriority'][0]
+cospriority = int(entry[1]['cospriority'][0])
 except KeyError:
 # if cospriority is not present in the entry, rather return 0
 # than crash
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] Reevaluation of confirmation of actions in Web UI.

2012-08-31 Thread Petr Vobornik

On 08/31/2012 05:00 PM, Petr Vobornik wrote:

Hi Endi,

I opened https://fedorahosted.org/freeipa/ticket/3035 can you please
comment? If everything seems good, I will try to implement some stuff to
RC1 (next week).

Thanks

Wrong address...

--
Petr Vobornik

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] Reevaluation of confirmation of actions in Web UI.

2012-08-31 Thread Petr Vobornik

Hi Endi,

I opened https://fedorahosted.org/freeipa/ticket/3035 can you please 
comment? If everything seems good, I will try to implement some stuff to 
RC1 (next week).


Thanks
--
Petr Vobornik

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] Paging in Web UI

2012-08-31 Thread Petr Vobornik

On 08/29/2012 06:50 PM, Endi Sukma Dewata wrote:

On 8/29/2012 8:49 AM, Petr Vobornik wrote:

Another possibility is to use VLV, but it seems to require open
connection too and only works with a predefined filter.


Apart from the possible connection problem I think it is very usable for
default views of search pages. User can freely page. I don't understand
from the wiki page if it can reasonably obtain total record count.


Correction, VLV doesn't seem to require a continuous connection.

The server will return a 'contentCount' in the VLV response:
http://www.ietf.org/proceedings/55/I-D/draft-ietf-ldapext-ldapv3-vlv-09.txt


IMO a modification of a hybrid solution may be best:

When user opens page a VLV result would be shown. User can page and see
all the data. Currently we don't support custom sorting so disability of
custom sorting isn't an issue. With predefined sorts we can even
improved these possibilities.

If there is many records and user don't want to go through pages he will
use search. If the search string is good he should fit to search limit
so I wouldn't use paging at this moment. If not the case and he really
don't know how to improve the filter he should have option to enable
paging (current implementation) and go through the records by hand. Such
record set should fit to limit of 2000 records. If not the filter is
really bad.


The thing is the filter is only used on certain attributes only. For
example I have a user with full name "Test User". Searching the "Test
User" in the filter doesn't return anything because the server doesn't
search the cn or displayName attributes. In that case I'd have to search
with first name or last name only, which may return too many results.


If we don't want to implement VLV or until that time I would set
not-paging as default setting.


How about this, when you open the search page for the first time it will
do a regular search, no paging. Instead of showing the paging controls
(Prev & Next buttons) we show "See more results..." If you click that it
will rerun the search with paging (which will give us the total number
of entries) and show the paging control.


I think we are in an agreement. It's basically what I wrote in different 
words. I just mentioned current size limit.


User workflow when searching for record (the same thing reworded):
1) open search page: VLV or normal search with empty filter
  1a) when VLV: user would see paging ui
  1b) when normal search: not sure if to show 'see more results' link 
for switching to paging. For smaller setups (<2000) it might be there, 
for larger rather not - pkey list would be truncated.

2) search for record: enter filter to textbox, normal search would be used
3) record not listed: switch to paging by clicking on 'see more results' 
Paging should be OK because most likely we would get less than 2000 
records. Otherwise filter should be improved. I guess we can't go around 
this unless we increase the limit or use other mean of paging.
4) record still not listed: page to find the record using current paging 
UI or some improved





Agreed, but if we implement the single continuous page I described in
the earlier email the page size won't be much of an issue anymore.


It may matter. For example when testing Web UI permissions. It often
requires to navigate to the last page. I can enter the number directly
and press enter or extend it for times (hypothetically) but I would
rather see those 90 permissions right away because I can be at the list
end in 0.5s.


If we use a continuous page we can use a bigger default page size, say
100 entries. We probably could optimize it such that if you scroll
directly to the bottom it will load just the last page.


Comments below.


We probably could also combine it with initial non-paging search like above, so
you'll need to click "See more results...", then the page will extend
and you can scroll to the last entry.


Yes as I wrote above.




We can also improve pager to offer some subset of pages. For example:

First Prev 22 23 *24* 25 26 Next Last Page [   ]
where 24 is current page.


Yes, that will work too.


Regarding the continuous paging: I would extend the page by clicking at
a bar at the end of the list and/or by mouse-scrolling at the end of the
list?


Ideally the scroll bar size should correspond to the visible part of the
entire page. So you should be able to go anywhere in the entire page by
moving the scroll bar around (either by keyboard, mouse click, mouse
drag, or mouse scroll). In this case no need to extend the page.
The thing is that we might have to create a table with many empty rows,
which could be big, and populate them if they become visible. Maybe
there's a way to avoid creating empty rows, we'll need to investigate.


Did you meant by 'continuous paging' this solution or the one when you 
click on 'see more results' (or scroll to last 10% of a scrollbar) and 
it would show you next page? I originally understood it as the latter 
but n

Re: [Freeipa-devel] [PATCH 81] ipa user-find --manager does not find matches

2012-08-31 Thread John Dennis
The original patch was missing a unit test to verify the fix. This 
version of the patch now includes a unit test.


--
John Dennis 

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
>From a989885f065a6d05eff16f92b259f76c3fceaace Mon Sep 17 00:00:00 2001
From: John Dennis 
Date: Fri, 31 Aug 2012 08:16:49 -0400
Subject: [PATCH 81-1] ipa user-find --manager does not find matches
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

The manager LDAP attribute is a dn pointing inside the user
container. When passed on the command it is typically a bare user
uid. The search filter will only succeed if the bare uid is converted
to a full dn because that is what is stored in the value for the
manager attribute.

The search failure is solved by calling _normalize_manager() which
does the conversion to a dn (if not already a dn).

It feels like this type of conversion should be performed in the pre
callback which allows one to modify the filter. But when the pre
callback is invoked it's complex string with the manager attribute
already inserted. This is because the LDAPSearch.execute() method
processes the options dict and constructs a filter component for each
key/value in the options dict prior to invoking the pre callback. If
we wanted to modify the manager value in the filter in the pre
callback we would have to decompose the filter string, perform dn
checking and then reassemble the filter. It's much cleaner to perform
the dn operations on the manager value before it gets embedded into
what otherwise might be a very complex filter. This is the reason why
the normalization is perfored in the execute method as opposed to the
pre callback. Other classes do similar things in their execute methods
as opposed to their callbacks's, selinuxusermap_find is one example.

Patch also introduces new unit test to verify.

https://fedorahosted.org/freeipa/ticket/2264
---
 ipalib/plugins/user.py|  7 +++
 tests/test_xmlrpc/test_user_plugin.py | 27 +++
 2 files changed, 34 insertions(+)

diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index d0f7da2..c024e85 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -642,6 +642,13 @@ class user_find(LDAPSearch):
 ),
 )
 
+def execute(self, *args, **options):
+# assure the manager attr is a dn, not just a bare uid
+manager = options.get('manager')
+if manager is not None:
+options['manager'] = self.obj._normalize_manager(manager)
+return super(user_find, self).execute(self, *args, **options)
+
 def pre_callback(self, ldap, filter, attrs_list, base_dn, scope, *keys, **options):
 assert isinstance(base_dn, DN)
 if options.get('whoami'):
diff --git a/tests/test_xmlrpc/test_user_plugin.py b/tests/test_xmlrpc/test_user_plugin.py
index 5240c8c..f146b34 100644
--- a/tests/test_xmlrpc/test_user_plugin.py
+++ b/tests/test_xmlrpc/test_user_plugin.py
@@ -657,6 +657,33 @@ class test_user(Declarative):
 ),
 ),
 
+dict(
+desc='Search for "%s" with manager "%s"' % (user2, user1),
+command=(
+'user_find', [user2], {'manager': user1}
+),
+expected=dict(
+result=[
+dict(
+dn=get_user_dn(user2),
+givenname=[u'Test'],
+homedirectory=[u'/home/tuser2'],
+loginshell=[u'/bin/sh'],
+sn=[u'User2'],
+uid=[user2],
+nsaccountlock=False,
+has_keytab=False,
+has_password=False,
+uidnumber=[fuzzy_digits],
+gidnumber=[fuzzy_digits],
+manager=[user1],
+),
+],
+summary=u'1 user matched',
+count=1,
+truncated=False,
+),
+),
 
 dict(
 desc='Delete "%s" and "%s" at the same time' % (user1, user2),
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

[Freeipa-devel] [PATCH 81] ipa user-find --manager does not find matches

2012-08-31 Thread John Dennis


--
John Dennis 

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
>From eda845c33c20259cce39621b707906808b2b9335 Mon Sep 17 00:00:00 2001
From: John Dennis 
Date: Fri, 31 Aug 2012 08:16:49 -0400
Subject: [PATCH 81] ipa user-find --manager does not find matches
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

The manager LDAP attribute is a dn pointing inside the user
container. When passed on the command it is typically a bare user
uid. The search filter will only succeed if the bare uid is converted
to a full dn because that is what is stored in the value for the
manager attribute.

The search failure is solved by calling _normalize_manager() which
does the conversion to a dn (if not already a dn).

It feels like this type of conversion should be performed in the pre
callback which allows one to modify the filter. But when the pre
callback is invoked it's complex string with the manager attribute
already inserted. This is because the LDAPSearch.execute() method
processes the options dict and constructs a filter component for each
key/value in the options dict prior to invoking the pre callback. If
we wanted to modify the manager value in the filter in the pre
callback we would have to decompose the filter string, perform dn
checking and then reassemble the filter. It's much cleaner to perform
the dn operations on the manager value before it gets embedded into
what otherwise might be a very complex filter. This is the reason why
the normalization is perfored in the execute method as opposed to the
pre callback. Other classes do similar things in their execute methods
as opposed to their callbacks's, selinuxusermap_find is one example.

https://fedorahosted.org/freeipa/ticket/2264
---
 ipalib/plugins/user.py | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index d0f7da2..c024e85 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -642,6 +642,13 @@ class user_find(LDAPSearch):
 ),
 )
 
+def execute(self, *args, **options):
+# assure the manager attr is a dn, not just a bare uid
+manager = options.get('manager')
+if manager is not None:
+options['manager'] = self.obj._normalize_manager(manager)
+return super(user_find, self).execute(self, *args, **options)
+
 def pre_callback(self, ldap, filter, attrs_list, base_dn, scope, *keys, **options):
 assert isinstance(base_dn, DN)
 if options.get('whoami'):
-- 
1.7.11.4

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel