Re: [Freeipa-devel] [PATCH 0086] disable ipa-replica prepare in non-zero domain levels

[Martin Basti]

On 15.10.2015 16:29, Martin Babinsky wrote:

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




NACK

with domain level 0

ipa-replica-prepare 

ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: DEBUG: File 
"/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 169, in 
execute

self.ask_for_options()
  File 
"/usr/lib/python2.7/site-packages/ipaserver/install/ipa_replica_prepare.py", 
line 215, in ask_for_options

bind_pw=self.dirman_password)
  File "/usr/lib/python2.7/site-packages/ipalib/backend.py", line 61, 
in connect

self.id, threading.currentThread().getName()
ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: DEBUG: The 
ipa-replica-prepare command failed, exception: Exception: connect: 
'context.ldap2_140616703529424' already exists in thread 'MainThread'
ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: ERROR: 
connect: 'context.ldap2_140616703529424' already exists in thread 
'MainThread'
ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: ERROR: The 
ipa-replica-prepare command failed.


without your patch it works

Martin^2
-- 
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 0086] disable ipa-replica prepare in non-zero domain levels

[Martin Babinsky]

On 10/15/2015 04:29 PM, Martin Babinsky wrote:

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




Updated patch attached

--
Martin^3 Babinsky
From aa899aa5f9d9f55c1f3dcaebf79c3460f937815b Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 15 Oct 2015 16:07:48 +0200
Subject: [PATCH] disable ipa-replica-prepare in non-zero IPA domain level

the original replica installation path (ipa-replica-prepare +
ipa-replica-install) remains valid only when IPA domain level is zero. When
this is not the case, ipa-replica-prepare will print out an error message which
instructs the user to use the new replica promotion machinery to setup
replicas.

https://fedorahosted.org/freeipa/ticket/5175
---
 ipaserver/install/ipa_replica_prepare.py | 37 +++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index 2b4a60e16bd23f9d4c8e0135708950a6cc40db9a..becfdc300e6bc77d521aa789081aa0cc4f748afb 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -41,7 +41,20 @@ from ipapython import version
 from ipalib import api
 from ipalib import errors
 from ipaplatform.paths import paths
-from ipalib.constants import CACERT
+from ipalib.constants import CACERT, MIN_DOMAIN_LEVEL
+
+
+UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE = """
+Replica creation using '{}' to generate replica file is supported only
+in {}-level IPA domain.
+
+The current IPA domain level is {} and thus the replica must be created by
+promoting an existing IPA client.
+
+To set up a replica use the following precedure:
+1.) set up a client on the host using 'ipa-client-install'
+2.) promote the client to replica running 'ipa-replica-install --promote'
+"""
 
 
 class ReplicaPrepare(admintool.AdminTool):
@@ -161,6 +174,8 @@ class ReplicaPrepare(admintool.AdminTool):
 api.bootstrap(in_server=True)
 api.finalize()
 
+self.check_domainlevel(api)
+
 if api.env.host == self.replica_fqdn:
 raise admintool.ScriptError("You can't create a replica on itself")
 
@@ -673,3 +688,23 @@ class ReplicaPrepare(admintool.AdminTool):
 '-w', dm_pwd_fd.name,
 '-o', ca_file
 ])
+
+def check_domainlevel(self, api):
+connected = api.Backend.ldap2.isconnected()
+try:
+if not connected:
+api.Backend.ldap2.connect()
+
+domain_level = api.Command.domainlevel_get()['result']
+except Exception as e:
+raise RuntimeError(
+"Cannot determine current domain level: {}".format(e))
+finally:
+if connected:
+api.Backend.ldap2.disconnect()
+
+if domain_level > MIN_DOMAIN_LEVEL:
+raise RuntimeError(
+UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format(
+self.command_name, MIN_DOMAIN_LEVEL, domain_level)
+)
-- 
2.4.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] [PATCHES 0318 - 0320, 0323, 0330] installer: allow to modify dse.ldif during installation

[Martin Babinsky]

On 10/19/2015 12:47 PM, Martin Basti wrote:



On 16.10.2015 12:36, Jan Cholasta wrote:

On 16.10.2015 10:31, Martin Basti wrote:



On 16.10.2015 10:05, Martin Babinsky wrote:

On 10/16/2015 08:47 AM, Jan Cholasta wrote:

On 16.10.2015 08:42, Martin Kosek wrote:

On 10/16/2015 06:00 AM, Jan Cholasta wrote:

On 15.10.2015 19:47, Martin Basti wrote:



On 15.10.2015 18:47, Martin Basti wrote:



