From ac78fcdf94f90ae5e7ca65dab2d8def3b2be1385 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Thu, 26 Sep 2024 16:26:58 -0400 Subject: [PATCH 1/4] Modify variable access permissions for UI users with write in only certain namespaces --- .changelog/24073.txt | 3 +++ ui/app/abilities/variable.js | 27 +++++++++++++++++---------- 2 files changed, 20 insertions(+), 10 deletions(-) create mode 100644 .changelog/24073.txt diff --git a/.changelog/24073.txt b/.changelog/24073.txt new file mode 100644 index 000000000000..85c50ed4eabe --- /dev/null +++ b/.changelog/24073.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: Fixes an issue where variables paths would not let namespaced users write variables unless they also had * namespace variable write permissions +``` diff --git a/ui/app/abilities/variable.js b/ui/app/abilities/variable.js index ccc4c548a3b2..3dc647246e34 100644 --- a/ui/app/abilities/variable.js +++ b/ui/app/abilities/variable.js @@ -116,22 +116,29 @@ export default class Variable extends AbstractAbility { @computed('allPaths', 'namespace', 'path', 'token.selfTokenPolicies') get policiesSupportVariableWriting() { - if (this.namespace === WILDCARD_GLOB && this.path === WILDCARD_GLOB) { - // If you're checking if you can write from root, and you don't specify a namespace, - // Then if you can write in ANY path in ANY namespace, you can get to /new. + if (this.path === WILDCARD_GLOB) { + // If checking for write permission on the root path return this.policyNamespacesIncludeVariablesCapabilities( this.token.selfTokenPolicies, ['write'], - this._nearestMatchingPath(this.path) + WILDCARD_GLOB ); } else { - // Checking a specific path in a specific namespace. - // TODO: This doesn't cover the case when you're checking for the * namespace at a specific path. - // Right now we require you to specify yournamespace to enable the button. + // Checking a specific path const matchingPath = this._nearestMatchingPath(this.path); - return this.allPaths - .find((path) => path.name === matchingPath) - ?.capabilities?.includes('write'); + if (this.namespace === WILDCARD_GLOB) { + // Checking for the * namespace at a specific path + return this.policyNamespacesIncludeVariablesCapabilities( + this.token.selfTokenPolicies, + ['write'], + matchingPath + ); + } else { + // Checking a specific path in a specific namespace + return this.allPaths + .find((path) => path.name === matchingPath) + ?.capabilities?.includes('write'); + } } } From 468dc16e7323d8d9e224a5b567aa7506634e1239 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Tue, 1 Oct 2024 10:30:51 -0400 Subject: [PATCH 2/4] Addressing some PR comments --- ui/app/abilities/variable.js | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/ui/app/abilities/variable.js b/ui/app/abilities/variable.js index 3dc647246e34..b77c4c0640be 100644 --- a/ui/app/abilities/variable.js +++ b/ui/app/abilities/variable.js @@ -85,7 +85,7 @@ export default class Variable extends AbstractAbility { capabilities = [], path ) { - const namespacesWithVariableCapabilities = policies + const variableCapabilitiesAmongNamespaces = policies .toArray() .filter((policy) => get(policy, 'rulesJSON.Namespaces')) .map((policy) => get(policy, 'rulesJSON.Namespaces')) @@ -109,36 +109,27 @@ export default class Variable extends AbstractAbility { .compact(); // Check for requested permissions - return namespacesWithVariableCapabilities.some((abilityList) => { - return capabilities.includes(abilityList); + return variableCapabilitiesAmongNamespaces.some((abilityList) => { + return capabilities.includes(abilityList); // at least one of the capabilities is included in the list }); } @computed('allPaths', 'namespace', 'path', 'token.selfTokenPolicies') get policiesSupportVariableWriting() { - if (this.path === WILDCARD_GLOB) { - // If checking for write permission on the root path + const matchingPath = this._nearestMatchingPath(this.path); + + if (this.namespace === WILDCARD_GLOB) { + // Checking for write permission in any namespace at the given path return this.policyNamespacesIncludeVariablesCapabilities( this.token.selfTokenPolicies, ['write'], - WILDCARD_GLOB + matchingPath ); } else { - // Checking a specific path - const matchingPath = this._nearestMatchingPath(this.path); - if (this.namespace === WILDCARD_GLOB) { - // Checking for the * namespace at a specific path - return this.policyNamespacesIncludeVariablesCapabilities( - this.token.selfTokenPolicies, - ['write'], - matchingPath - ); - } else { - // Checking a specific path in a specific namespace - return this.allPaths - .find((path) => path.name === matchingPath) - ?.capabilities?.includes('write'); - } + // Checking a specific path in a specific namespace + return this.allPaths + .find((path) => path.name === matchingPath) + ?.capabilities?.includes('write'); } } From ca70e85aae929a811bf15d437885aba18e3d650a Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Tue, 1 Oct 2024 23:04:52 -0400 Subject: [PATCH 3/4] Variables index namespaces on * and ability checks are now namespaced --- ui/app/abilities/variable.js | 147 +++++++++++++++++++-------- ui/app/templates/variables/index.hbs | 2 +- 2 files changed, 105 insertions(+), 44 deletions(-) diff --git a/ui/app/abilities/variable.js b/ui/app/abilities/variable.js index b77c4c0640be..a9fb8e6fbbd1 100644 --- a/ui/app/abilities/variable.js +++ b/ui/app/abilities/variable.js @@ -60,12 +60,64 @@ export default class Variable extends AbstractAbility { ); } - @computed('path', 'allPaths') + /** + * Check if the user has read access to a specific path in a specific namespace. + * @returns {boolean} + */ + @computed( + 'allVariablePathRules', + 'namespace', + 'path', + 'token.selfTokenPolicies' + ) get policiesSupportVariableRead() { const matchingPath = this._nearestMatchingPath(this.path); - return this.allPaths - .find((path) => path.name === matchingPath) - ?.capabilities?.includes('read'); + if (this.namespace === WILDCARD_GLOB) { + return this.policyNamespacesIncludeVariablesCapabilities( + this.token.selfTokenPolicies, + ['read'], + matchingPath + ); + } else { + return this.allVariablePathRules.some((rule) => { + const ruleMatchingPath = this._nearestMatchingPath(rule.name); + return ( + rule.namespace === this.namespace && + ruleMatchingPath === matchingPath && + rule.capabilities.includes('read') + ); + }); + } + } + + /** + * Check if the user has delete access to a specific path in a specific namespace. + * @returns {boolean} + */ + @computed( + 'allVariablePathRules', + 'namespace', + 'path', + 'token.selfTokenPolicies' + ) + get policiesSupportVariableDestroy() { + const matchingPath = this._nearestMatchingPath(this.path); + if (this.namespace === WILDCARD_GLOB) { + return this.policyNamespacesIncludeVariablesCapabilities( + this.token.selfTokenPolicies, + ['delete'], + matchingPath + ); + } else { + return this.allVariablePathRules.some((rule) => { + const ruleMatchingPath = this._nearestMatchingPath(rule.name); + return ( + rule.namespace === this.namespace && + ruleMatchingPath === matchingPath && + rule.capabilities.includes('delete') + ); + }); + } } /** @@ -110,63 +162,72 @@ export default class Variable extends AbstractAbility { // Check for requested permissions return variableCapabilitiesAmongNamespaces.some((abilityList) => { + ['write', 'read', 'destroy']; return capabilities.includes(abilityList); // at least one of the capabilities is included in the list }); } - @computed('allPaths', 'namespace', 'path', 'token.selfTokenPolicies') + /** + * Check if the user has write access to a specific path in a specific namespace. + * @returns {boolean} + */ + @computed( + 'allVariablePathRules', + 'namespace', + 'path', + 'token.selfTokenPolicies' + ) get policiesSupportVariableWriting() { const matchingPath = this._nearestMatchingPath(this.path); - if (this.namespace === WILDCARD_GLOB) { - // Checking for write permission in any namespace at the given path + // Check policyNamespacesIncludeVariablesCapabilities, which is namespace-agnostic. return this.policyNamespacesIncludeVariablesCapabilities( this.token.selfTokenPolicies, ['write'], matchingPath ); } else { - // Checking a specific path in a specific namespace - return this.allPaths - .find((path) => path.name === matchingPath) - ?.capabilities?.includes('write'); + // If the namespace is not wildcarded, then we dig into rules by namespace. + return this.allVariablePathRules.some((rule) => { + const ruleMatchingPath = this._nearestMatchingPath(rule.name); + return ( + rule.namespace === this.namespace && + ruleMatchingPath === matchingPath && + rule.capabilities.includes('write') + ); + }); } } - @computed('path', 'allPaths') - get policiesSupportVariableDestroy() { - const matchingPath = this._nearestMatchingPath(this.path); - return this.allPaths - .find((path) => path.name === matchingPath) - ?.capabilities?.includes('destroy'); - } - + /** + * Generate a list of all the path rules for all the policies + * that the user has access to. + * { + * namespace: string, + * name: string, + * capabilities: string[], + * } + * @returns {Array} + */ @computed('token.selfTokenPolicies.[]', 'namespace') - get allPaths() { + get allVariablePathRules() { return (get(this, 'token.selfTokenPolicies') || []) .toArray() - .reduce((paths, policy) => { - const namespaces = get(policy, 'rulesJSON.Namespaces'); - const matchingNamespace = this._nearestMatchingNamespace( - namespaces, - this.namespace - ); - - const variables = (namespaces || []).find( - (namespace) => namespace.Name === matchingNamespace - )?.Variables; - - const pathNames = variables?.Paths?.map((path) => ({ - name: path.PathSpec, - capabilities: path.Capabilities, - })); - - if (pathNames) { - paths = [...paths, ...pathNames]; - } - - return paths; - }, []); + .flatMap((policy) => { + const namespaces = get(policy, 'rulesJSON.Namespaces') || []; + + return namespaces.flatMap((namespace) => { + const variables = namespace.Variables; + const pathNames = + variables?.Paths?.map((path) => ({ + namespace: namespace.Name, + name: path.PathSpec, + capabilities: path.Capabilities, + })) || []; + + return pathNames; + }); + }); } _nearestMatchingNamespace(policyNamespaces, namespace) { @@ -199,7 +260,7 @@ export default class Variable extends AbstractAbility { } _nearestMatchingPath(path) { - const pathNames = this.allPaths.map((path) => path.name); + const pathNames = this.allVariablePathRules.map((path) => path.name); if (pathNames.includes(path)) { return path; } diff --git a/ui/app/templates/variables/index.hbs b/ui/app/templates/variables/index.hbs index 4cebdb013f0d..c97576dc59ca 100644 --- a/ui/app/templates/variables/index.hbs +++ b/ui/app/templates/variables/index.hbs @@ -23,7 +23,7 @@ {{/if}} - {{#if (can "write variable" path="*" namespace=this.namespaceSelection)}} + {{#if (can "write variable" path="*" namespace="*")}}
Date: Tue, 1 Oct 2024 23:51:12 -0400 Subject: [PATCH 4/4] Mistook Delete for Destroy, and update unit tests for mult-return allPaths --- .changelog/24073.txt | 2 +- ui/app/abilities/variable.js | 24 +++++++++------ ui/tests/unit/abilities/variable-test.js | 38 ++++++++++++++++++++---- 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/.changelog/24073.txt b/.changelog/24073.txt index 85c50ed4eabe..659c132a30ad 100644 --- a/.changelog/24073.txt +++ b/.changelog/24073.txt @@ -1,3 +1,3 @@ ```release-note:bug -ui: Fixes an issue where variables paths would not let namespaced users write variables unless they also had * namespace variable write permissions +ui: Fixes an issue where variables paths would not let namespaced users write variables unless they also had wildcard namespace variable write permissions ``` diff --git a/ui/app/abilities/variable.js b/ui/app/abilities/variable.js index a9fb8e6fbbd1..b0cc8949fb43 100644 --- a/ui/app/abilities/variable.js +++ b/ui/app/abilities/variable.js @@ -82,8 +82,10 @@ export default class Variable extends AbstractAbility { return this.allVariablePathRules.some((rule) => { const ruleMatchingPath = this._nearestMatchingPath(rule.name); return ( - rule.namespace === this.namespace && - ruleMatchingPath === matchingPath && + (rule.namespace === WILDCARD_GLOB || + rule.namespace === this.namespace) && + (ruleMatchingPath === WILDCARD_GLOB || + ruleMatchingPath === matchingPath) && rule.capabilities.includes('read') ); }); @@ -91,7 +93,7 @@ export default class Variable extends AbstractAbility { } /** - * Check if the user has delete access to a specific path in a specific namespace. + * Check if the user has destroy access to a specific path in a specific namespace. * @returns {boolean} */ @computed( @@ -105,16 +107,18 @@ export default class Variable extends AbstractAbility { if (this.namespace === WILDCARD_GLOB) { return this.policyNamespacesIncludeVariablesCapabilities( this.token.selfTokenPolicies, - ['delete'], + ['destroy'], matchingPath ); } else { return this.allVariablePathRules.some((rule) => { const ruleMatchingPath = this._nearestMatchingPath(rule.name); return ( - rule.namespace === this.namespace && - ruleMatchingPath === matchingPath && - rule.capabilities.includes('delete') + (rule.namespace === WILDCARD_GLOB || + rule.namespace === this.namespace) && + (ruleMatchingPath === WILDCARD_GLOB || + ruleMatchingPath === matchingPath) && + rule.capabilities.includes('destroy') ); }); } @@ -191,8 +195,10 @@ export default class Variable extends AbstractAbility { return this.allVariablePathRules.some((rule) => { const ruleMatchingPath = this._nearestMatchingPath(rule.name); return ( - rule.namespace === this.namespace && - ruleMatchingPath === matchingPath && + (rule.namespace === WILDCARD_GLOB || + rule.namespace === this.namespace) && + (ruleMatchingPath === WILDCARD_GLOB || + ruleMatchingPath === matchingPath) && rule.capabilities.includes('write') ); }); diff --git a/ui/tests/unit/abilities/variable-test.js b/ui/tests/unit/abilities/variable-test.js index 7b80e86783fe..f74d3099bce7 100644 --- a/ui/tests/unit/abilities/variable-test.js +++ b/ui/tests/unit/abilities/variable-test.js @@ -807,21 +807,27 @@ module('Unit | Ability | variable', function (hooks) { this.owner.register('service:token', mockToken); this.ability.namespace = 'bar'; - const allPaths = this.ability.allPaths; + const allPaths = this.ability.allVariablePathRules; assert.deepEqual( allPaths, [ + { + capabilities: ['write'], + name: 'foo', + namespace: 'default', + }, { capabilities: ['read', 'write'], name: 'foo', + namespace: 'bar', }, ], 'It should return the exact path match.' ); }); - test('it matches on default if no namespace is selected', function (assert) { + test('it matches if no namespace is selected', function (assert) { const mockToken = Service.extend({ aclEnabled: true, selfToken: { type: 'client' }, @@ -854,7 +860,7 @@ module('Unit | Ability | variable', function (hooks) { this.owner.register('service:token', mockToken); this.ability.namespace = undefined; - const allPaths = this.ability.allPaths; + const allPaths = this.ability.allVariablePathRules; assert.deepEqual( allPaths, @@ -862,9 +868,15 @@ module('Unit | Ability | variable', function (hooks) { { capabilities: ['write'], name: 'foo', + namespace: 'default', + }, + { + capabilities: ['read', 'write'], + name: 'foo', + namespace: 'bar', }, ], - 'It should return the exact path match.' + 'It should return both matches separated by namespace.' ); }); @@ -925,7 +937,7 @@ module('Unit | Ability | variable', function (hooks) { this.owner.register('service:token', mockToken); this.ability.namespace = 'pablo'; - const allPaths = this.ability.allPaths; + const allPaths = this.ability.allVariablePathRules; assert.deepEqual( allPaths, @@ -933,6 +945,22 @@ module('Unit | Ability | variable', function (hooks) { { capabilities: ['list'], name: '*', + namespace: '*', + }, + { + capabilities: ['list', 'read', 'destroy', 'create'], + name: '*', + namespace: 'namespace-1', + }, + { + capabilities: ['list', 'read', 'destroy', 'create'], + name: 'blue/*', + namespace: 'namespace-2', + }, + { + capabilities: ['list', 'read', 'create'], + name: 'nomad/jobs/*', + namespace: 'namespace-2', }, ], 'It should return the glob matching namespace match.'