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

fix: return proper error if sms rate limit is exceeded #1647

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

kangmingtay
Copy link
Member

What kind of change does this PR introduce?

What is the current behavior?

Please link any relevant issues here.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Add any other context or screenshots.

@kangmingtay kangmingtay requested a review from a team as a code owner July 4, 2024 01:42
@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 9787183147

Details

  • 0 of 3 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.008%) to 57.97%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/user.go 0 3 0.0%
Totals Coverage Status
Change from base Build 9784210350: -0.008%
Covered Lines: 8797
Relevant Lines: 15175

💛 - Coveralls

@kangmingtay kangmingtay merged commit 3c8d765 into master Jul 4, 2024
3 checks passed
@kangmingtay kangmingtay deleted the km/fix-update-user-phone-change branch July 4, 2024 07:28
kangmingtay pushed a commit that referenced this pull request Jul 4, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.155.1](v2.155.0...v2.155.1)
(2024-07-04)


### Bug Fixes

* apply mailer autoconfirm config to update user email
([#1646](#1646))
([a518505](a518505))
* check for empty aud string
([#1649](#1649))
([42c1d45](42c1d45))
* return proper error if sms rate limit is exceeded
([#1647](#1647))
([3c8d765](3c8d765))

---
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>
@iosephmagno
Copy link

@kangmingtay it is not clear how to enjoy this fix. At first glance it seems that issue was related to a rate limit in sending sms. Im pretty sure we never hit our twilio limits, so is that a Supabase limit? If so, should we handle this on client with reference to #1629 or Supabase's retry is transparent to us?

@iosephmagno
Copy link

@kangmingtay can you also explain how this commit fixes issue #1629 ?
Coz it is not clear to me. When debugging that issue, we were able to receive other sms from supabase, so I cant see why issue was related to rate limit. Can we know the rationale why this commit fixed issue for you? #1629

@kangmingtay
Copy link
Member Author

@iosephmagno it fixes the issue because i was only able to reproduce #1629 when i exceeded the rate limit, and noticed that the auth service was returning a 5xx error rather than a 429.

uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?
* Fixes supabase#1629 
* Previously, rate limit errors were being returned as an internal
server error, which is incorrect. This PR checks for the underlying
error type returned and ensures that rate limit errors are surfaced
appropriately.

## What is the current behavior?

Please link any relevant issues here.

## What is the new behavior?

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.155.1](supabase/auth@v2.155.0...v2.155.1)
(2024-07-04)


### Bug Fixes

* apply mailer autoconfirm config to update user email
([supabase#1646](supabase#1646))
([a518505](supabase@a518505))
* check for empty aud string
([supabase#1649](supabase#1649))
([42c1d45](supabase@42c1d45))
* return proper error if sms rate limit is exceeded
([supabase#1647](supabase#1647))
([3c8d765](supabase@3c8d765))

---
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>
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?
* Fixes supabase#1629 
* Previously, rate limit errors were being returned as an internal
server error, which is incorrect. This PR checks for the underlying
error type returned and ensures that rate limit errors are surfaced
appropriately.

## What is the current behavior?

Please link any relevant issues here.

## What is the new behavior?

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.155.1](supabase/auth@v2.155.0...v2.155.1)
(2024-07-04)


### Bug Fixes

* apply mailer autoconfirm config to update user email
([supabase#1646](supabase#1646))
([a518505](supabase@a518505))
* check for empty aud string
([supabase#1649](supabase#1649))
([42c1d45](supabase@42c1d45))
* return proper error if sms rate limit is exceeded
([supabase#1647](supabase#1647))
([3c8d765](supabase@3c8d765))

---
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>
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
## What kind of change does this PR introduce?
* Fixes supabase#1629 
* Previously, rate limit errors were being returned as an internal
server error, which is incorrect. This PR checks for the underlying
error type returned and ensures that rate limit errors are surfaced
appropriately.

## What is the current behavior?

Please link any relevant issues here.

## What is the new behavior?

Feel free to include screenshots if it includes visual changes.

## Additional context

Add any other context or screenshots.
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.155.1](supabase/auth@v2.155.0...v2.155.1)
(2024-07-04)


### Bug Fixes

* apply mailer autoconfirm config to update user email
([supabase#1646](supabase#1646))
([a518505](supabase@a518505))
* check for empty aud string
([supabase#1649](supabase#1649))
([42c1d45](supabase@42c1d45))
* return proper error if sms rate limit is exceeded
([supabase#1647](supabase#1647))
([3c8d765](supabase@3c8d765))

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

Auth fails to update user phone number under common conditions
4 participants