feat(user): add strict validations for username #1574

sujeethk · 2016-10-15T23:30:37Z

feat(user): add strict validations for username

Idea proposed by @sparshy #1204
Suggestions, rules and tests from Trustroots @simison
Added validations on user server model
Added relevant tests on user server tests

Fixes #1204

hyperreality · 2016-10-16T00:31:15Z

Good stuff. Client-side validation should be added too using ng-pattern and relevant ng-messages.

hyperreality · 2016-10-16T00:32:58Z

config/env/default.js

+  illegalUsernames: ['meanjs', 'fwd', 'fwd:', 'reply', 'administrator', 'password', 'community',
+                   'unknown', 'anonymous', 'null', 'undefined', 'home', 'signup', 'signin', 'login',
+                   'edit', 'settings', 'demo', 'support', 'networks', 'profile', 'avatar', 'mini',
+                   'photo', 'account', 'api', 'modify', 'feedback', 'security', 'accounts', 'tribe', 'tag'


I recommend get rid of 'tribe' and add 'admin' to the list.

In fact I recommend getting rid of lots of these, such as avatar, demo, unknown etc... it should be up to the user to populate this list and MEAN should only reserve very important ones like admin out the box IMO.

true, I didnt really spend time looking at what was in this array :)

sujeethk · 2016-10-16T01:47:00Z

@hyperreality ^[^.](?=.*[0-9a-z])[0-9a-z.\-_](?!.*(\.)\1).*[^.]{3,34} can you please validate this? I am not really good with regex

hyperreality · 2016-10-16T10:11:52Z

Yeah, that regex is difficult to read and doesn't do the right thing at all. For instance, the {3,34} length rule is actually being applied only to [^.], the positive lookahead (?=.*[0-9a-z]) isn't covering the whole username, [0-9a-z.\-_] only covers a single character, and there are lots of .* which allow any character as many times as possible, which we don't want.

I think we should use this:

^(?=.{3,34}$)(?![_.])(?!.*[_.]{2})[a-zA-Z0-9._]+(?<![_.])$

Adapted from here which has a nice explanation about what it does.

sujeethk · 2016-10-16T22:03:21Z

@hyperreality Lint is unhappy with that regex. It says the group is invalid on that regex.

Added a commit with existing regex, added client side validation with ng-pattern and relevant e2e tests. There is some issue with my protractor running e2e tests on my machine, so i couldnt run the tests. But client and server tests passed. Will wait for travis build to complete so i can check the e2e test results there.

sujeethk · 2016-10-16T22:48:28Z

was able to fix my protractor issue. e2e tests passed.

mleanos · 2016-10-17T00:33:49Z

@sujeethk Did you solve the issue with the linting errors on that regex? Also, what is the problem with the regex that @hyperreality said was incorrect? Was the actual issue with just combining it into one for the client-side check? Currently, it appears that the regex you're using here is the same as what @simison shared.

sujeethk · 2016-10-17T02:01:17Z

@mleanos when I used the Regex that hyperreality provided above I got a lint error saying it was invalid regex. Then I resorted to using the same regex that Simison referred and used the same on the client side. All tests passed. So it's good for now.

mleanos

I left a comment about the client-side regex. I think there's an issue there.

Other than that everything looks good, and seems to be working great. Glad to see the additional tests as well.

mleanos · 2016-10-17T03:26:49Z

modules/users/client/controllers/authentication.client.controller.js

@@ -15,6 +15,7 @@
    vm.signup = signup;
    vm.signin = signin;
    vm.callOauthProvider = callOauthProvider;
+    vm.usernameRegex = /^(?=.*[0-9a-z])[0-9a-z.\-_]{3,34}$/ && /^[^.](?!.*(\.)\1).*[^.]$/;


I notice the client-side doesn't catch "user$lastname" as invalid, but the server-side does. This must be an issue with this client-side regex.

fixed this in the latest commit and added a test case for the same

mleanos · 2016-10-17T04:04:29Z

@simison Care to take a quick glance at this?

@meanjs/contributors

simison · 2016-10-17T06:20:59Z

On holiday this week but i'll see if I can have a look at this tonight,
can't promise tho.

That forbidden words list is very TR specific, you should clean it out for
Meanjs.

On 17 Oct 2016 07:04, "Michael Leanos" notifications@github.com wrote:

@simison https://github.com/simison Care to take a quick glance at this?

@meanjs/contributors https://github.com/orgs/meanjs/teams/contributors

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1574 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFUgPOTK6E0TUVis3mcB3E65sLAecKPks5q0vPOgaJpZM4KX19G
.

sujeethk · 2016-10-17T17:40:38Z

@mleanos @hyperreality When using hyperreality's suggestion above this is the error I get
18:25 error Parsing error: Error parsing regular expression: Invalid regular expression: /^(?=.{3,34}$)(?![_.])(?!.*[_.]{2})[a-zA-Z0-9._]+(?<![_.])$/: Invalid group