On 15.10.2015 18:36, Martin Babinsky wrote:

On 10/15/2015 04:50 PM, Martin Basti wrote:



On 14.10.2015 16:10, Martin Basti wrote:



On 14.10.2015 15:51, Martin Babinsky wrote:

On 10/13/2015 06:38 PM, Martin Basti wrote:



On 12.10.2015 12:30, Martin Babinsky wrote:

On 10/08/2015 05:58 PM, Martin Basti wrote:

The attached patches fix following tickets:
https://fedorahosted.org/freeipa/ticket/4949
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930

With these patches, an administrator can specify LDIF file
that
contains
modifications to be applied to dse.ldif right after
creation
of DS
instance.



Hi,

Functionally the paches work as expected. However I have
following
nitpicks:

in patch 318:

1.) there is a typo in ModifyLDIF class docstring:

+Operations keep the order in whihc were specified
per DN.

in patch 320:

1.) you should use 'os.path.join' to construct FS paths:


-dse_filename = '%s/%s' % (
+dse_filename = os.path.join(
paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE %
self.serverid,
-'dse.ldif',
+'dse.ldif'
  )

2.) IIUC the 'config_ldif_file' knob in BaseServer holds the
path to
LDIF containing the mod operations to dse.ldif. However, the
knob name
sounds like the option accepts the path of dse.ldif
itself. I
propose
to rename the knob to something more in-line with the
supposed
function, like 'dse_mods_file'.



Updated patches + CI test attached


Patches work as expected and CI tests are OK.

However it seems that warning level messages are not logged to
installer output but only to log file (*waves hand* magic).

Maybe it would be a good idea to raise the message level to
"error",
so that it is immediately obvious to the user that his DSE
mods
contain an error and cannot be parsed.

Also you have a typo in the commit message of patch 320
(s/chages/changes/).


Updated patches attached.




Rebased patches for master branch.

ACK


Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b
Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5


These tickets are not for ipa-4-2,
reverted  a17936a1aad139236f18f0e5ad0b066f7747ca60


Can we use a better option name? --dirsrv-config-mods sounds weird,
as you need
to specify a file, not mods...



+1. maybe --dirsrv-config-ldif?


--dirsrv-config-file is most consistent with other options which
accept
files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.)


This, however, may be confusing to user since '--dirsrv-config-file'
may evoke a feeling that it consumes *whole new* dse.ldif while in
reality it is only a few custom mods to directory server configuration.

I agree, it expects only file containing modifications in LDIF format,
'config-file' suffix may be confusing


Sorry, but this does not make any sense. Why would anyone think they
are supposed to specify a complete dse.ldif? Is it written somewhere
that DS config file == dse.ldif? I don't think so. And, if you use
--help, you will see exactly what the option does right away.

What is actually confusing is inventing a "smart" name instead of
making it consistent with everything else.


Patch attached.
Martin^2


ACK





Speaking about the option, I saw some unescaped
"-"'s in the man page updates:

+.TP
+\fB\-\-dirsrv-config-mods\fR
+The path to LDIF file that will be used to modify configuration of
dse.ldif
during installation of the directory server instance

Martin

















--
Martin^3 Babinsky

--
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 0087] execute user-del pre-callback also during user preservation

[Tomas Babej]

On 10/19/2015 10:42 AM, Martin Babinsky wrote:
> On 10/19/2015 07:35 AM, Martin Babinsky wrote:
>> On 10/16/2015 07:28 PM, Martin Babinsky wrote:
>>> fixes tickets:
>>>
>>> https://fedorahosted.org/freeipa/ticket/5362
>>> https://fedorahosted.org/freeipa/ticket/5372
>>>
>>> Upon discussion with Simo we decided that OTP tokens should be
>>> orphaned/deleted also during the user preservation.
>>>
>>>
>>>
>> self-NACK, the current patch violates what we agreed on with Tomas
>> regarding removal of ID overrides.
>>
> Reworked patch attached.
> 

ACK, works as expected.

-- 
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 0012-0019] CA ACL tracker and functional test

[Martin Basti]

On 16.10.2015 15:43, Milan Kubík wrote:

On 09/30/2015 02:47 PM, Martin Basti wrote:










On 09/24/2015 02:49 PM, Milan Kubík
wrote:



Hi
all,




an update for CA ACL tests!




I, with help from M. Babinsky, managed to find a way how to change
the identity during acceptance cest run, which allows


to test CA ACLs (and perhaps other areas with some form of access
controll).




This allowed me to write a test for CA ACLs and certificate
profiles that checks if the ACL/profile is being used and
enforced.


