Re: [Freeipa-devel] Activation and password reset webapp UI

2011-11-29 Thread Adam Young

On 11/29/2011 08:57 PM, Ryan Thomson wrote:

Hi Endi,

Thanks for reviewing the patch. Looks like I have some work to do.

1-2) I have to admit I didn't even try building with these patches. I 
was pretty sure install/Makefile.am would need modification to install 
it but I didn't know if submitting patches to install/Makefile.am and 
the spec file was frowned upon or not and I assumed wrong. I'll test 
by building from git next time and making sure it's ready to go from 
there on.


3) Strange. I suppose originally coding against IPA as packaged by 
RHEL then "porting" to and testing against FreeIPA as packaged in F16 
could why I didn't catch that. Gotta do everything against the latest 
in git. Got it.


4) Box width will have to be increased in CSS to compensate unless we 
want "Current Password" and "Verify Password" to get wrapped but no 
big deal.


5-9) Noted.

10) Besides "finally" being redundant, error handling isn't great 
overall, let me see if I can improve that.


11-13) Git newbie mistakes. I really don't know what I'm doing with 
regards to git. I assumed sending multiple patches where it's just me 
fixing earlier mistakes would be worse than jamming all the commits 
into a single file. I kind of just wanted to send one patch to 
implement everything but I have no idea how to do that besides get 
everything "right" in a single commit. Is that how I'm supposed to do it?


I'd submit it all as one patch, as it isn't that big, and the work is 
fairly atomic.


Once you have it working as you want,  squash it.  We have a convention 
where we number the patches  using 4 digits.  So yours  would be 0001.  
And updated verson would be 0001-1  and the next fixed version would be 
0001-2.


Git is very forgiving.  Here's what I'd do.  Once you get it working 
right make two branches.  One will continue to have all of your changes 
in it in separate patches,  and you'll use the other one to squash them.



I'm going to assume that you did all of this in you home tree in your 
master branch.  So git checkout -b  activation_password  will create a 
branch with that name from the current point.  Then,  git checkout -b 
activation_password_squashed will do the same.  You can see your 
branches with git branch.  You can see which commit they refere to with 
git log .   Then run git rebase -i HEAD~4  (assuming you 
have three commits.  Always one more than the number of commits)  and 
then use the vi commands to edit the commits into a single commit.


Once you have it all in a single commit, run the git patchformat command 
and submit that.


https://fedorahosted.org/freeipa/wiki/PatchFormat






Might be a few weeks before I get another patch submitted to fix all 
these amateur mistakes.



No problem.  Just make sure that your run git fetch ;  git rebase 
origin/master to bring your code in line with any changes made between 
now and then.




Thanks,

--Ryan

On 11/29/2011 02:35 PM, Endi Sukma Dewata wrote:

On 11/28/2011 3:11 PM, Ryan Thomson wrote:

Hello,

Attached is my amateur attempt at contributing to FreeIPA.

The patch implements an account "activation" and password reset webapp
UI to cover use cases where FreeIPA may be acting only as a backend to
services such as Samba or other web application that do no expose a
method for changing expired kerberos credentials.

This code is based on the "migration" webapp UI.

See ticket 1907.

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


Hi, thanks for the patch! I found some issues:

1. Build failed due to missing variables:

install/activation/activation.py:94: [E0602, activate] Undefined
variable 'e'
install/activation/activation.py:97: [E0602, activate] Undefined
variable 'e'

2. After fixing #1, I found that the activation folder is not installed
into /usr/share/ipa. I think this needs to be fixed in
install/Makefile.am and freeipa.spec.in.

3. After copying the folder manually I can access the activation page,
but when test it I get this error in /var/log/httpd/errors_log:

[Tue Nov 29 16:13:30 2011] [error] ERROR:root:migration context search
failed: {
'info': 'TLS error -8172:Unknown code ___f 20', 'desc': "Can't contact
LDAP serv
er"}

It probably should use ldapi instead of ldaps. See migration.py.

4. In index.html:33 I think the "Password" label should say "Current
Password" for better clarity. Also in line 41 the "Confirm" label should
say "Verify Password" for consistency with the rest of the UI.

5. In index.html:44 it should be  instead of . This is an
existing problem in migration/index.html.

6. There are broken image links in ipa_activation.css. The images have
been slightly renamed and moved into install/ui/images recently.

7. There's a broken link to the CSS file in invalid.html:7.

8. The intall/activation/jquery-ui.css doesn't seem to be used anywhere.
This is also an existing problem in migration.

9. In activation.py:72 the error message still says 'migration'.

10. In activation.py:121 the return statement in the 'finally' 

Re: [Freeipa-devel] Activation and password reset webapp UI

2011-11-29 Thread Ryan Thomson

Hi Endi,

Thanks for reviewing the patch. Looks like I have some work to do.

1-2) I have to admit I didn't even try building with these patches. I 
was pretty sure install/Makefile.am would need modification to install 
it but I didn't know if submitting patches to install/Makefile.am and 
the spec file was frowned upon or not and I assumed wrong. I'll test by 
building from git next time and making sure it's ready to go from there on.


3) Strange. I suppose originally coding against IPA as packaged by RHEL 
then "porting" to and testing against FreeIPA as packaged in F16 could 
why I didn't catch that. Gotta do everything against the latest in git. 
Got it.


4) Box width will have to be increased in CSS to compensate unless we 
want "Current Password" and "Verify Password" to get wrapped but no big 
deal.


5-9) Noted.

10) Besides "finally" being redundant, error handling isn't great 
overall, let me see if I can improve that.


11-13) Git newbie mistakes. I really don't know what I'm doing with 
regards to git. I assumed sending multiple patches where it's just me 
fixing earlier mistakes would be worse than jamming all the commits into 
a single file. I kind of just wanted to send one patch to implement 
everything but I have no idea how to do that besides get everything 
"right" in a single commit. Is that how I'm supposed to do it?


Might be a few weeks before I get another patch submitted to fix all 
these amateur mistakes.


Thanks,

--Ryan

On 11/29/2011 02:35 PM, Endi Sukma Dewata wrote:

On 11/28/2011 3:11 PM, Ryan Thomson wrote:

Hello,

Attached is my amateur attempt at contributing to FreeIPA.

The patch implements an account "activation" and password reset webapp
UI to cover use cases where FreeIPA may be acting only as a backend to
services such as Samba or other web application that do no expose a
method for changing expired kerberos credentials.

This code is based on the "migration" webapp UI.

See ticket 1907.

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


Hi, thanks for the patch! I found some issues:

1. Build failed due to missing variables:

install/activation/activation.py:94: [E0602, activate] Undefined
variable 'e'
install/activation/activation.py:97: [E0602, activate] Undefined
variable 'e'

2. After fixing #1, I found that the activation folder is not installed
into /usr/share/ipa. I think this needs to be fixed in
install/Makefile.am and freeipa.spec.in.

3. After copying the folder manually I can access the activation page,
but when test it I get this error in /var/log/httpd/errors_log:

