feat: refactor one-time tokens for performance #1558

hf · 2024-04-28T14:15:40Z

Refactors all One-Time Tokens (OTP) used for sign-in with email, SMS, email confirmation, phone confirmation, change... to achieve:

Performance (as current method does not use an index due to the use of partial indexes which cannot be used in practice)
Future enhancements (such as OTP verification counters, adaptive OTP lengths, etc.)

Summary of the change:

A new one_time_tokens table is added which uses a double-write mechanism with users.
Each new OTP is both written in the corresponding users column and as a new row in one_time_tokens.
Lookup for an OTP hash is performed first in one_time_tokens and if not found, using the traditional users approach.
In a few days, once all OTPs using the users columns have expired, a new change will be deployed which removes the users lookup. This completely solves the performance issue for looking up OTPs.
In a future change, the one_time_tokens table can be used to add a verification counter based on lookups on the relates_to (email or phone number) column, enabling new security features.

internal/models/one_time_token.go

internal/models/user.go

coveralls · 2024-04-28T14:30:37Z

Pull Request Test Coverage Report for Build 8970405577

Details

202 of 339 (59.59%) changed or added relevant lines in 6 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.2%) to 65.474%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
internal/api/phone.go	9	17	52.94%
internal/api/verify.go	9	17	52.94%
internal/models/user.go	16	41	39.02%
internal/api/mail.go	28	62	45.16%
internal/models/one_time_token.go	138	200	69.0%

Files with Coverage Reduction	New Missed Lines	%
internal/models/user.go	2	75.22%
internal/api/mail.go	3	60.82%

Totals
Change from base Build 8906588569:	-0.2%
Covered Lines:	8365
Relevant Lines:	12776

💛 - Coveralls

internal/api/mail.go

kangmingtay

While testing the different OTP flows, I noticed that when secure email change is disabled, a row is created in one_time_tokens for email_change_token_current but no token_hash is present.

We should only be creating a one_time_token for email_change_token_new and not for email_change_token_current

internal/models/one_time_token.go

hf · 2024-05-03T10:19:34Z

While testing the different OTP flows, I noticed that when secure email change is disabled, a row is created in one_time_tokens for email_change_token_current but no token_hash is present.

Right this is because the code maps 1:1 to the column. Because we set the column to be '' it also ends up in one_time_tokens. I'll fix that.

migrations/20240427152123_add_one_time_tokens_table.up.sql

internal/models/one_time_token.go

internal/models/user.go

internal/models/one_time_token.go

kangmingtay · 2024-05-06T10:10:09Z

While testing the different OTP flows, I noticed that when secure email change is disabled, a row is created in one_time_tokens for email_change_token_current but no token_hash is present.

Right this is because the code maps 1:1 to the column. Because we set the column to be '' it also ends up in one_time_tokens. I'll fix that.

yeah i think it's better if we simply don't insert a row into one_time_tokens if the token_hash is empty

internal/api/mail.go

Co-authored-by: Joel Lee <lee.yi.jie.joel@gmail.com>

🤖 I have created a release *beep* *boop* --- ## [2.151.0](v2.150.1...v2.151.0) (2024-05-06) ### Features * refactor one-time tokens for performance ([#1558](#1558)) ([d1cf8d9](d1cf8d9)) ### Bug Fixes * do call send sms hook when SMS autoconfirm is enabled ([#1562](#1562)) ([bfe4d98](bfe4d98)) * format test otps ([#1567](#1567)) ([434a59a](434a59a)) * log final writer error instead of handling ([#1564](#1564)) ([170bd66](170bd66)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…1569) Removes legacy lookups in `auth.users` for when a corresponding entry in `one_time_tokens` is not found. Phase II of the refactor, based on #1558, to be released after it's deployed for a few days. --------- Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

