Re: [Freeipa-devel] [DESIGN] Text-based rules for CSR autogeneration using Jinja2

2016-07-20 Thread Ben Lipton

On 07/20/2016 12:21 PM, Simo Sorce wrote:

On Wed, 2016-07-20 at 12:14 -0400, Ben Lipton wrote:

On 07/20/2016 10:37 AM, Simo Sorce wrote:

On Wed, 2016-07-20 at 10:17 -0400, Ben Lipton wrote:

On 07/20/2016 06:27 AM, Simo Sorce wrote:

On Tue, 2016-07-19 at 16:20 -0400, Ben Lipton wrote:

Hi,

I have updated the design page
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_
Gene
rati
on/Mapping_Rules
with my plan for implementing user-configurable rules for
mapping
IPA
data into certificate requests. In brief: we will use Jinja2
for
templating. Data rules (which map individual data items) and
syntax
rules (which group them into certificate fields) will both be
snippets
of Jinja2 markup. The formatting process will be as follows:
1. Syntax rules will be rendered using Jinja2. Data rules
(rule
text,
not rendered) will be passed as the datarules attribute.
2. Rendered syntax rules will be processed by the Formatter
class
for
the selected CSR generation helper (e.g. openssl or
certutil).
The
formatter combines these partial rules into a full template
for
the
config.
3. The template will be rendered using Jinja2. Relevant data
from
the
IPA database will be available in the context for this
rendering.
4. The final rendered template will be returned to the
caller,
labeled
with its function (e.g. a command line or a config file).

Are there any comments or objections to this approach? Here's
an
example
to show what it might look like in practice.

Example data rules:
email={{subject.email}}
O={{config.ipacertificatesubjectbase}}\nCN={{subject.username
}}

Example syntax rule:
subjectAltName=@{% section %}{{datarules|join('\n')}}{%
endsection %}

Example composed config template:
[ req ]
prompt = no
encrypt_key = no

distinguished_name = {% section
%}O={{config.ipacertificatesubjectbase}}
CN={{subject.username}}{% endsection %}

req_extensions = exts

[ exts ]
subjectAltName=@{% section %}email={{subject.email}}{%
endsection
%}

There's a lot more information about the thinking behind this
at
http://blog.benjaminlipton.com/2016/07/19/csr-generation-temp
lati
ng.h
tml
if you're interested, as well.

Nice work Ben,
it's been really nice to be able to follow your notes on the
blog
post,
one question remains lingering in my head, why jinja2 ?
I know that engine relatively well as I used it in ipsilon, so
I am
not
questioning the choice just asking why specifically jinja2 and
not
something else, potentially language agnostic.

Simo.

Honestly, my reasoning didn't go very far beyond that it seems to
be
widely used and is compatible with python, which is the language
where
the implementation is taking place (in the IPA RPC server). I
thought
about using the built-in python format strings or creating a
simple
domain-specific language, but the likelihood of wanting the
built-in
text processing features (join, replace, maybe even for loops)
seemed
high, and I didn't want to reimplement those features.

Will the additional package dependency be a problem?

I am more concerned a out the ability to process the data (which I
guess is stored in LDAP) by another client, or in the CLI.
Other than that the dependency does not concern me too much
provided
jinja2 templating is stable and has some guarantee that it will be
supportable long term.

If that is not guaranteed it is a problem, we cannot easily swap
out
one language for another once data is stored and used by the
server.
So the most important consideration for me is whether we are
locking
ourselves into something that will be hard to deal with later or
not.

Should the jinja2 project fail by the wayside next year would we be
able to easily replace it with another engine without changing the
templates as stored ?

Simo.


Ah, ok, I understand the concern. For now, the plan is that the
server
will do all the text processing, so I don't really forsee a need for
any
other client to read the mapping rules from LDAP. However, it's true
that templates written in jinja2 would probably need at least minor
changes to be compatible with another templating engine. (Same goes
for
any other choice - a lot of these engines seem to have very similar,
but
not exactly compatible, syntax). I don't really know how to judge
the
long-term viability of the jinja2 project, though it seems to be
recognized by lots of projects (ansible[1], openstack[2], flask[3],
even
django[4] which has its own templating engine).

In any case, if the team prefers it, I'd be comfortable going with a
more minimal DSL that only has the features we know we need. It
might
slightly limit the types of certs that can be generated, but that can
be
iterated on. But it would be another thing to design, build and
maintain. Let me know what you think.

I am ok using jinja2 as long as we realize we may be on the hook for
maintaining it ourselves in the long term. It's probably easier to do
that than to write our own anyway.

Simo.
It might also be worth pointing out that although the examples in the 
document are 

Re: [Freeipa-devel] [DESIGN] Text-based rules for CSR autogeneration using Jinja2

2016-07-20 Thread Simo Sorce
On Wed, 2016-07-20 at 12:14 -0400, Ben Lipton wrote:
> On 07/20/2016 10:37 AM, Simo Sorce wrote:
> > 
> > On Wed, 2016-07-20 at 10:17 -0400, Ben Lipton wrote:
> > > 
> > > On 07/20/2016 06:27 AM, Simo Sorce wrote:
> > > > 
> > > > On Tue, 2016-07-19 at 16:20 -0400, Ben Lipton wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > I have updated the design page
> > > > > http://www.freeipa.org/page/V4/Automatic_Certificate_Request_
> > > > > Gene
> > > > > rati
> > > > > on/Mapping_Rules
> > > > > with my plan for implementing user-configurable rules for
> > > > > mapping
> > > > > IPA
> > > > > data into certificate requests. In brief: we will use Jinja2
> > > > > for
> > > > > templating. Data rules (which map individual data items) and
> > > > > syntax
> > > > > rules (which group them into certificate fields) will both be
> > > > > snippets
> > > > > of Jinja2 markup. The formatting process will be as follows:
> > > > > 1. Syntax rules will be rendered using Jinja2. Data rules
> > > > > (rule
> > > > > text,
> > > > > not rendered) will be passed as the datarules attribute.
> > > > > 2. Rendered syntax rules will be processed by the Formatter
> > > > > class
> > > > > for
> > > > > the selected CSR generation helper (e.g. openssl or
> > > > > certutil).
> > > > > The
> > > > > formatter combines these partial rules into a full template
> > > > > for
> > > > > the
> > > > > config.
> > > > > 3. The template will be rendered using Jinja2. Relevant data
> > > > > from
> > > > > the
> > > > > IPA database will be available in the context for this
> > > > > rendering.
> > > > > 4. The final rendered template will be returned to the
> > > > > caller,
> > > > > labeled
> > > > > with its function (e.g. a command line or a config file).
> > > > > 
> > > > > Are there any comments or objections to this approach? Here's
> > > > > an
> > > > > example
> > > > > to show what it might look like in practice.
> > > > > 
> > > > > Example data rules:
> > > > > email={{subject.email}}
> > > > > O={{config.ipacertificatesubjectbase}}\nCN={{subject.username
> > > > > }}
> > > > > 
> > > > > Example syntax rule:
> > > > > subjectAltName=@{% section %}{{datarules|join('\n')}}{%
> > > > > endsection %}
> > > > > 
> > > > > Example composed config template:
> > > > > [ req ]
> > > > > prompt = no
> > > > > encrypt_key = no
> > > > > 
> > > > > distinguished_name = {% section
> > > > > %}O={{config.ipacertificatesubjectbase}}
> > > > > CN={{subject.username}}{% endsection %}
> > > > > 
> > > > > req_extensions = exts
> > > > > 
> > > > > [ exts ]
> > > > > subjectAltName=@{% section %}email={{subject.email}}{%
> > > > > endsection
> > > > > %}
> > > > > 
> > > > > There's a lot more information about the thinking behind this
> > > > > at
> > > > > http://blog.benjaminlipton.com/2016/07/19/csr-generation-temp
> > > > > lati
> > > > > ng.h
> > > > > tml
> > > > > if you're interested, as well.
> > > > Nice work Ben,
> > > > it's been really nice to be able to follow your notes on the
> > > > blog
> > > > post,
> > > > one question remains lingering in my head, why jinja2 ?
> > > > I know that engine relatively well as I used it in ipsilon, so
> > > > I am
> > > > not
> > > > questioning the choice just asking why specifically jinja2 and
> > > > not
> > > > something else, potentially language agnostic.
> > > > 
> > > > Simo.
> > > Honestly, my reasoning didn't go very far beyond that it seems to
> > > be
> > > widely used and is compatible with python, which is the language
> > > where
> > > the implementation is taking place (in the IPA RPC server). I
> > > thought
> > > about using the built-in python format strings or creating a
> > > simple
> > > domain-specific language, but the likelihood of wanting the
> > > built-in
> > > text processing features (join, replace, maybe even for loops)
> > > seemed
> > > high, and I didn't want to reimplement those features.
> > > 
> > > Will the additional package dependency be a problem?
> > I am more concerned a out the ability to process the data (which I
> > guess is stored in LDAP) by another client, or in the CLI.
> > Other than that the dependency does not concern me too much
> > provided
> > jinja2 templating is stable and has some guarantee that it will be
> > supportable long term.
> > 
> > If that is not guaranteed it is a problem, we cannot easily swap
> > out
> > one language for another once data is stored and used by the
> > server.
> > So the most important consideration for me is whether we are
> > locking
> > ourselves into something that will be hard to deal with later or
> > not.
> > 
> > Should the jinja2 project fail by the wayside next year would we be
> > able to easily replace it with another engine without changing the
> > templates as stored ?
> > 
> > Simo.
> > 
> Ah, ok, I understand the concern. For now, the plan is that the
> server 
> will do all the text processing, so I don't really forsee a need for
> any 
> other client to read the mapping rules from 

Re: [Freeipa-devel] [DESIGN] Text-based rules for CSR autogeneration using Jinja2

2016-07-20 Thread Ben Lipton

On 07/20/2016 10:37 AM, Simo Sorce wrote:

On Wed, 2016-07-20 at 10:17 -0400, Ben Lipton wrote:

On 07/20/2016 06:27 AM, Simo Sorce wrote:

On Tue, 2016-07-19 at 16:20 -0400, Ben Lipton wrote:

Hi,

I have updated the design page
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Gene
rati
on/Mapping_Rules
with my plan for implementing user-configurable rules for mapping
IPA
data into certificate requests. In brief: we will use Jinja2 for
templating. Data rules (which map individual data items) and
syntax
rules (which group them into certificate fields) will both be
snippets
of Jinja2 markup. The formatting process will be as follows:
1. Syntax rules will be rendered using Jinja2. Data rules (rule
text,
not rendered) will be passed as the datarules attribute.
2. Rendered syntax rules will be processed by the Formatter class
for
the selected CSR generation helper (e.g. openssl or certutil).
The
formatter combines these partial rules into a full template for
the
config.
3. The template will be rendered using Jinja2. Relevant data from
the
IPA database will be available in the context for this rendering.
4. The final rendered template will be returned to the caller,
labeled
with its function (e.g. a command line or a config file).