The first several tests are based on Fraser's blogpost using SMIME
profile [1].




The master and ipa-4-2 branches diverged a bit, so I had to change
two commits when rebasing to ipa-4-2 branch.




Commits should be applied in the order (including rebased patches
I sent in an earlier email):




master:


* 12 - 17




ipa-4-2:


* 18, 13 - 15, 19, 17




For convenience:


patches on top of master:
https://github.com/apophys/freeipa/tree/acl-profile-functional


patches on top of ipa-4-2:
https://github.com/apophys/freeipa/tree/acl-42






[1]:
https://blog-ftweedal.rhcloud.com/2015/08/user-certificates-and-custom-profiles-with-freeipa-4-2/



Cheers,


Milan











NACK



0)

rpm file does not contain test_xmlrpc/data directory, please modify
setup.py.in.



1)

Code contains to much todo for my taste.



2)

Please do not use filter function, use dict comprehension.









Hi,

updated patches and the numbering mess somehow curbed. The patches are 
rebased on top of current master and ipa-4-2.


0) fixed by 0021

1) docs for tracker extended, added more test cases

2) changed


--
Milan Kubik

I have a few comments:

1)
+# TODO: rewrite these into Tracker instances
+@pytest.fixture(scope='class')
+def smime_user(request):
+api.Command.user_add(uid=u'alice', givenname=u'Alice', sn=u'SMIME',
+ userpassword=u'Change123')
+
+unlock_principal_password('alice', 'Change123', 'Secret123')
+
+def fin():
+api.Command.user_del(u'alice')
+request.addfinalizer(fin)
+
+return u'alice'

I do not like hardcoded password value, as this password is used in many 
places in the test, I sugest to use a module variable


2)
+class TestSignWithChangedProfile(XMLRPC_test):
+""" Test to verify that the updated profile is used."""
+pass  # import invalid profile, try to sign, expect fail

IMO something is missing here, a test maybe?

3)
# noqa
Please remove "# noqa" commets from commits
-- 
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] [PATCHES 0318 - 0320, 0323, 0330] installer: allow to modify dse.ldif during installation

[Martin Basti]

On 19.10.2015 14:12, Martin Babinsky wrote:

On 10/19/2015 12:47 PM, Martin Basti wrote:



On 16.10.2015 12:36, Jan Cholasta wrote:

On 16.10.2015 10:31, Martin Basti wrote:



On 16.10.2015 10:05, Martin Babinsky wrote:

On 10/16/2015 08:47 AM, Jan Cholasta wrote:

On 16.10.2015 08:42, Martin Kosek wrote:

On 10/16/2015 06:00 AM, Jan Cholasta wrote:

On 15.10.2015 19:47, Martin Basti wrote:



On 15.10.2015 18:47, Martin Basti wrote:



On 15.10.2015 18:36, Martin Babinsky wrote:

On 10/15/2015 04:50 PM, Martin Basti wrote:



On 14.10.2015 16:10, Martin Basti wrote:



On 14.10.2015 15:51, Martin Babinsky wrote:

On 10/13/2015 06:38 PM, Martin Basti wrote:



On 12.10.2015 12:30, Martin Babinsky wrote:

On 10/08/2015 05:58 PM, Martin Basti wrote:

The attached patches fix following tickets:
https://fedorahosted.org/freeipa/ticket/4949
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930

With these patches, an administrator can specify LDIF 
file

that
contains
modifications to be applied to dse.ldif right after
creation
of DS
instance.



Hi,

Functionally the paches work as expected. However I have
following
nitpicks:

in patch 318:

1.) there is a typo in ModifyLDIF class docstring:

+Operations keep the order in whihc were specified
per DN.

in patch 320:

1.) you should use 'os.path.join' to construct FS paths:


-dse_filename = '%s/%s' % (
+dse_filename = os.path.join(
paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE %
self.serverid,
-'dse.ldif',
+'dse.ldif'
  )

2.) IIUC the 'config_ldif_file' knob in BaseServer 
holds the

path to
LDIF containing the mod operations to dse.ldif. 
However, the

knob name
sounds like the option accepts the path of dse.ldif
itself. I
propose
to rename the knob to something more in-line with the
supposed
function, like 'dse_mods_file'.



Updated patches + CI test attached


Patches work as expected and CI tests are OK.

However it seems that warning level messages are not 
logged to

installer output but only to log file (*waves hand* magic).

Maybe it would be a good idea to raise the message level to
"error",
so that it is immediately obvious to the user that his DSE
mods
contain an error and cannot be parsed.

