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

Fix ACL requirements for job details UI #11672

Merged
merged 12 commits into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from 10 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/11672.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: Fix the ACL requirements for displaying the job details page
```
28 changes: 20 additions & 8 deletions ui/app/abilities/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,30 @@ import classic from 'ember-classic-decorator';
export default class Client extends AbstractAbility {
// Map abilities to policy options (which are coarse for nodes)
// instead of specific behaviors.
@or('bypassAuthorization', 'selfTokenIsManagement', 'policiesIncludeNodeRead')
canRead;

@or('bypassAuthorization', 'selfTokenIsManagement', 'policiesIncludeNodeWrite')
canWrite;

@computed('token.selfTokenPolicies.[]')
get policiesIncludeNodeWrite() {
// For each policy record, extract the Node policy
const policies = (this.get('token.selfTokenPolicies') || [])
.toArray()
.map(policy => get(policy, 'rulesJSON.Node.Policy'))
.compact();
get policiesIncludeNodeRead() {
return policiesIncludePermissions(this.get('token.selfTokenPolicies'), ['read', 'write']);
}

// Node write is allowed if any policy allows it
return policies.some(policy => policy === 'write');
@computed('token.selfTokenPolicies.[]')
get policiesIncludeNodeWrite() {
return policiesIncludePermissions(this.get('token.selfTokenPolicies'), ['write']);
}
}

function policiesIncludePermissions(policies = [], permissions = []) {
// For each policy record, extract the Node policy
const nodePolicies = policies
.toArray()
.map(policy => get(policy, 'rulesJSON.Node.Policy'))
.compact();

// Check for requested permissions
return nodePolicies.some(policy => permissions.includes(policy));
}
5 changes: 5 additions & 0 deletions ui/app/components/job-page/abstract.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import classic from 'ember-classic-decorator';

@classic
export default class Abstract extends Component {
@service can;
@service system;

job = null;
Expand All @@ -20,6 +21,10 @@ export default class Abstract extends Component {
// Set to a { title, description } to surface an error
errorMessage = null;

get shouldDisplayClientInformation() {
return this.can.can('read client') && this.job.hasClientStatus;
}

@action
clearErrorMessage() {
this.set('errorMessage', null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@ import classic from 'ember-classic-decorator';
export default class JobClientStatusSummary extends Component {
job = null;
jobClientStatus = null;
forceCollapsed = false;
gotoClients() {}

@computed
@computed('forceCollapsed')
get isExpanded() {
if (this.forceCollapsed) return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets turn forceCollapsed into a reactive getter and then this no longer needs to be a computed property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I follow.

forcedCollapsed is an external property that is set by the caller to force the collapse state. isExpanded is the getter used to set the component state.


const storageValue = window.localStorage.nomadExpandJobClientStatusSummary;
return storageValue != null ? JSON.parse(storageValue) : true;
}
Expand Down
2 changes: 2 additions & 0 deletions ui/app/components/list-accordion/accordion-head.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ export default class AccordionHead extends Component {
'data-test-accordion-head' = true;

buttonLabel = 'toggle';
tooltip = '';
isOpen = false;
isExpandable = true;
isDisabled = false;
item = null;

onClose() {}
Expand Down
10 changes: 10 additions & 0 deletions ui/app/models/allocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ export default class Allocation extends Model {
@fragment('resources') allocatedResources;
@attr('number') jobVersion;

// Store basic node information returned by the API to avoid the need for
// node:read ACL permission.
@attr('string') nodeName;
@computed
get shortNodeId() {
return this.belongsTo('node')
.id()
.split('-')[0];
}

@attr('number') modifyIndex;
@attr('date') modifyTime;

Expand Down
16 changes: 13 additions & 3 deletions ui/app/routes/jobs/job/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';
import { collect } from '@ember/object/computed';
import {
watchRecord,
Expand All @@ -9,12 +10,20 @@ import {
import WithWatchers from 'nomad-ui/mixins/with-watchers';

export default class IndexRoute extends Route.extend(WithWatchers) {
@service can;

async model() {
// Optimizing future node look ups by preemptively loading everything
await this.store.findAll('node');
return this.modelFor('jobs.job');
}

async afterModel(model) {
// Optimizing future node look ups by preemptively loading all nodes if
// necessary and allowed.
if (model && model.get('hasClientStatus') && this.can.can('read client')) {
await this.store.findAll('node');
Copy link

Choose a reason for hiding this comment

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

Do users with write access not need this optimization for some reason?

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'm not sure if this is how it works in other projects, but in Nomad ACL system a write permission assumes that you have read as well:
https://github.com/hashicorp/nomad/pull/11672/files#diff-e4c09a2e6416b7748fae6b3d5e3010f68eb770c0b1d757eed0ab8287146bb1b2R18

}
}

startWatchers(controller, model) {
if (!model) {
return;
Expand All @@ -29,7 +38,8 @@ export default class IndexRoute extends Route.extend(WithWatchers) {
list:
model.get('hasChildren') &&
this.watchAllJobs.perform({ namespace: model.namespace.get('name') }),
nodes: model.get('hasClientStatus') && this.watchNodes.perform(),
nodes:
this.can.can('read client') && model.get('hasClientStatus') && this.watchNodes.perform(),
});
}

Expand Down
34 changes: 22 additions & 12 deletions ui/app/templates/components/allocation-row.hbs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
<td data-test-indicators class="is-narrow">
{{#if this.allocation.unhealthyDrivers.length}}
<span data-test-icon="unhealthy-driver" class="tooltip text-center" role="tooltip" aria-label="Allocation depends on unhealthy drivers">
{{x-icon "alert-triangle" class="is-warning"}}
</span>
{{#if (can "read client")}}
{{#if this.allocation.unhealthyDrivers.length}}
<span data-test-icon="unhealthy-driver" class="tooltip text-center" role="tooltip" aria-label="Allocation depends on unhealthy drivers">
{{x-icon "alert-triangle" class="is-warning"}}
</span>
{{/if}}
{{/if}}
{{#if this.allocation.nextAllocation}}
<span data-test-icon="reschedule" class="tooltip text-center" role="tooltip" aria-label="Allocation was rescheduled">
Expand Down Expand Up @@ -38,20 +40,28 @@
</td>
{{#if (eq this.context "volume")}}
<td data-test-client>
<Tooltip @text={{this.allocation.node.name}}>
<LinkTo @route="clients.client" @model={{this.allocation.node}}>
{{this.allocation.node.shortId}}
</LinkTo>
<Tooltip @text={{this.allocation.nodeName}}>
{{#if (can "read client")}}
<LinkTo @route="clients.client" @model={{this.allocation.node}}>
{{this.allocation.shortNodeId}}
</LinkTo>
{{else}}
{{this.allocation.shortNodeId}}
{{/if}}
</Tooltip>
</td>
{{/if}}
{{#if (or (eq this.context "taskGroup") (eq this.context "job"))}}
<td data-test-job-version>{{this.allocation.jobVersion}}</td>
<td data-test-client>
<Tooltip @text={{this.allocation.node.name}}>
<LinkTo @route="clients.client" @model={{this.allocation.node}}>
{{this.allocation.node.shortId}}
</LinkTo>
<Tooltip @text={{this.allocation.nodeName}}>
{{#if (can "read client")}}
<LinkTo @route="clients.client" @model={{this.allocation.node}}>
{{this.allocation.shortNodeId}}
</LinkTo>
{{else}}
{{this.allocation.shortNodeId}}
{{/if}}
</Tooltip>
</td>
{{else if (or (eq this.context "node") (eq this.context "volume"))}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@

{{#if this.job.hasClientStatus}}
<JobPage::Parts::JobClientStatusSummary
@gotoClients={{this.gotoClients}}
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
/>
@jobClientStatus={{if this.shouldDisplayClientInformation this.jobClientStatus}}
@forceCollapsed={{not this.shouldDisplayClientInformation}}
@gotoClients={{this.gotoClients}} />
{{/if}}

<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{this.job.hasClientStatus}} />
<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{this.shouldDisplayClientInformation}} />

<JobPage::Parts::PlacementFailures @job={{this.job}} />

Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,28 @@
<ListAccordion
data-test-job-summary
data-test-job-client-summary
@source={{array this.job}}
@key="id"
@startExpanded={{this.isExpanded}}
@onToggle={{action this.persist}} as |a|
>
<a.head @buttonLabel={{if a.isOpen "collapse" "expand"}}>
<a.head
@buttonLabel={{if a.isOpen "collapse" "expand"}}
@tooltip={{if (cannot "read client") "You don’t have permission to read clients"}}
@isDisabled={{cannot "read client"}}
>
<div class="columns">
<div class="column is-minimum nowrap">
Job Status in Client
<span class="badge {{if a.isOpen "is-white" "is-light"}}">
{{this.jobClientStatus.totalNodes}}
</span>
{{#if this.jobClientStatus}}
<span class="badge {{if a.isOpen "is-white" "is-light"}}">
{{this.jobClientStatus.totalNodes}}
</span>
{{/if}}
<span class="tooltip multiline" aria-label="Aggreate status of job's allocations in each client.">
{{x-icon "info-circle-outline" class="is-faded"}}
</span>
</div>
{{#unless a.isOpen}}
{{#if (and this.jobClientStatus (not a.isOpen))}}
<div class="column">
<div class="inline-chart bumper-left">
<JobClientStatusBar
Expand All @@ -27,29 +33,31 @@
/>
</div>
</div>
{{/unless}}
{{/if}}
</div>
</a.head>
<a.body>
<JobClientStatusBar
@onSliceClick={{action this.onSliceClick}}
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
class="split-view" as |chart|
>
<ol data-test-legend class="legend">
{{#each chart.data as |datum index|}}
<li data-test-legent-label="{{datum.className}}" class="{{datum.className}} {{if (eq datum.label chart.activeDatum.label) "is-active"}} {{if (eq datum.value 0) "is-empty" "is-clickable"}}">
{{#if (gt datum.value 0)}}
<LinkTo @route="jobs.job.clients" @model={{this.job}} @query={{datum.legendLink.queryParams}}>
{{#if this.jobClientStatus}}
<JobClientStatusBar
@onSliceClick={{action this.onSliceClick}}
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
class="split-view" as |chart|
>
<ol data-test-legend class="legend">
{{#each chart.data as |datum index|}}
<li data-test-legent-label="{{datum.className}}" class="{{datum.className}} {{if (eq datum.label chart.activeDatum.label) "is-active"}} {{if (eq datum.value 0) "is-empty" "is-clickable"}}">
{{#if (gt datum.value 0)}}
<LinkTo @route="jobs.job.clients" @model={{this.job}} @query={{datum.legendLink.queryParams}}>
<JobPage::Parts::SummaryLegendItem @datum={{datum}} @index={{index}} />
</LinkTo>
{{else}}
<JobPage::Parts::SummaryLegendItem @datum={{datum}} @index={{index}} />
</LinkTo>
{{else}}
<JobPage::Parts::SummaryLegendItem @datum={{datum}} @index={{index}} />
{{/if}}
</li>
{{/each}}
</ol>
</JobClientStatusBar>
{{/if}}
</li>
{{/each}}
</ol>
</JobClientStatusBar>
{{/if}}
</a.body>
</ListAccordion>
8 changes: 4 additions & 4 deletions ui/app/templates/components/job-page/periodic-child.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@

{{#if this.job.hasClientStatus}}
<JobPage::Parts::JobClientStatusSummary
@gotoClients={{this.gotoClients}}
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
/>
@jobClientStatus={{if this.shouldDisplayClientInformation this.jobClientStatus}}
@forceCollapsed={{not this.shouldDisplayClientInformation}}
@gotoClients={{this.gotoClients}} />
{{/if}}

<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{this.job.hasClientStatus}} />
<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{this.shouldDisplayClientInformation}} />

<JobPage::Parts::PlacementFailures @job={{this.job}} />

Expand Down
5 changes: 3 additions & 2 deletions ui/app/templates/components/job-page/sysbatch.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@

<JobPage::Parts::JobClientStatusSummary
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
@jobClientStatus={{if this.shouldDisplayClientInformation this.jobClientStatus}}
@forceCollapsed={{not this.shouldDisplayClientInformation}}
@gotoClients={{this.gotoClients}} />

<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed="true" />
<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{this.shouldDisplayClientInformation}} />

<JobPage::Parts::PlacementFailures @job={{this.job}} />

Expand Down
5 changes: 3 additions & 2 deletions ui/app/templates/components/job-page/system.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@

<JobPage::Parts::JobClientStatusSummary
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
@jobClientStatus={{if this.shouldDisplayClientInformation this.jobClientStatus}}
@forceCollapsed={{not this.shouldDisplayClientInformation}}
@gotoClients={{this.gotoClients}} />

<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed="true" />
<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{this.shouldDisplayClientInformation}} />

<JobPage::Parts::PlacementFailures @job={{this.job}} />

Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/components/job-subnav.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
{{/if}}
<li data-test-tab="allocations"><LinkTo @route="jobs.job.allocations" @query={{hash namespace=this.job.namespace.id}} @model={{@job}} @activeClass="is-active">Allocations</LinkTo></li>
<li data-test-tab="evaluations"><LinkTo @route="jobs.job.evaluations" @query={{hash namespace=this.job.namespace.id}} @model={{@job}} @activeClass="is-active">Evaluations</LinkTo></li>
{{#if (and this.job.hasClientStatus (not this.job.hasChildren))}}
{{#if (and (can "read client") (and this.job.hasClientStatus (not this.job.hasChildren)))}}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move the shouldDisplayClientInformation into AbstractJob and then we'll have access to this property.

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 doesn't seem to work for the subnav. Do components inherit property from parents?

<li data-test-tab="clients"><LinkTo @route="jobs.job.clients" @query={{hash namespace=this.job.namespace.id}} @model={{@job}} @activeClass="is-active">Clients</LinkTo></li>
{{/if}}
</ul>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
</div>
<button
data-test-accordion-toggle
class="button is-light is-compact pull-right accordion-toggle {{unless this.isExpandable "is-invisible"}}"
class="button is-light is-compact pull-right accordion-toggle {{unless this.isExpandable "is-invisible"}} {{if this.tooltip "tooltip multiline"}}"
disabled={{if this.isDisabled "disabled"}}
aria-label={{this.tooltip}}
onclick={{action (if this.isOpen this.onClose this.onOpen) this.item}}
type="button">
{{this.buttonLabel}}
Expand Down
12 changes: 7 additions & 5 deletions ui/app/templates/components/task-row.hbs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
<td class="is-narrow">
{{#unless this.task.driverStatus.healthy}}
<span data-test-icon="unhealthy-driver" class="tooltip text-center" aria-label="{{this.task.driver}} is unhealthy">
{{x-icon "alert-triangle" class="is-warning"}}
</span>
{{/unless}}
{{#if (can "read client")}}
{{#unless this.task.driverStatus.healthy}}
<span data-test-icon="unhealthy-driver" class="tooltip text-center" aria-label="{{this.task.driver}} is unhealthy">
{{x-icon "alert-triangle" class="is-warning"}}
</span>
{{/unless}}
{{/if}}
</td>
<td data-test-name class="nowrap">
<LinkTo @route="allocations.allocation.task" @models={{array this.task.allocation this.task}} class="is-primary">
Expand Down
1 change: 1 addition & 0 deletions ui/mirage/factories/allocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ export default Factory.extend({
namespace,
jobId: job.id,
nodeId: node.id,
nodeName: node.name,
taskStateIds: [],
taskResourceIds: [],
taskGroup: taskGroup.name,
Expand Down
Loading