Are there any comments or objections to this approach? Here's an
example
to show what it might look like in practice.

Example data rules:
email={{subject.email}}
O={{config.ipacertificatesubjectbase}}\nCN={{subject.username}}

Example syntax rule:
subjectAltName=@{% section %}{{datarules|join('\n')}}{%
endsection %}

Example composed config template:
[ req ]
prompt = no
encrypt_key = no

distinguished_name = {% section
%}O={{config.ipacertificatesubjectbase}}
CN={{subject.username}}{% endsection %}

req_extensions = exts

[ exts ]
subjectAltName=@{% section %}email={{subject.email}}{% endsection
%}

There's a lot more information about the thinking behind this at
http://blog.benjaminlipton.com/2016/07/19/csr-generation-templati
ng.h
tml
if you're interested, as well.

Nice work Ben,
it's been really nice to be able to follow your notes on the blog
post,
one question remains lingering in my head, why jinja2 ?
I know that engine relatively well as I used it in ipsilon, so I am
not
questioning the choice just asking why specifically jinja2 and not
something else, potentially language agnostic.

Simo.

Honestly, my reasoning didn't go very far beyond that it seems to be
widely used and is compatible with python, which is the language
where
the implementation is taking place (in the IPA RPC server). I
thought
about using the built-in python format strings or creating a simple
domain-specific language, but the likelihood of wanting the built-in
text processing features (join, replace, maybe even for loops)
seemed
high, and I didn't want to reimplement those features.

Will the additional package dependency be a problem?

I am more concerned a out the ability to process the data (which I
guess is stored in LDAP) by another client, or in the CLI.
Other than that the dependency does not concern me too much provided
jinja2 templating is stable and has some guarantee that it will be
supportable long term.

If that is not guaranteed it is a problem, we cannot easily swap out
one language for another once data is stored and used by the server.
So the most important consideration for me is whether we are locking
ourselves into something that will be hard to deal with later or not.

Should the jinja2 project fail by the wayside next year would we be
able to easily replace it with another engine without changing the
templates as stored ?

Simo.

Ah, ok, I understand the concern. For now, the plan is that the server 
will do all the text processing, so I don't really forsee a need for any 
other client to read the mapping rules from LDAP. However, it's true 
that templates written in jinja2 would probably need at least minor 
changes to be compatible with another templating engine. (Same goes for 
any other choice - a lot of these engines seem to have very similar, but 
not exactly compatible, syntax). I don't really know how to judge the 
long-term viability of the jinja2 project, though it seems to be 
recognized by lots of projects (ansible[1], openstack[2], flask[3], even 
django[4] which has its own templating engine).


In any case, if the team prefers it, I'd be comfortable going with a 
more minimal DSL that only has the features we know we need. It might 
slightly limit the types of certs that can be generated, but that can be 
iterated on. But it would be another thing to design, build and 
maintain. Let me know what you think.


Ben

[1] 
http://docs.ansible.com/ansible/playbooks_variables.html#using-variables-about-jinja2

[2] http://lists.openstack.org/pipermail/openstack-dev/2013-July/012016.html
[3] http://flask.pocoo.org/docs/0.11/templating/
[4] 
https://docs.djangoproject.com/en/1.9/topics/templates/#django.template.backends.jinja2.Jinja2


--
Manage your subscription 

Re: [Freeipa-devel] [PATCH 0028][Tests] Fix failing user tests

2016-07-20 Thread Martin Babinsky

On 07/20/2016 04:11 PM, Lenka Doudova wrote:



On 07/20/2016 02:04 PM, Martin Babinsky wrote:

On 07/15/2016 06:10 PM, Lenka Doudova wrote:

Hi,

here's patch with fix for failing user tests, specifically tests with
renaming users.

Failures were caused by RFE Kerberos principal aliases. As part of the
fix, I had to rewrite few of the tests themselves, since they used
"--setattr" option rather than "--rename" option, which produces
different results.


Lenka





Hi Lenka,

I get nice green *user tests with this patch, so functionally it seems
ok.

However, the commit message does not meet our project standards[1]:

1.) The message summary is very vague, I would rather see something
more specific like:

"""
Tests: improve the handling of rename operations by user tracker
"""

2.) The message itself should be wrapped at the maximum of 78
character width (IIRC) but your lines are way too long.

A nice way to automate this in vim is to highlight the text, enter
command mode and run 'gq', that should format the text for you.

3.) You did not add upstream ticket URL to the end of the message,
please do so.

There is also an extraneous whitespace here:
"""
 else:
 if type(value) is list:
 self.attrs[key] = value
 else:
 self.attrs[key] = [value]
+
 for key, value in expected_updates.items():
 if value is None or value is '' or value is u'':
 del self.attrs[key]
"""

that has nothing to do with the scope of the patch. Please remove it.

[1] http://www.freeipa.org/page/Contribute/Patch_Format


Hi,

thanks for review, fixed patch attached.

Lenka


Thanks, ACK.

Pushed to master: 9093647f867e49bffc9042185b1765b48fe7400a

--
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] 0002 New User Role Tests

2016-07-20 Thread Peter Lacko
Sorry for late reply, I was waiting how the discussion with tracker improvement 
will end, but since there's no progress and I'm leaving soon, I'm attaching new 
patch. I also created mapping between old and new tests [1], to make life of 
reviewer easier. Number in first column denotes line number in original file, 
followed by line number in new tests file and its name. Tests that are not 
mentioned in there extend previous coverage.

Regards,
Peter

[1] http://etherpad.corp.redhat.com/Vk3tRLaPSK


- Original Message -
From: "Martin Basti" 
To: "Peter Lacko" 
Cc: freeipa-devel@redhat.com
Sent: Monday, June 6, 2016 12:29:29 PM
Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests



On 02.06.2016 16:16, Peter Lacko wrote:
> Rebased with updated tests.
>
> Peter
>
> - Original Message -
> From: "Martin Basti" 
> To: "Peter Lacko" 
> Cc: freeipa-devel@redhat.com
> Sent: Thursday, June 2, 2016 1:50:06 PM
> Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests
>
> Your patch doesn't apply anymore on master, there were changes in role
> test and patch have to be updated and rebased
>
> Please look at this for new changes in test_role_plugin.py
> https://git.fedorahosted.org/cgit/freeipa.git/commit/?id=5f42b42bd4557a669ab5cfcf1af6596f1a2535f1
>
> Martin^2
>
>
> On 02.06.2016 11:49, Peter Lacko wrote:
>> Sorry for late response, I wasn't working these days.
>> Fixed patch is in attachment.
>>
>> Peter
>>
>>
>> - Original Message -
>> From: "Martin Basti" 
>> To: "Peter Lacko" , freeipa-devel@redhat.com
>> Sent: Monday, May 9, 2016 1:06:08 PM
>> Subject: Re: [Freeipa-devel] [PATCH] 0002 New User Role Tests
>>
>>
>>
>> On 09.05.2016 13:04, Martin Basti wrote:
>>> On 09.05.2016 12:19, Peter Lacko wrote:
 +# pylint: disable=unicode-builtin
>>> I'm not doing complete review, just the line above hit my eyes.
>>>
>>> unicode() is not in Py3 because all strings there are unicode, thus
>>> you cannot use it directly, you need something like
>>>
>>> if six.PY2:
>>>   str = unicode
>>>
>>> and use str() everywhere and remove that #pylint line
>>>
>>> FYI all enabled pylint checks are there for a good reason, be careful
>>> with disabling it (mainly disabling it for a whole module) rather ask
>>> before if you are not sure.
>>>
>>> Martin^2
>>>
>> Nope, sorry, I temporarily forgot how to python
>>
>> instead of pylint disable use this
>>
>> if six.PY3:
>>unicode =str
>>
>> and keep unicode there. Sorry
>>
>> Martin^2
Hi,

I don't have time to continue with full review, maybe somebody else can 
continue instead of me, but anyway NACK, because using str(unicode()) or 
unicode(str()) is bad in both ways, it should be just unicode() in case 
of strings (with correct mapping unicode=str in Py3). I just  I noticed 
this by reading code, but I didn't do deeper review, so there might be 
more mistakes.

Martin^2
From a5192241e29181e68a480b2b2e802a4267cea4d6 Mon Sep 17 00:00:00 2001
From: Peter Lacko 
Date: Tue, 31 May 2016 14:34:19 +0200
Subject: [PATCH] New User Role tests.

Commit contains set of User Role tests, implementation of UserTracker
and basic implementation of PrivilegeTracker necessary for Role testing.
Tests replace previous declarative tests for User Role plugin and extends
their coverage.

Commit contains 74 tests split into classes as follows:

TestNonexistentRole
Test that no operation can be performed on non-existing role.
TestDeletedRole
Test that no operation can be performed on deleted role.
TestRoleBasic
Test basic role commands: create, update, delete.
TestRoleUpdate
Test role-mod command.
TestRolePrivileges
Test correct functionality role-privilege-* command.
TestRoleMembers
Test correct functionality of role-member-* command.
TestRoleFind
Test correct functionality of role-find command.

Other than previously mentiond functionality has not beed added, removed or modified.
---
 ipatests/test_xmlrpc/test_role_plugin.py | 1334 +++---
 ipatests/test_xmlrpc/tracker/privilege_plugin.py |   96 ++
 ipatests/test_xmlrpc/tracker/role_plugin.py  |  583 ++
 3 files changed, 1343 insertions(+), 670 deletions(-)
 create mode 100644 ipatests/test_xmlrpc/tracker/privilege_plugin.py
 create mode 100644 ipatests/test_xmlrpc/tracker/role_plugin.py

diff --git a/ipatests/test_xmlrpc/test_role_plugin.py b/ipatests/test_xmlrpc/test_role_plugin.py
index 6771cfad503e998a2c1433cc4c012210e815eab9..b6163cbf785e76d267f059658c314d80fbd64f51 100644
--- a/ipatests/test_xmlrpc/test_role_plugin.py
+++ b/ipatests/test_xmlrpc/test_role_plugin.py
@@ -2,8 +2,9 @@
 #   Rob Crittenden 
 #   Pavel Zuna 
 #   John Dennis 
+#   Peter Lacko 
 #
-# Copyright (C) 2009  

[Freeipa-devel] [PATCH] 0078-82: webui tests: tests for new certificate widget

2016-07-20 Thread Pavel Vomacka
Please review attached patches, which add tests for new certificate 
widget in WebUI.


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

--
Pavel^3 Vomacka

From 66879ea9d38ea42e2ac5641d0f89643edea83d16 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 20 Jul 2016 16:27:47 +0200
Subject: [PATCH 5/9] Add possibility to choose parent element by css