Also you have a typo in the commit message of patch 320
(s/chages/changes/).


Updated patches attached.




Rebased patches for master branch.

ACK


Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b
Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5


These tickets are not for ipa-4-2,
reverted  a17936a1aad139236f18f0e5ad0b066f7747ca60


Can we use a better option name? --dirsrv-config-mods sounds 
weird,

as you need
to specify a file, not mods...



+1. maybe --dirsrv-config-ldif?


--dirsrv-config-file is most consistent with other options which
accept
files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.)


This, however, may be confusing to user since '--dirsrv-config-file'
may evoke a feeling that it consumes *whole new* dse.ldif while in
reality it is only a few custom mods to directory server 
configuration.

I agree, it expects only file containing modifications in LDIF format,
'config-file' suffix may be confusing


Sorry, but this does not make any sense. Why would anyone think they
are supposed to specify a complete dse.ldif? Is it written somewhere
that DS config file == dse.ldif? I don't think so. And, if you use
--help, you will see exactly what the option does right away.

What is actually confusing is inventing a "smart" name instead of
making it consistent with everything else.


Patch attached.
Martin^2


ACK

Pushed to master: f4c8c93e7092b341c3ed2e04553dd5afbcc44dc5





Speaking about the option, I saw some unescaped
"-"'s in the man page updates:

+.TP
+\fB\-\-dirsrv-config-mods\fR
+The path to LDIF file that will be used to modify configuration of
dse.ldif
during installation of the directory server instance

Martin



















--
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] [PATCHES 0318 - 0320, 0323, 0330] installer: allow to modify dse.ldif during installation

[Martin Basti]

On 16.10.2015 12:36, Jan Cholasta wrote:

On 16.10.2015 10:31, Martin Basti wrote:



On 16.10.2015 10:05, Martin Babinsky wrote:

On 10/16/2015 08:47 AM, Jan Cholasta wrote:

On 16.10.2015 08:42, Martin Kosek wrote:

On 10/16/2015 06:00 AM, Jan Cholasta wrote:

On 15.10.2015 19:47, Martin Basti wrote:



On 15.10.2015 18:47, Martin Basti wrote:



On 15.10.2015 18:36, Martin Babinsky wrote:

On 10/15/2015 04:50 PM, Martin Basti wrote:



On 14.10.2015 16:10, Martin Basti wrote:



On 14.10.2015 15:51, Martin Babinsky wrote:

On 10/13/2015 06:38 PM, Martin Basti wrote:



On 12.10.2015 12:30, Martin Babinsky wrote:

On 10/08/2015 05:58 PM, Martin Basti wrote:

The attached patches fix following tickets:
https://fedorahosted.org/freeipa/ticket/4949
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930

With these patches, an administrator can specify LDIF file
that
contains
modifications to be applied to dse.ldif right after 
creation

of DS
instance.



Hi,

Functionally the paches work as expected. However I have
following
nitpicks:

in patch 318:

1.) there is a typo in ModifyLDIF class docstring:

+Operations keep the order in whihc were specified 
per DN.


in patch 320:

1.) you should use 'os.path.join' to construct FS paths:


-dse_filename = '%s/%s' % (
+dse_filename = os.path.join(
paths.ETC_DIRSRV_SLAPD_INSTANCE_TEMPLATE %
self.serverid,
-'dse.ldif',
+'dse.ldif'
  )

2.) IIUC the 'config_ldif_file' knob in BaseServer holds the
path to
LDIF containing the mod operations to dse.ldif. However, the
knob name
sounds like the option accepts the path of dse.ldif 
itself. I

propose
to rename the knob to something more in-line with the 
supposed

function, like 'dse_mods_file'.



Updated patches + CI test attached


Patches work as expected and CI tests are OK.

However it seems that warning level messages are not logged to
installer output but only to log file (*waves hand* magic).

Maybe it would be a good idea to raise the message level to
"error",
so that it is immediately obvious to the user that his DSE 
mods

contain an error and cannot be parsed.

Also you have a typo in the commit message of patch 320
(s/chages/changes/).


Updated patches attached.




Rebased patches for master branch.

ACK


Pushed to master: 5233165ce7062bb7aa649bf95a029103c375207b
Pushed to ipa-4-2: 94412b81c1c09b56ee2900942a1b804f21c264c5


These tickets are not for ipa-4-2,
reverted  a17936a1aad139236f18f0e5ad0b066f7747ca60


Can we use a better option name? --dirsrv-config-mods sounds weird,
as you need
to specify a file, not mods...



