Fix ACL requirements for job details UI #11672

lgfa29 · 2021-12-14T01:20:54Z

#11078 added a new component to compute and display the job status in individual clients. This resulted in an unexpected increase in the ACL permissions required to navigate to the job detail page. Namely the job detail page would now require

node {
  policy = "read"
}

This resulted in unexpected Not Authorized error page (#11660).

This PR adds a new ability to check for read node permission. It also updates the job details components to only render pieces and components that require node data when ACL permits.

…ve proper permission

github-actions · 2021-12-14T01:27:04Z

Ember Asset Size action

As of fe643d1

Files that got Bigger 🚨:

File	raw	gzip
nomad-ui.js	+869 B	+624 B

Files that stayed the same size 🤷‍:

File	raw	gzip
vendor.js	0 B	0 B
nomad-ui.css	0 B	0 B
vendor.css	0 B	0 B

github-actions · 2021-12-14T01:58:06Z

Ember Test Audit comparison

	main	`fe643d1`	change
passes	1203	1211	+8
failures	0	0	0
flaky	0	0	0
duration	8m 08s 038ms	9m 30s 177ms	+1m 22s 139ms

github-actions · 2021-12-14T01:58:12Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on main:

Acceptance | job dispatch: required meta fields are properly indicated

gregone

I am not versed in all of nomad's ACL minutiae, but from a code perspective, this looks fine. Just one comment about the preloading of all nodes only for read (and not write) users?

gregone · 2021-12-14T10:57:55Z

ui/app/routes/jobs/job/index.js

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


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

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

ChaiWithJai

This diff is exposing how tightly-coupled the jobs route hierarchy template components are. Particularly, based on how many repeat conditionals we're using across the templates and the high use of prop-drilling.

I'm concerned that we won't be able to actually catch future bugs and continue bloating our templates with conditional checks.

Let's chat regarding drawing out the behavior for ACL token checks and move the conditional checks up the job chain to clean up the repeat code in our templates.

We can accomplish this with using a contextual component pattern and delete a couple hundred lines of code in the process.

ChaiWithJai · 2021-12-14T16:44:12Z

ui/app/templates/components/job-page/sysbatch.hbs

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


This check can go into the JobClientStatusSummary.

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}} />

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.

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:

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.

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.

ui/app/templates/components/job-page/system.hbs

ChaiWithJai · 2021-12-14T16:45:50Z

ui/app/templates/components/job-subnav.hbs

@@ -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)))}}


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

It doesn't seem to work for the subnav. Do components inherit property from parents?

ui/app/components/job-page/parameterized-child.js

ChaiWithJai · 2021-12-14T16:47:48Z