[Tue Nov 29 16:13:30 2011] [error] ERROR:root:migration context search
failed: {
'info': 'TLS error -8172:Unknown code ___f 20', 'desc': "Can't contact
LDAP serv
er"}

It probably should use ldapi instead of ldaps. See migration.py.

4. In index.html:33 I think the "Password" label should say "Current
Password" for better clarity. Also in line 41 the "Confirm" label should
say "Verify Password" for consistency with the rest of the UI.

5. In index.html:44 it should be  instead of . This is an
existing problem in migration/index.html.

6. There are broken image links in ipa_activation.css. The images have
been slightly renamed and moved into install/ui/images recently.

7. There's a broken link to the CSS file in invalid.html:7.

8. The intall/activation/jquery-ui.css doesn't seem to be used anywhere.
This is also an existing problem in migration.

9. In activation.py:72 the error message still says 'migration'.

10. In activation.py:121 the return statement in the 'finally' clause is
redundant if there's an exception because it will return another page.
This return statement should be moved inside the 'try' clause. Also we
might need another return statement in the 'except' clause to handle the
case where the errno doesn't match EPERM or EIO.

11. The file actually contains 3 patches. It's better to put them in
separate files because in case one of them needs revision you won't have
to resend everything. Another option is to squash them into a single patch.

12. The second patch contains the first patch file.

13. There are some warnings about extra whitespaces when applying the
patch. Try applying the patch with --whitespace=fix then reexporting it
again.



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


Re: [Freeipa-devel] make-lint failures

2011-11-29 Thread John Dennis

On 11/29/2011 05:12 PM, Alexander Bokovoy wrote:

On Tue, 29 Nov 2011, Rob Crittenden wrote:

Seeing some new make-lint failures after the reworking of ipaldap
function signatures;

$ ./make-lint
ipaserver/install/adtrustinstance.py:101: [E1120,
ADTRUSTInstance.__create_samba_user] No value passed for parameter
'modlist' in function call
ipaserver/install/adtrustinstance.py:190: [E1120,
ADTRUSTInstance.__create_samba_domain_object] No value passed for
parameter 'modlist' in function call
ipaserver/install/adtrustinstance.py:198: [E1120,
ADTRUSTInstance.__create_samba_domain_object] No value passed for
parameter 'modlist' in function call

I wonder if the signature needs to be:

 def add_s(self, dn, modlist=None):

For the case were dn isn't an Entry we probably need to raise an
exception if modlist is None (or test to see what python-ldap add_s
does).

The original LDAPObject.add_s() has modlist as non-optional argument:
 def add_s(self,dn,modlist):

I don't think it is wise to break that API assumption.

In all the cases above it should get .add_s(entry) replaced by
.addEntry(entry).

The reason these failures are shown is because Martin reverted the
earlier version of Sumit's patch that you mistakenly committed. Sumit
has produced new patch already but there is one minor issue in it
(another .add_s() ->  .addEntry() replacement needs to be done).



FWIW this is a perfect example of why not using ldap2 in the installer 
code is problematic. When the installer code is directly invoking a 
native ldap method though inheritance it becomes difficult to pass 
different object types to the methods. In IPA there are many places 
where we would benefit from using IPA custom classes (e.g. Entry). For 
the DN work I would also like to pass DN objects. I had to add an entire 
class subclassed from LDAPObject for no purpose other than to support 
object conversion on native LDAP methods and then instantiate that class 
in the installer conn objects. It would be so much nicer if we had an 
abstraction layer that knew about our preferred internal objects 
exposing an LDAP interface suited to our needs. We have such an 
abstraction, it's called ldap2, but alas the installer code doesn't use 
it :-(


--
John Dennis 

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] Activation and password reset webapp UI

2011-11-29 Thread Endi Sukma Dewata

On 11/28/2011 3:11 PM, Ryan Thomson wrote:

Hello,

Attached is my amateur attempt at contributing to FreeIPA.

The patch implements an account "activation" and password reset webapp
UI to cover use cases where FreeIPA may be acting only as a backend to
services such as Samba or other web application that do no expose a
method for changing expired kerberos credentials.

This code is based on the "migration" webapp UI.

See ticket 1907.

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


Hi, thanks for the patch! I found some issues:

1. Build failed due to missing variables:

install/activation/activation.py:94: [E0602, activate] Undefined 
variable 'e'
install/activation/activation.py:97: [E0602, activate] Undefined 
variable 'e'


2. After fixing #1, I found that the activation folder is not installed 
into /usr/share/ipa. I think this needs to be fixed in 
install/Makefile.am and freeipa.spec.in.


3. After copying the folder manually I can access the activation page, 
but when test it I get this error in /var/log/httpd/errors_log:


[Tue Nov 29 16:13:30 2011] [error] ERROR:root:migration context search 
failed: {
'info': 'TLS error -8172:Unknown code ___f 20', 'desc': "Can't contact 
LDAP serv

er"}

It probably should use ldapi instead of ldaps. See migration.py.

4. In index.html:33 I think the "Password" label should say "Current 
Password" for better clarity. Also in line 41 the "Confirm" label should 
say "Verify Password" for consistency with the rest of the UI.


5. In index.html:44 it should be  instead of . This is an 
existing problem in migration/index.html.


6. There are broken image links in ipa_activation.css. The images have 
been slightly renamed and moved into install/ui/images recently.


7. There's a broken link to the CSS file in invalid.html:7.

8. The intall/activation/jquery-ui.css doesn't seem to be used anywhere. 
This is also an existing problem in migration.


9. In activation.py:72 the error message still says 'migration'.

10. In activation.py:121 the return statement in the 'finally' clause is 
redundant if there's an exception because it will return another page. 
This return statement should be moved inside the 'try' clause. Also we 
might need another return statement in the 'except' clause to handle the 
case where the errno doesn't match EPERM or EIO.


11. The file actually contains 3 patches. It's better to put them in 
separate files because in case one of them needs revision you won't have 
to resend everything. Another option is to squash them into a single patch.


12. The second patch contains the first patch file.

13. There are some warnings about extra whitespaces when applying the 
patch. Try applying the patch with --whitespace=fix then reexporting it 
again.


--
Endi S. Dewata

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


Re: [Freeipa-devel] make-lint failures

2011-11-29 Thread Alexander Bokovoy
On Tue, 29 Nov 2011, Rob Crittenden wrote:
> Seeing some new make-lint failures after the reworking of ipaldap
> function signatures;
> 
> $ ./make-lint
> ipaserver/install/adtrustinstance.py:101: [E1120,
> ADTRUSTInstance.__create_samba_user] No value passed for parameter
> 'modlist' in function call
> ipaserver/install/adtrustinstance.py:190: [E1120,
> ADTRUSTInstance.__create_samba_domain_object] No value passed for
> parameter 'modlist' in function call
> ipaserver/install/adtrustinstance.py:198: [E1120,
> ADTRUSTInstance.__create_samba_domain_object] No value passed for
> parameter 'modlist' in function call
> 
> I wonder if the signature needs to be:
> 
> def add_s(self, dn, modlist=None):
> 
> For the case were dn isn't an Entry we probably need to raise an
> exception if modlist is None (or test to see what python-ldap add_s
> does).
The original LDAPObject.add_s() has modlist as non-optional argument:
def add_s(self,dn,modlist):

I don't think it is wise to break that API assumption.

In all the cases above it should get .add_s(entry) replaced by 
.addEntry(entry).

The reason these failures are shown is because Martin reverted the 
earlier version of Sumit's patch that you mistakenly committed. Sumit 
has produced new patch already but there is one minor issue in it 
(another .add_s() -> .addEntry() replacement needs to be done).

-- 
/ Alexander Bokovoy

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


[Freeipa-devel] make-lint failures

2011-11-29 Thread Rob Crittenden
Seeing some new make-lint failures after the reworking of ipaldap 
function signatures;


$ ./make-lint
ipaserver/install/adtrustinstance.py:101: [E1120, 
ADTRUSTInstance.__create_samba_user] No value passed for parameter 
'modlist' in function call
ipaserver/install/adtrustinstance.py:190: [E1120, 
ADTRUSTInstance.__create_samba_domain_object] No value passed for 
parameter 'modlist' in function call
ipaserver/install/adtrustinstance.py:198: [E1120, 
ADTRUSTInstance.__create_samba_domain_object] No value passed for 
parameter 'modlist' in function call


I wonder if the signature needs to be:

def add_s(self, dn, modlist=None):

For the case were dn isn't an Entry we probably need to raise an 
exception if modlist is None (or test to see what python-ldap add_s does).


rob

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


Re: [Freeipa-devel] [PATCH] ipa-client-install with --no-sssd option should check for nss_ldap

2011-11-29 Thread Rob Crittenden

Ondrej Hamada wrote:

On 11/11/2011 02:55 PM, Ondrej Hamada wrote:

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

In order to check presence of nss_ldap when installing client with
'--no-sssd' option there was added code into ipa-client-install. Check
is base on existence of nss_ldap configuration files. This
configuration could be in 'etc/ldap.conf', '/etc/nss_ldap.conf' or
'/etc/libnss_ldap.conf'. Presence of any of these files is considered
as success otherwise failure.



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

I've rewritten it. Additionally it checks for existence of nss-pam-ldapd
and makes the results reusable by configure_{ldap|nslcd}_conf() functions.

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

In order to check presence of nss_ldap or nss-pam-ldapd when installing
client
with '--no-sssd' option there was added code into ipa-client-install.
Checking is based on existence of nss_ldap configuration files. This
configuration could be in 'etc/ldap.conf', '/etc/nss_ldap.conf' or
'/etc/libnss_ldap.conf'. Optionaly the nss_ldap could cooperate with
pam_ldap
module and hence the presence of it is checked by looking for
'pam_ldap.conf' file.
Existence of nss-pam-ldapd is checked against existence of 'nslcd.conf'
file.
All this checking is done by function nssldap_exists().
Because both main modules are maintained by two different functions, the
function
returns tuple containing return code and dictionary structure - its key
is name
of target function and value is list of existing configuration files.
Files to check are specified inside the nssldap_exists() function.

In order to fit the returned values, the functions
configure_{ldap|nslcd}_conf()
were slightly modified. They accept one more parameter which is list of
existing files.
They are not checking existence of above mentioned files anymore.


The patch looks good, just a couple of issues.

1. In the nslcd configurator you add ''.join(files). Did you mean 
','.join(files)?


2. The commit message lines wrap making it difficult to read. Can you 
limit the lines to ~70 chars per line?


3. I think the message printed when neither package is available can be 
simplified to:


One of these packages must be installed: nss_ldap or nss-pam-ldapd

It needs a rebase too.

rob

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


Re: [Freeipa-devel] [PATCH] Add ipasam samba passdb backend

2011-11-29 Thread Alexander Bokovoy
On Tue, 29 Nov 2011, Sumit Bose wrote:
> @@ -199,10 +216,11 @@ class ADTRUSTInstance(service.Service):
>  self.admin_conn.addEntry(entry)
>  
>  entry = ipaldap.Entry(self.smb_dom_dn)
> -entry.setValues("objectclass", ["sambaDomain", "nsContainer"])
> +entry.setValues("objectclass", [self.OBJC_DOMAIN, "nsContainer"])
>  entry.setValues("cn", self.domain_name)
> -entry.setValues("sambaDomainName", self.netbios_name)
> -entry.setValues("sambaSID", self.__gen_sid_string())
> +entry.setValues(self.ATTR_FLAT_NAME, self.netbios_name)
> +entry.setValues(self.ATTR_SID, self.__gen_sid_string())
> +entry.setValues(self.ATTR_GUID, str(uuid.uuid4()))
>  #TODO: which MAY attributes do we want to set ?
>  self.admin_conn.add_s(entry)
Could you please also convert this one to .addEntry(entry)?
I think it is the last one left...

-- 
/ Alexander Bokovoy

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


[Freeipa-devel] [PATCH] fix memleaks

2011-11-29 Thread Simo Sorce
Found a couple of memleaks while reviewing code.

Attached.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
>From 70840691e48e1ac89002499c08a9dd4fdcae7c50 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Sun, 20 Nov 2011 20:50:11 -0500
Subject: [PATCH] ipa-kdb: fix memleaks in ipa_kdb_mspac.c

---
 daemons/ipa-kdb/ipa_kdb_mspac.c |   13 +++--
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index cce1ca9060f0e03d525bb87d843bdd5811e9d20b..0c0da75ca8dbedf12e39e8ec5d87bfd4cd485d4a 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -466,7 +466,7 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 TALLOC_CTX *tmpctx;
 struct ipadb_e_data *ied;
 struct ipadb_context *ipactx;
-LDAPMessage *results;
+LDAPMessage *results = NULL;
 LDAPMessage *lentry;
 DATA_BLOB pac_data;
 krb5_data data;
@@ -479,11 +479,6 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 return KRB5_KDB_DBNOTINITED;
 }
 
-tmpctx = talloc_new(NULL);
-if (!tmpctx) {
-return ENOMEM;
-}
-
 ied = (struct ipadb_e_data *)client->e_data;
 if (ied->magic != IPA_E_DATA_MAGIC) {
 return EINVAL;
@@ -493,6 +488,11 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 return 0;
 }
 
+tmpctx = talloc_new(NULL);
+if (!tmpctx) {
+return ENOMEM;
+}
+
 memset(&pac_info, 0, sizeof(pac_info));
 pac_info.logon_info.info = talloc_zero(tmpctx, struct PAC_LOGON_INFO);
 if (!tmpctx) {
@@ -542,6 +542,7 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
 kerr = krb5_pac_add_buffer(kcontext, *pac, KRB5_PAC_LOGON_INFO, &data);
 
 done:
+ldap_msgfree(results);
 talloc_free(tmpctx);
 return kerr;
 }
-- 
1.7.7.1

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

[Freeipa-devel] [PATCH] fix uninitialized free() [Fwd: daemons/ipa-kdb]

2011-11-29 Thread Simo Sorce
I pushed the attached patch under the oneliner rule
-- 
Simo Sorce * Red Hat, Inc * New York
--- Begin Message ---
 daemons/ipa-kdb/ipa_kdb_principals.c |1 +
 1 file changed, 1 insertion(+)

New commits:
commit e727dc50ccdc9c62e5c4a374f03d0b9fa2d6b19c
Author: Simo Sorce 
Date:   Sun Nov 20 20:49:51 2011 -0500

ipa-kdb: fix free() of uninitialized var

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c 
b/daemons/ipa-kdb/ipa_kdb_principals.c
index f5bef84..431fd27 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -91,6 +91,7 @@ static int ipadb_ldap_attr_to_tl_data(LDAP *lcontext, 
LDAPMessage *le,
 
 *result = NULL;
 prev = NULL;
+next = NULL;
 vals = ldap_get_values_len(lcontext, le, attrname);
 if (vals) {
 for (i = 0; vals[i]; i++) {


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

Re: [Freeipa-devel] [PATCH] 6 Sort password policy by priority

2011-11-29 Thread Rob Crittenden

Ondrej Hamada wrote:

On 11/29/2011 03:46 PM, Ondrej Hamada wrote:

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

'ipa pwpolicy-find' output is now sorted by priority of the policies.
Lower position means lower priority. Global policy is then at the bottom.

The changes has also affected LDAPSearch class in baseldap.py:
LDAPSearch class sorts the search results by primary key be default
(which is usually 'cn'). Therefor a function pointer entries_sortfn
was added. If no sorting function exists, default sorting by primary key
is used.

Sorting function had to be introduced due to the fact that pwpolicy's
primary
key is also it's 'cn' and global policy is not allowed to have any
priority.


forget to attach the patch, sorry :-[




Doesn't work with the lite-server due to changing a ReadOnly value. I 
think you have the right idea you just need to assign the sorting 
statically instead of dynamically (it won't change after finalization).


$ ./ipa pwpolicy-add editors --minlife=2 --priority=9

ipa: DEBUG: Created connection context.ldap2
ipa: DEBUG: raw: pwpolicy_add(u'editors', krbminpwdlife=2, 
cospriority=9, all=False, raw=False, version=u'2.15')
ipa: DEBUG: pwpolicy_add(u'editors', krbminpwdlife=2, cospriority=9, 
all=False, raw=False, version=u'2.15')
ipa: DEBUG: raw: cosentry_add(u'editors', 
krbpwdpolicyreference=u'cn=editors,cn=GREYOAK.COM,cn=kerberos,dc=greyoak,dc=com', 
cospriority=9)
ipa: DEBUG: cosentry_add(u'editors', 
krbpwdpolicyreference=u'cn=editors,cn=GREYOAK.COM,cn=kerberos,dc=greyoak,dc=com', 
cospriority=9, all=False, raw=False)

ipa: DEBUG: raw: group_show(u'editors', all=True)
ipa: DEBUG: group_show(u'editors', rights=False, all=True, raw=False)
ipa: DEBUG: raw: cosentry_find(None, cospriority=9)
ipa: DEBUG: cosentry_find(None, cospriority=9, all=False, raw=False, 
pkey_only=False)
ipa: ERROR: non-public: AttributeError: locked: cannot set 
cosentry_find.entries_sortfn to  at 0x7f7abc33ec80>

Traceback (most recent call last):
  File "/home/rcrit/redhat/freeipa-review/ipaserver/rpcserver.py", line 
223, in wsgi_execute

result = self.Command[name](*args, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/frontend.py", line 
438, in __call__

ret = self.run(*args, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/frontend.py", line 
756, in run

return self.execute(*args, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/plugins/baseldap.py", 
line 700, in execute

ldap, dn, entry_attrs, attrs_list, *keys, **options
  File "/home/rcrit/redhat/freeipa-review/ipalib/plugins/pwpolicy.py", 
line 346, in pre_callback

cospriority=options.get('cospriority')
  File "/home/rcrit/redhat/freeipa-review/ipalib/frontend.py", line 
438, in __call__

ret = self.run(*args, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/frontend.py", line 
756, in run

return self.execute(*args, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/plugins/baseldap.py", 
line 700, in execute

ldap, dn, entry_attrs, attrs_list, *keys, **options
  File "/home/rcrit/redhat/freeipa-review/ipalib/plugins/pwpolicy.py", 
line 127, in pre_callback

self.obj.check_priority_uniqueness(*keys, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/plugins/pwpolicy.py", 
line 101, in check_priority_uniqueness

cospriority=options['cospriority']
  File "/home/rcrit/redhat/freeipa-review/ipalib/frontend.py", line 
438, in __call__

ret = self.run(*args, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/frontend.py", line 
756, in run

return self.execute(*args, **options)
  File "/home/rcrit/redhat/freeipa-review/ipalib/plugins/baseldap.py", 
line 1620, in execute
self.entries_sortfn=lambda x,y: 
cmp(x[1][self.obj.primary_key.name][0].lower(), 
y[1][self.obj.primary_key.name][0].lower())
  File "/home/rcrit/redhat/freeipa-review/ipalib/base.py", line 131, in 
__setattr__

SET_ERROR % (self.__class__.__name__, name, value)
AttributeError: locked: cannot set cosentry_find.entries_sortfn to 
 at 0x7f7abc33ec80>
ipa: INFO: ad...@greyoak.com: pwpolicy_add(u'editors', krbminpwdlife=2, 
cospriority=9, all=False, raw=False, version=u'2.15'): AttributeError

ipa: DEBUG: response: InternalError: an internal error has occurred
ipa: DEBUG: Destroyed connection context.ldap2

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


Re: [Freeipa-devel] [PATCH] 166+167 Zonemgr improvements

2011-11-29 Thread Martin Kosek
On Tue, 2011-11-29 at 10:39 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Mon, 2011-11-14 at 10:37 +0100, Martin Kosek wrote:
> >> On Fri, 2011-11-11 at 10:22 -0500, Rob Crittenden wrote:
> >>> Martin Kosek wrote:
>  There are 2 patches improving our zone zonemgr:
> 
>  1) ipa-server-install --zonemgr option validation and normalization +
>  the same thing in ipa dnszone-add/mod --admin-email. They now allow and
>  correctly process '.' in a local-part of the zonemgr e-mail (it is
>  encoded as '\.'.
> 
>  How to test:
> 
>  ipa-server-install -p secret123 -a secret123 --setup-dns 
>  --zonemgr=foo@example.com
>  OR if e-mail is passed in SOA format:
>  ipa-server-install -p secret123 -a secret123 --setup-dns 
>  --zonemgr='foo\.bar.example.com'
> 
>  In both cases, the zonemgr e-mail will be set to correct format: 'foo
>  \.bar.example.com'.
> 
>  2) Our default zonemgr is changed to follow RFC 2142 recommendation -
>  hostmaster@
> 
>  hostmaster is an alias to root anyway (see /etc/aliases).
> 
>  Martin
> >>>
> >>> NACK 166, this illegal address is allowed: foo\.bar\.baz\.com
> >>>
> >>> ACK 167 when 166 is ready.
> >>>
> >>> rob
> >>
> >> Are you sure that you quoted the string in shell properly? It likes to
> >> eat backslashes when one is not cautious. The zonemgr value, including
> >> backslashes, should then show up in `ipa dnszone-show ZONE'.
> >>
> >> This is my output:
> >>
> >> # ipa dnszone-mod example.com --admin-email='foo\.bar\.baz\.com'
> >> ipa: ERROR: invalid 'admin_email': address domain is not fully qualified
> >> ("example.com" instead of just "example")
> >>
> >> Anyway, attaching a rebased patch (it collided with my patch 120).
> >>
> >> Martin
> >
> > I rebased both patches. I also fixed a bug when ipa-replica-prepare
> > $HOST --ip-address=$IP was failing because None was passed as zonemgr.
> >
> > Martin
> 
> 
> $ ./make-lint
> ipaserver/install/bindinstance.py:392: [E0602, BindInstance.setup] 
> Undefined variable 'normalize_zonemgr'
> 
> Fixed with:
> 
> --- a/ipaserver/install/bindinstance.py
> +++ b/ipaserver/install/bindinstance.py
> @@ -32,7 +32,7 @@ from ipapython import sysrestore
>   from ipapython import ipautil
>   from ipalib.constants import DNS_ZONE_REFRESH
>   from ipalib.parameters import IA5Str
> -from ipalib.util import validate_zonemgr
> +from ipalib.util import validate_zonemgr, normalize_zonemgr
>   from ipapython.ipa_log_manager import *
> 
>   import ipalib
> 
> ACK for both with this import fixed.
> 
> rob

Good catch. This must have been some late fix again. Fixed.

Both patches pushed to master.

Martin

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


Re: [Freeipa-devel] [PATCH] 016 Fixed: Some widgets do not have space for validation error message

2011-11-29 Thread Endi Sukma Dewata

On 11/29/2011 9:17 AM, Simo Sorce wrote:

I couldn't find a clear ACK for this patch but it has been pushed as
well.


Correct.


Guys please try to better report on the list when patches are pushed and
close the thread.


Yes, sorry, sometimes I missed a few, my fault.

--
Endi S. Dewata

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


Re: [Freeipa-devel] [PATCH] 888 always verify hostname

2011-11-29 Thread Martin Kosek
On Tue, 2011-11-29 at 10:18 -0500, Simo Sorce wrote:
> On Tue, 2011-10-11 at 17:07 +0200, Martin Kosek wrote:
> > On Fri, 2011-10-07 at 09:18 -0400, Rob Crittenden wrote:
> > > Martin Kosek wrote:
> > > >>
> > > >> Yes but the entry is added /etc/hosts at the very END of installation,
> > > >> apparently too late for some things. We can alternately add this prior
> > > >> to configuring anything else.
> > > >
> > > > But we add the entry to /etc/hosts right in the beginning. After the
> > > > line marked with<<  is printed. I double-checked it right now.
> > > 
> > > Ok, this is totally freaky then. See ticket 
> > > https://fedorahosted.org/freeipa/ticket/1931
> > > >
> > 
> > I think it is worth mentioning there that the /etc/hosts entry is added
> > in the beginning only if the hostname is not resolvable and IP address
> > is passed by the user, i.e. only when the following line printed:
> > 
> > # ipa-server-install --setup-dns (or --no-host-dns)
> > ...
> > Please provide the IP address to be used for this host name: 10.16.78.50
> > Adding [10.16.78.50 ipa.example.com] to your /etc/hosts file
> > ...
> > 
> > I saw that 1931 should be solved by a new custom hostname parameter
> > passed to bind-dyndb-ldap plugin.
> > 
> > 
> > I did some additional testing of my proposed patch 140 and it behaved
> > fine. It is able to catch misconfigured /etc/hosts in both following ways:
> > 
> > 1) invalid hostname for given IP address
> > 
> > 1.2.3.4  foo
> > 
> > or short name first:
> > 
> > 1.2.3.4 foo foo.example.com
> > 
> > 
> > To sum this up - I think the patch is ready for review.
> 
> What's the status of this patch ?
> 
> Simo.
> 

All patches related to this topic has been reviewed, acked and pushed in
different thread named "[PATCH] 140 + 148 + 147 Hostname fixes".

Both relevant tickets has been fixed with these 3 patches:

https://fedorahosted.org/freeipa/ticket/1923
https://fedorahosted.org/freeipa/ticket/1931

But you are right that no information that the discussion continues
somewhere else was given here.

Martin

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


Re: [Freeipa-devel] [PATCH] 166+167 Zonemgr improvements

2011-11-29 Thread Rob Crittenden

Martin Kosek wrote:

On Mon, 2011-11-14 at 10:37 +0100, Martin Kosek wrote:

On Fri, 2011-11-11 at 10:22 -0500, Rob Crittenden wrote:

Martin Kosek wrote:

There are 2 patches improving our zone zonemgr:

1) ipa-server-install --zonemgr option validation and normalization +
the same thing in ipa dnszone-add/mod --admin-email. They now allow and
correctly process '.' in a local-part of the zonemgr e-mail (it is
encoded as '\.'.

How to test:

ipa-server-install -p secret123 -a secret123 --setup-dns 
--zonemgr=foo@example.com
OR if e-mail is passed in SOA format:
ipa-server-install -p secret123 -a secret123 --setup-dns 
--zonemgr='foo\.bar.example.com'

In both cases, the zonemgr e-mail will be set to correct format: 'foo
\.bar.example.com'.

2) Our default zonemgr is changed to follow RFC 2142 recommendation -
hostmaster@

hostmaster is an alias to root anyway (see /etc/aliases).

Martin


NACK 166, this illegal address is allowed: foo\.bar\.baz\.com

ACK 167 when 166 is ready.

rob


Are you sure that you quoted the string in shell properly? It likes to
eat backslashes when one is not cautious. The zonemgr value, including
backslashes, should then show up in `ipa dnszone-show ZONE'.

This is my output:

# ipa dnszone-mod example.com --admin-email='foo\.bar\.baz\.com'
ipa: ERROR: invalid 'admin_email': address domain is not fully qualified
("example.com" instead of just "example")

Anyway, attaching a rebased patch (it collided with my patch 120).

Martin


I rebased both patches. I also fixed a bug when ipa-replica-prepare
$HOST --ip-address=$IP was failing because None was passed as zonemgr.

Martin



$ ./make-lint
ipaserver/install/bindinstance.py:392: [E0602, BindInstance.setup] 
Undefined variable 'normalize_zonemgr'


Fixed with:

--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -32,7 +32,7 @@ from ipapython import sysrestore
 from ipapython import ipautil
 from ipalib.constants import DNS_ZONE_REFRESH
 from ipalib.parameters import IA5Str
-from ipalib.util import validate_zonemgr
+from ipalib.util import validate_zonemgr, normalize_zonemgr
 from ipapython.ipa_log_manager import *

 import ipalib

ACK for both with this import fixed.

rob

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


Re: [Freeipa-devel] [PATCH] 888 always verify hostname

2011-11-29 Thread Simo Sorce
On Tue, 2011-10-11 at 17:07 +0200, Martin Kosek wrote:
> On Fri, 2011-10-07 at 09:18 -0400, Rob Crittenden wrote:
> > Martin Kosek wrote:
> > >>
> > >> Yes but the entry is added /etc/hosts at the very END of installation,
> > >> apparently too late for some things. We can alternately add this prior
> > >> to configuring anything else.
> > >
> > > But we add the entry to /etc/hosts right in the beginning. After the
> > > line marked with<<  is printed. I double-checked it right now.
> > 
> > Ok, this is totally freaky then. See ticket 
> > https://fedorahosted.org/freeipa/ticket/1931
> > >
> 
> I think it is worth mentioning there that the /etc/hosts entry is added
> in the beginning only if the hostname is not resolvable and IP address
> is passed by the user, i.e. only when the following line printed:
> 
> # ipa-server-install --setup-dns (or --no-host-dns)
> ...
> Please provide the IP address to be used for this host name: 10.16.78.50
> Adding [10.16.78.50 ipa.example.com] to your /etc/hosts file
> ...
> 
> I saw that 1931 should be solved by a new custom hostname parameter
> passed to bind-dyndb-ldap plugin.
> 
> 
> I did some additional testing of my proposed patch 140 and it behaved
> fine. It is able to catch misconfigured /etc/hosts in both following ways:
> 
> 1) invalid hostname for given IP address
> 
> 1.2.3.4  foo
> 
> or short name first:
> 
> 1.2.3.4 foo foo.example.com
> 
> 
> To sum this up - I think the patch is ready for review.

What's the status of this patch ?

Simo.

-- 
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] 016 Fixed: Some widgets do not have space for validation error message

2011-11-29 Thread Simo Sorce
I couldn't find a clear ACK for this patch but it has been pushed as
well.

Guys please try to better report on the list when patches are pushed and
close the thread.

Simo.

On Thu, 2011-09-29 at 14:52 +0200, Petr Vobornik wrote:
> On 09/27/2011 10:54 PM, Endi Sukma Dewata wrote:
> > On 9/27/2011 7:50 AM, Petr Vobornik wrote:
> >> I've added padding and unified font-weight in error-message style. The
> >> padding is added because text of the message was right on the border
> >> which wasn't much readable. Font-weight is added because sometimes it
> >> inherits font-weight from other style and so it is inconsistent with
> >> other appearances. I'm not sure about the 'bold', though.
> >
> > This is good but can we use a new CSS class for the error message? I'd
> > rather not change the jquery-ui.css (removing white spaces is ok)
> > because it will be difficult to keep track the changes if we need to
> > upgrade it.
> Moved this change to ipa.css (no need to add extra class, it will 
> overwrite it as long ipa.css is linked after jquery-ui.css).
> >
> > There's another minor issue, but this one can be fixed separately if you
> > want. Try editing a self-service permission, uncheck all attributes,
> > then click Update. The undo link and the error message appear in 2
> > separate lines.
> Kept them on 2 lines.
> > I think it's better to show them in a single line. When
> > we fix the attributes widget to inherit from table, they will fit into
> > the footer.
> I agree.
> > The padding should be consistent as well, right now the undo
> > box is smaller than the error message.
> Added padding to undo. Added 1px margin-bottom to undo to separate undo 
> and error message borders when they are on two lines.
> 
> Changed display of undo in attributes_widget from inline to inline-block 
> - looks better with added padding.
> 
> ___
> 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] 288 Disable enroll button if nothing selected.

2011-11-29 Thread Simo Sorce
This patch was also pushed.

On Thu, 2011-09-29 at 13:59 +0200, Petr Vobornik wrote:
> On 09/29/2011 06:50 AM, Endi Sukma Dewata wrote:
> > On 9/28/2011 7:59 PM, Adam Young wrote:
> >> On 09/28/2011 06:50 PM, Endi Sukma Dewata wrote:
> >>> A new IPA.dialog_button class has been added to encapsulate the
> >>> buttons in the dialog box so they can be managed more easily.
> >>>
> >>> The adder dialog has been modified to disable the enroll button if
> >>> there is no entries selected.
> >>>
> >>> Ticket #1856
> >
> > The solution is simple, but it requires refactoring which probably
> > should have been done much earlier. So this patch is like a combination
> > of several patches.
> >
> 
> > As a further enhancement, the IPA.dialog_button class probably can be
> > combined with the IPA.action_button/IPA.button class so we can use icons
> > or enable/disable buttons in a more consistent way. This is one step in
> > that direction.
> I Agree. This would be nice especially in association table widget.
> 
> ACK
> 
> 

-- 
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] 275 Use editable combobox for service type.

2011-11-29 Thread Simo Sorce
This patch was also pushed.

On Thu, 2011-09-29 at 11:35 +0200, Petr Vobornik wrote:
> On 09/28/2011 12:59 AM, Endi Sukma Dewata wrote:
> > On 9/16/2011 12:16 PM, Endi Sukma Dewata wrote:
> >> The service type field in the service adder dialog has been modified
> >> to use an editable combobox.
> >>
> >> Ticket #1633.
> >
> > Rebased (removed undo parameter).
> 
> ACK

-- 
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] 286 Fixed tab and dialog widths.

2011-11-29 Thread Simo Sorce
This patch was pushed too.

(I am closing loose ends)

On Thu, 2011-09-29 at 11:20 +0200, Petr Vobornik wrote:
> On 09/28/2011 01:02 AM, Endi Sukma Dewata wrote:
> > The width of the 1st level tab has been modified to expand according
> > to the size of the tab label.
> >
> > The width of the adder dialogs have been increased to allow longer
> > button labels.
> >
> > Ticket #1825
> ACK
> 

-- 
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] 266 Fixed sudo rule association dialogs.

2011-11-29 Thread Simo Sorce
This patch too seem to have been pushed.

On Fri, 2011-09-09 at 12:55 +0200, Petr Vobornik wrote:
> On 09/08/2011 06:53 PM, Endi Sukma Dewata wrote:
> > On 9/8/2011 10:28 AM, Endi Sukma Dewata wrote:
> >> The adder dialog for the user and host tables in sudo rule details
> >> page have been fixed to use --not-in-sudorules to avoid showing
> >> entries that are already added into the rule either directly or
> >> indirectly via groups.
> >>
> >> This does not apply to the command and run-as tables because they
> >> do not support such option.
> >>
> >> Ticket #1768
> >
> > Wrong email title. It should be patch #266.
> >
> ACK
> 

-- 
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] 265 Fixed layout problem in permission adder dialog.

2011-11-29 Thread Simo Sorce
Apparently this patch was pushed.

On Fri, 2011-09-09 at 10:19 +0200, Petr Vobornik wrote:
> On 09/08/2011 06:51 PM, Endi Sukma Dewata wrote:
> > On 9/8/2011 11:13 AM, Petr Vobornik wrote:
> >> In IPA.details_table_section:
> >> 1)not renamed list_section_create method
> >
> > Fixed.
> >
> >> Code clean-up in aci.js:
> >> 2) IPA.rights_section can be deleted and replaced by spec object usage.
> >> It doesn't add any functionality.
> >
> > Fixed.
> >
> >> 3) IPA.permission_details_facet can be deleted - it isn't used anywhere.
> >
> > Fixed.
> >
> 
> ACK
> 

-- 
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] 6 Sort password policy by priority

2011-11-29 Thread Ondrej Hamada

On 11/29/2011 03:46 PM, Ondrej Hamada wrote:

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

'ipa pwpolicy-find' output is now sorted by priority of the policies.
Lower position means lower priority. Global policy is then at the bottom.

The changes has also affected LDAPSearch class in baseldap.py:
LDAPSearch class sorts the search results by primary key be default
(which is usually 'cn'). Therefor a function pointer entries_sortfn
was added. If no sorting function exists, default sorting by primary key
is used.

Sorting function had to be introduced due to the fact that pwpolicy's 
primary
key is also it's 'cn' and global policy is not allowed to have any 
priority.



forget to attach the patch, sorry :-[

--
Regards,

Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada

From 8078a025aaa68fb55482bfe7b3c410773d0583d4 Mon Sep 17 00:00:00 2001
From: Ondrej Hamada 
Date: Tue, 29 Nov 2011 15:18:48 +0100
Subject: [PATCH] Sort password policy by priority

'ipa pwpolicy-find' output is now sorted by priority of the policies.
Lower position means lower priority. Global policy is then at the bottom.

The changes has also affected LDAPSearch class in baseldap.py:
LDAPSearch class sorts the search results by primary key be default
(which is usually 'cn'). Therefor a function pointer entries_sortfn
was added. If no sorting function exists, default sorting by primary key
is used.

Sorting function had to be introduced due to the fact that pwpolicy's primary
key is also it's 'cn' and global policy is not allowed to have any priority.

https://fedorahosted.org/freeipa/ticket/2045
---
 ipalib/plugins/baseldap.py |   12 +---
 ipalib/plugins/pwpolicy.py |   13 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 4fd5fe4a1e7ff2d8fac7d3a65379b4ae0c5eb554..f35412f0abef6c1a6ca6881e0b475ee2e54e8d28 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -1434,6 +1434,9 @@ class LDAPSearch(CallbackInterface, crud.Search):
 member_param_incl_doc = _('Search for %s with these %s %s.')
 member_param_excl_doc = _('Search for %s without these %s %s.')
 
+# if True, self.execute function won't sort the entries by 'cn' value
+entries_sortfn = None
+
 takes_options = (
 Int('timelimit?',
 label=_('Time Limit'),
@@ -1612,9 +1615,12 @@ class LDAPSearch(CallbackInterface, crud.Search):
 else:
 callback(self, ldap, entries, truncated, *args, **options)
 
-if self.obj.primary_key:
-sortfn=lambda x,y: cmp(x[1][self.obj.primary_key.name][0].lower(), y[1][self.obj.primary_key.name][0].lower())
-entries.sort(sortfn)
+if not self.entries_sortfn:
+if self.obj.primary_key:
+self.entries_sortfn=lambda x,y: cmp(x[1][self.obj.primary_key.name][0].lower(), y[1][self.obj.primary_key.name][0].lower())
+entries.sort(self.entries_sortfn)
+else:
+entries.sort(self.entries_sortfn)
 
 if not options.get('raw', False):
 for e in entries:
diff --git a/ipalib/plugins/pwpolicy.py b/ipalib/plugins/pwpolicy.py
index db42bca0424da34bc17b7df376d529bd60f55751..0f9175eb9691470ba94f4fcbbc2e4b76e5c0a199 100644
--- a/ipalib/plugins/pwpolicy.py
+++ b/ipalib/plugins/pwpolicy.py
@@ -455,7 +455,20 @@ api.register(pwpolicy_show)
 class pwpolicy_find(LDAPSearch):
 __doc__ = _('Search for group password policies.')
 
+def sort_priority(self,x,y):
+# global policy will be always last in the output
+if x[1]['cn'][0] == global_policy_name:
+return 1
+elif y[1]['cn'][0] == global_policy_name:
+return -1
+else:
+# policies with higher priority will be at the beginning of the list
+return cmp(int(x[1]['cospriority'][0]), int(y[1]['cospriority'][0]))
+
+entries_sortfn = sort_priority
+
 def post_callback(self, ldap, entries, truncated, *args, **options):
+
 if options.get('pkey_only', False):
 return False
 for e in entries:
-- 
1.7.6.4

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

[Freeipa-devel] [PATCH] spec file fix

2011-11-29 Thread Simo Sorce
Pushed the following patch under one-liner.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
>From 4d73e9cea769680a7b47db52f7f45440d0e27367 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Wed, 23 Nov 2011 08:25:55 -0500
Subject: [PATCH] spec: We do not need krb5-server-ldap anymore

We now use our own ipa-kdb DAL driver
---
 freeipa.spec.in |1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index cfd87f954a96e541ca2c2979af34d9026a5a34a6..c14da8bb5895eba48bc612f2c9ac0f2b3ce6e467 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -98,7 +98,6 @@ Requires: krb5-server >= 1.9.1-15
 %else
 Requires: krb5-server
 %endif
-Requires: krb5-server-ldap
 Requires: krb5-pkinit-openssl
 Requires: cyrus-sasl-gssapi%{?_isa}
 Requires: ntp
-- 
1.7.7.1

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

[Freeipa-devel] [PATCH] 6 Sort password policy by priority

2011-11-29 Thread Ondrej Hamada

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

'ipa pwpolicy-find' output is now sorted by priority of the policies.
Lower position means lower priority. Global policy is then at the bottom.

The changes has also affected LDAPSearch class in baseldap.py:
LDAPSearch class sorts the search results by primary key be default
(which is usually 'cn'). Therefor a function pointer entries_sortfn
was added. If no sorting function exists, default sorting by primary key
is used.

Sorting function had to be introduced due to the fact that pwpolicy's 
primary

key is also it's 'cn' and global policy is not allowed to have any priority.

--
Regards,

Ondrej Hamada
FreeIPA team
jabber: oh...@jabbim.cz
IRC: ohamada

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


Re: [Freeipa-devel] [PATCH] #2122 Fix PAC re-signing

2011-11-29 Thread Simo Sorce
On Tue, 2011-11-29 at 10:13 +0100, Sumit Bose wrote:
> On Mon, Nov 28, 2011 at 07:43:57PM -0500, Simo Sorce wrote:
> > On Thu, 2011-11-24 at 13:54 +0100, Sumit Bose wrote:
> > > I think I found two issues which should be fixed by the following
> > > patch:
> > >  - krb5_pac_add_buffer() expects krb5_pac and not krb5_pac * as a
> > > second
> > >argument
> > 
> > good catch
> > 
> > >  - your patch copies all buffers, including the checksums, which you
> > >wanted to remove from the new pac
> > 
> > also good catch
> > 
> > > With this patch applied I do not see any errors in the krb5kdc.log and
> > > ssh from AD to IPA server works.
> > 
> > I still haven't solved my ssh issue from an AD client to IPA, but I get
> > a ticket on the client now, so it must be unrelated stuff.
> > 
> > I have prepared a patch which have a slightly different version of your
> > fixes.
> 
> The patch looks fine and works for me.
> 
> ACK

Thanks,
pushed to master.

Simo.

-- 
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] 908 make some fields required

2011-11-29 Thread Rob Crittenden

Endi Sukma Dewata wrote:

On 11/28/2011 12:09 PM, Rob Crittenden wrote:

Some attributes in the framework were not marked as required even though
they are in the schema. These are typically computed values and I think
the intention was to not prompt for them. This has the side-effect of
them showing as not required in the UI even though they are.

Since they all have default values set they won't be prompted for on the
CLI so there won't be any practical changes.


This patch fixes the problem with required attributes in DNS Zones and
cn, uidNumber, and gidNumber in Users. The UI now shows the required
indicators for these attributes. So this patch is ACKed.

Some problems mentioned in ticket #2015 are still present:

1. Removing the homeDirectory from a user fails because it's required by
posixAccount.


Ok, I can probably fix this one.


2. Removing the gidNumber from a group fails because it's required by
posixGroup.


Well, its hard to make gidNumber required since you can have non-posix 
groups. I think we'll have to just live with this one.




3. Removing config attributes listed in the ticket generates internal
error. I think at least the server should return a proper error message.
The required indicator can be hard-coded in the UI if necessary.



The config problems are a separate issue. To make them required would 
mean that the user would need to provide a value with a -mod request.


rob

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


Re: [Freeipa-devel] [PATCH] 8 Add DNS service records for Windows

2011-11-29 Thread Martin Kosek
On Mon, 2011-11-28 at 18:16 +0100, Sumit Bose wrote:
> On Mon, Nov 28, 2011 at 02:26:00PM +0200, Alexander Bokovoy wrote:
> > On Fri, 25 Nov 2011, Sumit Bose wrote:
> > > On Wed, Nov 23, 2011 at 05:33:42PM -0500, Rob Crittenden wrote:
> > > > Alexander Bokovoy wrote:
> > > > >Hi Sumit,
> > > > >
> > > > >On Fri, 14 Oct 2011, Sumit Bose wrote:
> > > > >>>It would make more clear what is the default and that it is really
> > > > >>>optional setting -- I'm thinking from the perspective of maintenance
> > > > >>>of the code in future.
> > > > >>
> > > > >>Thank you for your comments, new version attached.
> > > > >Finally got to test it. ACK.
> > > > >
> > > > 
> > > > pushed to master.
> > > 
> > > Sorry, I think you pushed the first version and not -3- which was ACKed
> > > by Alexander.
> > Hm. Sumit, could you please rebase -3- on top of current master 
> > HEAD+(1)? 
> > 
> > I tried that briefly myself and failed. 
> > 
> > (1) Right now I'm also getting make-lint failing due to more strict 
> > PyLint in F16/Rawhide and those seems to be corect and also affect 
> > adtrustinstance.
> > 
> > I'm sending the patch shortly, so please rebase on top of it.
> 
> ok, rebased version is attached. To push this upstream you still have to
> revert the wrong commit.
> 
> bye,
> Sumit

I reverted the wrong version of patch so that Sumit can provide correct
rebased version:

master: ac45a5eee8382ab62b7c8b7c78c2bca36e5bf8a6

Martin

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


Re: [Freeipa-devel] [PATCH 48/48] Ticket #1879 - IPAdmin undefined anonymous parameter lists

2011-11-29 Thread Martin Kosek
On Tue, 2011-10-04 at 14:26 +0200, Jan Cholasta wrote:
> On 26.9.2011 21:52, John Dennis wrote:
> > The IPAdmin class in ipaserver/ipaldap.py has methods with anonymous
> > undefined parameter lists.
> >
> > For example:
> >
> >  def getList(self,*args):
> >
> > In Python syntax this means you can call getList with any positional
> > parameter list you want.
> >
> > This is bad because:
> >
> > 1) It's not true, *args gets passed to an ldap function with a well
> > defined parameter list, so you really do have to call it with a
> > defined parameter list. *args will let you pass anything, but once it
> > gets passed to the ldap function it will blow up if the parameters do
> > not match (what parameters are those you're wondering? see item 2).
> >
> > 2) The programmer does not know what the valid parameters are unless
> > they are defined in the formal parameter list.
> >
> > 3) Without a formal parameter list automatic documentation generators
> > cannot produce API documentation (see item 2)
> >
> > 4) The Python interpreter cannot validate the parameters being passed
> > because there is no formal parameter list. Note, Python does not
> > validate the type of parameters, but it does validate the correct
> > number of postitional parameters are passed and only defined keyword
> > parameters are passed. Bypassing the language support facilities leads
> > to programming errors.
> >
> > 5) Without a formal parameter list program checkers such as pylint
> > cannot validate the program which leads to progamming errors.
> >
> > 6) Without a formal parameter list which includes default keyword
> > parameters it's not possible to use keyword arguments nor to know what
> > their default values are (see item 2). One is forced to pass a keyword
> > argument as a positional argument, plus you must then pass every
> > keyword argument between the end of the positional argument list and
> > keyword arg of interest even of the other keyword arguments are not of
> > interest. This also demands you know what the default value of the
> > intermediate keyword arguments are (see item 2) and hope they don't
> > change.
> >
> > Also the *args anonymous tuple get passed into the error handling code
> > so it can report what the called values were. But because the tuple is
> > anonymous the error handler cannot not describe what it was passed. In
> > addition the error handling code makes assumptions about the possible
> > contents of the anonymous tuple based on current practice instead of
> > actual defined values. Things like "if the number of items in the
> > tuple is 2 or less then the first tuple item must be a dn
> > (Distinguished Name)" or "if the number of items in the tuple is
> > greater than 2 then the 3rd item must be an ldap search filter". These
> > are constructs which are not robust and will fail at some point in the
> > future.
> >
> > This patch also fixes the use of IPAdmin.addEntry(). It was sometimes
> > being called with (dn, modlist), sometimes a Entry object, or
> > sometimes a Entity object. Now it's always called with either a Entry
> > or Entity object and IPAdmin.addEntry() validates the type of the
> > parameter passed.
> >
> > --
> > John Dennis
> >
> > Looking to carve out IT costs?
> > www.redhat.com/carveoutcosts/
> >
> 
> ACK.
> 
> Honza
> 

