Multi-factor authentication by ozsay · Pull Request #796 · accounts-js/accounts

ozsay · 2019-09-04T13:04:28Z

an alternative way for #777 of doing MFA following the auth0 way.

example login flow of user:

/loginWithService('password', { username, password })
1. if the user has MFA enabled will return an MFA token & challenges (array of challenges: sms|email|...) to complete
2. otherwise will return the login response as before.
/performMfaChallenge('sms', mfaToken, { codeFromSms })
1. upon success returns the mfaLoginToken.
/loginWithService('mfa', { mfaLoginToken })
1. validates the mfaLoginToken and returns login response.

db:

add database interface
implement database interface for mongo
implement database interface for manager
implement database interface for typeorm

server:

implement login process
add unit tests

rest api:

client
server

graphql api:

client
server

client:

add api of transport

pradel · 2019-09-04T15:42:50Z

@ozsay since it's pretty hard to have a generic loginWithService and performMfaChallenge mutation (you can pass an email + password or a sms code or whatever for other services) what do you think about adding a JSON scalar for graphql that would allow any inputs?

pradel · 2019-09-05T06:21:49Z

packages/server/src/accounts-server.ts

+  }
+
+  private async createMfaLoginProcess(user: User): Promise<MFALoginResult> {
+    const loginToken = generateRandomToken();


@ozsay I don't exactly understand why do we need this loginToken, can you explain a bit why is it really needed please? :)

the user can only log in if he has the loginToken which is provided to him upon successfully completing the challenge.

Did I over-think this? :)

Since the mfaToken is already a random token I feel like having this loginToken does not add more security no?

following auth0, the last step of the mfa is to call oauth/token (in our case loginWithService) with a token. I can't use the mfaToken here because it's provided to the client before it does the challenge. that's why I have the extra token.

if we want, we can skip this step and have the performMfaChallenge create the session and return the access and refresh tokens.

codecov · 2019-09-11T13:59:07Z

Codecov Report

❗ No coverage uploaded for pull request base (master@0470532). Click here to learn what that means.
The diff coverage is 99.23%.

@@           Coverage Diff            @@
##             master    #796   +/-   ##
========================================
  Coverage          ?   95.5%           
========================================
  Files             ?      83           
  Lines             ?    1937           
  Branches          ?     401           
========================================
  Hits              ?    1850           
  Misses            ?      79           
  Partials          ?       8

Impacted Files	Coverage Δ
...raphql-api/src/modules/accounts/schema/mutation.ts	`100% <ø> (ø)`
...s/graphql-api/src/modules/accounts/schema/types.ts	`66.66% <ø> (ø)`
...ges/database-typeorm/src/entity/MfaLoginAttempt.ts	`100% <100%> (ø)`
packages/rest-express/src/express-middleware.ts	`100% <100%> (ø)`
packages/client/src/accounts-client.ts	`98.9% <100%> (ø)`
...t-express/src/endpoints/oauth/provider-callback.ts	`95.23% <100%> (ø)`
packages/client-password/src/client-password.ts	`100% <100%> (ø)`
...-api/src/modules/accounts/resolvers/loginResult.ts	`100% <100%> (ø)`
packages/database-mongo/src/mongo.ts	`99.36% <100%> (ø)`
...hql-api/src/modules/accounts/resolvers/mutation.ts	`94.44% <100%> (ø)`
... and 8 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 0470532...06c8d67. Read the comment docs.

davidyaha · 2019-09-16T10:11:33Z

