(feat) Add support for TOTP 2FA to O3 #1385
Conversation
| }; | ||
|
|
||
| return ( | ||
| <form onSubmit={handleSubmit} style={{ maxWidth: 400, margin: '0 auto' }}> |
There was a problem hiding this comment.
As much as possible, we prefer to keep styles in scss files and pass in a className, rather than using the style attribute.
| return ( | ||
| <form onSubmit={handleSubmit} style={{ maxWidth: 400, margin: '0 auto' }}> | ||
| <h2>Login with Secret Question</h2> | ||
| <TextInput |
There was a problem hiding this comment.
It doesn't seem like we should need to prompt for the username here, since presumably they've already filled this in.
| return response; | ||
| }) | ||
| .then((response) => { | ||
| if (response.headers.has('location')) { |
There was a problem hiding this comment.
I'd probably check the followRedirects setting here too, even though it's necessary for the feature to work.
|
|
||
| return ( | ||
| <form onSubmit={handleSubmit} style={{ maxWidth: 400, margin: '0 auto' }}> | ||
| <h2>Login with Secret Question</h2> |
| value={username} | ||
| onChange={(e) => setUsername(e.target.value)} | ||
| required | ||
| /> |
There was a problem hiding this comment.
We also should not prompt the user for their Secret Question here - this is something that is stored in their user profile, not something they enter in. So ideally we would retrieve this via REST and (if needed) pass it back to the backend as a hidden input. The user should not need to enter or remember this question text.
| /> | ||
| <TextInput | ||
| id="answer" | ||
| labelText="Answer" |
There was a problem hiding this comment.
I would think that the label text for the answer should just be the value of the secret question, no? If we do want to have text called "Answer", then this should be translated.
| required | ||
| /> | ||
| <Button type="submit" style={{ marginTop: 16 }}> | ||
| Login |
There was a problem hiding this comment.
This should be translated and css externalized
| title="Error" | ||
| subtitle={error} | ||
| onCloseButtonClick={() => setError(null)} | ||
| style={{ marginTop: 16 }} |
There was a problem hiding this comment.
This should be translated and css externalized
| setTimeout(() => navigate('/login/location'), 0); | ||
| } | ||
| } else { | ||
| setError('Invalid MFA code'); |
| setTotp(''); | ||
| } | ||
| } catch (error) { | ||
| setError(error instanceof Error ? error.message : 'Failed to verify MFA code'); |
| <Logo t={t} /> | ||
| </div> | ||
| <form onSubmit={handleSubmit}> | ||
| <div className={styles.inputGroup}> |
There was a problem hiding this comment.
My guess is that we may want some kind of instructions / help text here that can remind a user how to log in with TOTP.
| <HeaderGlobalAction aria-label={t('setupMfa', 'Setup 2FA')} className={styles.setupMfaButton} onClick={setupTotp}> | ||
| <TwoFactorAuthentication size={20} /> | ||
| <span className={styles.setupMfaText}>{t('setupMfa', 'Setup 2FA')}</span> | ||
| </HeaderGlobalAction> |
There was a problem hiding this comment.
I guess this is fine for now, but doesn't seem ideal:
a) This is hard-coded to go to totp-seup, but is called Setup 2FA. What about secret question support?
b) We really need a page for a user to view their account settings, and to self-manage their account, beyond just using links in the header that is directly accessible from everywhere. I would be more inclined to add the ability to configure one's authentication methods via a user account page, rather than the header. Not sure if anything like this has been designed out (and also out of scope) but something to think about. @ibacher ?
| <Route path="logout" element={<RedirectLogout />} /> | ||
| <Route path="change-password" element={<ChangePassword />} /> | ||
| <Route path="login/totp" element={<LoginWithTotp />} /> | ||
| <Route path="totp-setup" element={<TOTPSetup />} /> |
There was a problem hiding this comment.
See my comments above about how I feel about this menu. Also, what is the use case for having LoginWithTotp available in the header? Doesn't the user need to already be logged in to see this?
denniskigen
changed the title
denniskigen
changed the title
3 participants
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
This is a (WIP) PR introducing 2FA to O3. At the moment it is still work in progress and needs an updated Authentication Module.
Screenshots
Related Issue
Other