Pushed to master (additional information in a thread for Alexander's
patch 0037).

Martin

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


Re: [Freeipa-devel] [PATCH 49/49] ticket #1870 - subclass SimpleLDAPObject

2011-11-29 Thread Martin Kosek
On Thu, 2011-09-29 at 15:06 -0600, Rich Megginson wrote:
> On 09/28/2011 01:27 PM, John Dennis wrote: 
> > We use convenience types (classes) in IPA which make working with LDAP
> > easier and more robust. It would be really nice if the basic python-ldap
> > library understood our utility types and could accept them as parameters
> > to the basic ldap functions and/or the basic ldap functions returned our
> > utility types.
> > 
> > Normally such a requirement would trivially be handled in an object-
> > oriented language (which Python is) by subclassing to extend and modify
> > the functionality. For some reason we didn't do this with the python-ldap
> > classes.
> > 
> > python-ldap objects are primarily used in two different places in our
> > code, ipaserver.ipaldap.py for the IPAdmin class and in
> > ipaserver/plugins/ldap2.py for the ldap2 class's .conn member.
> > 
> > In IPAdmin we use a IPA utility class called Entry to make it easier to
> > use the results returned by LDAP. The IPAdmin class is derived from
> > python-ldap.SimpleLDAPObject. But for some reason when we added the
> > support for the use of the Entry class in SimpleLDAPObject we didn't
> > subclass SimpleLDAPObject and extend it for use with the Entry class as
> > would be the normal expected methodology in an object-oriented language,
> > rather we used an obscure feature of the Python language to override all
> > methods of the SimpleLDAPObject class by wrapping those class methods in
> > another function call. The reason why this isn't a good approach is:
> > 
> > * It violates object-oriented methodology.
> > 
> > * Other classes cannot be derived and inherit the customization (because
> > the method wrapping occurs in a class instance, not within the class
> > type).
> > 
> > * It's non-obvious and obscure
> > 
> > * It's inefficient.
> > 
> > Here is a summary of what the code was doing:
> > 
> > It iterated over every member of the SimpleLDAPObject class and if it was
> > callable it wrapped the method. The wrapper function tested the name of
> > the method being wrapped, if it was one of a handful of methods we wanted
> > to customize we modified a parameter and called the original method. If
> > the method wasn't of interest to use we still wrapped the method.
> > 
> > It was inefficient because every non-customized method (the majority)
> > executed a function call for the wrapper, the wrapper during run-time used
> > logic to determine if the method was being overridden and then called the
> > original method. So every call to ldap was doing extra function calls and
> > logic processing which for the majority of cases produced nothing useful
> > (and was non-obvious from brief code reading some methods were being
> > overridden).
> > 
> > Object-orientated languages have support built in for calling the right
> > method for a given class object that do not involve extra function call
> > overhead to realize customized class behaviour. Also when programmers look
> > for customized class behaviour they look for derived classes. They might
> > also want to utilize the customized class as the base class for their use.
> > 
> > Also the wrapper logic was fragile, it did things like: if the method name
> > begins with "add" I'll unconditionally modify the first and second
> > argument. It would be some much cleaner if the "add", "add_s", etc.
> > methods were overridden in a subclass where the logic could be seen and
> > where it would apply to only the explicit functions and parameters being
> > overridden.
> > 
> > Also we would really benefit if there were classes which could be used as
> > a base class which had specific ldap customization.
> > 
> > At the moment our ldap customization needs are:
> > 
> > 1) Support DN objects being passed to ldap operations
> > 
> > 2) Support Entry & Entity objects being passed into and returned from
> > ldap operations.
> > 
> > We want to subclass the ldap SimpleLDAPObject class, that is the base
> > ldap class with all the ldap methods we're using. IPASimpleLDAPObject
> > class would subclass SimpleLDAPObject class which knows about DN
> > objects (and possilby other IPA specific types that are universally
> > used in IPA). Then  IPAEntrySimpleLDAPObject would subclass
> > IPASimpleLDAPObject which knows about Entry objects.
> > 
> > The reason for the suggested class hierarchy is because DN objects will be
> > used whenever we talk to LDAP (in the future we may want to add other IPA
> > specific classes which will always be used). We don't add Entry support to
> > the the IPASimpleLDAPObject class because Entry objects are (currently)
> > only used in IPAdmin.
> > 
> > What this patch does is:
> > 
> > * Introduce IPASimpleLDAPObject derived from
> >   SimpleLDAPObject. IPASimpleLDAPObject is DN object aware.
> > 
> > * Introduce IPAEntryLDAPObject derived from
> >   IPASimpleLDAPObject. IPAEntryLDAPObject is Entry object aware.
> > 
> > * Derive IPAdmin from IPAEntryLDAPObject and rem

