Re: [Freeipa-devel] [PATCH] 142 Moved entity builder registration into webui.js.
On 04/19/2011 06:50 PM, Endi Sukma Dewata wrote: The entity builder registration have been moved into webui.js. This allows the WebUI to control which entities will be loaded. For instance, the entitlement should be loaded only if the plugin is enabled. This is done by checking the metadata. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel NACK. I like the concept, but the implementation is too verbose. Instead: Leave the factories as the top level object. For the factory name, say 'entitle' check that the object is in the metadata. If not, don't create the entity. Then, when processing the tabs, if the entity does not exist, drop the tab from the tab set. The explicit enumeration of entities in webui.js is not necessary, nor is putting every entity's factory into its own namespace. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 142 Moved entity builder registration into webui.js.
On 04/20/2011 12:28 PM, Endi Sukma Dewata wrote:
On 4/20/2011 9:43 AM, Adam Young wrote:
On 04/19/2011 06:50 PM, Endi Sukma Dewata wrote:
The entity builder registration have been moved into webui.js. This
allows the WebUI to control which entities will be loaded. For
instance,
the entitlement should be loaded only if the plugin is enabled. This
is done by checking the metadata.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
NACK.
I like the concept, but the implementation is too verbose. Instead:
Leave the factories as the top level object. For the factory name, say
'entitle' check that the object is in the metadata. If not, don't create
the entity. Then, when processing the tabs, if the entity does not
exist, drop the tab from the tab set. The explicit enumeration of
entities in webui.js is not necessary, nor is putting every entity's
factory into its own namespace.
I think it would be even better to create only the entities that are
actually needed. So entity creation should be done after we determine
the user and the tab set for that user. What do you think?
Absolutely. I think that the check should be something like:
that.start_entities = function(){
...
for (name in that.entity_factories){
if (!metadata.objects[name]) continue;
...
}
and then the tab gets dropped later if the entity doesn't exist.
Creating a namespace for each entity is actually a separate but
related issue. It's mainly needed to avoid conflicts and we have done
that for certificates and entitlements. Sooner or later we'll create
name space for other entities anyway.
You don't need it now lets avoid introducing it for as long as
possible. The namespaces are not likely to be needed for most
entities. They are really the exception, not the rule.
The namespace will also improve modularity. It can be used to specify
the default builder for that entity (e.g. IPA.user.entity_builder)
instead of registering itself into IPA.entity_factories with a fixed
name. This would be useful suppose one day we want to provide
different entity implementations with the same name for different
users. For example:
IPA.user.entity_builder = simplified UI
IPA.admin.user.entity_builder = complete UI
Interesting approach. I'd rather not do that right now, though.
Both are 'user' entity, but depending on which user logs in, the web
ui can decide which implementation to use:
var module = either IPA.user or IPA.admin.user
IPA.entity_factory[entity_name] = module.entity_builder;
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 771 include rights for all aci components in permission_show
Martin Kosek wrote: On Wed, 2011-04-13 at 11:34 -0400, Rob Crittenden wrote: Provide attributelevelrights for the aci components in permission_show. Since the broken-out components are just part of the aci just copy right access rights for aci. ticket 943 rob NACK. Works fine for permission-show, but I think we should add the attributelevelrights for permission-mod command too. This issue is very similar to ticket #1103, where I was adding missing rights for pwpolicy - we can get some inspiration from there. Martin permission_mod calls permission_show so passing **options should fix it. rob freeipa-rcrit-771-2-permission.patch Description: application/mbox ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
JR Aquino wrote:
On Apr 12, 2011, at 10:55 AM, Rob Crittenden wrote:
JR Aquino wrote:
On Apr 7, 2011, at 7:08 PM, JR Aquino wrote:
On Apr 7, 2011, at 4:04 PM, JR Aquino wrote:
On Apr 7, 2011, at 3:42 PM, JR Aquino wrote:
On Mar 31, 2011, at 2:16 PM, JR Aquino wrote:
On Mar 31, 2011, at 1:48 PM, Rob Crittenden wrote:
JR Aquino wrote:
The following patch Removes around 20 lines of code and provides a substantial
increase in performance for FreeIPA member/memberof verification searches.
The current code base blindly searches static containers for the possible
presence of members.
This patch provides a method for dynamically identifying the specific objects
to verify memberships for.
The attached patch addresses ticket:
https://fedorahosted.org/freeipa/ticket/1139
Without patch
ipa hostgroup-find
...
-
Number of entries returned 52
-
real0m20.054s
user0m0.934s
sys 0m0.050s
With Patch
ipa find-hostgroup
...
-
Number of entries returned 52
-
real0m15.064s
user0m0.945s
sys 0m0.057s
--
Number of entries returned 100
--
real0m16.471s
user0m0.814s
sys 0m0.040s
Without Patch
ipa host-find
...
--
Number of entries returned 100
--
real0m41.277s
user0m0.806s
sys 0m0.060s
With Patch
ipa host-find
...
--
Number of entries returned 100
--
real0m16.385s
user0m0.814s
sys 0m0.053s
There is a typo in the first block, memeber.
Wouldn't it be clearer to do a negative test to continue:
if not 'member' in r[1]:
continue
rob
You're right!
Corrected patch attached.
Self Nack
After cli and webui testing, it turned out there was a previous try / except
block that was reseting the results value back to []
Corrected and reattaching new patch.
Testing cli and webui checks out correctly. Speed AND accuracy are now
addressed.
It was also discovered during the course of testing that this patch addresses
one of the causes for the bug thrown in:
https://fedorahosted.org/freeipa/ticket/1133
-JR
NACK
Looks like there may still need to be work with the indirect / direct functions.
Will revisit next week.
Ok I finally think I've got it.
My for loop was in my try / except block. It has now been corrected.
I've tested the searches for: users, groups, sudocmds, sudcmdgroups, sudorules,
hosts, hostgroups, hbacrules, hbacsv, hbsvcgroups, and all return as expected.
Please make sure that they return for you as well.
Please let me know if there is anything else I have missed.
Final Patch attached due to relationship with ticket 1133:
Added Comments regarding Ticket 1133 / calculating indirect:
+# If there is an exception here, it is due to a failure in referential
integrity.
+# All members should have corresponding memberOf entries.
Retested against all xmlrpc tests and passed.
Seems to work as advertised, I just have a couple of requests:
- Some of the comments are really long, can you limit to ~75 chars per line?
- In this code block:
for r in results:
direct.append(r[0])
try:
indirect.remove(r[0].lower())
except ValueError:
pass
We should log if a ValueError is thrown, this would mean something is really
wrong. Can you import logging in the file and replace pass with something like:
logging.info('Failed to remove indirect entry %s from %s' % r[0], entry_dn)
I wonder if we should raise the ValueError too. This means that something went
very wrong.
Yes I agree that we should raise the error.
Here is the patch with the requested changes:
* Fixed width to be PEP8 compliant
* import logging
* Added logging line in exception
* Raise ValueError if we blow up during indirect removal.
Fixes 1-3 look good. I think for the exception you want:
except ValueError, e:
logging
raise e
A ValueError won't get returned over XML-RPC but the full backtrace will
be logged on the server side. The end-user will get a 903 (Internal
error raised).
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 23 Optimize and dynamically verify group membership
On Apr 20, 2011, at 10:32 AM, Rob Crittenden wrote:
...
Seems to work as advertised, I just have a couple of requests:
- Some of the comments are really long, can you limit to ~75 chars per line?
- In this code block:
for r in results:
direct.append(r[0])
try:
indirect.remove(r[0].lower())
except ValueError:
pass
We should log if a ValueError is thrown, this would mean something is
really wrong. Can you import logging in the file and replace pass with
something like:
logging.info('Failed to remove indirect entry %s from %s' % r[0], entry_dn)
I wonder if we should raise the ValueError too. This means that something
went very wrong.
Yes I agree that we should raise the error.
Here is the patch with the requested changes:
* Fixed width to be PEP8 compliant
* import logging
* Added logging line in exception
* Raise ValueError if we blow up during indirect removal.
Fixes 1-3 look good. I think for the exception you want:
except ValueError, e:
logging
raise e
A ValueError won't get returned over XML-RPC but the full backtrace will be
logged on the server side. The end-user will get a 903 (Internal error
raised).
rob
Ok adjusted patch attached to correct for the exception raised.
binBzK83O11Mp.bin
Description: freeipa-jraquino-0023-Optimize-and-dynamically-verify-group-membership.patch
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 12 Remove unused classes
Jan Cholasta wrote: Removes unused classes NSPRConnection and NSPRHTTP from ipapython.nsslib. ack, pushed to master and ipa-2-0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 13 Final lint fixes
Jan Cholasta wrote: Fixes additional false positives found in install/po/test_i18n.py and a missing import in ipa-nis-manage. This should be the last patch dealing with pylint-discovered issues. ack, pushed to master and ipa-2-0. FYI, I amended the commit message to be a bit more specific. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 3 Add ability to specify netmask with IP addresses during installation
Jan Cholasta wrote:
On 11.4.2011 12:48, Jan Cholasta wrote:
On 8.4.2011 16:26, Rob Crittenden wrote:
Jan Cholasta wrote:
On 29.3.2011 22:15, Rob Crittenden wrote:
Jan Cholasta wrote:
Sorry, forgot to attach the patch.
Is this why you have some blind excepts?
installutils._IPAddressWithPrefix('192.168.0.1/33')
Traceback (most recent call last):
File stdin, line 1, in module
File ipaserver/install/installutils.py, line 167, in __init__
net = netaddr.IPNetwork(addr)
File /usr/lib/python2.7/site-packages/netaddr/ip/__init__.py, line
919, in __init__
implicit_prefix, flags)
File /usr/lib/python2.7/site-packages/netaddr/ip/__init__.py, line
782, in parse_ip_network
value = ip._value
UnboundLocalError: local variable 'ip' referenced before assignment
We should get an upstream bug filed on python-netaddr about this.
https://github.com/drkjam/netaddr/issues/closed#issue/5
https://github.com/drkjam/netaddr/issues/closed#issue/6
https://github.com/drkjam/netaddr/issues/closed#issue/8
Apparently it's already been fixed for the next release.
IMHO it's not much of an issue for us, because the exception gets
caught
in parse_ip_address and that's currently the only place where
_IPAddressWithPrefix is used.
Shoudl parse_ip_address() raise an exception on bad data rather than
returning 0.0.0.0?
I've been down that road and it would need a rewrite of the fragile IP
address handling logic of ipa-server-install, which is something I'd
rather avoid.
installutils.parse_ip_address('355.555.3.3')
_IPAddressWithPrefix('0.0.0.0')
or
installutils.parse_ip_address('192.168.0.1/55')
_IPAddressWithPrefix('0.0.0.0')
Should it disallow net addresses like 192.168.0.0?
If you mean network and broadcast addresses, it probably should. It
might be a good idea to disallow localhost, multicast and/or link-local
addresses too.
Are you going to resubmit the patch with these added or should we open a
separate ticket?
I'm going to resubmit it. Right now it disallows loopback, IANA
reserved, link-local, network, multicast and broadcast IP addresses.
Does it make sense to also allow only IP addresses attached to one of
the local network interfaces? Perhaps it would be sufficient just to
print a warning. Or should we not care about that at all?
Sending the updated patch.
This looks ok, just one question. Should we add a dependency on the
iproute package because of the /sbin/ip package?
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 142 Moved entity builder registration into webui.js.
On 4/20/2011 11:50 AM, Adam Young wrote:
Leave the factories as the top level object. For the factory name, say
'entitle' check that the object is in the metadata. If not, don't create
the entity. Then, when processing the tabs, if the entity does not
exist, drop the tab from the tab set. The explicit enumeration of
entities in webui.js is not necessary, nor is putting every entity's
factory into its own namespace.
I think it would be even better to create only the entities that are
actually needed. So entity creation should be done after we determine
the user and the tab set for that user. What do you think?
Absolutely. I think that the check should be something like:
that.start_entities = function(){
...
for (name in that.entity_factories){
if (!metadata.objects[name]) continue;
...
}
and then the tab gets dropped later if the entity doesn't exist.
I don't think this approach will work for DNS. If dns_is_enabled is
false the entity should not be created either. I'm not sure inserting
this logic in the IPA.start_entities() would be a good idea. The
start_entities() should read from a list of entities to be created
instead of filtering out the entities within the loop.
Creating a namespace for each entity is actually a separate but
related issue. It's mainly needed to avoid conflicts and we have done
that for certificates and entitlements. Sooner or later we'll create
name space for other entities anyway.
You don't need it now lets avoid introducing it for as long as
possible. The namespaces are not likely to be needed for most entities.
They are really the exception, not the rule.
The navigation is using a namespace too. It's not a rule, but more like
planning ahead rather than reactively fixing it when we encounter the
first problem. This is important since JavaScript will not alert us when
there's a conflict. User, Host, Service, DNS, HBAC, Sudo, ACI and
Widgets are strong candidates because they have a set of related
classes. This is similar to Python modules vs. C-style prefixes. We do
have the IPA namespace, but right now it's still mostly flat without
additional organization.
--
Endi S. Dewata
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 142 Moved entity builder registration into webui.js.
On 4/20/2011 4:32 PM, Endi Sukma Dewata wrote:
On 4/20/2011 11:50 AM, Adam Young wrote:
Leave the factories as the top level object. For the factory name, say
'entitle' check that the object is in the metadata. If not, don't
create
the entity. Then, when processing the tabs, if the entity does not
exist, drop the tab from the tab set. The explicit enumeration of
entities in webui.js is not necessary, nor is putting every entity's
factory into its own namespace.
I think it would be even better to create only the entities that are
actually needed. So entity creation should be done after we determine
the user and the tab set for that user. What do you think?
Absolutely. I think that the check should be something like:
that.start_entities = function(){
...
for (name in that.entity_factories){
if (!metadata.objects[name]) continue;
...
}
and then the tab gets dropped later if the entity doesn't exist.
I don't think this approach will work for DNS. If dns_is_enabled is
false the entity should not be created either. I'm not sure inserting
this logic in the IPA.start_entities() would be a good idea. The
start_entities() should read from a list of entities to be created
instead of filtering out the entities within the loop.
Attached is a new patch that creates only the entities that are enabled
and needed by the navigation tabs. It passes jslint, qunit, and
essential Selenium tests.
This patch can be applied against master or your patch #221.
--
Endi S. Dewata
From 6ca409845363fb7620060384c6e5457da06afd74 Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Wed, 20 Apr 2011 19:11:10 -0500
Subject: [PATCH] Fixed entity creation and navigation.
Currently all entities are always created regardless its usage. Some
entities such as DNS and entitlement may not actually be available
depending on server configuration. Also the user's authorization
determines which entities are accessible via navigation tabs. The
Web UI has been modified to take these factors into consideration
when creating the entities and navigation tabs.
---
install/ui/ipa.js | 52 +--
install/ui/navigation.js| 146 --
install/ui/test/details_tests.js|2 +-
install/ui/test/entity_tests.js |2 +-
install/ui/test/navigation_tests.js | 138 ++---
install/ui/webui.js | 168 --
6 files changed, 291 insertions(+), 217 deletions(-)
diff --git a/install/ui/ipa.js b/install/ui/ipa.js
index a4fbec4014202956799d1a058f1e0ec767f7959a..de90e7feff775411872f9b78983dfb8e1bb3717b 100644
--- a/install/ui/ipa.js
+++ b/install/ui/ipa.js
@@ -134,18 +134,26 @@ var IPA = ( function () {
return that.entities_by_name[name];
};
-function add_entity(entity) {
+that.add_entity = function(entity) {
that.entities.push(entity);
that.entities_by_name[entity.name] = entity;
-}
+};
-that.start_entities = function(){
-var factory;
-var name ;
-for (name in that.entity_factories){
-factory = that.entity_factories[name];
+that.init_entities = function(entity_names) {
+var entity_name;
+
+if (!entity_names) {
+entity_names = [];
+for (entity_name in that.entity_factories) {
+entity_names.push(entity_name);
+}
+}
+
+for (var i=0; ientity_names.length; i++) {
+entity_name = entity_names[i];
+var factory = that.entity_factories[entity_name];
var entity = factory();
-add_entity(entity);
+that.add_entity(entity);
entity.init();
}
};
@@ -180,31 +188,19 @@ var IPA = ( function () {
return true;
};
-that.show_page = function (entity_name, facet_name) {
-if (!IPA.test_dirty()){
-return false;
+that.switch_and_show_page = function(entity_name, facet_name, pkey) {
+if (!IPA.test_dirty()) {
+return;
}
var state = {};
+
+if (pkey) {
+state[entity_name + '-pkey'] = pkey;
+}
+
state[entity_name + '-facet'] = facet_name;
$.bbq.pushState(state);
-return true;
-};
-
-that.switch_and_show_page = function (this_entity, facet_name, pkey) {
-if (!IPA.test_dirty()){
-return false;
-}
-
-if (!pkey){
-that.show_page(this_entity, facet_name);
-return false;
-}
-var state = {};
-state[this_entity+'-pkey'] = pkey;
-state[this_entity + '-facet'] = facet_name;
-$.bbq.pushState(state);
-return true;
};
that.display_activity_icon = function() {
diff --git a/install/ui/navigation.js b/install/ui/navigation.js
index 365bde66de39c36e473fc2429f78039e41a8b374..37e24da79be31298dd0b7f156ce314274547462b
