WebUI: Vault Management #139

pvomacka · 2016-10-05T16:04:18Z

Add vault management into WebUI, there are some constraints:

There is no crypto library so Symmetric and Assymetric vaults
are not supported in WebUI. Also retrieving or archiving data
is not supported.
There aren't any container support right now

Supported is:

Browsing vaults
Adding Standard vaults (users, service, shared)
Removing vaults
Adding and removing owners
Adding and removing members

MartinBasti · 2016-10-06T14:24:20Z

I'm not sure if this is done on purpose, but Vault section is shown there even I have no KRA installed in topology, and I'm getting error

An error has occurred (IPA Error 3000: InvocationError)

KRA service is not enabled

It is not nice, IMO some placeholder pointing to ipa-kra-install could be better

MartinBasti · 2016-10-06T15:10:32Z

ipaserver/plugins/internal.py

+                "shared": _("Shared"),
+                "shared_vaults_title": _("Shared Vaults"),
+                "standard_type": _("Standard"),
+                "type_tooltip": _("Only standard vaults can be created in \


You dont need \ there because string is inside (), please use:

"first " "second"

pvoborni · 2016-10-06T15:25:23Z

For other optional UIs like CA/Trusts or DNS, Web UI checks on UI start if the component is installed by batch command with:

{method: "env", params: [[], {}]}
{method: "dns_is_enabled", params: [[], {}]}
{method: "trustconfig_show", params: [[], {}]}
{method: "domainlevel_get", params: [[], {}]}
{method: "ca_is_enabled", params: [[], {}]}

For KRA, it can add kra_is_enabled command.

Traditionally, UI is hidden if component is not installed.

MartinBasti · 2016-10-06T16:29:57Z

I created shared vault, but I cannot see it in 'Shared Vaults', it is show only in 'My Vaults'
i.e. it was created ad user vault according CLI

'My Vaults' I expected that it will show all vaults created by me, but it is not true, it shows only my user vaults. Can we set name to more explicit, like 'My User Vaults' or is it too much and only I'm dumb?

I broke it, I cannot add vault, adder dialog just show and instantly disappears

Steps to reproduce:
a. click on add vault
b. click on vault user, mark empty line (dont click)
c. press ESC (dialog should disappear)
d. click on add Vault again, it should not work
e. dialog suddenly shows when you click on My Vaults

No errors in browser console. What could cause this?

Can you please add tests for this?

Nitpick
If you add vault from Service vault, then predefined value should be service vault in adder dialog.
Same for shared vault

Missing 'type' column in my vaults

For symmetric vault, there is 'salt' shown in CLI, and I can change this in CLI. IMO this should be supported in webUI too

For asymetric vault, public key is show in CLI, and user can also change this public key, IMO this should work in webUI too.

I would like to see big fat warning in adder dialog that content of 'standard' vaults can be seen by users with higher privileges (admins). This is the reason why we set symmetric vault as default in CLI. But because in webUI the standard vault is the only one vault that can be added, we should inform users to use rather CLI and create symmetric vault

IMO we should add this warning into CLI too

Vaultconfig-show shows transport certificate, should we shown this in webUI as well?

MartinBasti · 2016-10-06T16:37:46Z

Yeah, and I forgot to write:

There should be an information in webUI, that secrets can be added/retrieved to vault only by using vault-archive and vault-retrieve from CLI

MartinBasti · 2016-10-06T16:42:49Z

Please disregard comment 1), it works (sorry :( my fault)

pvomacka · 2016-10-25T15:07:23Z

@mbasti-rh
2) fixed
3) I filled a ticket: https://fedorahosted.org/freeipa/ticket/6388
4) Tests added
5) Fixed
6) Fixed
7) Salt added
8) Field for public key added
9) Warning added
10) Transport certificate is now visible in WebUI
11) Information added into adder dialog

The issue with showing error in case that KRA is not installed is also fixed.

pvomacka · 2016-10-26T07:34:07Z

Fixed PEP8 errors.

MartinBasti · 2016-11-22T17:33:05Z

NACK

view My User Vaults/Add vault
There is no marked radio button and all fields are shown which are mutual exclusive. A one option from radio group should be marked. It is doing fancy things when no radiobutton is marked and you fill other fields and press add.

Vault config view shows only one server, not list of all KRA servers installed

I'm quite puzzled wit behavior User Vaults and My User Vaults executes following commands

User Vaults:  vault-find --users --pkey-only
My Users Vaults: vault-find

So how does it actually works? What My User Vault do then? I would expect a filter in that command and users flag. Also why in one case was called command with --pkey-only and not in second time?

pvomacka · 2016-12-12T09:18:51Z

