Two Factor Authentication #6977
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6977 +/- ##
==========================================
- Coverage 93.87% 84.12% -9.75%
==========================================
Files 169 169
Lines 12438 12570 +132
==========================================
- Hits 11676 10575 -1101
- Misses 762 1995 +1233
Continue to review full report at Codecov.
|
|
Awesome, as I understand there are still some open questions regarding this PR, so let's continue the discussion in #4024 to see how to address the remaining obstacles, so the discussion is kept in one place. Once we have agreed on a solution, we can come back and review the updated PR. Do you think you can post a short conceptual outline in #4024 about this PR and which parts of it still need to be solved? Maybe others from the thread have some input as well. |
|
Thank you, I think i've covered most of the issues discussed in that thread. I was expecting changes to be made around the encryption as it's not something i've worked with before. If you wouldn't mind, could you let me know if there's anything I skimmed over, or misunderstood in the linked comment thread 😊. I'll just need to work on SDK endpoints, I'll get started on that when we're happy with the PR. For reference, I was following this draft. |
|
Can we call this MFA (multi-factor authentication) throughout the code, endpoints, DB fields and docs? That would make it easier for future contributors to read the code. In the docs it makes sense to additionally mention 2FA to make it easier for developers to understand / search for the feature. |
|
Just some thoughts:
|
|
Thanks for the thoughts guys. I've replaced How would you best see the Parse Server config to be set up? I think from a security perspective we should strongly recommend New error codes:
Flows: Enabling MFA:
Logging in with MFA:
Recovering an account:
EDIT: long pressing scanning on mobile only works in browser. The server would return an optauth:// URL, and if the user has Google Auth installed, opening the URL will add the token. If the user has no 2FA app installed we'd need seperate UI for that. The URL looks like: otpauth://totp/Example:alice@google.com?secret=JBSWY3DPEHPK3PXP&issuer=Example |
|
Thanks, that was quick, can you please add the error codes into the flow at the respective steps where they may be returned? So we can check whether the error messages cover all scenarios or need rewording. |
|
I’ve edited my comment above to provide that detail requested. I’ll also write some more tests around the expected errors. |
|
This is a really exciting feature! Have you thought about generating & returning backup codes and the facility to provide one to disable MFA if required? Not an expert myself but it seems to be what most services that have MFA do. |
|
Thanks @TomWFox! I was expecting to add that in a future rollout. I was thinking for the initial version that could be provided via cloud code. But I don't think it would be too hard to implement on top of this feature. |
|
Are recovery codes technically part of MFA or is that a technically separate feature? |
|
Seperate feature I believe. From otplib:
So, I'd imagine the flow for recovery codes would be:
Even when we build that out internally, I still think that developers that use our MFA would have to implement some sort of custom logic for users who lose their MFA device and their recovery tokens. |
|
This may cause an issue. A fix would be to add an option that's not
Is the intention to enable file encryption and 2 authentication at the same time with one option? |
|
Thanks for your thoughts @cbaker6. The The thought was to keep naming conventions consistent. Do you think this creates confusion? Edit: I've updated the initial post to make it clearer that the PR uses |
Not sure, it may be okay and I may be wrong. I saw reconfigure server in the test cases, and thought the option was getting passed directly to the parse server |
This seems okay and shouldn't cause a conflict IMO |
|
When added to the docs, we should probably recommend that all keys (especially encryption) should be different |
Correct, but yeah it will never mutate I foresee these features being added in future iterations:
|
I think it's fine to use the same name. I see the issue more in the fact that - thanks to our amazing contributors - the Parse Server feature set is constantly growing, but most options are still on the config root level. At some point it makes sense to restructure some options and group them by nesting, for better overview and reduce possible confusion.
It may make sense to at least conceptualize rotation before the MFA feature is merged, so that we don't run into difficulties later on, when deployments already have existing MFA tokens. |
The decryption and encryption are already there for the MFA secret, so I don't think it would be too hard at all. I'm just not overly familiar with the best way to perform bulk updates internally. Something like: async rotateMFA(oldKey,newKey) {
const users = await req.config.database.find('_User', {
$exists: _mfa // not sure if this is the best way to get users with MFA
});
const saveAll = [];
for (const user of users) {
const mfaKey = user._mfa;
const decrypted = await this.decryptMFAKey(mfaKey, oldKey);
const encrypted = this.encryptMFAKey(decrypted, newKey);
user._mfa = encrypted;
saveAll.push(user);
}
// not what the best save call would be
await Parse.User.saveAll(saveAll, {useMasterKey:true});
} |
|
Thanks for the example code, I understand now what you mean by key rotation. You are right that bulk updating will be tricky because it needs to be scalable. Updating a collection with millions of users can take a substantial amount of time. There are two aspects to consider:
It seems like we need an update process that allows for two parallel keys to be used, like a mechanism, that tries either key to verify MFA. I guess due to this complexity, it would be hard to viably conceptualize it now, so we may as well ignore this for now. |
This reverts commit 9113c99.
|
Can't seem to get the WiredTrigger test to pass, this error is showing: I have tried running locally with:
But the error isn't showing. Any pointers would be much appreciated. |
That's fine, we'll just wait with merging this PR until the JS SDK PR that includes the errors is released. The process when introducing new error codes is somewhat cumbersome unfortunately:
Could be a flaky test, you can try to close and re-open this PR to restart the CI. |
|
Historically we wouldn’t wait for an SDK release, I would wait in this case. You will have to add the new endpoints to SDK as well. New features are great but require some work. |
|
Just curious, how would we do it historically in other cases? |
Ok, no worries. Let me know what else needs to be done to merge. So, if I understand correctly, wait until JS SDK update for error codes, then change this PR to include them, and then work on JS SDK endpoints, JS docs, etc? |
If you look at most of the test cases they use the REST api instead of the SDK.
We have other SDKs too like PHP. It’s been a while since we’ve had a new feature that require both server and SDK updates. |
Yes, I was referring to the feature code itself. When Parse Server throws a Parse Error, these errors are defined in the JS SDK repo, not in the Parse Server repo as I understand, so also with the REST API, these error codes need to be defined in the JS SDK. I think it makes sense to have a single definition of Parse Error codes. We should improve that at some point to also have the error messages defined there, so it cannot happen like in this PR review that there are different error messages for the same error code. Or maybe move the Parse Errors to a separate repo and reference that from every other repo. That would make introducing new Parse Errors less convoluted. |
|
Sorry I gave a bad example. One new error code had been added in the last 2 years. A SDK release is in the works with the new GitHub actions CI. |
I am happy to work on the JS SDK methods and potentially swift but the other sdk’s will probably depend on another contributor to create the functionality, which obviously will require a lot of work across all the PRs. I’m happy to have a crack at them but I’d assume I’d need support from the repo’s maintainers / you guys. Could we potentially merge the feature undocumented/experimental to Parse Server and then “release” and document it when all the SDK methods are available? Considering this feature is likely to be widely adopted I’d think we’d need full SDK support. And it doesn’t really make sense to me to ask for help building the SDK for a feature based on a PR that may or may not be merged to the master. |
|
Let’s focus on one feature at a time. We’ll figure it out in the end. |
|
I have opened issue parse-community/Parse-SDK-JS#1269 to demonstrate the current Parse Error inconsistencies and problems arising from there. I will introduce a process for how to add new error codes to avoid making the current mess even worse. Therefore I suggest we wait for the release of the Parse JS SDK and replace error codes here with references to the error codes in the Parse JS SDK before merging this PR. We should establish a single source as reference to determine (un)used Parse Error codes, which is currently supposed to be the Parse JS SDK. |
There was a problem hiding this comment.
Since i'm gonna to work on the Webauthn system and after some check, i think here we should improve our AuthAdapter interface, since the MFA implementation should be feasible through the AuthAdapter. May be the MFA adapter could have some special handling into the code. But i'm in favor of improving AuthAdapter interface and triggers to allow this kind of addition touching as little as possible Routers Controllers.
Dependency injection and Adapter strategy is the source of the power of parse server and i think we should keep this strategy, by improving adapters capabilities.
@dblythy @davimacedo @dplewis ?
What do you think ?
@dblythy I can propose you to collaborate together on the new interface since we have 2 interesting case studies to manage MFA and Webauthn, then after the PR of the new interface we will be able to implement easily and quickly the new systems.
| for (const recToken of mfaRecTokens) { | ||
| const setAllowedFromMatch = async (recoveryKey, first) => { | ||
| const doesMatch = await passwordCrypto.compare(recoveryKey, recToken); | ||
| if (!doesMatch) { | ||
| return; | ||
| } | ||
| if (first) { | ||
| firstAllowed = true; | ||
| } else { | ||
| secondAllowed = true; | ||
| } | ||
| }; | ||
| await setAllowedFromMatch(recoveryKeysStr.substring(0, 20), true); | ||
| await setAllowedFromMatch(recoveryKeysStr.substring(21, 41)); | ||
| } |
There was a problem hiding this comment.
We should avoid for loop with await, use Promise.all or Promise.race
| const encryption = crypto | ||
| .createHash('sha256') | ||
| .update(String(encryptionKey)) | ||
| .digest('base64') | ||
| .substr(0, 32); |
There was a problem hiding this comment.
avoid duplicated code with encryptMfaKey, may be you should create a separate little util function
|
will probably need: #7050 |
|
I have closed this as @Moumouls is working on #7050 to be able to support more advanced auth adapters, and I think it makes more sense to handle MFA in an adapter as this could allow custom SMS / passwordless auth. I hope to work on a new MFA adapter that can be included as part of the master repo, after Antoine completes the new adapter handling. |
Hello again,
This PR creates a feature that has been long requested, and builds off #5306.
By setting
In your server settings, this PR will enforce mfa once users have signed up to it.
A couple of notes:
enableMFAreturns a QR code that can be displayed in apps / on sites. Users can then add it to Google Auth or whatever authenticator they use, and then they must pass the code toverifyMFA_mfa(because Parse doesn't allow for numbers in keys). IfmultiFactorAuth.encryptionKeyis set in the server settings, the secret keys will be encrypted on the server, using essentially the same algorithm that was built for the file encryption. Also setsMFAEnabledtotrueso that cloud code and app functionality can verify accordingly./recoverMFA.I imagine there will be a function user.enableMFA() and user.verifyMFA() in the future. I also think it would be cool if we could handle as much as the UI as possible.
Let me know what you think!
Closes #4024.