Re: [Freeipa-devel] [PATCH] 0037 make-lint fails on Fedora 16/Rawhide

2011-11-29 Thread Martin Kosek
On Tue, 2011-11-29 at 13:54 +0200, Alexander Bokovoy wrote:
> On Tue, 29 Nov 2011, Alexander Bokovoy wrote:
> > On Tue, 29 Nov 2011, Martin Kosek wrote:
> > > > Conservative patch is attached.
> > > > 
> > > 
> > > What about other add_s(entry) calls? I see we call it this way on more
> > > places, especially in replication.py:
> > > 
> > > $ git grep "add_s(e"
> > > ipaserver/install/cainstance.py:ld.add_s(entry_dn, entry)
> > > ipaserver/install/krbinstance.py:self.admin_conn.add_s(entry) 
> > > #pylint: disable=E1120
> > > ipaserver/install/krbinstance.py:self.admin_conn.add_s(entry) 
> > > #pylint: disable=E1120
> > > ipaserver/install/replication.py:conn.add_s(ent)
> > > ipaserver/install/replication.py:conn.add_s(entry)
> > > ipaserver/install/replication.py:conn.add_s(entry)
> > > ipaserver/install/replication.py:self.conn.add_s(entry)
> > > ipaserver/install/replication.py:conn.add_s(entry)
> > > ipaserver/install/replication.py:a_conn.add_s(entry)
> > > ipaserver/install/replication.py:self.conn.add_s(entry)
> > > ipaserver/install/service.py:conn.add_s(entry) #pylint: 
> > > disable=E1120
> > > 
> > > Should we patch ipa-2-1 branch as well? If we do another release for
> > > F-16 we want to have pylint check clean. We would need a rebased patch
> > > for ipa-2-1 branch in this case.
> > I think this patch also can go away, we found out with Rob yesterday 
> > night that John has addressed these issues in his patches 48 and 49 in 
> > September. The patches were ACKed but not merged. I'm doing their 
> > testing now and will push them after that.
> > 
> > Look at John's 48/49 patches in meanwhile.
> Ok, I've tested patches 48 and 49 and also made versions for ipa-2-1 
> branch. All attached.
> 

