Skip to content
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

Merged
merged 52 commits into from
Jan 20, 2020
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
e38fa97
Add preliminary hardcoded job run ability check
backspace Jul 9, 2019
be5d47a
Change run job button to respect permissions
backspace Jul 9, 2019
e38bbef
Change permissions to require management token
backspace Jul 9, 2019
3c57124
Remove unused parameter
backspace Jul 9, 2019
19a3e30
Add token-clearing before regions test
backspace Jul 9, 2019
c77e458
Add placeholder HCL-to-JSON parser
backspace Jul 9, 2019
6509b6c
Add policy-fetch and interpretation
backspace Jul 10, 2019
4f207c2
Add missing yield
backspace Jul 10, 2019
dc98403
Add fetch of anonymous policy with no token
backspace Jul 10, 2019
6446054
Remove unused variable
backspace Jul 10, 2019
00291da
Add anonymous policy to default Mirage scenario
backspace Jul 10, 2019
2592278
Add Mirage hack to serve anonymous policy
backspace Jul 10, 2019
7452cb9
Add tooltip
backspace Jul 10, 2019
5aa4e03
Add filtering of policy route 404
backspace Jul 10, 2019
7d132c0
Change request-examination to ignore policy query
backspace Jul 10, 2019
aaab9c9
Merge branch 'master' into f-ui/respect-acl
backspace Jul 10, 2019
701ba1b
Add placeholder test for default fallback
backspace Jul 17, 2019
b1d618e
Add fallback to default namespace when no matches
backspace Jul 18, 2019
80d5720
Add partial support for globs
backspace Jul 18, 2019
de12248
Add match-length-comparison for glob names
backspace Jul 18, 2019
d949352
Add ability to match more than one wildcard
backspace Jul 19, 2019
313552a
Remove HCL-parsing in favour of API-provided JSON
backspace Jul 25, 2019
d2de141
Add question about only looking at capabilities
backspace Jul 25, 2019
a6641e2
Remove question about matchable characters
backspace Jul 25, 2019
0ef2fc2
Merge branch 'master' into f-ui/respect-acl
backspace Jul 26, 2019
25ad69b
Merge master
backspace Aug 29, 2019
cb91553
Merge branch 'f-ui/respect-acl' of github.com:hashicorp/nomad into f-…
backspace Aug 29, 2019
2a5e084
Change ability to check capabilities, not policy
backspace Aug 29, 2019
62efe99
Remove HCL versions of rules
backspace Aug 29, 2019
89ef215
Add link to explain namespace-matching
backspace Aug 29, 2019
08f5341
Update to latest Ember-can
backspace Aug 29, 2019
892f544
Add conditional button for mobile
backspace Aug 29, 2019
7b354ed
Correct module name
backspace Aug 29, 2019
05f4786
Correct dependent key
backspace Aug 29, 2019
e9ddcf2
Add explanatory comment
backspace Aug 29, 2019
2e21d59
Merge branch 'master' into f-ui/respect-acl
backspace Nov 26, 2019
b5cb8a9
Merge commit '2e21d594c7cc81c674de82c74f6f2fb33bbeafd6' into f-ui/res…
backspace Nov 26, 2019
f878d72
Remove comment superseded by #6021
backspace Nov 26, 2019
284e615
Remove unused dependent key
backspace Nov 26, 2019
d5c3071
Remove extraneous whitespace
backspace Dec 2, 2019
1dd9284
Change const to let
backspace Dec 2, 2019
c3a0afa
Remove extraneous autoformat changes
backspace Dec 2, 2019
71c2fac
Remove comment superseded by #6021
backspace Dec 3, 2019
6f68c0e
Remove Mirage anonymous policy
backspace Dec 3, 2019
777df99
Remove FIXME
backspace Dec 4, 2019
c50c55c
Change tooltip to be right-aligned
backspace Dec 4, 2019
94304e6
Remove uses of getWithDefault
backspace Dec 5, 2019
e054561
Change disabled div-buttons to true buttons
backspace Dec 5, 2019
9057d26
Replace error-handling with simpler implementation
backspace Dec 5, 2019
27cedda
Remove resolver override
backspace Dec 12, 2019
d72d8ed
Merge branch 'master' into f-ui/respect-acl
backspace Jan 20, 2020
ad82387
Convert test request-filtering to avoid Emberisms
backspace Jan 20, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions ui/app/abilities/job.js
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, getWithDefault } 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'),
Comment on lines +10 to +12
Copy link
Contributor

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.


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 = getWithDefault(policy, 'rulesJSON.Namespaces', []);
backspace marked this conversation as resolved.
Show resolved Hide resolved

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 = getWithDefault(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';
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 😆

});
1 change: 1 addition & 0 deletions ui/app/models/policy.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ export default Model.extend({
name: attr('string'),
description: attr('string'),
rules: attr('string'),
rulesJSON: attr(),
});
49 changes: 28 additions & 21 deletions ui/app/routes/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export default Route.extend({
config: service(),
system: service(),
store: service(),
token: service(),

queryParams: {
region: {
Expand All @@ -22,28 +23,34 @@ export default Route.extend({
},

beforeModel(transition) {
return RSVP.all([this.get('system.regions'), this.get('system.defaultRegion')]).then(
promises => {
if (!this.get('system.shouldShowRegions')) return promises;

const queryParam = transition.to.queryParams.region;
const defaultRegion = this.get('system.defaultRegion.region');
const currentRegion = this.get('system.activeRegion') || defaultRegion;

// Only reset the store if the region actually changed
if (
(queryParam && queryParam !== currentRegion) ||
(!queryParam && currentRegion !== defaultRegion)
) {
this.system.reset();
this.store.unloadAll();
}

this.set('system.activeRegion', queryParam || defaultRegion);

return promises;
const fetchSelfTokenAndPolicies = this.get('token.fetchSelfTokenAndPolicies')
.perform()
.catch();

return RSVP.all([
this.get('system.regions'),
this.get('system.defaultRegion'),
fetchSelfTokenAndPolicies,
]).then(promises => {
if (!this.get('system.shouldShowRegions')) return promises;

const queryParam = transition.to.queryParams.region;
const defaultRegion = this.get('system.defaultRegion.region');
const currentRegion = this.get('system.activeRegion') || defaultRegion;

// Only reset the store if the region actually changed
if (
(queryParam && queryParam !== currentRegion) ||
(!queryParam && currentRegion !== defaultRegion)
) {
this.system.reset();
this.store.unloadAll();
}
);

this.set('system.activeRegion', queryParam || defaultRegion);

return promises;
});
},

// Model is being used as a way to transfer the provided region
Expand Down
7 changes: 7 additions & 0 deletions ui/app/routes/jobs/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default Route.extend({
can: service(),
store: service(),
system: service(),

Expand All @@ -12,6 +13,12 @@ export default Route.extend({
},
],

beforeModel() {
if (this.can.cannot('run job')) {
this.transitionTo('jobs');
}
},
backspace marked this conversation as resolved.
Show resolved Hide resolved

model() {
return this.store.createRecord('job', {
namespace: this.get('system.activeNamespace'),
Expand Down
37 changes: 37 additions & 0 deletions ui/app/services/token.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import Service, { inject as service } from '@ember/service';
import { computed } from '@ember/object';
import { alias } from '@ember/object/computed';
import { getOwner } from '@ember/application';
import { assign } from '@ember/polyfills';
import { task } from 'ember-concurrency';
import queryString from 'query-string';
import fetch from 'nomad-ui/utils/fetch';

export default Service.extend({
store: service(),
system: service(),

secret: computed({
Expand All @@ -22,6 +26,39 @@ export default Service.extend({
},
}),

fetchSelfToken: task(function*() {
const TokenAdapter = getOwner(this).lookup('adapter:token');
try {
return yield TokenAdapter.findSelf();
} catch (e) {
return null;
}
}),

selfToken: alias('fetchSelfToken.lastSuccessful.value'),

fetchSelfTokenPolicies: task(function*() {
try {
if (this.selfToken) {
return yield this.selfToken.get('policies');
} else {
return yield this.store
.findRecord('policy', 'anonymous')
.then(policy => [policy])
.catch(() => []);
}
} catch (e) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure that you probably did this because you want things to fail silently here, I also noticed the empty catch up above somewhere, just thought I'd check that you don't want any user visible errors here, I'm guessing that's what this means.

Also, just curious more than anything, but there is this mix of try/catch / .then()/.catch() code here, is there anyway to write it so that you always use one style? No prob if not just wondering really.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s possible that something should happen if policy-fetching fails… I’m not sure what that would be, it’s maybe somewhat of a design question. But it’s true that it was overly convoluted, I’ve removed the redundancy so it’s more idiomatic, thanks.

}
}),

selfTokenPolicies: alias('fetchSelfTokenPolicies.lastSuccessful.value'),

fetchSelfTokenAndPolicies: task(function*() {
yield this.fetchSelfToken.perform();
yield this.fetchSelfTokenPolicies.perform();
}),

// All non Ember Data requests should go through authorizedRequest.
// However, the request that gets regions falls into that category.
// This authorizedRawRequest is necessary in order to fetch data
Expand Down
4 changes: 4 additions & 0 deletions ui/app/styles/components/tooltip.scss
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@
transition: top 0.1s ease-in-out;
}

.tooltip.is-right-aligned::after {
transform: translateX(-75%);
}

backspace marked this conversation as resolved.
Show resolved Hide resolved
.tooltip:hover::after,
.tooltip.always-active::after {
opacity: 1;
Expand Down
12 changes: 10 additions & 2 deletions ui/app/templates/jobs/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
<div data-test-run-job class="button tooltip is-right-aligned" aria-label="You don’t have permission to run jobs" disabled>Run Job</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed the disabled here, I think I've seen this working on form like things like fieldsets, does it work with divs also? Actually should this be a button not a div? I suppose its a disabled button/non-interactive button which may as well be a div 😁 , not sure what accessibility things might come into play here? But if it was me personally I'd prefer semantic HTML.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also just noticed the curl apostrophe, super nit I know, I'm guessing that will come out ok on all platforms, but thought I'd check just incase, do you use curly punctuation elsewhere in nomad?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I chose a div over a button because the thing it’s in parallel to is an a, but it’s true that having it be a true button makes the most sense, so I’ve changed it, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re the curly apostrophe, maybe it’s the only one that isn’t in code only; I type this way automatically so it doesn’t occur to me. Maybe @DingoEatingFuzz has a preference but I like them haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a fan of using proper typographic marks, therefore I am a fan of the curly apostrophe. That said, I haven't been disciplined about ensuring things like curly quotes, ellipses, en dashes, and the like.

{{/if}}
</div>
{{/if}}
<div class="toolbar-item is-right-aligned is-mobile-full-width">
Expand Down Expand Up @@ -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}}
<div data-test-run-job class="button tooltip is-right-aligned" aria-label="You don’t have permission to run jobs" disabled>Run Job</div>
{{/if}}
Copy link
Contributor

Choose a reason for hiding this comment

The 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 job-run-button component.

</div>
{{/if}}
</div>
Expand Down
8 changes: 8 additions & 0 deletions ui/mirage/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,14 @@ export default function() {
const secret = req.requestHeaders['X-Nomad-Token'];
const tokenForSecret = tokens.findBy({ secretId: secret });

if (req.params.id === 'anonymous') {
if (policy) {
return this.serialize(policy);
} else {
return new Response(404, {}, null);
}
}

// Return the policy only if the token that matches the request header
// includes the policy or if the token that matches the request header
// is of type management
Expand Down
1 change: 1 addition & 0 deletions ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

Choose a reason for hiding this comment

The 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-can!

"ember-cli": "~3.12.0",
"ember-cli-babel": "^7.7.3",
"ember-cli-clipboard": "^0.13.0",
Expand Down
4 changes: 3 additions & 1 deletion ui/tests/acceptance/allocation-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,9 @@ module('Acceptance | allocation detail', function(hooks) {
await Allocation.visit({ id: 'not-a-real-allocation' });

assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
server.pretender.handledRequests
.reject(request => request.url.includes('policy'))
backspace marked this conversation as resolved.
Show resolved Hide resolved
.findBy('status', 404).url,
'/v1/allocation/not-a-real-allocation',
'A request to the nonexistent allocation is made'
);
Expand Down
4 changes: 3 additions & 1 deletion ui/tests/acceptance/client-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,9 @@ module('Acceptance | client detail', function(hooks) {
await ClientDetail.visit({ id: 'not-a-real-node' });

assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
server.pretender.handledRequests
.reject(request => request.url.includes('policy'))
.findBy('status', 404).url,
'/v1/node/not-a-real-node',
'A request to the nonexistent node is made'
);
Expand Down
4 changes: 3 additions & 1 deletion ui/tests/acceptance/job-allocations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ module('Acceptance | job allocations', function(hooks) {
await Allocations.visit({ id: 'not-a-real-job' });

assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
server.pretender.handledRequests
.reject(request => request.url.includes('policy'))
.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
Expand Down
4 changes: 3 additions & 1 deletion ui/tests/acceptance/job-definition-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ module('Acceptance | job definition', function(hooks) {
await Definition.visit({ id: 'not-a-real-job' });

assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
server.pretender.handledRequests
.reject(request => request.url.includes('policy'))
.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
Expand Down
4 changes: 3 additions & 1 deletion ui/tests/acceptance/job-deployments-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,9 @@ module('Acceptance | job deployments', function(hooks) {
await Deployments.visit({ id: 'not-a-real-job' });

assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
server.pretender.handledRequests
.reject(request => request.url.includes('policy'))
.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
Expand Down
4 changes: 3 additions & 1 deletion ui/tests/acceptance/job-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ moduleForJob(
await JobDetail.visit({ id: 'not-a-real-job' });

assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
server.pretender.handledRequests
.reject(request => request.url.includes('policy'))
.findBy('status', 404).url,
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
Expand Down
4 changes: 3 additions & 1 deletion ui/tests/acceptance/job-evaluations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ module('Acceptance | job evaluations', function(hooks) {
await Evaluations.visit({ id: 'not-a-real-job' });

assert.equal(
server.pretender.handledRequests.findBy('status', 404).url,
server.pretender.handledRequests
.reject(request => request.url.includes('policy'))
.findBy('status', 404).url,
backspace marked this conversation as resolved.
Show resolved Hide resolved
'/v1/job/not-a-real-job',
'A request to the nonexistent job is made'
);
Expand Down
14 changes: 14 additions & 0 deletions ui/tests/acceptance/job-run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import JobRun from 'nomad-ui/tests/pages/jobs/run';
const newJobName = 'new-job';
const newJobTaskGroupName = 'redis';

let managementToken, clientToken;

const jsonJob = overrides => {
return JSON.stringify(
assign(
Expand Down Expand Up @@ -45,6 +47,11 @@ module('Acceptance | job run', function(hooks) {
hooks.beforeEach(function() {
// Required for placing allocations (a result of creating jobs)
server.create('node');

managementToken = server.create('token');
clientToken = server.create('token');

window.localStorage.nomadTokenSecret = managementToken.secretId;
});

test('visiting /jobs/run', async function(assert) {
Expand Down Expand Up @@ -86,4 +93,11 @@ module('Acceptance | job run', function(hooks) {
`Redirected to the job overview page for ${newJobName} and switched the namespace to ${newNamespace}`
);
});

test('when the user doesn’t have permission to run a job, redirects to the job overview page', async function(assert) {
window.localStorage.nomadTokenSecret = clientToken.secretId;

await JobRun.visit();
assert.equal(currentURL(), '/jobs');
});
});
Loading