WebUI: Certificate Mapping #400

pvomacka · 2017-01-18T08:44:51Z

Add WebUI for certificate mapping

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

flo-renaud · 2017-01-19T14:16:29Z

install/ui/src/freeipa/plugins/certmap.js

+
+        return options;
+    };
+


Hi @pvomacka,
thanks for the patch. Would it be possible to have a specific Add dialog window, which would allow to add Certificate mapping data with different input formats? Currently, the dialog takes the input and calls the equivalent of user-add-certmap --data, but it would also be interesting to allow the user to provide a certificate and call user-add-certmap --certificate, or allow the user to provider a subject + issuer and call user-add-certmap --subject --issuer.

Hi @flo-renaud,
Yes, it is possible and not hard to implement. I can add it.

@flo-renaud After some thinking I would rather remove --issuer, --subject and --certificate from user-add-certmap command. I don't like the fact that single command does several different things and I think it's confusing.
I think the idea is great but I don't know how to do it in straightforward (for the user) way.

@dkupka @flo-renaud I would say that it would be kind of hard (especially in CLI) for users to understand which options should be used together and which can't be used together. From point of view of WebUI, there is no problem to create dialog, which allows only correct combinations of options.

pvoborni

I only clicked through Pavel's testing vm and read code. The usability issues like missing examples can be handled in separate PR.

I'd let @flo-renaud or @dkupka to test if UI matches expectation from design.

There are some nitpicks and question about field in adder dialog withou server counterpar, it's up to Pavel if he sees it as something which should block this PR.

pvoborni · 2017-02-17T09:50:33Z

install/ui/src/freeipa/widget.js

+    /**
+     * Prefix of CSS class of each row.
+     */
+    that.data_name = spec.data_name || 'default';


Nitpick: Instead of 'default' should it be rather something like 'non-editable' ? Later when use in class "default-data" is too general and doesn't even add any context to the widget type.

pvoborni · 2017-02-17T09:53:30Z

install/ui/src/freeipa/field.js

