parse-server icon indicating copy to clipboard operation
parse-server copied to clipboard

feat: new auth adapter

Open Moumouls opened this issue 4 years ago • 87 comments

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.

Moumouls avatar Dec 16 '20 17:12 Moumouls

Codecov Report

Merging #7079 (bc0e38c) into alpha (4de1c9b) will increase coverage by 0.13%. The diff coverage is 100.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.

codecov[bot] avatar Dec 16 '20 18:12 codecov[bot]

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

Moumouls avatar Dec 18 '20 18:12 Moumouls

Great job @Moumouls ! @dplewis @mtrezza @dblythy any additional thoughts here?

davimacedo avatar Jan 05 '21 18:01 davimacedo

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 !

Moumouls avatar Jan 06 '21 11:01 Moumouls

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)
		}
	},
}

Moumouls avatar Jan 07 '21 14:01 Moumouls

@davimacedo @mtrezza @dplewis , so this one is now fully finished and also tested on a critical security project !

Moumouls avatar Jan 12 '21 09:01 Moumouls

closed https://github.com/parse-community/parse-server/pull/7052 in favor of this one

Moumouls avatar Jan 18 '21 09:01 Moumouls

@Moumouls it looks the tests are failing after the last commit

davimacedo avatar Jan 19 '21 20:01 davimacedo

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

Moumouls avatar Jan 20 '21 08:01 Moumouls

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

dblythy avatar Feb 01 '21 01:02 dblythy

So here after a master update, how can help to make this one merged ? :) @davimacedo @mtrezza

Moumouls avatar Mar 02 '21 09:03 Moumouls

Resolving the open comments. There is an open discussion about stale auth data in the advisory.

mtrezza avatar Mar 02 '21 13:03 mtrezza

@mtrezza @davimacedo this pr is now synced with master, but it seems that CI have issue with mongodb runner, any idea on this ?

Moumouls avatar Apr 02 '21 10:04 Moumouls

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.

mtrezza avatar Apr 02 '21 13:04 mtrezza

@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.

mtrezza avatar Apr 05 '21 15:04 mtrezza

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

Moumouls avatar Apr 09 '21 07:04 Moumouls

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 ?

Moumouls avatar Apr 09 '21 07:04 Moumouls

SDK (optional PR) synced: https://github.com/parse-community/Parse-SDK-JS/pull/1276

Moumouls avatar Apr 09 '21 07:04 Moumouls

@Moumouls Good idea, we currently have no CI check for package-lock. I will look into adding one.

mtrezza avatar Apr 09 '21 09:04 mtrezza

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?

mtrezza avatar May 24 '21 10:05 mtrezza

Yes @mtrezza no breaking changes, just some new interfaces and the security patch on authData validation.

Moumouls avatar May 27 '21 14:05 Moumouls

i'll try to find some time next/end of the week to resolve conflicts and adjust last details

Moumouls avatar May 27 '21 14:05 Moumouls

We should definitely merge this one asap

Moumouls avatar May 27 '21 14:05 Moumouls

@Moumouls Do you need a hand in this, so we can get this merged?

mtrezza avatar Jun 02 '21 09:06 mtrezza

Hi @mtrezza, i've some hard time to work on this I will try to do my best to re-sync with master

Moumouls avatar Jun 03 '21 07:06 Moumouls

Changing to draft; PR needs rebase and resolving open questions in previous review feedback

mtrezza avatar Aug 05 '21 09:08 mtrezza

⚠️ 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!

mtrezza avatar Sep 03 '21 00:09 mtrezza

@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 avatar Nov 01 '21 21:11 mtrezza

@mtrezza I'll take a look ☺️

Moumouls avatar Nov 01 '21 22:11 Moumouls

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!