[Management] Address UI searching bug with index pattern creation #14646

chrisronline · 2017-10-27T18:39:35Z

Overview

On the index pattern creation wizard, if the user types in a character into the input field, we have code that will automatically append a * character to the end of what they typed and then move the cursor back to right after the typed letter. This experience came from discussions on the original PR.

Unfortunately, there is a bug with how we're transferring state between the angular directives where we weren't properly updating the state before the parent directive accessed it, leading to an initial query against the typed character, instead of the typed character plus the *.

For super users, this isn't a big deal because they aren't any security gates against searching for indices, but it becomes a problem for restricted users who aren't able to query for certain indices.

Take this sample of queries for a restricted user who can only access the logstash index:

POST l/_search -> 403
POST *l/_search -> 200
POST l*/_search -> 200
POST *l*/_search -> 200

The UI bug caused a search for just l instead of l* which resulted in 403 errors in the browser.

This PR addresses that and separates out the code into a separate directive. It also functions off keydown instead of an angular watch due to issues mutating that value post watch firing.

It's very possible this is a hacky and there is a better way, so please let me know!

TODO

Tests are hard for this kind of experience. Any recommendations?

bmcconaghy · 2017-10-27T20:17:20Z

...anagement/sections/indices/create_index_pattern_wizard/step_index_pattern/append_wildcard.js

+        }
+      }
+
+      $elem.on('keydown', (e) => {


Maybe a little concerned that this will not always work as intended (all work completing before watcher has a chance to fire). Maybe that is unfounded though.

It's a good point and something I just assumed. I figured that the digest cycle doesn't kickoff until all interactions are "done", and I assumed that any listeners directly listening to UI interacts will be handled prior to anything angular does, but I could be mistaken there

Methods run to completion in JS, so this should be good, just being paranoid.

bmcconaghy · 2017-10-27T20:17:21Z

...anagement/sections/indices/create_index_pattern_wizard/step_index_pattern/append_wildcard.js

+        event.preventDefault();
+        $elem.val(val);
+        if (moveSelectionRange) {
+          $elem[0].setSelectionRange(1, 1);


I had to think for a minute to grok why this line is doing what it does. Maybe parameter could be named better to make that clear.

bmcconaghy · 2017-10-28T09:02:18Z

LGTM

kobelb · 2017-10-30T13:14:02Z

...anagement/sections/indices/create_index_pattern_wizard/step_index_pattern/append_wildcard.js

+      }
+
+      $elem.on('keydown', (e) => {
+        const charStr = String.fromCharCode(e.keyCode);


MDN warns against using KeyboardEvent.keyCode and recommends using KeyboardEvent.key. However, Chrome 51 was the first to introduce KeyboardEvent.key, so it'd be nice if this "gracefully degrades" in unsupported versions, perhaps by simply not intelligently adding the *.

This might be related to the following behavior, if I type Command + R (normally refreshes the page in Chrome on macOS) it's inserting a lower-case "r":

Nice call! Addressing the first one fixes the second one

kobelb · 2017-10-30T13:25:27Z

...anagement/sections/indices/create_index_pattern_wizard/step_index_pattern/append_wildcard.js

+
+        if (indexPatternName && indexPatternName.length === 1) {
+          if (indexPatternName === '*') {
+            if ($scope.appendedWildcard) {


This causes an interesting behavior, when the page first loads you're allowed to put * as the index-pattern, but if I create an index pattern where we append the * and $scope.appendedWildcard === true, if I try to type * again, it won't do anything, but if I press it again, it'll let me. This appears to be behavior that was carried over from the previous code, so if you'd prefer to create a separate PR/Issue to address this, I won't let it hold up my approval.

chrisronline · 2017-10-30T14:45:46Z

@kobelb Thanks for the great thoughts! I fixed a couple things and refactored some of the code to hopefully avoid some of the confusion/issues. Lemme know

chrisronline · 2017-10-30T14:46:11Z

...anagement/sections/indices/create_index_pattern_wizard/step_index_pattern/append_wildcard.js

+          return;
+        }
+
+        // If the user is holdinng down ctrl/cmd, they are performing some shortcut


Bah, typo in the comment

chrisronline · 2017-10-30T14:46:31Z

...anagement/sections/indices/create_index_pattern_wizard/step_index_pattern/append_wildcard.js

+
+        let indexPatternName = $elem.val() + e.key;
+        if (indexPatternName && indexPatternName.length === 1) {
+          if (indexPatternName !== '*') {


This can be combined in the above if

kobelb

LGTM

…it around how it interacts with angular.

…astic#14646) * Move this logic to a separate directive. Also fix a couple bugs with it around how it interacts with angular. * Updated variable name * Handle some edge cases and simplify code * Fixing typo and further simplyfing code

…4646) (#14922) * Move this logic to a separate directive. Also fix a couple bugs with it around how it interacts with angular. * Updated variable name * Handle some edge cases and simplify code * Fixing typo and further simplyfing code

chrisronline · 2017-11-13T20:29:40Z

Backport

6.x: 076cea1

chrisronline added :Management review v6.1.0 v7.0.0 labels Oct 27, 2017

chrisronline self-assigned this Oct 27, 2017

chrisronline requested review from kobelb and bmcconaghy October 27, 2017 18:39

bmcconaghy reviewed Oct 27, 2017

View reviewed changes

kobelb requested changes Oct 30, 2017

View reviewed changes

chrisronline commented Oct 30, 2017

View reviewed changes

kobelb approved these changes Nov 13, 2017

View reviewed changes

chrisronline added 4 commits November 13, 2017 13:22

Move this logic to a separate directive. Also fix a couple bugs with …

d60222f

…it around how it interacts with angular.

Updated variable name

2a8b4cf

Handle some edge cases and simplify code

2d14aec

Fixing typo and further simplyfing code

f84e4be

chrisronline force-pushed the fix/index_pattern_creation_wizard branch from 281d3ab to f84e4be Compare November 13, 2017 18:22

chrisronline merged commit a5bd0af into elastic:master Nov 13, 2017

chrisronline mentioned this pull request Nov 13, 2017

[6.x] [Management] Address UI searching bug with index pattern creation (#14646) #14922

Merged

chrisronline deleted the fix/index_pattern_creation_wizard branch November 13, 2017 20:29

chrisronline mentioned this pull request Nov 15, 2017

[Management] Rethink our auto-append wildcard strategy for the index pattern creation wizard #14978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Management] Address UI searching bug with index pattern creation #14646

[Management] Address UI searching bug with index pattern creation #14646

chrisronline commented Oct 27, 2017

bmcconaghy Oct 27, 2017

chrisronline Oct 27, 2017

bmcconaghy Oct 28, 2017

bmcconaghy Oct 27, 2017

chrisronline Oct 27, 2017

bmcconaghy commented Oct 28, 2017

kobelb Oct 30, 2017

kobelb Oct 30, 2017

chrisronline Oct 30, 2017

kobelb Oct 30, 2017

chrisronline commented Oct 30, 2017

chrisronline Oct 30, 2017

chrisronline Oct 30, 2017

kobelb left a comment

chrisronline commented Nov 13, 2017

[Management] Address UI searching bug with index pattern creation #14646

[Management] Address UI searching bug with index pattern creation #14646

Conversation

chrisronline commented Oct 27, 2017

Overview

TODO

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

bmcconaghy commented Oct 28, 2017

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

chrisronline commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb left a comment

Choose a reason for hiding this comment

chrisronline commented Nov 13, 2017