feat: Add TOTP authentication adapter

[dblythy]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: #8441

Approach

Creates a OTP auth adapter, for both SMS based OTP and time based OTP (TOTP)

Tasks

• Add tests
• Increase coverage
• Make sure recovery data cannot be accessed without masterKey

[parse-github-assistant]

I will reformat the title to use the proper commit message syntax.

[parse-github-assistant]

Thanks for opening this pull request!

[codecov]

Codecov Report

Patch coverage: 83.72% and project coverage change: -0.05 ⚠️

Comparison is base (3ec3e40) 94.45% compared to head (3a80d49) 94.40%.

❗ Current head 3a80d49 differs from pull request most recent head c2af191. Consider uploading reports for the commit c2af191 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #8457      +/-   ##
==========================================
- Coverage   94.45%   94.40%   -0.05%     
==========================================
  Files         184      185       +1     
  Lines       14635    14756     +121     
==========================================
+ Hits        13823    13930     +107     
- Misses        812      826      +14     

Impacted Files	Coverage Δ
src/RestWrite.js	95.05% <ø> (ø)
src/index.js	100.00% <ø> (ø)
src/Adapters/Auth/mfa.js	81.57% <81.57%> (ø)
src/Adapters/Auth/AuthAdapter.js	36.36% <100.00%> (+6.36%)	⬆️
src/Adapters/Auth/index.js	98.14% <100.00%> (+0.97%)	⬆️
src/Auth.js	99.62% <100.00%> (+0.77%)	⬆️
src/Routers/UsersRouter.js	97.13% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mtrezza]

Looks good! I think the new auth adapter provides so much versatility, it merits its own section in the docs to make it easier for developers to use; opened parse-community/docs#929.

Definitions check fails, is this check flaky?

[Moumouls]

Hi @mtrezza @dblythy i'm really asking my self maybe we need to start a mono repo for adapters, to avoid to add too many deps to parse-server.

Then we will be able to move out all adapters and may be GraphQL. I think we need to avoid now to add new packages to parse-server, to avoid a big monolith ( slower to start, to test, more security updates and more)

! I think the new auth adapter provides so much versatility

I saw on the forum some users were interested in this new feature and especially in a MFA system (QR code + TOTP), since it is now an industry standard.

[mtrezza]

That topic is discussed in #7293. The point you mention have been discussed extensively in the past.

[Moumouls]

I know @mtrezza i'm just thinking here if until the mono repo is ready, new package installation on parse-server should be prevented ?

I posted a comment with some feedback on my company monorepo usage: #7293 (comment)

[mtrezza]

i'm just thinking here if until the mono repo is ready, new package installation on parse-server should be prevented ?

Since we don't have a timeline for migrating to mono and it's a more complex undertaking, we can't categorically prohibit the addition of new dependencies as it may block further development. But we should certainly - as we always try to - be critical of any dependency addition when reviewing PRs.

[mtrezza]

@dblythy could you resolve the conflicts so we can merge?

[cbaker6]

@dblythy Sidenote, based on the rest of the construction of the TOTP adapter (ignoring the type issues I mentioned), everything else looks good to me. My guess is it will take me roughly 1 hour (maybe less) to add TOTP support with tests in the Swift SDK.

[cbaker6]

Question:

For auth.mfa.expiry = expiry; the expiry is 1) never sent to the client? and 2) never received from the client? If either are true, it would seem expiry is of Date type which could cause an issue, other 3rd party auths that require date/time values have it as a String type, for example, the Parse Facebook auth: "expiration_date": "token expiration date of the format: yyyy-MM-dd'T'HH:mm:ss.SSS'Z'"

[cbaker6]

For context, the original REST API documentation doesn't strictly state the required type for authData, but only shows the type of [String: [String: String]. The REST API documentation in combination with the Server 3rd Party OAuth documentation indirectly support that authData is designed to be [String: [String: String].

