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.

Sign up for GitHub

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

Jump to bottom

feat: Passwordless SMS code authentication #2033

Closed
wants to merge 1 commit into from
Closed

feat: Passwordless SMS code authentication #2033

wants to merge 1 commit into from

Conversation

splaunov
Copy link
Contributor

@splaunov splaunov commented Dec 7, 2021

Implements new "SMS" login strategy. This strategy requires two login flow submissions:

  • the first one is to send a phone number to the server
  • and the second - to send the received SMS code for validation

Related issue(s)

#1570

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

@splaunov splaunov changed the title Passwordless SMS code authentication feat: Passwordless SMS code authentication Dec 7, 2021
@splaunov
Copy link
Contributor Author

splaunov commented Dec 7, 2021

@alexey-reshetnik Hi! I have merged code from your PR #1941 to take advantage of sending authentication SMS codes through courier.

@aeneasr
Copy link
Member

aeneasr commented Dec 8, 2021

Thank you for continuing the work on this! Tag me for a review once you want it :)

@splaunov splaunov marked this pull request as ready for review December 8, 2021 08:58
@splaunov
Copy link
Contributor Author

splaunov commented Dec 8, 2021

Thank you for continuing the work on this! Tag me for a review once you want it :)

Have submitted for review. Will appreciate your feedback!

@oleksiireshetnik
Copy link
Contributor

Thank you very much for picking up the development. I fell out from work for couple of weeks due to family emergency

@aeneasr
Copy link
Member

aeneasr commented Dec 10, 2021

Thank you, to be honest I have a bit of a hard time reviewing the PR because it is so large. Could you maybe explain what the PR solves in particular? Adding documentation could also be very helpful to understand e.g. how to set it up!

aeneasr
aeneasr reviewed Dec 10, 2021
View reviewed changes
persistence/sql/persister_test.go Outdated Show resolved Hide resolved
schema/extension.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Dec 10, 2021

Another important topic is probably wether this "code" strategy can be used for email also? So #1451?

@aeneasr
Copy link
Member

aeneasr commented Dec 22, 2021

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

@aeneasr aeneasr marked this pull request as draft December 22, 2021 15:05
@aeneasr
Copy link
Member

aeneasr commented Dec 29, 2021

One of the PRs by @alexey-reshetnik was now merged :) The other one (courier) is also looking very solid!

@splaunov
Copy link
Contributor Author

Another important topic is probably wether this "code" strategy can be used for email also? So #1451?

Yes, great idea. Have renamed the strategy from sms to code to be able to use it with emails also.

@yingy77
Copy link

yingy77 commented Jan 25, 2022

When logging in with the same phone number multiple times, there are duplicate records in the identities table, which seems to be wrong. I modified the selfservice/strategy/sms/login.go file and it seems to have solved the problem.
fix-sms_login-multi_identities.zip
.

@splaunov splaunov force-pushed the feature/sms-login branch 2 times, most recently from 82ccb3b to 1e01289 Compare February 2, 2022 13:07
@splaunov splaunov mentioned this pull request Feb 28, 2022
Closed
7 tasks
@splaunov splaunov force-pushed the feature/sms-login branch 2 times, most recently from bec33a4 to 41cef7e Compare March 1, 2022 08:00
@splaunov
Copy link
Contributor Author

splaunov commented Mar 1, 2022

@yingy77 thanks for reporting the issue!
Should have been fixed for now.

@splaunov
Copy link
Contributor Author

splaunov commented Mar 1, 2022

Have added verification flow to the code strategy.

@splaunov
Copy link
Contributor Author

splaunov commented Mar 3, 2022

I have a requirement to add fixed-code logins for testing purposes and to ease apple review process for iOS apps.
My suggestion is to add list of fake phone numbers and a fixed code to the code strategy config.
So, testers and reviewers could login with the known phone number and the fixed code.

@aeneasr what do you think about this?
This is highly essential for apps, where phone login is the only auth method.

@aeneasr
Copy link
Member

aeneasr commented Mar 3, 2022

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

@splaunov
Copy link
Contributor Author

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

@aeneasr can you summarise what are the key points of the pattern?
Should I update this PR according to what is done in #2260 or what are your plans regarding SMS auth?

@aeneasr
Copy link
Member

aeneasr commented Apr 28, 2022

Ok, I'm back from vacation and have time to review this :)

@aeneasr aeneasr marked this pull request as ready for review April 28, 2022 12:02
@aeneasr aeneasr self-assigned this Apr 28, 2022
@splaunov splaunov requested a review from aeneasr April 28, 2022 12:04
@sidharthramesh
Copy link

This would be a great feature to have!
Most of our users don’t have emails and rely on mobile phones as their primary means of identification.

@shasha-zhang
Copy link

Any updates for this? I agree this is a great feature. A lot of people like to login with code sent through SMS.

@ankit-shopflo
Copy link

