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

Refactoring Input Validation #283

Merged
merged 4 commits into from
Jun 5, 2020
Merged

Conversation

mattxwang
Copy link
Collaborator

@mattxwang mattxwang commented May 25, 2020

As I was working on the new login page PR (#269), I realized that some of our input validation needs both a refactor and a policy lookover.

This PR implements some of the refactoring: renaming and juggling badInputs in CreateUserForm and ProfilePanel to its own validate file in src/lib, and writing individual tests for isValidUsername, isValidPassword, and isValidDisplayname functions. I'll probably also bundle any other input validation into this PR as well.

From a policy perspective, just some things I noticed while writing tests

  • isValidUsername should only accept things that can go in an email address before the @. In particular, this means that @ should not be allowed in usernames, and + can. I think we should either change the policy to match something like gmail's email filter, or only allow alphanumeric characters.
  • isValidPassword is currently too restrictive. I think users should be allowed things like spaces, underscores, question marks and the like - and the length restriction (20 characters) is probably too small, especially if people use password managers. It's unlikely with our crowd, but with passworks in the works we should probably follow our own advice.
  • isValidDisplayName is probably also too restrictive: I don't see why many special characters should be allowed. we can also re-consider length when we redesign the profile panel

Any thoughts? I think we made this policy stuff well over two years ago, and there might be some Firebase quirks I'm missing.

(also, you know what would be cool? If we didn't allow common passwords)

Note: this PR depends on #269.

@mattxwang mattxwang added the refactor Refactoring & cleanup label May 25, 2020
@krashanoff krashanoff self-requested a review May 26, 2020 05:31
Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

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

Yo Matt! Thanks for refactoring this - our components could definitely be easier to follow. Sorry it took a while to get to reviewing. Everything looks good except I think we should remove @ from our list of permitted characters or find a way to escape it in our user creation process, as using @ in an username will cause a failure on user creation. To reproduce:

  • Let your username in the create user form be something like test@test
  • Create user normally
  • Fails to create user despite @ being permitted

Just a quick screenshot:
image

The same issue occurs when logging in with an username that contains @:
image

@mattxwang
Copy link
Collaborator Author

Yeah, I agree that that's a problem but we should address it in another PR - at the current moment, this PR is a pure refactor (i.e. the functionality is the same on a diff).

We should make another PR that changes the policy of our username/password/display name set, and I'll make the change involving @ (and other characters) there!

Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

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

I agree that we should probably address it in another PR. In that case, this looks good to merge. Apologies on getting this review out - finals are starting up soon.

Thank you again Matt!

@krashanoff krashanoff merged commit 3bb503c into master Jun 5, 2020
@krashanoff krashanoff deleted the refactor-input-validation branch June 5, 2020 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring & cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants