Improved Auth Adapter Interface #7052

Moumouls · 2020-12-08T17:00:46Z

Challenge endpoint
Adjust error codes (replace existing MFA error codes in JS SDK if unused)
Add auth data check on login route
Support some secret keys into autData (never retrieve key like password)
Add a required tag on AuthAdatpers to force user to complete all process
GraphQL endpoint
JS SDK PR to support new authData field on logIn Improved Auth Spec & challenge endpoint Parse-SDK-JS#1276
JS SDK PR to support challenge endpoint
Tests

This PR can then allow to implement MFA, Webauthn through the Auth Adapter Interface.
The new system can also support multi auth pipeline, ex (Super strong auth): login user by username + password + OTP + MFA + Webauthn + other auth system

codecov · 2020-12-08T17:06:48Z

Codecov Report

Merging #7052 (42b88f2) into master (05f5aa0) will increase coverage by 0.11%.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master    #7052      +/-   ##
==========================================
+ Coverage   93.63%   93.74%   +0.11%     
==========================================
  Files         169      169              
  Lines       12500    12620     +120     
==========================================
+ Hits        11704    11831     +127     
+ Misses        796      789       -7

Impacted Files	Coverage Δ
src/GraphQL/helpers/objectsQueries.js	`90.83% <0.00%> (ø)`
src/GraphQL/loaders/parseClassTypes.js	`94.19% <ø> (ø)`
src/GraphQL/loaders/usersMutations.js	`93.40% <94.73%> (-0.02%)`	⬇️
src/Adapters/Auth/index.js	`96.05% <95.65%> (+2.50%)`	⬆️
src/RestWrite.js	`93.83% <98.00%> (+0.31%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`95.94% <100.00%> (+<0.01%)`	⬆️
src/Auth.js	`100.00% <100.00%> (ø)`
src/RestQuery.js	`95.52% <100.00%> (ø)`
src/Routers/UsersRouter.js	`95.40% <100.00%> (+1.03%)`	⬆️
src/Controllers/DatabaseController.js	`95.76% <0.00%> (+0.14%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05f5aa0...42b88f2. Read the comment docs.

Moumouls · 2020-12-10T15:06:02Z

@dblythy i just finished implementation and fixed regressions, if you want to take a look, i hope i can finish tests on friday then we will be able to implement easly webauthn, TOTP (you named mfa) and combined auth (ex OAuth + Webauthn + MFA )

dblythy · 2020-12-10T15:18:17Z

Looks good. I wish you did this before I built mfa it would’ve saved me heaps of time 😂. I am happy to build MFA and passwordless adapters based off this PR, I will be eagerly awaiting.

mtrezza · 2020-12-10T20:51:38Z

I see that Adjust error codes is still an open task, but reviews have already been requested or this PR?

Moumouls · 2020-12-10T21:06:42Z

@mtrezza, this PR is currently draft, i'm on tests to cover new features !
Currently i just use the Other cause error code but i think we need to adjusts some ones with existing error codes.I need some feedbacks of Rest API experts :)

Note here, common SDKs will need an update to support the new combined auth feature (username , password + TOTP). We need to support the new authData field on Parse.User.logIn

To avoid a breaking change i can suggest

Parse.User.logIn({
  username: 'test',
  password: 'test',
  authData: { 
    aProvider: {
      someData: true
    }
  }
}
,options)

Note: i request reviews if some are interested in giving feedback on what I pushed

Moumouls · 2020-12-11T18:56:30Z

Okay i'm close to the end, @mtrezza if you want to take a look, i'm think this pr will be finished on monday 😄

PS: It seems that i got a package lock change due to my node version and the new npm version

Moumouls · 2020-12-14T13:51:09Z

Only missing the last graphql test, here coverage report for the main touched files

# Conflicts: # package.json # src/GraphQL/loaders/parseClassTypes.js

Moumouls · 2020-12-14T15:51:16Z

Okay @mtrezza @dplewis @davimacedo this PR is ready for review !
@dblythy tell me if new auth adapters features seems logic to you and if you see how you could use the new spec for the TOTP 😉

Normally the spec should allow easy integration of 2-step authentication systems such as SMS OTP, WebAuthn and many others.

i would like to apologize in advance to reviewers since the PR is pretty huge, i tried to write as much as i can easy to read code, i hope it will facilitate your review ! 😃

Moumouls · 2020-12-14T15:59:17Z