Any update on this feature? We are planning to move from firebase, the only blocker is this SMS feature

@vinckr
Copy link
Member

vinckr commented Jul 26, 2022

Any update on this feature? We are planning to move from firebase, the only blocker is this SMS feature

No update, contributions welcome.

@nicolasburtey
Copy link

Any update on this feature? We are planning to move from firebase, the only blocker is this SMS feature

same for us I guess, waiting for this to be merged to been able to use kratos

@mohdalali
Copy link

I am also waiting for this to be able to use Kratos. Would like to see this soon!

@mangalaman93
Copy link

Hi, what is pending to get this PR merged? I am happy to spend some time to get it to completion. We need this change as soon as possible. We have already decided to use kratos but we can't go to production unless this change is merged. Thank you.

@adgang
Copy link

adgang commented Nov 11, 2022

May I please know why this ubiquitous use case is still not in kratos. Even after we have a PR? Is there anything we can do to help this feature along?

@CNLHC CNLHC mentioned this pull request Dec 4, 2022
Closed
6 tasks
@splaunov splaunov requested a review from hperl as a code owner August 28, 2023 11:19
@nair-sreerag
Copy link

@aeneasr Can u pls look into this and merge it with the main branch?? The absence of this (much required) feature is the only thing that is keeping us from migrating to Ory .

@CNLHC
Copy link
Contributor

CNLHC commented Jan 28, 2024 •
edited
Loading

It seems that the Passwordless SMS code flow is quite similar to #3378, especially with the help of the courier layer. Both the SMS and the Email need to send a one-time code to the user, the only difference is the delivery method.

Since #3378 is merged (and works well in our deployment), I am a little curious about why this PR needs to change 5000 LOC to implement the Passwordless SMS code authentication.

@aeneasr it seems that you are the BDFL of this project, could you please make some comments about why this PR needs so much time to be merged? If the reason is lacking of workers, I am willing to complete the remain work.

@splaunov splaunov force-pushed the feature/sms-login branch 2 times, most recently from 70c409a to 8f9ad19 Compare March 27, 2024 10:38
@splaunov
all commits before Jan 2024
956863e 
fix: android app expects 6 digit verification code (PS-186)

(cherry picked from commit c4d8cdc)

feat: reduce verification code length to 4 (PS-183)

(cherry picked from commit af55347)

feat: add url link to SMS phone verification message (PS-153)

(cherry picked from commit 0d5a9ab)

fix: sort ui nodes when `code` is added - add test (PS-144)

(cherry picked from commit e1dd6ae)

fix: sort ui nodes when `code` is added (PS-144)

(cherry picked from commit 9a120fc)

feat: set flow active method for `code` strategy (PS-144)

(cherry picked from commit 71ba520)

feat: set transient_payload to `code` method registration flow (PS-122)

(cherry picked from commit 83de4a5)

ignore: add TemplateData to sms message body template context (CORE-2361)

(cherry picked from commit f0eff32)

ignore: fix flaky test (CORE-2361)

(cherry picked from commit 485e7cc)

feat: add `transient_payload` to `code` login and register flows (CORE-2361)

(cherry picked from commit e103508)

fix(sms-login): error handling for invalid sms code

(cherry picked from commit 781dfe1)

fix(sms-login): verify phones with code even if verification.use = link

(cherry picked from commit 9e0f4b1)

feat: sms-login initial commit

fix: change group for 'identifier' field

feat: add sms spam protection to `code` strategy

fix: delete credential identifier if trait deleted

fix: sms spam protection 'like' clause

fix: Validate and normalize phone numbers

chore: format

feat: normalize phone number if used as identifier

fix: correctly process invalid phone numbers

feat: add standby SMS service
(cherry picked from commit a972194)
@aeneasr
Copy link
Member

aeneasr commented Apr 19, 2024

This feature is now available in Ory Kratos! :) Thank you for your contribution

@aeneasr aeneasr closed this Apr 19, 2024
@aeneasr
Copy link
Member

aeneasr commented Apr 19, 2024

Sorry, I thought this was email code login, which is supported (sms passwordless login is afaik not yet supported).

@aeneasr aeneasr reopened this Apr 19, 2024
@wgorczyca
Copy link

any update about this feature?

@aeneasr
Copy link
Member

aeneasr commented Sep 14, 2024

This should now be possible on master by using an sms channel and code with passwordless!

I‘m closing this PR as it is obsolete

@aeneasr aeneasr closed this Sep 14, 2024
@aeneasr
Copy link
Member

aeneasr commented Sep 16, 2024

Looks like it's not fully here yet, but almost!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zepatrik zepatrik Awaiting requested review from zepatrik zepatrik is a code owner

@aeneasr aeneasr Awaiting requested review from aeneasr aeneasr is a code owner

@hperl hperl Awaiting requested review from hperl hperl is a code owner

Assignees

@aeneasr aeneasr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.