+1. maybe --dirsrv-config-ldif?


--dirsrv-config-file is most consistent with other options which 
accept

files (--external-cert-file, --ca-cert-file, --kasp-db-file, etc.)


This, however, may be confusing to user since '--dirsrv-config-file'
may evoke a feeling that it consumes *whole new* dse.ldif while in
reality it is only a few custom mods to directory server configuration.

I agree, it expects only file containing modifications in LDIF format,
'config-file' suffix may be confusing


Sorry, but this does not make any sense. Why would anyone think they 
are supposed to specify a complete dse.ldif? Is it written somewhere 
that DS config file == dse.ldif? I don't think so. And, if you use 
--help, you will see exactly what the option does right away.


What is actually confusing is inventing a "smart" name instead of 
making it consistent with everything else.


Patch attached.
Martin^2






Speaking about the option, I saw some unescaped
"-"'s in the man page updates:

+.TP
+\fB\-\-dirsrv-config-mods\fR
+The path to LDIF file that will be used to modify configuration of
dse.ldif
during installation of the directory server instance

Martin














From 3a78ba53300ea0eba2760edc34076a74b2de970a Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 16 Oct 2015 13:43:43 +0200
Subject: [PATCH] Rename option --dirsrv-config-mods to --dirsrv-config-file

Option is renamed to be consistent with other options.

Affected tickets:
https://fedorahosted.org/freeipa/ticket/4949
https://fedorahosted.org/freeipa/ticket/4048
https://fedorahosted.org/freeipa/ticket/1930
---
 install/tools/man/ipa-replica-install.1| 2 +-
 install/tools/man/ipa-server-install.1 | 2 +-
 ipaserver/install/server/common.py | 6 +++---
 ipaserver/install/server/install.py| 4 ++--
 ipaserver/install/server/replicainstall.py | 2 +-
 ipatests/test_integration/test_customized_ds_config_install.py | 4 ++--
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/install/tools/man/ipa-replica-install.1 b/install/tools/man/ipa-replica-install.1
index 12a5dd7b85af30023614567e9d70073082bda9c1..df01c616c89abeb5c1f421c6e29b1034b2ec9ea7 100644
--- 

Re: [Freeipa-devel] [PATCH 0087] execute user-del pre-callback also during user preservation

[Martin Babinsky]

On 10/19/2015 07:35 AM, Martin Babinsky wrote:

On 10/16/2015 07:28 PM, Martin Babinsky wrote:

fixes tickets:

https://fedorahosted.org/freeipa/ticket/5362
https://fedorahosted.org/freeipa/ticket/5372

Upon discussion with Simo we decided that OTP tokens should be
orphaned/deleted also during the user preservation.




self-NACK, the current patch violates what we agreed on with Tomas
regarding removal of ID overrides.


Reworked patch attached.

--
Martin^3 Babinsky
From bdb643323d5129eb6ebd05874cbc887d022bf3aa Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 16 Oct 2015 19:16:46 +0200
Subject: [PATCH] execute user-del pre-callback also during user preservation

user preservation code was not using the pre-callback function which did check
whether a protected member is being deleted and facilitated the
orphaning/deletion of OTP tokens owner/managed by the user.

https://fedorahosted.org/freeipa/ticket/5362
https://fedorahosted.org/freeipa/ticket/5372
---
 ipalib/plugins/user.py | 47 +++
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/ipalib/plugins/user.py b/ipalib/plugins/user.py
index 848836cd15add707f002b032e31c54d520472bb7..5c3e78b138acb89eb66a6f724019b0c1041b76ce 100644
--- a/ipalib/plugins/user.py
+++ b/ipalib/plugins/user.py
@@ -617,6 +617,10 @@ class user_del(baseuser_del):
 except errors.NotFound:
 self.obj.handle_not_found(pkey)
 
+for callback in self.get_callbacks('pre'):
+dn = callback(self, ldap, dn, pkey, **options)
+assert isinstance(dn, DN)
+
 # start to move the entry to Delete container
 self._exc_wrapper(pkey, options, ldap.move_entry)(dn, delete_dn,
   del_old=True)
@@ -671,28 +675,31 @@ class user_del(baseuser_del):
 # For User life Cycle: user-del is a common plugin
 # command to delete active user (active container) and
 # delete user (delete container).
-# If the target entry is a Delete entry, skip the updates
-# protected member and otptoken owner
-if not dn.endswith(DN(self.obj.delete_container_dn, api.env.basedn)):
-check_protected_member(keys[-1])
+# If the target entry is a Delete entry, skip the orphaning/removal
+# of OTP tokens.
+check_protected_member(keys[-1])
 