@mbasti-rh Thank you for review.

Fixed
Fixed
Additionally I fixed two more issues which I found during testing. The tables on User Vaults or Service Vaults didn't show different (different users/services) vaults which have the same name - FIXED now.
The second issue was that when i.e. admin add user/share vault on service page, the search page with new vault was not refreshed - FIXED now.
My User Vaults calls vault-find which returns all Vaults which have currently logged user is in Owners or Members group. It is called without --pkey-only, because we want to get also information about vault type in response.
User Vaults shows all user vaults (of all users) and there is --pkey-only because we call vault-show for each user vault which is returned and in each vault-show response we get all (and several more) information which are also in vault-find (without --pkey-only). So, we don't need to transfer data (those parts which are in both responses) twice.

I understand that the difference between those two sections could not be very clear. If you have any idea on how to improve this feel free to put a comment here or open a ticket.

MartinBasti · 2016-12-12T11:56:00Z

I understand that the difference between those two sections could not be very clear. If you have any idea on how to improve this feel free to put a comment here or open a ticket.

I have, you can extend vault-find command :)

MartinBasti · 2016-12-14T18:00:47Z

NACK: DNS records page is broken

MartinBasti · 2016-12-14T18:26:08Z

Server roles page is broken too, or at least it looks weird, probably server names are missing

pvomacka · 2016-12-20T15:20:25Z

@mbasti-rh Both bugs fixed, thank you.

Back to the difference between My User Vault and User Vault. I forgot to mention that My User Vault shows only vaults which are created for the user (who is logged in) and where that user is in Member or Owner group. I think that it is consistent with CLI, or not?

MartinBasti · 2017-01-19T17:43:44Z

Works for me

tiran · 2017-02-23T17:02:03Z

@MartinBasti you approved this PR a month ago but it has neither the ACK flag nor was it merged.

@pvomacka Your work would be useful for my Custodia Vault work. Can you rebase this PR to master to verify it still works?

pvomacka · 2017-02-23T17:26:42Z

@tiran Yes, rebased.

pvoborni

Comments in code.

pvoborni · 2017-02-24T12:15:23Z

install/ui/src/freeipa/vault.js

+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */


Short copyright note should be used for new files.

pvoborni · 2017-02-24T12:16:21Z

ipaserver/plugins/internal.py

+                "status_mod_s": _("Modified"),
+                "status_new_ns": _("New: key not set"),
+                "status_new_s": _("New: key set"),
+            },


missing in ipa_init.json, but could be ignored given that ipa_init.json will be removed anyway

Right, I won't add it there

pvoborni · 2017-02-24T13:07:38Z

install/ui/src/freeipa/association.js

+     *
+     * @property {String} fieldname
+     */
+    that.additional_add_del_field = spec.additional_add_del_field;


I don't like this approach much. But we can live with it.

The approach adds relatively big amount of code but it is limited only to one field.

You could use similar approach which is in IPA.dialog.

that.save = function(record) { var fields = that.fields.get_fields(); for (var i=0; i<fields.length; i++) { var field = fields[i]; field.save(record); } };

Where only get_fields would be replaced by add_del_fields: ['field1','field2'] record could be empty object and then you would use command.set_options(otps) method.

pvoborni · 2017-02-24T13:16:35Z

install/ui/src/freeipa/association.js

+     * @property other_option_name {String}
+     */
+    that.other_option_name = spec.other_option_name;
+
    that.other_entity = IPA.get_entity(spec.other_entity);


Would be good also to add doc string for this one and attribute_member. This logic is becoming to be quite complex and confusing.

I wonder if we can come up with a general approach which would cover this and the join_additional_option use case. E.g. define a list of option value providers. But probably it is out of range of this patch.

pvoborni · 2017-02-24T13:45:12Z

install/ui/src/freeipa/association.js

+     * In case that there is set acl_param in spec then this property will be
+     * set to true and it checks ACLs.
+     */
+    that.acl_param_set = !!spec.acl_param;


Do we really need it? Why can't you use !!that.acl_param later in the if?

pvoborni · 2017-02-24T14:02:20Z

install/ui/src/freeipa/details.js

+     *
+     * @property {string}
+     */
+    that.refresh_url_arg = spec.refresh_url_arg || null;


Why do you use naming *_url_arg when previous commit had naming refresh_attribute. Same issue in following commit with update_url_arg. *_attribute is IMO better because it defines usage and not source of the value. Facets in fact should not be aware of urls. That is a role of navigation subsystem.

pvoborni · 2017-02-24T14:04:17Z

install/ui/src/freeipa/facet.js

+     *
+     * @property {String}
+     */
+    that.load_page_additional_attr = spec.load_page_additional_attr || null;


