On 06/30/2016 05:27 PM, Petr Vobornik wrote:
On 06/30/2016 02:48 PM, Pavel Vomacka wrote:
Hello,

please review these patches. First two patches fix two minor bugs in
custom_command_multivalued_widget.

The rest of patches add webui for kerberos aliases.

https://fedorahosted.org/freeipa/ticket/5927

A preliminary review - I only read the code.

Patch 0067: LGTM,

btw same wrong interface is in on_error_add, but there it is not use
fixed for on_error_add as well.

Patch 0068: lGTM

Patch 0069:

1. A nitpick, not necessarily a NACK.
krb_custom_command_multivalued_widget should be named e.g.
krb_principal_multivalued_widget.

2. Doc texts for the new widges are missing. This can be added later.


3. `!principal_name || principal_name === '')` is the same as
`!principal_name`

so

var principal_name = value[0];

         if (!principal_name || principal_name === '') {
             principal_name = {};
         }

could be simplified into
   var principal_name = value[0] || {};

but why is an object set into that.principal_name when it is later used
as a text: `that.principal_text.text(that.principal_name);`

fixed
Patch 0070: LGTM

Patch 0071: LGTM

Patch 0072: LGTM if the change of krbprincipalname to krbcanonicalname
is good.


Updated patches attached.

--
Pavel^3 Vomacka

From 27d3d1b23a58de9fb8133ea7ad7102dc8f4118df Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Thu, 30 Jun 2016 14:32:27 +0200
Subject: [PATCH 1/6] Change error handling in
 custom_command_multivalued_widget

The custom_command_multivalued_widget now handles remove and add commands errors
correctly and shows error message.

Part of: https://fedorahosted.org/freeipa/ticket/5381

