ui: Fix URL params decoding (#11931)

* ui: Move wildcard param decoding to routlet service

.changelog/11931.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+ui: Fixes a bug with URL decoding within KV area
+```

ui/packages/consul-ui/app/routing/route.js

@@ -2,17 +2,14 @@ import Route from '@ember/routing/route';
 import { get, setProperties, action } from '@ember/object';
 import { inject as service } from '@ember/service';
 
-// paramsFor
 import { routes } from 'consul-ui/router';
-import wildcard from 'consul-ui/utils/routing/wildcard';
-
-const isWildcard = wildcard(routes);
 
 export default class BaseRoute extends Route {
   @service('container') container;
   @service('env') env;
   @service('repository/permission') permissions;
   @service('router') router;
+  @service('routlet') routlet;
 
   _setRouteName() {
     super._setRouteName(...arguments);
@@ -114,27 +111,14 @@ export default class BaseRoute extends Route {
   }
 
   /**
-   * Adds urldecoding to any wildcard route `params` passed into ember `model`
+   * Normalizes any params passed into ember `model` hooks, plus of course
-   * hooks, plus of course anywhere else where `paramsFor` is used. This means
+   * anywhere else where `paramsFor` is used. This means the entire ember app
-   * the entire ember app is now changed so that all paramsFor calls returns
+   * is now changed so that all paramsFor calls returns normalized params
-   * urldecoded params instead of raw ones.
+   * instead of raw ones. For example we use this largely for URLs for the KV
-   * For example we use this largely for URLs for the KV store:
+   * store: /kv/*key > /ui/kv/%25-kv-name/%25-here > key = '%-kv-name/%-here'
-   * /kv/*key > /ui/kv/%25-kv-name/%25-here > key = '%-kv-name/%-here'
    */
   paramsFor(name) {
-    const params = super.paramsFor(...arguments);
+    return this.routlet.normalizeParamsFor(this.routeName, super.paramsFor(...arguments));
-    if (isWildcard(this.routeName)) {
-      return Object.keys(params).reduce(function(prev, item) {
-        if (typeof params[item] !== 'undefined') {
-          prev[item] = decodeURIComponent(params[item]);
-        } else {
-          prev[item] = params[item];
-        }
-        return prev;
-      }, {});
-    } else {
-      return params;
-    }
   }
 
   @action

ui/packages/consul-ui/app/services/routlet.js

@@ -1,6 +1,11 @@
 import Service, { inject as service } from '@ember/service';
 import { schedule } from '@ember/runloop';
 
+import wildcard from 'consul-ui/utils/routing/wildcard';
+import { routes } from 'consul-ui/router';
+
+const isWildcard = wildcard(routes);
+
 class Outlets {
   constructor() {
     this.map = new Map();
@@ -87,6 +92,24 @@ export default class RoutletService extends Service {
     return {};
   }
 
+  /**
+   * Adds urldecoding to any wildcard route `params`
+   */
+  normalizeParamsFor(name, params = {}) {
+    if (isWildcard(name)) {
+      return Object.keys(params).reduce(function(prev, item) {
+        if (typeof params[item] !== 'undefined') {
+          prev[item] = decodeURIComponent(params[item]);
+        } else {
+          prev[item] = params[item];
+        }
+        return prev;
+      }, {});
+    } else {
+      return params;
+    }
+  }
+
   paramsFor(name) {
     let outletParams = {};
     const outlet = outlets.get(name);
@@ -102,16 +125,14 @@ export default class RoutletService extends Service {
     // of the specified params with the values specified
     let current = route;
     let parent;
-    let routeParams = {
+    let routeParams = this.normalizeParamsFor(name, current.params);
-      ...current.params,
-    };
     // TODO: Not entirely sure whether we are ok exposing queryParams here
     // seeing as accessing them from here means you can get them but not set
     // them as yet
     // let queryParams = {};
     while ((parent = current.parent)) {
       routeParams = {
-        ...parent.params,
+        ...this.normalizeParamsFor(parent.name, parent.params),
         ...routeParams,
       };
       // queryParams = {

ui/packages/consul-ui/app/templates/dc/kv/edit.hbs

@@ -83,7 +83,7 @@ as |parts|}}
     </BlockSlot>
 
     <BlockSlot @name="header">
-        <h1>
+        <h1 data-test-kv-key={{item.Key}}>
 {{#if (and item.Key (not-eq item.Key parentKey))}}
           <route.Title @title="Edit Key / Value" @render={{false}} />
           {{left-trim item.Key parentKey}}

ui/packages/consul-ui/tests/acceptance/dc/kvs/edit.feature

@@ -1,6 +1,19 @@
 @setupApplicationTest
 Feature: dc / kvs / edit: KV Viewing
-  Scenario:
+  Scenario: Viewing a KV with a URL unsafe character
+    Given 1 datacenter model with the value "datacenter"
+    And 1 kv model from yaml
+    ---
+      Key: "@key"
+    ---
+    When I visit the kv page for yaml
+    ---
+      dc: datacenter
+      kv: "@key"
+    ---
+    Then the url should be /datacenter/kv/%40key/edit
+    And I see Key on the kv like "@key"
+  Scenario: Viewing a Session attached to a KV
     Given 1 datacenter model with the value "datacenter"
     And 1 kv model from yaml
     ---
@@ -14,7 +27,9 @@ Feature: dc / kvs / edit: KV Viewing
     ---
     Then the url should be /datacenter/kv/key/edit
     And I see ID on the session like "session-id"
-    Given 1 kv model from yaml
+  Scenario: Viewing a Session attached to a KV
+    Given 1 datacenter model with the value "datacenter"
+    And 1 kv model from yaml
     ---
       Key: another-key
       Session: ~

ui/packages/consul-ui/tests/pages/dc/kv/edit.js

@@ -11,6 +11,9 @@ export default function(visitable, attribute, present, submitable, deletable, ca
     ...submitable({}, 'main'),
     ...cancelable(),
     ...deletable(),
+    kv: {
+      Key: attribute('data-test-kv-key', '[data-test-kv-key]')
+    },
     session: {
       warning: present('[data-test-session-warning]'),
       ID: attribute('data-test-session', '[data-test-session]'),