Part of: https://fedorahosted.org/freeipa/ticket/6064
---
 ipatests/test_webui/ui_driver.py | 42 +++-
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/ipatests/test_webui/ui_driver.py b/ipatests/test_webui/ui_driver.py
index 7c4ca75efa3e642f4a2c0cdcd72be3cafa3c305a..c82164c53095d8552aef665c5091deeb3bd71437 100644
--- a/ipatests/test_webui/ui_driver.py
+++ b/ipatests/test_webui/ui_driver.py
@@ -615,12 +615,17 @@ class UI_driver(object):
 s = "a[name='%s'].action-button" % name
 self._button_click(s, parent, name)
 
-def button_click(self, name, parent=None):
+def button_click(self, name, parent=None,
+parents_css_sel=None):
 """
 Click on .ui-button
 """
 if not parent:
-parent = self.get_form()
+if parents_css_sel:
+parent = self.find(parents_css_sel, By.CSS_SELECTOR,
+strict=True)
+else:
+parent = self.get_form()
 
 s = "[name='%s'].btn" % name
 self._button_click(s, parent, name)
@@ -1413,14 +1418,25 @@ class UI_driver(object):
 for key in pkeys:
 self.assert_record(key, parent, table_name, negative=True)
 
-def action_list_action(self, name, confirm=True, confirm_btn="ok"):
+def action_list_action(self, name, confirm=True, confirm_btn="ok",
+parents_css_sel=None):
 """
 Execute action list action
 """
-cont = self.find(".active-facet .facet-actions", By.CSS_SELECTOR, strict=True)
-expand = self.find(".dropdown-toggle", By.CSS_SELECTOR, cont, strict=True)
+context = None
+
+if not parents_css_sel:
+context = self.find(".active-facet .facet-actions",
+By.CSS_SELECTOR, strict=True)
+else:
+context = self.find(parents_css_sel, By.CSS_SELECTOR,
+strict=True)
+
+expand = self.find(".dropdown-toggle", By.CSS_SELECTOR, context,
+strict=True)
 expand.click()
-action_link = self.find("li[data-name=%s] a" % name, By.CSS_SELECTOR, cont, strict=True)
+action_link = self.find("li[data-name=%s] a" % name, By.CSS_SELECTOR,
+context, strict=True)
 action_link.click()
 if confirm:
 self.wait(0.5)  # wait for dialog
@@ -1739,17 +1755,25 @@ class UI_driver(object):
 assert is_enabled == enabled, ('Invalid enabled state of action button %s. '
'Expected: %s') % (action, str(visible))
 
-def assert_action_list_action(self, action, visible=True, enabled=True, parent=None):
+def assert_action_list_action(self, action, visible=True, enabled=True, parent=None,
+parents_css_sel=None, facet_actions=True):
 """
 Assert that action dropdown action is visible/hidden, and enabled/disabled
 
 Enabled is checked only if action is visible.
 """
+
+li_s = " li[data-name='%s']" % action
+
 if not parent:
 parent = self.get_form()
 
-s = ".facet-actions li[data-name='%s']" % action
-li = self.find(s, By.CSS_SELECTOR, parent)
+if facet_actions:
+li_s = ".facet-actions" + li_s
+else:
+li_s = parents_css_sel + li_s
+
+li = self.find(li_s, By.CSS_SELECTOR, parent)
 link = self.find("a", By.CSS_SELECTOR, li)
 
 is_visible = li is not None and link is not None
-- 
2.5.5

From 22c0f8c5b2cd961795770662d22b3bb35573eb07 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 20 Jul 2016 16:28:38 +0200
Subject: [PATCH 6/9] Add function which check whether the field is empty

Part of: https://fedorahosted.org/freeipa/ticket/6064
---
 ipatests/test_webui/ui_driver.py | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/ipatests/test_webui/ui_driver.py b/ipatests/test_webui/ui_driver.py
index c82164c53095d8552aef665c5091deeb3bd71437..9c8380056ebee8feab6435e4383009ae6a9aeb9f 100644
--- a/ipatests/test_webui/ui_driver.py
+++ b/ipatests/test_webui/ui_driver.py
@@ -1566,6 +1566,17 @@ class UI_driver(object):
 s = "div[name='%s'] %s[name='%s']" % (name, element, name)
 self.assert_text(s, value, parent)
 
+def assert_empty_value(self, selector, parent=None, negative=False):
+"""
+ 

[Freeipa-devel] [PATCH 0002][Tests] Small fix for dns_plugin tests

2016-07-20 Thread Ganna Kaihorodova
Greetings!

Fix for ipatests/test_xmlrpc/test_dns_plugin.py

Fix conflict between “got” and “expected” values when testing "dnsconfig_mod: 
Update global DNS settings"

Best regards,
Ganna Kaihorodova
Associate Software Quality Engineer


From 94899bbb538129384a7faa22be1228e2fcd453cf Mon Sep 17 00:00:00 2001
From: Ganna Kaihorodova 
Date: Mon, 18 Jul 2016 19:20:51 +0200
Subject: [PATCH 2/2] =?UTF-8?q?Fix=20conflict=20between=20=E2=80=9Cgot?=
 =?UTF-8?q?=E2=80=9D=20and=20=E2=80=9Cexpected=E2=80=9D=20values=20when=20?=
 =?UTF-8?q?testing=20"dnsconfig=5Fmod:=20Update=20global=20DNS=20settings"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 ipatests/test_xmlrpc/test_dns_plugin.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipatests/test_xmlrpc/test_dns_plugin.py b/ipatests/test_xmlrpc/test_dns_plugin.py
index 038d17e9debc86a8e9afcadcea30e7cee29bbd05..fd32ef24beeaa245e829554fb7a6da173f6624bc 100644
--- a/ipatests/test_xmlrpc/test_dns_plugin.py
+++ b/ipatests/test_xmlrpc/test_dns_plugin.py
@@ -1781,6 +1781,7 @@ class test_dns(Declarative):
  }},
 ),
 'result': {
+'dns_server_server': [api.env.host],
 'idnsforwarders': [fwd_ip],
 },
 },
@@ -1792,7 +1793,7 @@ class test_dns(Declarative):
 expected={
 'value': None,
 'summary': u'Global DNS configuration is empty',
-'result': {},
+'result': {'dns_server_server': [api.env.host]},
 },
 ),

--
2.7.4

-- 
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] webui test: bunch of patches which fix webui patches

2016-07-20 Thread Pavel Vomacka



On 07/11/2016 06:33 PM, Pavel Vomacka wrote:

Hello,

please review these patches. First four of them fixes patches and the 
last one fixes small bug in WebUI which causes that some tests fail.


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

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

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

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




4 patches have incorrect information about user who makes the patch. 
Sending new patches which correct it.


--
Pavel^3 Vomacka

From 411f99e6ba9e2f811d8e4408eedd3399c2d89275 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 20 Jul 2016 16:13:29 +0200
Subject: [PATCH 1/9] Remove navigation using breadcrumb menus

https://fedorahosted.org/freeipa/ticket/6054
---
 ipatests/test_webui/test_automember.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/ipatests/test_webui/test_automember.py b/ipatests/test_webui/test_automember.py
index 9f3da1e9693ec8f8a924b9e81545161eb2af44c9..5b07849b82b09177093a5bfeaa11780af419ac3c 100644
--- a/ipatests/test_webui/test_automember.py
+++ b/ipatests/test_webui/test_automember.py
@@ -159,7 +159,6 @@ class test_automember(UI_driver):
 
 # Rebuild membership for all hosts, using action on hosts search facet
 self.navigate_by_menu('identity/host')
-self.navigate_by_breadcrumb('Hosts')
 self.action_list_action('automember_rebuild')
 
 # Assert that hosts are now members of hostgroup
@@ -256,8 +255,7 @@ class test_automember(UI_driver):
 self.assert_record('dev2', negative=True)
 
 # Rebuild membership for all users, using action on users search facet
-self.navigate_by_menu('identity/user')
-self.navigate_by_breadcrumb('Users')
+self.navigate_by_menu('identity/user_search')
 self.action_list_action('automember_rebuild')
 
 # Assert that users are now members of group
-- 
2.5.5

From 00e0d44ba47df679cd8b70c7b92052d5ff915ad9 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 20 Jul 2016 16:14:16 +0200
Subject: [PATCH 2/9] Fix test_navigation tests

Some menu item names has changed. This commit sets the correct names.

https://fedorahosted.org/freeipa/ticket/6053
---
 ipatests/test_webui/test_navigation.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ipatests/test_webui/test_navigation.py b/ipatests/test_webui/test_navigation.py
index b5dc928ac4d631fb1fc8f9ea4b0d7ddbc213e5dc..b51d71d03caa3f7f0f5abfc0939e9b41cd471037 100644
--- a/ipatests/test_webui/test_navigation.py
+++ b/ipatests/test_webui/test_navigation.py
@@ -107,7 +107,7 @@ class test_navigation(UI_driver):
 # Identity
 # don't start by users (default)
 self.navigate_by_menu('identity/group', False)
-self.navigate_by_menu('identity/user', False)
+self.navigate_by_menu('identity/user_search', False)
 self.navigate_by_menu('identity/host', False)
 self.navigate_by_menu('identity/hostgroup', False)
 self.navigate_by_menu('identity/netgroup', False)
@@ -136,9 +136,9 @@ class test_navigation(UI_driver):
 self.navigate_by_menu('authentication/radiusproxy', False)
 self.navigate_by_menu('authentication/otptoken', False)
 if self.has_ca():
-self.navigate_by_menu('authentication/cert', False)
+self.navigate_by_menu('authentication/cert_search', False)
 else:
-self.assert_menu_item('authentication/cert', False)
+self.assert_menu_item('authentication/cert_search', False)
 
 # Network Services
 self.navigate_by_menu('network_services')
-- 
2.5.5

From b39224afcf9b9d69c8823d7a0622dc78c40e81a5 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 20 Jul 2016 16:15:07 +0200
Subject: [PATCH 3/9] Fix test which checks removing of user

The name of delete action is now 'delete_active_user' not just 'delete' therefore
tests needs to be fixed.

https://fedorahosted.org/freeipa/ticket/6052
---
 ipatests/test_webui/test_user.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipatests/test_webui/test_user.py b/ipatests/test_webui/test_user.py
index 5b509d18ca9a0738af1595678fd56ba57d405079..e926c22123ab449db2c3c061e500fa6e137d875d 100644
--- a/ipatests/test_webui/test_user.py
+++ b/ipatests/test_webui/test_user.py
@@ -161,7 +161,7 @@ class test_user(UI_driver):
 self.action_list_action('unlock')
 
 # delete
-self.delete_action(user.ENTITY, user.PKEY)
+self.delete_action(user.ENTITY, user.PKEY, action='delete_active_user')
 
 @screenshot
 def test_password_expiration_notification(self):
-- 
2.5.5

From 050cb5e6f9f87f360a2a32562b1a83e9b3c0cedf Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 20 Jul 2016 16:15:34 +0200
Subject: [PATCH 4/9] Set default delete action name to 'delete'

Only specific delete 

Re: [Freeipa-devel] [PATCH] 0011 server uninstall fails to remove krb principals

2016-07-20 Thread Martin Basti



On 19.07.2016 12:56, Petr Vobornik wrote:

On 07/11/2016 09:52 AM, Florence Blanc-Renaud wrote:

Hi,

please find a patch for the 3rd issue of ticket 6012.

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



bump for review


ACK

Pushed to master: a0d90263d62f48f0c04b8b9e7da3aaa10201c3a0

--
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 0180] allow multiple dashes in the components of server hostname

2016-07-20 Thread Martin Basti



On 04.07.2016 12:54, Martin Babinsky wrote:
The PKI bug preventing use of multiple dashes in hostnames [1] was 
already fixed. We may now relax our own syntax constraints.


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

[1] https://fedorahosted.org/pki/ticket/1260





ACK
Pushed to master: 15cfd0ee20fd05735473d3677b6f9f349339197e


-- 
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] Text-based rules for CSR autogeneration using Jinja2

2016-07-20 Thread Ben Lipton

On 07/20/2016 06:27 AM, Simo Sorce wrote:

On Tue, 2016-07-19 at 16:20 -0400, Ben Lipton wrote:

Hi,

I have updated the design page
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generati
on/Mapping_Rules
with my plan for implementing user-configurable rules for mapping
IPA
data into certificate requests. In brief: we will use Jinja2 for
templating. Data rules (which map individual data items) and syntax
rules (which group them into certificate fields) will both be
snippets
of Jinja2 markup. The formatting process will be as follows:
1. Syntax rules will be rendered using Jinja2. Data rules (rule
text,
not rendered) will be passed as the datarules attribute.
2. Rendered syntax rules will be processed by the Formatter class
for
the selected CSR generation helper (e.g. openssl or certutil). The
formatter combines these partial rules into a full template for the
config.
3. The template will be rendered using Jinja2. Relevant data from
the
IPA database will be available in the context for this rendering.
4. The final rendered template will be returned to the caller,
labeled
with its function (e.g. a command line or a config file).

Are there any comments or objections to this approach? Here's an
example
to show what it might look like in practice.

Example data rules:
email={{subject.email}}
O={{config.ipacertificatesubjectbase}}\nCN={{subject.username}}

Example syntax rule:
subjectAltName=@{% section %}{{datarules|join('\n')}}{% endsection %}

Example composed config template:
[ req ]
prompt = no
encrypt_key = no

distinguished_name = {% section
%}O={{config.ipacertificatesubjectbase}}
CN={{subject.username}}{% endsection %}

req_extensions = exts

[ exts ]
subjectAltName=@{% section %}email={{subject.email}}{% endsection %}

There's a lot more information about the thinking behind this at
http://blog.benjaminlipton.com/2016/07/19/csr-generation-templating.h
tml
if you're interested, as well.

Nice work Ben,
it's been really nice to be able to follow your notes on the blog post,
one question remains lingering in my head, why jinja2 ?
I know that engine relatively well as I used it in ipsilon, so I am not
questioning the choice just asking why specifically jinja2 and not
something else, potentially language agnostic.

Simo.
Honestly, my reasoning didn't go very far beyond that it seems to be 
widely used and is compatible with python, which is the language where 
the implementation is taking place (in the IPA RPC server). I thought 
about using the built-in python format strings or creating a simple 
domain-specific language, but the likelihood of wanting the built-in 
text processing features (join, replace, maybe even for loops) seemed 
high, and I didn't want to reimplement those features.


Will the additional package dependency be a problem?

Ben

--
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][Tests] Fix for dns_plugin tests

2016-07-20 Thread Martin Basti

Hello,


On 19.07.2016 17:45, Ganna Kaihorodova wrote:

Greetings!

Fix for ipatests/test_xmlrpc/test_dns_plugin.py 
(test_forwardzone_delegation_warnings.test)

You can't have a DNS zone with the authoritative nameserver that does not have 
a A or  record in the local DNS.

Not true


  Since in some test environments primary hostname of the master is managed by 
an external DNS, this hostname can not be used as a NS-record for IPA-managed 
zones. Therefore another existing fqdn corresponding to the master's ip and 
managed by IPA DNS was used.
I really don't like using 'ipa-ca.' domain name as server 
name, this is not right.


I was digging deeper today to find what is the root case why it doesn't 
work, and it looks that after some tests, named is not able to resolve 
hostname, even if global forwarder is specified.
So this looks like bug on named/bind-dyndb-ldap side, because when I 
restarted named-pkcs11 before the first test that is failing and all 
forwardzone tests passed.


So I think this patch just hides the real issue and we should discard 
it. I and pspacek will be continue investigating this tomorrow.


Notes to the patch format:

Please split that huge text in commit message to:

short summary (max 72 chars)
empty line
longer description
empty line
[ticket (if available)]

regars
Martin^2



Best regards,
Ganna Kaihorodova
Associate Software Quality Engineer






-- 
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 0028][Tests] Fix failing user tests

2016-07-20 Thread Lenka Doudova



On 07/20/2016 02:04 PM, Martin Babinsky wrote:

On 07/15/2016 06:10 PM, Lenka Doudova wrote:

Hi,

here's patch with fix for failing user tests, specifically tests with
renaming users.

Failures were caused by RFE Kerberos principal aliases. As part of the
fix, I had to rewrite few of the tests themselves, since they used
"--setattr" option rather than "--rename" option, which produces
different results.


Lenka





Hi Lenka,

I get nice green *user tests with this patch, so functionally it seems 
ok.


However, the commit message does not meet our project standards[1]:

1.) The message summary is very vague, I would rather see something 
more specific like:


"""
Tests: improve the handling of rename operations by user tracker
"""

2.) The message itself should be wrapped at the maximum of 78 
character width (IIRC) but your lines are way too long.


A nice way to automate this in vim is to highlight the text, enter 
command mode and run 'gq', that should format the text for you.


3.) You did not add upstream ticket URL to the end of the message, 
please do so.


There is also an extraneous whitespace here:
"""
 else:
 if type(value) is list:
 self.attrs[key] = value
 else:
 self.attrs[key] = [value]