-# Delete all tokens owned and managed by this user.
-# Orphan all tokens owned but not managed by this user.
-owner = self.api.Object.user.get_primary_key_from_dn(dn)
-results = self.api.Command.otptoken_find(ipatokenowner=owner)['result']
-for token in results:
-orphan = not [x for x in token.get('managedby_user', []) if x == owner]
-token = self.api.Object.otptoken.get_primary_key_from_dn(token['dn'])
-if orphan:
-self.api.Command.otptoken_mod(token, ipatokenowner=None)
-else:
-self.api.Command.otptoken_del(token)
+if not options.get('preserve', False):
+# Remove any ID overrides tied with this user
+try:
+remove_ipaobject_overrides(self.obj.backend, self.obj.api, dn)
+except errors.NotFound:
+self.obj.handle_not_found(*keys)
 
-# Remove any ID overrides tied with this user
-try:
-remove_ipaobject_overrides(self.obj.backend, self.obj.api, dn)
-except errors.NotFound:
-self.obj.handle_not_found(*keys)
+if dn.endswith(DN(self.obj.delete_container_dn, api.env.basedn)):
+return dn
+
+# Delete all tokens owned and managed by this user.
+# Orphan all tokens owned but not managed by this user.
+owner = self.api.Object.user.get_primary_key_from_dn(dn)
+results = self.api.Command.otptoken_find(ipatokenowner=owner)['result']
+for token in results:
+orphan = not [x for x in token.get('managedby_user', []) if x == owner]
+token = self.api.Object.otptoken.get_primary_key_from_dn(token['dn'])
+if orphan:
+self.api.Command.otptoken_mod(token, ipatokenowner=None)
+else:
+self.api.Command.otptoken_del(token)
 
 return dn
 
-- 
2.4.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 0329] Tests: fix user tracker

[Martin Basti]

Attribute nsaccountlock has not been processed correctly

Patch attached.
From 200717fc3343533a06daafaa98689f7fc517ac58 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Mon, 19 Oct 2015 12:21:07 +0200
Subject: [PATCH] Tests: Fix user tracker

---
 ipatests/test_xmlrpc/test_user_plugin.py | 35 +---
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_user_plugin.py b/ipatests/test_xmlrpc/test_user_plugin.py
index ed95debe72e6ea0d93ca30e520e9e9cb34cad735..9662f033b736e0d8290243e35683a8c4d5dbb920 100644
--- a/ipatests/test_xmlrpc/test_user_plugin.py
+++ b/ipatests/test_xmlrpc/test_user_plugin.py
@@ -1650,7 +1650,7 @@ class test_denied_bind_with_expired_principal(XMLRPC_test):
 
 
 class UserTracker(Tracker):
