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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up project with 3 routes #1

Merged
merged 7 commits into from Oct 3, 2019

Conversation

@vitokhangnguyen
Copy link
Contributor

vitokhangnguyen commented Oct 1, 2019

Hi,
I have the functionality of login, register and check existence done. You can take a look and tell me if you want to change anything 馃槃
I will also make the client-side tmessage interact with this API.

@Haider8

This comment has been minimized.

Copy link
Owner

Haider8 commented Oct 3, 2019

@vitokhangnguyen Great work!

I am getting this error for the route localhost:8080/api/user/register :

SyntaxError: Unexpected token ' in JSON at position 3
    at JSON.parse (<anonymous>)
    at parse (/home/haider/github-repos/tmessage-api/node_modules/body-parser/lib/types/json.js:89:19)
    at /home/haider/github-repos/tmessage-api/node_modules/body-parser/lib/read.js:121:18
    at invokeCallback (/home/haider/github-repos/tmessage-api/node_modules/raw-body/index.js:224:16)
    at done (/home/haider/github-repos/tmessage-api/node_modules/raw-body/index.js:213:7)
    at IncomingMessage.onEnd (/home/haider/github-repos/tmessage-api/node_modules/raw-body/index.js:273:7)
    at IncomingMessage.emit (events.js:188:13)
    at endReadableNT (_stream_readable.js:1129:12)
    at process.internalTickCallback (internal/process/next_tick.js:72:19)
@vitokhangnguyen

This comment has been minimized.

Copy link
Contributor Author

vitokhangnguyen commented Oct 3, 2019

Hi @Haider8,
I am not having that error on my side, can you check 2 things?

  • You did npm i to install all the packages
  • Your JSON sent in the body of the request is well-formatted, for eg:
{
	"userName": "vitokhangnguyen",
	"displayedName": "Khang Nguyen",
	"password": "password",
	"passwordConfirm": "password"
}
@Haider8

This comment has been minimized.

Copy link
Owner

Haider8 commented Oct 3, 2019

Now I am getting this response

{
    "success": false,
    "message": "not authorized on admin to execute command { insert: \"userAccount\", documents: [[{_id ObjectIdHex(\"5d95807a651f3e1e88e91997\")} {userName vitokhangnguyen} {displayedName Khang Nguyen} {password $2a$10$JBbQOnJq.7KfYe/QbbsiBeDz42nO2ZQ3ymNaXkFqIjGF4AhHBaOxO} {__v 0}]], ordered: true, writeConcern: { w: \"majority\" }, lsid: { id: {4 [246 245 25 101 102 131 64 67 168 212 198 114 160 25 213 177]} }, txnNumber: 1.000000, $clusterTime: { clusterTime: 6743437274236584936, signature: { hash: [207 182 28 93 27 160 154 69 160 16 144 62 135 242 161 92 116 188 209 43], keyId: 6740875669841903616.000000 } }, $db: \"admin\" }"
}
@vitokhangnguyen

This comment has been minimized.

Copy link
Contributor Author

vitokhangnguyen commented Oct 3, 2019

I suspect that error rises from the setting in the MongoDB. If you are using MongoDB Atlas, check the Database Access panel:
image
Make sure you have an admin user there and substitute the credential into the connection string.

mongodb+srv://[ADMIN_USERNAME]:[ADMIN_PASSOWRD]@bti425-cluster-jyzoe.azure.mongodb.net/[DATABASE_NAME]?retryWrites=true&w=majority
@Haider8

This comment has been minimized.

Copy link
Owner

Haider8 commented Oct 3, 2019

Yes, I checked that. Can you please share the format of your connection string here?

@vitokhangnguyen

This comment has been minimized.

Copy link
Contributor Author

vitokhangnguyen commented Oct 3, 2019

Here it is:

mongodb+srv://[ADMIN_USERNAME]:[ADMIN_PASSOWRD]@bti425-cluster-jyzoe.azure.mongodb.net/[DATABASE_NAME]?retryWrites=true&w=majority
@vitokhangnguyen