+
 for key, value in expected_updates.items():
 if value is None or value is '' or value is u'':
 del self.attrs[key]
"""

that has nothing to do with the scope of the patch. Please remove it.

[1] http://www.freeipa.org/page/Contribute/Patch_Format


Hi,

thanks for review, fixed patch attached.

Lenka
From b76139052fa290111642464d3256b4ff15b184eb Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Fri, 15 Jul 2016 17:57:53 +0200
Subject: [PATCH] Tests: Improve handling of rename operation by user tracker

Improving handling of rename operation by user tracker, together with
fixes for user tests, that failed as consequence.
Failures were caused by RFE Kerberos principal alias.

Some tests were rewritten, since they used "--setattr" option instead of
"--rename", and hence didn't reflect proper behaviour of the principal
aliases feature.

https://fedorahosted.org/freeipa/ticket/6024
---
 ipatests/test_xmlrpc/test_user_plugin.py| 31 ++---
 ipatests/test_xmlrpc/tracker/user_plugin.py |  9 +
 2 files changed, 15 insertions(+), 25 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_user_plugin.py b/ipatests/test_xmlrpc/test_user_plugin.py
index def522814f6c0a894f0bd8f352e110a95e5aa09a..7c27abc56cb859eb4fb710f1ff384793dfbe453c 100644
--- a/ipatests/test_xmlrpc/test_user_plugin.py
+++ b/ipatests/test_xmlrpc/test_user_plugin.py
@@ -316,24 +316,10 @@ class TestUpdate(XMLRPC_test):
 renameduser.ensure_missing()
 olduid = user.uid
 
-# using user.update(dict(uid=value)) results in
-# OverlapError: overlapping arguments and options: ['uid']
-user.attrs.update(uid=[renameduser.uid])
-command = user.make_update_command(
-updates=dict(setattr=(u'uid=%s' % renameduser.uid))
-)
-result = command()
-user.check_update(result)
-user.uid = renameduser.uid
+user.update(updates=dict(rename=renameduser.uid))
 
 # rename the test user back so it gets properly deleted
-user.attrs.update(uid=[olduid])
-command = user.make_update_command(
-updates=dict(setattr=(u'uid=%s' % olduid))
-)
-result = command()
-user.check_update(result)
-user.uid = olduid
+user.update(updates=dict(rename=olduid))
 
 def test_rename_to_the_same_value(self, user):
 """ Try to rename user to the same value """
@@ -640,18 +626,13 @@ class TestUserWithGroup(XMLRPC_test):
 if its manager is also renamed """
 renamed_name = u'renamed_npg2'
 old_name = user_npg2.uid
-command = user_npg2.make_update_command(dict(rename=renamed_name))
-result = command()
-user_npg2.attrs.update(uid=[renamed_name])
-user_npg2.check_update(result)
+
+user_npg2.update(updates=dict(rename=renamed_name))
+
 user_npg.attrs.update(manager=[renamed_name])
 user_npg.retrieve(all=True)
 
-command = user_npg2.make_command(
-'user_mod', renamed_name, **dict(rename=old_name)
-)
-# we rename the user back otherwise the tracker is too confused
-result = command()
+user_npg2.update(updates=dict(rename=old_name))
 
 def test_check_if_manager_gets_removed(self, user_npg, user_npg2):
 """ Delete manager and check if it's gone from user's attributes """
diff --git a/ipatests/test_xmlrpc/tracker/user_plugin.py b/ipatests/test_xmlrpc/tracker/user_plugin.py
index 1a85e93327e5d517249fd67e208e83a922509002..d95c3e223106451b4114ace4ddf564085e6a0511 100644
--- 

Re: [Freeipa-devel] PATCH: Improve on #2795 patches

2016-07-20 Thread David Kupka

On 20/07/16 12:11, Simo Sorce wrote:

Attached patch introduces a helper function and avoids the questionable
replace+delete operations where possible (still employed in the
entry_to_mods function).
Compiles and I am about to test it, but I'd like feedback on it if
anyone wants to take a look.

Simo.





Looks and works good, ACK.

--
David Kupka

--
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] [freeipa] #6002: Default CA can be used without an ACL

2016-07-20 Thread Jan Cholasta

On 19.7.2016 09:01, Fraser Tweedale wrote:

On Tue, Jul 19, 2016 at 08:26:22AM +0200, Jan Cholasta wrote:

Hi,

On 4.7.2016 09:06, Fraser Tweedale wrote:

On Tue, Jun 28, 2016 at 01:47:23PM -, freeipa wrote:

#6002: Default CA can be used without an ACL

