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

Patch fix for issues caused by adding type check in #5906 #5987

Closed
wants to merge 10 commits into from

Conversation

aalej
Copy link
Contributor

@aalej aalej commented Jun 15, 2023

Description

Type checking reqBody.email and reqBody.password introduced in #5906 caused some issues with the Auth Emulator UI.

Request made by the emulator UI ofter contain a blank values by default. Creating a user via the Emulator UI using phone number will provide a blank email and password string. This will result in the request received by the Auth Emulator to look like:

reqBody: {
  customAttributes: '',
  displayName: '',
  photoUrl: '',
  email: '',
  password: '',
  phoneNumber: '+1230104124',
  ....
} 

Adding type checking of reqBody.email and reqBody.password causes the emulator to always assert a valid email address value. Proposed solution is to remove type check for both reqBody.email and reqBody.password.

To keep the solution in #5906, add a condition to specifically check if reqBody.email is an empty string and a provider is defined (reqBody.email === "" && provider).

Scenarios Tested

Scenario Prod Emulator PR
email="" password="" Error (auth/invalid-email) Creates a user with blank identifier and provider Error (auth/invalid-email)
email="" password="123456" Error (auth/missing-email) Error (auth/missing-email) Error (auth/missing-email)
email="[email protected]" password="123456" Creates a user with identifier and email provider Creates a user with identifier and email provider Creates a user with identifier and email provider

Emulator UI

  1. Tested creating a user using only a phone number
  2. Tested toggling the verified email toggle in the Auth Emulator UI

@@ -198,13 +198,13 @@ async function signUp(
}
}

if (typeof reqBody.email === "string") {
if (reqBody.email || (reqBody.email === "" && provider)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add a comment here describing the conditions that we're testing for and why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment describing the additional condition

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c552816) 55.02% compared to head (e3552fe) 55.02%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5987   +/-   ##
=======================================
  Coverage   55.02%   55.02%           
=======================================
  Files         339      339           
  Lines       23222    23222           
  Branches     4747     4747           
=======================================
  Hits        12779    12779           
  Misses       9313     9313           
  Partials     1130     1130           
Impacted Files Coverage Δ
src/emulator/auth/operations.ts 83.42% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aalej aalej enabled auto-merge (squash) June 21, 2023 15:00
@aalej
Copy link
Contributor Author

aalej commented Jul 19, 2023

Closing this PR, changes are fixed in #6127

@aalej aalej closed this Jul 19, 2023
auto-merge was automatically disabled July 19, 2023 15:02

Pull request was closed

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.

4 participants