Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
Summary to maintain order in the list:
This effort was split into patches:
freeipa-pvoborni-0030-Extending-facet-s-mechanism-of-gathering-changes
freeipa-pvoborni-0045-Code-cleanup-of-HBAC-Sudo-rules.patch - a part of
greater UI refactoring effort ticket #2040 (patches 32-49).
Patches mentioned above were ACKed and pushed to master.
On 10/29/2011 03:45 AM, Adam Young wrote:
On 10/27/2011 08:55 PM, Endi Sukma Dewata wrote:
On 10/27/2011 6:39 PM, Adam Young wrote:
We might need to distinguish 2 different usages of 'entity'. The first
one represents a collection of entries:
Call that an instance. Entity is the term that is the analogue of Class
Not sure I understand correctly. You mean entity is a class which is a
collection, similar to a table in database? And instance is an object
or individual entry or row in a table?
Yes, that is a pretty good analogy.
So we want to distinguish getting the primary key field for the entity,
as opposed to the primary key of the instance.
From the URL we want to get the primary key for a particular
instance/object to show in the detail page.
Yes. In a RESTful scheme it would be /IPA/entities/users/ayoung to get
my user object
The IPA.client will represent a connection to the IPA server. In a
browser IPA.client can only connect to the server it's loaded from:
var client = IPA.client();
but in a JS engine like Rhino the IPA.client can connect to any IPA
server:
var client = IPA.client('ipa.example.com');
This will work now, but you will not be able to see the results of the
command. Integrations like this are how the Like buttons from Facebook
work. Cross site posting is tricky, but permitted, and might be useful
in some cases. Possibly we should call it connection.
The second code is not supposed to be used inside a browser. This is
suppose you're writing a JS script running in Rhino, you need to
specify the IPA server you're connecting to. I haven't tried this, but
I suppose in Rhino we will be able to set the Referer to match the
server name.
I'd rather call it IPA.client because it will do other things too, not
just connection.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
--
Petr Vobornik
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
On 10/27/2011 08:55 PM, Endi Sukma Dewata wrote:
On 10/27/2011 6:39 PM, Adam Young wrote:
We might need to distinguish 2 different usages of 'entity'. The first
one represents a collection of entries:
Call that an instance. Entity is the term that is the analogue of Class
Not sure I understand correctly. You mean entity is a class which is a
collection, similar to a table in database? And instance is an object
or individual entry or row in a table?
Yes, that is a pretty good analogy.
So we want to distinguish getting the primary key field for the entity,
as opposed to the primary key of the instance.
From the URL we want to get the primary key for a particular
instance/object to show in the detail page.
Yes. In a RESTful scheme it would be /IPA/entities/users/ayoung to
get my user object
The IPA.client will represent a connection to the IPA server. In a
browser IPA.client can only connect to the server it's loaded from:
var client = IPA.client();
but in a JS engine like Rhino the IPA.client can connect to any IPA
server:
var client = IPA.client('ipa.example.com');
This will work now, but you will not be able to see the results of the
command. Integrations like this are how the Like buttons from Facebook
work. Cross site posting is tricky, but permitted, and might be useful
in some cases. Possibly we should call it connection.
The second code is not supposed to be used inside a browser. This is
suppose you're writing a JS script running in Rhino, you need to
specify the IPA server you're connecting to. I haven't tried this, but
I suppose in Rhino we will be able to set the Referer to match the
server name.
I'd rather call it IPA.client because it will do other things too, not
just connection.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
I'll rebase this patch on recent patches (which will be soon pushed).
This effort is doing too many things, I'll split the patch into several.
On 10/25/2011 07:57 PM, Endi Sukma Dewata wrote:
On 10/25/2011 7:49 AM, Petr Vobornik wrote:
2. The set_facet() is added to widget. I don't think we want to make
widget dependent on facet. The facet so far is only used by
IPA.sudo.enable_widget. In IPA.sudo.options_section the facet object is
passed as a parameter in spec.
Are you saying that dependency on facet in ALL widgets is bad and only
in a few is good, or in any is bad?
In general less dependency is better. More dependency will limit its
usage. This is inline with the goal to make purely HTML widget.
But if a widget is meant to be used only with a facet then it's
certainly ok to make it depends on facet. However, the rest should not
become unnecessarily dependent on facet too.
I think it doesn't matter if all is dependant when one is dependant.
Anyway currently all association table widgets are dependant
(association.js:386).
Let's use this as an example. The association table explicitly checks if
the facet is dirty and asks the user whether to update/reset/cancel.
Suppose we want to use this inside a dialog, it will still check whether
the current facet is dirty instead of the dialog itself, which may not
be what we want.
Removed. Using instead: that.entity.get_facet() and
that.entity.get_primary_key().
But still I think it would be better to be able to get container
(facet/dialog) for a widget. As you wrote, that.entity.get_facet() may
not always be what we want.
Btw similar topic could be: How to get current entity's pkey?'.
Dependancy on facet.get_primary_key() or
IPA.nav.get_state(that.entity.name+'-pkey') seems wrong too.
Yes. Ideally we should have a path (REST-style URL) instead of the
current parameter-based URL. We should get the pkey from that path.
From dependency point of view, widgets would be still dependant on the
implementation of navigation (IMHO bad).
But it seems that using entity.get_primary key partially solves the problem.
Maybe it would be better if navigation would inject pkey to entity.
Entity wouldn't be that much dependant on navigation implementation.
3. When updating sudo rule, in the original code the rule is
enabled/disabled after all the modifications are done. In the new code
it's reversed. I'm not sure the importance of the execution order for
this particular situation, but suppose it is, is there a way to maintain
the original order?
I was thinking about a command or widget priority. The 'mod' command
would have some default priority or it would get it from facet. The
commands would be sorted by priority before adding them to batch. This
way we can set exact order in facet update.
It's possible, but managing priority could be problematic if they are
spread out, because we have to know all existing priorities before we
can add a new one.
The priority could be part of update info. As there is a field_info we
can create a command_info: { command, priority }.
The original code uses an ordered list of commands.
Before adding commands to batch_command, array of command_info objects
would joined with 'mod' command with default priority and then sorted by
priority.
If the priority was set on each widget, priority management will be on
facet level, which may be fine. There can be some corner case like
dynamic change of priority.
The order which is implemented now is reflecting the order in HBAC
details facet - there were errors when 'mod' was executed before
clearing associations.
Right, so for certain things the order is important.
4. The IPA.header_widget is not really a widget, it's actually part of
layout. In the current implementation a widget always corresponds to an
attribute, so it will be loaded, saved, etc. Since there is no attribute
name matching the header name currently this is not a problem, but it
can pollute the namespace.
This is part of widget nesting topic. In this manner composite_widget
isn't a proper widget too (it can correspond to several or none
attribute).
Purely html rendering widgets can be separated from attribute widgets,
but it would be nice to have them because they can provide easier form
composing. Without them it would be required to override the create
method (in facet or composite_widget/section) which is sometimes better
but often it creates only code duplication with little added logic.
One other solution is to split widgets into non-visual fields and purely
HTML components. Then in the facet we use separate lists for the fields
and HTML components. The fields will have a reference to the
corresponding HTML components. There can be more HTML components than
fields. But this will require a significant restructuring.
Maybe we can use hybrid solution: html widgets would be simple object
with some properties and create() method. They should not be called
widgets. They would
Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
On 10/27/2011 8:39 AM, Petr Vobornik wrote:
But still I think it would be better to be able to get container
(facet/dialog) for a widget. As you wrote, that.entity.get_facet() may
not always be what we want.
One possibility is to convert the facet dialog into subclasses of an
abstract container class. The container needs to provide functions such
as refresh() so the widget doesn't need to know whether it's a facet or
dialog.
Btw similar topic could be: How to get current entity's pkey?'.
Dependancy on facet.get_primary_key() or
IPA.nav.get_state(that.entity.name+'-pkey') seems wrong too.
Yes. Ideally we should have a path (REST-style URL) instead of the
current parameter-based URL. We should get the pkey from that path.
From dependency point of view, widgets would be still dependant on the
implementation of navigation (IMHO bad).
But it seems that using entity.get_primary key partially solves the
problem.
It could be argued that since this is a web app everything will have a
path, but for now getting primary key from entity is fine since
everything also has an associated entity.
Maybe it would be better if navigation would inject pkey to entity.
Entity wouldn't be that much dependant on navigation implementation.
We might need to distinguish 2 different usages of 'entity'. The first
one represents a collection of entries:
var users = IPA.get_entity('user');
users.add([ 'test' ], { givenName: 'Test', ... });
var result = users.find('t*');
The second one represents individual entry:
var user = result.result[0];
user.update({ sn: 'User' });
The entity collection should be 'stateless', it will be used in search
pages. The individual entity will be 'stateful', it contains the pkey
and the values of its attributes, and it will be used in the details page.
The entity.get_primary_key() is an interface to get the primary keys for
a particular entity from the current path. So when you open a URL for an
entry's details facet, it will execute the following command:
var pkeys = entity.get_primary_key();
var object = entity.get(pkeys);
Then the facet will use the object to populate the page.
The priority could be part of update info. As there is a field_info we
can create a command_info: { command, priority }.
Before adding commands to batch_command, array of command_info objects
would joined with 'mod' command with default priority and then sorted by
priority.
If the priority was set on each widget, priority management will be on
facet level, which may be fine. There can be some corner case like
dynamic change of priority.
I'd have to see how it's implemented.
One other solution is to split widgets into non-visual fields and purely
HTML components. Then in the facet we use separate lists for the fields
and HTML components. The fields will have a reference to the
corresponding HTML components. There can be more HTML components than
fields. But this will require a significant restructuring.
Maybe we can use hybrid solution: html widgets would be simple object
with some properties and create() method. They should not be called
widgets. They would not be part of sections fields. Rendering would be
done in widgets/sections create() method (as it is now).
I'm not very clear, we could be talking about the same thing. The
(non-HTML) fields could be defined like this:
fields: [
{
name: 'cn',
widget: 'identity.cn'
},
{
name: 'email',
widget: 'identity.email'
},
{
factory: IPA.password_field,
name: 'userpassword',
widgets: [ 'account.password1', 'account.password2' ]
}
]
The specs above will be used to create IPA.field objects (not widget) to
assist loading/saving attributes. Then we could also define the HTML
components (let's call it widget) separately:
widgets: [
{
factory: IPA.table_section,
name: 'identity',
label: 'Identity'
widgets: [
{
factory: IPA.text_widget,
name: 'cn'
},
{
factory: IPA.multivalued_widget,
name: 'email',
widget: IPA.text_widget
}
]
}
]
The specs above will be used to create IPA.widget objects to generate
the HTML content. If there is no widget specs specified, we could
generate it automatically from the field specs.
Pros:
- reusable layouts, headers...
- not polluting field names
Cons:
- not so declarative - need to override update method, create custom
section/widget factories - same as now
The above example should be quite declarative.
Question:
- what with widget nesting? rule_details_section is in fact a composite
widget. I would like to keep the concept, because it offers better code
reuse.
As shown above, widget (not field) nesting is possible.
A section is a composite widget with a specific
Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
On 10/27/2011 05:51 PM, Endi Sukma Dewata wrote:
On 10/27/2011 8:39 AM, Petr Vobornik wrote:
But still I think it would be better to be able to get container
(facet/dialog) for a widget. As you wrote, that.entity.get_facet() may
not always be what we want.
One possibility is to convert the facet dialog into subclasses of an
abstract container class. The container needs to provide functions
such as refresh() so the widget doesn't need to know whether it's a
facet or dialog.
Btw similar topic could be: How to get current entity's pkey?'.
Dependancy on facet.get_primary_key() or
IPA.nav.get_state(that.entity.name+'-pkey') seems wrong too.
Yes. Ideally we should have a path (REST-style URL) instead of the
current parameter-based URL. We should get the pkey from that path.
From dependency point of view, widgets would be still dependant on the
implementation of navigation (IMHO bad).
But it seems that using entity.get_primary key partially solves the
problem.
It could be argued that since this is a web app everything will have a
path, but for now getting primary key from entity is fine since
everything also has an associated entity.
Maybe it would be better if navigation would inject pkey to entity.
Entity wouldn't be that much dependant on navigation implementation.
We might need to distinguish 2 different usages of 'entity'. The first
one represents a collection of entries:
Call that an instance. Entity is the term that is the analogue of Class
So we want to distinguish getting the primary key field for the entity,
as opposed to the primary key of the instance.
var users = IPA.get_entity('user');
users.add([ 'test' ], { givenName: 'Test', ... });
var result = users.find('t*');
The second one represents individual entry:
var user = result.result[0];
user.update({ sn: 'User' });
The entity collection should be 'stateless', it will be used in search
pages. The individual entity will be 'stateful', it contains the pkey
and the values of its attributes, and it will be used in the details
page.
The entity.get_primary_key() is an interface to get the primary keys
for a particular entity from the current path. So when you open a URL
for an entry's details facet, it will execute the following command:
var pkeys = entity.get_primary_key();
var object = entity.get(pkeys);
Then the facet will use the object to populate the page.
The priority could be part of update info. As there is a field_info we
can create a command_info: { command, priority }.
I'm a little unclear on the usage of priority.
Before adding commands to batch_command, array of command_info objects
would joined with 'mod' command with default priority and then sorted by
priority.
If the priority was set on each widget, priority management will be on
facet level, which may be fine. There can be some corner case like
dynamic change of priority.
I'd have to see how it's implemented.
One other solution is to split widgets into non-visual fields and
purely
HTML components. Then in the facet we use separate lists for the fields
and HTML components. The fields will have a reference to the
corresponding HTML components. There can be more HTML components than
fields. But this will require a significant restructuring.
Maybe we can use hybrid solution: html widgets would be simple object
with some properties and create() method. They should not be called
widgets. They would not be part of sections fields. Rendering would be
done in widgets/sections create() method (as it is now).
I think this is not quite right. Widget is just a generic term for an
element of the UI object model. It is intentionally generic. A Widget
can be just about anything. Some bind down to the individual fields, or
in somcses, even smaller, but widget could easily be the base class for
Facets, sections and the widget we have now.
I'm not very clear, we could be talking about the same thing. The
(non-HTML) fields could be defined like this:
fields: [
{
name: 'cn',
widget: 'identity.cn'
},
{
name: 'email',
widget: 'identity.email'
},
{
factory: IPA.password_field,
name: 'userpassword',
widgets: [ 'account.password1', 'account.password2' ]
}
]
The specs above will be used to create IPA.field objects (not widget)
to assist loading/saving attributes. Then we could also define the
HTML components (let's call it widget) separately:
widgets: [
{
factory: IPA.table_section,
name: 'identity',
label: 'Identity'
widgets: [
{
factory: IPA.text_widget,
name: 'cn'
},
{
factory: IPA.multivalued_widget,
name: 'email',
widget: IPA.text_widget
}
]
}
]
The specs above will be used to create
Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
Comments in text. New patch not supplied yet - some topics may require
further discussion. Most of the comments should be part of the 'Nesting
widgets' thread.
On 10/24/2011 06:29 PM, Endi Sukma Dewata wrote:
On 10/24/2011 3:29 AM, Petr Vobornik wrote:
https://fedorahosted.org/freeipa/ticket/1515
Some issues:
1. The IPA.hbacrule_details_facet uses IPA.sudo.enable_widget(). This
makes HBAC unnecessarily dependent on sudo. The enable_widget should be
moved to widget.js or rule.js which is shared between HBAC and sudo.
* enable_widget moved to widget.js
* rule_association_table_widget and rule_association_adder_dialog
moved to rule.js
2. The set_facet() is added to widget. I don't think we want to make
widget dependent on facet. The facet so far is only used by
IPA.sudo.enable_widget. In IPA.sudo.options_section the facet object is
passed as a parameter in spec.
Are you saying that dependency on facet in ALL widgets is bad and only
in a few is good, or in any is bad?
I think it doesn't matter if all is dependant when one is dependant.
Anyway currently all association table widgets are dependant
(association.js:386).
Btw similar topic could be: How to get current entity's pkey?'.
Dependancy on facet.get_primary_key() or
IPA.nav.get_state(that.entity.name+'-pkey') seems wrong too.
3. When updating sudo rule, in the original code the rule is
enabled/disabled after all the modifications are done. In the new code
it's reversed. I'm not sure the importance of the execution order for
this particular situation, but suppose it is, is there a way to maintain
the original order?
I was thinking about a command or widget priority. The 'mod' command
would have some default priority or it would get it from facet. The
commands would be sorted by priority before adding them to batch. This
way we can set exact order in facet update.
The order which is implemented now is reflecting the order in HBAC
details facet - there were errors when 'mod' was executed before
clearing associations.
4. The IPA.header_widget is not really a widget, it's actually part of
layout. In the current implementation a widget always corresponds to an
attribute, so it will be loaded, saved, etc. Since there is no attribute
name matching the header name currently this is not a problem, but it
can pollute the namespace.
This is part of widget nesting topic. In this manner composite_widget
isn't a proper widget too (it can correspond to several or none attribute).
Purely html rendering widgets can be separated from attribute widgets,
but it would be nice to have them because they can provide easier form
composing. Without them it would be required to override the create
method (in facet or composite_widget/section) which is sometimes better
but often it creates only code duplication with little added logic.
5. In details facet update(), instead of storing the callbacks in
update_custom_on_win and update_custom_on_fail and requiring the
subclasses to call them, it might be better to call them from the
update() itself:
var command = IPA.command({
...
on_success: function(data, text_status, xhr) {
that.on_update_success(data, text_status, xhr)
if (on_win) on_win.call(this, data, text_status, xhr);
},
on_error: function(xhr, text_status, error_thrown) {
that.on_update_error(xhr, text_status, error_thrown);
if (on_fail) on_fail.call(
this, xhr, text_status, error_thrown);
}
});
There are two types of success/error handlers:
1) the default ones in details facet
2) the ones supplied to update(win, fail) method.
In my implementation I have separated 1) from update method so if
default behaviour isn't suitable these methods can be overridden without
overriding whole update method. Overriding isn't required. This is IMHO
good (less code duplication).
6. Can IPA.batch_details_facet be combined into IPA.details_facet? I
think the base details facet should support both regular mod and batch.
The only point where they overlap is the update method. It's possible to
add a logic to switch between command creations - maybe based on
attribute (could be defined in spec). But should we do that? Isn't this
the reason for inheriting the class?
7. In sudo details page, the As Whom category is missing the RunAs
User category radio buttons.
Fixed
8. The IPA.sudo.rule_details_runas_widget uses composite widget instead
of section. They are right now functionally identical, but I suppose the
composite widget is an abstract class and the section will later contain
code specific to section.
Part of widget nesting topic. This was briefly discussed with Adam (in
that topic). The important question is what is a section? I understand
section as top level container (in facet) which contains widgets (even
composites) and semantically separates parts of the facet (with headers
and section folding...). From this point of view, composite_widget isn't
an abstract class, but it isn't used used anywhere else
Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
On 10/25/2011 7:49 AM, Petr Vobornik wrote:
2. The set_facet() is added to widget. I don't think we want to make
widget dependent on facet. The facet so far is only used by
IPA.sudo.enable_widget. In IPA.sudo.options_section the facet object is
passed as a parameter in spec.
Are you saying that dependency on facet in ALL widgets is bad and only
in a few is good, or in any is bad?
In general less dependency is better. More dependency will limit its
usage. This is inline with the goal to make purely HTML widget.
But if a widget is meant to be used only with a facet then it's
certainly ok to make it depends on facet. However, the rest should not
become unnecessarily dependent on facet too.
I think it doesn't matter if all is dependant when one is dependant.
Anyway currently all association table widgets are dependant
(association.js:386).
Let's use this as an example. The association table explicitly checks if
the facet is dirty and asks the user whether to update/reset/cancel.
Suppose we want to use this inside a dialog, it will still check whether
the current facet is dirty instead of the dialog itself, which may not
be what we want.
Btw similar topic could be: How to get current entity's pkey?'.
Dependancy on facet.get_primary_key() or
IPA.nav.get_state(that.entity.name+'-pkey') seems wrong too.
Yes. Ideally we should have a path (REST-style URL) instead of the
current parameter-based URL. We should get the pkey from that path.
3. When updating sudo rule, in the original code the rule is
enabled/disabled after all the modifications are done. In the new code
it's reversed. I'm not sure the importance of the execution order for
this particular situation, but suppose it is, is there a way to maintain
the original order?
I was thinking about a command or widget priority. The 'mod' command
would have some default priority or it would get it from facet. The
commands would be sorted by priority before adding them to batch. This
way we can set exact order in facet update.
It's possible, but managing priority could be problematic if they are
spread out, because we have to know all existing priorities before we
can add a new one. The original code uses an ordered list of commands.
The order which is implemented now is reflecting the order in HBAC
details facet - there were errors when 'mod' was executed before
clearing associations.
Right, so for certain things the order is important.
4. The IPA.header_widget is not really a widget, it's actually part of
layout. In the current implementation a widget always corresponds to an
attribute, so it will be loaded, saved, etc. Since there is no attribute
name matching the header name currently this is not a problem, but it
can pollute the namespace.
This is part of widget nesting topic. In this manner composite_widget
isn't a proper widget too (it can correspond to several or none attribute).
Purely html rendering widgets can be separated from attribute widgets,
but it would be nice to have them because they can provide easier form
composing. Without them it would be required to override the create
method (in facet or composite_widget/section) which is sometimes better
but often it creates only code duplication with little added logic.
One other solution is to split widgets into non-visual fields and purely
HTML components. Then in the facet we use separate lists for the fields
and HTML components. The fields will have a reference to the
corresponding HTML components. There can be more HTML components than
fields. But this will require a significant restructuring.
5. In details facet update(), instead of storing the callbacks in
update_custom_on_win and update_custom_on_fail and requiring the
subclasses to call them, it might be better to call them from the
update() itself:
var command = IPA.command({
...
on_success: function(data, text_status, xhr) {
that.on_update_success(data, text_status, xhr)
if (on_win) on_win.call(this, data, text_status, xhr);
},
on_error: function(xhr, text_status, error_thrown) {
that.on_update_error(xhr, text_status, error_thrown);
if (on_fail) on_fail.call(
this, xhr, text_status, error_thrown);
}
});
There are two types of success/error handlers:
1) the default ones in details facet
2) the ones supplied to update(win, fail) method.
In my implementation I have separated 1) from update method so if
default behaviour isn't suitable these methods can be overridden without
overriding whole update method. Overriding isn't required. This is IMHO
good (less code duplication).
There is still code duplication, the subclass that overrides the
on_update_success/error (#1) will still have to call
update_custom_on_win/error (#2) manually. In the changes that I proposed
you don't have to do that because #2 are called inside update().
6. Can IPA.batch_details_facet be combined into IPA.details_facet? I
think the base details facet should support both regular mod and batch.
The only point
Re: [Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules
On 10/24/2011 3:29 AM, Petr Vobornik wrote:
https://fedorahosted.org/freeipa/ticket/1515
Changes:
* Added functionality for widget composition. This allows to gather
update information from nested widgets and gradually combine it to the
facet level.
* Details' facet update method was split to several parts to allow finer
overriding in subclasses.
* Sudo rule and HBAC rule details facet was reworked to use new
functionality.
Changes in composite_widget (formal details_facet):
* added get_update_info method (for all widgets)
* modified save method
* added links to parent widget and facet
Note: cleanup of sudo options section may be part of
https://fedorahosted.org/freeipa/ticket/1690
Some issues:
1. The IPA.hbacrule_details_facet uses IPA.sudo.enable_widget(). This
makes HBAC unnecessarily dependent on sudo. The enable_widget should be
moved to widget.js or rule.js which is shared between HBAC and sudo.
2. The set_facet() is added to widget. I don't think we want to make
widget dependent on facet. The facet so far is only used by
IPA.sudo.enable_widget. In IPA.sudo.options_section the facet object is
passed as a parameter in spec.
3. When updating sudo rule, in the original code the rule is
enabled/disabled after all the modifications are done. In the new code
it's reversed. I'm not sure the importance of the execution order for
this particular situation, but suppose it is, is there a way to maintain
the original order?
4. The IPA.header_widget is not really a widget, it's actually part of
layout. In the current implementation a widget always corresponds to an
attribute, so it will be loaded, saved, etc. Since there is no attribute
name matching the header name currently this is not a problem, but it
can pollute the namespace.
5. In details facet update(), instead of storing the callbacks in
update_custom_on_win and update_custom_on_fail and requiring the
subclasses to call them, it might be better to call them from the
update() itself:
var command = IPA.command({
...
on_success: function(data, text_status, xhr) {
that.on_update_success(data, text_status, xhr)
if (on_win) on_win.call(this, data, text_status, xhr);
},
on_error: function(xhr, text_status, error_thrown) {
that.on_update_error(xhr, text_status, error_thrown);
if (on_fail) on_fail.call(
this, xhr, text_status, error_thrown);
}
});
6. Can IPA.batch_details_facet be combined into IPA.details_facet? I
think the base details facet should support both regular mod and batch.
7. In sudo details page, the As Whom category is missing the RunAs
User category radio buttons.
8. The IPA.sudo.rule_details_runas_widget uses composite widget instead
of section. They are right now functionally identical, but I suppose the
composite widget is an abstract class and the section will later contain
code specific to section.
9. It's probably better to define the proper update_info and field_info
classes rather than constructing them on the fly.
10. There are some whitespace warnings during git am.
11. The following code doesn't seem to be used:
* param_info in details.js:469
* update_command_name in details.js:597
--
Endi S. Dewata
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