Comment (by ftweedal):

 This is expected behaviour; if a CA ACL does not reference any CAs,
 and does not have cacat=all, then it is assumed to refer to the
 default CA.  This is for backwards compatibility with existing
 CA ACLs, which do not reference any CAs but did (and still do)
 allow access to IPA CA.

 Leaving open for discussion about whether to break compatibility
 for a more consistent behaviour.


Didn't get any feedback in the ticket yet so raising on list for
visibility.  If people agree with current behaviour I can add a
clarification to caacl plugin help text and close out this ticket.


(Sorry for the late reply, I was on vacation the last 2 weeks.)

I would very much prefer if this was consistent with (literally) every other
member list+category attribute, that is, no member and no category means the
rule never matches.

While documenting this as an exception to the above rule is the easy way
out, IMHO adhering to the rule is even better - anyone who touched HBAC or
sudo in IPA would immediately know their way around CA ACLs without having
to read the documentation at all, which is a win, because people don't
generally read documentation until something goes wrong. The current
behavior might surprise them, even if documented properly (it sure surprised
me at first :-).

BTW I think this can be done without breaking compatibility, e.g. by using a
new objectclass to distinguish between "old" (CA is always implicitly the
top-level CA) and "new" (CAs are specified using the member and category
attributes) CA ACLs.

Honza


Is there precedent of "varying behaviour based on objectclass" in
IPA?


Yes, for example in the permission plugin.



Would it go along the lines of...

1. subtype 'ipaCaAcl' object class (let us call it 'ipaCaAclWithCa'
   for sake of discussion).  (Note that moving the 'ipaCa' attribute
   to be part of 'ipaCaAclWithCa' would not be backwards compatible
   with v4.4 as currently released, so it would remain in 'ipaCaAcl'
   objectclass.)

2. Upgrade script to take 'ipaCaAcl' objects that are NOT
   'ipaCaAclWithCa', and make them so, while adding the IPA CA.

Is this what you have in mind?


Yes, something like this, but we can't inherit the new object class from 
ipaCaAcl if we want "new" CA ACLs to be invisible on 4.3 servers.




If so, I don't think there is actually a need to vary the behaviour
based on object class, other than during upgrade.  The reason I was
not doing upgrades in the first place was because I could not in
distinguish between "old" CA ACLs, and "new" CA ACLs that don't have
any CAs defined.  However, adding a new objectclass resolves this
ambiguity.


I'm not sure if this can be handled properly during upgrade only.

One could have a 4.3 server and 4.4 replica, add a CA ACL on the 4.3 
server and then uninstall the server, which would leave the CA ACL not 
upgraded for indeterminate time.


Also we should keep "old" CA ACLs working on 4.3 servers, otherwise we 
would break cert-request on them, and I don't think this can be done 
without changing the object class at runtime.




Lmk what you think; I could be way off track :)

Cheers,
Fraser




--
Jan Cholasta

--
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 0029][Tests] Adding authentication test to trust test suite

2016-07-20 Thread Martin Babinsky

On 07/19/2016 10:41 AM, Lenka Doudova wrote:

Hi,


this patch adds authentication test (specifically "kinit -E
ipauser@IPADOMAIN") to basic trust test suite, as requested by Sumit.

Intended to be applied after my patches 25.4 and 26.3 (already waiting
to be pushed).


Lenka





Hi Lenka,

Code review:

1.) You have several PEP8 transgressions in the patch, please fix them:
"""
$ git show -U0 | pep8 --diff
./ipatests/test_integration/test_trust.py:172:34: E127 continuation line 
over-indented for visual indent
./ipatests/test_integration/test_trust.py:176:35: E128 continuation line 
under-indented for visual indent
./ipatests/test_integration/test_trust.py:180:27: E231 missing 
whitespace after ','

"""

2.)
+
+
+def unlock_principal_password(user, oldpw, newpw, master):
+container_user = "cn=users,cn=accounts"
+basedn = master.domain.basedn
+
+userdn = "uid={},{},{}".format(user, container_user, basedn)
+
+args = [paths.LDAPPASSWD, '-D', userdn, '-w', oldpw, '-a', oldpw,
+'-s', newpw, '-x']
+return run(args)

there is already a function with the same name in other module:

"""
git grep -ni 'def unlock_principal_password' ipatests
ipatests/test_integration/util.py:82:def unlock_principal_password(user, 
oldpw, newpw, master):

ipatests/util.py:676:def unlock_principal_password(user, oldpw, newpw):
"""

Having functions with the same names in different modules makes for poor 
coding practice IMHO. Please rename the function to something like 
"ldappasswd_user" or something like that so that we have a distinction.


Also, you should call ldappasswd directly on master (since you pass it 
as an argument anyway) using "master.run_command(args)". You should 
*not* run any in-test code on the controller unless absolutely necessary.


You can then remove the ipautil.run import from the beginning of the module.

Commit message woes:

1.) vague summary is vague, I would rather see something like:

"""
test that IPA user can kinit using enterprise principal with IPA domain
"""

2.) Commit message body is longer than 78 characters.

3.) there is no ticket URL, I think you can link 
https://fedorahosted.org/freeipa/ticket/6036 or create a new ticket for 
the change.


--
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 0028][Tests] Fix failing user tests

2016-07-20 Thread Martin Babinsky

On 07/15/2016 06:10 PM, Lenka Doudova wrote:

Hi,

here's patch with fix for failing user tests, specifically tests with
renaming users.

Failures were caused by RFE Kerberos principal aliases. As part of the
fix, I had to rewrite few of the tests themselves, since they used
"--setattr" option rather than "--rename" option, which produces
different results.


Lenka





Hi Lenka,

I get nice green *user tests with this patch, so functionally it seems ok.

However, the commit message does not meet our project standards[1]:

1.) The message summary is very vague, I would rather see something more 
specific like:


"""
Tests: improve the handling of rename operations by user tracker
"""

2.) The message itself should be wrapped at the maximum of 78 character 
width (IIRC) but your lines are way too long.


A nice way to automate this in vim is to highlight the text, enter 
command mode and run 'gq', that should format the text for you.


3.) You did not add upstream ticket URL to the end of the message, 
please do so.


There is also an extraneous whitespace here:
"""
 else:
 if type(value) is list:
 self.attrs[key] = value
 else:
 self.attrs[key] = [value]
+
 for key, value in expected_updates.items():
 if value is None or value is '' or value is u'':
 del self.attrs[key]
"""

that has nothing to do with the scope of the patch. Please remove it.

[1] http://www.freeipa.org/page/Contribute/Patch_Format

--
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] 0210 frontend: fix output validation for multiple type choices

2016-07-20 Thread Jan Cholasta

On 20.7.2016 13:15, Martin Babinsky wrote:

On 07/20/2016 12:08 PM, Martin Babinsky wrote:

On 07/19/2016 01:25 PM, Martin Babinsky wrote:

On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, Alexander Bokovoy wrote:
> > > Hi,
> > > > > > I had some time and was blocked by these bugs to do my
tickets so I
> > > actually fixed these three problems that are assigned to Martin
> > > Babinsky. Hopefully, Martin wouldn't be offended by that. :)
> > > > > > --
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin
client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise
type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the
trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to
simply
not supply 'value' field at all but this fix is simpler.


We found out that we still would need to handle
automember-default-group-{set,remove} commands which use the value to
print out summary, so it seems that we cannot win without some more
extensive fixing.



I have botched the wording in the comment, attaching updated patch.




And I have missed adding the new output to `trustconfig-mod` so
attaching another version.


Works for me, ACK.

Pushed to master: f0a61546f552d4df887617167f7dc1378cb95083

--
Jan Cholasta

--
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 frontend: fix output validation for multiple type choices

2016-07-20 Thread Martin Babinsky

On 07/20/2016 12:08 PM, Martin Babinsky wrote:

On 07/19/2016 01:25 PM, Martin Babinsky wrote:

On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, Alexander Bokovoy wrote:
> > > Hi,
> > > > > > I had some time and was blocked by these bugs to do my
tickets so I
> > > actually fixed these three problems that are assigned to Martin
> > > Babinsky. Hopefully, Martin wouldn't be offended by that. :)
> > > > > > --
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin
client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise
type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the
trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to simply
not supply 'value' field at all but this fix is simpler.


We found out that we still would need to handle
automember-default-group-{set,remove} commands which use the value to
print out summary, so it seems that we cannot win without some more
extensive fixing.



I have botched the wording in the comment, attaching updated patch.



And I have missed adding the new output to `trustconfig-mod` so 
attaching another version.


--
Martin^3 Babinsky
From d65b24d92dadb31a3499a654096397151a2fa53b Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 13:18:44 +0200
Subject: [PATCH] allow 'value' output param in commands without primary key

`PrimaryKey` output param works only for API objects that have primary keys,
otherwise it expects None (nothing is associated with this param). Since the
validation of command output was tightened durng thin client effort, some
commands not honoring this contract began to fail output validation.

A custom output was implemented for them to restore their functionality. It
should however be considered as a fix for broken commands and not used
further.

https://fedorahosted.org/freeipa/ticket/6037
https://fedorahosted.org/freeipa/ticket/6061
---
 API.txt | 10 +-
 VERSION |  4 ++--
 ipalib/output.py| 10 ++
 ipaserver/plugins/automember.py |  3 +++
 ipaserver/plugins/trust.py  |  2 ++
 5 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/API.txt b/API.txt
index eb33c1fb7f94f5af45ec0b38fc7e45e484a1044e..535d8ec9a4990395207e2455a09a8c1bdef5529a 100644
--- a/API.txt
+++ b/API.txt
@@ -144,7 +144,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_set/1
 args: 0,6,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -155,7 +155,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_show/1
 args: 0,4,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -164,7 +164,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_del/1
 args: 1,2,3
 arg: Str('cn+', cli_name='automember_rule')