-""" Class for host plugin like tests """
+""" Class for user plugin like tests """
 
 retrieve_keys = {
 u'uid', u'givenname', u'sn', u'homedirectory',
@@ -1672,7 +1672,7 @@ class UserTracker(Tracker):
 retrieve_preserved_keys = retrieve_keys - {u'memberof_group'}
 retrieve_preserved_all_keys = retrieve_all_keys - {u'memberof_group'}
 
-create_keys = retrieve_all_keys | {
+create_keys = (retrieve_all_keys - {u'nsaccountlock'}) | {
 u'randompassword', u'mepmanagedentry',
 u'krbextradata', u'krbpasswordexpiration', u'krblastpwdchange',
 u'krbprincipalkey', u'randompassword', u'userpassword'
@@ -1693,6 +1693,17 @@ class UserTracker(Tracker):
 
 self.kwargs = kwargs
 
+def _fix_nsaccountlock_attr(self, result):
+# small override because user-* commands returns different type
+# of nsaccountlock value than DS, but overall the value fits
+# expected result
+if u'nsaccountlock' in result:
+if result[u'nsaccountlock'] == [u'true']:
+result[u'nsaccountlock'] = True
+elif result[u'nsaccountlock'] == [u'false']:
+result[u'nsaccountlock'] = False
+
+
 def make_create_command(self, force=None):
 """ Make function that crates a user using user-add """
 return self.make_command(
@@ -1769,6 +1780,7 @@ class UserTracker(Tracker):
 has_password=False,
 mepmanagedentry=[get_group_dn(self.uid)],
 memberof_group=[u'ipausers'],
+nsaccountlock=[u'false'],
 )
 
 for key in self.kwargs:
@@ -1812,14 +1824,7 @@ class UserTracker(Tracker):
 else:
 expected = self.filter_attrs(self.retrieve_keys)
 
-# small override because stageuser-find returns different type
-# of nsaccountlock value than DS, but overall the value fits
-# expected result
-if u'nsaccountlock' in expected:
-if expected[u'nsaccountlock'] == [u'true']:
-expected[u'nsaccountlock'] = True
-elif expected[u'nsaccountlock'] == [u'false']:
-expected[u'nsaccountlock'] = False
+self._fix_nsaccountlock_attr(expected)
 
 assert_deepequal(dict(
 value=self.uid,
@@ -1837,6 +1842,8 @@ class UserTracker(Tracker):
 else:
 expected = self.filter_attrs(self.find_keys)
 
+self._fix_nsaccountlock_attr(expected)
+
 assert_deepequal(dict(
 count=1,
 truncated=False,
@@ -1855,10 +1862,14 @@ class UserTracker(Tracker):
 
 def check_update(self, result, extra_keys=()):
 """ Check 'user-mod' command result """
+expected = self.filter_attrs(self.update_keys | set(extra_keys))
+
+self._fix_nsaccountlock_attr(expected)
+
 assert_deepequal(dict(
 value=self.uid,
 summary=u'Modified user "%s"' % self.uid,
-result=self.filter_attrs(self.update_keys | set(extra_keys))
+result=expected,
 ), result)
 
 def create_from_staged(self, stageduser):
@@ -1916,7 +1927,7 @@ class UserTracker(Tracker):
 ), result)
 
 def track_delete(self, preserve=False):
-"""Update expected state for host deletion"""
+"""Update expected state for user deletion"""
 if preserve:
 self.exists = True
 if u'memberof_group' in self.attrs:
-- 
2.4.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 0086] disable ipa-replica prepare in non-zero domain levels

[Martin Babinsky]

On 10/19/2015 02:47 PM, Martin Basti wrote:



On 15.10.2015 16:29, Martin Babinsky wrote:

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




NACK

with domain level 0

ipa-replica-prepare 

ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: DEBUG: File
"/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 169, in
execute
 self.ask_for_options()
   File
"/usr/lib/python2.7/site-packages/ipaserver/install/ipa_replica_prepare.py",
line 215, in ask_for_options
 bind_pw=self.dirman_password)
   File "/usr/lib/python2.7/site-packages/ipalib/backend.py", line 61,
in connect
 self.id, threading.currentThread().getName()
ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: DEBUG: The
ipa-replica-prepare command failed, exception: Exception: connect:
'context.ldap2_140616703529424' already exists in thread 'MainThread'
ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: ERROR:
connect: 'context.ldap2_140616703529424' already exists in thread
'MainThread'
ipa.ipaserver.install.ipa_replica_prepare.ReplicaPrepare: ERROR: The
ipa-replica-prepare command failed.

without your patch it works

Martin^2


The function was leaking opened backend connection due to incorrect 
disconnect logic. Updated patch should fix this.


--
Martin^3 Babinsky
From 99f42975f478eabf7bd6ebfbf403d04db2ab6866 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 15 Oct 2015 16:07:48 +0200
Subject: [PATCH] disable ipa-replica-prepare in non-zero IPA domain level

the original replica installation path (ipa-replica-prepare +
ipa-replica-install) remains valid only when IPA domain level is zero. When
this is not the case, ipa-replica-prepare will print out an error message which
instructs the user to use the new replica promotion machinery to setup
replicas.

https://fedorahosted.org/freeipa/ticket/5175
---
 ipaserver/install/ipa_replica_prepare.py | 38 +++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index 2b4a60e16bd23f9d4c8e0135708950a6cc40db9a..f4214c8b3c9f084bfe2557b6e750bfe7c1670ee6 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -41,7 +41,21 @@ from ipapython import version
 from ipalib import api
 from ipalib import errors
 from ipaplatform.paths import paths
-from ipalib.constants import CACERT
+from ipalib.constants import CACERT, MIN_DOMAIN_LEVEL
+
+
+UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE = """
+Replica creation using '{}' to generate replica file is supported only
+in {}-level IPA domain.
+
+The current IPA domain level is {} and thus the replica must be created by
+promoting an existing IPA client.
+
+To set up a replica use the following procedure:
+1.) set up a client on the host using 'ipa-client-install'
+2.) promote the client to replica running 'ipa-replica-install' *without*
+replica file specified
+"""
 
 
 class ReplicaPrepare(admintool.AdminTool):
@@ -161,6 +175,8 @@ class ReplicaPrepare(admintool.AdminTool):
 api.bootstrap(in_server=True)
 api.finalize()
 
+self.check_domainlevel(api)
+
 if api.env.host == self.replica_fqdn:
 raise admintool.ScriptError("You can't create a replica on itself")
 
@@ -673,3 +689,23 @@ class ReplicaPrepare(admintool.AdminTool):
 '-w', dm_pwd_fd.name,
 '-o', ca_file
 ])
+
+def check_domainlevel(self, api):
+was_connected = api.Backend.ldap2.isconnected()
+try:
+if not was_connected:
+api.Backend.ldap2.connect()
+
+domain_level = api.Command.domainlevel_get()['result']
+except Exception as e:
+raise RuntimeError(
+"Cannot determine current domain level: {}".format(e))
+finally:
+if not was_connected:
+api.Backend.ldap2.disconnect()
+
+if domain_level > MIN_DOMAIN_LEVEL:
+raise RuntimeError(
+UNSUPPORTED_DOMAIN_LEVEL_TEMPLATE.format(
+self.command_name, MIN_DOMAIN_LEVEL, domain_level)
+)
-- 
2.4.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 0057] Warn in no installation found when running ipa-server-install --uninstall

