Re: [Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

[Milan Kubik]

Hello,

sorry for delays. The patch no longer applies to master. Rebase it, please.

Milan

- Original Message -
From: "Filip Škola" 
To: "Milan Kubík" 
Cc: freeipa-devel@redhat.com
Sent: Wednesday, 9 December, 2015 7:01:02 PM
Subject: Re: [Freeipa-devel] [PATCH 0002] Refactor test_group_plugin

On Mon, 7 Dec 2015 17:49:18 +0100
Milan Kubík  wrote:

> On 12/03/2015 08:15 PM, Filip Škola wrote:
> > On Mon, 30 Nov 2015 17:18:30 +0100
> > Milan Kubík  wrote:
> >
> >> On 11/23/2015 04:42 PM, Filip Škola wrote:
> >>> Sending updated patch.
> >>>
> >>> F.
> >>>
> >>> On Mon, 23 Nov 2015 14:59:34 +0100
> >>> Filip Škola  wrote:
> >>>
>  Found couple of issues (broke some dependencies).
> 
>  NACK
> 
>  F.
> 
>  On Fri, 20 Nov 2015 13:56:36 +0100
>  Filip Škola  wrote:
> 
> > Another one.
> >
> > F.
> >>>
> >> Hi, the tests look good. Few remarks, though.
> >>
> >> 1. Please, use the shortes copyright notice in new modules.
> >>
> >>   #
> >>   # Copyright (C) 2015  FreeIPA Contributors see COPYING for
> >> license #
> >>
> >> 2. The tests `test_group_remove_group_from_protected_group` and
> >> `test_group_full_set_of_objectclass_not_available_post_detach`
> >> were not ported. Please, include them in the patch.
> >>
> >> Also, for less hassle, please rebase your patches on top of
> >> freeipa-mkubik-0025-3-Separated-Tracker-implementations-into-standalone-pa.patch
> >> Which changes the location of tracker implementations and prevents
> >> circular imports.
> >>
> >> Thanks.
> >>
> >
> >
> > Hi,
> >
> > these cases are there, in corresponding classes. They are marked
> > with the original comments. (However I can move them to separate
> > class if desirable.)
> >
> > The copyright notice is changed. Also included a few changes in the
> > test with user without private group.
> >
> > Filip
> NACK
> 
> linter:
> * Module tracker.group_plugin
> ipatests/test_xmlrpc/tracker/group_plugin.py:257: 
> [E0102(function-redefined), GroupTracker.check_remove_member] method 
> already defined line 253)
> 
> Probably a leftover after the rebase made on top of my patch. Please
> fix it. You can check youch changes by make-lint script before
> sending them.
> 
> Thanks
> 


Hi,

I learned to use make-lint!

Thanks,
F.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0005] Refactor test_nesting, create HostGroupTracker

[Milan Kubik]

Hi,

the patch no longer applies to master. Please rebase it.

Thanks,
Milan

- Original Message -
From: "Filip Skola" 
To: freeipa-devel@redhat.com
Cc: "Milan Kubík" , "Aleš Mareček" 
Sent: Tuesday, 22 December, 2015 11:56:15 AM
Subject: [PATCH 0005] Refactor test_nesting, create HostGroupTracker

Hi,

another patch from refactoring-test_xmlrpc series.

Filip

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [patch 0004] spec file: Update the package name from libipa_hbac-python to python-libipa_hbac

[Milan Kubik]

Name update + the renamed package breaks 'dnf builddep'. I will report 
the bug.

Yum can take care of the conflict resolution.

Patch attached.

Milan
From 3d79c32ffad3ab280b7d84507d402039b70fa8e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= mku...@redhat.com
Date: Fri, 10 Jul 2015 11:59:24 +0200
Subject: [PATCH] spec file: update the package name from libipa_hbac-python to
 python-libipa_hbac

---
 freeipa.spec.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index e78ad1a0851186c7fdb5ab0a4649b64b2b1e010f..5310fc643b209c9ea895184f96836b1d958a6a01 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -75,7 +75,7 @@ BuildRequires:  python-rhsm
 BuildRequires:  pyOpenSSL
 BuildRequires:  pylint = 1.0
 BuildRequires:  python-polib
-BuildRequires:  libipa_hbac-python
+BuildRequires:  python-libipa_hbac
 BuildRequires:  python-memcached
 BuildRequires:  sssd = 1.13.0
 BuildRequires:  python-lxml
@@ -296,7 +296,7 @@ Requires: python-nss = 0.16
 Requires: python-cryptography
 Requires: python-lxml
 Requires: python-netaddr
-Requires: libipa_hbac-python
+Requires: python-libipa_hbac
 Requires: python-qrcode-core = 5.0.0
 Requires: python-pyasn1
 Requires: python-dateutil
-- 
1.9.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [patch 0004] spec file: Update the package name from libipa_hbac-python to python-libipa_hbac

[Milan Kubik]

On 07/10/2015 12:55 PM, Jan Cholasta wrote:

Hi,



Dne 10.7.2015 v 12:05 Milan Kubik napsal(a):


Name update + the renamed package breaks 'dnf builddep'. I will report

the bug.

Yum can take care of the conflict resolution.



Patch attached.




You might as well update libsss_nss_idmap-python to
python-libsss_nss_idmap while you are at it.



Honza







Hi, new patch is here :)

Self-NACK on  0004.
From 3067b69c1b5b11ba7ee6ae34d8efcf97219e1d7a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= mku...@redhat.com
Date: Fri, 10 Jul 2015 11:59:24 +0200
Subject: [PATCH] spec file: update the python package names for libipa_hbac
 and libsss_nss_idmap

---
 freeipa.spec.in | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index e78ad1a0851186c7fdb5ab0a4649b64b2b1e010f..e9f97c3d68898c63a299408b93a6330e65f35d0e 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -75,7 +75,7 @@ BuildRequires:  python-rhsm
 BuildRequires:  pyOpenSSL
 BuildRequires:  pylint = 1.0
 BuildRequires:  python-polib
-BuildRequires:  libipa_hbac-python
+BuildRequires:  python-libipa_hbac
 BuildRequires:  python-memcached
 BuildRequires:  sssd = 1.13.0
 BuildRequires:  python-lxml
@@ -204,7 +204,7 @@ Requires: samba-python
 Requires: samba = %{samba_version}
 Requires: samba-winbind
 Requires: libsss_idmap
-Requires: libsss_nss_idmap-python
+Requires: python-libsss_nss_idmap
 Requires: oddjob
 Requires: python-sss
 # We use alternatives to divert winbind_krb5_locator.so plugin to libkrb5
@@ -296,7 +296,7 @@ Requires: python-nss = 0.16
 Requires: python-cryptography
 Requires: python-lxml
 Requires: python-netaddr
-Requires: libipa_hbac-python
+Requires: python-libipa_hbac
 Requires: python-qrcode-core = 5.0.0
 Requires: python-pyasn1
 Requires: python-dateutil
-- 
1.9.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [patch 0006] ipalib: pass api instance into textui in doctest snippets

[Milan Kubik]

Hi,

the recent set of patches that modified api broke the tests that are 
included in ipalib/cli.py


This patch fixes the problems by passing api instance to textui() calls.

Milan
From 5df216ad49c6787a6e170a483c545d0fdcc99828 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= mku...@redhat.com
Date: Fri, 10 Jul 2015 11:56:02 +0200
Subject: [PATCH] ipalib: pass api instance into textui in doctest snippets

---
 ipalib/cli.py | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/ipalib/cli.py b/ipalib/cli.py
