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.

Sign up for GitHub

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

Fix ACL requirements for job details UI #11672

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
eefcfe4
ui: add "read client" ability
lgfa29 Dec 13, 2021
5c9b6af
ui: don't fetch node data in job details page if ACL token doesn't ha…
lgfa29 Dec 13, 2021
c92304b
ui: auto-collapse job status in client summary if not allowed
lgfa29 Dec 14, 2021
a11b171
ui: check node read capability when displaying allocation and task row
lgfa29 Dec 14, 2021
0c04a38
changelog: add entry for #11672
lgfa29 Dec 14, 2021
3d8a615
ui: store basic node data from allocation details request
lgfa29 Dec 14, 2021
f837114
ui: check for null model in afterModel hook
lgfa29 Dec 14, 2021
cac1550
ui: update tests after adding check for node:read ACLs in job details
lgfa29 Dec 14, 2021
96bf92f
ui: refactor unauthorized component for job client statu summary
lgfa29 Dec 14, 2021
dde13e7
ui: refactor conditional rendering of job client summary
lgfa29 Dec 14, 2021
e5e6fea
ui: refactor job client summary and related job details components
lgfa29 Dec 16, 2021
fe643d1
ui: fix computed property dependency
lgfa29 Dec 16, 2021
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
View file
Open in desktop
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
View file
Open in desktop
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/parameterized-child.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import jobClientStatus from 'nomad-ui/utils/properties/job-client-status';
@classic
export default class ParameterizedChild extends PeriodicChildJobPage {
@alias('job.decodedPayload') payload;
@service can;
@service store;

@computed('payload')
Expand All @@ -26,4 +27,8 @@ export default class ParameterizedChild extends PeriodicChildJobPage {
get nodes() {
return this.store.peekAll('node');
}

get shouldDisplayClientInformation() {
return this.can.can('read client') && this.job.hasClientStatus;
}
lgfa29 marked this conversation as resolved.
Show resolved Hide resolved
}
5 changes: 4 additions & 1 deletion ui/app/components/job-page/parts/job-client-status-summary.js
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions ui/app/components/job-page/periodic-child.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import jobClientStatus from 'nomad-ui/utils/properties/job-client-status';

@classic
export default class PeriodicChild extends AbstractJobPage {
@service can;
@service store;

@computed('job.{name,id}', 'job.parent.{name,id}')
Expand All @@ -31,4 +32,8 @@ export default class PeriodicChild extends AbstractJobPage {
get nodes() {
return this.store.peekAll('node');
}

get shouldDisplayClientInformation() {
return this.can.can('read client') && this.job.hasClientStatus;
}
}
10 changes: 10 additions & 0 deletions ui/app/models/allocation.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 4 additions & 2 deletions ui/app/templates/components/job-page/parameterized-child.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@
</:before-namespace>
</JobPage::Parts::StatsBox>

{{#if this.job.hasClientStatus}}
{{#if this.shouldDisplayClientInformation}}
<JobPage::Parts::JobClientStatusSummary
@gotoClients={{this.gotoClients}}
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
/>
{{else}}
<JobPage::Parts::JobClientStatusSummary @job={{this.job}} @forceCollapsed="true" />
{{/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
61 changes: 36 additions & 25 deletions ui/app/templates/components/job-page/parts/job-client-status-summary.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<ListAccordion
data-test-job-summary
data-test-job-client-summary
@source={{array this.job}}
@key="id"
@startExpanded={{this.isExpanded}}
Expand All @@ -9,14 +9,16 @@
<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 +29,38 @@
/>
</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>
{{else if (cannot "read clients") }}
<div data-test-not-authorized-message class="empty-message">
<h3 class="empty-message-headline">Not Authorized</h3>
<p class="empty-message-body">
Your <LinkTo @route="settings.tokens">ACL token</LinkTo> does not provide <code>node:read</code> permission.
</p>
</div>
{{/if}}
</a.body>
</ListAccordion>
6 changes: 4 additions & 2 deletions ui/app/templates/components/job-page/periodic-child.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,17 @@
</:before-namespace>
</JobPage::Parts::StatsBox>

{{#if this.job.hasClientStatus}}
{{#if this.shouldDisplayClientInformation}}
<JobPage::Parts::JobClientStatusSummary
@gotoClients={{this.gotoClients}}
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
/>
{{else}}
<JobPage::Parts::JobClientStatusSummary @job={{this.job}} @forceCollapsed="true" />
{{/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
16 changes: 10 additions & 6 deletions ui/app/templates/components/job-page/sysbatch.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@

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

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

<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed="true" />
{{#if (can "read client")}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This check can go into the JobClientStatusSummary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it cant'...as soon as Ember accesses jobClientStatus it will make an API request to get the Node information, which will 403.

I moved the check to the param access instead, does this look better?

<JobPage::Parts::JobClientStatusSummary
  @job={{this.job}}
  @jobClientStatus={{if (can "read client") this.jobClientStatus}}
  @forceCollapsed={{cannot "read client"}}
  @gotoClients={{this.gotoClients}} />

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... I think this breaks my mental model of Ember, and my understanding of the Ember Way.

Currently, we're loading all the nodes into the store when we enter the jobs/job route. And then, we peek the store (no API call) when we're in a nested controller. To my knowledge this isn't a data loading component either.

Can we move this conversation to Whimsical and map this out? That behavior sounds like a lurking bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, we're loading all the nodes into the store when we enter the jobs/job route.

That's the problem I'm solving: we don't want to do this if the user's ACL token doesn't have permission to access the /v1/node API endpoint.

But if we try to read a node from the store, Ember will attempt to fetch it but will receive a 403.

Not sure if this what you're looking for, but this is the flow:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. We're using the computed macro jobClientStatus in the wrong place. JobClientStatusSummary is the lowest common ancestor that should compute and store this derived state.
  2. The API call happens on the route which is protected by the ember-can check. The associated controllers aren't fetching nodes (with the exception of the jobs/job/clients controller, and this tab should be protected using the can helper -- this is another associated bug for this issue) because they're peaking from the store. The component is not fetching data either. Now, there's one more concern, the computed macro jobClientStatus this should not be making any API calls, if it is then its a bug.

<JobPage::Parts::JobClientStatusSummary
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
@gotoClients={{this.gotoClients}} />
{{else}}
<JobPage::Parts::JobClientStatusSummary @job={{this.job}} @forceCollapsed="true" />
{{/if}}

<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{if (can "read client") true false}} />

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

Expand Down
14 changes: 9 additions & 5 deletions ui/app/templates/components/job-page/system.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,16 @@
{{/each}}
{{/if}}

<JobPage::Parts::JobClientStatusSummary
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
@gotoClients={{this.gotoClients}} />
{{#if (can "read client")}}
<JobPage::Parts::JobClientStatusSummary
@job={{this.job}}
@jobClientStatus={{this.jobClientStatus}}
@gotoClients={{this.gotoClients}} />
{{else}}
<JobPage::Parts::JobClientStatusSummary @job={{this.job}} @forceCollapsed="true" />
{{/if}}
lgfa29 marked this conversation as resolved.
Show resolved Hide resolved

<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed="true" />
<JobPage::Parts::Summary @job={{this.job}} @forceCollapsed={{if (can "read client") true false}} />
lgfa29 marked this conversation as resolved.
Show resolved Hide resolved

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

Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/components/job-subnav.hbs
View file
Open in desktop
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
12 changes: 7 additions & 5 deletions ui/app/templates/components/task-row.hbs
View file
Open in desktop
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
Loading