UI: Add exchange of one-time token on application load #10066
Conversation
Ember Asset Size actionAs of 412da49 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
|
@backspace just a heads up that #10134 has been merged. |
|
Thanks! Sadly I likely won’t be able to return to this until March 22, but hopefully things proceed smoothly then 😯 |
| <h1 data-test-error-title class="title is-spaced">Token Exchange Error</h1> | ||
| <p data-test-error-message class="subtitle"> | ||
| Failed to exchange the one-time token. | ||
| </p> |
There was a problem hiding this comment.
I followed the existing no-leader-error pattern and created the ott-exchange-error that’s checked for here to display this custom message… maybe a more generic known error type with custom title and message would be useful eventually 🤔
There was a problem hiding this comment.
Yeah. Errors and notifications I think are high on the "to refactor" list.
There was a problem hiding this comment.
🎉 Works as advertised!
I agree that it would be nice if the query param always got deleted, but I think your choice to ship this as is and follow up with that improvement is the right call.
Everything here is looking good aside from the couple br elements I'd like to avoid introducing. Aside from that, comments are commentary and weak suggestions.
ui/app/services/token.js
Outdated
| const token = yield TokenAdapter.exchangeOneTimeToken(oneTimeToken); | ||
| this.secret = token.secret; | ||
| }) | ||
| exchangeOneTimeToken; |
There was a problem hiding this comment.
Hm, even though it's consistent to the file to use an EC task here, looking at this with fresh eyes it seems like a simple promise would suffice? We aren't doing any fancy chaining or error control flow. Idk.
There was a problem hiding this comment.
I took it seriously in fa06c5e, it makes sense to remove the needless wrapping, thank you!
| <h1 data-test-error-title class="title is-spaced">Token Exchange Error</h1> | ||
| <p data-test-error-message class="subtitle"> | ||
| Failed to exchange the one-time token. | ||
| </p> |
There was a problem hiding this comment.
Yeah. Errors and notifications I think are high on the "to refactor" list.
| @@ -17,6 +17,12 @@ | |||
| requisite | |||
| {{/if}} | |||
| permission to view this. | |||
| <br> | |||
| <br> | |||
There was a problem hiding this comment.
Woah, is this the first instance of two paragraphs of text in the UI? I couldn't find any prior art for this type of spacing.
What do you think about changing empty-message-body to being a div and then making each line a p and then creating this paragraph spacing with margins instead?
Something like this in the .empty-message-body selector:
p:not(:last-child) {
margin-bottom: 0.5rem; // idk how much spacing this should actually be, somewhere between 0.5 and 1.
}There was a problem hiding this comment.
I did find this two-paragraph empty message, though it’s slightly different in that the second paragraph is a button. In 412da49 I added this within .empty-message-body:
&:not(:last-child) {
margin-bottom: 1rem;
}
and removed the unsightly <br>s, thanks for the push there. Here are screenshots of these two two-paragraph messages, does it seem suitable to you?
ui/app/routes/application.js
Outdated
| exchangeOneTimeToken = this.get('token.exchangeOneTimeToken') | ||
| .perform(transition.to.queryParams.ott) | ||
| .catch(() => { | ||
| this.controllerFor('application').set('error', new OTTExchangeError()); |
There was a problem hiding this comment.
I think ideally this error casting would happen in the data layer. Presumably in the token adapter.
Not a big deal though if that ends up being more trouble than it's worth.
There was a problem hiding this comment.
It was easy enough, makes sense, done in d5e2e5d, thank you!
As sensibly suggested here: #10066 (comment)
Oops! As suggested here: #10066 (comment)
A possible approach for this: #10066 (comment)
Nice work!This followup to #10066 adds a step to clear the one-time token from the URL after the application has loaded. The delay is required for it to actually clear, but only when the OTT is present to avoid slowing down the entire test suite.
|
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. |
This adds UI support for receiving the one-time token passed via query parameter, as in #10134
and related PRs, and exchanging it for its corresponding secret ID. When this works, it’s mostly
invisible, as seen here when used with
nomad ui -authenticate, where the OTT is briefly onscreen:You can also see the amended message when authentication fails suggesting to
-authenticate.When it fails, it shows a whole-page error:
Unfortunately, I was unable to produce the desired UX of having the OTT be erased from the URL
after processing, so I’m submitting this for review with that sad caveat. This will only be seen when
jumping directly with an identifier (
nomad ui -authenticate jobname), asnomad ui -authenticateforwards to
/ui/?ott=…which then forwards to/ui/jobsafter the exchange is complete, but it’sundesirable regardless. I’ll continue trying to find a way to cleanly and reliably remove it.
The whole-page error currently has this minimal text:
This is an edge case where I can’t actually imagine what would produce it, but the error doesn’t
really help you if it does happen… I’m open to suggestions for something better!