[Freeipa-devel] [PATCH] 0018-0030, 52 webui: add support for more certificates
Pavel Vomacka
pvomacka at redhat.com
Tue Jun 14 19:41:43 UTC 2016
On 05/13/2016 06:56 PM, Petr Vobornik wrote:
> On 04/26/2016 04:23 PM, Pavel Vomacka wrote:
>> Self-NACK for patches 0027, 28, 29, 30 - used incorrect policy. I also attach
>> all patches which were not changed - it is easier to get the whole patchset.
>>
>> On 04/26/2016 02:02 PM, Pavel Vomacka wrote:
>>> I forgot to mention that my patches requires patches from :
>>> https://www.redhat.com/archives/freeipa-devel/2016-April/msg00209.html
>>>
>>>
>>> On 04/26/2016 01:33 PM, Pavel Vomacka wrote:
>>>> Hello,
>>>>
>>>> the attached patches add support for more certificates and ability to add and
>>>> remove certificates. Fixes these two tickets:
>>>> https://fedorahosted.org/freeipa/ticket/5108
>>>> https://fedorahosted.org/freeipa/ticket/5381
>>>>
>>>> These patches add ability to view, get, download, revoke, restore and delete
>>>> each certificate directly from user/host/service details page. There is also
>>>> button for adding new certificates.
>>>>
>>>> There is one known issue, that after page save action is performed some data
>>>> disappear (includes certificates). This issue has a ticket already:
>>>> https://fedorahosted.org/freeipa/ticket/5776
>>>>
>>>> --
>>>> Pavel^3 Vomacka
>>>>
> Great stuff, couple comments below.
>
> We can discuss some items in person. Not everything needs to be done.
>
> I didn't run it, just reading the code.
>
> Patch 0018:
>
> 1. Nit pick: When a value should be boolean, then following method won't
> make sure that dropdown_menu won't be e.g. an object.
> + that.dropdown_menu = spec.dropdown_menu || false;
>
> I would prefer:
> + that.dropdown_menu = !!spec.dropdown_menu;
>
> Which retypes it to boolean. If default should be true (not this case) then:
> that.dropdown_menu = spec.dropdown_menu !== undefined ?
> !!spec.dropdown_menu : true;
>
> Also the interface is very specific. It says that the child widget will
> have dropdown menu. What if the actions won't be in dropdown menu but,
> e.g., some overlay menu.
>
> Imho the interface should be:
>
> that.custom_actions = !!spec.custom_actions;
>
> Than the child object would have define,e.g., :
>
> action_object get_custom_actions()
>
> Interface of action_object would be e.g.:
> get_items()
> set_items(items)
> enable_item(name)
> disable_item(name)
>
> Dropdown menu would have to define these methods.
>
> Patch 0019:
>
> 1. Shouldn't disable_item or enable_item automatically rerender the items?
>
> 2. The rerender, used in later patches. Imo it should do only:
>
> if (this.ul_node) {
> construct.empty(this.ul_node);
> this._render_items(this.items);
> }
>
> Or just re-render the one item.
> $( "li[data-name=" + item.name +"]", this.ul_node ).replaceWith(
> this._render_item(item));
>
> 3. in future for loops write:
> for (var i=0, l=this.items.length; i<l; i++) {
> instead of
> for (var i=0; i<this.items.length; i++) {
>
> It's defensive style when you don't know if computing length is O(1) or
> O(n) or how big is n. In this instance it doesn't probably matter.
>
> Patch 0020:
>
> 1. Working widgets inherits from IPA.widget so why do you redefine
> 'name', 'facet', 'entity'
>
> 2. why to you introduce 'base_cls' and 'other_cls' when there are
> 'base_css_class' and 'css_class'?
>
> 3. typo in comment 'opaciti'
>
> 4. We can ignore IE < 9, I'm not sure if you avoid using some features
> because of IE9.
>
> 5. _normalize_bg_color_css could be moved to util.js But I wonder if we
> need it. rgba is supported in 95% of browsers and definitely in all
> supported by IPA (chrome, firefox).
>
> Patch 0021:
>
> 1. The original code is not great - RPC code should be agnostic to
> display layer. Ideally it would not call methods like
> IPA.hide_activity_icon(); and IPA.display_activity_icon();
>
> I would rather avoid adding more of such things there and would refactor
> now, when it is easy.
>
> IMO the rpc class could use Evented mixin and have:
> 4 local events: start, end, success, error
> 2 global topics: rpc-start, rpc-end
>
> Local events will be new - will replace the notify_activity_start and
> notify_activity_end. For convenience, spec object could accept a
> handlers in similar fashion as now. The handlers would be registered to
> the events.
>
> Global topics would replace current IPA.hide_activity_icon(); and
> IPA.display_activity_icon();
>
> App class would subscribe to the global events and to stuff (hide|show)
> accordingly. Example of such subscription:
> topic.subscribe('phase-error', lang.hitch(this, this.on_phase_error));
>
> Then hide_activity_icon could be something like notify_global with
> default to true which can be suppressed by setting it to false.
>
> What do you think?
>
> Patch 0022:
>
> 1. Leftover line:
> var sn = record[i].serial_number;
>
> Patch 0023:
>
> 1. move the css code from ipa.css to layout.less?
>
> 2. Did you mean "display: table"?
> display: cert-table;
>
> 3. Why are create_layout, create_row and create_cell private methods of
> create_content? It would make sense to create a table dialog mixin
> (similar as confirm mixin) and use the same code in all cert dialogs(if
> applicable).
>
> 4. 'table-head' class is used in every cell. Why is it call 'head' when
> it is in fact in all cells?
>
> Patch 0024:
>
> Will require changes described in patch 21(rpc).
>
> 1. should the methods have rather params: serial_number and
> revocation_reason, instead of certificate(maybe ok) and dialog(not ok)?
> That way it would be more general/reusable.
>
> Patch 0025: ACK
>
> Patch 0026:
>
> 1. Add link use exactly the same code as in multivalued_widget. The code
> should be moved to method: create_add_link(container) and then reused.
> new_row would be overridedn and it would call open_addcert_dialog or
> open_addcert_dialog renamed to new_row.
>
> 2. it can have a spec default:
> spec.dropdown_menu = spec.dropdown_menu !== undefined ? true :
> spec.dropdown_menu;
>
> And then it doesn't have to be defined in patches 28, 29, 30.
>
>
> Patch 0027:
>
> 1. Why is download action name a 'that.data_url' ?
>
> 2. the dropdown_menu -> custom_actions change
>
> 3. "that.certificate &&" is not required it will be always true
> that.certificate = $.extend({}, certificate);
>
> if (that.certificate && that.certificate.revoked !== 'undefined') {
> that.get_cert_revoke_status();
> }
>
> 4. update_link is weird name for a method which updates data in a table
>
> 5. "that.update_link = function(r)", "r" shouldn't be there
>
> 6. Following doesn't look future proof:
> """
> that.get_cert_revoke_status = function() {
> // Double check whether the certificate is issued by our CA.
> var issuer = 'CN=' + that.ipa_issuer_commonname + ',' +
> that.ipa_issuer_subjbase;
> if (that.certificate.issuer !== issuer) return;
> """
> Would check it with Fraser
>
> 7. in handle_revocation_reason, useless call:
> var dd_menu = that.get_dropdown_menu();
>
> 8. in compose_dialog_title, there is a check for `cert.subject` if true
> it sets `cn`, `o`. But if false then the values are `undefined` which
> are then used in the r.replace calls. Is it intended?
>
> Patch 0028:
> Looks OK, "dropdown_menu: true" doesn't have to be there as mentioned
> earlier
>
> Will test later.
>
> Patch 0029:
>
> 1. What about extending value_state_evaluator with adapter and param
> property. So that it would use standard adapter by default and every
> child could use it's own? IMO other parts of UI could use it as well.
> The reason why it doesn't use adapter already is that adapters were
> introduced later.
>
> It would simplify both definitions + the definitions in patch 30.
>
> Patch 0030:
>
> Same as patch 0029.
Thank you for the review. I fixed everthing, only rpc code isn't
changed. I will do it later in separate patch.
These patches requires jcholast's patches 551.2, 552.2, 623, 624 (which
require ftweedal's Sub-CA patches) and WebUI patches for Sub-CAs
(because of download functionality).
Edited and rebased patches are attached.
--
Pavel^3 Vomacka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0018-2-Add-support-for-custom-menu-in-multivalued-widget.patch
Type: text/x-patch
Size: 4759 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0019-2-Extends-functionality-of-DropdownWidget.patch
Type: text/x-patch
Size: 2149 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0020-2-Add-working-widget.patch
Type: text/x-patch
Size: 3236 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0021-2-Add-ability-to-turn-off-activity-icon.patch
Type: text/x-patch
Size: 3385 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0022-2-Add-Object-adapter.patch
Type: text/x-patch
Size: 2275 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0023-2-Refactored-certificate-view-and-remove-hold-dialog.patch
Type: text/x-patch
Size: 15981 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0024-2-Changed-the-way-how-to-handle-remove-hold-and-revoke.patch
Type: text/x-patch
Size: 4884 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0025-2-Remove-old-useless-actions-get-and-view.patch
Type: text/x-patch
Size: 1158 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0026-2-Add-widget-for-showing-multiple-certificates.patch
Type: text/x-patch
Size: 11403 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0027-3-Add-certificate-widget.patch
Type: text/x-patch
Size: 17600 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0028-3-Add-new-certificates-widget-to-the-user-details-page.patch
Type: text/x-patch
Size: 2411 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0029-3-Add-new-certificates-widget-to-the-host-details-page.patch
Type: text/x-patch
Size: 5709 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0030-3-Add-new-certificates-widget-to-the-service-details-p.patch
Type: text/x-patch
Size: 3537 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0052-Updated-certificates-table.patch
Type: text/x-patch
Size: 1546 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160614/99a778e5/attachment-0013.bin>
More information about the Freeipa-devel
mailing list