fix: Return friendly error message when MFA is required but the user has no devices enrolled 🐛 #57909
fix: Return friendly error message when MFA is required but the user has no devices enrolled 🐛 #57909
Conversation
…has no devices enrolled Signed-off-by: Chris Thach <chris.thach@goteleport.com>
e292a62 to
fe0d42f
Compare
| text = `Access to this app requires multi-factor authentication (MFA), but user has no MFA devices; see Account Settings in the Web UI or use 'tsh mfa add' to register MFA devices. | ||
|
|
||
| See https://goteleport.com/docs/zero-trust-access/access-controls/guides/per-session-mfa/ for help. |
There was a problem hiding this comment.
Open to feedback on how this should be worded. I did try to follow the existing pattern established but wanted to provide a little more detail. Copied from
teleport/lib/auth/authclient/clt.go
Line 86 in c71b3f9
There was a problem hiding this comment.
What do you think about unifying all of our error messaging for this scenario? Right now tsh will result in MFA is required to access this resource but user has no MFA devices; see Account Settings in the Web UI or use 'tsh mfa add' to register MFA devices and the web ui will give you some form of only WebAuthn and SSO MFA methods are supported on the web terminal, please register a supported mfa method to connect to this server.
Maybe something similar to the following?
Multi-factor authentication is required to access this resource but the current user has no MFA devices enrolled; see Account Settings in the Web UI or use 'tsh mfa add' to register an MFA device.
@zmb3 any thoughts on this?
There was a problem hiding this comment.
Love the the suggestion! Will unify messaging
There was a problem hiding this comment.
Updated in 2d9c589. I also updated the screenshots in the PR description. I opted to reuse the custom error ErrNoMFADevices instead of copy-pasting the same message throughout.
I also opted to wrap the error with additional context in the case of accessing via the Web UI since we only support a subset of MFA methods that a user might have added (WebAuthn and SSO MFA).
Thoughts?
There was a problem hiding this comment.
I think I'd remove the error wrapping to keep the error messaging more concise and consistent.
There was a problem hiding this comment.
Yes, I agree!
Added in #57909 and updated screenshots in PR desc
…irements are not met upon connection to an App Signed-off-by: Chris Thach <chris.thach@goteleport.com>
6d5cc83 to
99802bb
Compare
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
| text = `Multi-factor authentication (MFA) is required to access this resource but the current user has no supported MFA devices enrolled; see Account Settings in the Web UI or use 'tsh mfa add' to register an MFA device. | ||
|
|
||
| See https://goteleport.com/docs/zero-trust-access/access-controls/guides/per-session-mfa/ for help. | ||
| ` |
There was a problem hiding this comment.
For consistency with the other error messages?
| text = `Multi-factor authentication (MFA) is required to access this resource but the current user has no supported MFA devices enrolled; see Account Settings in the Web UI or use 'tsh mfa add' to register an MFA device. | |
| See https://goteleport.com/docs/zero-trust-access/access-controls/guides/per-session-mfa/ for help. | |
| ` | |
| text = authclient.ErrNoMFADevices.Error() |
Signed-off-by: Chris Thach <chris.thach@goteleport.com>
… body assertion Signed-off-by: Chris Thach <chris.thach@goteleport.com>
|
I appreciate the reviews! I just pushed a commit cb8c839 updating the unit tests after you submitted your approvals. It's passing locally for me. Can you please give that look before we merge? |
…has no devices enrolled 🐛 (#57909) * fix: return friendly error message when MFA is required but the user has no devices enrolled Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make error messages consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove wrapped message Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add test for enrolled MFA device and update trusted device resp body assertion Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…has no devices enrolled 🐛 (#57909) * fix: return friendly error message when MFA is required but the user has no devices enrolled Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make error messages consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove wrapped message Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add test for enrolled MFA device and update trusted device resp body assertion Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…has no devices enrolled 🐛 (#57909) * fix: return friendly error message when MFA is required but the user has no devices enrolled Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make error messages consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove wrapped message Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add test for enrolled MFA device and update trusted device resp body assertion Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…has no devices enrolled 🐛 (#57909) * fix: return friendly error message when MFA is required but the user has no devices enrolled Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make error messages consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove wrapped message Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add test for enrolled MFA device and update trusted device resp body assertion Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…has no devices enrolled 🐛 (#57909) * fix: return friendly error message when MFA is required but the user has no devices enrolled Signed-off-by: Chris Thach <chris.thach@goteleport.com> * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: make error messages consistent Signed-off-by: Chris Thach <chris.thach@goteleport.com> * refactor: remove wrapped message Signed-off-by: Chris Thach <chris.thach@goteleport.com> * test: add test for enrolled MFA device and update trusted device resp body assertion Signed-off-by: Chris Thach <chris.thach@goteleport.com> --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…has no devices enrolled 🐛 (#57909) (#58045) * fix: return friendly error message when MFA is required but the user has no devices enrolled * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App * refactor: make error messages consistent * refactor: remove wrapped message * test: add test for enrolled MFA device and update trusted device resp body assertion --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…has no devices enrolled 🐛 (#57909) (#58042) * fix: return friendly error message when MFA is required but the user has no devices enrolled * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App * refactor: make error messages consistent * refactor: remove wrapped message * test: add test for enrolled MFA device and update trusted device resp body assertion --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
…has no devices enrolled 🐛 (#57909) (#58044) * fix: return friendly error message when MFA is required but the user has no devices enrolled * fix: improve error handling to provide clearer feedback when MFA requirements are not met upon connection to an App * refactor: make error messages consistent * refactor: remove wrapped message * test: add test for enrolled MFA device and update trusted device resp body assertion --------- Signed-off-by: Chris Thach <chris.thach@goteleport.com>
What?
Fixes the cluster client library and Apps connections handler so that they return a friendly error message when MFA is required and the user has no MFA devices enrolled. This is consistent with how the rest of the product handles this edge case (e.g., SSH, Kubernetes, Desktop, etc).
changelog: Improve error message when a User without any MFA devices enrolled attempts to access a resource that requires MFA.
Why?
Resolves the following issues:
After testing the SSH, Desktop, Kubernetes, DB, etc product features, it was found that only CLIs and Apps features do not properly handle this edge case.
Once this PR is merged, the product should handle this MFA-UX edge case in a consistent way
Screenshots
When accessing an SSH host through
tshand the user does not have a MFA device enrolled:When accessing an App through the UI and the user does not have a MFA device enrolled:
How was this tested?
./tsh ssh root@Macusing the GitHub SSO user produces the expected errorTODOs
tshin v19tshBackport?
Yes, testing in progress and I'll fill this affected versions matrix as I figure out the extent of this bug. Backport PRs will be created and linked for all affected versions.
Legend
X axis: Teleport client version
Y axis: Teleport server version
Intersection meanings:
❓ = Untested, results coming 🔜
☺️ = Bug does not affect this Teleport server and client versions
🐛 = Bug affects this Teleport server and client versions
🙈 = Server-client versions are not supported so will not be tested