I'm checking why postgres has started to fail ...

Postgres error is super weird:
I don't know why it start failing after my refresh from upstream

error: column "authData" specified more than once
    at Parser.parseErrorMessage (/Users/antoinecormouls/Desktop/OpenSource/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
    at Parser.handlePacket (/Users/antoinecormouls/Desktop/OpenSource/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
    at Parser.parse (/Users/antoinecormouls/Desktop/OpenSource/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
    at Socket.<anonymous> (/Users/antoinecormouls/Desktop/OpenSource/parse-server/node_modules/pg-protocol/dist/index.js:10:42)

Okay i found an issue on how Postgres adapter save the authData

Moumouls · 2020-12-15T10:33:53Z

So i will begin the webauthn implementation based on this branch

Moumouls · 2020-12-15T11:34:52Z

SDK PR Ready: parse-community/Parse-SDK-JS#1276

Moumouls · 2020-12-16T19:41:49Z

If somebody have some time to give a quick review I'll be happy 😁 Then it will unlock the mfa and webauth adapter 😊 @parse-community/server

davimacedo

Great job, @Moumouls ! I've left two questions to better understand the options. Also, how would that work in the sign up process? I mean... maybe we want mfa to be forced in log in, but not in sign up. We also only want to force mfa in the case that the user has previously set it up. I see that also in most of tests you are saving new users. Maybe it would be good to have more tests for the log in process.

davimacedo · 2020-12-16T20:10:33Z

src/Adapters/Auth/AuthAdapter.js

+     * Set to true if you want to force to validateAuthData
+     * even if authData do not change
+     */
+    this.alwaysValidate = false;


When should the validation happen?

Currently validation only occur if authData is detected as changed (a loadash equality check on the DB authData and provided authData)

The option allow to force the validation even if authData are detected as same

But what always mean? Every time that the user logs in, every time that the session token is used?

every time when authData is touched by the client ! (login/save/signup)

I believe that the token should be always validated at login regardless of this option. Otherwise we can have a security issue since someone would be able to use the current token, even if expired, to log in. What is the use case that you imagined for always validate false?

Otherwise we can have a security issue since someone would be able to use the current token,

Parse Server has always handled authData this way. i think also like you, it's a security issue ! So i will remove the alwaysValidate options, remove the loadash equality check, and the hasMutatedAuthData behavior

@davimacedo @TomWFox may be we should open a security advisory about this (i don't know why but i can't open one)

davimacedo · 2020-12-16T20:11:40Z

src/Adapters/Auth/AuthAdapter.js

+     * additional: could be only used with a default policy auth provider
+     * solo: Will ignore ALL additional providers if additional configured on user
+     */
+    this.policy = 'default';


I didn't get it well. We can't have multilpe adapters anymore, let's say for google, facebook, twitter?

Yes we need to improve this doc part. Policy allow parse server to know how to handle 2FA. default is the policy that always existed in parse server, a user can login/signup directly with just facebook or twitter etc... adapters with default policy always have an ID to identify a user

Then we will have the MFA adapter with additional policy, additional policy tell to parse server that if this kind of auth is configured on the user, it need to be provided as a second factor authentication method like facebook + MFA, twitter + MFA, username + MFA, facebook + SMS OTP; Auth Adapters with additional policy DO NOT HAVE ids , since there will be used as a validation auth system (OTP, MFA, etc...) and are not intended to be used as an identifier auth system.

Finally the policy solo tell to the parse server that the auth adapter (like webauthn) DO NOT NEED additionnal validation if an additional auth adapter (like MFA) was configured on the User.

This way we have:
default: Intended to signup/login/identify the user and can be combined with an additional auth system if the user has one configured
additional: Intended to be used as a 2FA to validate an initial login via a default adapter
solo: Can be used to signup/login/identify the user and CAN'T be combined with an additional auth system

I am quite open if you have an idea to clarify as much as possible these behaviors :)

Note the system that i designed only support 2FA (not 3FA,4FA since it's useless), so if a user has for example: username/password (considered as default), he has SMS OTP (additional) and also MFA (additional), parse server will require the user to provide ONE OF the additional auth system, in this use case username/password/OTP OR username/password/MFA the client currently has the choice about which configured additional auth system he wants to use.

Ok. Now I understand. Maybe this explanation should go in the code or docs :)

Moumouls · 2020-12-16T22:46:15Z

Great job, @Moumouls ! I've left two questions to better understand the options. Also, how would that work in the sign up process? I mean... maybe we want mfa to be forced in log in, but not in sign up. We also only want to force mfa in the case that the user has previously set it up. I see that also in most of tests you are saving new users. Maybe it would be good to have more tests for the log in process.

Tests fully cover logins use cases, and for sure the system ONLY REQUIRE an adapter on login if it was configured before by the user (presence of data into authData object). In other hand the required tag on an adapter force a user to provide/configure an auth adapter directly at create/signup time.

Moumouls

@davimacedo I just added some comments for a better understanding 👍
Maybe by reading the tests carefully, it might be easier to understand my main intention with this PR :)
Sorry again for this massive changes, i know that's not easy to review correctly !

Moumouls · 2020-12-16T22:47:39Z

src/Auth.js

+
+  // In case of signup we need to only check
+  // required providers
+  if (!userAuthData) return;


Here logic of required auth adapters @davimacedo

Moumouls · 2020-12-16T22:48:01Z

src/Auth.js

+
+  // Solo providers can be considered as safe
+  // so we do not have to check if the user need
+  // to provide an additional provider to login
+  if (hasProvidedASoloProvider) return;
+


Solo provider handling (like webauthn)

Moumouls · 2020-12-16T22:48:42Z

src/Auth.js

+  const additionProvidersNotFound = [];
+  const hasProvidedAtLeastOneAdditionalProvider = savedUserProviders.some(provider => {
+    if (config.auth[provider] && config.auth[provider].policy === 'additional') {
+      if (authData[provider]) {
+        return true;
+      } else {
+        // Push missing provider for plausible error return
+        additionProvidersNotFound.push(provider);
+      }
+    }
+  });
+  if (hasProvidedAtLeastOneAdditionalProvider || !additionProvidersNotFound.length) return;
+
+  throw new Parse.Error(
+    Parse.Error.OTHER_CAUSE,
+    `Missing additional authData ${additionProvidersNotFound.join(',')}`
+  );


Additional adpater handling (like mfa), here we are searching if an additional adpater was configured on the user

Moumouls · 2020-12-16T22:51:07Z

src/Auth.js

+const hasMutatedAuthData = (authData, userAuthData, config) => {
+  if (!userAuthData) return { hasMutatedAuthData: true, mutatedAuthData: authData };
+  const mutatedAuthData = {};
+  Object.keys(authData).forEach(provider => {
+    // Anonymous provider is not handled this way
+    if (provider === 'anonymous') return;
+    const providerData = authData[provider];
+    const userProviderAuthData = userAuthData[provider];
+    if (!_.isEqual(providerData, userProviderAuthData) || config.auth[provider].alwaysValidate) {
+      mutatedAuthData[provider] = providerData;
+    }
+  });
+  const hasMutatedAuthData = Object.keys(mutatedAuthData).length !== 0;
+  return { hasMutatedAuthData, mutatedAuthData };
+};


old code used !_.isEqual(providerData, userProviderAuthData), here i just added the alwaysValidate option to force authData validation

Moumouls · 2020-12-16T22:54:05Z

spec/AuthenticationAdapters.spec.js

+
+  it('should pass user to auth adapter on update by matching session', async () => {
+    spyOn(alwaysValidateAdapter, 'validateAuthData').and.resolveTo({});
+    await reconfigureServer({ auth: { alwaysValidateAdapter } });


@davimacedo here use case about updating a user authData already logged in

Moumouls · 2020-12-16T22:55:01Z

spec/AuthenticationAdapters.spec.js

+
+  it('should return authData response and save some info on username login', async () => {
+    spyOn(requiredAdapter, 'validateAuthData').and.resolveTo({
+      response: { someData: true },


Here a login via username and authData use case

Moumouls · 2020-12-16T22:55:38Z

spec/AuthenticationAdapters.spec.js

+  });
+  it('should return authData response and save some info on non username login', async () => {
+    spyOn(requiredAdapter, 'validateAuthData').and.resolveTo({
+      response: { someData: true },


Here loginWith user case

davimacedo · 2020-12-17T05:34:19Z

Great job, @Moumouls ! I've left two questions to better understand the options. Also, how would that work in the sign up process? I mean... maybe we want mfa to be forced in log in, but not in sign up. We also only want to force mfa in the case that the user has previously set it up. I see that also in most of tests you are saving new users. Maybe it would be good to have more tests for the log in process.

Tests fully cover logins use cases, and for sure the system ONLY REQUIRE an adapter on login if it was configured before by the user (presence of data into authData object). In other hand the required tag on an adapter force a user to provide/configure an auth adapter directly at create/signup time.

So, if I add a mfa adapter to my app, all users will be obligated to setup the mfa? I don't have the option to make it optional for my users?

Moumouls · 2020-12-17T07:48:21Z

So, if I add a mfa adapter to my app, all users will be obligated to setup the mfa? I don't have the option to make it optional for my users?

if you add MFA adapter to your app, User will have the choice to configure the auth system on their account. Once a user has configured the MFA on his account (authData will contain something like this authData: {mfa: {secret: 'xxxxx'}}), the user will need to provide MFA each time to confirm his authentication. Then he can remove the mfa by saving authData like this authData: {mfa: null}. Currently the new auth adapter system will ONLY FORCE a user to provide additional data (like MFA) if the auth adapter is detected as configured into user authData object 😃

In other hand an authAdapter with required property will be required every time, and force user to configure it at signup and use it at each authentication.

Moumouls · 2020-12-17T08:21:02Z

After more thinking @davimacedo here, i will add new methods validateLogin and validateSetUp for better understanding and easier Auth Adapter implementation. Currently validateAuthData handle to much things in the improved spec, i will keep the method, then a developer depending of the complexity of the system will have the choice to use validateAuthData or validateLogin/validateSetup

Moumouls · 2020-12-17T09:33:06Z

src/Adapters/Auth/AuthAdapter.js

  }

-  /*
+  /**


@dblythy does the new following validate functions seems more clear to you to implement MFA ?

Not entirely yet, I haven't looked through it completely. I'll probably wait until this is merged and then create a PR for a MFA adapter based on the final interface and the test code that you've written for it. Is that ok?

Yeah no problem !

Thank you legend! 😊

I might have another look 😊

Moumouls · 2020-12-17T10:55:08Z

Okay working on removing mutatedAuthData a should allow PUT request with stale auth Data test start failing seems that was introduced by flovimart in 2017, i don't know why parse server should accept stale authData...

Issue related: #3867 and #3872

Ok, I found out why they validated this, it's for some SDKs and dashboards that back up authentication data every time

I will clarify the code

Moumouls · 2020-12-17T17:45:30Z

Okay @davimacedo , auth spec is now better and easier to understand and implement, i wait you feedbacks ! 😃
@mtrezza if you can also take a look when you will have some time it could better before merging this one 😉

It seems that we have some flaky tests on wiredTigger/4.0.4/replica , I had to re-run the tests twice in order for them to pass.

Moumouls · 2020-12-18T14:02:27Z

Seems to have a strange cod cov issue on src/Options/parsers.js

Moumouls · 2021-01-04T09:49:44Z

@davimacedo @dplewis if you want to give a look to this one ! 🙂

davimacedo

@Moumouls sorry for the delay. It looks good to me. Just one more question. @dplewis @mtrezza thoughts?

davimacedo · 2021-01-05T07:42:35Z

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 () => {


Maybe it should be mentioned in the breaking changes?

[redacted by @mtrezza]

@Moumouls, again I had to redact, just stumbled upon it by chance.

Moumouls · 2021-01-05T08:19:46Z

Also if you want to take a look at: #7079
i'have adjusted some things that i missed here (hard to detect it without a real world implementation)

Moumouls · 2021-01-18T09:29:26Z

Closing this one in favor of #7079
Since i pushed many little fixes to #7079 based on real project usage.

Add Auth Challenge system

9be5711

Moumouls marked this pull request as draft December 8, 2020 17:00

Moumouls linked an issue Dec 8, 2020 that may be closed by this pull request

Improve AuthAdapter capabilities #7050

Closed

2 tasks

Moumouls requested review from mtrezza, dplewis, omairvaiyani and davimacedo December 8, 2020 17:02

Moumouls added 4 commits December 9, 2020 12:20

wip

77ed429

wip

8e0ce66

regression fixed

a59ab48

introduce doNoteSave

3dbb11d

Moumouls added 2 commits December 10, 2020 17:22

test spec

e0f915a

wip: tests

f6c9b35

mtrezza mentioned this pull request Dec 10, 2020

⚡️ Release 2.19.0 parse-community/Parse-SDK-JS#1273

Merged

wip: tests ,missing challenge test and regression fixes

6bec1e2

Moumouls added 2 commits December 14, 2020 11:49

test ok: missing challenge/graphql test

17b3fef

tested challenge: missing graphql mutation test

5674f85

Moumouls added 2 commits December 14, 2020 16:37

graphql tests

c2e9e3c

Merge branch 'upstream/master' into moumouls/auth-adapter-spec

62a88f6

# Conflicts: # package.json # src/GraphQL/loaders/parseClassTypes.js

Moumouls marked this pull request as ready for review December 14, 2020 15:40

rollback to package json v1

0126ed0

pass authData to challenge method

d9ded2a

fix gql test

5b7bf4f

Moumouls mentioned this pull request Dec 15, 2020

Modernize Auth Adapter #7042

Closed

Moumouls changed the title ~~New Auth Adapter Interface~~ Improved Auth Adapter Interface Dec 15, 2020

davimacedo reviewed Dec 16, 2020

View reviewed changes

Moumouls commented Dec 16, 2020

View reviewed changes

wip: new trigger functions setup/login/update

a55e4c8

Moumouls commented Dec 17, 2020

View reviewed changes

Moumouls added 2 commits December 17, 2020 16:53

wip: remove required, clarified handleAuthData

ff6ddd2

updated spec

36e44b1

Moumouls requested a review from davimacedo December 17, 2020 17:43

Add test, fix coverage

96274f4

Moumouls mentioned this pull request Dec 18, 2020

feat: new auth adapter #7079

Closed

7 tasks

Moumouls added 2 commits December 24, 2020 12:01

Merge branch 'upstream/master' into moumouls/auth-adapter-spec

8a1d1e0

changelog

42b88f2

davimacedo reviewed Jan 5, 2021

View reviewed changes

Moumouls closed this Jan 18, 2021

                 }
-                /*
+                /**

Improved Auth Adapter Interface #7052

Improved Auth Adapter Interface #7052

Conversation

Moumouls commented Dec 8, 2020 • edited Loading

codecov bot commented Dec 8, 2020 • edited Loading

Codecov Report

Moumouls commented Dec 10, 2020 • edited Loading

dblythy commented Dec 10, 2020 • edited Loading

mtrezza commented Dec 10, 2020

Moumouls commented Dec 10, 2020 • edited Loading

Moumouls commented Dec 11, 2020 • edited Loading

Moumouls commented Dec 14, 2020

Moumouls commented Dec 14, 2020

Moumouls commented Dec 14, 2020 • edited Loading

Moumouls commented Dec 15, 2020

Moumouls commented Dec 15, 2020

Moumouls commented Dec 16, 2020 • edited Loading

davimacedo left a comment • edited Loading

Choose a reason for hiding this comment

davimacedo Dec 16, 2020 • 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

Moumouls Dec 17, 2020 • edited by mtrezza 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

Moumouls commented Dec 16, 2020

Moumouls left a 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

davimacedo commented Dec 17, 2020

Moumouls commented Dec 17, 2020 • edited Loading

Moumouls commented Dec 17, 2020 • 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

Moumouls commented Dec 17, 2020 • edited Loading

Moumouls commented Dec 17, 2020 • edited Loading

Moumouls commented Dec 18, 2020

Moumouls commented Jan 4, 2021

davimacedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moumouls Jan 5, 2021 • edited by mtrezza Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Moumouls commented Jan 5, 2021

Moumouls commented Jan 18, 2021

Moumouls commented Dec 8, 2020 •

edited

Loading

codecov bot commented Dec 8, 2020 •

edited

Loading

Moumouls commented Dec 10, 2020 •

edited

Loading

dblythy commented Dec 10, 2020 •

edited

Loading

Moumouls commented Dec 10, 2020 •

edited

Loading

Moumouls commented Dec 11, 2020 •

edited

Loading

Moumouls commented Dec 14, 2020 •

edited

Loading

Moumouls commented Dec 16, 2020 •

edited

Loading

davimacedo left a comment •

edited

Loading

davimacedo Dec 16, 2020 •

edited

Loading

Moumouls Dec 17, 2020 •

edited by mtrezza

Loading

Moumouls commented Dec 17, 2020 •

edited

Loading

Moumouls commented Dec 17, 2020 •

edited

Loading

Moumouls commented Dec 17, 2020 •

edited

Loading

Moumouls commented Dec 17, 2020 •

edited

Loading

Moumouls Jan 5, 2021 •

edited by mtrezza

Loading