index b260ca65172dab7ba56a23b78c086f49f5c18f70..4104e6482e4e713d701c6c1a4313ab6ecc899057 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -50,6 +50,7 @@ from errors import (PublicError, CommandError, HelpError, InternalError,
 from constants import CLI_TAB, LDAP_GENERALIZED_TIME_FORMAT
 from parameters import File, Str, Enum, Any, Flag
 from text import _
+from ipalib import api
 from ipapython.version import API_VERSION
 from ipapython.dnsutil import DNSName
 
@@ -100,7 +101,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  rows = [
 ... ('a', 'package'),
 ... ('an', 'egg'),
@@ -178,7 +179,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  ui.print_line('This line can fit!', width=18)
 This line can fit!
  ui.print_line('This line wont quite fit!', width=18)
@@ -204,7 +205,7 @@ class textui(backend.Backend):
 ... Python is a dynamic object-oriented programming language that can
 ... be used for many kinds of software development.
 ... '''
- ui = textui()
+ ui = textui(api)
  ui.print_paragraph(text, width=45)
 Python is a dynamic object-oriented
 programming language that can be used for
@@ -229,7 +230,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  ui.print_indented('One indentation level.')
   One indentation level.
  ui.print_indented('Two indentation levels.', indent=2)
@@ -249,7 +250,7 @@ class textui(backend.Backend):
 ... ('in_server', True),
 ... ('mode', u'production'),
 ... ]
- ui = textui()
+ ui = textui(api)
  ui.print_keyval(items)
   in_server = True
   mode = u'production'
@@ -269,7 +270,7 @@ class textui(backend.Backend):
 For example:
 
  attr = 'dn'
- ui = textui()
+ ui = textui(api)
  ui.print_attribute(attr, u'dc=example,dc=com')
   dn: dc=example,dc=com
  attr = 'objectClass'
@@ -407,7 +408,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  ui.print_dashed('Dashed above and below.')
 ---
 Dashed above and below.
@@ -434,7 +435,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  ui.print_h1('A primary header')
 
 A primary header
@@ -448,7 +449,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  ui.print_h2('A secondary header')
   --
   A secondary header
@@ -464,7 +465,7 @@ class textui(backend.Backend):
 command.  For example, a hypothetical ``show_status`` command would
 output something like this:
 
- ui = textui()
+ ui = textui(api)
  ui.print_name('show_status')
 
 show-status:
@@ -481,7 +482,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  ui.print_summary('Added user jdoe')
 -
 Added user jdoe
@@ -500,7 +501,7 @@ class textui(backend.Backend):
 
 For example:
 
- ui = textui()
+ ui = textui(api)
  ui.print_count(1, '%d goose', '%d geese')
 ---
 1 goose
-- 
1.9.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [patch 0006] ipalib: pass api instance into textui in doctest snippets

[Milan Kubik]

On 07/10/2015 01:57 PM, Milan Kubik wrote:

Hi,

the recent set of patches that modified api broke the tests that are 
included in ipalib/cli.py


This patch fixes the problems by passing api instance to textui() calls.

Milan


This may not be the complete solution. Similar problems arise in the 
rest of the tests in ipalib modules.
I guess the code examples (doctest test cases) are all affected by the 
changes to the api object.
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] certprofile test plan update

[Milan Kubik]

Hello,

here is an update to certificate profile test plan based on
the latest version of the design document [1,2].

The test itself is merely CRUD test at the moment.
To write a functional test for the feature, I will need to write a test
for CA ACLs (to implement the caacl tracker class). Together the
trackers for these two (once Sub CAs are implemented, three)
classes [3] can be used to extend cert-request tests as to check
whether the ACLs and profiles are being enforced.

Fraser,
the show command in the design has an 'output' option to retrieve
the profile itself. Will this be implemented or did you just forget to
remove it from the design page?


[1]: http://www.freeipa.org/page/V4/Certificate_Profiles/Test_Plan
[2]: http://www.freeipa.org/page/V4/Certificate_Profiles
[3]: 
https://git.fedorahosted.org/cgit/freeipa.git/commit/?id=d25a45a9f99aa5d841f47baa0332f49223ecffca


--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 0003] Fix for a typo in certprofile mod command.

[Milan Kubik]

Patch attached.

Milan
From e8f08c2f5a567701e7991b1db8e796a0b5db0512 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= mku...@redhat.com
Date: Fri, 19 Jun 2015 11:57:21 +0200
Subject: [PATCH] Fix for a typo in certprofile mod command.

---
 ipalib/plugins/certprofile.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipalib/plugins/certprofile.py b/ipalib/plugins/certprofile.py
index 1a2d143882469858f225b37ba4ff2dd368fb8853..5158bf0bb994451eecb55451b944687e74c95ee8 100644
--- a/ipalib/plugins/certprofile.py
+++ b/ipalib/plugins/certprofile.py
@@ -246,7 +246,7 @@ class certprofile_del(LDAPDelete):
 @register()
 class certprofile_mod(LDAPUpdate):
 __doc__ = _(Modify Certificate Profile configuration.)
-msg_summary = _('Modified Certificate Profile %(value)s')
+msg_summary = _('Modified Certificate Profile %(value)s')
 
 def execute(self, *args, **kwargs):
 ca_enabled_check()
-- 
1.9.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0040] generalize certificate creation during testing

[Milan Kubik]

On 06/09/2015 01:14 PM, Martin Babinsky wrote:
A slight hack to ipatests/test_xmlrpc/testcert.py module in order to 
enable generation of multiple host/service/user certificates.


It should make writing tests for new CA profile/sub-CA/user 
certificate functionality easier.



Hi,

looks good to me, ACK.

Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0014 v3] Support multiple user and host certificates

[Milan Kubik]

On 06/03/2015 01:17 PM, Martin Basti wrote:

On 02/06/15 16:03, Jan Cholasta wrote:

Dne 2.6.2015 v 12:36 Martin Basti napsal(a):

On 02/06/15 11:42, Fraser Tweedale wrote:

On Mon, Jun 01, 2015 at 02:50:45PM +0200, Martin Basti wrote:

On 01/06/15 06:40, Fraser Tweedale wrote:

New version of patch; ``{host,service}-show --out=FILE`` now writes
all certs to FILE.  Rebased on latest master.

Thanks,
Fraser

On Thu, May 28, 2015 at 09:18:04PM +1000, Fraser Tweedale wrote:

Updated patch attached.  Notably restores/adds revocation behaviour
to host-mod and service-mod.

Thanks,
Fraser

On Wed, May 27, 2015 at 06:12:50PM +0200, Martin Basti wrote:

On 27/05/15 15:53, Fraser Tweedale wrote:
This patch adds supports for multiple user / host 
certificates.  No

schema change is needed ('usercertificate' attribute is already
multi-value).  The revoke-previous-cert behaviour of host-mod and
user-mod has been removed but revocation behaviour of -del and
-disable is preserved.

The latest profiles/caacl patchset (0001..0013 v5) depends on 
this

patch for correct cert-request behaviour.

There is one design question (or maybe more, let me know): the
`--out=FILENAME' option to {host,service} show saves ONE 
certificate

to the named file.  I propose to either:

a) write all certs, suffixing suggested filename with either a
sequential numerical index, e.g. cert.pem becomes
cert.pem.1, cert.pem.2, and so on; or

b) as above, but suffix with serial number and, if there are
different issues, some issuer-identifying information.

Let me know your thoughts.

Thanks,
Fraser



Is there a possible way how to store certificates into one file?
I read about possibilities to have multiple certs in one .pem
file, but I'm
not cert guru :)

I personally vote for serial number in case there are multiple
certificates,
if ^ is no possible.


1)
+if len(certs)  0:

please use only,
if certs:

2)
You need to re-generate API/ACI.txt in this patch

3)
syntax error:
+for dercert in certs_der


4)
command
ipa user-mod ca_user --certificate=ceritifcate

removes the current certificate from the LDAP, by design.
Should be the old certificate(s) revoked? You removed that part in
the code.

only the --addattr='usercertificate=cert' appends new value 
there


--
Martin Basti


My objections/proposed solutions in attached patch.

* VERSION
* In the previous version normalized values was stored in LDAP, so I
added
it back.  (I dont know why there is no normalization in param
settings, but
normalization for every certificate is done in callback. I will 
file a

ticket for this)
* IMO only normalized certificates should be compared in the old
certificates detection


I incorporated your suggested changes in new patch (attached).

There were no proposed changes to the other patchset (0001..0013)
since rebase.

Thanks,
Fraser

Thank you,
ACK
Martin^2



Pushed to master: 7f7c247bb5a4b0030d531f4f14c156162e808212


Regression found.

Patch to fix the issue is attached.


The fix works, thanks.

Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [patch 0002] Abstract the HostTracker class from host plugin test

[Milan Kubik]

Hello,

this is the (first) patch with the Tracker class implementation based on 
host plugin test.


It is meant to be used as a base class to implement a helper class to 
write xml-rpc (api)
tests for LDAP based plugins and to replace the Declarative class which 
is used for

most of the xml-rpc tests at the moment.

For an example usage take a look at the host plugin test.

Cheers,
Milan
From a0cba2ffaafb7cc44ccfef4f00f047df1f91cf37 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Milan=20Kub=C3=ADk?= mku...@redhat.com
Date: Mon, 25 May 2015 16:05:46 +0200
Subject: [PATCH] Abstract the HostTracker class from host plugin test

Implements a base class to help test LDAP based plugins.

The class has been decoupled from the original host plugin test
and moved to separate module ipatests.test_xmlrpc.ldaptracker.

https://fedorahosted.org/freeipa/ticket/5032
---
 ipatests/test_xmlrpc/ldaptracker.py  | 287 +++
 ipatests/test_xmlrpc/test_host_plugin.py | 155 +
 2 files changed, 292 insertions(+), 150 deletions(-)
 create mode 100644 ipatests/test_xmlrpc/ldaptracker.py

diff --git a/ipatests/test_xmlrpc/ldaptracker.py b/ipatests/test_xmlrpc/ldaptracker.py
new file mode 100644
index ..97412440446a0cc56e283e58fb731a31c4d9458f
--- /dev/null
+++ b/ipatests/test_xmlrpc/ldaptracker.py
@@ -0,0 +1,287 @@
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+
+Implements a base class to track changes to an LDAP object.
+
+
+import functools
+
+from ipalib import api, errors
+from ipapython.dn import DN
+from ipapython.version import API_VERSION
+
+
+class Tracker(object):
+Wraps and tracks modifications to a plugin LDAP entry object
+
+Stores a copy of state of a plugin entry object and allows checking that
+the state in the database is the same as expected.
+This allows creating independent tests: the individual tests check
+that the relevant changes have been made. At the same time
+the entry doesn't need to be recreated and cleaned up for each test.
+
+Two attributes are used for tracking: ``exists`` (true if the entry is
+supposed to exist) and ``attrs`` (a dict of LDAP attributes that are
+expected to be returned from IPA commands).
+
+For commonly used operations, there is a helper method, e.g.
+``create``, ``update``, or ``find``, that does these steps:
+
+* ensure the entry exists (or does not exist, for create)
+* store the expected modifications
+* get the IPA command to run, and run it
+* check that the result matches the expected state
+
+Tests that require customization of these steps are expected to do them
+manually, using lower-level methods.
+Especially the first step (ensure the entry exists) is important for
+achieving independent tests.
+
+The Tracker object also stores information about the entry, e.g.
+``dn``, ``rdn`` and ``name`` which is derived from DN property.
+
+To use this class, the programer must subclass it and provide the
+implementation of following methods:
+
+ * make_*_command   -- implementing the API call for particular plugin
+   and operation (add, delete, ...)
+   These methods should use the make_command method
+ * check_* commands -- an assertion for a plugin command (CRUD)
+ * track_create -- to make an internal representation of the
+   entry
+
+Apart from overriding these methods, the subclass must provide the
+distinguished name of the entry in `self.dn` property.
+
+It is also required to override the class variables defining the sets
+of ldap attributes/keys for these operations specific to the plugin
+being implemented. Take the host plugin test for an example.
+
+The implementation of these methods is not strictly enforced.
+A missing method will cause a NotImplementedError during runtime
+as a result.
+
+retrieve_keys = None
+retrieve_all_keys = None
+create_keys = None
+update_keys = None
+managedby_keys = None
+allowedto_keys = None
+
+_override_me_msg = This method needs to be overriden in a subclass
+
+def __init__(self, default_version=None):
+self.api = api
+self.default_version = default_version or API_VERSION
+self._dn = None
+
+self.exists = False
+
+@property
+def dn(self):
+A property containing the distinguished name of the entry.
+if not self._dn:
+raise ValueError('The DN must be set in the init method.')
+return self._dn
+
+@dn.setter
+def dn(self, value):
+if not isinstance(value, DN):
+raise ValueError('The value must be an instance of DN.')
+self._dn = value
+
+@property
+def rdn(self):
+return self.dn[0]
+
+@property
+def name(self):
+Property holding the name of 

Re: [Freeipa-devel] Fix password changes via kadmin

[Milan Kubik]

On 05/27/2015 04:50 PM, Martin Babinsky wrote:

On 05/27/2015 04:33 PM, Martin Kosek wrote:

On 05/27/2015 03:55 PM, Alexander Bokovoy wrote:

On Wed, 27 May 2015, Simo Sorce wrote:

On Wed, 2015-05-27 at 15:25 +0200, Martin Babinsky wrote:

On 05/25/2015 10:48 AM, Martin Babinsky wrote:

On 04/06/2015 12:53 AM, Simo Sorce wrote:

Fix for bug 4914.

I've tested it locally and seem to do exactly what is needed. I 
couldn't

detect any side effects, except that if you use kadmin to get a
randomized password for a service then you'll get a key for all
supported types (currently aes256, aes128, des3, rc4, camellia128,
camellia256) instead of just the default ones (aes256, aes128, 
des3,

rc4) if you do not specify enctypes. I think that is fine, we use
ipa-getkeytab anyway in the normal course of business and that 
one uses

a different code path.

Simo.





Hi Simo,

the patch works as expected.

My only gripe is with the duplicate code in 
'daemons/ipa-kdb/ipa_kdb.c'
between lines 389 and 455. It could be made into a single 
function to

get key encoding/salt types from LDAP (see my feeble and untested
attempt which I attached).




ACK.

I will then send the patch fixing duplicate code separately once I
consult it with somebody more skilled in C than myself.



Thanks, added your reviewed-by and pushed to master.

Martin, should we push this to other branches too ?

I think we also need this in 4.1 so that it can go to Fedora, Debian,
and RHEL releases.


4.2 will be released soon, but if you are confident about the patch 
so that it
does not break stuff, we may add it to 4.1.x too, given the positive 
impact.



I actually tested it also with 4.1 branch with no problem.


Hello,

there is actually a problem with this patch.

I built it on both branches (to be sure) and the patch causes the 
ipa-server-install fail during the provisioning of directory server 
keytab [1] on *Fedora 21*.
The failure is reproducible. Martin was able to reproduce it on F21. 
Apparently Martin only tested the patch on F22 where it doesn't cause 
any (immediately visible) problems.


[1]: http://paste.fedoraproject.org/226915/90153914/

Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] Fixup fix for 4914

[Milan Kubik]

On 05/29/2015 06:03 PM, Simo Sorce wrote:

New patch attached.

Simo.





Hi,

thanks for the quick fix. With the patch applied, the server was able to 
install.


ACK

Thanks,
Milan
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] certprofiles -- problem with delete

[Milan Kubik]

Hi Fraser and list,

I ran into this when I was tinkering with the commands.

The ipa certprofile plugin[s] does not take the backend result into the
picture right now. When I tried to delete the *default profile*, the entry
from ipa suffix got deleted. However the command failed
and the profile is still in the dogtag managed suffix.
After I've done this to the installed instance, subsequent uninstall
operation failed on some step involving dogtag. I suspect it is related.
I haven't been able to reproduce this for now as at the moment there
was no package with dogtag in the copr repo.
Reproducer for this is attached. (This reproducer requires patches at
least up to freeipa-ftweedal-0005-3-Add-certprofile-plugin.patch)

It may be more complicated issue than it seems, though.
If we delete the ipa managed entry before the dogtag operation
and this one fails, it leaves us in an inconsistent state.
If on the other hand we delete the ipa managed entry after dogtag
call, it opens an possibility of failing to delete the entry in ipa, leading
to inconsistency again.

The solution to this would be a transaction. The problem here is
that the transaction here would span through two integrated
components (three actually, ipa, 389 and dogtag, in this context).
Not an easy thing to do I assume.

TL;DR:

 * certprofile-del deletes ipa managed entry and dogtag doesn't
 * how do we approach possibly irreversible changes in LDAPObject
plugins when integrated component doesn't behave?

Your thoughts on this?

Thanks,
Milan


delete_default_profile.sh
Description: application/shellscript
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 801-806 webui-ci: otptoken tests

[Milan Kubik]

On 05/12/2015 01:57 PM, Petr Vobornik wrote:

On 05/11/2015 01:25 PM, Milan Kubik wrote:

On 05/07/2015 01:38 PM, Petr Vobornik wrote:

On 02/19/2015 03:51 PM, Petr Vobornik wrote:

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

For ipa-4-1 apply:
- patch 800 (different thread)
- patches 801-806

For master apply:
- patch 800 (different thread)
- patch 807 (different thread)
- patch 801-master
- patches 802-806

Patch 801 allows to use ipalib rpc client in Web UI test suite.
Patches 802-805 are various ui_driver fixes to allow stuff in patch 
806.


== [PATCH] 806 webui-ci: otptoken tests ==

Basic otptoken Web UI CI coverage.

tests:
* crud for otptokens as admin
* crud for normal users
* checks fields of adder dialog for both token types and user role
(admin/user)
* token actions as admin (enable, disable, delete)
* token actions as normal user (delete)
* login as normal user with hotp and totp token
* sync token hotp and totp token as normal user and then login

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

== [PATCH] 805 webui-ci: allow custom names for disable/enable
actions ==

Not all disable and enable actions are called 'disable' and 'enable'.

== [PATCH] 804 webui-ci: allow to update pkey in post-add in 
basic-crud

  tests ==

== [PATCH] 803 webui-ci: add post_add_action ==

post add action allows to fill autogenerated values, e.g. a pkey of 
new

otptoken.

This value can be then used in other subsequent test which would 
depend

on it - like crud tests.

== [PATCH] 802 webui-ci: fix negative visibility check ==

Allow to define, that element doesn't have to be present on a page for
negative visible checks.

E.g. if element is added only if it's displayed and is removed
otherwise.

== [PATCH] 801 webui-ci: support direct IPA API calls ==

Add IPA API support to ui_driver. It leverages new ipalib RPC client's
forms based authentication. It then allows to call an IPA API while
the machine is not an IPA client nor is kerberized.

api's environment values are taken from test configuration and
therefore duplication in ~/.ipa/default.conf is not required.

Since the machine doesn't have to be IPA client, it then also doesn't
have nss database with IPA's CA certificate. Therefore on each API
initialization a new NSS database is created with a CA certificate
downloaded from IPA. This db is deleted in tearDown phase.

Usage:

1. as admin one can immediately call rpc commands, api will be
initialized upon first request and is available under self.api
(assuming self is ui_driver):
   self.api.Command.user_del(USER_ID, **{'continue': True})

2. to reconnect as other user:
   self.reconnect_api(USER_ID, USER_PW)

3. reconnect back as admin:
   self.reconnect_api()



Patch #803 needed rebase.



Hi, thanks for the patches.

Please, fix pep8 complaints in 803, 805 and 806.



$ git diff HEAD~6 -U0 | pep8 --diff

returns 20x E501 line too long

IMO, it's better this way for better code readability.



Also, change the header in 806 to the shorter version, please.


Fixed, patches were regenerated.



#
# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
#

Patches 801, 802 and 804 look good to me.
The test cases in 806 look good to me as well.

Milan

I have reviewed the pep8 complaints closely and yes, readability would 
suffer a little.

nicpick-alertI don't like the line 317 after patch 806./nicpick-alert
Fix it at your discretion.
Otherwise ACK.

Thanks,
Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [QE] Place for test data

[Milan Kubik]

Hi list,

while drafting the test cases I have realized I don't know
how to approach $SUBJ. Is there any agreed on place
or 'best practices' for the data that is needed for the testing?
What I need for the test is to have a text file/s and pass
them as arguments to the command.
It is not feasible to have the data with the test source code
in one file.

One possible scenario is:

./test_abc.py
./data/test_abc_xyz_1.txt
./data/test_abc_xyz_2.xml
etc. (emphasis on directory)

or a place dedicated for data of all of the tests such as:
ipatests/test_xmlrpc/test_abc.py
ipatests/data/some/path/to/data.txt

or droping the data files along the test source.

For me the first option looks more sane (less maintenance.
separated from the code, though).

A note from mkosek:
mkosek@balmora [ ~/freeipa ]$ find -type d -name data
./install/ui/test/data
./ipatests/test_ipaserver/data

OTOH, test_pkcs10 has its CSRs in the directory with the test,
test_install is similar to pkcs10.

Can I get your thoughts on this, please?

Thanks,
Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 801-806 webui-ci: otptoken tests

[Milan Kubik]

On 05/07/2015 01:38 PM, Petr Vobornik wrote:

On 02/19/2015 03:51 PM, Petr Vobornik wrote:

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

For ipa-4-1 apply:
- patch 800 (different thread)
- patches 801-806

For master apply:
- patch 800 (different thread)
- patch 807 (different thread)
- patch 801-master
- patches 802-806

Patch 801 allows to use ipalib rpc client in Web UI test suite.
Patches 802-805 are various ui_driver fixes to allow stuff in patch 806.

== [PATCH] 806 webui-ci: otptoken tests ==

Basic otptoken Web UI CI coverage.

tests:
* crud for otptokens as admin
* crud for normal users
* checks fields of adder dialog for both token types and user role
(admin/user)
* token actions as admin (enable, disable, delete)
* token actions as normal user (delete)
* login as normal user with hotp and totp token
* sync token hotp and totp token as normal user and then login

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

== [PATCH] 805 webui-ci: allow custom names for disable/enable 
actions ==


Not all disable and enable actions are called 'disable' and 'enable'.

== [PATCH] 804 webui-ci: allow to update pkey in post-add in basic-crud
  tests ==

== [PATCH] 803 webui-ci: add post_add_action ==

post add action allows to fill autogenerated values, e.g. a pkey of new
otptoken.

This value can be then used in other subsequent test which would depend
on it - like crud tests.

== [PATCH] 802 webui-ci: fix negative visibility check ==

Allow to define, that element doesn't have to be present on a page for
negative visible checks.

E.g. if element is added only if it's displayed and is removed 
otherwise.


== [PATCH] 801 webui-ci: support direct IPA API calls ==

Add IPA API support to ui_driver. It leverages new ipalib RPC client's
forms based authentication. It then allows to call an IPA API while
the machine is not an IPA client nor is kerberized.

api's environment values are taken from test configuration and
therefore duplication in ~/.ipa/default.conf is not required.

Since the machine doesn't have to be IPA client, it then also doesn't
have nss database with IPA's CA certificate. Therefore on each API
initialization a new NSS database is created with a CA certificate
downloaded from IPA. This db is deleted in tearDown phase.

Usage:

1. as admin one can immediately call rpc commands, api will be
initialized upon first request and is available under self.api
(assuming self is ui_driver):
   self.api.Command.user_del(USER_ID, **{'continue': True})

2. to reconnect as other user:
   self.reconnect_api(USER_ID, USER_PW)

3. reconnect back as admin:
   self.reconnect_api()



Patch #803 needed rebase.



Hi, thanks for the patches.

Please, fix pep8 complaints in 803, 805 and 806.
Also, change the header in 806 to the shorter version, please.

#
# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
#

Patches 801, 802 and 804 look good to me.
The test cases in 806 look good to me as well.

Milan
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 807 webui-ci: do not open 2 browser windows

[Milan Kubik]

On 02/19/2015 03:51 PM, Petr Vobornik wrote:

Only for master branch.


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

ACK

Milan
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 800 rpc-client: add forms based auth support

[Milan Kubik]

On 02/19/2015 03:51 PM, Petr Vobornik wrote:
This patch is a prerequisite for patch 801 which will follow. It was 
developed to enable to use ipalib RPC client in Web UI tests. Plus it 
will enable to significantly speed up Web UI tests suite (if 
preparation of data is transformed to use this method).


Partly related https://fedorahosted.org/freeipa/ticket/4772 and 
https://fedorahosted.org/freeipa/ticket/4307



Leverage session support to enable forms-based authenticate in rpc 
client.


In order to do that session support in KerbTransport was moved to new
SessionTransport. RPCClient.create_connection is then modified to
force forms-based auth if new optional options - user and password are
specified. For this case SessionTransport is used and user is
authenticated by calling
'https://ipa.server/ipa/session/login_password'. Session cookie is
stored and used in subsequent calls.

This feature is usable for use cases where one wants to call the API
without being on ipa client. Non-being on ipa client also means that
IPA's NSS database and configuration is not available. Therefore one
has to define ~/.ipa/default.conf in a similar way as ipa client
does and prepare a NSS database with IPA CA cert.

Usage:

api.Backend.rpcclient.connect(
nss_dir=my_nss_dir_path,
user=user,
password=password
)

It's possible to switch users with:

api.Backend.rpcclient.disconnect()

api.Backend.rpcclient.connect(
nss_dir=my_nss_dir_path,
user=other_user,
password=other_password
)

Or check connection with:

api.Backend.rpcclient.isconnected()

Example: download a CA cert and add it to a new temporary NSS database:
from urllib2 import urlparse
from ipaplatform.paths import paths
from ipapython import certdb, ipautil
from ipapython.ipautil import run
from ipalib import x509

# create new NSSDatabase
tmp_db = certdb.NSSDatabase()
pwd_file = ipautil.write_tmp_file(ipautil.ipa_generate_password())
tmp_db.create_db(pwd_file.name)

# download and add cert
url = urlparse.urlunparse(('http', ipautil.format_netloc(ipa_server),
   '/ipa/config/ca.crt', '', '', ''))
stdout, stderr, rc = run([paths.BIN_WGET, -O, -, url])
certs = x509.load_certificate_list(stdout, tmp_db.secdir)
ca_certs = [cert.der_data for cert in certs]
for i, cert in enumerate(ca_certs):
tmp_db.add_cert(cert, 'CA certificate %d' % (i + 1), 'C,,')

my_nss_dir_path = tmp_db.secdir


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

Hi,

thanks for the patch. Please, fix the pep8 complaints.

Can someone else look at the code as well, please?

Thanks,
Milan
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] design review: Certificate Profiles

[Milan Kubik]

On 04/16/2015 10:03 AM, Fraser Tweedale wrote:

Hi everyone,

Please review my Certificate Profiles design proposal:
http://www.freeipa.org/page/V4/Certificate_Profiles

Let me know what is unclear, what needs expansion, and what is plain
wrong :)

The schema for storing multiple certificates for a principal is
still being discussed but I expect it will be agreed soon, and I
will add it to the document.

I am revising the sub-CAs design proposal and it will soon be
published for review as well.

Cheers,
Fraser


Hello Fraser,

I will reiterate one of my concernes from our private mails here for the 
wider audience :)


I'd really like to have a way how to list the profiles managed by IPA 
other than using
the dogtag REST API directly. Simple wrapper around the api call for 
/ca/rest/profiles[/$id[/raw]]
endpoints returning a list of IDs [and dumping the profile to file] for 
the sake of consistency,
since other endpoints are wrapped by ipa commands, would be sufficient 
for me.


This can be also used to query the information (at least the list of 
IDs) when used in the web UI.


I don't know how exactly dogtag is wired into IPA (I've seen that there 
is separate suffix
on the DS instance) and I don't really need to duplicate any data into 
the defaultNamingContext

and its subtree.


Cheers,
Milan

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0027] do not install CA on replica during integration test if setup_ca=False

[Milan Kubik]

On 04/08/2015 08:44 AM, Martin Babinsky wrote:

I have discovered another little bug in the integration test suite.

Attaching a patch that fixes it.




Hello,

thanks for the patch.

I hereby invoke the One Liner rule.

Cheers,
Milan


-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0025] proper client host setup/teardown in forced client reenrollment integration test suite

[Milan Kubik]

On 04/14/2015 03:20 PM, Milan Kubik wrote:



On 03/31/2015 10:42 AM, Martin Babinsky wrote:
During the investigation of 
https://fedorahosted.org/freeipa/ticket/4614 I discovered a bug (?) 
in forced client reenrollment integration test.


During test scenario, master and replica are setup correctly at the 
beginning of the test, but the client is never setup resulting in a 
couple of tracebacks.


After some investigation I realized that the setUp/tearDown methods 
are actually never called because they are supposed to be inherited 
from unittest.TestCase. However, IntegrationTest no longer inherits 
from this class, hence the bug.


I have tried to fix this by adding a fixture which runs client 
fixup/teardown and doing some other small modifications. Tests now 
work as expected, but I need a review from QE guys or someone 
well-versed in pytest framework.


TL;DR: I think I have fixed a bug in integration test but I need 
someone to review the fix because I may not know what I'm doing.



Hello,

please fix the pep8 complaints. Otherwise looks good to me.

Thanks,
Milan

Taking back request on pep8, this is not related to the patch introduced 
code.


ACK.

Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0025] proper client host setup/teardown in forced client reenrollment integration test suite

[Milan Kubik]

On 03/31/2015 10:42 AM, Martin Babinsky wrote:
During the investigation of 
https://fedorahosted.org/freeipa/ticket/4614 I discovered a bug (?) in 
forced client reenrollment integration test.


During test scenario, master and replica are setup correctly at the 
beginning of the test, but the client is never setup resulting in a 
couple of tracebacks.


After some investigation I realized that the setUp/tearDown methods 
are actually never called because they are supposed to be inherited 
from unittest.TestCase. However, IntegrationTest no longer inherits 
from this class, hence the bug.


I have tried to fix this by adding a fixture which runs client 
fixup/teardown and doing some other small modifications. Tests now 
work as expected, but I need a review from QE guys or someone 
well-versed in pytest framework.


TL;DR: I think I have fixed a bug in integration test but I need 
someone to review the fix because I may not know what I'm doing.



Hello,

please fix the pep8 complaints. Otherwise looks good to me.

Thanks,
Milan

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0210] DNSSEC: CI test

[Milan Kubik]

On 04/08/2015 12:46 PM, Martin Basti wrote:

On 07/04/15 15:45, Milan Kubik wrote:



On 03/23/2015 03:54 PM, Martin Basti wrote:

Hello,

a patch with DNSSEC CI tests attached.

* Two types of installation tested
* Tests check if zones are signed on both replica and master
* The root zone test also checks chain of trust

Can somebody very familiar with pytest do review? I'm not sure If I 
used pytest friendly constructions.


PS: test may failure occasionally due a bug in DNSSEC code, but CI 
test itself should be OK


Useful information: http://www.freeipa.org/page/Howto/DNSSEC




Hello,

the patch looks good to me.

Fix the pep8 complaints please (unused imports and long lines).

Thanks,
Milan

Thanks,

updated patch attached.

--
Martin Basti

Thanks,

ack.

Milan
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0210] DNSSEC: CI test

[Milan Kubik]

On 03/23/2015 03:54 PM, Martin Basti wrote:

Hello,

a patch with DNSSEC CI tests attached.

* Two types of installation tested
* Tests check if zones are signed on both replica and master
* The root zone test also checks chain of trust

Can somebody very familiar with pytest do review? I'm not sure If I 
used pytest friendly constructions.


PS: test may failure occasionally due a bug in DNSSEC code, but CI 
test itself should be OK


Useful information: http://www.freeipa.org/page/Howto/DNSSEC




Hello,

the patch looks good to me.

Fix the pep8 complaints please (unused imports and long lines).

Thanks,
Milan
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0001] ipatests: port of p11helper test from github

[Milan Kubik]

Hi,

thanks for the review and sparing me few rounds for these modifications. :)

ACK for the improvements.

Milan

On 03/30/2015 10:28 AM, Martin Basti wrote:

On 27/03/15 13:52, Milan Kubik wrote:

Hi,

On 03/24/2015 04:40 PM, Martin Basti wrote:

On 24/03/15 14:41, Milan Kubik wrote:

Hello,

thanks for the review.

On 03/24/2015 12:39 PM, Martin Basti wrote:

On 17/03/15 10:38, Milan Kubik wrote:

Hi,

On 03/16/2015 05:23 PM, Martin Basti wrote:

On 16/03/15 15:32, Milan Kubik wrote:

On 03/16/2015 12:03 PM, Milan Kubik wrote:

On 03/13/2015 02:59 PM, Milan Kubik wrote:

Hi,

this is a patch with port of [1] to pytest.

[1]: 
https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py 



Cheers,
Milan



Added few more asserts in methods where the test could fail 
and cause other errors.



New version of the patch after brief discussion with Martin 
Basti. Removed unnecessary variable assignments and separated a 
new test case.




Hello,

thank you for the patch.
I have a few nitpicks:
1)
You can remove this and use just hexlify(s)
+def str_to_hex(s):
+return ''.join({:02x}.format(ord(c)) for c in s)

done


2)
+ def test_find_secret_key(self, p11):
+ assert p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY, 
label=užžž-aest)


In tests before you tested the exact number of expected IDs 
returned by find_keys method, why not here?

Lack of attention.
Fixed the assert in `test_search_for_master_key` which does the 
same thing. Merged `test_find_secret_key` with 
`test_search_for_master_key` where it belongs.


Martin^2


Milan



Thank you for patches, just two nitpicks:

1)
Can you use the ipaplatform.paths constant? This is platform specific.
LIBSOFTHSM2_SO = /usr/lib/pkcs11/libsofthsm2.so
LIBSOFTHSM2_SO_64 = /usr/lib64/pkcs11/libsofthsm2.so

Respectively use just LIBSOFTHSM2_SO, on 64bit systems it is 
automatically mapped into LIBSOFTHSM2_SO_64


instead of:
+
+libsofthsm = /usr/lib64/pkcs11/libsofthsm2.so
+


Done.

2)
Can you please check if keys were really deleted?
+def test_delete_key(self, p11):

Done.

--
Martin Basti


I also moved `test_search_for_master_key` right after 
`test_generate_master_key` and changed the assert message to a more 
specific one.


Cheers,
Milan

Please fix this:

1)
$ git am 
freeipa-mkubik-0001-5-ipatests-port-of-p11helper-test-from-github.patch

Applying: ipatests: port of p11helper test from github
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:228: new 
blank line at EOF.

+
warning: 1 line adds whitespace errors.


fixed (TIL: vim doesn't show the last empty line)

2) Please respect PEP8 if it is possible
Mostly done, there are few instances of long variable names off by 
few characters.


3)
I'm still not sure with this:
assert len(master_key) == 0, The master key should be deleted.

following example is more pythonic
assert not master_key, The master key


Changed to the latter variant.

4)
Related to 3), should we test return value, if correct type was 
returned?

assert isinstance(master_key, list) and not master_key, .
I do not insist on this.

Otherwise it works as expected.
--
Martin Basti


Milan


Hello,

I did few modifications:

* new license header
* PEP8 fixes
* variables instead of magic constants for key labels an IDs

Patch attached

Do you accept my modifications?
Martin^2
--
Martin Basti


-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] ipatests: port of p11helper test from github

[Milan Kubik]

Hi,

On 03/24/2015 04:40 PM, Martin Basti wrote:

On 24/03/15 14:41, Milan Kubik wrote:

Hello,

thanks for the review.

On 03/24/2015 12:39 PM, Martin Basti wrote:

On 17/03/15 10:38, Milan Kubik wrote:

Hi,

On 03/16/2015 05:23 PM, Martin Basti wrote:

On 16/03/15 15:32, Milan Kubik wrote:

On 03/16/2015 12:03 PM, Milan Kubik wrote:

On 03/13/2015 02:59 PM, Milan Kubik wrote:

Hi,

this is a patch with port of [1] to pytest.

[1]: 
https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py 



Cheers,
Milan



Added few more asserts in methods where the test could fail and 
cause other errors.



New version of the patch after brief discussion with Martin 
Basti. Removed unnecessary variable assignments and separated a 
new test case.




Hello,

thank you for the patch.
I have a few nitpicks:
1)
You can remove this and use just hexlify(s)
+def str_to_hex(s):
+return ''.join({:02x}.format(ord(c)) for c in s)

done


2)
+ def test_find_secret_key(self, p11):
+ assert p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY, 
label=užžž-aest)


In tests before you tested the exact number of expected IDs 
returned by find_keys method, why not here?

Lack of attention.
Fixed the assert in `test_search_for_master_key` which does the 
same thing. Merged `test_find_secret_key` with 
`test_search_for_master_key` where it belongs.


Martin^2


Milan



Thank you for patches, just two nitpicks:

1)
Can you use the ipaplatform.paths constant? This is platform specific.
LIBSOFTHSM2_SO = /usr/lib/pkcs11/libsofthsm2.so
LIBSOFTHSM2_SO_64 = /usr/lib64/pkcs11/libsofthsm2.so

Respectively use just LIBSOFTHSM2_SO, on 64bit systems it is 
automatically mapped into LIBSOFTHSM2_SO_64


instead of:
+
+libsofthsm = /usr/lib64/pkcs11/libsofthsm2.so
+


Done.

2)
Can you please check if keys were really deleted?
+def test_delete_key(self, p11):

Done.

--
Martin Basti


I also moved `test_search_for_master_key` right after 
`test_generate_master_key` and changed the assert message to a more 
specific one.


Cheers,
Milan

Please fix this:

1)
$ git am 
freeipa-mkubik-0001-5-ipatests-port-of-p11helper-test-from-github.patch

Applying: ipatests: port of p11helper test from github
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:228: new blank 
line at EOF.

+
warning: 1 line adds whitespace errors.


fixed (TIL: vim doesn't show the last empty line)

2) Please respect PEP8 if it is possible
Mostly done, there are few instances of long variable names off by few 
characters.


3)
I'm still not sure with this:
assert len(master_key) == 0, The master key should be deleted.

following example is more pythonic
assert not master_key, The master key


Changed to the latter variant.

4)
Related to 3), should we test return value, if correct type was returned?
assert isinstance(master_key, list) and not master_key, .
I do not insist on this.

Otherwise it works as expected.
--
Martin Basti


Milan
From 64308fc10192ed7892845dd17d5bcb42846d55c2 Mon Sep 17 00:00:00 2001
From: Milan Kubik mku...@redhat.com
Date: Thu, 12 Mar 2015 16:52:33 +0100
Subject: [PATCH] ipatests: port of p11helper test from github

Ported the github hosted [1] script to use pytest's abilities
and included it in ipatests/test_ipapython directory.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

https://fedorahosted.org/freeipa/ticket/4829
---
 ipatests/test_ipapython/test_ipap11helper.py | 271 +++
 make-lint|   2 +-
 2 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 ipatests/test_ipapython/test_ipap11helper.py

diff --git a/ipatests/test_ipapython/test_ipap11helper.py b/ipatests/test_ipapython/test_ipap11helper.py
new file mode 100644
index ..56083c96aa935c36e83eacfc510afbe75c0c78ab
--- /dev/null
+++ b/ipatests/test_ipapython/test_ipap11helper.py
@@ -0,0 +1,271 @@
+# -*- coding: utf-8 -*-
+# Authors:
+#   Milan Kubik mku...@redhat.com
+#
+# Copyright (C) 2015  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see http://www.gnu.org/licenses/.
+
+Test the `ipapython/ipap11helper/p11helper.c` module.
+
+
+
+from binascii import hexlify
+import os
+import os.path
+import logging
+import subprocess
+import tempfile
+
+import pytest
+from ipaplatform.paths import paths
+
+import

Re: [Freeipa-devel] [PATCH] ipatests: port of p11helper test from github

[Milan Kubik]

Hello,

thanks for the review.

On 03/24/2015 12:39 PM, Martin Basti wrote:

On 17/03/15 10:38, Milan Kubik wrote:

Hi,

On 03/16/2015 05:23 PM, Martin Basti wrote:

On 16/03/15 15:32, Milan Kubik wrote:

On 03/16/2015 12:03 PM, Milan Kubik wrote:

On 03/13/2015 02:59 PM, Milan Kubik wrote:

Hi,

this is a patch with port of [1] to pytest.

[1]: 
https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py


Cheers,
Milan



Added few more asserts in methods where the test could fail and 
cause other errors.



New version of the patch after brief discussion with Martin Basti. 
Removed unnecessary variable assignments and separated a new test case.




Hello,

thank you for the patch.
I have a few nitpicks:
1)
You can remove this and use just hexlify(s)
+def str_to_hex(s):
+return ''.join({:02x}.format(ord(c)) for c in s)

done


2)
+ def test_find_secret_key(self, p11):
+ assert p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY, 
label=užžž-aest)


In tests before you tested the exact number of expected IDs returned 
by find_keys method, why not here?

Lack of attention.
Fixed the assert in `test_search_for_master_key` which does the same 
thing. Merged `test_find_secret_key` with 
`test_search_for_master_key` where it belongs.


Martin^2


Milan



Thank you for patches, just two nitpicks:

1)
Can you use the ipaplatform.paths constant? This is platform specific.
LIBSOFTHSM2_SO = /usr/lib/pkcs11/libsofthsm2.so
LIBSOFTHSM2_SO_64 = /usr/lib64/pkcs11/libsofthsm2.so

Respectively use just LIBSOFTHSM2_SO, on 64bit systems it is 
automatically mapped into LIBSOFTHSM2_SO_64


instead of:
+
+libsofthsm = /usr/lib64/pkcs11/libsofthsm2.so
+


Done.

2)
Can you please check if keys were really deleted?
+def test_delete_key(self, p11):

Done.

--
Martin Basti


I also moved `test_search_for_master_key` right after 
`test_generate_master_key` and changed the assert message to a more 
specific one.


Cheers,
Milan
From 059c8ab33ee4b43fff9e74080294ad68075062c7 Mon Sep 17 00:00:00 2001
From: Milan Kubik mku...@redhat.com
Date: Thu, 12 Mar 2015 16:52:33 +0100
Subject: [PATCH] ipatests: port of p11helper test from github

Ported the github hosted [1] script to use pytest's abilities
and included it in ipatests/test_ipapython directory.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

https://fedorahosted.org/freeipa/ticket/4829
---
 ipatests/test_ipapython/test_ipap11helper.py | 216 +++
 make-lint|   2 +-
 2 files changed, 217 insertions(+), 1 deletion(-)
 create mode 100644 ipatests/test_ipapython/test_ipap11helper.py

diff --git a/ipatests/test_ipapython/test_ipap11helper.py b/ipatests/test_ipapython/test_ipap11helper.py
new file mode 100644
index ..6b96f70c7c00f2cbeefd105c637f94c0b498a734
--- /dev/null
+++ b/ipatests/test_ipapython/test_ipap11helper.py
@@ -0,0 +1,216 @@
+# -*- coding: utf-8 -*-
+# Authors:
+#   Milan Kubik mku...@redhat.com
+#
+# Copyright (C) 2015  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see http://www.gnu.org/licenses/.
+
+Test the `ipapython/ipap11helper/p11helper.c` module.
+
+
+
+from binascii import hexlify
+import os
+import os.path
+import logging
+import sys
+import subprocess
+import tempfile
+
+import pytest
+from ipaplatform.paths import paths
+
+import _ipap11helper
+
+
+config_data = 
+# SoftHSM v2 configuration file
+directories.tokendir = %s/tokens
+objectstore.backend = file
+
+
+libsofthsm = paths.LIBSOFTHSM2_SO
+softhsm2_util = paths.SOFTHSM2_UTIL
+
+logging.basicConfig(level=logging.INFO)
+log = logging.getLogger('t')
+
+@pytest.fixture(scope=module)
+def p11(request):
+token_path = tempfile.mkdtemp(prefix='pytest_', suffix='_pkcs11')
+os.chdir(token_path)
+os.mkdir('tokens')
+
+with open('softhsm2.conf', 'w') as cfg:
+cfg.write(config_data % token_path)
+os.environ['SOFTHSM2_CONF'] = os.path.join(token_path, 'softhsm2.conf')
+subprocess.check_call([softhsm2_util, '--init-token', '--slot', '0',
+  '--label', 'test', '--pin', '1234', '--so-pin', '1234'])
+
+try:
+p11 = _ipap11helper.P11_Helper(0, 1234, libsofthsm)
+except _ipap11helper.Error:
+pytest.fail('Failed to initialize the helper object.', pytrace=False)
+
+def fin

Re: [Freeipa-devel] [PATCH] ipatests: port of p11helper test from github

[Milan Kubik]

On 03/16/2015 12:03 PM, Milan Kubik wrote:

On 03/13/2015 02:59 PM, Milan Kubik wrote:

Hi,

this is a patch with port of [1] to pytest.

[1]: 
https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py


Cheers,
Milan



Added few more asserts in methods where the test could fail and cause 
other errors.



New version of the patch after brief discussion with Martin Basti. 
Removed unnecessary variable assignments and separated a new test case.
From d231c1fa215f0eb7ca43750d5b85c9c745b078d0 Mon Sep 17 00:00:00 2001
From: Milan Kubik mku...@redhat.com
Date: Thu, 12 Mar 2015 16:52:33 +0100
Subject: [PATCH] ipatests: port of p11helper test from github

Ported the github hosted [1] script to use pytest's abilities
and included it in ipatests/test_ipapython directory.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

https://fedorahosted.org/freeipa/ticket/4829
---
 ipatests/test_ipapython/test_ipap11helper.py | 220 +++
 make-lint|   2 +-
 2 files changed, 221 insertions(+), 1 deletion(-)
 create mode 100644 ipatests/test_ipapython/test_ipap11helper.py

diff --git a/ipatests/test_ipapython/test_ipap11helper.py b/ipatests/test_ipapython/test_ipap11helper.py
new file mode 100644
index ..3f55156dc2570f005d4744463cae76f9a420bd01
--- /dev/null
+++ b/ipatests/test_ipapython/test_ipap11helper.py
@@ -0,0 +1,220 @@
+# -*- coding: utf-8 -*-
+# Authors:
+#   Milan Kubik mku...@redhat.com
+#
+# Copyright (C) 2015  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see http://www.gnu.org/licenses/.
+
+Test the `ipapython/ipap11helper/p11helper.c` module.
+
+
+
+from binascii import hexlify
+import os
+import os.path
+import logging
+import sys
+import subprocess
+import tempfile
+
+import pytest
+
+import _ipap11helper
+
+config_data = 
+# SoftHSM v2 configuration file
+directories.tokendir = %s/tokens
+objectstore.backend = file
+
+
+libsofthsm = /usr/lib64/pkcs11/libsofthsm2.so
+
+logging.basicConfig(level=logging.INFO)
+log = logging.getLogger('t')
+
+@pytest.fixture(scope=module)
+def p11(request):
+token_path = tempfile.mkdtemp(prefix='pytest_', suffix='_pkcs11')
+os.chdir(token_path)
+os.mkdir('tokens')
+
+with open('softhsm2.conf', 'w') as cfg:
+cfg.write(config_data % token_path)
+
+os.environ['SOFTHSM2_CONF'] = os.path.join(token_path, 'softhsm2.conf')
+
+subprocess.check_call(['softhsm2-util', '--init-token', '--slot', '0',
+  '--label', 'test', '--pin', '1234', '--so-pin', '1234'])
+
+try:
+p11 = _ipap11helper.P11_Helper(0, 1234, libsofthsm)
+except _ipap11helper.Error:
+pytest.fail('Failed to initialize the helper object.', pytrace=False)
+
+def fin():
+try:
+p11.finalize()
+except _ipap11helper.Error:
+pytest.fail('Failed to finalize the helper object.', pytrace=False)
+finally:
+del os.environ['SOFTHSM2_CONF']
+
+request.addfinalizer(fin)
+
+return p11
+
+def str_to_hex(s):
+return ''.join({:02x}.format(ord(c)) for c in s)
+
+class test_p11helper(object):
+def test_generate_master_key(self, p11):
+assert p11.generate_master_key(užžž-aest, m, key_length=16, cka_wrap=True,
+   cka_unwrap=True)
+
+# replica 1
+def test_generate_replica_key_pair(self, p11):
+assert p11.generate_replica_key_pair(ureplica1, id1, pub_cka_wrap=True,
+ priv_cka_unwrap=True)
+
+def test_find_key(self, p11):
+rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY, label=ureplica1, cka_wrap=True)
+assert len(rep1_pub) == 1, replica key pair has to contain 1 pub key instead of %s % len(rep1_pub)
+
+rep1_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY, label=ureplica1, cka_unwrap=True)
+assert len(rep1_priv) == 1, replica key pair has to contain 1 private key instead of %s % len(rep1_priv)
+
+def test_find_key_by_uri(self, p11):
+rep1_pub = p11.find_keys(uri=pkcs11:object=replica1;objecttype=public)
+assert len(rep1_pub) == 1, replica key pair has to contain 1 pub key instead of %s % len(rep1_pub)
+
+def test_get_attribute_from_object(self, p11

[Freeipa-devel] [PATCH] ipatests: port of p11helper test from github

[Milan Kubik]

Hi,

this is a patch with port of [1] to pytest.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

Cheers,
Milan

From 0bbd56eb04e9494ed008d212dabdf32cf6f36e17 Mon Sep 17 00:00:00 2001
From: Milan Kubik mku...@redhat.com
Date: Thu, 12 Mar 2015 16:52:33 +0100
Subject: [PATCH] ipatests: port of p11helper test from github

Ported the github hosted [1] script to use pytest's abilities
and included it in ipatests/test_ipapython directory.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

https://fedorahosted.org/freeipa/ticket/4829
---
 ipatests/test_ipapython/test_ipap11helper.py | 222 +++
 make-lint|   2 +-
 2 files changed, 223 insertions(+), 1 deletion(-)
 create mode 100644 ipatests/test_ipapython/test_ipap11helper.py

diff --git a/ipatests/test_ipapython/test_ipap11helper.py b/ipatests/test_ipapython/test_ipap11helper.py
new file mode 100644
index ..b10cf9a27594aaa0041d54dcb3805d753e6adeb4
--- /dev/null
+++ b/ipatests/test_ipapython/test_ipap11helper.py
@@ -0,0 +1,222 @@
+# -*- coding: utf-8 -*-
+# Authors:
+#   Milan Kubik mku...@redhat.com
+#
+# Copyright (C) 2015  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see http://www.gnu.org/licenses/.
+
+Test the `ipapython/ipap11helper/p11helper.c` module.
+
+
+
+from binascii import hexlify
+import os
+import os.path
+import logging
+import sys
+import subprocess
+import tempfile
+
+import pytest
+
+import _ipap11helper
+
+config_data = 
+# SoftHSM v2 configuration file
+directories.tokendir = %s/tokens
+objectstore.backend = file
+
+
+libsofthsm = /usr/lib64/pkcs11/libsofthsm2.so
+
+logging.basicConfig(level=logging.INFO)
+log = logging.getLogger('t')
+
+@pytest.fixture(scope=module)
+def p11(request):
+token_path = tempfile.mkdtemp(prefix='pytest_', suffix='_pkcs11')
+os.chdir(token_path)
+os.mkdir('tokens')
+
+with open('softhsm2.conf', 'w') as cfg:
+cfg.write(config_data % token_path)
+
+os.environ['SOFTHSM2_CONF'] = os.path.join(token_path, 'softhsm2.conf')
+
+subprocess.check_call(['softhsm2-util', '--init-token', '--slot', '0',
+  '--label', 'test', '--pin', '1234', '--so-pin', '1234'])
+
+try:
+p11 = _ipap11helper.P11_Helper(0, 1234, libsofthsm)
+except _ipap11helper.Error:
+pytest.fail('Failed to initialize the helper object.', pytrace=False)
+
+def fin():
+try:
+p11.finalize()
+except _ipap11helper.Error:
+pytest.fail('Failed to finalize the helper object.', pytrace=False)
+finally:
+del os.environ['SOFTHSM2_CONF']
+
+request.addfinalizer(fin)
+
+return p11
+
+def str_to_hex(s):
+return ''.join({:02x}.format(ord(c)) for c in s)
+
+class test_p11helper(object):
+def test_generate_master_key(self, p11):
+r = p11.generate_master_key(užžž-aest, m, key_length=16, cka_wrap=True,
+cka_unwrap=True)
+assert r
+
+# replica 1
+def test_generate_replica_key_pair(self, p11):
+r = p11.generate_replica_key_pair(ureplica1, id1, pub_cka_wrap=True,
+  priv_cka_unwrap=True)
+assert r
+
+def test_find_key(self, p11):
+rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY, label=ureplica1, cka_wrap=True)
+assert len(rep1_pub) == 1, replica key pair has to contain 1 pub key instead of %s % len(rep1_pub)
+
+rep1_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY, label=ureplica1, cka_unwrap=True)
+assert len(rep1_priv) == 1, replica key pair has to contain 1 private key instead of %s % len(rep1_priv)
+
+def test_find_key_by_uri(self, p11):
+rep1_pub = p11.find_keys(uri=pkcs11:object=replica1;objecttype=public)
+assert len(rep1_pub) == 1, replica key pair has to contain 1 pub key instead of %s % len(rep1_pub)
+
+def test_get_attribute_from_object(self, p11):
+rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY, label=ureplica1, cka_wrap=True)[0]
+
+iswrap = p11.get_attribute(rep1_pub, _ipap11helper.CKA_WRAP)
+assert iswrap is True, replica public key has to have CKA_WRAP = TRUE
+
+
+# replica 2
+def