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

[Endi Sukma Dewata]

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