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

OTP recovery and verification strategy #2184

Closed

Conversation

oleksiireshetnik
Copy link
Contributor

@oleksiireshetnik oleksiireshetnik commented Feb 2, 2022

Changes:

  • introduced a new otp strategy (similar to link) with recovery and verification methods implementation
  • added sender, which uses SMS sending functionality from courier to send OTP
  • moved logic for persisting recovery/verification tokens to separate package (for reusing between otp and link strategies)

Related issue(s)

Needs #1941 to be merged
Implements #1451

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@oleksiireshetnik
Copy link
Contributor Author

@aeneasr will add a small guide about how to configure Kratos with Twilio and use phone numbers as identifiers, plus recovery and verification process

@oleksiireshetnik
Copy link
Contributor Author

@splaunov just for your information - this PR implements recovery and verification by using OTP codes. Maybe you will find it useful

@oleksiireshetnik oleksiireshetnik marked this pull request as draft February 2, 2022 08:45
@oleksiireshetnik oleksiireshetnik force-pushed the otp-recovery-verification branch from 1c7fba1 to 6c7aca0 Compare February 23, 2022 14:44
@aeneasr
Copy link
Member

aeneasr commented Feb 26, 2022

@alexey-reshetnik could you give a brief flow chart of how you except the flow to work? You can use mermaid to create such a chart :) I think we have a couple of states we need to take care of:

  • New flow
  • OTP sent
  • OTP entered but wrong, expired, ...
  • OTP valid

and probably a few other error flows :)

Also, it would be awesome if the OTP strategy could work with links as well, so for example:

/verification?code=123456&flow=...

which would pre-fill the form with the OTP code. This could be useful if we want to use this strategy to send magic links and would basically render the link strategy completely obsolete.

Also, we need to talk about entropy. Either, we make the OTP entropy high enough (e.g. [a-zA-Z0-9]{8}) or we introduce something which marks the flow as failed after the OTP has been entered incorrectly for e.g. 3 times.

WDYT?

@splaunov
Copy link
Contributor

Hi!
I've tried to review the otp strategy draft and did some modifications to make all existing tests pass:
monta-app@613a599

But after the review it seems to me that there is a good reason to merge it with the 'code' registration/login strategy.
All the code processing logic should be the same in #2033 and in #2184

What do you think?

@aeneasr
Copy link
Member

aeneasr commented Mar 3, 2022

fyi I am currently working on passwordless auth using webauthn. I suggest that once it is merged we copy that pattern for SMS auth: #2260

@oleksiireshetnik oleksiireshetnik force-pushed the otp-recovery-verification branch from 6c7aca0 to 0018efe Compare June 1, 2022 21:04
@oleksiireshetnik oleksiireshetnik force-pushed the otp-recovery-verification branch from 0018efe to d938b8c Compare June 16, 2022 00:00
@oleksiireshetnik oleksiireshetnik force-pushed the otp-recovery-verification branch from 2e1a07c to d5fe68e Compare June 16, 2022 22:55
@oleksiireshetnik oleksiireshetnik force-pushed the otp-recovery-verification branch from f6838e2 to 2cb7536 Compare July 11, 2022 14:27
@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #2184 (8df90be) into master (1515b83) will decrease coverage by 2.15%.
The diff coverage is 19.81%.

❗ Current head 8df90be differs from pull request most recent head 5f6d1cb. Consider uploading reports for the commit 5f6d1cb to get more accurate results

@@            Coverage Diff             @@
##           master    #2184      +/-   ##
==========================================
- Coverage   75.26%   73.10%   -2.16%     
==========================================
  Files         294      299       +5     
  Lines       17159    16935     -224     
==========================================
- Hits        12914    12380     -534     
- Misses       3266     3613     +347     
+ Partials      979      942      -37     
Impacted Files Coverage Δ
driver/registry.go 73.33% <ø> (+12.22%) ⬆️
driver/registry_default_verify.go 80.95% <0.00%> (-9.75%) ⬇️
identity/credentials.go 69.23% <ø> (-5.77%) ⬇️
persistence/sql/persister_identity.go 67.15% <0.00%> (-2.93%) ⬇️
selfservice/flow/recovery/state.go 90.90% <ø> (ø)
selfservice/flow/recovery/strategy.go 70.00% <ø> (ø)
selfservice/flow/verification/state.go 90.90% <ø> (ø)
selfservice/flow/verification/strategy.go 40.00% <ø> (ø)
selfservice/strategy/link/strategy.go 100.00% <ø> (ø)
selfservice/strategy/otp/sender.go 0.00% <0.00%> (ø)
... and 125 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@oleksiireshetnik oleksiireshetnik force-pushed the otp-recovery-verification branch from 8df90be to 64485cd Compare August 7, 2022 16:47
@oleksiireshetnik oleksiireshetnik force-pushed the otp-recovery-verification branch from 64485cd to 45ddf6f Compare September 7, 2022 23:28
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.

3 participants