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

edit ember-can to add additional attribute for namespace #10893

Merged
merged 5 commits into from
Jul 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .changelog/10893.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: Fixes bug where UI was not detecting namespace-specific capabilities.
```
9 changes: 7 additions & 2 deletions ui/app/components/task-group-row.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@ export default class TaskGroupRow extends Component {
@oneWay('taskGroup.count') count;
@alias('taskGroup.job.runningDeployment') runningDeployment;

@computed('runningDeployment')
get namespace() {
return this.get('taskGroup.job.namespace.name');
}
Comment on lines +22 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being used? The template is accessing the namespace from the task group directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, its used in line 28 below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks! I missed that.

Follow-up question: should this be used in the view then? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that means that we can clean things up. Great callout.


@computed('runningDeployment', 'namespace')
get tooltipText() {
if (this.can.cannot('scale job')) return "You aren't allowed to scale task groups";
if (this.can.cannot('scale job', null, { namespace: this.namespace }))
return "You aren't allowed to scale task groups";
if (this.runningDeployment) return 'You cannot scale task groups during a deployment';
return undefined;
}
Expand Down
5 changes: 3 additions & 2 deletions ui/app/controllers/jobs/job/task-group.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,10 @@ export default class TaskGroupController extends Controller.extend(
})
shouldShowScaleEventTimeline;

@computed('model.job.runningDeployment')
@computed('model.job.{namespace,runningDeployment}')
get tooltipText() {
if (this.can.cannot('scale job')) return "You aren't allowed to scale task groups";
if (this.can.cannot('scale job', null, { namespace: this.model.job.namespace.get('name') }))
return "You aren't allowed to scale task groups";
if (this.model.job.runningDeployment) return 'You cannot scale task groups during a deployment';
return undefined;
}
Expand Down
4 changes: 2 additions & 2 deletions ui/app/routes/jobs/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ export default class RunRoute extends Route {
},
];

beforeModel() {
if (this.can.cannot('run job')) {
beforeModel(transition) {
if (this.can.cannot('run job', null, { namespace: transition.to.queryParams.namespace })) {
this.transitionTo('jobs');
}
}
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/components/job-version.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
</span>
<div class="pull-right">
{{#unless this.isCurrent}}
{{#if (can "run job")}}
{{#if (can "run job" namespace=this.version.job.namespace)}}
<TwoStepButton
data-test-revert-to
@classes={{hash
Expand Down
7 changes: 4 additions & 3 deletions ui/app/templates/components/task-group-row.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,25 @@
{{#if this.taskGroup.scaling}}
<div
data-test-scale-controls
class="button-bar is-shadowless is-text bumper-left {{if (or this.runningDeployment (cannot "scale job")) "tooltip multiline"}}"
class="button-bar is-shadowless is-text bumper-left {{if (or this.runningDeployment (cannot "scale job" namespace=this.namespace)) "tooltip multiline"}}"
aria-label={{this.tooltipText}}>
<button
data-test-scale="decrement"
role="button"
aria-label="decrement"
class="button is-xsmall is-light"
disabled={{or this.isMinimum this.runningDeployment (cannot "scale job")}}
disabled={{or this.isMinimum this.runningDeployment (cannot "scale job" namespace=this.namespace)}}
onclick={{action "countDown"}}
type="button">
{{x-icon "minus-plain" class="is-text"}}
</button>
<button
data-test-scale-controls-increment
data-test-scale="increment"
role="button"
aria-label="increment"
class="button is-xsmall is-light"
disabled={{or this.isMaximum this.runningDeployment (cannot "scale job")}}
disabled={{or this.isMaximum this.runningDeployment (cannot "scale job" namespace=this.namespace)}}
onclick={{action "countUp"}}
type="button">
{{x-icon "plus-plain" class="is-text"}}
Expand Down
8 changes: 4 additions & 4 deletions ui/app/templates/jobs/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
</div>
{{#if (media "isMobile")}}
<div class="toolbar-item is-right-aligned">
{{#if (can "run job")}}
<LinkTo @route="jobs.run" data-test-run-job class="button is-primary">Run Job</LinkTo>
{{#if (can "run job" namespace=this.qpNamespace)}}
<LinkTo @route="jobs.run" @query={{hash namespace=this.qpNamespace}} data-test-run-job class="button is-primary">Run Job</LinkTo>
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't comment on the ability file, but I think we need to add an extra check to allow running jobs if the wildcard namespace (*) is selected.

For example, a policy like this:

namespace "dev" {
  policy = "write"
}

Should allow the Run Job button to be enabled. If the input job has a namespace that is not dev, then the job submission will fail. But the user should have the ability to at least open the job submit page if the * namespace is selected and their token has a submit-job capability in any namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Buck disagreed with that, I think. Which is why we had this issue: #10545.

Buck's opinions were that routes and views shouldn't be accessible if you don't have permissions. You shouldn't think you can go somewhere that you actually can't and then fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

But in the example that I gave you do have permission to run jobs in the dev namespace. If you are looking at All (*) namespaces, this includes dev, so you should be able to click on Run Job. Currently the button is disabled.

{{else}}
<button
data-test-run-job
Expand Down Expand Up @@ -66,8 +66,8 @@
</div>
{{#if (not (media "isMobile"))}}
<div class="toolbar-item is-right-aligned">
{{#if (can "run job")}}
<LinkTo @route="jobs.run" data-test-run-job class="button is-primary">Run Job</LinkTo>
{{#if (can "run job" namespace=this.qpNamespace)}}
<LinkTo @route="jobs.run" @query={{hash namespace=this.qpNamespace}} data-test-run-job class="button is-primary">Run Job</LinkTo>
{{else}}
<button
data-test-run-job
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/jobs/job/task-group.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
@max={{this.model.scaling.max}}
@value={{this.model.count}}
@class="is-primary is-small"
@disabled={{or this.model.job.runningDeployment (cannot "scale job")}}
@disabled={{or this.model.job.runningDeployment (cannot "scale job" namespace=this.model.job.namespace.name)}}
@onChange={{action "scaleTaskGroup"}}>
Count
</StepperInput>
Expand Down
68 changes: 68 additions & 0 deletions ui/tests/acceptance/job-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,4 +253,72 @@ module('Acceptance | job detail (with namespaces)', function(hooks) {
0
);
});

test('when the dynamic autoscaler is applied, you can scale a task within the job detail page', async function(assert) {
const SCALE_AND_WRITE_NAMESPACE = 'scale-and-write-namespace';
const READ_ONLY_NAMESPACE = 'read-only-namespace';
const clientToken = server.create('token');

const namespace = server.create('namespace', { id: SCALE_AND_WRITE_NAMESPACE });
const secondNamespace = server.create('namespace', { id: READ_ONLY_NAMESPACE });

job = server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: SCALE_AND_WRITE_NAMESPACE,
});

const job2 = server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: READ_ONLY_NAMESPACE,
});
const scalingGroup2 = server.create('task-group', {
job: job2,
name: 'scaling',
count: 1,
shallow: true,
withScaling: true,
});
job2.update({ taskGroupIds: [scalingGroup2.id] });

const policy = server.create('policy', {
id: 'something',
name: 'something',
rulesJSON: {
Namespaces: [
{
Name: SCALE_AND_WRITE_NAMESPACE,
Capabilities: ['scale-job', 'submit-job', 'read-job', 'list-jobs'],
},
{
Name: READ_ONLY_NAMESPACE,
Capabilities: ['list-jobs', 'read-job'],
},
],
},
});
const scalingGroup = server.create('task-group', {
job,
name: 'scaling',
count: 1,
shallow: true,
withScaling: true,
});
job.update({ taskGroupIds: [scalingGroup.id] });

clientToken.policyIds = [policy.id];
clientToken.save();
window.localStorage.nomadTokenSecret = clientToken.secretId;

await JobDetail.visit({ id: job.id, namespace: namespace.name });
assert.notOk(JobDetail.incrementButton.isDisabled);

await JobDetail.visit({ id: job2.id, namespace: secondNamespace.name });
assert.ok(JobDetail.incrementButton.isDisabled);
});
});
34 changes: 34 additions & 0 deletions ui/tests/acceptance/job-run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,38 @@ module('Acceptance | job run', function(hooks) {
await JobRun.visit();
assert.equal(currentURL(), '/jobs');
});

test('when using client token user can still go to job page if they have correct permissions', async function(assert) {
const clientTokenWithPolicy = server.create('token');
const newNamespace = 'second-namespace';

server.create('namespace', { id: newNamespace });
server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: newNamespace,
});

const policy = server.create('policy', {
id: 'something',
name: 'something',
rulesJSON: {
Namespaces: [
{
Name: newNamespace,
Capabilities: ['scale-job', 'submit-job', 'read-job', 'list-jobs'],
},
],
},
});

clientTokenWithPolicy.policyIds = [policy.id];
clientTokenWithPolicy.save();
window.localStorage.nomadTokenSecret = clientTokenWithPolicy.secretId;

await JobRun.visit({ namespace: newNamespace });
assert.equal(currentURL(), `/jobs/run?namespace=${newNamespace}`);
});
});
69 changes: 63 additions & 6 deletions ui/tests/acceptance/job-versions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ module('Acceptance | job versions', function(hooks) {
test('all versions but the current one have a button to revert to that version', async function(assert) {
let versionRowToRevertTo;

Versions.versions.forEach((versionRow) => {
Versions.versions.forEach(versionRow => {
if (versionRow.number === job.version) {
assert.ok(versionRow.revertToButton.isHidden);
} else {
Expand All @@ -67,7 +67,9 @@ module('Acceptance | job versions', function(hooks) {
await versionRowToRevertTo.revertToButton.idle();
await versionRowToRevertTo.revertToButton.confirm();

const revertRequest = this.server.pretender.handledRequests.find(request => request.url.includes('revert'));
const revertRequest = this.server.pretender.handledRequests.find(request =>
request.url.includes('revert')
);

assert.equal(revertRequest.url, `/v1/job/${job.id}/revert?namespace=${namespace.id}`);

Expand All @@ -81,7 +83,9 @@ module('Acceptance | job versions', function(hooks) {
});

test('when reversion fails, the error message from the API is piped through to the alert', async function(assert) {
const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
const versionRowToRevertTo = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowToRevertTo) {
const message = 'A plaintext error message';
Expand All @@ -104,7 +108,9 @@ module('Acceptance | job versions', function(hooks) {
});

test('when reversion has no effect, the error message explains', async function(assert) {
const versionRowToRevertTo = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
const versionRowToRevertTo = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowToRevertTo) {
// The default Mirage implementation updates the job version as passed in, this does nothing
Expand All @@ -116,7 +122,10 @@ module('Acceptance | job versions', function(hooks) {
assert.ok(Layout.inlineError.isShown);
assert.ok(Layout.inlineError.isWarning);
assert.ok(Layout.inlineError.title.includes('Reversion Had No Effect'));
assert.equal(Layout.inlineError.message, 'Reverting to an identical older version doesn’t produce a new version');
assert.equal(
Layout.inlineError.message,
'Reverting to an identical older version doesn’t produce a new version'
);
} else {
assert.expect(0);
}
Expand Down Expand Up @@ -154,7 +163,9 @@ module('Acceptance | job versions (with client token)', function(hooks) {
});

test('reversion buttons are disabled when the token lacks permissions', async function(assert) {
const versionRowWithReversion = Versions.versions.filter(versionRow => versionRow.revertToButton.isPresent)[0];
const versionRowWithReversion = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowWithReversion) {
assert.ok(versionRowWithReversion.revertToButtonIsDisabled);
Expand All @@ -164,4 +175,50 @@ module('Acceptance | job versions (with client token)', function(hooks) {

window.localStorage.clear();
});

test('reversion buttons are available when the client token has permissions', async function(assert) {
const REVERT_NAMESPACE = 'revert-namespace';
window.localStorage.clear();
const clientToken = server.create('token');

server.create('namespace', { id: REVERT_NAMESPACE });

const job = server.create('job', {
groupCount: 0,
createAllocations: false,
shallow: true,
noActiveDeployment: true,
namespaceId: REVERT_NAMESPACE,
});

const policy = server.create('policy', {
id: 'something',
name: 'something',
rulesJSON: {
Namespaces: [
{
Name: REVERT_NAMESPACE,
Capabilities: ['submit-job'],
},
],
},
});

clientToken.policyIds = [policy.id];
clientToken.save();

window.localStorage.nomadTokenSecret = clientToken.secretId;

versions = server.db.jobVersions.where({ jobId: job.id });
await Versions.visit({ id: job.id, namespace: REVERT_NAMESPACE });
const versionRowWithReversion = Versions.versions.filter(
versionRow => versionRow.revertToButton.isPresent
)[0];

if (versionRowWithReversion) {
assert.ok(versionRowWithReversion.revertToButtonIsDisabled);
} else {
assert.expect(0);
}
});
});
36 changes: 36 additions & 0 deletions ui/tests/acceptance/jobs-list-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,42 @@ module('Acceptance | jobs list', function(hooks) {
assert.equal(currentURL(), `/csi/volumes?namespace=${namespace.id}`);
});

test('when the user has a client token that has a namespace with a policy to run a job', async function(assert) {
lgfa29 marked this conversation as resolved.
Show resolved Hide resolved
const READ_AND_WRITE_NAMESPACE = 'read-and-write-namespace';
const READ_ONLY_NAMESPACE = 'read-only-namespace';

server.create('namespace', { id: READ_AND_WRITE_NAMESPACE });
server.create('namespace', { id: READ_ONLY_NAMESPACE });

const policy = server.create('policy', {
id: 'something',
name: 'something',
rulesJSON: {
Namespaces: [
{
Name: READ_AND_WRITE_NAMESPACE,
Capabilities: ['submit-job'],
},
{
Name: READ_ONLY_NAMESPACE,
Capabilities: ['list-job'],
},
],
},
});

clientToken.policyIds = [policy.id];
clientToken.save();

window.localStorage.nomadTokenSecret = clientToken.secretId;

await JobsList.visit({ namespace: READ_AND_WRITE_NAMESPACE });
assert.notOk(JobsList.runJobButton.isDisabled);

await JobsList.visit({ namespace: READ_ONLY_NAMESPACE });
assert.ok(JobsList.runJobButton.isDisabled);
});

pageSizeSelect({
resourceName: 'job',
pageObject: JobsList,
Expand Down
Loading