add_error
---
 install/ui/src/freeipa/widget.js | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 0972efadb46ac7b5811f4e072121a448618fe434..e3c2dc555df9dbebdea62e19ce445cae4d60c8f7 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1533,8 +1533,11 @@ IPA.custom_command_multivalued_widget = function(spec) {
     /**
      * Called on error of add command. Override point.
      */
-    that.on_error_add = function(data) {
-        that.adder_dialog.focus_first_element();
+    that.on_error_add = function(xhr, text_status, error_thrown) {
+        if (error_thrown.message) {
+            var msg = error_thrown.message;
+            IPA.notify(msg, 'error');
+        }
     };
 
     /**
@@ -1548,8 +1551,11 @@ IPA.custom_command_multivalued_widget = function(spec) {
     /**
      * Called on error of remove command. Override point.
      */
-    that.on_error_remove = function(data) {
-        IPA.notify(data.result.summary, 'error');
+    that.on_error_remove = function(xhr, text_status, error_thrown) {
+        if (error_thrown.message) {
+            var msg = error_thrown.message;
+            IPA.notify(msg, 'error');
+        }
     };
 
     /**
-- 
2.5.5

From f52a0e5561497e0a86baa149eb7aa949f7b04c07 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Thu, 30 Jun 2016 14:34:33 +0200
Subject: [PATCH 3/6] Add widgets for kerberos aliases

Create own custom_command_multivalued_widget for kerberos aliases.

https://fedorahosted.org/freeipa/ticket/5927
---
 install/ui/src/freeipa/widget.js   | 108 +++++++++++++++++++++++++++++++++++++
 install/ui/test/data/ipa_init.json |   6 +++
 ipaserver/plugins/internal.py      |   6 +++
 3 files changed, 120 insertions(+)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index d6c1672ad614d13642fe4c9e3dcef55d458bcd7d..c365a8c4f83a039f0e9b2d454e2d9ab1d8fc0772 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1796,6 +1796,110 @@ IPA.custom_command_multivalued_widget = function(spec) {
     return that;
 };
 
+/**
+ * Multivalued widget which is used for working with kerberos principal aliases.
+ *
+ * @class
+ * @extends IPA.custom_command_multivalued_widget
+ */
+IPA.krb_principal_multivalued_widget = function (spec) {
+
+    spec = spec || {};
+
+    spec.adder_dialog_spec = spec.adder_dialog_spec || {
+        title: '@i18n:krbaliases.adder_title',
+        fields: [
+            {
+                $type: 'text',
+                name: 'krbprincalname',
+                label: '@i18n:krbaliases.add_krbal_label'
+            }
+        ]
+    };
+
+    var that = IPA.custom_command_multivalued_widget(spec);
+
+    that.create_remove_dialog_title = function(row) {
+        return text.get('@i18n:krbaliases.remove_title');
+    };
+
+    that.create_remove_dialog_message = function(row) {
+        var message = text.get('@i18n:krbaliases.remove_message');
+        message = message.replace('${alias}', row.widget.principal_name);
+
+        return message;
+    };
+
+
+    that.create_remove_args = function(row) {
+        var pkey = that.facet.get_pkey();
+        var krbprincipalname = row.widget.principal_name;
+        krbprincipalname = [ krbprincipalname ];
+
+        var args = [
+            pkey,
+            krbprincipalname
+        ];
+
+        return args;
+    };
+
+    that.create_add_args = function(row) {
+        var pkey = that.facet.get_pkey();
+        var krbprincipalname = that.adder_dialog.get_field('krbprincalname').value;
+
+        var args = [
+            pkey,
+            krbprincipalname
+        ];
+
+        return args;
+    };
+
+    return that;
+};
+
+/**
+ * Widget which is used as row in kerberos aliases multivalued widget.
+ * It contains only string where is the principal alias name and delete button.
+ *
+ * @class
+ * @extends IPA.input_widget
+ */
+IPA.krb_principal_widget = function(spec) {
+    spec = spec || {};
+
+    var that = IPA.input_widget();
+
+    that.create = function(container) {
+        that.widget_create(container);
+
+        that.principal_text = $('<span />', {
+            'class': 'krb-principal-name',
+            text: ''
+        }).appendTo(container);
+
+        if (that.undo) {
+            that.create_undo(container);
+        }
+
+        that.create_error_link(container);
+    };
+
+    that.update = function(value) {
+
+        var principal_name = value[0] || '';
+
+        that.principal_name = principal_name;
+        that.update_text();
+    };
+
+    that.update_text = function() {
+        that.principal_text.text(that.principal_name);
+    };
+
+    return that;
+};
 
 /**
  * Option widget base
@@ -7051,6 +7155,10 @@ exp.register = function() {
     w.register('multivalued', IPA.multivalued_widget);
     w.register('custom_command_multivalued',
         IPA.custom_command_multivalued_widget);
+    w.register('krb_principal_multivalued',
+                            IPA.krb_principal_multivalued_widget);
+    w.register('krb_principal',
+                            IPA.krb_principal_widget);
     w.register('password', IPA.password_widget);
     w.register('radio', IPA.radio_widget);
     w.register('select', IPA.select_widget);
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index 8768766ed883ea438c422b4286bb73eb757f2c5f..77d6fce4e9ca0cf281d89e09f803d6a1a81c6870 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -170,6 +170,12 @@
                         "remove_create": "Disallow ${other_entity} to create keytab of ${primary_key}",
                         "remove_retrieve": "Disallow ${other_entity} to retrieve keytab of ${primary_key}"
                     },
+                    "krbaliases": {
+                        "adder_title": "Add Kerberos Principal Alias",
+                        "add_krbal_label": "New kerberos principal alias",
+                        "remove_title": "Remove Kerberos Alias",
+                        "remove_message": "Do you want to remove kerberos alias ${alias}?"
+                    },
                     "krbauthzdata": {
                         "inherited": "Inherited from server configuration",
                         "mspac": "MS-PAC",
diff --git a/ipaserver/plugins/internal.py b/ipaserver/plugins/internal.py
index 5eee7572eec3e610609cd4d3046413dc30c105ba..ff29262180a967b2611db17271a742c5e472a19f 100644
--- a/ipaserver/plugins/internal.py
+++ b/ipaserver/plugins/internal.py
@@ -319,6 +319,12 @@ class i18n_messages(Command):
             "remove_create": _("Disallow ${other_entity} to create keytab of ${primary_key}"),
             "remove_retrieve": _("Disallow ${other_entity} to retrieve keytab of ${primary_key}"),
         },
+        "krbaliases": {
+            "adder_title": _("Add Kerberos Principal Alias"),
+            "add_krbal_label": _("New kerberos principal alias"),
+            "remove_title": _("Remove Kerberos Alias"),
+            "remove_message": _("Do you want to remove kerberos alias ${alias}?"),
+        },
         "krbauthzdata": {
             "inherited": _("Inherited from server configuration"),
             "mspac": _("MS-PAC"),
-- 
2.5.5

From cf323d8a49765250712c774072961ba72e660c94 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Thu, 30 Jun 2016 14:12:49 +0200
Subject: [PATCH 4/6] Add widget for kerberos aliases to user page

https://fedorahosted.org/freeipa/ticket/5927
---
 install/ui/src/freeipa/user.js | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/user.js b/install/ui/src/freeipa/user.js
index d8d22ffbc67adec39a75009cd4de0ebc6500b03c..0bebf443a78901ccad1e271b3bba249e2e1b66e0 100644
--- a/install/ui/src/freeipa/user.js
+++ b/install/ui/src/freeipa/user.js
@@ -187,7 +187,14 @@ return {
                         },
                         'uidnumber',
                         'gidnumber',
-                        'krbprincipalname',
+                        {
+                            $type: 'krb_principal_multivalued',
+                            name: 'krbprincipalname',
+                            item_name: 'principal',
+                            child_spec: {
+                                $type: 'krb_principal'
+                            }
+                        },
                         {
                             $type: 'datetime',
                             name: 'krbprincipalexpiration'
-- 
2.5.5

From 2f1b659625734c1d67580b8af1dde937d21912ed Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Thu, 30 Jun 2016 14:13:33 +0200
Subject: [PATCH 5/6] Add widget for kerberos aliases to hosts page

https://fedorahosted.org/freeipa/ticket/5927
---
 install/ui/src/freeipa/host.js | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/host.js b/install/ui/src/freeipa/host.js
index ba8d0f2a5a7e9903bd8f810e6bd23125a0ec2cc5..33d443c2bc96c385bd13abf4d85adda6e51db718 100644
--- a/install/ui/src/freeipa/host.js
+++ b/install/ui/src/freeipa/host.js
@@ -88,7 +88,14 @@ return {
                             name: 'fqdn',
                             other_entity: 'dnsrecord'
                         },
-                        'krbprincipalname',
+                        {
+                            $type: 'krb_principal_multivalued',
+                            name: 'krbprincipalname',
+                            item_name: 'principal',
+                            child_spec: {
+                                $type: 'krb_principal'
+                            }
+                        },
                         {
                             $type: 'textarea',
                             name: 'description'
-- 
2.5.5

From 117f38381244bd7cc02be74adb14b0f0c9aa2acf Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Thu, 30 Jun 2016 14:13:53 +0200
Subject: [PATCH 6/6] Add widget for kerberos aliases to service page

Also changes the name of option which is send during adding new service from
'krbprincipalname' to 'krbcanonicalname'.

https://fedorahosted.org/freeipa/ticket/5927
---
 install/ui/src/freeipa/service.js | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/install/ui/src/freeipa/service.js b/install/ui/src/freeipa/service.js
index a9a4c1bcb91ede1d388014a80749d215a8840c12..35d486605ebfee41d8b3ffa5bb77bf9e72a60c01 100644
--- a/install/ui/src/freeipa/service.js
+++ b/install/ui/src/freeipa/service.js
@@ -58,7 +58,7 @@ return {
     facets: [
         {
             $type: 'search',
-            columns: [ 'krbprincipalname' ]
+            columns: [ 'krbcanonicalname' ]
         },
         {
             $type: 'details',
@@ -67,7 +67,14 @@ return {
                 {
                     name: 'details',
                     fields: [
-                        'krbprincipalname',
+                        {
+                            $type: 'krb_principal_multivalued',
+                            name: 'krbprincipalname',
+                            item_name: 'principal',
+                            child_spec: {
+                                $type: 'krb_principal'
+                            }
+                        },
                         {
                             name: 'service',
                             label: '@i18n:objects.service.service',
@@ -435,14 +442,14 @@ IPA.service_adder_dialog = function(spec) {
 
     var init = function() {
 
-        //small hack - krbprincipalname should not be displayed. This way
+        //small hack - krbcanonicalname should not be displayed. This way
         //creation of associated widget is skipped.
         //In future it would be better split section definion into widget and
         //fields definition and create custom field with two associated
         //widgets - 'service' and 'host' with this dialog's save logic.
         that.builder.build_field({
             $type: 'field',
-            name: 'krbprincipalname',
+            name: 'krbcanonicalname',
             required: false
         });
     };
@@ -455,7 +462,7 @@ IPA.service_adder_dialog = function(spec) {
         field = that.fields.get_field('host');
         var host = field.save()[0];
 
-        record['krbprincipalname'] = [ service+'/'+host ];
+        record['krbcanonicalname'] = [ service+'/'+host ];
 
         field = that.fields.get_field('force');
         record['force'] = field.save();
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to