again, different naming pattern

pvoborni · 2017-02-24T14:23:57Z

install/ui/src/freeipa/facet.js

+     * attribute, then checks whether the element exists and if it exists, then
+     * it removes it.
+     */
+    that.remove_tab_from_sidebar = function(tab_name) {


this logic, if it needs to be somewhere then it should be in FacetGroupsWidget which is then accessible as that.tabs_widget. With interface hide_tab(tab_name) and show_tab(tab_name).

There you can get tab element by var tab_el = this.tab_els[tab_name]; which is more robust because tabs doesn't have to be <li> elements.

Yes, that make sense to move it there. Fixed.

pvoborni · 2017-02-24T15:17:49Z

install/ui/src/freeipa/vault.js

+ * Creates specification of search facet for User Vaults
+ */
+var make_user_vault_search_spec = function() {
+    return {


In self-service, user cannot add vault because 'add' button is hidden. He can add stadard vault on CLI though.

It is hidden because of spec.hide_cond = spec.hide_cond || ['self-service']; in IPA.add_action

Yes, that's true, I will make a function which allows to override default actions of search facet and use own actions.

Fixed.

pvoborni · 2017-02-24T15:22:19Z

install/ui/src/freeipa/vault.js

+    that.create_content = function() {
+        var warn_st_w = that.widgets.get_widget('warning_st.warn_standard');
+        var warn_arch_ret_w = that.widgets.get_widget('warning_ar.warn_arch_ret');
+        var warn_standard = $('<div />', {


can you use widget.alert_helper for generating the alert?

pvoborni

Read the code, looks good, only one minor issue - wrong indentation.

pvoborni · 2017-03-13T16:57:12Z

install/ui/src/freeipa/vault.js

+        ]
+    };
+
+        var section_warn_standard = {


wrong indentation

@pvoborni fixed

By setting the property 'additional_add_del_field' to the name of one of the fields which are on current details page, we choose field which value will be added to *_add_* and *_del_* commands in this format: {field_name: field_value} --field_name: field_value Part of: https://fedorahosted.org/freeipa/ticket/5426

Association table's add, del commands needs as option list of cn of other_entity, which is added or deleted. There is a case (currently in vaults) that the name of option is different than the name of other_entity. In this situation we can set 'other_option_name' and put there the option name. This option name will be used instead of 'other_entity' name. Part of: https://fedorahosted.org/freeipa/ticket/5426

Useful in association tables which need to ignore object's metadata flags. Association tables don't check right at all. They check them only when 'acl_param' is set in association table field spec. In case that checking metadata needs to be turned on even for Association table, then set 'check_writable_from_metadata' true value in spec. Part of: https://fedorahosted.org/freeipa/ticket/5426

The 'refresh_option' of association field takes string. This string has to correspond with field name on details page. In case that the field is present the value of the field is passed to command as option in following format: {fieldname: field_value} Part of: https://fedorahosted.org/freeipa/ticket/5426

'refresh_attribute' can be set to the name of url parameter name. This parameter with its value is then passed to refresh command of the details facet. Part of: https://fedorahosted.org/freeipa/ticket/5426

'update_attribute' can contain a name of field in details page. In that case the value of the field with field name will be appended to the update command options. Part of: https://fedorahosted.org/freeipa/ticket/5426

Allow pagination to table facets which needs to call _show on all rows with additional parameter. 'show_command_additional_attr' can be set to any attribute from result of _find command. This attribute is taken with its value and added to options of _each command for each row. Part of: https://fedorahosted.org/freeipa/ticket/5426

…el command 'additional_table_attrs' can contain array of names of columns. Value from each column with its name will be added to the batch _del command. in case that the column with set name does not exists - the name is skipped. Part of: https://fedorahosted.org/freeipa/ticket/5426

Removes item selected by name attribute from sidebar Part of: https://fedorahosted.org/freeipa/ticket/5426

While defining search facet and adding custom actions with the same name as default actions in search facet. Custom actions will be used and their definition will override default actions. Part of:https://fedorahosted.org/freeipa/ticket/5426

Allows to show rows which have the same primary key. Used in Vault. https://fedorahosted.org/freeipa/ticket/5426

Add vault management into WebUI, there are some constraints: - There is no crypto library so Symmetric and Assymetric vaults are not supported in WebUI. Also retrieving or archiving data is not supported. - There aren't any container support right now Supported is: - Browsing vaults - Adding Standard vaults (users, service, shared) - Removing vaults - Adding and removing owners - Adding and removing members https://fedorahosted.org/freeipa/ticket/5426

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

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

Bunch of tests for WebUI Vault Management. Covers: Adding vaults Modifying vaults Adding members and owners to all types of vaults https://fedorahosted.org/freeipa/ticket/5426

MartinBasti · 2017-03-14T09:41:00Z

master:

c3115fa Additional option to add and del operations can be set
ec63456 Allow to set another other_entity name
93a7f4c Possibility to skip checking writable according to metadata
6d1374f Added optional option in refreshing after modifying association table
bbca1d9 Add property which allows refresh command to use url value
042e113 Add possibility to pass url parameter to update command of details page
2e6e069 Extend _show command after _find command in table facets
039a6f7 Possibility to set list of table attributes which will be added to _del command
8dfe692 Add possibility to hide only one tab in sidebar
de4d4a5 WebUI: search facet's default actions might be overriden
587b732 WebUI: allow to show rows with same pkey in tables
39d7ef3 WebUI: add vault management
ab8c69f TESTS: Add support for KRA in ui_driver
0808504 TESTS: Add support for sidebar with facets
f952757 TESTS WebUI: Vaults management

MartinBasti self-assigned this Oct 5, 2016

pvomacka force-pushed the t5426 branch from a457c66 to c5e46ee Compare October 6, 2016 08:05

MartinBasti reviewed Oct 6, 2016

View reviewed changes

pvomacka force-pushed the t5426 branch from c5e46ee to 6c33c87 Compare October 25, 2016 15:03

pvomacka force-pushed the t5426 branch from 6c33c87 to f971130 Compare October 26, 2016 07:06

pvomacka force-pushed the t5426 branch from f971130 to 2295af5 Compare December 9, 2016 13:09

pvomacka force-pushed the t5426 branch from 2295af5 to c902b03 Compare December 20, 2016 15:02

MartinBasti approved these changes Jan 19, 2017

View reviewed changes

tiran requested a review from pvoborni February 23, 2017 17:25

pvomacka force-pushed the t5426 branch from c902b03 to 89f5598 Compare February 23, 2017 17:25

tiran assigned pvoborni Feb 23, 2017

pvoborni requested changes Feb 24, 2017

View reviewed changes

pvomacka force-pushed the t5426 branch from 89f5598 to dfbc3fb Compare March 12, 2017 21:54

pvoborni requested changes Mar 13, 2017

View reviewed changes

Pavel Vomacka added 15 commits March 13, 2017 18:22

Add property which allows refresh command to use url value

e77d89b

'refresh_attribute' can be set to the name of url parameter name. This parameter with its value is then passed to refresh command of the details facet. Part of: https://fedorahosted.org/freeipa/ticket/5426

Add possibility to hide only one tab in sidebar

b928b51

Removes item selected by name attribute from sidebar Part of: https://fedorahosted.org/freeipa/ticket/5426

WebUI: allow to show rows with same pkey in tables

cd1e47d

Allows to show rows which have the same primary key. Used in Vault. https://fedorahosted.org/freeipa/ticket/5426

TESTS: Add support for KRA in ui_driver

ef60aca

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

TESTS: Add support for sidebar with facets

66e897c

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

TESTS WebUI: Vaults management

d563e9c

Bunch of tests for WebUI Vault Management. Covers: Adding vaults Modifying vaults Adding members and owners to all types of vaults https://fedorahosted.org/freeipa/ticket/5426

pvomacka force-pushed the t5426 branch from dfbc3fb to d563e9c Compare March 13, 2017 17:22

pvoborni approved these changes Mar 14, 2017

View reviewed changes

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

MartinBasti added the pushed Pull Request has already been pushed label Mar 14, 2017

MartinBasti removed their assignment Mar 14, 2017

MartinBasti closed this Mar 14, 2017

WebUI: Vault Management #139

WebUI: Vault Management #139

Conversation

pvomacka commented Oct 5, 2016 • edited by MartinBasti

MartinBasti commented Oct 6, 2016

Choose a reason for hiding this comment

pvoborni commented Oct 6, 2016

MartinBasti commented Oct 6, 2016

MartinBasti commented Oct 6, 2016

MartinBasti commented Oct 6, 2016

pvomacka commented Oct 25, 2016

pvomacka commented Oct 26, 2016

MartinBasti commented Nov 22, 2016

pvomacka commented Dec 12, 2016

MartinBasti commented Dec 12, 2016

MartinBasti commented Dec 14, 2016

MartinBasti commented Dec 14, 2016

pvomacka commented Dec 20, 2016

MartinBasti commented Jan 19, 2017

tiran commented Feb 23, 2017

pvomacka commented Feb 23, 2017

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

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

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

MartinBasti commented Mar 14, 2017

pvomacka commented Oct 5, 2016 •

edited by MartinBasti