Setup boilerplate, Added Auth #1

temanisparsh · 2020-07-08T15:33:55Z

Setup a project structure
Setup auth service using passport-jwt
Setup basic login / register auth flows

aniketnk · 2020-07-10T15:55:32Z

Is this ready for merge?
Unit test included?
Let's work on getting a PR template? And CI/CD solution too.

temanisparsh · 2020-07-10T16:32:02Z

@aniketnk Not yet, it needs to be updated with these things:

Token is stored while logging in and the token is deleted from the DB when the user logs out.
Need to update this using typescript.
Need to write unit tests for auth.
Will update all these by tomorrow.
Should I use Mocha or Jest for tests?

aniketnk · 2020-07-10T19:32:18Z

Oh that's great. Looking forward to review!
I've made issues #2 and #3 for the GitHub actions/workflows. I will work on this over the weekend, and hopefully
we will have a solid ecosystem for development.

src/utils/auth.util.ts

RITIKHARIANI · 2020-07-12T19:43:15Z

Also please add comments wherever possible 😊

.gitignore

aniketnk

I've added comments in a few places. Resolve and/or reply if it needs to be discussed.
Can you also work on a README or CONTRIBUTING.md with dev setup instructions.

package.json

aniketnk · 2020-07-13T09:10:27Z

src/app.ts

+
+import passport from 'passport';
+
+import config from './config';


Do we need any error handling if the config file is not present?

Umm, I have an example config file and I will add in the instructions to create config in the rEADME, I don't see a reason to add error handling for this.

Okay.
But I think that a 'file not found' error doesn't provide the developer with enough information. That's why if we had some config store, and if that import failed we could throw an appropriate detailed error message. And that would have to be handled only in one place(where the store is initialised).

Got it, I will update it so that it throws an appropriate error for this.

This is the error it throws for now. Is this fine?

6 import config from './config'; ~~~~~~~~~~ at createTSError (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:434:12) at reportTSError (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:438:19) at getOutput (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:578:36) at Object.compile (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:775:32) at Module.m._compile (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:858:43) at Module._extensions..js (internal/modules/cjs/loader.js:789:10) at Object.require.extensions.(anonymous function) [as .ts] (/home/sparsh/Desktop/Sparsh/Projects/alcoding/posterior/node_modules/ts-node/src/index.ts:861:12) at Module.load (internal/modules/cjs/loader.js:653:32) at tryModuleLoad (internal/modules/cjs/loader.js:593:12) at Function.Module._load (internal/modules/cjs/loader.js:585:3) [nodemon] app crashed - waiting for file changes before starting...

No, this error message is not informative enough. Let's open an issue to solve this config feature.

src/config.example.ts

aniketnk · 2020-07-13T09:16:09Z

src/middleware/auth.middleware.ts

+  })
+);
+
+export const verifyUser = passport.authenticate('jwt', { session: false });


Since the JWT doesn't have any user information apart from the passport "id", does it do a database lookup to verify each time?

Yes, it does a DB lookup everytime

Hmmm. That's an unnecessary overhead. Any way around this?

I will have to look into this. I came up with a few solutions but most of them seem to have some or the other side effect.
Think of an ideal scenario where we have established a system where DB lookup isn't needed. Now I have logged in from 2 systems s1 and s2. If I were to change my password from s2 and then try to access my account from s1..there has to be a DB lookup to invalidate those credentials right? This would contradict the first assumption.

Yes, this issue won't arise once we have the refresh token mechanism. Let's add that as an issue and work on it later!

src/models/user.model.ts

src/routers/auth.router.ts

aniketnk · 2020-07-13T09:19:25Z

src/services/auth.service.ts

@@ -0,0 +1,27 @@
+import User, { IUser } from '../models/user.model';


Some documentation needed here too!

souravtecken

There was some hook that applied changes after a commit was made, I'm not sure where that's configured, it didn't prevent the commit from happening, it formatted a few files after the commit. Could that behaviour be changed?

temanisparsh · 2020-07-13T18:28:03Z

