Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[discuss] code authentication service #776

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ozsay
Copy link
Contributor

@ozsay ozsay commented Aug 21, 2019

Hi,

I implemented a new authentication service which validates a previously sent code to the client.

Common use-cases:

  • login with code provided by sms
  • login with code provided by email

The service both sends the code ("prepare authentication") using a code-provider and validates it after the client logs-in.

TODO:

  • implement core service
  • implement twilio sms provider
  • add unit tests
  • api docs ?

@davidyaha Thoughts?

@codecov
Copy link

codecov bot commented Aug 21, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #776   +/-   ##
========================================
  Coverage          ?   95.2%           
========================================
  Files             ?      85           
  Lines             ?    1835           
  Branches          ?     356           
========================================
  Hits              ?    1747           
  Misses            ?      80           
  Partials          ?       8
Impacted Files Coverage Δ
...ages/code/src/utils/randomstring-code-generator.ts 100% <100%> (ø)
packages/code/src/errors.ts 100% <100%> (ø)
packages/code/src/index.ts 100% <100%> (ø)
packages/code/src/utils/simple-code-hash.ts 100% <100%> (ø)
packages/code/src/accounts-code.ts 94.87% <94.87%> (ø)

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...937f60a. Read the comment docs.

@@ -0,0 +1,50 @@
import { Twilio } from 'twilio';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it's safe to say that twilio pretty much dominates this field, I wonder if we should allow the user to supply some SMSService rather then depending on twilio.
Another option is to just call this package code-provider-twilio so that it will serve as an example for others that need different SMS providers. Once we get another feature request about some other provider we could refactor for generic SMS providers.
@pradel thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renaming it to twilio will prevent another layer of abstraction that we might not need.. so i'm +1 for doing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notifme is a pretty good abstraction, and their notification catcher is really cool.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow didn't know about notifme.. it looks great! What do you think @ozsay?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be useful, though I can't see all the features that twilio provides (like messagingServiceSid)

@ozsay ozsay mentioned this pull request Aug 22, 2019
3 tasks
@ozsay ozsay mentioned this pull request Sep 23, 2019
11 tasks
@pradel
Copy link
Member

pradel commented Sep 24, 2019

@ozsay From my understanding of the pr, this service can't be used alone since it does not take care of the user account creation right?

@ozsay
Copy link
Contributor Author

ozsay commented Sep 24, 2019

@pradel true. we use it with another service.

@pradel
Copy link
Member

pradel commented Sep 24, 2019

I guess this should be clarified somewhere in the documentation as we want the service to be able to register a user I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants