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: add additional startsWith('@') check when validating domain #487

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Oct 19, 2020

Problem

This PR adds an additional check for the allowed domains entered by the user to enforce starting with an '@' symbol. This prevents a form admin from adding a complete email address such as test@example.com into the allowed domain field.

Related to https://github.com/datagovsg/formsg-private/issues/53

Solution

Bug Fixes:

  • fix: add additional startsWith('@') check when validating domain

Before & After Screenshots

BEFORE:
Allows full email domains
Screenshot 2020-10-19 at 5 26 32 PM

AFTER:
Prevents full email addresses
Screenshot 2020-10-19 at 3 51 58 PM

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

write a script to check whether the current database state is consistent with this change. otherwise, some admins may not be able to save their form.

@karrui
Copy link
Contributor Author

karrui commented Oct 19, 2020

write a script to check whether the current database state is consistent with this change. otherwise, some admins may not be able to save their form.

I've done a check, as of this instance the only malformed emails are testing domains by me and @syan-syan

Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

suggesting changing the error copy to "You may have duplicate or invalid email domains. Please check that all the domains start with @."

Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

Ok

@tshuli tshuli merged commit fca5dc3 into develop Oct 20, 2020
@tshuli tshuli deleted the fix/strengthen-restrict-domain branch October 20, 2020 02:32
@tshuli tshuli mentioned this pull request Oct 20, 2020
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.

None yet

4 participants