[Gabe Alford]

Bump for re-review.

On Tue, Oct 13, 2015 at 7:15 AM, Gabe Alford  wrote:

> No worries Petr. All a part of the review process.
>
> I have attached an updated patch that prints only a warning message.
>
> thanks,
>
> Gabe
>
> On Tue, Oct 13, 2015 at 12:39 AM, Petr Spacek  wrote:
>
>> Hello Gabe,
>>
>> I would like to apologize for the confusion regarding this patch and the
>> repeated reworking.
>>
>> Unfortunately Honza's position is not mentioned in the ticket so you
>> could not
>> know what to do, but Honza is our "installer architect" so he has final
>> say.
>>
>> Petr^2 Spacek
>>
>> On 13.10.2015 08:31, Jan Cholasta wrote:
>> > Hi,
>> >
>> > I don't think this is the correct approach. We are aiming to have
>> idempotent
>> > installers, which means that running uninstall on a system without IPA
>> > installed should be a no-op. This is the current behavior, so your
>> patch is
>> > actually moving us back.
>> >
>> > The proper fix would be to *remove* the check from install (as opposed
>> to
>> > adding it to uninstall), but this requires the install code to be
>> idempotent,
>> > and we're not there yet.
>> >
>> > I'm OK with making this a warning, but don't make it a fatal error
>> and/or
>> > require --force.
>> >
>> > Honza
>> >
>> > On 12.10.2015 17:12, Gabe Alford wrote:
>> >> Thanks, Petr. Updated patch attached.
>> >>
>> >> Gabe
>> >>
>> >> On Mon, Oct 12, 2015 at 12:47 AM, Petr Spacek > >> > wrote:
>> >>
>> >> Hello Gabe,
>> >>
>> >> thank you for your patch!
>> >>
>> >> Please note that there might be a case where detection
>> >> is_ipa_configured() is
>> >> broken but the user still needs to run the uninstall process to
>> >> clean it up.
>> >>
>> >> Could you amend the patch to respect --force option? In that case
>> the
>> >> detection should be skipped.
>> >>
>> >> Thank you for your time!
>> >>
>> >> Petr^2 Spacek
>> >>
>> >> On 9.10.2015 19:17, Gabe Alford wrote:
>> >>  > diff --git a/ipaserver/install/server/install.py
>> >> b/ipaserver/install/server/install.py
>> >>  > index
>> >>
>> >>
>> 13a59a0e6149dc22ded4a895db02516e9360e02b..ca93e7a6fd7276d9c0d82eb6f94575730759d858
>> >>
>> >> 100644
>> >>  > --- a/ipaserver/install/server/install.py
>> >>  > +++ b/ipaserver/install/server/install.py
>> >>  > @@ -954,6 +954,12 @@ def uninstall_check(installer):
>> >>  >
>> >>  >  installer._installation_cleanup = False
>> >>  >
>> >>  > +if not is_ipa_configured():
>> >>  > +print("IPA server is not configured on this system.\n"
>> +
>> >>  > +  "If you want to install the IPA server, please
>> >> install " +
>> >>  > +  "it using 'ipa-server-install'.")
>> >>  > +sys.exit(1)
>> >>  > +
>> >>  >  fstore = sysrestore.FileStore(SYSRESTORE_DIR_PATH)
>> >>  >  sstore = sysrestore.StateFile(SYSRESTORE_DIR_PATH)
>>
>
>
-- 
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