[Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules

[Petr Vobornik]

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

Note: the use of composite_widget in rule_details_runas_widget was 
change to rule_details_section (should be renamed to widget) to fix 7.

> 9. It's probably better to define the proper update_info and field_info
> classes rather than constructing them on the fly.

Will do.

> 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

Removed 469, used 597 (as configurable 'mod' command - for future 
framework use).

-- 
Petr Vobornik