Add username check to signup form #257

Coteh · 2021-06-13T02:28:42Z

Fixes #14.

When username is available, username field will be green.

When username is not available, username field will be red with error message below.

When long username is submitted, it will look like this. This is acceptable for now.

When other error occurs (e.g. rate limit error), there will be no highlight and no message. It would effectively look like the way it did before. I decided it would be better UX to not show an error in this case.

codecov · 2021-06-13T02:28:48Z

Codecov Report

Merging #257 (d130133) into development (caec0af) will increase coverage by 1.17%.
The diff coverage is 75.00%.

@@               Coverage Diff               @@
##           development     #257      +/-   ##
===============================================
+ Coverage        52.66%   53.83%   +1.17%     
===============================================
  Files               14       15       +1     
  Lines             1088     1133      +45     
===============================================
+ Hits               573      610      +37     
- Misses             515      523       +8

Impacted Files	Coverage Δ
lib/database-ops.js	`53.87% <57.14%> (+0.27%)`	⬆️
lib/routes/route.js	`40.69% <72.09%> (+2.77%)`	⬆️
lib/util/username.js	`100.00% <100.00%> (ø)`
lib/util.js	`94.54% <0.00%> (-1.82%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update caec0af...d130133. Read the comment docs.

lgtm-com · 2021-06-13T02:35:29Z

This pull request introduces 1 alert when merging a11af41 into dcb08a9 - view on LGTM.com

new alerts:

1 for Database query built from user-controlled sources

Coteh · 2021-06-17T00:27:29Z

assets/js/main.js

+    field.classList.add(`input-field-${exists ? "error" : "success"}`);
+    const label = field.nextElementSibling;
+    if (label) {
+        label.innerText = exists ? `${username} is not available` : "";


From UX perspective I think I might actually want to display “username is available” if I’m not adding in checkmark.

Coteh · 2021-06-17T00:42:14Z

test/integ/check_username_integ_test.js

+const server = require("../../lib/server");
+const { MongoMemoryServer } = require('mongodb-memory-server');
+const { MongoClient } = require("mongodb");
+const bcrypt = require("bcrypt");


I think this can be removed.

Coteh · 2021-06-20T03:26:35Z

Also adapted username length check done in #261 to be used in username check endpoint as well. This is how it would look in frontend if user typed too long of a username before signing up:

lgtm-com · 2021-06-20T03:27:45Z

This pull request introduces 3 alerts when merging 1d91699 into caec0af - view on LGTM.com

new alerts:

1 for Database query built from user-controlled sources
1 for Unused variable, import, function or class
1 for Type confusion through parameter tampering

- Add message on frontend when username check returns long username error - Add message when username is available - Extract username check from database-ops into a separate method, to be used in /register route handler and in /check_username. Ideally controllers should be calling the method, but for now I think having the check be called in route handler is better than in DB method. - Remove sanitize username and email calls in addUser, which are now useless. Sanitize method to be dealt with in #95 - Add a couple new test cases to check_username to handle username too long and undefined error with username verification

lgtm-com · 2021-06-20T03:31:55Z

This pull request introduces 2 alerts when merging 60dcf2f into caec0af - view on LGTM.com

new alerts:

1 for Database query built from user-controlled sources
1 for Type confusion through parameter tampering

Coteh

Also leaving a comment to remind myself to check the codecov and LGTM warnings before merging.

assets/js/main.js

lgtm-com · 2021-06-27T22:13:11Z

This pull request introduces 2 alerts when merging 2fba9e8 into caec0af - view on LGTM.com

new alerts:

1 for Database query built from user-controlled sources
1 for Type confusion through parameter tampering

…ndUser - Also add test to check_username to ensure that setting up a MongoDB query with $ character will not work

lgtm-com · 2021-06-28T00:35:36Z

This pull request introduces 1 alert when merging 4b68922 into caec0af - view on LGTM.com

new alerts:

1 for Type confusion through parameter tampering

…ssed in is a string specifically

lgtm-com · 2021-07-03T21:57:57Z

This pull request introduces 1 alert when merging 460abfb into caec0af - view on LGTM.com

new alerts:

1 for Type confusion through parameter tampering

…e first one passed. I believe the LGTM check here https://lgtm.com/projects/g/Coteh/simpleimage/rev/pr-f9f55bc57545265fe0b92210ec390456e24bab06 is a false positive. Previous commit prevents arrays from reaching the verifyUsername call. Nonetheless, I have also added this extra handle after looking into this further. I think it would make sense to only check the first username passed in, if multiple were passed in. User should not be passing in multiple usernames ideally, though.

--inspect-brk, unlike --inspect, breaks on start of execution of the script. Seems like this was preventing execution of the line that starts the server. By changing it to --inspect, things are working as expected now.

lgtm-com · 2021-07-03T23:58:33Z

This pull request introduces 1 alert when merging ce0984b into caec0af - view on LGTM.com

new alerts:

1 for Type confusion through parameter tampering

Coteh

The final LGTM alert appears to be false positive. Non-string username parameters should not make it to the verifyUsername check. The handler will either reject it outright, or in the case of an array, will take the first element only and check that one.

Everything else looks good, will merge it in.

… are passed into it. Also rename the function to isValidUsername and add unit tests.

Coteh

Fixed LGTM alert with another check for username type in the util function itself, and added unit tests as well.

Coteh added 3 commits June 5, 2021 17:17

Add failing tests for check username functionality

c39392b

Add route for check_username and pass tests

c8b42ae

Add frontend for username check

a11af41

pull-request-size bot added the size/L label Jun 13, 2021

Coteh commented Jun 17, 2021

View reviewed changes

Coteh changed the base branch from master to development June 18, 2021 17:05

Coteh added 2 commits June 19, 2021 21:41

Remove unnecessary deps from check_username_integ_test.js

26e79c0

Merge branch 'development' into check-username

1ace82f

Coteh force-pushed the check-username branch from 1d91699 to 60dcf2f Compare June 20, 2021 03:28

Coteh commented Jun 20, 2021

View reviewed changes

assets/js/main.js Outdated Show resolved Hide resolved

Update assets/js/main.js

2fba9e8

Add mongo-sanitize library to sanitize username when passing it to fi…

4b68922

…ndUser - Also add test to check_username to ensure that setting up a MongoDB query with $ character will not work

Add an additional check in check_username route to ensure username pa…

460abfb

…ssed in is a string specifically

Coteh added 2 commits July 3, 2021 19:36

Fix running app with inspector not starting the server.

ce0984b

--inspect-brk, unlike --inspect, breaks on start of execution of the script. Seems like this was preventing execution of the line that starts the server. By changing it to --inspect, things are working as expected now.

Coteh mentioned this pull request Jul 4, 2021

Add register user integration tests #268

Closed

Coteh commented Jul 4, 2021

View reviewed changes

Add an additional check to verifyUsername to ensure that only strings…

d130133

… are passed into it. Also rename the function to isValidUsername and add unit tests.

Coteh commented Jul 4, 2021

View reviewed changes

Coteh merged commit 341df34 into development Jul 4, 2021

Coteh deleted the check-username branch July 4, 2021 01:20

Coteh mentioned this pull request Jul 4, 2021

Add Signup to Upload Images mode (#253) #267

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add username check to signup form #257

Add username check to signup form #257

Coteh commented Jun 13, 2021 •

edited

Loading

codecov bot commented Jun 13, 2021 •

edited

Loading

lgtm-com bot commented Jun 13, 2021

Coteh Jun 17, 2021

Coteh Jun 20, 2021

Coteh Jun 17, 2021

Coteh Jun 20, 2021

Coteh commented Jun 20, 2021

lgtm-com bot commented Jun 20, 2021

lgtm-com bot commented Jun 20, 2021

Coteh left a comment

lgtm-com bot commented Jun 27, 2021

lgtm-com bot commented Jun 28, 2021

lgtm-com bot commented Jul 3, 2021

lgtm-com bot commented Jul 3, 2021

Coteh left a comment

Coteh left a comment

Add username check to signup form #257

Add username check to signup form #257

Conversation

Coteh commented Jun 13, 2021 • edited Loading

codecov bot commented Jun 13, 2021 • edited Loading

Codecov Report

lgtm-com bot commented Jun 13, 2021

Coteh Jun 17, 2021

Choose a reason for hiding this comment

Coteh Jun 20, 2021

Choose a reason for hiding this comment

Coteh Jun 17, 2021

Choose a reason for hiding this comment

Coteh Jun 20, 2021

Choose a reason for hiding this comment

Coteh commented Jun 20, 2021

lgtm-com bot commented Jun 20, 2021

lgtm-com bot commented Jun 20, 2021

Coteh left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 27, 2021

lgtm-com bot commented Jun 28, 2021

lgtm-com bot commented Jul 3, 2021

lgtm-com bot commented Jul 3, 2021

Coteh left a comment

Choose a reason for hiding this comment

Coteh left a comment

Choose a reason for hiding this comment

Coteh commented Jun 13, 2021 •

edited

Loading

codecov bot commented Jun 13, 2021 •

edited

Loading