UI: [BUGFIX] Decode/encode urls (#5206)

In 858b05fc31 (diff-46ef88aa04507fb9b039344277531584)
we removed encoding values in pathnames as we thought they were
eventually being encoded by `ember`. It looks like this isn't the case.

Turns out sometimes they are encoded sometimes they aren't. It's complicated.
If at all possible refer to the PR https://github.com/hashicorp/consul/pull/5206.

It's related to the difference between `dynamic` routes and `wildcard` routes.

Partly related to this is a decision on whether we urlencode the slashes within service names or not. Whilst historically we haven't done this, we feel its a good time to change this behaviour, so we'll also be changing services to use dynamic routes instead of wildcard routes. So service links will then look like /ui/dc-1/services/application%2Fservice rather than /ui/dc-1/services/application/service

Here, we define our routes in a declarative format (for the moment at least JSON) outside of Router.map, and loop through this within Router.map to set all our routes using the standard this.route method. We essentially configure our Router from the outside. As this configuration is now done declaratively outside of Router.map we can also make this data available to href-to and paramsFor, allowing us to detect wildcard routes and therefore apply urlencoding/decoding.

Where I mention 'conditionally' below, this is detection is what is used for the decision.

We conditionally add url encoding to the `{{href-to}}` helper/addon. The
reasoning here is, if we are asking for a 'href/url' then whatever we
receive back should always be urlencoded. We've done this by reusing as much
code from the original `ember-href-to` addon as possible, after this
change every call to the `{{href-to}}` helper will be urlencoded.

As all links using `{{href-to}}` are now properly urlencoded. We also
need to decode them in the correct place 'on the other end', so..

We also override the default `Route.paramsFor` method to conditionally decode all
params before passing them to the `Route.model` hook.

Lastly (the revert), as we almost consistently use url params to
construct API calls, we make sure we re-encode any slugs that have been
passed in by the user/developer. The original API for the `createURL`
function was to allow you to pass values that didn't need encoding,
values that **did** need encoding, followed by query params (which again
require url encoding)

All in all this should make the entire ember app url encode/decode safe.

ui-v2/app/adapters/application.js

@@ -111,6 +111,7 @@ export default Adapter.extend({
   // url encoded
 
   appendURL: function(path, parts = [], query = {}) {
-    return createURL([this.buildURL(), path], parts, query);
+    // path can be a string or an array of parts that will be slash joined
+    return createURL([this.buildURL()].concat(path), parts, query);
   },
 });

ui-v2/app/adapters/service.js

@@ -13,13 +13,6 @@ export default Adapter.extend({
     }
     return this.appendURL(URL_PREFIX_SINGLE, [query.id], this.cleanQuery(query));
   },
-  isQueryRecord: function(url, method) {
-    // services can have slashes in them
-    // so just check for the prefix which is
-    // unique to the url here
-    const parts = url.pathname.split('/');
-    return `${parts[2]}/${parts[3]}` === URL_PREFIX_SINGLE;
-  },
   handleResponse: function(status, headers, payload, requestData) {
     let response = payload;
     const method = requestData.method;

ui-v2/app/helpers/href-to.js

@@ -0,0 +1,31 @@
+// This helper requires `ember-href-to` for the moment at least
+// It's similar code but allows us to check on the type of route
+// (dynamic or wildcard) and encode or not depending on the type
+import Helper from '@ember/component/helper';
+import { hrefTo } from 'ember-href-to/helpers/href-to';
+
+import wildcard from 'consul-ui/utils/routing/wildcard';
+
+import { routes } from 'consul-ui/router';
+
+const isWildcard = wildcard(routes);
+
+export default Helper.extend({
+  compute([targetRouteName, ...rest], namedArgs) {
+    if (namedArgs.params) {
+      return hrefTo(this, ...namedArgs.params);
+    } else {
+      if (isWildcard(targetRouteName)) {
+        const split = rest.map(function(item, i) {
+          return item
+            .split('/')
+            .map(encodeURIComponent)
+            .join('/');
+        });
+        return hrefTo(this, targetRouteName, ...split);
+      } else {
+        return hrefTo(this, targetRouteName, ...rest);
+      }
+    }
+  },
+});

ui-v2/app/initializers/route-urldecode-params.js

@@ -0,0 +1,32 @@
+import Route from '@ember/routing/route';
+import { routes } from 'consul-ui/router';
+import wildcard from 'consul-ui/utils/routing/wildcard';
+const isWildcard = wildcard(routes);
+/**
+ * This initializer adds urldecoding to the `params` passed into
+ * ember `model` hooks, plus of course anywhere else where `paramsFor`
+ * is used. This means the entire ember app is now changed so that all
+ * paramsFor calls returns urldecoded params instead of raw ones
+ */
+Route.reopen({
+  paramsFor: function() {
+    const params = this._super(...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;
+    }
+  },
+});
+export function initialize() {}
+
+export default {
+  initialize,
+};

ui-v2/app/router.js

@@ -1,58 +1,98 @@
 import EmberRouter from '@ember/routing/router';
 import config from './config/environment';
+import walk from 'consul-ui/utils/routing/walk';
 
 const Router = EmberRouter.extend({
   location: config.locationType,
   rootURL: config.rootURL,
 });
-Router.map(function() {
+export const routes = {
   // Our parent datacenter resource sets the namespace
   // for the entire application
-  this.route('dc', { path: '/:dc' }, function() {
+  dc: {
+    _options: { path: ':dc' },
     // Services represent a consul service
-    this.route('services', { path: '/services' }, function() {
+    services: {
+      _options: { path: '/services' },
       // Show an individual service
-      this.route('show', { path: '/*name' });
-    });
+      show: {
+        _options: { path: '/:name' },
+      },
+    },
     // Nodes represent a consul node
-    this.route('nodes', { path: '/nodes' }, function() {
+    nodes: {
+      _options: { path: '/nodes' },
       // Show an individual node
-      this.route('show', { path: '/:name' });
-    });
+      show: {
+        _options: { path: '/:name' },
+      },
+    },
     // Intentions represent a consul intention
-    this.route('intentions', { path: '/intentions' }, function() {
-      this.route('edit', { path: '/:id' });
-      this.route('create', { path: '/create' });
-    });
+    intentions: {
+      _options: { path: '/intentions' },
+      edit: {
+        _options: { path: '/:id' },
+      },
+      create: {
+        _options: { path: '/create' },
+      },
+    },
     // Key/Value
-    this.route('kv', { path: '/kv' }, function() {
-      this.route('folder', { path: '/*key' });
-      this.route('edit', { path: '/*key/edit' });
-      this.route('create', { path: '/*key/create' });
-      this.route('root-create', { path: '/create' });
-    });
+    kv: {
+      _options: { path: '/kv' },
+      folder: {
+        _options: { path: '/*key' },
+      },
+      edit: {
+        _options: { path: '/*key/edit' },
+      },
+      create: {
+        _options: { path: '/*key/create' },
+      },
+      'root-create': {
+        _options: { path: '/create' },
+      },
+    },
     // ACLs
-    this.route('acls', { path: '/acls' }, function() {
-      this.route('edit', { path: '/:id' });
-      this.route('create', { path: '/create' });
-      this.route('policies', { path: '/policies' }, function() {
-        this.route('edit', { path: '/:id' });
-        this.route('create', { path: '/create' });
-      });
-      this.route('tokens', { path: '/tokens' }, function() {
-        this.route('edit', { path: '/:id' });
-        this.route('create', { path: '/create' });
-      });
-    });
-  });
-
+    acls: {
+      _options: { path: '/acls' },
+      edit: {
+        _options: { path: '/:id' },
+      },
+      create: {
+        _options: { path: '/create' },
+      },
+      policies: {
+        _options: { path: '/policies' },
+        edit: {
+          _options: { path: '/:id' },
+        },
+        create: {
+          _options: { path: '/create' },
+        },
+      },
+      tokens: {
+        _options: { path: '/tokens' },
+        edit: {
+          _options: { path: '/:id' },
+        },
+        create: {
+          _options: { path: '/create' },
+        },
+      },
+    },
+  },
   // Shows a datacenter picker. If you only have one
   // it just redirects you through.
-  this.route('index', { path: '/' });
-
+  index: {
+    _options: { path: '/' },
+  },
   // The settings page is global.
-  // this.route('settings', { path: '/settings' });
-  this.route('notfound', { path: '/*path' });
-});
-
-export default Router;
+  // settings: {
+  //   _options: { path: '/setting' },
+  // },
+  notfound: {
+    _options: { path: '/*path' },
+  },
+};
+export default Router.map(walk(routes));

ui-v2/app/templates/dc/kv/index.hbs

@@ -46,7 +46,7 @@
             {{/block-slot}}
             {{#block-slot 'row'}}
                 <td data-test-kv="{{item.Key}}" class={{if item.isFolder 'folder' 'file' }}>
-                    <a href={{href-to (if item.isFolder 'dc.kv.folder' 'dc.kv.edit') item.Key}}>{{right-trim (left-trim item.Key parent.Key) '/'}}</a>
+                    <a href={{href-to (if item.isFolder 'dc.kv.folder' 'dc.kv.edit') item.Key }}>{{right-trim (left-trim item.Key parent.Key) '/'}}</a>
                 </td>
             {{/block-slot}}
             {{#block-slot 'actions' as |index change checked|}}

ui-v2/app/utils/createURL.js

@@ -12,7 +12,7 @@
  */
 export default function(encoded, raw, query = {}, encode = encodeURIComponent) {
   return [
-    encoded.concat(raw).join('/'),
+    encoded.concat(raw.map(encode)).join('/'),
     Object.keys(query)
       .map(function(key, i, arr) {
         if (query[key] != null) {

ui-v2/app/utils/routing/walk.js

@@ -0,0 +1,80 @@
+export const walk = function(routes) {
+  const keys = Object.keys(routes);
+  keys.forEach((item, i) => {
+    if (item === '_options') {
+      return;
+    }
+    const options = routes[item]._options;
+    let cb;
+    if (Object.keys(routes[item]).length > 1) {
+      cb = function() {
+        walk.apply(this, [routes[item]]);
+      };
+    }
+    this.route(item, options, cb);
+  });
+  if (typeof routes.index === 'undefined') {
+    routes.index = {
+      _options: {
+        path: '',
+      },
+    };
+  }
+};
+
+/**
+ * Drop in for the Router.map callback e.g. `Router.map(walk(routes))`
+ * Uses { walk } to recursively walk through a JSON object of routes
+ * and use `Router.route` to define your routes for your ember application
+ *
+ * @param {object} routes - JSON representation of routes
+ */
+export default function(routes) {
+  return function() {
+    walk.apply(this, [routes]);
+  };
+}
+
+// The following code is purposefully commented out to prevent it from ending up
+// in the production codebase. In future it would be good to figure out how to do this
+// without having to use comments.
+
+// const indent = function(num) {
+//   return Array(num).fill('  ', 0, num).join('')
+// }
+// /**
+//  * String dumper to produce Router.map code
+//  * Uses { walk } to recursively walk through a JSON object of routes
+//  * to produce the code necessary to define your routes for your ember application
+//  *
+//  * @param {object} routes - JSON representation of routes
+//  * @example `console.log(dump(routes));`
+//  */
+// export const dump = function(routes) {
+//   let level = 2;
+//   const obj = {
+//     out: '',
+//     route: function(name, options, cb) {
+//       this.out += `${indent(level)}this.route('${name}', ${JSON.stringify(options)}`;
+//       if(cb) {
+//         level ++;
+//         this.out += `, function() {
+// `;
+//         cb.apply(this, []);
+//         level --;
+//         this.out += `${indent(level)}});
+// `;
+//       } else {
+//         this.out += ');';
+//       }
+//       this.out += `
+// `;
+//     }
+//   };
+//   walk.apply(obj, [routes])
+//   return `Router.map(
+//   function() {
+// ${obj.out}
+//   }
+// );`;
+// }

ui-v2/app/utils/routing/wildcard.js

@@ -0,0 +1,12 @@
+import { get } from '@ember/object';
+export default function(routes) {
+  return function(name) {
+    let wildcard = false;
+    try {
+      wildcard = get(routes, name)._options.path.indexOf('*') !== -1;
+    } catch (e) {
+      // passthrough
+    }
+    return wildcard;
+  };
+}

ui-v2/tests/acceptance/dc/kvs/update.feature

@@ -5,14 +5,14 @@ Feature: dc / kvs / update: KV Update
   Scenario: Update to [Name] change value to [Value]
     And 1 kv model from yaml
     ---
-      Key: [Name]
+      Key: "[Name]"
     ---
     When I visit the kv page for yaml
     ---
       dc: datacenter
-      kv: [Name]
+      kv: "[Name]"
     ---
-    Then the url should be /datacenter/kv/[Name]/edit
+    Then the url should be /datacenter/kv/[EncodedName]/edit
     # Turn the Code Editor off so we can fill the value easier
     And I click "[name=json]"
     Then I fill in with yaml
@@ -20,16 +20,18 @@ Feature: dc / kvs / update: KV Update
       value: [Value]
     ---
     And I submit
-    Then a PUT request is made to "/v1/kv/[Name]?dc=datacenter" with the body "[Value]"
+    Then a PUT request is made to "/v1/kv/[EncodedName]?dc=datacenter" with the body "[Value]"
     And "[data-notification]" has the "notification-update" class
     And "[data-notification]" has the "success" class
   Where:
-      --------------------------------------------
-      | Name                      | Value        |
-      | key                       | value        |
-      | key-name                  | a value      |
-      | folder/key-name           | a value      |
-      --------------------------------------------
+      ---------------------------------------------------------
+      | Name            | EncodedName          | Value        |
+      | key             | key                  | value        |
+      | #key            | %23key               | value        |
+      | key-name        | key-name             | a value      |
+      | key name        | key%20name           | a value      |
+      | folder/key-name | folder/key-name      | a value      |
+      ---------------------------------------------------------
   Scenario: Update to a key change value to '   '
     And 1 kv model from yaml
     ---

ui-v2/tests/acceptance/dc/services/show-with-slashes.feature

@@ -17,5 +17,5 @@ Feature: dc / services / show-with-slashes: Show Service that has slashes in its
     Then the url should be /dc1/services
     Then I see 1 service model
     And I click service on the services
-    Then the url should be /dc1/services/hashicorp/service/service-0
+    Then the url should be /dc1/services/hashicorp%2Fservice%2Fservice-0
 

ui-v2/tests/pages/dc/kv/edit.js

@@ -2,7 +2,14 @@ export default function(visitable, submitable, deletable, cancelable) {
   return submitable(
     cancelable(
       deletable({
-        visit: visitable(['/:dc/kv/:kv/edit', '/:dc/kv/create'], str => str),
+        visit: visitable(['/:dc/kv/:kv/edit', '/:dc/kv/create'], function(str) {
+          // this will encode the parts of the key path but means you can no longer
+          // visit with path parts containing slashes
+          return str
+            .split('/')
+            .map(encodeURIComponent)
+            .join('/');
+        }),
         session: deletable({}, '[data-test-session]'),
       })
     )

ui-v2/tests/unit/utils/routing/walk-test.js

@@ -0,0 +1,37 @@
+import { walk } from 'consul-ui/utils/routing/walk';
+import { module } from 'qunit';
+import test from 'ember-sinon-qunit/test-support/test';
+
+module('Unit | Utility | routing/walk');
+
+test('it walks down deep routes', function(assert) {
+  const route = this.stub();
+  const Router = {
+    route: function(name, options, cb) {
+      route();
+      if (cb) {
+        cb.apply(this, []);
+      }
+    },
+  };
+  walk.apply(Router, [
+    {
+      route: {
+        _options: {
+          path: '/:path',
+        },
+        next: {
+          _options: {
+            path: '/:path',
+          },
+          inside: {
+            _options: {
+              path: '/*path',
+            },
+          },
+        },
+      },
+    },
+  ]);
+  assert.equal(route.callCount, 3);
+});

ui-v2/tests/unit/utils/routing/wildcard-test.js

@@ -0,0 +1,25 @@
+import wildcard from 'consul-ui/utils/routing/wildcard';
+import { module, test } from 'qunit';
+
+module('Unit | Utility | routing/wildcard');
+
+test('it finds a * in a path', function(assert) {
+  const isWildcard = wildcard({
+    route: {
+      _options: {
+        path: 'i-m-a-wildcard*',
+      },
+    },
+  });
+  assert.ok(isWildcard('route'));
+});
+test("it returns false without throwing if it doesn't find route", function(assert) {
+  const isWildcard = wildcard({
+    route: {
+      _options: {
+        path: 'i-m-a-wildcard*',
+      },
+    },
+  });
+  assert.notOk(isWildcard('not-route'));
+});