examples/graphql-server-typescript/src/index.ts

  const accountsDb = new DatabaseManager({
    sessionStorage: userStorage,
    userStorage,
+    mfaLoginAttemptsStorage: userStorage,


Make sure mfaLoginAttemptsStorage is optional so that it would not break the current usages. Also no need for it when MFA is not configured

pradel

The pr looks good 👍
A few comments I think would be great before merging.

Do we want to have the ability to limit the sms code to 3 attempts for example?
I will try to see if I can bypass the loginToken to see if we really need it or not :)
should the authenticator api a part of this pull request or better to split in another one?
mfaChallenges as a an array of string sounds not really extendable, where will you store the number associated to the user or other?
on the client isn't it better to return the mfaToken to the user instead of setting it in the local storage?

pradel · 2019-09-22T13:57:58Z

packages/graphql-api/src/modules/accounts/resolvers/loginResult.ts

 export const LoginResult = {};
+
+export const LoginWithServiceResult = {
+  __resolveType(obj: any) {


Can we type this one with the generated types?

not sure what you mean

Instead of using any import the types generated by graphql-codegen :)

i hope I understood you correctly :)
52ac287

pradel · 2019-09-22T13:59:45Z

packages/client-password/src/client-password.ts

   * Log the user in with a password.
   */
-  public async login(user: any): Promise<LoginResult> {
+  public async login(user: any): Promise<LoginResult | Omit<MFALoginResult, 'mfaToken'>> {


Why don't we return the mfaToken there?

mainly, to simplify the usage

pradel · 2019-09-22T14:10:57Z

packages/database-manager/src/database-manager.ts

 export class DatabaseManager implements DatabaseInterface {
  private userStorage: DatabaseInterface;
  private sessionStorage: DatabaseInterface | DatabaseInterfaceSessions;
+  private mfaLoginAttemptsStorage?: DatabaseInterface | DatabaseInterfaceMfaLoginAttempts;


Should this be a required field?

@davidyaha pointed out that people might not want mfa at all, so I made it optional

ozsay · 2019-09-23T12:36:57Z

@pradel

Do we want to have the ability to limit the sms code to 3 attempts for example?

I think it's related to the #776 PR. The mfa process is kinda agnostic of which authenticators are being used.

should the authenticator api a part of this pull request or better to split in another one?

I don't think any changes are required to the authenticators API. I will test an alpha version this week, but I believe that it should work.

mfaChallenges as a an array of string sounds not really extendable, where will you store the number associated to the user or other?

currently, it will allow authenticating with any of the challenges in the array (only 1 challenge completion is sufficient for login). We can extend it in the future for more complex scenarios. I think the basic scenarios are covered here.

on the client isn't it better to return the mfaToken to the user instead of setting it in the local storage?

I can return it but I don't see any client usage of it, that's why I've "hidden" it inside accounts

thanks for the comments @pradel :)

acomito · 2019-10-29T17:12:42Z

Will this allow for 2-step authentication through email, or just SMS?

Also is this in the docs yet 😈?

davidyaha · 2019-11-28T12:43:58Z

@ozsay Are we missing something here? Could this be merged?

pradel · 2019-11-29T08:23:50Z

@davidyaha I proposed another design for the MFA implementation which will allow us to support more authenticators. I posted the architecture design on slack and I started an implementation on this pr #813

work on mfa

c9c4453

pradel reviewed Sep 5, 2019

View reviewed changes

pradel mentioned this pull request Sep 5, 2019

Add a scalar JSON to graphql and use it for the authenticate params type #798

Closed

ozsay mentioned this pull request Sep 6, 2019

[discuss] multi-step-authentication #777

Closed

3 tasks

ozsay added 9 commits September 10, 2019 18:36

Merge branch 'master' of github.com:accounts-js/accounts into mfa

b529529

continue working on mfa

b2c250b

add typeorm implementation

b38f162

fix client-password

5eb219d

fix graphql-client

c2baa53

fix accounts-boost

f9e08bc

fixed e2e tests and graphql client

a23142f

mistake

c44bd59

fix examples

dfd2623

add accounts-server tests

2b3ccc5

davidyaha requested changes Sep 16, 2019

View reviewed changes

ozsay added 4 commits September 16, 2019 14:46

increase coverage

04807f4

fixes of CR

09ee967

remove graphql schema from client code

23d5dcb

Merge branch 'master' of github.com:accounts-js/accounts into mfa

58083a1

ozsay changed the title ~~[WIP] Multi-factor authentication~~ Multi-factor authentication Sep 16, 2019

ozsay requested a review from davidyaha September 16, 2019 12:38

davidyaha approved these changes Sep 16, 2019

View reviewed changes

davidyaha requested a review from pradel September 16, 2019 14:09

Merge branch 'master' into mfa

035e1ae

pradel reviewed Sep 22, 2019

View reviewed changes

ozsay added 2 commits September 23, 2019 16:05

add typing to resolver

52ac287

Merge branch 'mfa' of github.com:ozsay/accounts into mfa

1d0a390

pradel mentioned this pull request Sep 23, 2019

WIP feat(mfa): new authenticator api #813

Closed

2 tasks

ozsay added 2 commits September 24, 2019 15:43

Merge branch 'master' of github.com:accounts-js/accounts into mfa

e64c265

fix lock

06c8d67

darkbasic force-pushed the master branch 9 times, most recently from bdf442b to 3e57ec8 Compare September 13, 2023 17:04

darkbasic force-pushed the master branch from e4b593a to 1637b35 Compare September 25, 2023 17:57

darkbasic force-pushed the master branch from acef284 to c4c6e6d Compare November 20, 2023 16:43

Uh oh!

Comments

Conversation

ozsay commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pradel commented Sep 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozsay Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ozsay commented Sep 23, 2019

Uh oh!

acomito commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidyaha commented Nov 28, 2019

Uh oh!

pradel commented Nov 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ozsay commented Sep 4, 2019 •

edited

Loading

ozsay Sep 9, 2019 •

edited

Loading

codecov bot commented Sep 11, 2019 •

edited

Loading

pradel left a comment •

edited

Loading

acomito commented Oct 29, 2019 •

edited

Loading