-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI: Change Run Job availability based on ACLs #5944
Changes from all commits
e38fa97
be5d47a
e38bbef
3c57124
19a3e30
c77e458
6509b6c
4f207c2
dc98403
6446054
00291da
2592278
7452cb9
5aa4e03
7d132c0
aaab9c9
701ba1b
b1d618e
80d5720
de12248
d949352
313552a
d2de141
a6641e2
0ef2fc2
25ad69b
cb91553
2a5e084
62efe99
89ef215
08f5341
892f544
7b354ed
05f4786
e9ddcf2
2e21d59
b5cb8a9
f878d72
284e615
d5c3071
1dd9284
c3a0afa
71c2fac
6f68c0e
777df99
c50c55c
94304e6
e054561
9057d26
27cedda
d72d8ed
ad82387
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import { Ability } from 'ember-can'; | ||
import { inject as service } from '@ember/service'; | ||
import { computed, get } from '@ember/object'; | ||
import { equal, or } from '@ember/object/computed'; | ||
|
||
export default Ability.extend({ | ||
system: service(), | ||
token: service(), | ||
|
||
canRun: or('selfTokenIsManagement', 'policiesSupportRunning'), | ||
|
||
selfTokenIsManagement: equal('token.selfToken.type', 'management'), | ||
|
||
activeNamespace: computed('system.activeNamespace.name', function() { | ||
return this.get('system.activeNamespace.name') || 'default'; | ||
}), | ||
|
||
rulesForActiveNamespace: computed('activeNamespace', 'token.selfTokenPolicies.[]', function() { | ||
let activeNamespace = this.activeNamespace; | ||
|
||
return (this.get('token.selfTokenPolicies') || []).toArray().reduce((rules, policy) => { | ||
let policyNamespaces = get(policy, 'rulesJSON.Namespaces') || []; | ||
|
||
let matchingNamespace = this._findMatchingNamespace(policyNamespaces, activeNamespace); | ||
|
||
if (matchingNamespace) { | ||
rules.push(policyNamespaces.find(namespace => namespace.Name === matchingNamespace)); | ||
} | ||
|
||
return rules; | ||
}, []); | ||
}), | ||
|
||
policiesSupportRunning: computed('rulesForActiveNamespace.@each.capabilities', function() { | ||
return this.rulesForActiveNamespace.some(rules => { | ||
let capabilities = get(rules, 'Capabilities') || []; | ||
return capabilities.includes('submit-job'); | ||
}); | ||
}), | ||
|
||
// Chooses the closest namespace as described at the bottom here: | ||
// https://www.nomadproject.io/guides/security/acl.html#namespace-rules | ||
_findMatchingNamespace(policyNamespaces, activeNamespace) { | ||
let namespaceNames = policyNamespaces.mapBy('Name'); | ||
|
||
if (namespaceNames.includes(activeNamespace)) { | ||
return activeNamespace; | ||
} | ||
|
||
let globNamespaceNames = namespaceNames.filter(namespaceName => namespaceName.includes('*')); | ||
|
||
let matchingNamespaceName = globNamespaceNames.reduce( | ||
(mostMatching, namespaceName) => { | ||
// Convert * wildcards to .* for regex matching | ||
let namespaceNameRegExp = new RegExp(namespaceName.replace(/\*/g, '.*')); | ||
let characterDifference = activeNamespace.length - namespaceName.length; | ||
|
||
if ( | ||
characterDifference < mostMatching.mostMatchingCharacterDifference && | ||
activeNamespace.match(namespaceNameRegExp) | ||
) { | ||
return { | ||
mostMatchingNamespaceName: namespaceName, | ||
mostMatchingCharacterDifference: characterDifference, | ||
}; | ||
} else { | ||
return mostMatching; | ||
} | ||
}, | ||
{ mostMatchingNamespaceName: null, mostMatchingCharacterDifference: Number.MAX_SAFE_INTEGER } | ||
).mostMatchingNamespaceName; | ||
|
||
if (matchingNamespaceName) { | ||
return matchingNamespaceName; | ||
} else if (namespaceNames.includes('default')) { | ||
return 'default'; | ||
} | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading through this whole file, I could make some nitpicks about potential performance gains around caching intermediate values and sorting lists before scanning them, but instead I won't 😛 There is unlikely to ever be so many namespaces that this becomes performance critical code. However I will point out that there is a lot going on in this ability file. It speaks to the possible need for a better policy primitive that can be used to make authoring abilities easier. I'm curious what your thoughts here are since you have spent more time working through policy parsing and traversing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that there’s a lot happening and that it’ll be worth extracting, I just tend to push that kind of thing off into the future when it’s actually needed to avoid premature abstraction. There was a time when I’d create elaborate generalised structures in anticipation and end up with something that didn’t work as well when it came time to be used elsewhere, so I now err on the side of solving the immediate problem and then trying to generalise when it becomes useful so the solution can be informed by real needs. I’m planning to check ACLs for the exec button so that time isn’t far off 😆 |
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,11 @@ | |
</div> | ||
{{#if (media "isMobile")}} | ||
<div class="toolbar-item is-right-aligned"> | ||
{{#link-to "jobs.run" data-test-run-job class="button is-primary"}}Run Job{{/link-to}} | ||
{{#if (can "run job")}} | ||
{{#link-to "jobs.run" data-test-run-job class="button is-primary"}}Run Job{{/link-to}} | ||
{{else}} | ||
<button data-test-run-job class="button tooltip is-right-aligned" aria-label="You don’t have permission to run jobs" disabled>Run Job</button> | ||
{{/if}} | ||
</div> | ||
{{/if}} | ||
<div class="toolbar-item is-right-aligned is-mobile-full-width"> | ||
|
@@ -48,7 +52,11 @@ | |
</div> | ||
{{#if (not (media "isMobile"))}} | ||
<div class="toolbar-item is-right-aligned"> | ||
{{#link-to "jobs.run" data-test-run-job class="button is-primary"}}Run Job{{/link-to}} | ||
{{#if (can "run job")}} | ||
{{#link-to "jobs.run" data-test-run-job class="button is-primary"}}Run Job{{/link-to}} | ||
{{else}} | ||
<button data-test-run-job class="button tooltip is-right-aligned" aria-label="You don’t have permission to run jobs" disabled>Run Job</button> | ||
{{/if}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The duplication is a bummer, but it's also not the worst. It's right at the point where it could be dangerous, but at least the two occurrences are co-located. I also wouldn't be opposed to |
||
</div> | ||
{{/if}} | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,7 @@ | |
"d3-transition": "^1.1.0", | ||
"ember-ajax": "^5.0.0", | ||
"ember-auto-import": "^1.2.21", | ||
"ember-can": "^2.0.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am literally about to work on something very similar to this PR, so thanks for the hinter for |
||
"ember-cli": "~3.12.0", | ||
"ember-cli-babel": "^7.7.3", | ||
"ember-cli-clipboard": "^0.13.0", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see this pattern being repeated a bunch. Might be worth thinking about by one of us as we repeat it for both exec and node drain.