Both pushed to master.

As discussed on IRC, we are not sure that these pylint errors would
manifest in ipa-2-1 branch, so for the sake of stability I decided to
push them to master branch only.

Martin

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


Re: [Freeipa-devel] [PATCH] 0037 make-lint fails on Fedora 16/Rawhide

2011-11-29 Thread Alexander Bokovoy
On Tue, 29 Nov 2011, Martin Kosek wrote:
> > Conservative patch is attached.
> > 
> 
> What about other add_s(entry) calls? I see we call it this way on more
> places, especially in replication.py:
> 
> $ git grep "add_s(e"
> ipaserver/install/cainstance.py:ld.add_s(entry_dn, entry)
> ipaserver/install/krbinstance.py:self.admin_conn.add_s(entry) 
> #pylint: disable=E1120
> ipaserver/install/krbinstance.py:self.admin_conn.add_s(entry) 
> #pylint: disable=E1120
> ipaserver/install/replication.py:conn.add_s(ent)
> ipaserver/install/replication.py:conn.add_s(entry)
> ipaserver/install/replication.py:conn.add_s(entry)
> ipaserver/install/replication.py:self.conn.add_s(entry)
> ipaserver/install/replication.py:conn.add_s(entry)
> ipaserver/install/replication.py:a_conn.add_s(entry)
> ipaserver/install/replication.py:self.conn.add_s(entry)
> ipaserver/install/service.py:conn.add_s(entry) #pylint: 
> disable=E1120
> 
> Should we patch ipa-2-1 branch as well? If we do another release for
> F-16 we want to have pylint check clean. We would need a rebased patch
> for ipa-2-1 branch in this case.
I think this patch also can go away, we found out with Rob yesterday 
night that John has addressed these issues in his patches 48 and 49 in 
September. The patches were ACKed but not merged. I'm doing their 
testing now and will push them after that.

