Skip to content

Update headless modal to show both Reject and Cancel#31069

Merged
ravicious merged 1 commit intomasterfrom
ravicious/headless-reject
Aug 29, 2023
Merged

Update headless modal to show both Reject and Cancel#31069
ravicious merged 1 commit intomasterfrom
ravicious/headless-reject

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented Aug 28, 2023

While adding test plan items for headless auth in Connect, I realized that the modal doesn't quite work if the cert expires.

I made an issue for that (#31070), but in short after clicking Approve the modal gets into this state:

Details

approve with expired cert

My natural instinct was to click Cancel – I assumed it'd close the modal. Instead, it attempted to reject the request and simply caused the error to reappear. In Connect, the Cancel button in modals typically is the equivalent of the close button in the top right.

I thought that it was a mistake and that the callback for that button should call onCancel instead of onReject. But after running git blame I realized that it's actually intentional (#28844), so I documented this in the code.

I started wondering why the button is called Cancel and not Reject like in the Web UI. I suspected that it was due to HeadlessPrompt reusing PromptWebauthn which doesn't allow for much customization.

This PR makes it so that we just copy the only relevant part of PromptWebauthn and show both the Reject button and the Cancel button.

Before After
before image

@ravicious
Copy link
Copy Markdown
Member Author

@gzdunek While I was going through the old PR to get some ideas as to what I should add to the test plan, I found a comment you made about the Cancel button being aligned to the right side rather than the left side. #29097 (comment)

I was thinking of fixing it here, but I actually like that they're on the other side. IMHO this signals that those are extra actions that are not part of the main flow – the user is expected to touch the key at this moment, but they might also decide to reject or cancel. If we kept buttons from step 2 on the same side as in step 1, I feel like it'd create this expectation that the user is supposed to choose between them now or something.

Copy link
Copy Markdown
Contributor

@Joerger Joerger left a comment

Choose a reason for hiding this comment

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

Thanks for following through with this!

As you pointed out, the lack of a Reject button here was something we noticed and figured it could be implemented later if needed. I didn't see it as a high priority at the time, as I was unaware of the cert expiration error. Next time, I'll create an issue to better track it.

@ravicious ravicious added this pull request to the merge queue Aug 29, 2023
Merged via the queue into master with commit 1908b61 Aug 29, 2023
@ravicious ravicious deleted the ravicious/headless-reject branch August 29, 2023 09:03
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants