feat: new auth adapter #7079

Moumouls · 2020-12-16T17:50:51Z

New Pull Request Checklist

I am not disclosing a vulnerability.
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

Add tests
Add changes to documentation (guides, repository pages, in-code descriptions)
Add security check
Add new Parse Error codes to Parse JS SDK
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 · 2020-12-16T18:00:33Z

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%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`95.72% <100.00%> (+<0.01%)`	⬆️
src/Auth.js	`99.54% <100.00%> (+0.23%)`	⬆️
src/Config.js	`89.13% <100.00%> (+0.12%)`	⬆️
src/GraphQL/loaders/usersMutations.js	`92.52% <100.00%> (+1.21%)`	⬆️
... 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.

Moumouls · 2020-12-18T18:10:31Z

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

davimacedo · 2021-01-05T18:21:35Z

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

Moumouls · 2021-01-06T11:49:26Z

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 · 2021-01-07T14:44:05Z

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 · 2021-01-12T09:09:20Z

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

Moumouls · 2021-01-18T09:31:20Z

closed #7052 in favor of this one

davimacedo · 2021-01-19T20:38:35Z

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

Moumouls · 2021-01-20T08:09:35Z

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

davimacedo · 2021-01-22T05:53:37Z

spec/ParseUser.spec.js

@@ -1789,7 +1789,7 @@ describe('Parse.User testing', () => {
    });
  });

-  it('should allow login with old authData token', done => {
+  it('should not allow login with old authData token', async () => {


@mtrezza @dplewis should we change this? Any other comment for this PR?

Maybe we can pick up the discussion in the advisory, I think there were some open questions what it means to change this?

Should we add that as a breaking change?

dblythy · 2021-02-01T01:46:18Z

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

Moumouls · 2021-03-02T09:24:51Z

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

mtrezza · 2021-03-02T13:07:33Z

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

Moumouls · 2021-04-02T10:24:47Z

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

CHANGELOG.md

mtrezza · 2021-04-02T13:18:34Z

Not sure what the issue here is, but looking at other PRs like #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.

package.json

mtrezza · 2022-07-02T22:25:50Z

I guess #7079 (comment) cannot be easily suggested via review, so I hope @Moumouls can add this to the PR.

Co-authored-by: dblythy <[email protected]>

Moumouls · 2022-07-03T10:19:33Z

@mtrezza @dblythy , I'll not add #7079 (comment)

The AuthAdapter file is only used for documentation/example purposes. Also as you can see, all other adapters are in fact not classes, there is no notion of constructor and parameter validation. The policy key is just an optional key that a developer could add (or not add) to the adapter object to tell Parse Server how to handle the auth.

Anyway, I added validation on the policy key. it could help a developer to debug an auth adapter !

mtrezza · 2022-07-03T11:53:04Z

The policy key is just an optional key that a developer could add (or not add) to the adapter object to tell Parse Server how to handle the auth.

How would the developer add that key to the adapter?

dblythy

I haven't seen any tests for how a developer would throw a custom error from an auth adapter, such as challenge.
I have also found that if an error is thrown from validateSetUp, validateUpdate, it is not returned to the client or caught by the server. Errors thrown from challenge are returned as {"code":1,"message":"Internal server error."}.
I noticed the last argument in all the adapter functions is a Config option, which is inconsistent with the Parse Cloud protocol, and pending discussion in Expose Parse Server config in Cloud Code #7869. Config should either be exposed via config.get or req.config (as per the Parse Cloud trigger request), assigning as the last argument seems like a completely different option.
The request param (Parse.Cloud.TriggerRequest) field triggerName is always empty, it would be good to pass authAdapter.field to there (e.g challengeAdapter.challenge).
The request param (Parse.Cloud.TriggerRequest) field user is always empty, I think it's intuitive for the user calling the auth to be passed in here (can other users' with write access update authData on other users?)
The third argument for challenge and the second argument for the other functions is set to JSDocs as "additional options", but in my usage it shows up as the Auth adapter itself (perhaps it should be "this", but then what's the point of the argument):

{
      validateSetUp: [Function: validateSetUp],
      validateUpdate: [Function: validateUpdate],
      validateLogin: [Function: validateLogin],
      validateAppId: [Function: validateAppId],
      validateAuthData: [Function: validateAuthData],
      challenge: [Function: challenge],
      options: [Object]
},

Throwing from validateAuthData returns User not found regardless of the error
I think error code "OTHER CAUSE" is overused here, I think we should probably get some new error codes considering this is an Auth adapter upgrade. Other cause is code -1 which I think should probably only be used sparingly as a last resort. Your thoughts @mtrezza?
Perhaps the new Auth Adapter protocol should get its own spec - the spec file this in is quite bloated and more or less refers to custom auth providers.
I also think the tests should start with a simple test which maps out usage (no spies, expects on validateAppId fields for example) so developers can get an easy view of how to implement. All the existing tests are quite complex to cover the code - a few simple ones at the start e.g how Parse Cloud tests are done could be nice.
It would be good for an enum on policy so it's clear what "default", "solo" and "additional" actually mean
I like how you don't have to return promises - nice improvement.

Thanks again for your hard work here @Moumouls!

dblythy · 2022-07-04T11:07:29Z

src/RestQuery.js

@@ -659,7 +659,7 @@ RestQuery.prototype.runFind = function (options = {}) {
    .then(results => {
      if (this.className === '_User' && !findOptions.explain) {
        for (var result of results) {
-          cleanResultAuthData(result);
+          this.cleanResultAuthData(result, this.auth, this.config);


What's the purpose of these arguments?

dblythy · 2022-07-04T11:10:51Z

src/RestWrite.js

+    key => this.data.authData[key] && this.data.authData[key].id
+  );
+
+  if (!hasAuthDataId) return;


Suggested change

if (!hasAuthDataId) return;

if (!hasAuthDataId) {

return;

}

dblythy · 2022-07-04T11:11:59Z

src/RestWrite.js

+
+  if (!hasAuthDataId) return;
+
+  const r = await Auth.findUsersWithAuthData(this.config, this.data.authData);


Poorly named variable

dblythy · 2022-07-04T11:12:13Z

src/RestWrite.js

+};
+
+RestWrite.prototype.handleAuthData = async function (authData) {
+  const r = await Auth.findUsersWithAuthData(this.config, authData);


Poorly named variable

dblythy · 2022-07-04T11:14:39Z

src/Routers/UsersRouter.js

+      } catch (e) {
+        // Rewrite the error to avoid guess id attack
+        logger.error(e);
+        throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'User not found.');


Should the error here be ignored here? Could the error be thrown from await validator

dblythy · 2022-07-04T11:21:14Z

src/Adapters/Auth/index.js

-
-    return authDataValidator(adapter, appIds, providerOptions);
+    const authAdapter = loadAuthAdapter(provider, authOptions);
+    if (!authAdapter) return;


Suggested change

if (!authAdapter) return;

if (!authAdapter) {

return;

}

dblythy · 2022-07-04T11:22:10Z

src/Auth.js

+};
+
+const hasMutatedAuthData = (authData, userAuthData) => {
+  if (!userAuthData) return { hasMutatedAuthData: true, mutatedAuthData: authData };


Suggested change

if (!userAuthData) return { hasMutatedAuthData: true, mutatedAuthData: authData };

if (!userAuthData) {

return { hasMutatedAuthData: true, mutatedAuthData: authData };

}

dblythy · 2022-07-04T11:22:28Z

src/Auth.js

+  const mutatedAuthData = {};
+  Object.keys(authData).forEach(provider => {
+    // Anonymous provider is not handled this way
+    if (provider === 'anonymous') return;


Suggested change

if (provider === 'anonymous') return;

if (provider === 'anonymous') {

return;

}

dblythy · 2022-07-04T11:24:53Z

src/Routers/UsersRouter.js

+      user = await this._authenticateUserFromRequest(req);
+    }
+
+    if (!challengeData) throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'Nothing to challenge.');


Suggested change

if (!challengeData) throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'Nothing to challenge.');

if (!challengeData) {

throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'Nothing to challenge.');

}

dblythy · 2022-07-04T11:25:45Z

src/Routers/UsersRouter.js

+    if (typeof challengeData !== 'object')
+      throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'challengeData should be an object.');


Suggested change

if (typeof challengeData !== 'object')

throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'challengeData should be an object.');

if (typeof challengeData !== 'object') {

throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'challengeData should be an object.');

}

mtrezza · 2022-07-06T14:53:06Z

I think error code "OTHER CAUSE" is overused here, I think we should probably get some new error codes considering this is an Auth adapter upgrade. Other cause is code -1 which I think should probably only be used sparingly as a last resort. Your thoughts @mtrezza?

Yes

mtrezza · 2022-07-19T12:25:52Z

@Moumouls, @rhuanbarreto, any feedback to @dblythy's points?

rhuanbarreto · 2022-07-19T12:32:29Z

My only feedback is that this PR will never be merged once many discussions are outside the scope of the PR, like correct usage of error messages. I think its functionality should be split in many different PRs with features.

In this PR, the only feature I'm interested in is the ability to not save sensitive data (authData) in the database, once this is a security issue. If the database leaks, not only the session token but also auth data gets leaked which can lead to a bigger liability due to GDPR.

mtrezza · 2022-07-19T12:42:06Z

discussions are outside the scope of the PR

Which of the points in #7079 (review) do you see outside of the scope of this PR? Let's see what we can split off into separate PRs.

dblythy · 2022-07-19T13:00:08Z

like correct usage of error messages

Our community has been very vocal on breaking changes, and if we introduce new features only to potentially break them (such as the expected error codes - e.g, a developer builds an auth system around the error OTHER_CAUSE for it to only change to a different error one version later) that’s only going to cause more frustration and delays in line with our policies.

This PR is almost there in my view though and most of my feedback is around future proofing / making tests easier to interpret.

Moumouls · 2022-07-19T13:18:42Z

Hi @mtrezza, @rhuanbarreto @dblythy

Yes, I tried to do my best in this PR, but then I start to "waste" time trying to make these new features join the official Parse Server repo (like some other features).

This PR helped a lot, my team, to better control the auth, implement reusable 2FA, MFA, and improve the security of data saved in the database.

Parse Server auth system needed a major refactor. Maybe this PR could have been split into many tiny PR (the future will tell us maybe if a contributor wants to try), but a global overview was also needed to perform this refactor.

I tried to find the best time/effort/value to produce these changes. But for now, I'll not provide updates to this PR. And I'll not probably sync new changes that we do internally with my team to continue to improve Parse Auth.

I always try to push new features to Parse Server, like Defined Schema, Parse Server config in the request object, Mongo Serverless support, and a new auth system, but even for a "frequent" committer, it feels harder and harder to contribute to Parse Server.

I've many plans to improve Parse Server, but based on my current "feeling" and how things going on here I'll prefer to preserve my effort for my team and maybe start a complete fork (Yarn Monorepo, Typescript, Serverless goal, Terraform templates to run Parse Server in 100% serverless on GCP, focus on GraphQL, Test helper package for Jest, improve GraphQL API performance, Improve Parse Server query performance, A Job task adapter to send Jobs to services like Google Cloud Tasks).

I really want to send all of this to the community repo but it feels nearly impossible when I see how this PR is going.

Have a nice day all !

mtrezza · 2022-07-20T11:35:35Z

Complex changes require more scrutiny.

Everybody is thankful that you share your code and you surely invested a lot of time writing this change. Let's be mindful that others invested a lot of time as well to review your change. Review feedback is an important part of the open-source process to uphold code quality and we want to be thankful to everyone involved as their time is as valuable as yours.

Not every feedback needs to be incorporated. Some feedback may be ideas to inspire follow-up contributions while other feedback may be a requirement to merge. Then there is feedback in between where the change works as is but merging it likely causes complications later on. Evaluating these in a mindful discussion is what everyone hopefully aims for.

We have seen a similar pattern in #7091 where you submitted a change but didn't have the capacity to incorporate the review feedback. Someone else then continued your PR in #7418. That's legitimate in a collaborative environment.

So let's better invest our time to look at the detailed feedback point by point and determine:

what is an idea for a future improvment
what is essential to fix before merge
what is optional but would be highly beneficial before merge

mtrezza · 2022-09-08T12:37:38Z

Continued in #8156.

Moumouls marked this pull request as draft December 16, 2020 17:51

Moumouls marked this pull request as ready for review December 18, 2020 18:09

Moumouls requested review from mtrezza, dplewis and davimacedo December 18, 2020 18:09

Moumouls mentioned this pull request Jan 5, 2021

Improved Auth Adapter Interface #7052

Closed

9 tasks

This was referenced Jan 10, 2021

Improving security using 2FA parse-community/parse-dashboard#856

Closed

Add MFA to Dashboard parse-community/parse-dashboard#1624

Merged

Moumouls changed the title ~~From Improved Auth Spec: Webauthn Adapter~~ Improved Auth Spec & Webauthn Adapter Jan 18, 2021

This was linked to issues Jan 18, 2021

Modernize Auth Adapter #7042

Closed

Improve AuthAdapter capabilities #7050

Closed

davimacedo reviewed Jan 22, 2021

View reviewed changes

mtrezza mentioned this pull request Mar 8, 2021

Sessiontoken renewal #7248

Closed

mtrezza reviewed Apr 2, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mtrezza requested changes Apr 2, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

mtrezza closed this Apr 2, 2021

Merge branch 'alpha' into moumouls/webauthn-adapter

814ba6c

mtrezza and others added 5 commits July 3, 2022 00:26

Update src/Routers/UsersRouter.js

f6bf6a1

Co-authored-by: dblythy <[email protected]>

Update src/Routers/UsersRouter.js

0cace74

Co-authored-by: dblythy <[email protected]>

Merge branch 'alpha' into moumouls/webauthn-adapter

b8a6fc9

fix: test

57026b2

feat: add validation on policy

7353012

Merge branch 'alpha' into moumouls/webauthn-adapter

7dd7d2f

dblythy suggested changes Jul 4, 2022

View reviewed changes

mtrezza added 2 commits July 20, 2022 13:41

Merge branch 'alpha' into moumouls/webauthn-adapter

1d8a4d8

Merge branch 'alpha' into moumouls/webauthn-adapter

de2e08a

This was referenced Aug 11, 2022

Override default functionality for Parse Server express routes #8126

Open

Pass Bearer token as query request option parse-community/Parse-SDK-JS#1528

Open

Merge branch 'alpha' into moumouls/webauthn-adapter

713aa5d

mtrezza removed the request for review from davimacedo August 31, 2022 00:37

Merge branch 'alpha' into moumouls/webauthn-adapter

bc0e38c

dblythy mentioned this pull request Sep 8, 2022

feat: Improve authentication adapter interface #8156

Merged

4 tasks

mtrezza closed this Sep 8, 2022

mtrezza removed a link to an issue Sep 20, 2022

Modernize Auth Adapter #7042

Closed

mtrezza mentioned this pull request Dec 25, 2023

Remove deprecation DEPPS8: Login with expired 3rd party authentication token defaults to false #8844

Closed

3 tasks


		if (!hasAuthDataId) return;

		const r = await Auth.findUsersWithAuthData(this.config, this.data.authData);

		if (typeof challengeData !== 'object')
		throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'challengeData should be an object.');

feat: new auth adapter #7079

feat: new auth adapter #7079

Conversation

Moumouls commented Dec 16, 2020 • edited Loading

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

codecov bot commented Dec 16, 2020 • edited Loading

Codecov Report

Moumouls commented Dec 18, 2020 • edited Loading

davimacedo commented Jan 5, 2021

Moumouls commented Jan 6, 2021

Moumouls commented Jan 7, 2021

Moumouls commented Jan 12, 2021

Moumouls commented Jan 18, 2021

davimacedo commented Jan 19, 2021

Moumouls commented Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblythy commented Feb 1, 2021 • edited Loading

Moumouls commented Mar 2, 2021 • edited Loading

mtrezza commented Mar 2, 2021 • edited Loading

Moumouls commented Apr 2, 2021

mtrezza commented Apr 2, 2021 • edited Loading

mtrezza commented Jul 2, 2022

Moumouls commented Jul 3, 2022

mtrezza commented Jul 3, 2022

dblythy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrezza commented Jul 6, 2022

mtrezza commented Jul 19, 2022

rhuanbarreto commented Jul 19, 2022

mtrezza commented Jul 19, 2022 • edited Loading

dblythy commented Jul 19, 2022 • edited Loading

Moumouls commented Jul 19, 2022

mtrezza commented Jul 20, 2022 • edited Loading

mtrezza commented Sep 8, 2022

Moumouls commented Dec 16, 2020 •

edited

Loading

codecov bot commented Dec 16, 2020 •

edited

Loading

Moumouls commented Dec 18, 2020 •

edited

Loading

Moumouls commented Jan 20, 2021 •

edited

Loading

dblythy commented Feb 1, 2021 •

edited

Loading

Moumouls commented Mar 2, 2021 •

edited

Loading

mtrezza commented Mar 2, 2021 •

edited

Loading

mtrezza commented Apr 2, 2021 •

edited

Loading

dblythy left a comment •

edited

Loading

mtrezza commented Jul 19, 2022 •

edited

Loading

dblythy commented Jul 19, 2022 •

edited

Loading

mtrezza commented Jul 20, 2022 •

edited

Loading