In my experience, there are only a few places on the Parse Server that uses objects with mixed value types. In these particular cases, it makes it harder to design on the client side. My recommendation would be to retain type as much as possible, or else the server will eventually run into issues as it moves towards TypeScript and OpenAPI Support (#7744).

[mtrezza]

We need to distinguish between a client SDK not supporting a specific auth adapter (and its response), and an actual breaking change that occurs if someone upgrades to a version of Parse Server that includes this PR. Also, if we have auth object types explicitly mentioned that are violated by this PR, it would probably also be a breaking change, but I haven't seen that in the docs.

We strive to be consistent with types in preparation for TS support. If the adapter response can be modified in a follow-up PR to support more client SDKs without requiring a change on the SDK side - even better.

However, a type inconsistency per se that doesn't break a client if the specific auth adapter is not used is not necessarily a breaking change and should not prevent a release. We have an established policy for how to deal with breaking changes should this response need to change in the future. It may even make sense to make the client SDKs more tolerant to handle different response types per auth adapter.

Is that sense, is this an actual breaking change?

[cbaker6]

In combination with #8457 (comment), I recommend searching the repo for validateAuthData to see how all of the 3rd party auths validate authData. There's also https://github.com/parse-community/parse-server/blob/alpha/spec/AuthenticationAdapters.spec.js. On the client side, the Swift SDK implements the most 3rd party adapters (9 currently) that I know of (if there's another, feel free to post), all of the implemented adapters use [String: [String: String]], parse-community/Parse-Swift#55

While we strive to be consistent with types in preparation for TS support, a type inconsistency per se that doesn't break a client if the specific auth adapter is not used is not necessarily a breaking change.

I posted that this is not the case here #8457 (comment), basically it will break a client even if the client doesn't have TOTP implemented based on:

I believe the new adapter would be breaking in the following scenario:

• A user signs up using the new TOTP adapter using the REST API (or any other client)
• The same user attempts to login using a strongly typed client (can be Swift or another), they cannot login as errors will be thrown because authData cannot be decoded because it contains types (i.e enabled: true not inline with what previous Parse Servers send in authData

[mtrezza]

The same user attempts to login using a strongly typed client (can be Swift or another), they cannot login as errors will be thrown because authData cannot be decoded because it contains types (i.e enabled: true not inline with what previous Parse Servers send in authData

Which client SDKs are currently affected? It may be a bug on the SDKs side, if they expect a type that is not explicitly documented anywhere.

[cbaker6]

Which client SDKs are currently affected? It may be a bug on the SDKs side, if they expect a type that is not explicitly documented anywhere.

In addition to my fist paragraph in #8457 (comment) which includes documentation by original developers of Parse, look at one of the original SDKs, particularly all of the methods with authData in https://github.com/parse-community/Parse-SDK-iOS-OSX/blob/2fb02591f452e153f1dfa6ce2f665bde29273cd5/Parse/Parse/Source/PFUser.h#L318

[dblythy]

@cbaker6 all of the current in built auth adapters return the string type, but if a developer builds a custom auth adapter that returns / requires other types, would it be correct to assume it would break client SDKs?

[cbaker6]

if a developer builds a custom auth adapter that returns / requires other types, would it be correct to assume it would break client SDKs?

Yes. if a custom adapter returns something other than a String it would break all clients that expect a String type. They essentially won't be able to log in, and if they were already logged in, they couldn't fetch the current user.

[mtrezza]

The server (with its adapters) is the authoritative entity and the SDKs need to adapt to it, not the other way around.

If the adapter can be modified to return the same types as the other adapters, then that's great and the preferred option. If not, then some SDKs simply don't support the adapter, meaning, it can be a caveat of the adapter that just needs to be documented.

@dblythy Is it reasonably possible to modify the adapter types?

[dblythy]

To me it sounds entirely plausible that a custom auth adapter would return other types, even though none of the in built adapters do, it could trip up developers implementing their own authentication adapters. Based on @cbaker6’s comments, it doesn’t seem like anyone using a version of Parse Swift has had this issue though.

It’s easy enough to adapt this PR with using status: “enabled” instead of the current enabled: true. Perhaps we should add a section to the docs to recommend considering return types when creating custom Auth.

[mtrezza]

Sounds good to me. If possible we could add a test for adapters to return the restrictive, historic types. With a future release of Parse Server we should probably break this restriction up and make a PR in the client SDKs to support more auth adapters return types.

[cbaker6]

It’s easy enough to adapt this PR with using status: “enabled” instead of the current enabled: true.

This will work and be inline with the rest of the adapters. @dblythy did you see my question here? #8457 (comment)

To me it sounds entirely plausible that a custom auth adapter would return other types, even though none of the in built adapters do, it could trip up developers implementing their own authentication adapters.

Of course, you all can do whatever you want here as I'm not on the server team. Accepting an infinite type in something likeauthData is going to cause issues down the road when trying to implement Typescript an OpenAPI... I'm confident, that if Parse was like other large repos, or those maintained by groups that enforce a proposal/review submission process, this type of change would not make it through the process. A possible better way would be (if you really wanted to accept any type) is to make authData a concrete type on it's own, but that's not without limitations either... In the authData current implementation, it's better to leave it as String IMO.

[dblythy]

You might have misunderstood, I'm not referring to Parse Server built supported, internal auth adapters, I'm referring to a developer implementing their own auth adapter, and returning custom types there. Should the server validate these types here? I’m thinking we should perhaps code a stronger auth policy to either prevent or allow custom developer defined auth adapters to require specific types

[cbaker6]

Should the server validate these types here?

I say yes, but it depends on how the server team proceeds. If a dev implements a custom auth and it puts a type that is not a String inside of authData, it will have the same issues I mentioned in #8457 (comment)

[mtrezza]

Parse Server is about openness and versatility. If a developer has a use case in which they use only the REST API and custom auth adapter with custom types, I don't think the server should be prohibiting that.

There is also no rule that every adapter that comes included with Parse Server is compatible with all existing client SDKs, just whatever caveats it has should be documented.

I also don't think that supporting multiple types and having a variable response structure (per adapter) is out of the ordinary for an API or even "bad practice". It just needs to be properly implemented on the SDK side.

[cbaker6]

@dblythy you may have answered #8457 (comment) somewhere in the thread, if so, my apologies for missing it, can you point me to the answer?

Also, can you verify the return type of authDataResponse since it's a new property? From what I can tell, it looks like it might be, [String: [String]]? Whatever type it is, will it always remain that type?

[dblythy]

As far as I can tell, expiry doesn't ever make it to the client side, due to the afterFind condition.

authDataResponse is mixed in this PR, but i'll adapt it to make it consistent with authData

[parseplatformorg]

🎉 This change has been released in version 6.4.0-beta.1

[parseplatformorg]

🎉 This change has been released in version 6.4.0-alpha.1

[parseplatformorg]

🎉 This change has been released in version 6.4.0