+            // WebUI.
+            if (that.metadata.flags &&
+                array.indexOf(that.metadata.flags, 'no_update') > -1 &&
+                that.widget && !that.widget.always_writable) {


Should the widget reference really be part of field? Why not add the always_writable override to field itself and inherit it in widget?

pvoborni · 2017-02-17T10:57:22Z

install/ui/src/freeipa/plugins/certmap.js

+    that.create_add_command = function(record) {
+        var command = that.entity_adder_dialog_create_add_command(record);
+
+        command.set_option('ipaenabledflag', true);


I wonder if we could do such stuff declaratively and thus avoid overriding the method. For this PR this approach is OK.

pvoborni · 2017-02-17T11:02:22Z

install/ui/src/freeipa/plugins/certmap.js

+        };
+
+        that.adder_dialog = certmap.custom_adder_dialog(spec);
+        that.adder_dialog.create_button({


Could we use confirmation mixin for the adder dialog? So that you would have to define the buttons? Or if it is not possible then define a dialog class for custom_command_multivalued_widget

pvoborni · 2017-02-17T11:13:43Z

install/ui/src/freeipa/navigation/menu_spec.js

-                { entity: 'radiusproxy' }
+                { entity: 'radiusproxy' },
+                {
+                    entity: 'certmaprule',


Maybe more a question for UX person: is the position of the menu item on good place? Right now the order is:

Certificates

OTP Tokens

RADIUS Servers

Certificate Identity Mapping Rules

Following order would be more natural to me:

Certificates

Certificate Identity Mapping Rules

OTP Tokens

RADIUS Servers

But I don't insist on it.

pvoborni · 2017-02-17T11:15:40Z

install/ui/test/data/ipa_init.json

@@ -315,6 +315,18 @@
                            "view_certificate": "Certificate for ${entity} ${primary_key}",
                            "view_certificate_btn": "View Certificate"
                        },
+                        "certmap": {


If I'm correct, some of your other PRs no longer update this file. We've talk to remove the offline mode. So what about removing it? Or is this still used for unit tests?

Yes, you're right. I will remove this change from current PR and will create new PR for removing offline WebUI.

pvoborni · 2017-02-17T11:17:34Z

install/ui/src/freeipa/plugins/certmap.js

+        $factory: certmap.rule_adder_dialog,
+        fields: [
+            'cn',
+            'ipacertmapissuer',


In your testing vm, ipacertmapissuer option doesn't exist.

pvoborni · 2017-02-17T11:20:32Z

install/ui/src/freeipa/plugins/certmap.js

+                        },
+                        'ipacertmapissuer',
+                        'ipacertmapmaprule',
+                        'ipacertmapmatchrule',


Would be good if mapping and matching rules had examples written near the fields. Without reading any docs I don't have a clue what should be written there. Same in details page.

pvoborni · 2017-02-17T11:26:55Z

install/ui/src/freeipa/user.js

@@ -218,6 +218,15 @@ return {
                            label: '@i18n:objects.cert.certificates'
                        },
                        {
+                            $type: 'certmap_multivalued',


Would be good if it had the question mark hint to describe what is the purpose of this field. What problem can be solved by it. I believe it is not self explanatory.

pvoborni · 2017-02-17T11:28:38Z

install/ui/src/freeipa/plugins/certmap.js

+                },
+                {
+                    name: 'subject'
+                }


Example in issuer and subject would be also helpful.

flo-renaud

Hi @pvomacka
Thanks for the GUI!
I have inline comments related to late design changes.
Apart from that, it may be a bug in the browser but when I open a user details page, and the user doesn't have any cert map yet, the Add button is not displayed.

flo-renaud · 2017-02-23T09:14:50Z

install/ui/src/freeipa/plugins/certmap.js

+                            $type: 'textarea',
+                            name: 'description'
+                        },
+                        'ipacertmapissuer',


You can remove the ipacertmapissuer attribute as the new design doesn't define this attr any more.

flo-renaud · 2017-02-23T09:21:44Z

install/ui/src/freeipa/plugins/certmap.js

+        },
+        {
+            $type: 'multivalued',
+            name: 'usercertificate',


this should be 'certificate'

flo-renaud · 2017-02-23T09:23:15Z

install/ui/src/freeipa/plugins/certmap.js

+                {
+                    name: 'data',
+                    label: '@i18n:objects.certmap.data_label',
+                    fields: ['ipacertmapdata', 'usercertificate'],


This should be 'certificate'

flo-renaud · 2017-02-23T09:23:36Z

install/ui/src/freeipa/plugins/certmap.js

+                },
+                {
+                    $type: 'multivalued',
+                    name: 'usercertificate',


This should be 'certificate'

pvomacka · 2017-02-27T17:38:19Z

Hello @flo-renaud and @pvoborni

thank you for reviews, all proposed changes are done in last commits, please look at them. Thank you very much.

flo-renaud · 2017-02-28T09:34:44Z

Hi @pvomacka
Thank you for the updated PR.
I probably wongly advised you to replace 'usercertificate' with 'certificate' in one extra place where it was not needed, because now the "Certificates" field of the user details page does not display any more the full certificates. My bad...
Apart from that, everything works as expected. Thanks!

pvomacka · 2017-02-28T13:58:29Z

Hi @flo-renaud
Thank you for review.

The issue about certificates is different and here is the fix: #519

pvomacka · 2017-03-02T14:05:32Z

In last update I changed just line 33 in certmap.js file.

flo-renaud · 2017-03-03T10:46:15Z

Hi @pvomacka
thank you, LGTM.

pvoborni

The space needs to be removed. Otherwise it's good.

pvoborni · 2017-03-07T16:31:08Z

install/ui/src/freeipa/widget.js

 *
 * @class
 * @extends IPA.input_widget
 */
-IPA.krb_principal_widget = function(spec) {
+ IPA.non_editable_row_widget = function(spec) {


Remove leading space.

If field will have set attribute 'always_writable' to true, then 'no_update' flag will be ingored. Used in command user-{add,remove}-certmap which needs to be writable in WebUI and also needs to be omitted from user-mod command. Part of: https://fedorahosted.org/freeipa/ticket/6601

Old krb-principal widget is changed to general one. And used also for ipacertmapdata in user. This widget make every line non-editable. Part of: https://fedorahosted.org/freeipa/ticket/6601

Adder dialog which is used along with custom_command_multivalued_widget. It behaivor of confirm dialog and adds fields which are necessary. Part of: https://fedorahosted.org/freeipa/ticket/6601

Add facets for certmaprule and certmapconfigure entities. https://fedorahosted.org/freeipa/ticket/6601

pvomacka · 2017-03-07T17:31:19Z

@pvoborni Thanks for review. I removed the space :)

tkrizek · 2017-03-08T09:15:30Z

master:

27027bb WebUI: Add possibility to set field always writable
fba318b WebUI: Create non editable row widget for mutlivalued widget
d370027 WebUI: Add Custom command multivalued adder dialog
19426f3 WebUI: Add certmap module

flo-renaud reviewed Jan 19, 2017

View reviewed changes

pvomacka force-pushed the t6601 branch from c42f95e to 65661ec Compare February 15, 2017 09:46

pvoborni reviewed Feb 17, 2017

View reviewed changes

flo-renaud reviewed Feb 23, 2017

View reviewed changes

pvomacka force-pushed the t6601 branch from 65661ec to 8b66d4d Compare February 27, 2017 17:36

pvomacka force-pushed the t6601 branch from 8b66d4d to ad8d4fc Compare March 2, 2017 14:03

pvoborni requested changes Mar 7, 2017

View reviewed changes

Pavel Vomacka added 4 commits March 7, 2017 18:26

WebUI: Create non editable row widget for mutlivalued widget

ef26ff2

Old krb-principal widget is changed to general one. And used also for ipacertmapdata in user. This widget make every line non-editable. Part of: https://fedorahosted.org/freeipa/ticket/6601

WebUI: Add Custom command multivalued adder dialog

c4bca41

Adder dialog which is used along with custom_command_multivalued_widget. It behaivor of confirm dialog and adds fields which are necessary. Part of: https://fedorahosted.org/freeipa/ticket/6601

WebUI: Add certmap module

e2f011e

Add facets for certmaprule and certmapconfigure entities. https://fedorahosted.org/freeipa/ticket/6601

pvomacka force-pushed the t6601 branch from ad8d4fc to e2f011e Compare March 7, 2017 17:28

pvomacka mentioned this pull request Mar 7, 2017

WebUI: certmap match #549

Closed

pvoborni approved these changes Mar 8, 2017

View reviewed changes

pvoborni added the ack Pull Request approved, can be merged label Mar 8, 2017

tkrizek added the pushed Pull Request has already been pushed label Mar 8, 2017

tkrizek closed this Mar 8, 2017

WebUI: Certificate Mapping #400

WebUI: Certificate Mapping #400

pvomacka commented Jan 18, 2017 •

edited by tkrizek

flo-renaud Jan 19, 2017

pvomacka Jan 24, 2017

ghost Jan 24, 2017

pvomacka Jan 24, 2017

pvoborni left a comment

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

pvomacka Feb 21, 2017

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

pvoborni Feb 17, 2017

flo-renaud left a comment

flo-renaud Feb 23, 2017

flo-renaud Feb 23, 2017

flo-renaud Feb 23, 2017

flo-renaud Feb 23, 2017

pvomacka commented Feb 27, 2017

flo-renaud commented Feb 28, 2017

pvomacka commented Feb 28, 2017

pvomacka commented Mar 2, 2017

flo-renaud commented Mar 3, 2017

pvoborni left a comment

pvoborni Mar 7, 2017

pvomacka commented Mar 7, 2017

tkrizek commented Mar 8, 2017

WebUI: Certificate Mapping #400

WebUI: Certificate Mapping #400

Conversation

pvomacka commented Jan 18, 2017 • edited by tkrizek

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvoborni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flo-renaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvomacka commented Feb 27, 2017

flo-renaud commented Feb 28, 2017

pvomacka commented Feb 28, 2017

pvomacka commented Mar 2, 2017

flo-renaud commented Mar 3, 2017

pvoborni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvomacka commented Mar 7, 2017

tkrizek commented Mar 8, 2017

pvomacka commented Jan 18, 2017 •

edited by tkrizek