@@ -5574,7 +5574,7 @@ option: StrEnum('trust_type', autofill=True, 

Re: [Freeipa-devel] [PATCH] 0010 Show full error message for selinuxusermap-add-hostgroup

2016-07-20 Thread Jan Cholasta

On 20.7.2016 11:46, Florence Blanc-Renaud wrote:

On 07/20/2016 10:50 AM, Jan Cholasta wrote:

On 20.7.2016 10:26, Florence Blanc-Renaud wrote:

On 07/18/2016 02:52 PM, Florence Blanc-Renaud wrote:

On 07/18/2016 08:20 AM, Jan Cholasta wrote:

Hi,

On 7.7.2016 16:40, Florence Blanc-Renaud wrote:

On 07/07/2016 01:23 PM, Petr Vobornik wrote:

On 07/05/2016 02:38 PM, Florence Blanc-Renaud wrote:

Hi,

the output of ipa selinuxusermap-add-hostgroup and
selinuxusermap-add-user does not display any more the host/host
group or
user/group that could not be added. This patch fixes this
regression by
adding the labels host/hostgroup/user/group to the list of
_failed_member_output_params of the class ClientMethod.


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



I've a feeling that this issue is more general and multiple commands
regressed. Would be good to check other member options, e.g. also in
user plugin.


Hi Petr,

you are right, a lot of other commands regressed. So far I checked
only
user and sudocmd but it is likely to be a long task. Are there
regression tests that could help me make sure that the fix is
exhaustive?

Flo


See attachment for a patch with an universal fix.

Honza


Hi Honza,

the patch fixes most of the issues. I still see some CLI that do not
print everything (while they used to before the regression):
ipa servicedelegationrule-add-member
ipa servicedelegationrule-remove-member
ipa servicedelegationtarget-add-member
ipa servicedelegationtarget-remove-member

And the following CLI do not print the failed members (but they never
did):
ipa automember-add-condition
ipa automember-remove-condition
ipa sudorule-add-allow-command
ipa sudorule-remove-allow-command
ipa sudorule-add-deny-command
ipa sudorule-remove-deny-command

It is probably ok to commit this patch and investigate in another
ticket
the remaining issues,
Flo.



Hi,
please find a new version of the patch, thanks to Jan's help. This
version also fixes servicedelegation commands.


I would rather keep the patches separate, as the fixes are different.
Otherwise LGTM.


Hi Honza,

please find an updated version which handles only servicedelegation
issue (I also attached your original patch for reference).
For the reviewers: both have to be applied to completely fix the ticket.
Flo.


Thanks, ACK.

Pushed to master: 90704df59dbe996ef1db58d7a11f826c008d08a3

--
Jan Cholasta

--
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] Text-based rules for CSR autogeneration using Jinja2

2016-07-20 Thread Simo Sorce
On Tue, 2016-07-19 at 16:20 -0400, Ben Lipton wrote:
> Hi,
> 
> I have updated the design page 
> http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generati
> on/Mapping_Rules 
> with my plan for implementing user-configurable rules for mapping
> IPA 
> data into certificate requests. In brief: we will use Jinja2 for 
> templating. Data rules (which map individual data items) and syntax 
> rules (which group them into certificate fields) will both be
> snippets 
> of Jinja2 markup. The formatting process will be as follows:
> 1. Syntax rules will be rendered using Jinja2. Data rules (rule
> text, 
> not rendered) will be passed as the datarules attribute.
> 2. Rendered syntax rules will be processed by the Formatter class
> for 
> the selected CSR generation helper (e.g. openssl or certutil). The 
> formatter combines these partial rules into a full template for the
> config.
> 3. The template will be rendered using Jinja2. Relevant data from
> the 
> IPA database will be available in the context for this rendering.
> 4. The final rendered template will be returned to the caller,
> labeled 
> with its function (e.g. a command line or a config file).
> 
> Are there any comments or objections to this approach? Here's an
> example 
> to show what it might look like in practice.
> 
> Example data rules:
> email={{subject.email}}
> O={{config.ipacertificatesubjectbase}}\nCN={{subject.username}}
> 
> Example syntax rule:
> subjectAltName=@{% section %}{{datarules|join('\n')}}{% endsection %}
> 
> Example composed config template:
> [ req ]
> prompt = no
> encrypt_key = no
> 
> distinguished_name = {% section
> %}O={{config.ipacertificatesubjectbase}}
> CN={{subject.username}}{% endsection %}
> 
> req_extensions = exts
> 
> [ exts ]
> subjectAltName=@{% section %}email={{subject.email}}{% endsection %}
> 
> There's a lot more information about the thinking behind this at 
> http://blog.benjaminlipton.com/2016/07/19/csr-generation-templating.h
> tml 
> if you're interested, as well.

Nice work Ben,
it's been really nice to be able to follow your notes on the blog post,
one question remains lingering in my head, why jinja2 ?
I know that engine relatively well as I used it in ipsilon, so I am not
questioning the choice just asking why specifically jinja2 and not
something else, potentially language agnostic.

Simo.

-- 
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: Improve on #2795 patches

2016-07-20 Thread Simo Sorce
Attached patch introduces a helper function and avoids the questionable
replace+delete operations where possible (still employed in the
entry_to_mods function).
Compiles and I am about to test it, but I'd like feedback on it if
anyone wants to take a look.

Simo.From fec7ed2d2d7d8352d1a6a9cf5607476c9fd5d65f Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Tue, 19 Jul 2016 07:43:50 -0400
Subject: [PATCH] Simplify date manipulation in pwd plugin

Use a helper function to perform operations on dates in LDAP attributes.

Related to #2795

Signed-off-by: Simo Sorce 
---
 daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c  | 66 +--
 daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h  |  2 +
 daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c | 31 ---
 3 files changed, 50 insertions(+), 49 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
index 0bb50fc319e2b2520d36534d369ad42f95c80c8e..cab7b7c7bf0816de736cceaa9a8067920b770a2e 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/common.c
@@ -702,6 +702,33 @@ next:
 return kvno;
 }
 