Look at John's 48/49 patches in meanwhile.

-- 
/ Alexander Bokovoy

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


Re: [Freeipa-devel] [PATCH] 0037 make-lint fails on Fedora 16/Rawhide

2011-11-29 Thread Martin Kosek
On Mon, 2011-11-28 at 17:26 +0200, Alexander Bokovoy wrote:
> On Mon, 28 Nov 2011, Alexander Bokovoy wrote:
> 
> > Hi,
> > 
> > Attached are fixes for ldap.LDAPObject.add_s(self, dn, modlist) uses
> > which now don't pass 'make-lint' on Fedora 16/Rawhide.
> > 
> > -- 
> > / Alexander Bokovoy
> 
> > >From dd866262c98be779a094a617975145e2fb1e0dd1 Mon Sep 17 00:00:00 2001
> > From: Alexander Bokovoy 
> > Date: Mon, 28 Nov 2011 14:21:17 +0200
> > Subject: [PATCH] Be more explicit when passing Entry class to
> >  ldap.LDAPObject.add_s()
> > 
> > ldap.LDAPObject.add_s(self, dn, modlist) requires two positional arguments.
> > We used to pass our Entry class which implements dictionary access that 
> > gives
> > proper way to pass the positional arguments, but PyLint in Fedora 16/Rawhide
> > became more strict about that and can't infer dictionary interface through
> > static checking.
> > 
> > Thus, we need to explicitly annotate dictionary passing with **entry syntax.
> > This has additional benefit to remind that we deal with multiple arguments 
> > here.
> Self-NACK, doesn't really work this way. I'm still looking at better 
> approach but intermediate solution is to use pylint hints.
> 
> Conservative patch is attached.
> 