This comment has been minimized.

Copy link
Contributor Author

vitokhangnguyen commented Oct 3, 2019

There's one more thing I would syggest you to check. Make sure your cluster allows connection from all IP (0.0.0.0).

@Haider8

This comment has been minimized.

Copy link
Owner

Haider8 commented Oct 3, 2019

@vitokhangnguyen

This comment has been minimized.

Copy link
Contributor Author

vitokhangnguyen commented Oct 3, 2019

I did some Googling and maybe you can try...

  • Use connection string for an earlier version of Nodejs (you can get this in MongoDB Atlas)
  • Create a collection on MongoDB Atlas before running the API

I hope this helps...

server.js Outdated

var jwtOptions = {};
jwtOptions.jwtFromRequest = ExtractJwt.fromAuthHeaderAsBearerToken();
jwtOptions.secretOrKey = 'xIyC#bzK$42AmKfU8GyHw^9sPtF&C2$Ic$mKolkiU6XVK88KYG';

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Oct 3, 2019

Make this an environment variable

This comment has been minimized.

Copy link
@Haider8

Haider8 Oct 3, 2019

Owner

Do this change


app.post('/api/user/register', async (req, res) => {
try {
let user = await auth.register(req.body);

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Oct 3, 2019

There should be data validation that user has sent data in correct format?

Maybe using joi or alternative https://github.com/hapijs/joi or https://docs.nestjs.com/techniques/validation

},

register: async function(newUser) {
if (!newUser.userName || newUser.userName == '') {

This comment has been minimized.

Copy link
@iamareebjamal

iamareebjamal Oct 3, 2019

Use underscores as separators instead of camel case. It makes it extremely difficult to create clients in mobile

This comment has been minimized.

Copy link
@Haider8

Haider8 Oct 3, 2019

Owner

And do this change also. The rest of the changes can be done later. I will open new issues for that.

@Haider8

This comment has been minimized.

Copy link
Owner

Haider8 commented Oct 3, 2019

Here it is:

mongodb+srv://[ADMIN_USERNAME]:[ADMIN_PASSOWRD]@bti425-cluster-jyzoe.azure.mongodb.net/[DATABASE_NAME]?retryWrites=true&w=majority

This works now @vitokhangnguyen . MongoDB was generating the wrong connection URL for me. Thank you!

Copy link

iamareebjamal left a comment

I know this is very nascent stage, but perhaps it should be using a proper framework like nestjs to avoid common problems https://docs.nestjs.com/techniques/mongodb

Also, I recommend typeorm so that if in future, the requirement is to use a relational database(as one should), it'll be easy to integrate https://github.com/typeorm/typeorm

Also, will make it easy to swap for sqlite for local development

@Haider8

This comment has been minimized.

Copy link
Owner

Haider8 commented Oct 3, 2019

@vitokhangnguyen All the routes are working fine.

Work on the changes suggested by @iamareebjamal and we can then merge this PR.

@iamareebjamal

This comment has been minimized.

Copy link

iamareebjamal commented Oct 3, 2019

You can choose between https://foalts.org/ and nest later on, but at least change camel case to underscores and move secret key to env

@vitokhangnguyen

This comment has been minimized.

Copy link
Contributor Author

vitokhangnguyen commented Oct 3, 2019

Hi,
I will make the change soon. Thank you for all of your effort and feedbacks.

@Haider8

This comment has been minimized.

Copy link
Owner

Haider8 commented Oct 3, 2019

@vitokhangnguyen Just do those two changes. We can do other changes later on.

@vitokhangnguyen

This comment has been minimized.

Copy link
Contributor Author

vitokhangnguyen commented Oct 3, 2019

I've made the 2 changes. I will work on the client-side now to enable user authentication.

@Haider8 Haider8 merged commit c6e4b31 into Haider8:master Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.