Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix ACL requirements for job details UI #11672
Changes from 5 commits
eefcfe4
5c9b6af
c92304b
a11b171
0c04a38
3d8a615
f837114
cac1550
96bf92f
dde13e7
e5e6fea
fe643d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 haveread
as well:https://github.com/hashicorp/nomad/pull/11672/files#diff-e4c09a2e6416b7748fae6b3d5e3010f68eb770c0b1d757eed0ab8287146bb1b2R18
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jobClientStatus
in the wrong place.JobClientStatusSummary
is the lowest common ancestor that should compute and store this derived state.ember-can
check. The associated controllers aren't fetchingnodes
(with the exception of thejobs/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 macrojobClientStatus
this should not be making any API calls, if it is then its a bug.There was a problem hiding this comment.
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
intoAbstractJob
and then we'll have access to this property.There was a problem hiding this comment.
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?