Refactors all One-Time Tokens (OTP) used for sign-in with email, SMS, email confirmation, phone confirmation, change... to achieve: - Performance (as current method does not use an index due to the use of [partial indexes](https://github.com/supabase/auth/blob/master/migrations/20220429102000_add_unique_idx.up.sql#L10-L14) which [cannot be used in practice](https://www.postgresql.org/docs/current/indexes-partial.html)) - Future enhancements (such as OTP verification counters, adaptive OTP lengths, etc.) Summary of the change: - A new `one_time_tokens` table is added which uses a double-write mechanism with `users`. - Each new OTP is both written in the corresponding `users` column and as a new row in `one_time_tokens`. - Lookup for an OTP hash is performed first in `one_time_tokens` and if not found, using the traditional `users` approach. - In a few days, once all OTPs using the `users` columns have expired, a new change will be deployed which removes the `users` lookup. This completely solves the performance issue for looking up OTPs. - In a future change, the `one_time_tokens` table can be used to add a verification counter based on lookups on the `relates_to` (email or phone number) column, enabling new security features. --------- Co-authored-by: Joel Lee <lee.yi.jie.joel@gmail.com>

🤖 I have created a release *beep* *boop* --- ## [2.151.0](supabase/auth@v2.150.1...v2.151.0) (2024-05-06) ### Features * refactor one-time tokens for performance ([supabase#1558](supabase#1558)) ([d1cf8d9](supabase@d1cf8d9)) ### Bug Fixes * do call send sms hook when SMS autoconfirm is enabled ([supabase#1562](supabase#1562)) ([bfe4d98](supabase@bfe4d98)) * format test otps ([supabase#1567](supabase#1567)) ([434a59a](supabase@434a59a)) * log final writer error instead of handling ([supabase#1564](supabase#1564)) ([170bd66](supabase@170bd66)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…upabase#1569) Removes legacy lookups in `auth.users` for when a corresponding entry in `one_time_tokens` is not found. Phase II of the refactor, based on supabase#1558, to be released after it's deployed for a few days. --------- Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

Refactors all One-Time Tokens (OTP) used for sign-in with email, SMS, email confirmation, phone confirmation, change... to achieve: - Performance (as current method does not use an index due to the use of [partial indexes](https://github.com/supabase/auth/blob/master/migrations/20220429102000_add_unique_idx.up.sql#L10-L14) which [cannot be used in practice](https://www.postgresql.org/docs/current/indexes-partial.html)) - Future enhancements (such as OTP verification counters, adaptive OTP lengths, etc.) Summary of the change: - A new `one_time_tokens` table is added which uses a double-write mechanism with `users`. - Each new OTP is both written in the corresponding `users` column and as a new row in `one_time_tokens`. - Lookup for an OTP hash is performed first in `one_time_tokens` and if not found, using the traditional `users` approach. - In a few days, once all OTPs using the `users` columns have expired, a new change will be deployed which removes the `users` lookup. This completely solves the performance issue for looking up OTPs. - In a future change, the `one_time_tokens` table can be used to add a verification counter based on lookups on the `relates_to` (email or phone number) column, enabling new security features. --------- Co-authored-by: Joel Lee <lee.yi.jie.joel@gmail.com>

🤖 I have created a release *beep* *boop* --- ## [2.151.0](supabase/auth@v2.150.1...v2.151.0) (2024-05-06) ### Features * refactor one-time tokens for performance ([supabase#1558](supabase#1558)) ([d1cf8d9](supabase@d1cf8d9)) ### Bug Fixes * do call send sms hook when SMS autoconfirm is enabled ([supabase#1562](supabase#1562)) ([bfe4d98](supabase@bfe4d98)) * format test otps ([supabase#1567](supabase#1567)) ([434a59a](supabase@434a59a)) * log final writer error instead of handling ([supabase#1564](supabase#1564)) ([170bd66](supabase@170bd66)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…upabase#1569) Removes legacy lookups in `auth.users` for when a corresponding entry in `one_time_tokens` is not found. Phase II of the refactor, based on supabase#1558, to be released after it's deployed for a few days. --------- Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

Refactors all One-Time Tokens (OTP) used for sign-in with email, SMS, email confirmation, phone confirmation, change... to achieve: - Performance (as current method does not use an index due to the use of [partial indexes](https://github.com/supabase/auth/blob/master/migrations/20220429102000_add_unique_idx.up.sql#L10-L14) which [cannot be used in practice](https://www.postgresql.org/docs/current/indexes-partial.html)) - Future enhancements (such as OTP verification counters, adaptive OTP lengths, etc.) Summary of the change: - A new `one_time_tokens` table is added which uses a double-write mechanism with `users`. - Each new OTP is both written in the corresponding `users` column and as a new row in `one_time_tokens`. - Lookup for an OTP hash is performed first in `one_time_tokens` and if not found, using the traditional `users` approach. - In a few days, once all OTPs using the `users` columns have expired, a new change will be deployed which removes the `users` lookup. This completely solves the performance issue for looking up OTPs. - In a future change, the `one_time_tokens` table can be used to add a verification counter based on lookups on the `relates_to` (email or phone number) column, enabling new security features. --------- Co-authored-by: Joel Lee <lee.yi.jie.joel@gmail.com>

🤖 I have created a release *beep* *boop* --- ## [2.151.0](supabase/auth@v2.150.1...v2.151.0) (2024-05-06) ### Features * refactor one-time tokens for performance ([supabase#1558](supabase#1558)) ([d1cf8d9](supabase@d1cf8d9)) ### Bug Fixes * do call send sms hook when SMS autoconfirm is enabled ([supabase#1562](supabase#1562)) ([bfe4d98](supabase@bfe4d98)) * format test otps ([supabase#1567](supabase#1567)) ([434a59a](supabase@434a59a)) * log final writer error instead of handling ([supabase#1564](supabase#1564)) ([170bd66](supabase@170bd66)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…upabase#1569) Removes legacy lookups in `auth.users` for when a corresponding entry in `one_time_tokens` is not found. Phase II of the refactor, based on supabase#1558, to be released after it's deployed for a few days. --------- Co-authored-by: Kang Ming <kang.ming1996@gmail.com>

hf requested a review from a team as a code owner April 28, 2024 14:15

hf force-pushed the hf/add-one-time-tokens branch 2 times, most recently from 988959a to 46d4d44 Compare April 28, 2024 14:24