-
Notifications
You must be signed in to change notification settings - Fork 212
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
feat(payments): add Email Verification to Passwordless Flow #12683
Conversation
dd4da40
to
55b390c
Compare
This seems to only hook the email check into a client-driven API call that it checks. If a user tweaks their local code to ignore the invalid response here, will our other account creation code run this email domain check as well? If not, they can still bypass this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the front-end side, just 1 small change to the .ftl
file.
I also got some different behaviour from the content-server
vs payments-server
, which I've tried summarizing below.
Content Server
- ray@j.com => Error with message "Mistyped email? j.com does not offer email."
- ray@jj.com => Error with message "Mistyped email?"
Payments Server
- ray@j.com => Error with message "Mistyped email? j.com does not offer email."
- ray@jj.com => No error message
@StaberindeZA not sure why content errors on it, i will look into it but UPDATE: @StaberindeZA so i just checked this on production, content server is behaving as your described on production which is incorrect. |
55b390c
to
0d4a6f3
Compare
Yeah I think it is because the content server warns if it can resolve the domain to an IP address but not find a MX record for the domain. If thats the case, the content server actually lets you proceed, you can click "Continue" again and it will dismiss the warning and proceed, whereas if it can't resolve either a MX record or IP address it blocks it entirely. What I am missing, i guess, from the payments server is the warning but I am not sure with how we have the payments server setup if i could do something similar. I'll take a look but my vote would be to ignore the warning since we do allow users to use a domain that resolves to an IP address even if an MX record cannot be found. |
5361a97
to
1eafe77
Compare
6b0856f
to
d7cc356
Compare
@bbangert I caught an error in domain check so i added basically a bypess if there is any error; in the error check now it sets both checks (mail exchange and ipv4) to true so that we don't block anyone from signing up if domain checker fails for any reason. To answer your question in your other comment: this check does not happen on account creation, this is purely a FE check. I can look into adding to the account creation as well - my original intent was for this to behave exactly as it does for the content server but obvisously the abuse/fraud is happening on subplat so if we are concerned malicious actors will bypass the check then that necessitates adding it on account creation as well. @StaberindeZA i briefly looked over the component where we display the error and the app running on the content server - the app on the content server has all 4 scenarios accounted for: skipped, passes, fails mail exchange but not IP (which results in a warning tool tip but does not block the user from moving forward), or fails both (user is blocked from proceeding). I dont think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+ wc
As you mentioned, this blocks the invalid domains, the same way as the content server. For the "warning" message, could you file a follow up ticket so we can discuss whether or not we should pick it up?
packages/fxa-payments-server/src/components/NewUserEmailForm/index.test.tsx
Outdated
Show resolved
Hide resolved
packages/fxa-payments-server/src/components/NewUserEmailForm/index.tsx
Outdated
Show resolved
Hide resolved
d7cc356
to
129080b
Compare
@bbangert I have added the check to the stub account creation and added tests. |
Because: * We want to prevent subscriptions created for fraudulent purposes that use invalid email domains This commit: * As a temp solution refactors the email domain check used by content server and piggy-backs off the account status check to return information about the validity of the email domain Closes #12406
129080b
to
4ad86a4
Compare
Because:
This commit:
Closes #12406
Checklist
Put an
x
in the boxes that applyOther information (Optional)
This is a temporary solution until we evaluate alternatives in #12405 and determine what a more permanent solution will be. I wanted this to be minimally invasive in how things currently work and as easily to remove/revert as possible later.