@souravtecken Yeah, it's a pre commit hook which runs the linter and checks other rules too.
Example: It will give you a warning when you have unused variables in your code. So you won't be able to commit till you fix it. According to me we should keep it. Another example would be to remove all console logs which everyone tends to use a lot during Dev.

aniketnk · 2020-07-18T12:16:28Z

LGTM, let's merge this so other work can go on in parallel !!
Would've liked more comments everywhere, let's add this on later.

src/controllers/auth.controller.ts

src/models/user.model.ts

aniketnk · 2020-07-20T20:56:45Z

Can you clean up the commit history so we can "rebase and merge"?

* added dependencies * added auth model and router * setup passport config

* convert to typescript * add types as ddev dependencies * add eslint and prettier config * setup nodemon dev server

temanisparsh requested review from Ash110 and souravtecken July 8, 2020 15:34

aniketnk marked this pull request as draft July 10, 2020 19:32

temanisparsh marked this pull request as ready for review July 12, 2020 12:13

temanisparsh requested review from adityavinodk and aniketnk July 12, 2020 12:14

RITIKHARIANI reviewed Jul 12, 2020

View reviewed changes

src/utils/auth.util.ts Outdated Show resolved Hide resolved

aniketnk reviewed Jul 13, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

aniketnk requested changes Jul 13, 2020

View reviewed changes

souravtecken reviewed Jul 13, 2020

View reviewed changes

temanisparsh requested a review from aniketnk July 14, 2020 07:19

aniketnk previously approved these changes Jul 18, 2020

View reviewed changes

adityavinodk reviewed Jul 19, 2020

View reviewed changes

src/controllers/auth.controller.ts Show resolved Hide resolved

src/models/user.model.ts Show resolved Hide resolved

aniketnk mentioned this pull request Jul 20, 2020

Remove unused dependencies #7

Closed

temanisparsh dismissed aniketnk’s stale review via b329bb1 July 21, 2020 01:52

temanisparsh added 2 commits July 21, 2020 13:11

initialize boilerplate [javascript]

90c8772

* added dependencies * added auth model and router * setup passport config

convert boilerplate to typescript

a9c371a

* convert to typescript * add types as ddev dependencies * add eslint and prettier config * setup nodemon dev server

temanisparsh requested a review from aniketnk July 21, 2020 21:00

temanisparsh merged commit 2fce313 into pes-alcoding-club:master Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup boilerplate, Added Auth #1

Setup boilerplate, Added Auth #1

temanisparsh commented Jul 8, 2020

aniketnk commented Jul 10, 2020

temanisparsh commented Jul 10, 2020

aniketnk commented Jul 10, 2020

RITIKHARIANI commented Jul 12, 2020

aniketnk left a comment

aniketnk Jul 13, 2020

temanisparsh Jul 13, 2020

aniketnk Jul 14, 2020

temanisparsh Jul 14, 2020

temanisparsh Jul 14, 2020

aniketnk Jul 18, 2020

aniketnk Jul 13, 2020

temanisparsh Jul 13, 2020

aniketnk Jul 14, 2020

temanisparsh Jul 14, 2020

aniketnk Jul 18, 2020

aniketnk Jul 13, 2020

souravtecken left a comment

temanisparsh commented Jul 13, 2020 •

edited

Loading

aniketnk commented Jul 18, 2020

aniketnk commented Jul 20, 2020


		import passport from 'passport';

		import config from './config';

		@@ -0,0 +1,27 @@
		import User, { IUser } from '../models/user.model';

Setup boilerplate, Added Auth #1

Setup boilerplate, Added Auth #1

Conversation

temanisparsh commented Jul 8, 2020

aniketnk commented Jul 10, 2020

temanisparsh commented Jul 10, 2020

aniketnk commented Jul 10, 2020

RITIKHARIANI commented Jul 12, 2020

aniketnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

souravtecken left a comment

Choose a reason for hiding this comment

temanisparsh commented Jul 13, 2020 • edited Loading

aniketnk commented Jul 18, 2020

aniketnk commented Jul 20, 2020

temanisparsh commented Jul 13, 2020 •

edited

Loading