fix: adjust 2fa configuration ui to new api #1244
Conversation
|
🚀 Deployed on https://pr-1244--dhis2-user-profile.netlify.app |
HendrikThePendric
changed the title
| userProfileStore.state.twoFaEnabled = nextIsTwoFaEnabled | ||
| userProfileStore.setState(userProfileStore.state) |
There was a problem hiding this comment.
are the two needed? the mutation of mutation of userProfileStore directly, and the setState
There was a problem hiding this comment.
I'm not exactly sure, it could be possible to do the following:
userProfileStore.setState({
...userProfileStore.state,
twoFaEnabled: attemptingToEnableTwoFa,
})But this seems to be the idiomatic way to do it with the old Store in d2-ui: first mutate then call setState with the mutated object as a parameter....
There was a problem hiding this comment.
I approve from a code perspective, but defer to the UX review. Not really a UX comment, but it would be nice if we consistently called this the same thing throughout (sometimes it's "Two factor authentication", "2-factor authentication", "2 Factor Authentication" etc.,)
| lastActionWasTwoFaDisableSuccess, | ||
| setLastActionWasTwoFaDisableSuccess, | ||
| ] = useState(false) | ||
| const { show: showAlert } = useAlert(getAlertMessage, getAlertOptions) |
There was a problem hiding this comment.
I don't think this is a big issue (and maybe it's just a UX choice, in which case I would be quite 🙊), but it seems that the use of this showAlert function for all of these different alerts leads to them being replaced by one another rather than stacked on top of one another. So, like if I successfully enable and then disable my 2FA in succession, I only end up with the one success message about disabling...and the text just changes in the existing alert, so it's hard to see as a new alert.
Realistically, I don't think people would be making changes in such rapid succession that it really matters, but I just point it out (if this wasn't already the intent)
There was a problem hiding this comment.
This wasn't a consious decision, but now that I've thought about it a bit more, I think it could possibly make sense to keep it this way for simplicity's sake... I am thinking in particular about the scenario where a user could first get an error and then fixes things and gets a success message. The problem here is that the error is a "sticky" alert, it only hides when the user clicks the close icon. So when the success message comes in, the error message would still show below it, resulting in a very confusing situation. So then I'd end up calling hide() on the error alert before showing a success alert, and then I've pretty much ended up with a more complex implementation of what is effectively identical functionality as we have now.
* fix: twofactor design changes * chore: fix i18n --------- Co-authored-by: HendrikThePendric <[email protected]>
|
🎉 This PR is included in version 30.3.30 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes DHIS2-14555
I found the previous UI version to configure 2FA super confusing, and it didn't seem to actually match the process users were supposed to follow. In particular there were a few point that stood out:
So I've attempted to make things a bit clearer and whilst doing that I noticed I was restructuring things so much that I might as well bring this section of the app into 2023 and rewrite the lot. So I've upgraded all dependencies and implemented the 2FA section using our new toolbox.
I also added a new item to the sidebar, since this was already on a separate route anyway. I don't see why we wouldn't have it as a separate section.
In any case, a lot for @cooper-joe to review.
And I requested you @tomzemp because this work is loosely related to what you're doing in the login app.
2fa_config_demo.mov
On a general note, mostly for @cooper-joe I think:
As you can see in the screencast, the QR code is not showing once the account has 2FA enabled. This is partly due to a technical limitation: it's only possible to successfully fetch the QR Code image for users that currently have 2FA disabled. Almost all of the time this also makes sense from a practical perspective, but there are some edge cases where it could be useful to allow the user to set things up again. Personally I can actually only think of one use case, which would be when a user has bought a new phone and then logs in with his/her old phone and wants to set up the new phone for 2FA. But even for this use case there are some easy work arounds:
As such, probably the above is not really an issue. I can think of a lot of scenarios where users would effectively get locked out of their accounts, but those problems are more to do with shortcomings of the authenticator app in general. The only cases where it could help to show a QR for users that have 2FA configured already are cases where something goed wrong with the 2FA device (i.e. phone is replaced, authenticator app login record is accidentally deleted) during an active user session. I think these scenarios are too obscure to cater for.