ui/app/components/job-page/parts/job-client-status-summary.js

  get isExpanded() {
+    if (this.forceCollapsed) return false;


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

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.

ui/app/templates/components/job-page/system.hbs

lgfa29 · 2021-12-14T21:20:34Z

This diff is exposing how tightly-coupled the jobs route hierarchy template components are. Particularly, based on how many repeat conditionals we're using across the templates and the high use of prop-drilling.

Yeah, there's quite a bit of repetition, but also these pages are similar but not identical. We could try to DRY some of it, but personally I'm not sure it would be bring enough benefits. There's always some quirky details that differ among them.

I'm concerned that we won't be able to actually catch future bugs and continue bloating our templates with conditional checks.

The test suite uses a common module, so a lot of the shared behaviour is tested automatically across all job types.

Let's chat regarding drawing out the behavior for ACL token checks and move the conditional checks up the job chain to clean up the repeat code in our templates.

We can accomplish this with using a contextual component pattern and delete a couple hundred lines of code in the process.

Not sure if I follow this part. The ACL check conditionally renders a specific part of the page, so I think it needs to be done at the component level?

lgfa29 · 2021-12-14T21:27:19Z

ui/tests/helpers/module-for-job.js

-      assert.equal(
-        currentURL(),
-        urlWithNamespace(`/jobs/${encodeURIComponent(job.id)}`, job.namespace)
+      const expectedURL = new URL(
+        urlWithNamespace(`/jobs/${encodeURIComponent(job.id)}`, job.namespace),
+        window.location
      );
+      const gotURL = new URL(currentURL(), window.location);
+
+      assert.deepEqual(gotURL.path, expectedURL.path);
+      assert.deepEqual(gotURL.searchParams, expectedURL.searchParams);
      assert.equal(document.title, `Job ${job.name} - Nomad`);
    });

    test('the subnav links to overview', async function(assert) {
      await JobDetail.tabFor('overview').visit();
-      assert.equal(
-        currentURL(),
-        urlWithNamespace(`/jobs/${encodeURIComponent(job.id)}`, job.namespace)
+
+      const expectedURL = new URL(
+        urlWithNamespace(`/jobs/${encodeURIComponent(job.id)}`, job.namespace),
+        window.location
      );
+      const gotURL = new URL(currentURL(), window.location);
+
+      assert.deepEqual(gotURL.path, expectedURL.path);
+      assert.deepEqual(gotURL.searchParams, expectedURL.searchParams);


I have no idea why these started to fail 🤷

The currentURL would have and extra query params for sort, so I'm parsing the URL to ignore order.

This fails on periodic and parameterized jobs. Likely points to some more lurking bugs in how we handle periodic and parameterized jobs.

lgfa29 · 2021-12-14T21:29:02Z

ui/tests/helpers/module-for-job.js

+      await Tokens.visit();
+      await Tokens.secret(clientToken.secretId).submit();


I think that we can't set a token directly into localStorage after visiting a page? I'm not sure if I missing something, but it didn't work in a few acceptance tests where visit is called in the beforeEach hook, so I had to do it this way.

And TBH I kind of like it, feels more intentional 😅

We need to establish boundaries for API's we don't control. Ideally, we should wrap localStorage access into a service and then write mocks.

This is a bug. We initialize query parameters on period and parameterized jobs whereas we don't follow this behavior for other jobs. Will need to take a deeper dive into this.

github-actions · 2021-12-14T21:34:45Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on main:

Acceptance | task group detail: /jobs/:id/:task-group second breadcrumb should link to the job for the task group
Integration | Component | TopoViz: clicking on an allocation in a deeply nested TopoViz::Node will associate sibling allocations with curves
Integration | Component | TopoViz: when the count of sibling allocations is high enough relative to the node count, curves are not rendered
Integration | Utility | exec-socket-xterm-adapter: resizing the window passes a resize message through the socket

ChaiWithJai

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.
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.
3. There's some more lurking bugs that this PR raises as mentioned in #2.
4. Now that tests are failing and we're circumnavigating currentURL(), I think I need to take a deeper look into this.

Can you give me 2 days to close out the breadcrumbs work so I can take a few hours to review this?

lgfa29 · 2021-12-15T15:12:53Z

Sure 👍

github-actions · 2021-12-16T01:18:37Z

Ember Test Audit flaky tests

Ember Test Audit detected these flaky tests on main:

Acceptance | job dispatch: required meta fields are properly indicated

github-actions · 2022-11-04T02:39:29Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

lgfa29 added 4 commits December 13, 2021 16:56

ui: add "read client" ability

eefcfe4

ui: don't fetch node data in job details page if ACL token doesn't ha…

5c9b6af

…ve proper permission

ui: auto-collapse job status in client summary if not allowed

c92304b

ui: check node read capability when displaying allocation and task row

a11b171

lgfa29 added this to the 1.2.4 milestone Dec 14, 2021

lgfa29 requested review from ChaiWithJai and a team December 14, 2021 01:20

changelog: add entry for #11672

0c04a38

vercel bot temporarily deployed to Preview – nomad December 14, 2021 01:22 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 14, 2021 01:22 View deployment

lgfa29 marked this pull request as draft December 14, 2021 03:10

gregone approved these changes Dec 14, 2021

View reviewed changes

tgross mentioned this pull request Dec 14, 2021

Cannot read a job in Nomad UI with ACL #11660

Closed

lgfa29 added 3 commits December 14, 2021 11:35

ui: store basic node data from allocation details request

3d8a615

ui: check for null model in afterModel hook

f837114

ui: update tests after adding check for node:read ACLs in job details

cac1550

vercel bot temporarily deployed to Preview – nomad December 14, 2021 16:41 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 14, 2021 16:41 View deployment

ChaiWithJai suggested changes Dec 14, 2021

View reviewed changes

ChaiWithJai reviewed Dec 14, 2021

View reviewed changes

ui/app/templates/components/job-page/system.hbs Outdated Show resolved Hide resolved

gregone self-requested a review December 14, 2021 17:02

lgfa29 self-assigned this Dec 14, 2021

lgfa29 added 2 commits December 14, 2021 14:59

ui: refactor unauthorized component for job client statu summary

96bf92f

ui: refactor conditional rendering of job client summary

dde13e7

vercel bot temporarily deployed to Preview – nomad December 14, 2021 20:54 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui December 14, 2021 20:54 View deployment

lgfa29 marked this pull request as ready for review December 14, 2021 21:20

lgfa29 requested a review from ChaiWithJai December 14, 2021 21:20

lgfa29 commented Dec 14, 2021

View reviewed changes

ChaiWithJai suggested changes Dec 15, 2021

View reviewed changes

ui: refactor job client summary and related job details components

e5e6fea

vercel bot deployed to Preview – nomad-storybook-and-ui December 16, 2021 00:42 View deployment

vercel bot temporarily deployed to Preview – nomad December 16, 2021 00:42 Inactive

ui: fix computed property dependency

fe643d1

vercel bot deployed to Preview – nomad-storybook-and-ui December 16, 2021 01:13 View deployment

vercel bot temporarily deployed to Preview – nomad December 16, 2021 01:13 Inactive

lgfa29 added the theme/ui label Dec 20, 2021

ChaiWithJai mentioned this pull request Jan 5, 2022

fix: authorization bug for job-client-status-summary #11780

Merged

lgfa29 assigned ChaiWithJai Jan 10, 2022

ChaiWithJai approved these changes Jan 10, 2022

View reviewed changes

lgfa29 merged commit 6b488bd into main Jan 13, 2022

lgfa29 deleted the ui-fix-acl-job-details branch January 13, 2022 02:26

github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ACL requirements for job details UI #11672

Fix ACL requirements for job details UI #11672

lgfa29 commented Dec 14, 2021

github-actions bot commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Dec 14, 2021

gregone left a comment

gregone Dec 14, 2021

lgfa29 Dec 14, 2021

ChaiWithJai left a comment

ChaiWithJai Dec 14, 2021

lgfa29 Dec 14, 2021

ChaiWithJai Dec 14, 2021

lgfa29 Dec 14, 2021

ChaiWithJai Dec 15, 2021

ChaiWithJai Dec 14, 2021

lgfa29 Dec 14, 2021

ChaiWithJai Dec 14, 2021

lgfa29 Dec 14, 2021

lgfa29 commented Dec 14, 2021

lgfa29 Dec 14, 2021 •

edited

Loading

ChaiWithJai Dec 15, 2021

lgfa29 Dec 14, 2021 •

edited

Loading

ChaiWithJai Dec 15, 2021

ChaiWithJai Dec 15, 2021

github-actions bot commented Dec 14, 2021

ChaiWithJai left a comment

lgfa29 commented Dec 15, 2021

github-actions bot commented Dec 16, 2021

github-actions bot commented Nov 4, 2022

		await Tokens.visit();
		await Tokens.secret(clientToken.secretId).submit();

Fix ACL requirements for job details UI #11672

Fix ACL requirements for job details UI #11672

Conversation

lgfa29 commented Dec 14, 2021

github-actions bot commented Dec 14, 2021 • edited Loading

Ember Asset Size action

github-actions bot commented Dec 14, 2021 • edited Loading

Ember Test Audit comparison

github-actions bot commented Dec 14, 2021

Ember Test Audit flaky tests

gregone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaiWithJai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgfa29 commented Dec 14, 2021

lgfa29 Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgfa29 Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 14, 2021

Ember Test Audit flaky tests

ChaiWithJai left a comment

Choose a reason for hiding this comment

lgfa29 commented Dec 15, 2021

github-actions bot commented Dec 16, 2021

Ember Test Audit flaky tests

github-actions bot commented Nov 4, 2022

github-actions bot commented Dec 14, 2021 •

edited

Loading

github-actions bot commented Dec 14, 2021 •

edited

Loading

lgfa29 Dec 14, 2021 •

edited

Loading

lgfa29 Dec 14, 2021 •

edited

Loading