accounts icon indicating copy to clipboard operation
accounts copied to clipboard

Multi-factor authentication

Open ozsay opened this issue 5 years ago • 6 comments

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

example login flow of user:

  1. /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.
  2. /performMfaChallenge('sms', mfaToken, { codeFromSms })
    1. upon success returns the mfaLoginToken.
  3. /loginWithService('mfa', { mfaLoginToken })
    1. validates the mfaLoginToken and returns login response.

db:

  • [x] add database interface
  • [x] implement database interface for mongo
  • [x] implement database interface for manager
  • [x] implement database interface for typeorm

server:

  • [x] implement login process
  • [x] add unit tests

rest api:

  • [x] client
  • [x] server

graphql api:

  • [x] client
  • [x] server

client:

  • [x] add api of transport

ozsay avatar Sep 04 '19 13:09 ozsay

@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 avatar Sep 04 '19 15:09 pradel

Codecov Report

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

Impacted file tree graph

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

codecov[bot] avatar Sep 11 '19 13:09 codecov[bot]

@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 :)

ozsay avatar Sep 23 '19 12:09 ozsay

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

Also is this in the docs yet 😈?

acomito avatar Oct 29 '19 17:10 acomito

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

davidyaha avatar Nov 28 '19 12:11 davidyaha

@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 https://github.com/accounts-js/accounts/pull/813

pradel avatar Nov 29 '19 08:11 pradel