parse-server
parse-server copied to clipboard
feat: new auth adapter
New Pull Request Checklist
- [x] I am not disclosing a vulnerability.
- [x] I am creating this PR in reference to an issue.
Issue Description
Allow user to set up webauthn (biometric authentication on mobile device) once he is logged, and then i can log in with webauthn (fingerprint scan on mobile device). Rework auth adapter too support MFA systems, auth challenges systems and better interfaces when writing an auth adapter.
Related issue: #7042
Approach
Implement new WebAuthn Adapter with a challenge JWT strategy to avoid to store challenge into the db. Add new auth hook methods tiggered in dedicated use cases.
TODOs before merging
- [x] Add tests
- [x] Add changes to documentation (guides, repository pages, in-code descriptions)
- [x] Add security check
- [x] Add new Parse Error codes to Parse JS SDK
- [x] A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)
Futur work: Need to implement client side webauthn system in JS SDK for an easy usage.
Codecov Report
Merging #7079 (bc0e38c) into alpha (4de1c9b) will increase coverage by
0.13%. The diff coverage is100.00%.
@@ Coverage Diff @@
## alpha #7079 +/- ##
==========================================
+ Coverage 94.17% 94.30% +0.13%
==========================================
Files 182 182
Lines 13717 13880 +163
==========================================
+ Hits 12918 13090 +172
+ Misses 799 790 -9
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/Deprecator/Deprecations.js | 100.00% <ø> (ø) |
|
| src/GraphQL/loaders/parseClassTypes.js | 94.87% <ø> (ø) |
|
| src/Options/Definitions.js | 100.00% <ø> (ø) |
|
| src/Options/index.js | 100.00% <ø> (ø) |
|
| src/cloud-code/Parse.Cloud.js | 99.20% <ø> (ø) |
|
| src/Adapters/Auth/index.js | 98.73% <100.00%> (+5.18%) |
:arrow_up: |
| ...dapters/Storage/Postgres/PostgresStorageAdapter.js | 95.72% <100.00%> (+<0.01%) |
:arrow_up: |
| src/Auth.js | 99.54% <100.00%> (+0.23%) |
:arrow_up: |
| src/Config.js | 89.13% <100.00%> (+0.12%) |
:arrow_up: |
| src/GraphQL/loaders/usersMutations.js | 92.52% <100.00%> (+1.21%) |
:arrow_up: |
| ... and 7 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
WebAuthn adapter finished, Parse Server is ready to handle FIDO:
- biometric authentication on mobile device
- security keys like yubikey
- MBP with touch id
- And more
Since we need a username for webauthn : a developer can combine anonymous user + webauthn to allow full secure password less authentication :)
Need: #7052 to reduce changes
@davimacedo we can also merge this one directly and close #7052 , this branch has some small corrections compared to #7052
Great job @Moumouls ! @dplewis @mtrezza @dblythy any additional thoughts here?
Okay in real world implementation of an OTP SMS system i detected wrong implementation on challenge handling, i will push the fix then we will be okay !
Okay i pushed some fixes after some beta testing on my company project that use extensively the Auth System.
i implemented an SMS OTP adapter, everything works perfectly.
OTP Adapter pseudo code
export const OTPAuth = {
policy: 'additional',
async challenge(
challengeData: boolean,
authData: OtpAuthDataInput,
options: any,
req: any,
user?: Parse.User,
) {
if (!user || !user.get('phone')) throw new Error('User not found')
const otp = new OTP(user.id, user.get('phone'))
await SMSAdapter.send(
user.get('phone'),
`OTP code : ${await otp.generate()}`,
)
},
async validateSetUp(authData: CreateUpdateOtpAuthDataInput, options: any, req: AuthReq) {
return checkAuthorization(req)
},
async validateUpdate(authData: CreateUpdateOtpAuthDataInput, options: any, req: AuthReq) {
return checkAuthorization(req)
},
async validateLogin(authData: OtpAuthDataInput, options: any, req: AuthReq, user?: Parse.User) {
try {
// As an additional auth system user should be already identified
// with another default auth system
if (!authData.code || !user?.id) return throwError()
if (!user.get('phone')) return throwError()
const otp = new OTP(user.id, user.get('phone'))
await otp.check(authData.code)
return { doNotSave: true }
} catch (e) {
// eslint-disable-next-line @typescript-eslint/no-throw-literal
throw new Parse.Error(403, e.message)
}
},
}
@davimacedo @mtrezza @dplewis , so this one is now fully finished and also tested on a critical security project !
closed https://github.com/parse-community/parse-server/pull/7052 in favor of this one
@Moumouls it looks the tests are failing after the last commit
Thanks @davimacedo , I'll take look this morning. Do you think we can merge this one asap ?
Okay it seems that i pushed an
fdescribe... now it should be okay
Looks good so far @Moumouls!
Would you see any value in adding a helper method to Parse.Cloud that can register auth, rather then it all being required in index.js?
For example, I have all my login code in /functions/login.js.
It would be nice to be able to write:
Parse.Cloud.registerAuth('authName', {...});
Rather than having to export and import back to index.js.
EDIT: you’ve already done so much on this PR so I might tackle this one after this is merged
So here after a master update, how can help to make this one merged ? :) @davimacedo @mtrezza
Resolving the open comments. There is an open discussion about stale auth data in the advisory.
@mtrezza @davimacedo this pr is now synced with master, but it seems that CI have issue with mongodb runner, any idea on this ?
Not sure what the issue here is, but looking at other PRs like https://github.com/parse-community/parse-server/pull/7312 the CI works (except for some nasty flaky tests).
You could try to revert the package-lock file and add the new dependencies without deleting the package-lock. Maybe this issue is due a sub-dependency.
@Moumouls Could you try to delete package-lock and regenerate the file? The error looks like git can't install a remote repo, it could be that a repo URI in package-lock is outdated.
Okay we should be on green here now 🚀 @mtrezza
Tried to regenerate the package lock the issue is still there.
It seems that adep of mongodb runner is not ok (mongodb-tools):
"node_modules/mongodb-runner": {
"version": "4.8.1",
"resolved": "https://registry.npmjs.org/mongodb-runner/-/mongodb-runner-4.8.1.tgz",
"integrity": "sha512-1jv7EEyh+ajvGmLwDMXY5BjT/Xqdxgf+AwPK99JHhgeoAISffS3l9Z1c/IIOsSCulMlL7KlR10TyUdzSHZqMhg==",
"dev": true,
"license": "Apache-2.0",
"dependencies": {
"async": "^3.1.0",
"clui": "^0.3.6",
"debug": "^4.1.1",
"fs-extra": "^8.1.0",
"is-mongodb-running": "^1.0.1",
"lodash.defaults": "^4.2.0",
"minimist": "^1.2.0",
"mkdirp": "^0.5.1",
"mongodb": "^3.4.0",
"mongodb-dbpath": "^0.0.1",
"mongodb-tools": "github:mongodb-js/mongodb-tools#0d1a90f49796c41f6d47c7c7999fe384014a16a0",
"mongodb-version-manager": "^1.4.3",
"untildify": "^4.0.0",
"which": "^2.0.1"
},
"bin": {
"mongodb-runner": "bin/mongodb-runner.js"
}
},
May be related: https://github.com/mongodb-js/runner/issues/184
Okay @mtrezza i found that since npm 7.0 (node 15) npm i rewrite the lock to v2 (i've this versions on my setup). I think we could add a new CI check with a simple regex to ensure that Parse Server lock is not changed/rewrited since it seems that it leads to weird issues 🙂 What do you think about this ? @dplewis may be you have a suggestion for this one ?
SDK (optional PR) synced: https://github.com/parse-community/Parse-SDK-JS/pull/1276
@Moumouls Good idea, we currently have no CI check for package-lock. I will look into adding one.
I think we should pick this up and merge in the next days. This is open for quite some time already.
@Moumouls From the changelog I understand that there are no breaking changes in this PR, apart from the token validation on login, correct? Could you please resolve the conflicts, take a look at the open existing review comments, and we give the review another go to merge this?
Yes @mtrezza no breaking changes, just some new interfaces and the security patch on authData validation.
i'll try to find some time next/end of the week to resolve conflicts and adjust last details
We should definitely merge this one asap
@Moumouls Do you need a hand in this, so we can get this merged?
Hi @mtrezza, i've some hard time to work on this I will try to do my best to re-sync with master
Changing to draft; PR needs rebase and resolving open questions in previous review feedback
⚠️ Important change for merging PRs from Parse Server 5.0 onwards!
We are planning to release the first beta version of Parse Server 5.0 in October 2021.
If a PR contains a breaking change and is not merged before the beta release of Parse Server 5.0, it cannot be merged until the end of 2022. Instead it has to follow the Deprecation Policy and phase-in breaking changes to be merged during the course of 2022.
One of the most voiced community feedbacks was the demand for predictability in breaking changes to make it easy to upgrade Parse Server. We have made a first step towards this by introducing the Deprecation Policy in February 2021 that assists to phase-in breaking changes, giving developers time to adapt. We will follow-up with the introduction of Release Automation and a branch model that will allow breaking changes only with a new major release, scheduled for the beginning of each calendar year.
We understand that some PRs are a long time in the making and we very much appreciate your contribution. We want to make it easy for PRs that contain a breaking change and were created before the introduction of the Deprecation Policy. These PRs can be merged with a breaking change without being phased-in before the beta release of Parse Server 5.0. We are making this exception because we appreciate that this is a time of transition that requires additional effort from contributors to adapt. We encourage everyone to prepare their PRs until the end of September and account for review time and possible adaptions.
If a PR contains a breaking change and should be merged before the beta release, please mention @parse-community/server-maintenance and we will coordinate with you to merge the PR.
Thanks for your contribution and support during this transition to Parse Server release automation!
@Moumouls Do you think you can schedule some time anytime soon to go over this PR and get it ready for merge? I understand from the previous comments that it doesn't include a breaking change, is that still correct?
I'm afraid, the longer this PR is stale, the more difficult it will become to merge it.
@mtrezza I'll take a look ☺️
Thanks for opening this pull request!
- 🎉 We are excited about your hands-on contribution!