UI variables made to be unique by namespace and path (#14072)

* Starting on namespaced id * Traversal for variables uniqued by namespace * Delog * Basic CRUD complete w namespaces included * Correct secvar breadcrumb joining and testfix now that namespaces are included * Testfixes with namespaces in place * Namespace-aware duplicate path warning * Duplicate path warning test additions * Trimpath reimplemented on dupe check * Solves a bug where slash was not being passed to the can write check * PR fixes * variable paths integration test fix now uses store * Seems far less hacky in retrospect * PR feedback addressed * test fixes after inclusion of path as local non-model var * Prevent confusion by dropping namespace from QPs on PUT, since its already in .data * Solves a harsh bug where you have namespace access but no secvars access (#14098) * Solves a harsh bug where you have namespace access but no secvars access * Lint cleanup * Remove unneeded condition
hashicorp · Aug 15, 2022 · 9f51a5d · 9f51a5d
1 parent ed55b97
commit 9f51a5d
Show file tree

Hide file tree

Showing 24 changed files with 289 additions and 198 deletions.
diff --git a/ui/app/adapters/variable.js b/ui/app/adapters/variable.js
@@ -8,13 +8,10 @@ export default class VariableAdapter extends ApplicationAdapter {
 
   // PUT instead of POST on create;
   // /v1/var instead of /v1/vars on create (urlForFindRecord)
-  createRecord(_store, _type, snapshot) {
+  createRecord(_store, type, snapshot) {
     let data = this.serialize(snapshot);
-    return this.ajax(
-      this.urlForFindRecord(snapshot.id, snapshot.modelName),
-      'PUT',
-      { data }
-    );
+    let baseUrl = this.buildURL(type.modelName, data.ID);
+    return this.ajax(baseUrl, 'PUT', { data });
   }
 
   urlForFindAll(modelName) {
@@ -27,21 +24,30 @@ export default class VariableAdapter extends ApplicationAdapter {
     return pluralize(baseUrl);
   }
 
-  urlForFindRecord(id, modelName, snapshot) {
-    const namespace = snapshot?.attr('namespace') || 'default';
-
-    let baseUrl = this.buildURL(modelName, id, snapshot);
+  urlForFindRecord(identifier, modelName, snapshot) {
+    const { namespace, id } = _extractIDAndNamespace(identifier, snapshot);
+    let baseUrl = this.buildURL(modelName, id);
     return `${baseUrl}?namespace=${namespace}`;
   }
 
-  urlForUpdateRecord(id, modelName) {
-    return this.buildURL(modelName, id);
+  urlForUpdateRecord(identifier, modelName, snapshot) {
+    const { id } = _extractIDAndNamespace(identifier, snapshot);
+    let baseUrl = this.buildURL(modelName, id);
+    return `${baseUrl}`;
   }
 
-  urlForDeleteRecord(id, modelName, snapshot) {
-    const namespace = snapshot?.attr('namespace') || 'default';
-
+  urlForDeleteRecord(identifier, modelName, snapshot) {
+    const { namespace, id } = _extractIDAndNamespace(identifier, snapshot);
     const baseUrl = this.buildURL(modelName, id);
     return `${baseUrl}?namespace=${namespace}`;
   }
 }
+
+function _extractIDAndNamespace(identifier, snapshot) {
+  const namespace = snapshot?.attr('namespace') || 'default';
+  const id = snapshot?.attr('path') || identifier;
+  return {
+    namespace,
+    id,
+  };
+}
diff --git a/ui/app/components/secure-variable-form.hbs b/ui/app/components/secure-variable-form.hbs
@@ -16,18 +16,17 @@
       </span>
       <Input
         @type="text"
-        @value={{@model.path}}
+        @value={{this.path}}
         placeholder="nomad/jobs/my-job/my-group/my-task"
         class="input path-input {{if this.duplicatePathWarning "error"}}"
         disabled={{not @model.isNew}}
-        {{on "input" this.validatePath}}
         {{autofocus}}
         data-test-path-input
       />
       {{#if this.duplicatePathWarning}}
         <p class="duplicate-path-error help is-danger">
           There is already a Secure Variable located at
-          {{@model.path}}
+          {{this.path}}
           .
           <br />
           Please choose a different path, or

diff --git a/ui/app/components/secure-variable-form.js b/ui/app/components/secure-variable-form.js
@@ -23,18 +23,15 @@ export default class SecureVariableFormComponent extends Component {
   @service router;
   @service store;
 
-  /**
-   * @typedef {Object} DuplicatePathWarning
-   * @property {string} path
-   */
-
-  /**
-   * @type {DuplicatePathWarning}
-   */
-  @tracked duplicatePathWarning = null;
   @tracked variableNamespace = null;
   @tracked namespaceOptions = null;
 
+  @tracked path = '';
+  constructor() {
+    super(...arguments);
+    set(this, 'path', this.args.model.path);
+  }
+
   @action
   setNamespace(namespace) {
     this.variableNamespace = namespace;
@@ -52,9 +49,8 @@ export default class SecureVariableFormComponent extends Component {
 
   get shouldDisableSave() {
     const disallowedPath =
-      this.args.model?.path?.startsWith('nomad/') &&
-      !this.args.model?.path?.startsWith('nomad/jobs');
-    return !!this.JSONError || !this.args.model?.path || disallowedPath;
+      this.path?.startsWith('nomad/') && !this.path?.startsWith('nomad/jobs');
+    return !!this.JSONError || !this.path || disallowedPath;
   }
 
   /**
@@ -94,19 +90,28 @@ export default class SecureVariableFormComponent extends Component {
     ]);
   }
 
-  @action
-  validatePath(e) {
-    const value = trimPath([e.target.value]);
+  /**
+   * @typedef {Object} DuplicatePathWarning
+   * @property {string} path
+   */
+
+  /**
+   * @type {DuplicatePathWarning}
+   */
+  get duplicatePathWarning() {
     const existingVariables = this.args.existingVariables || [];
+    const pathValue = trimPath([this.path]);
     let existingVariable = existingVariables
       .without(this.args.model)
-      .find((v) => v.path === value);
+      .find(
+        (v) => v.path === pathValue && v.namespace === this.variableNamespace
+      );
     if (existingVariable) {
-      this.duplicatePathWarning = {
+      return {
         path: existingVariable.path,
       };
     } else {
-      this.duplicatePathWarning = null;
+      return null;
     }
   }
 
@@ -144,7 +149,7 @@ export default class SecureVariableFormComponent extends Component {
     if (e.type === 'submit') {
       e.preventDefault();
     }
-    // TODO: temp, hacky way to force translation to tabular keyValues
+
     if (this.view === 'json') {
       this.translateAndValidateItems('table');
     }
@@ -168,20 +173,21 @@ export default class SecureVariableFormComponent extends Component {
       }
 
       this.args.model.set('keyValues', this.keyValues);
+      this.args.model.set('path', this.path);
       this.args.model.setAndTrimPath();
       await this.args.model.save();
 
       this.flashMessages.add({
         title: 'Secure Variable saved',
-        message: `${this.args.model.path} successfully saved`,
+        message: `${this.path} successfully saved`,
         type: 'success',
         destroyOnClick: false,
         timeout: 5000,
       });
-      this.router.transitionTo('variables.variable', this.args.model.path);
+      this.router.transitionTo('variables.variable', this.args.model.id);
     } catch (error) {
       this.flashMessages.add({
-        title: `Error saving ${this.args.model.path}`,
+        title: `Error saving ${this.path}`,
         message: error,
         type: 'error',
         destroyOnClick: false,

diff --git a/ui/app/components/variable-paths.hbs b/ui/app/components/variable-paths.hbs
@@ -35,7 +35,7 @@
           {{#if (can "read variable" path=file.absoluteFilePath namespace=file.variable.namespace)}}
           <LinkTo
             @route="variables.variable"
-            @model={{file.absoluteFilePath}}
+            @model={{file.variable.id}}
             @query={{hash namespace="*"}}
           >
             {{file.name}}

diff --git a/ui/app/components/variable-paths.js b/ui/app/components/variable-paths.js
@@ -26,9 +26,9 @@ export default class VariablePathsComponent extends Component {
   }
 
   @action
-  async handleFileClick({ path, variable: { namespace } }) {
+  async handleFileClick({ path, variable: { id, namespace } }) {
     if (this.can.can('read variable', null, { path, namespace })) {
-      this.router.transitionTo('variables.variable', path);
+      this.router.transitionTo('variables.variable', id);
     }
   }
 }
diff --git a/ui/app/controllers/variables/path.js b/ui/app/controllers/variables/path.js
@@ -4,16 +4,23 @@ import { action } from '@ember/object';
 const ALL_NAMESPACE_WILDCARD = '*';
 
 export default class VariablesPathController extends Controller {
+  get absolutePath() {
+    return this.model?.absolutePath || '';
+  }
   get breadcrumbs() {
-    let crumbs = [];
-    this.model.absolutePath.split('/').reduce((m, n) => {
-      crumbs.push({
-        label: n,
-        args: [`variables.path`, m + n],
-      });
-      return m + n + '/';
-    }, []);
-    return crumbs;
+    if (this.absolutePath) {
+      let crumbs = [];
+      this.absolutePath.split('/').reduce((m, n) => {
+        crumbs.push({
+          label: n,
+          args: [`variables.path`, m + n],
+        });
+        return m + n + '/';
+      }, []);
+      return crumbs;
+    } else {
+      return [];
+    }
   }
 
   @controller variables;

diff --git a/ui/app/controllers/variables/variable.js b/ui/app/controllers/variables/variable.js
@@ -3,12 +3,14 @@ import Controller from '@ember/controller';
 export default class VariablesVariableController extends Controller {
   get breadcrumbs() {
     let crumbs = [];
-    this.params.path.split('/').reduce((m, n) => {
+    let id = decodeURI(this.params.id.split('@').slice(0, -1).join('@')); // remove namespace
+    let namespace = this.params.id.split('@').slice(-1)[0];
+    id.split('/').reduce((m, n) => {
       crumbs.push({
         label: n,
         args:
-          m + n === this.params.path // If the last crumb, link to the var itself
-            ? [`variables.variable`, m + n]
+          m + n === id // If the last crumb, link to the var itself
+            ? [`variables.variable`, `${m + n}@${namespace}`]
             : [`variables.path`, m + n],
       });
       return m + n + '/';

diff --git a/ui/app/helpers/trim-path.js b/ui/app/helpers/trim-path.js
@@ -6,11 +6,12 @@ import Helper from '@ember/component/helper';
  * @param {Array<string>} params
  * @returns {string}
  */
-export function trimPath([path = '']) {
+export function trimPath([path]) {
+  if (!path) return;
   if (path.startsWith('/')) {
     path = trimPath([path.slice(1)]);
   }
-  if (path.endsWith('/')) {
+  if (path?.endsWith('/')) {
     path = trimPath([path.slice(0, -1)]);
   }
   return path;

diff --git a/ui/app/models/variable.js b/ui/app/models/variable.js
@@ -67,7 +67,9 @@ export default class VariableModel extends Model {
    */
   setAndTrimPath() {
     this.set('path', trimPath([this.path]));
-    this.set('id', this.get('path'));
+    if (!this.get('id')) {
+      this.set('id', `${this.get('path')}@${this.get('namespace')}`);
+    }
   }
 
   /**

diff --git a/ui/app/router.js b/ui/app/router.js
@@ -84,7 +84,7 @@ Router.map(function () {
     this.route(
       'variable',
       {
-        path: '/var/*path',
+        path: '/var/*id',
       },
       function () {
         this.route('edit');

diff --git a/ui/app/routes/variables.js b/ui/app/routes/variables.js
@@ -1,7 +1,7 @@
 import Route from '@ember/routing/route';
 import { inject as service } from '@ember/service';
 import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state';
-import notifyError from 'nomad-ui/utils/notify-error';
+import notifyForbidden from 'nomad-ui/utils/notify-forbidden';
 import PathTree from 'nomad-ui/utils/path-tree';
 
 export default class VariablesRoute extends Route.extend(WithForbiddenState) {
@@ -30,13 +30,13 @@ export default class VariablesRoute extends Route.extend(WithForbiddenState) {
         { namespace },
         { reload: true }
       );
-
       return {
         variables,
         pathTree: new PathTree(variables),
       };
     } catch (e) {
-      notifyError(this)(e);
+      notifyForbidden(this)(e);
+      return e;
     }
   }
 }
diff --git a/ui/app/routes/variables/index.js b/ui/app/routes/variables/index.js
@@ -1,11 +1,19 @@
 import Route from '@ember/routing/route';
+import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state';
+import notifyForbidden from 'nomad-ui/utils/notify-forbidden';
 
-export default class VariablesIndexRoute extends Route {
+export default class VariablesIndexRoute extends Route.extend(
+  WithForbiddenState
+) {
   model() {
-    const { variables, pathTree } = this.modelFor('variables');
-    return {
-      variables,
-      root: pathTree.paths.root,
-    };
+    if (this.modelFor('variables').errors) {
+      notifyForbidden(this)(this.modelFor('variables'));
+    } else {
+      const { variables, pathTree } = this.modelFor('variables');
+      return {
+        variables,
+        root: pathTree.paths.root,
+      };
+    }
   }
 }
diff --git a/ui/app/routes/variables/path.js b/ui/app/routes/variables/path.js
@@ -1,12 +1,20 @@
 import Route from '@ember/routing/route';
-export default class VariablesPathRoute extends Route {
+import WithForbiddenState from 'nomad-ui/mixins/with-forbidden-state';
+import notifyForbidden from 'nomad-ui/utils/notify-forbidden';
+export default class VariablesPathRoute extends Route.extend(
+  WithForbiddenState
+) {
   model({ absolutePath }) {
-    const treeAtPath =
-      this.modelFor('variables').pathTree.findPath(absolutePath);
-    if (treeAtPath) {
-      return { treeAtPath, absolutePath };
+    if (this.modelFor('variables').errors) {
+      notifyForbidden(this)(this.modelFor('variables'));
     } else {
-      return { absolutePath };
+      const treeAtPath =
+        this.modelFor('variables').pathTree.findPath(absolutePath);
+      if (treeAtPath) {
+        return { treeAtPath, absolutePath };
+      } else {
+        return { absolutePath };
+      }
     }
   }
 }
diff --git a/ui/app/routes/variables/variable.js b/ui/app/routes/variables/variable.js
@@ -9,7 +9,7 @@ export default class VariablesVariableRoute extends Route.extend(
   @service store;
   model(params) {
     return this.store
-      .findRecord('variable', decodeURIComponent(params.path), {
+      .findRecord('variable', decodeURIComponent(params.id), {
         reload: true,
       })
       .catch(notifyForbidden(this));