Hence I used simison's suggestion. @mleanos you are right the same regex works on server side, but on the client side it allows special characters. I am trying to find why, is ngPattern using the regex differently or is it because i concatenated the two regex using &&. If any regex experts out here, your help is much appreciated!

sujeethk · 2016-10-17T18:46:58Z

@mleanos Fixed the regex on client side using help from a regex IRC :). Used the same regex on the server side eliminating the need for a usernameRegex and dotRegex. Also added a couple of tests to check for minimum length and log$in special character

@simison

Idea proposed by @sparshy meanjs#1204 Suggestions, rules and tests from Trustroots @simison Added validations on user server model Added client side validations Added relevant tests on user server tests Added relevant tests on user e2e tests Fixes meanjs#1204

mleanos · 2016-10-17T21:03:09Z

@sujeethk LGTM. I'll try to get this merged in the next couple of days. Thanks for taking this on.

mleanos · 2016-10-20T00:10:01Z

@sujeethk Would you mind rebasing? I'll merge right after.

sujeethk · 2016-10-20T03:00:11Z

@mleanos Rebased

mleanos · 2016-10-20T03:41:00Z

Thank you @sujeethk!

hyperreality reviewed Oct 16, 2016

View reviewed changes

sujeethk force-pushed the strictusername branch from bad95d9 to 6023cff Compare October 16, 2016 22:00

sujeethk force-pushed the strictusername branch 4 times, most recently from 9fb5b38 to fd0aa03 Compare October 16, 2016 22:47

mleanos suggested changes Oct 17, 2016

View reviewed changes

mleanos added the enhance:feature label Oct 17, 2016

mleanos added this to the 0.5.0 milestone Oct 17, 2016

mleanos self-assigned this Oct 17, 2016

sujeethk force-pushed the strictusername branch from fd0aa03 to 4aede0f Compare October 17, 2016 18:45

sujeethk force-pushed the strictusername branch from 4aede0f to 6a9b5af Compare October 17, 2016 18:55

mleanos approved these changes Oct 17, 2016

View reviewed changes

mleanos added the status:ready label Oct 18, 2016

Rebased upstream/master

5b8ed06

mleanos approved these changes Oct 20, 2016

View reviewed changes

mleanos merged commit fb9d9d9 into meanjs:master Oct 20, 2016

This was referenced Apr 26, 2021

[Snyk] Fix for 5 vulnerabilities SibuStephen/mean#38

Open

[Snyk] Fix for 5 vulnerabilities ikhsanalatsary/mean#32

Open

[Snyk] Fix for 5 vulnerabilities #2071

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(user): add strict validations for username #1574

feat(user): add strict validations for username #1574

sujeethk commented Oct 15, 2016

hyperreality commented Oct 16, 2016 •

edited

Loading

hyperreality Oct 16, 2016

hyperreality Oct 16, 2016

sujeethk Oct 16, 2016

sujeethk commented Oct 16, 2016

hyperreality commented Oct 16, 2016 •

edited

Loading

sujeethk commented Oct 16, 2016

sujeethk commented Oct 16, 2016

mleanos commented Oct 17, 2016

sujeethk commented Oct 17, 2016

mleanos left a comment

mleanos Oct 17, 2016

sujeethk Oct 17, 2016

mleanos commented Oct 17, 2016

simison commented Oct 17, 2016

sujeethk commented Oct 17, 2016

sujeethk commented Oct 17, 2016

mleanos commented Oct 17, 2016

mleanos commented Oct 20, 2016

sujeethk commented Oct 20, 2016

mleanos commented Oct 20, 2016

feat(user): add strict validations for username #1574

feat(user): add strict validations for username #1574

Conversation

sujeethk commented Oct 15, 2016

hyperreality commented Oct 16, 2016 • edited Loading

hyperreality Oct 16, 2016

Choose a reason for hiding this comment

hyperreality Oct 16, 2016

Choose a reason for hiding this comment

sujeethk Oct 16, 2016

Choose a reason for hiding this comment

sujeethk commented Oct 16, 2016

hyperreality commented Oct 16, 2016 • edited Loading

sujeethk commented Oct 16, 2016

sujeethk commented Oct 16, 2016

mleanos commented Oct 17, 2016

sujeethk commented Oct 17, 2016

mleanos left a comment

Choose a reason for hiding this comment

mleanos Oct 17, 2016

Choose a reason for hiding this comment

sujeethk Oct 17, 2016

Choose a reason for hiding this comment

mleanos commented Oct 17, 2016

simison commented Oct 17, 2016

sujeethk commented Oct 17, 2016

sujeethk commented Oct 17, 2016

mleanos commented Oct 17, 2016

mleanos commented Oct 20, 2016

sujeethk commented Oct 20, 2016

mleanos commented Oct 20, 2016

hyperreality commented Oct 16, 2016 •

edited

Loading

hyperreality commented Oct 16, 2016 •

edited

Loading