+int ipapwd_setdate(Slapi_Entry *source, Slapi_Mods *smods, const char *attr,
+   time_t date, bool remove)
+{
+char timestr[GENERALIZED_TIME_LENGTH+1];
+struct tm utctime;
+Slapi_Attr *t;
+bool exists;
+
+exists = (slapi_entry_attr_find(source, attr, ) == 0);
+
+if (remove) {
+if (exists) {
+ slapi_mods_add_mod_values(smods, LDAP_MOD_DELETE, attr, NULL);
+}
+return LDAP_SUCCESS;
+}
+
+if (!gmtime_r(, )) {
+LOG_FATAL("failed to convert %s date\n", attr);
+return LDAP_OPERATIONS_ERROR;
+}
+strftime(timestr, GENERALIZED_TIME_LENGTH + 1, "%Y%m%d%H%M%SZ", );
+slapi_mods_add_string(smods, exists ?  LDAP_MOD_REPLACE : LDAP_MOD_ADD,
+  attr, timestr);
+return LDAP_SUCCESS;
+}
+
 /* Modify the Password attributes of the entry */
 int ipapwd_SetPassword(struct ipapwd_krbcfg *krbcfg,
struct ipapwd_data *data, int is_krb)
@@ -711,8 +738,6 @@ int ipapwd_SetPassword(struct ipapwd_krbcfg *krbcfg,
 Slapi_Value **svals = NULL;
 Slapi_Value **ntvals = NULL;
 Slapi_Value **pwvals = NULL;
-struct tm utctime;
-char timestr[GENERALIZED_TIME_LENGTH+1];
 char *nt = NULL;
 int is_smb = 0;
 int is_ipant = 0;
@@ -764,34 +789,19 @@ int ipapwd_SetPassword(struct ipapwd_krbcfg *krbcfg,
 		 * keytab so don't set it on hosts.
 		 */
 if (!is_host) {
-	/* change Last Password Change field with the current date */
-			if (!gmtime_r(&(data->timeNow), )) {
-LOG_FATAL("failed to retrieve current date (buggy gmtime_r ?)\n");
-ret = LDAP_OPERATIONS_ERROR;
-goto free_and_return;
-			}
-			strftime(timestr, GENERALIZED_TIME_LENGTH + 1,
- "%Y%m%d%H%M%SZ", );
-			slapi_mods_add_string(smods, LDAP_MOD_REPLACE,
-  "krbLastPwdChange", timestr);
+	/* change Last Password Change field with the current date */
+ret = ipapwd_setdate(data->target, smods, "krbLastPwdChange",
+ data->timeNow, false);
+if (ret != LDAP_SUCCESS)
+goto free_and_return;
 
-			/* set Password Expiration date */
-			if (!gmtime_r(&(data->expireTime), )) {
-LOG_FATAL("failed to convert expiration date\n");
-ret = LDAP_OPERATIONS_ERROR;
-goto free_and_return;
-			}
-			strftime(timestr, GENERALIZED_TIME_LENGTH + 1,
- "%Y%m%d%H%M%SZ", );
-			slapi_mods_add_string(smods, LDAP_MOD_REPLACE,
-  "krbPasswordExpiration", timestr);
-			if (data->expireTime == 0) {
-			slapi_mods_add_string(smods, LDAP_MOD_DELETE,
-			  "krbPasswordExpiration", timestr);
-			}
-
-		}
+	/* set Password Expiration date */
+ret = ipapwd_setdate(data->target, smods, "krbPasswordExpiration",
+ data->expireTime, (data->expireTime == 0));
+if (ret != LDAP_SUCCESS)
+goto free_and_return;
 	}
+}
 
 if (nt && is_smb) {
 slapi_mods_add_string(smods, LDAP_MOD_REPLACE,
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h
index 83c0222635ece033a37b3540201ae674b5191257..e96aa44d2fb19251c43d8a981dea5f8441007c6a 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h
@@ -119,6 +119,8 @@ int ipapwd_gen_checks(Slapi_PBlock *pb, char **errMesg,
 int ipapwd_CheckPolicy(struct ipapwd_data *data);
 int ipapwd_getEntry(const char *dn, Slapi_Entry **e2, char **attrlist);
 int ipapwd_get_cur_kvno(Slapi_Entry *target);
+int ipapwd_setdate(Slapi_Entry *source, Slapi_Mods *smods, const char *attr,

Re: [Freeipa-devel] [PATCH 190] expose `--secret` option in radiusproxy-* commands

2016-07-20 Thread Martin Babinsky

On 07/19/2016 12:32 PM, Jan Cholasta wrote:

Hi,

On 18.7.2016 13:51, Martin Babinsky wrote:

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


I don't think we want the secret searchable. Add a 'no_search' flag to
the param to fix that.

Honza



'no_search' flag breaks the API backwards compatibility, so I am sending 
another two patches which fix handling of deprecated options in the 
framework and deprecate `--secret` in radiusproxy-find command.


I hope this solution is the best.

--
Martin^3 Babinsky
From 645b7ece72e902c9b108d41a5e71d7e88a48720f Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 10:45:48 +0200
Subject: [PATCH] expose `--secret` option in radiusproxy-* commands

Option `--secret` was hidden from radiusproxy CLI preventing setting a secret
on existing server or searching by secret. Since thin client implementation it
was also not recognized by the interactive prompt code in CLI frontend since
it never got there.

https://fedorahosted.org/freeipa/ticket/6078
---
 ipaserver/plugins/radiusproxy.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ipaserver/plugins/radiusproxy.py b/ipaserver/plugins/radiusproxy.py
index 44d87b9ae1337278bb6237d471f64693b0eac3db..5657e002c1ce66335b7697b98f95a49207c61d87 100644
--- a/ipaserver/plugins/radiusproxy.py
+++ b/ipaserver/plugins/radiusproxy.py
@@ -126,7 +126,6 @@ class radiusproxy(LDAPObject):
 label=_('Secret'),
 doc=_('The secret used to encrypt data'),
 confirm=True,
-flags=['no_option'],
 ),
 Int('ipatokenradiustimeout?',
 cli_name='timeout',
-- 
2.7.4

From 4e3c8077f1d8bc8c4467ccbcd4d6c9d0f4631c46 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Tue, 19 Jul 2016 17:05:32 +0200
Subject: [PATCH] raise ValidationError when deprecated param is passed to
 command

https://fedorahosted.org/freeipa/ticket/6078
---
 ipalib/parameters.py | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index 1581b7dcac5259e5c4a127e2a38e13335002b204..5c3d7705a004f77614a754e1ecdcf4f6ca386eaf 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -852,6 +852,9 @@ class Param(ReadOnly):
 if self.required or (supplied and 'nonempty' in self.flags):
 raise RequirementError(name=self.name)
 return
+if self.deprecated:
+raise ValidationError(name=self.get_param_name(),
+  error=_('this option is deprecated'))
 if self.multivalue:
 if type(value) is not tuple:
 raise TypeError(
@@ -874,10 +877,6 @@ class Param(ReadOnly):
 if error is not None:
 raise ValidationError(name=self.get_param_name(), error=error)
 
-def _rule_deprecated(self, _, value):
-if self.deprecated:
-return _('this option is deprecated')
-
 def get_default(self, **kw):
 """
 Return the static default or construct and return a dynamic default.
-- 
2.7.4

From 631281ec6e50090cf819eda1f131c8e0b0011d7f Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Tue, 19 Jul 2016 13:04:05 +0200
Subject: [PATCH] prevent search for RADIUS proxy servers by secret

radiusproxy-find should not allow search by proxy secret even for privileged
users. Deprecate this option so that it is not shown in command's help and is
not allowed to be specified as parameter.

https://fedorahosted.org/freeipa/ticket/6078
---
 API.txt  | 2 +-
 VERSION  | 4 ++--
 ipaserver/plugins/radiusproxy.py | 8 
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/API.txt b/API.txt
index cbe23f4bde3a29cf3f28a9e361f83e176ede08e0..e5a5cc2ae07d2c44df31934dda857d63f6b90f1e 100644
--- a/API.txt
+++ b/API.txt
@@ -3818,7 +3818,7 @@ option: Flag('all', autofill=True, cli_name='all', default=False)
 option: Str('cn?', autofill=False, cli_name='name')
 option: Str('description?', autofill=False, cli_name='desc')
 option: Int('ipatokenradiusretries?', autofill=False, cli_name='retries')
-option: Password('ipatokenradiussecret?', autofill=False, cli_name='secret', confirm=True)
+option: Password('ipatokenradiussecret?', autofill=False, cli_name='secret', confirm=True, deprecated=True)
 option: Str('ipatokenradiusserver*', autofill=False, cli_name='server')
 option: Int('ipatokenradiustimeout?', autofill=False, cli_name='timeout')
 option: Str('ipatokenusermapattribute?', autofill=False, cli_name='userattr')
diff --git a/VERSION b/VERSION
index ca489965050f32d2d8987dfd251ec2b2a0ba1768..401b7d92839496f1b7bf97bf61475d53fd8e77df 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2

Re: [Freeipa-devel] [PATCH] 0210 frontend: fix output validation for multiple type choices

2016-07-20 Thread Martin Babinsky

On 07/19/2016 01:25 PM, Martin Babinsky wrote:

On 07/19/2016 01:13 PM, Alexander Bokovoy wrote:

On Mon, 18 Jul 2016, Martin Babinsky wrote:

On 07/18/2016 12:29 PM, Martin Babinsky wrote:
> On 07/18/2016 10:01 AM, Jan Cholasta wrote:
> > Hi,
> > > > On 16.7.2016 12:46, Alexander Bokovoy wrote:
> > > Hi,
> > > > > > I had some time and was blocked by these bugs to do my
tickets so I
> > > actually fixed these three problems that are assigned to Martin
> > > Babinsky. Hopefully, Martin wouldn't be offended by that. :)
> > > > > > --
> > > > > > Output entry elements may have multiple types allowed. We
need to check
> > > all of them to properly validate the output. Right now, thin
client
> > > receives type specifications for elements as tuples of types, so
> > > what is seen as 'None' on the server side becomes (type(None),)
tuple
> > > on the thin client side.
> > > > > > Change validation to account this by processing each
separate type
> > > of the element and account for both None and type(None). Raise
type
> > > error only if all of the type checks failed.
> > > > > > https://fedorahosted.org/freeipa/ticket/6061
> > > > NACK, this only hides the real issue, which is that
trustconfig-show
> > (and automember-set-default in #6037) claims to return the primary
key
> > of the object in the 'value' output field, but the object does not
have
> > a primary key, so the client rightfully expects None.
> > > > A proper fix would be to set "has_output =
output.simple_value" for
> > these commands (all of automember_default_group_{set,remove,show},
> > trustconfig_{mod,show}).
> > > > Honza
> > > > The problem is that these commands do not return a simple
boolean in
> 'result' but a full entry dict, so 'simple_value' won't do the
trick in
> this case.
> > But I agree, we should rather fix misbehaving commands rather than
bend
> the framework to accomodate their idiosyncracies, we have enough of
that
> already.
> I am attaching a patch that adds a custom shim for misbehaving
commands so that they work as before. There is also a big fat warning
added to discourage implementation of such commands.

Let's go by this patch. I originally thought changing trust.py to simply
not supply 'value' field at all but this fix is simpler.


We found out that we still would need to handle
automember-default-group-{set,remove} commands which use the value to
print out summary, so it seems that we cannot win without some more
extensive fixing.



I have botched the wording in the comment, attaching updated patch.

--
Martin^3 Babinsky
From 5b45004ffac840aedec989728321afeeeff15ff6 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 18 Jul 2016 13:18:44 +0200
Subject: [PATCH] allow 'value' output param in commands without primary key

`PrimaryKey` output param works only for API objects that have primary keys,
otherwise it expects None (nothing is associated with this param). Since the
validation of command output was tightened durng thin client effort, some
commands not honoring this contract began to fail output validation.

A custom output was implemented for them to restore their functionality. It
should however be considered as a fix for broken commands and not used
further.

https://fedorahosted.org/freeipa/ticket/6037
https://fedorahosted.org/freeipa/ticket/6061
---
 API.txt |  8 
 VERSION |  4 ++--
 ipalib/output.py| 10 ++
 ipaserver/plugins/automember.py |  3 +++
 ipaserver/plugins/trust.py  |  1 +
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/API.txt b/API.txt
index eb33c1fb7f94f5af45ec0b38fc7e45e484a1044e..cbe23f4bde3a29cf3f28a9e361f83e176ede08e0 100644
--- a/API.txt
+++ b/API.txt
@@ -144,7 +144,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_set/1
 args: 0,6,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -155,7 +155,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_default_group_show/1
 args: 0,4,3
 option: Flag('all', autofill=True, cli_name='all', default=False)
@@ -164,7 +164,7 @@ option: StrEnum('type', values=[u'group', u'hostgroup'])
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: PrimaryKey('value')
+output: Output('value', type=[])
 command: automember_del/1
 args: 1,2,3
 arg: Str('cn+', cli_name='automember_rule')
@@ -5584,7 +5584,7 @@ option: StrEnum('trust_type', autofill=True, cli_name='type', default=u'ad', val
 option: Str('version?')
 output: Entry('result')
 output: Output('summary', type=[, ])
-output: 

Re: [Freeipa-devel] [PATCH] 0010 Show full error message for selinuxusermap-add-hostgroup

2016-07-20 Thread Florence Blanc-Renaud

On 07/20/2016 10:50 AM, Jan Cholasta wrote:

On 20.7.2016 10:26, Florence Blanc-Renaud wrote:

On 07/18/2016 02:52 PM, Florence Blanc-Renaud wrote:

On 07/18/2016 08:20 AM, Jan Cholasta wrote:

Hi,

On 7.7.2016 16:40, Florence Blanc-Renaud wrote:

On 07/07/2016 01:23 PM, Petr Vobornik wrote:

On 07/05/2016 02:38 PM, Florence Blanc-Renaud wrote:

Hi,

the output of ipa selinuxusermap-add-hostgroup and
selinuxusermap-add-user does not display any more the host/host
group or
user/group that could not be added. This patch fixes this
regression by
adding the labels host/hostgroup/user/group to the list of
_failed_member_output_params of the class ClientMethod.


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



I've a feeling that this issue is more general and multiple commands
regressed. Would be good to check other member options, e.g. also in
user plugin.


Hi Petr,

you are right, a lot of other commands regressed. So far I checked
only
user and sudocmd but it is likely to be a long task. Are there
regression tests that could help me make sure that the fix is
exhaustive?

Flo


See attachment for a patch with an universal fix.

Honza


Hi Honza,

the patch fixes most of the issues. I still see some CLI that do not
print everything (while they used to before the regression):
ipa servicedelegationrule-add-member
ipa servicedelegationrule-remove-member
ipa servicedelegationtarget-add-member
ipa servicedelegationtarget-remove-member

And the following CLI do not print the failed members (but they never
did):
ipa automember-add-condition
ipa automember-remove-condition
ipa sudorule-add-allow-command
ipa sudorule-remove-allow-command
ipa sudorule-add-deny-command
ipa sudorule-remove-deny-command

It is probably ok to commit this patch and investigate in another ticket
the remaining issues,
Flo.



Hi,
please find a new version of the patch, thanks to Jan's help. This
version also fixes servicedelegation commands.


I would rather keep the patches separate, as the fixes are different.
Otherwise LGTM.


Hi Honza,

please find an updated version which handles only servicedelegation 
issue (I also attached your original patch for reference).

For the reviewers: both have to be applied to completely fix the ticket.
Flo.
>From 2fc614eac6465af6d55580c51600d287bf1160f5 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud 
Date: Wed, 20 Jul 2016 11:02:30 +0200
Subject: [PATCH] Show full error message for selinuxusermap-add-hostgroup

While investigating the issue for selinuxusermap-add-hostgroup,
we discovered that other commands were missing output.
A first patch fixes most of the issues:
freeipa-jcholast-677-frontend-copy-command-arguments-to-output-params-on-.patch

This patch fixes servicedelegation CLI, where
servicedelegation.takes_params was missing
ipaallowedtarget_servicedelegationtarget, ipaallowedtoimpersonate and
memberprincipal

https://fedorahosted.org/freeipa/ticket/6026
---
 ipaserver/plugins/servicedelegation.py | 53 ++
 1 file changed, 15 insertions(+), 38 deletions(-)

diff --git a/ipaserver/plugins/servicedelegation.py b/ipaserver/plugins/servicedelegation.py
index 958c3b739a2dd465c2b685672c3deb1af8c36e4e..6f38c36a30363755c80081d02bf4c86d829eae34 100644
--- a/ipaserver/plugins/servicedelegation.py
+++ b/ipaserver/plugins/servicedelegation.py
@@ -96,30 +96,6 @@ PROTECTED_CONSTRAINT_TARGETS = (
 )
 
 
-output_params = (
-Str(
-'ipaallowedtarget_servicedelegationtarget',
-label=_('Allowed Target'),
-),
-Str(
-'ipaallowedtoimpersonate',
-label=_('Allowed to Impersonate'),
-),
-Str(
-'memberprincipal',
-label=_('Member principals'),
-),
-Str(
-'failed_memberprincipal',
-label=_('Failed members'),
-),
-Str(
-'ipaallowedtarget',
-label=_('Failed targets'),
-),
-)
-
-
 class servicedelegation(LDAPObject):
 """
 Service Constrained Delegation base object.
@@ -175,6 +151,21 @@ class servicedelegation(LDAPObject):
 label=_('Delegation name'),
 primary_key=True,
 ),
+Str(
+'ipaallowedtarget_servicedelegationtarget',
+label=_('Allowed Target'),
+flags={'virtual_attribute', 'no_create', 'no_update', 'no_search'},
+),
+Str(
+'ipaallowedtoimpersonate',
+label=_('Allowed to Impersonate'),
+flags={'no_create', 'no_update', 'no_search'},
+),
+Str(
+'memberprincipal',
+label=_('Member principals'),
+flags={'no_create', 'no_update', 'no_search'},
+),
 )
 
 
@@ -186,8 +177,6 @@ class servicedelegation_add_member(LDAPAddMember):
 principal_attr = 'memberprincipal'
 principal_failedattr = 'failed_memberprincipal'
 
-has_output_params = LDAPAddMember.has_output_params + output_params
-
 def get_options(self):
 for option in 

Re: [Freeipa-devel] [PATCH] 0010 Show full error message for selinuxusermap-add-hostgroup

2016-07-20 Thread Jan Cholasta

On 20.7.2016 10:26, Florence Blanc-Renaud wrote:

On 07/18/2016 02:52 PM, Florence Blanc-Renaud wrote:

On 07/18/2016 08:20 AM, Jan Cholasta wrote:

Hi,

On 7.7.2016 16:40, Florence Blanc-Renaud wrote:

On 07/07/2016 01:23 PM, Petr Vobornik wrote:

On 07/05/2016 02:38 PM, Florence Blanc-Renaud wrote:

Hi,

the output of ipa selinuxusermap-add-hostgroup and
selinuxusermap-add-user does not display any more the host/host
group or
user/group that could not be added. This patch fixes this
regression by
adding the labels host/hostgroup/user/group to the list of
_failed_member_output_params of the class ClientMethod.


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



I've a feeling that this issue is more general and multiple commands
regressed. Would be good to check other member options, e.g. also in
user plugin.


Hi Petr,

you are right, a lot of other commands regressed. So far I checked only
user and sudocmd but it is likely to be a long task. Are there
regression tests that could help me make sure that the fix is
exhaustive?

Flo


See attachment for a patch with an universal fix.

Honza


Hi Honza,

the patch fixes most of the issues. I still see some CLI that do not
print everything (while they used to before the regression):
ipa servicedelegationrule-add-member
ipa servicedelegationrule-remove-member
ipa servicedelegationtarget-add-member
ipa servicedelegationtarget-remove-member

And the following CLI do not print the failed members (but they never
did):
ipa automember-add-condition
ipa automember-remove-condition
ipa sudorule-add-allow-command
ipa sudorule-remove-allow-command
ipa sudorule-add-deny-command
ipa sudorule-remove-deny-command

It is probably ok to commit this patch and investigate in another ticket
the remaining issues,
Flo.



Hi,
please find a new version of the patch, thanks to Jan's help. This
version also fixes servicedelegation commands.


I would rather keep the patches separate, as the fixes are different. 
Otherwise LGTM.


--
Jan Cholasta

--
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] 0010 Show full error message for selinuxusermap-add-hostgroup

2016-07-20 Thread Florence Blanc-Renaud

On 07/18/2016 02:52 PM, Florence Blanc-Renaud wrote:

On 07/18/2016 08:20 AM, Jan Cholasta wrote:

Hi,

On 7.7.2016 16:40, Florence Blanc-Renaud wrote:

On 07/07/2016 01:23 PM, Petr Vobornik wrote:

On 07/05/2016 02:38 PM, Florence Blanc-Renaud wrote:

Hi,

the output of ipa selinuxusermap-add-hostgroup and
selinuxusermap-add-user does not display any more the host/host
group or
user/group that could not be added. This patch fixes this
regression by
adding the labels host/hostgroup/user/group to the list of
_failed_member_output_params of the class ClientMethod.


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



I've a feeling that this issue is more general and multiple commands
regressed. Would be good to check other member options, e.g. also in
user plugin.


Hi Petr,

you are right, a lot of other commands regressed. So far I checked only
user and sudocmd but it is likely to be a long task. Are there
regression tests that could help me make sure that the fix is
exhaustive?

Flo


See attachment for a patch with an universal fix.

Honza


Hi Honza,

the patch fixes most of the issues. I still see some CLI that do not
print everything (while they used to before the regression):
ipa servicedelegationrule-add-member
ipa servicedelegationrule-remove-member
ipa servicedelegationtarget-add-member
ipa servicedelegationtarget-remove-member

And the following CLI do not print the failed members (but they never did):
ipa automember-add-condition
ipa automember-remove-condition
ipa sudorule-add-allow-command
ipa sudorule-remove-allow-command
ipa sudorule-add-deny-command
ipa sudorule-remove-deny-command

It is probably ok to commit this patch and investigate in another ticket
the remaining issues,
Flo.



Hi,
please find a new version of the patch, thanks to Jan's help. This 
version also fixes servicedelegation commands.

Flo.

>From efcc93dc2ebded7d3cae7622b44201d786d13573 Mon Sep 17 00:00:00 2001
From: Florence Blanc-Renaud 
Date: Tue, 19 Jul 2016 18:49:57 +0200
Subject: [PATCH] Show full error message for selinuxusermap-add-hostgroup

Incorporate Jcholast fix + fix for servicedelegation CLI:

1/ In commit f554078291d682d59956998af97f7d3066fbe7e7 we stopped copying
command arguments to output params in order to remove redundancies and
reduce API schema in size. Since then, output params were removed from
API schema completely and are reconstructed on the client.

Not including arguments in output params hides failed members from member
commands' CLI output. To fix this, copy arguments to output params again,
but only on the client side.

2/ servicedelegation.takes_params was missing
ipaallowedtarget_servicedelegationtarget, ipaallowedtoimpersonate and
memberprincipal

https://fedorahosted.org/freeipa/ticket/6026
---
 ipaclient/frontend.py  |  4 +++
 ipaserver/plugins/servicedelegation.py | 53 ++
 2 files changed, 19 insertions(+), 38 deletions(-)

diff --git a/ipaclient/frontend.py b/ipaclient/frontend.py
index e8eacc068f4bec5ccdb21228b32a88aea24424df..1525c88b3dfeadccd8115cb4b6ba149caef22103 100644
--- a/ipaclient/frontend.py
+++ b/ipaclient/frontend.py
@@ -95,6 +95,10 @@ class ClientMethod(ClientCommand, Method):
 
 def get_output_params(self):
 seen = set()
+for param in self.params():
+if param.name not in self.obj.params:
+seen.add(param.name)
+yield param
 for output_param in super(ClientMethod, self).get_output_params():
 seen.add(output_param.name)
 yield output_param
diff --git a/ipaserver/plugins/servicedelegation.py b/ipaserver/plugins/servicedelegation.py
index 958c3b739a2dd465c2b685672c3deb1af8c36e4e..6f38c36a30363755c80081d02bf4c86d829eae34 100644
--- a/ipaserver/plugins/servicedelegation.py
+++ b/ipaserver/plugins/servicedelegation.py
@@ -96,30 +96,6 @@ PROTECTED_CONSTRAINT_TARGETS = (
 )
 
 
-output_params = (
-Str(
-'ipaallowedtarget_servicedelegationtarget',
-label=_('Allowed Target'),
-),
-Str(
-'ipaallowedtoimpersonate',
-label=_('Allowed to Impersonate'),
-),
-Str(
-'memberprincipal',
-label=_('Member principals'),
-),
-Str(
-'failed_memberprincipal',
-label=_('Failed members'),
-),
-Str(
-'ipaallowedtarget',
-label=_('Failed targets'),
-),
-)
-
-
 class servicedelegation(LDAPObject):
 """
 Service Constrained Delegation base object.
@@ -175,6 +151,21 @@ class servicedelegation(LDAPObject):
 label=_('Delegation name'),
 primary_key=True,
 ),
+Str(
+'ipaallowedtarget_servicedelegationtarget',
+label=_('Allowed Target'),
+flags={'virtual_attribute', 'no_create', 'no_update', 'no_search'},
+),
+Str(
+'ipaallowedtoimpersonate',
+label=_('Allowed to Impersonate'),
+flags={'no_create', 

Re: [Freeipa-devel] [Design Review Request] V4/Automatic_Certificate_Request_Generation

2016-07-20 Thread Jan Cholasta

Hi,

On 17.6.2016 00:06, Ben Lipton wrote:

On 06/14/2016 08:27 AM, Ben Lipton wrote:

Hello all,

I have written up a design proposal for making certificate requests
easier to generate when using alternate certificate profiles:
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation.
The use case for this is described in
https://fedorahosted.org/freeipa/ticket/4899. I will be working on
implementing this design over the next couple of months. If you have
the time and interest, please take a look and share any comments or
concerns that you have.

Thanks!

Ben


Just a quick update to say that I've created a new document that covers
the proposed schema additions in a more descriptive way (with diagrams!)
I'm very new to developing with LDAP, so some more experienced eyes on
the proposal would be very helpful, even if you don't have time to
absorb the full design. Please take a look at
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation/Schema
if you have a chance.


I finally had a chance to take a look at this, here are some comments:

1) I don't like how transformation rules are tied to a particular helper 
and have to be duplicated for each of them. They should be generic and 
work with any helper, as helpers are just an implementation detail and 
their resulting data is the same.


In fact, I think I would prefer if the CSR was generated using 
python-cryptography's CertificateSigningRequestBuilder [1] rather than 
openssl or certutil or any other command line tool.



2) The schema seems unnecessarily complex. I think all we need is a 
single new multi-value attribute in certprofile. For your S/MIME 
example, it could be something like:


attr: subjectname=CN={subject.cn},{subject_base}
attr: san_rfc822name={subject.email}
attr: san_directoryname={subject.dn}


Honza

[1] 



--
Jan Cholasta

--
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