What about other add_s(entry) calls? I see we call it this way on more
places, especially in replication.py:

$ git grep "add_s(e"
ipaserver/install/cainstance.py:ld.add_s(entry_dn, entry)
ipaserver/install/krbinstance.py:self.admin_conn.add_s(entry) 
#pylint: disable=E1120
ipaserver/install/krbinstance.py:self.admin_conn.add_s(entry) 
#pylint: disable=E1120
ipaserver/install/replication.py:conn.add_s(ent)
ipaserver/install/replication.py:conn.add_s(entry)
ipaserver/install/replication.py:conn.add_s(entry)
ipaserver/install/replication.py:self.conn.add_s(entry)
ipaserver/install/replication.py:conn.add_s(entry)
ipaserver/install/replication.py:a_conn.add_s(entry)
ipaserver/install/replication.py:self.conn.add_s(entry)
ipaserver/install/service.py:conn.add_s(entry) #pylint: 
disable=E1120

Should we patch ipa-2-1 branch as well? If we do another release for
F-16 we want to have pylint check clean. We would need a rebased patch
for ipa-2-1 branch in this case.

Martin

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


Re: [Freeipa-devel] [PATCH] 5 User-add random password support

2011-11-29 Thread Martin Kosek
On Thu, 2011-11-24 at 17:51 +0100, Ondrej Hamada wrote:
> On 11/24/2011 03:54 PM, Ondrej Hamada wrote: 
> > https://fedorahosted.org/freeipa/ticket/1979 
> > 
> > I've used code from ipalib/plugins/host.py to add support for
> > random 
> > password generation. The '--random' option is now available in 
> > user-add and user-mod commands. If both the 'password' and 'random' 
> > options are used the 'random' option will be ignored. 
> > 
> > 

