Re: [Freeipa-devel] DN patch and documentation
On 07/11/2012 05:24 PM, Alexander Bokovoy wrote:
On Wed, 11 Jul 2012, Petr Viktorin wrote:
On 07/07/2012 08:45 PM, John Dennis wrote:
The DN work I was doing on master is ready for review and testing. It's
been a long haul and I've been working relentlessly to get this work
completed. I am on PTO for a week starting today (I know bad timing) but
I spent yesterday and my first day of PTO today writing up extensive
documentation for the work so others can start taking a look at it while
I'm gone. The documentation as well as where to find the code can be
found here:
http://jdennis.fedorapeople.org/dn_summary.html
The document is long but I felt it was better to provide explanations
for as much as possible.
I may check in during the week but I'm going to try and discipline
myself not to and take an actual much needed break.
John
Two more code review points:
ipa-adtrust-install uses DN without importing it, that'll fail
Where?
$ git grep DN ipaserver/install/adtrustinstance.py|grep import
ipaserver/install/adtrustinstance.py:from ipalib.dn import DN
This is in master.
In John's dn branch, install/tools/ipa-adtrust-install.
$ git grep -w DN install/tools/ipa-adtrust-install
install/tools/ipa-adtrust-install:209:
api.Backend.ldap2.connect(bind_dn=DN(('cn', 'Directory Manager')),
bind_pw=smb.dm_password)
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] DN patch and documentation
On Tue, 17 Jul 2012, Petr Viktorin wrote:
On 07/11/2012 05:24 PM, Alexander Bokovoy wrote:
On Wed, 11 Jul 2012, Petr Viktorin wrote:
On 07/07/2012 08:45 PM, John Dennis wrote:
The DN work I was doing on master is ready for review and testing. It's
been a long haul and I've been working relentlessly to get this work
completed. I am on PTO for a week starting today (I know bad timing) but
I spent yesterday and my first day of PTO today writing up extensive
documentation for the work so others can start taking a look at it while
I'm gone. The documentation as well as where to find the code can be
found here:
http://jdennis.fedorapeople.org/dn_summary.html
The document is long but I felt it was better to provide explanations
for as much as possible.
I may check in during the week but I'm going to try and discipline
myself not to and take an actual much needed break.
John
Two more code review points:
ipa-adtrust-install uses DN without importing it, that'll fail
Where?
$ git grep DN ipaserver/install/adtrustinstance.py|grep import
ipaserver/install/adtrustinstance.py:from ipalib.dn import DN
This is in master.
In John's dn branch, install/tools/ipa-adtrust-install.
$ git grep -w DN install/tools/ipa-adtrust-install
install/tools/ipa-adtrust-install:209:
api.Backend.ldap2.connect(bind_dn=DN(('cn', 'Directory Manager')),
bind_pw=smb.dm_password)
It needs rebasing then. Also after my patch 0060 this particular line is
not there anymore.
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 170 Differentiation of widget type and text_widget input type
On 07/17/2012 01:09 AM, Endi Sukma Dewata wrote: On 7/12/2012 8:10 AM, Petr Vobornik wrote: There was a clash of 'type' attribute in widget's spec. Usually 'type' is used for telling a builder which field and widget to build. Text widget used this attribute also for definion of html input type. It was problematic for some special widgets, which defined own field and used text_widget, like service_type or dnszone_name. In those and possibly other cases it used widget type for specifying input type which lead to execution error in Internet Explorer. Firefox and Chrome took it. This patch is changing text_widget's 'type' to 'input_type' which removes the collision and hence fixes the problem. https://fedorahosted.org/freeipa/ticket/2806 and half of: https://fedorahosted.org/freeipa/ticket/2834 ACK. Pushed to master. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 171 Fixed display of attributes_widget in IE9
On 07/17/2012 01:10 AM, Endi Sukma Dewata wrote: On 7/16/2012 3:56 AM, Petr Vobornik wrote: Attributes widget is using overflow css rule in tbody element. IE9 doesn't handle it well. To fix the issue, attributes widget was slightly modified and conditional css stylesheet was added just for fixing IE problems. https://fedorahosted.org/freeipa/ticket/2822 ACK. Pushed to master. Separate issue, on IE the main navigation tabs aren't rendered nicely. There is a dark line underneath the label (Identity, Policy, IPA Server) when the tab is inactive. If you put the mouse over the tab the dark line will disappear. Ticket https://fedorahosted.org/freeipa/ticket/2817 needs update of jquery-ui to be fixed. We have some overrides of jquery-ui styles which are not displayed in IE nicely. I'll regenerate jquery-ui (lib and css) with our greenish style. Then I'll remove our overrides and fix the 'dark line problem' with it. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 172 Bigger textarea for permission type=subtree
On 07/17/2012 01:10 AM, Endi Sukma Dewata wrote:
On 7/16/2012 6:53 AM, Petr Vobornik wrote:
Patch description:
Adder dialog and details facet for permission type=subtree have small
textarea for defining subtree filter. It was unconfortable to define the
filter. This difference was removed.
https://fedorahosted.org/freeipa/ticket/2832
Note regarding related ticket:
Resizable textareas are browser-specific. We can do it in code too but I
don't think it is worth the effort.
Textarea in IE still seems smaller than in Firefox, but it has the same
number of rows and cols. I think it has enough space for defining the
filter so it should be fixing the problem.
ACK.
Possible improvement, instead of using a fixed column size the text area
also could be made to occupy 100% of available width. Ideally it should
have the same width as the text field or drop down list in this dialog.
Updated patch attached.
I added two styles:
.textarea-widget textarea {
width: 250px;
}
.facet-content .textarea-widget textarea {
width: 400px;
}
First one makes textarea width the same as text_widget - good for dialogs.
Second one makes textareas wider in facets. IMO it is better - more
space for the text. Also previous implementation was about 330px in FF
so it isn't big difference. 400px is about the same width as ssh-key
widget and in also leaves space for possible action panel.
--
Petr Vobornik
From 77cf857afc8d69852bc3e732c9635e2f7c503d4b Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Mon, 16 Jul 2012 13:40:52 +0200
Subject: [PATCH] Bigger textarea for permission type=subtree
Adder dialog and details facet for permission type=subtree have small textarea for defining subtree filter. It was unconfortable to define the filter. This difference was removed.
https://fedorahosted.org/freeipa/ticket/2832
---
install/ui/aci.js |2 --
install/ui/ipa.css |8
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/install/ui/aci.js b/install/ui/aci.js
index b2e5e19e54fe083093171d29fe894df321bb4c88..63181efac5f335d28a4c23bfa1ae2da95b9a0e75 100644
--- a/install/ui/aci.js
+++ b/install/ui/aci.js
@@ -688,8 +688,6 @@ IPA.permission_target_widget = function(spec) {
that.subtree_textarea = IPA.textarea_widget({
entity: that.entity,
name: 'subtree',
-cols: 30,
-rows: 1,
hidden: true
});
diff --git a/install/ui/ipa.css b/install/ui/ipa.css
index a3b93078b64fc057ba81271abbe093717d766149..76ce265f0f5c90404046726fe55bd54d73c9b365 100644
--- a/install/ui/ipa.css
+++ b/install/ui/ipa.css
@@ -1307,6 +1307,14 @@ table.scrollable tbody {
width: 250px;
}
+.textarea-widget textarea {
+width: 250px;
+}
+
+.facet-content .textarea-widget textarea {
+width: 400px;
+}
+
.combobox-widget-input {
display: inline-block;
position: relative;
--
1.7.10.4
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 172 Bigger textarea for permission type=subtree
On 7/17/2012 9:25 AM, Petr Vobornik wrote:
Possible improvement, instead of using a fixed column size the text area
also could be made to occupy 100% of available width. Ideally it should
have the same width as the text field or drop down list in this dialog.
Updated patch attached.
I added two styles:
.textarea-widget textarea {
width: 250px;
}
.facet-content .textarea-widget textarea {
width: 400px;
}
First one makes textarea width the same as text_widget - good for dialogs.
Second one makes textareas wider in facets. IMO it is better - more
space for the text. Also previous implementation was about 330px in FF
so it isn't big difference. 400px is about the same width as ssh-key
widget and in also leaves space for possible action panel.
Looks much better. ACK.
--
Endi S. Dewata
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0061 ValidationError takes 'error' named argument, not 'reason'
Alexander Bokovoy wrote: Hi, https://fedorahosted.org/freeipa/ticket/2865 ACK ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0062 support various forms of user account when establishing trusts
Alexander Bokovoy wrote: Hi, Realm administrator account may be specified using different form: Administrator, DOM\Administrator, Administrator@DOMAIN This patch introduces handling of the second two forms: - In DOM\Administrator only user name is used, short domain name is then taken from a discovered record from the AD DC - In Administrator@DOMAIN first DOMAIN is verified to be the same as the domain we are establishing trust to, and then user name is taken, together with short domain name taken from a discovered record from the AD DC Note that we do not support using to-be-trusted domain's trusted domains' accounts to establish trust as there is basically zero chance to verify that things will work with them. In addition, in order to establish trust one needs to belong to Enterprise Admins group in AD or have specially delegated permissions. These permissions are unlikely delegated to the ones in already trusted domain. https://fedorahosted.org/freeipa/ticket/2864 ACK ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0060 Ensure ipa-adtrust-install is run as admin user
Alexander Bokovoy wrote: On Fri, 13 Jul 2012, Alexander Bokovoy wrote: Hi, when adding AD trusts support, we need to ensure we have valid kerberos ticket of the user from 'admins' group or otherwise appropriate ACIs will not be granted. This patch introduces a check for that. We already check if ipa-adtrust-install is run by root so this complements existing checks. https://fedorahosted.org/freeipa/ticket/2815 After discussing on IRC with Simo and Rob, we came to conclusion that it is possible to switch to LDAPI and autobind feature of dirsrv for authentication and remove requirement for Directory Manager credentials altogether. Updated patch makes use of LDAPI + autobind under root privileges to map automatically to Directory Manager privileges in dirsrv. Additionally it ensures we have Kerberos credentials to fetch keytab with CIFS service key. Service._ldap_mod() is extended to switch to autobind when self.ldapi is set to True and we are running as root. For those interested in why ACIError is mapped to 'outdated Kerberos credentials' error message, this is because we'll get ACIError for 'ipa user-show uid' command when authenticated by the Kerberos credentials for uid in a default ccache only when Kerberos credentials are stale -- either belong to a user that was removed or to a previous IPA install that was wiped before reinstalling. The latter is how I discovered this case. :) I think that this should raise an exception if one tries to use ldapi, doesn't provide the DM password and is not root. Otherwise it won't authenticate at all. In reality, I think all this service code always runs as root, so it may be a moot point, but this code is kinda convoluted. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 1035 case sensitivity when calculating indirect members
When determining whether a member is direct or indirect we were not doing a case-insensitive comparison which led to marking a member as both direct and indirect (in a test case no less). This patch fixes the comparison and the test. rob From c03329d6ea7b5ddda917bca64a6f085efe9e6a95 Mon Sep 17 00:00:00 2001 From: Rob Crittenden rcrit...@redhat.com Date: Tue, 17 Jul 2012 14:48:47 -0400 Subject: [PATCH] Fix case sensitivity problem when comparing indirect group members. When determining whether a member is direct or indirect the case of the DN should not be considered. Fix a bad test associated with this as well. https://fedorahosted.org/freeipa/ticket/2872 --- ipaserver/plugins/ldap2.py |2 +- tests/test_xmlrpc/test_permission_plugin.py |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py index 6a3d2164ef9954f2cd5bace8cf75ee8ac5d7d82c..4eb49c0b1c951ec21871ccc8ce0a8ccc62a0e17d 100644 --- a/ipaserver/plugins/ldap2.py +++ b/ipaserver/plugins/ldap2.py @@ -1172,7 +1172,7 @@ class ldap2(CrudBackend, Encoder): entries = [] for e in results: -if unicode(e[0]) not in real_members and unicode(e[0]) not in entries: +if unicode(e[0].lower()) not in (member.lower() for member in real_members) and unicode(e[0].lower()) not in (entry.lower() for entry in entries): if membertype == MEMBERS_INDIRECT: entries.append(e[0]) else: diff --git a/tests/test_xmlrpc/test_permission_plugin.py b/tests/test_xmlrpc/test_permission_plugin.py index 8aaa4a0241a78c15f094ad6ae80beb48aec3..6589904ba4660d84fa25a800f99e4b170a3e964a 100644 --- a/tests/test_xmlrpc/test_permission_plugin.py +++ b/tests/test_xmlrpc/test_permission_plugin.py @@ -475,7 +475,7 @@ class test_permission(Declarative): 'permissions' : [u'write'], 'attrs': [u'servicecategory', u'sourcehostcategory', u'cn', u'description', u'ipaenabledflag', u'accesstime', u'usercategory', u'hostcategory', u'accessruletype', u'sourcehost'], 'subtree' : u'ldap:///ipauniqueid=*,cn=hbac,%s' % api.env.basedn, -'memberindirect': [u'cn=hbac administrator,cn=privileges,cn=pbac,%s' % api.env.basedn, u'cn=it security specialist,cn=roles,cn=accounts,%s' % api.env.basedn], +'memberindirect': [u'cn=it security specialist,cn=roles,cn=accounts,%s' % api.env.basedn], }, ], ), -- 1.7.10.2 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
Petr Viktorin wrote: On 06/29/2012 11:28 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 06/25/2012 03:00 PM, Petr Viktorin wrote: On 06/20/2012 06:15 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 06/04/2012 04:56 PM, Petr Viktorin wrote: Currently, FreeIPA's install/admin scripts are long pieces of code that aren't very reusable, importable, or testable. They have been extended over time with features such as logging and error handling, but since each tool was extended individually, there is much inconsistency and code duplication. This patch starts a framework which the admin tools can use, and converts ipa-ldap-updater to use the framework. In an earlier patch I found that improving a particular functionality in all the commands is not workable, so I want to tackle this one tool at a time. I'm starting with ipa-ldap-updater, because it's pretty small, doesn't use DNs (I don't want conflicts with John's work), and has the interesting --upgrade option. The framework does these tasks: - Parse options - Select tool to run (see below) - Validate options - Set up logging - Run the tool code - Handle any errors - Log success/failure The base class has some defaults for these that the tools can extend/override. To handle the case where one script does two different things (ipa-ldap-updater with/without --upgrade, or ipa-server-install with/without --uninstall), I want to split the tool in two classes rather than have repeated ifs in the code. This meant that option parsing (and initializing the parser) has to be done before creating an instance of the tool. I use a factory classmethod. I put the admintool base class in ipapython/ as it should be useful for ipa-client-install as well. First part of the work for: https://fedorahosted.org/freeipa/ticket/2652 Attaching rebased patch. I gather you want people to be calling run_cli() in their admin tools. Should main() be made private then? I could see someone getting confused and using main instead, which would work, but then the return value might not do the right thing. Or maybe just drop run_cli and have main exit with sys.exit()? I don't see why running a command as a Python function should be discouraged. In fact it could even help -- for example logging could only be set up once, so if we call, say, ipa-ldap-updater from ipa-server-install, all related logs would go to a single file. A C-style main (taking a list of arguments and returning the exit status) is a good thing for modularity and testability. The `run_cli` method is just a convenient shortcut for the usual usage, so the calling modules can be as small as possible. If people get confused and call main instead of run_cli, they need to manually pass in sys.argv. I think this is enough of a warning that their assumptions aren't right. To make it even clearer I've removed the possibility to pass None as argv to main() and have it auto-filled. Some relevant reading: http://www.artima.com/weblogs/viewpost.jsp?thread=4829 (old but still valid) http://en.wikipedia.org/wiki/Main_function#Python It isn't correctly handling the case of an update not found: ipa : INFO Parsing file ad [Errno 2] No such file or directory: 'ad' ipa : INFO File /usr/lib/python2.7/site-packages/ipapython/admintool.py, line 151, in execute self.run() File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_ldap_updater.py, line 180, in run modified = ld.update(self.files) File /usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py, line 828, in update sys.exit(1) ipa : INFO The ipa-ldap-updater command failed, exception: SystemExit: 1 I've added validation for missing files, and improved the error message ldapupdate raises (for cases the validation doesn't catch, like passing directories or unreadable files). Ideally ldapupdate would not try to handle the error itself, but that code is used in more places that I don't want to break, so I'm leaving the extraneous print in. Running in test mode with the attached update doesn't seem to work either. There is nothing special about this file, just something I had lying around: ipa : INFO File /usr/lib/python2.7/site-packages/ipapython/admintool.py, line 151, in execute self.run() File /usr/lib/python2.7/site-packages/ipaserver/install/ipa_ldap_updater.py, line 184, in run 'Update complete, changes to be made, test mode', 2) ipa : INFO The ipa-ldap-updater command failed, exception: ScriptError: Update complete, changes to be made, test mode ipa : ERROR Update complete, changes to be made, test mode ipa : ERROR None Fixed. The unit tests still pass which is good. With ipa-ldap-updater the return value is a bit strange. All the updates themselves can fail for one reason or another and the command can still consider this a success (it may fail because a feature is not enabled, for example). Still, the success message displayed at the end is a bit jarring when the updates themselves aren't applied. Here is a snippet when
Re: [Freeipa-devel] DN patch and documentation
On 07/10/2012 04:23 AM, Petr Viktorin wrote: I've read your summary (which you should summarize into a commit message before this is pushed), and gone through the patch. Here is what I found doing that; I didn't get to actual testing yet. I also didn't do a detailed review of ldap2.py changes yet. Thank you for your careful review Petr, you had many good suggestions and comments. It was an enormous amount of material to wade through, thank you for your diligence. I'm in agreement with most of your suggestions. I agree with Simo that it would have been better to put at least your automated changes in a separate patch. Without knowing what was automated and what wasn't, I have to eyeball each change at least to figure that out. Not complaining, it's a reviewer's work after all, and with an intra-line diff tool it's not that hard, but I can't really honor your suggestion for a faster review :) I agree fully it would be nice if things were broken up into smaller well defined patches. When I started the work I tried to keep things separate but as I worked I discovered there were so many inter-dependent relationships I was spending as much time juggling patch sets that could be applied independently as I was making forward progress to the real goal. So I abandoned the idea of independent patch sets in favor of getting the work completed as expeditiously as possible, it was a pragmatic trade-off. Now that the work is (nearly) completed I can see where some things can be broken out cleanly that was not obvious in the midst of the work. Hindsight is 20/20. Blockers Mutable objects that support __hash__ are a *very* bad thing to do. It violates a fundamental assumption in Python, as you outlined. Mutable objects *must not* be allowed to be dictionary keys. I understand the patch is not perfect, but this is a big issue that invites very hard-to-debug errors. You say you're already working on a patch to fix this. Please post that ASAP; I can't ack the DN work without it. Understood, I agree with you and I will start work on those changes immediately. In the long run, it would be good to use immutable DNs only: I think modeling them after Python strings in this way would be beneficial. We edit DN's in a variety of places thus I feel mutable DN's have a distinct value. If we only had immutable DN's then forming a new DN would require more code that would extract the components from the right locations in the original DN and use those extracted components to build a new DN. That would not be too far from some of the earlier code which used a series of calls over multiple lines to slice and dice the strings. I believe using just one simple obvious Python operator beats multiple lines of convoluted code every time. The expression of the concept is inherently obvious in the line of code using Python operator(s), plus it's vastly more succinct (easier to read, easier to code, more robust). Because the logic to build the new DN is inherently centralized in the object operator it's a single implementation (much easier to maintain and prove correctness) rather than the cut-n-paste coping of code to decompose and reassemble an DN over many locations in the code base. I believe mutable DN's have a valuable role to play and make the best use of Python's object oriented facilities. They go a long way to making IPA more robust and easier to comprehend. You seem to forged locked=True for some global DNs, e.g. ipalib/plugins/pwpolicy.py:172: global_policy_dn ipalib/plugins/migration.py:117: _compat_dn Locked by default would prevent these omissions. Please fix a lint failure; ipaserver.ipautil moved to ipapython.ipautil. Design decision criticism Originally I was against automatic type promotion on the belief if you were comparing a string to a DN it was a failure of program logic, but there were some circumstances where it was evil necessity. Would you care to elaborate? I also think that's a logic failure; such circumstances should at least be marked by a TODO. I should have taken better notes because I don't remember the exact place this came up, it was late in the testing and I believe the problem showed up in the ldap updater, possibly some other places. But here is the bottom line: I encountered a handful of places in the code where a string was being compared to a DN object for equality (I seem to recall the equality comparison was indirect by virtue of the in operator). What I do recall is it was logically impossible to know a prori the string being tested was supposed to be a DN. If it were possible to know the string represented a DN it would have been easy to promote it to a DN object. But there were a handful of situations where there simply was not enough information available to ascertain if the string was logically a DN or not. This was never an issue when dn's were simple strings. If it's getting compared to a DN