Functionally, it works OK. I would just like to propose few
improvements:

1) Minor API version in VERSION file should be bumped since you add a
new option
2) We should add some tests exercising this new functionality so that we
can detect regressions early
3) (optional) I am thinking if the passwords we generate are not very
user friendly. I would love to see user's face when he is told that his
new password is 5QU;8l2%]y"? .

While this is may be OK for hosts bulk passwords which are only
manipulated by admins, we may want to develop more user friendly
passwords in the user plugin.

Martin

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


Re: [Freeipa-devel] [PATCH] #2122 Fix PAC re-signing

2011-11-29 Thread Sumit Bose
On Mon, Nov 28, 2011 at 07:43:57PM -0500, Simo Sorce wrote:
> On Thu, 2011-11-24 at 13:54 +0100, Sumit Bose wrote:
> > I think I found two issues which should be fixed by the following
> > patch:
> >  - krb5_pac_add_buffer() expects krb5_pac and not krb5_pac * as a
> > second
> >argument
> 
> good catch
> 
> >  - your patch copies all buffers, including the checksums, which you
> >wanted to remove from the new pac
> 
> also good catch
> 
> > With this patch applied I do not see any errors in the krb5kdc.log and
> > ssh from AD to IPA server works.
> 
> I still haven't solved my ssh issue from an AD client to IPA, but I get
> a ticket on the client now, so it must be unrelated stuff.
> 
> I have prepared a patch which have a slightly different version of your
> fixes.

The patch looks fine and works for me.

ACK

bye,
Sumit

> 
> Simo.
> 
> -- 
> 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] 157 Add --delattr option to complement --setattr/--addattr

2011-11-29 Thread Martin Kosek
On Mon, 2011-11-28 at 23:24 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > On Sat, 2011-11-05 at 13:43 +0100, Martin Kosek wrote:
> >> On Fri, 2011-11-04 at 09:23 -0400, Rob Crittenden wrote:
> >>> Martin Kosek wrote:
>  Add a --delattr option to round out multi-valued attribute manipulation.
>  The new option is be available for all LDAPUpdate based commands.
> 
>  --delattr is evaluated last, it can remove any value present either
>  in --addattr/--setattr options or stored in LDAP.
> 
>  https://fedorahosted.org/freeipa/ticket/1929
> >>>
> >>> Should --delattr raise an error if the value doesn't exist?
> >>>
> >>> I think it probably should.
> >>>
> >>> rob
> >>
> >> You are right, it would be more expected behavior. I fixed that. In the
> >> process of implementing the change I found that current --*attr
> >> processing is not good, so I refactored it completely to one function
> >> available for all baseldap.py commands.
> >>
> >> In the process I found out that we don't have any common class for all
> >> baseldap.py commands and the result is BaseLDAPCommand which can now
> >> handle attribute processing and provide it to other LDAP commands.
> >>
> >> And I also fixed one group unit test. Now all unit tests were OK.
> >>
> >> Martin
> >
> > I rebased the patch (API.txt format was changed) and tested that it
> > still works ok.
> >
> > Martin
> 
> ACK but some of the comments need to be cleaned up before pushing. It 
> will also require a minor rebase in frontend.py.
> 
> process_attr_options() should probably read:
> 
> Process all --setattr, --addattr, and --delattr options and add the 
> resulting value to the list of attributes. --setattr is processed first,
> then --addattr and finally --delattr.
> 
> When --setattr is not used then the original LDAP object is looked up 
> (of course, not when dn is None) and the changes are applied to old 
> object values.
> 
> ...
> AttrValueNotFound exception may be raised when an attribute value was 
> not found either by --setattr and --addattr nor in existing LDAP object.
> 
> rob

I fixed the comment, it is now much more readable. I see you improved
__convert_2_dict function (which is moved to baseldap.py in this patch)
in your update plugin framework patch. This has been properly merged.

Pushed to master.

Martin

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


Re: [Freeipa-devel] [PATCH] 161 Make ipa-server-install clean after itself

2011-11-29 Thread Martin Kosek
On Mon, 2011-11-28 at 23:34 -0500, Rob Crittenden wrote:
> Martin Kosek wrote:
> > How to test:
> >
> > 1) ipa-server-install -p secret123 -a secret123 --hostname
> > ipa.example.com
> > 2) Continue in interactive wizard until IP address is requested (as
> > ipa.example.com cannot be resolved)
> > 3) When it is entered and ipa-server-install gives this output:
> >
> > # ipa-server-install -p kokos123 -a kokos123 --hostname ipa.example.com
> > --setup-dns
> > ...
> > Please confirm the domain name [example.com]:
> >
> > Unable to resolve IP address for host name
> > Please provide the IP address to be used for this host name: 10.16.78.93
> > Adding [10.16.78.93 ipa.example.com] to your /etc/hosts file<
> > ...
> >
> > hit CTRL+C to quit from the installation.
> > 4) Try running ipa-server-install again. It will fail as /etc/hosts has
> > been changed - this patch fixes it.
> 
> ACK. Needs a rebase.
> 
> rob

Rebased for master branch. Pushed to master, ipa-2-1.

Martin

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


Re: [Freeipa-devel] [PATCH] 152 Enable automember for upgraded servers

2011-11-29 Thread Martin Kosek
On Mon, 2011-11-28 at 18:16 -0500, Rob Crittenden wrote:
> Nathan Kinder wrote:
> > On 11/04/2011 02:35 PM, Nathan Kinder wrote:
> >> On 11/04/2011 02:26 PM, Martin Kosek wrote:
> >>> On Fri, 2011-11-04 at 14:04 -0700, Nathan Kinder wrote:
>  On 11/04/2011 02:02 PM, Rob Crittenden wrote:
> > Martin Kosek wrote:
> >> automember functionality is depends on predefined data is in LDAP.
> >> Since we add it for fresh installs only, automember cannot be used
> >> for upgraded servers. Make sure that automember LDAP data is added
> >> during upgrade too.
> >>
> >> https://fedorahosted.org/freeipa/ticket/1992
> > I think you need that automember schema as well. Can you check with
> > the 389-ds team to see if their upgrade script automatically adds new
> > schema or if we have to handle that ourselves?
>  The new automember schema should be added by 'setup-ds.pl -u', so I
>  don't expect you need to do anything around schema in FreeIPA.
> >>> Nathan, when is the "setup-ds.pl -u" executed? When the dirsrv rpm is
> >>> updated, just like FreeIPA runs ipa-ldap-updater in rpm update %post? Or
> >>> does it have to be run manually?
> >> It is run in the the %posttrans stage for 389-ds-base.
> >>> I am asking because the schema problem seems like the root cause that
> >>> one user has here (the last post):
> >>>
> >>> https://bugzilla.redhat.com/show_bug.cgi?id=746589
> >> There should be a
> >> '/etc/dirsrv/slapd-/schema/10automember-plugin.ldif' file if
> >> the proper version
> >> of 389-ds-base is being used and if 'setup-ds.pl -u' successfully
> >> updated the schema. There should also be
> >> a '/etc/dirsrv/schema/10automember-plugin.ldif' file present
> >> regardless of 'setup-ds.pl -u' having run
> >> successfully.
> > I just tested running 'setup-ds.pl -u' manually with a master build of
> > 389-ds-base, and there is a bug that is preventing the updates from
> > being applied. I logged the following bug for this:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=751495
> >
> > The fix is a one-liner, and I believe Rich is working on getting a fixed
> > build out ASAP.
> 
> ACK, works for me.
> 
> rob

Pushed to master, ipa-2-1.

Martin

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