🔀 Add login flow and update user model #64

mckingho · 2018-06-20T12:14:47Z

No description provided.

williamchong

also commit message is ambiguous, probably want to state clearly what jwt and avatar is for, and where

williamchong · 2018-06-21T05:11:28Z

server/api/users.js

+      }
+    }
+    const payload = await getUserChallenge(wallet);
+    if (!payload || !payload.wallet) res.sendStatus(404);


need a exit/throw after res.sendStatus(404) or it will continue to run

williamchong · 2018-06-21T05:11:44Z

package.json

    "cross-env": "^5.1.6",
+    "crypto": "^1.0.1",


crypto should be built-in for current nodejs version and should not be pull from npm

williamchong · 2018-06-21T05:14:41Z

server/util/jwt.js

+  });
+};
+
+export const jwtAuthNoBlock = (req, res, next) => {


jwtAuth and jwtAuthNoBlock should either be

Two functions calling 1 same function with different params, or

Make jwtAuth a function that returns a middleware, with credentialsRequired value based on a input param

williamchong · 2018-06-21T05:15:58Z

server/util/jwt.js

+  );
+  res.setHeader('Pragma', 'no-cache');
+  res.setHeader('Expires', '0');
+}


ambiguous naming of setAuthHeader which is actually setting header to no-cache

williamchong · 2018-06-21T05:16:57Z

server/api/index.js


 import users from './users';
 import assets from './assets';

 const router = Router();

+router.use(cookieParser());
+router.use(bodyParser.json());
+router.use(bodyParser.urlencoded({ extended: true }));


probably should remove urlencoded if not needed

williamchong · 2018-06-21T05:26:14Z

server/util/auth.js

+      params: { wallet: queryWallet },
+    }));
+  } else {
+    data = Object.assign({ wallet: queryWallet }, LIKECOIN_USER_STUB);


LIKECOIN_USER_STUB should be used only when NODE_ENV is not production

williamchong · 2018-06-21T05:26:17Z

server/util/auth.js

+      wallet,
+    }));
+  } else {
+    data = Object.assign({ wallet }, LIKECOIN_USER_STUB);


LIKECOIN_USER_STUB should be used only when NODE_ENV is not production

williamchong · 2018-06-21T05:32:48Z

server/util/auth.js

+    avatar,
+  } = data;
+  if (!likecoinId || !displayName || wallet !== responseWallet) {
+    throw new Error('POST_CHALLENGE_FAILED');


probably want to wrap whole function in try catch for POST_CHALLENGE_FAILED
and this particular part seems like SOMETHING_EVIL_IS_GOING_ON for me

williamchong · 2018-06-21T05:38:43Z

server/util/jwt.js

+  }
+  throw new UnauthorizedError('credentials_required', {
+    message: 'No authorization token was found',
+  });


should obey credentialsRequired flag instead of checking req.route.path and throwing UnauthorizedError randomly
probably want to remove line 24 to 29 and just return null

williamchong · 2018-06-21T05:40:28Z

server/util/jwt.js

+}
+
+export const jwtSign = (payload) =>
+  jwt.sign(payload, secret, { expiresIn: '7d' });


7d should be configurable and no idea why cookie expire in 365 but jwt in 7

williamchong · 2018-06-21T13:42:43Z

server/util/auth.js

+    const { challenge, signature, wallet } = payload;
+    let data;
+    if (process.env.production) {
+      ({ data } = await axios.post(LIKECOIN_AUTH_URL, {


should be process.env.NODE_ENV === 'production' and might also want to check if LIKECOIN_AUTH_URL is not falsy

williamchong · 2018-06-21T13:42:58Z

server/util/auth.js

+export async function getUserChallenge(queryWallet) {
+  let data;
+  if (process.env.production) {
+    ({ data } = await axios.get(LIKECOIN_AUTH_URL, {


should be process.env.NODE_ENV === 'production' and might also want to check if LIKECOIN_AUTH_URL is not falsy

williamchong · 2018-06-21T13:43:21Z

server/util/jwt.js

+  ) {
+    return req.headers.authorization.split(' ')[1];
+  } else if (req.cookies && req.cookies[JWT_COOKIE_KEY]) {
+    return req.cookies.auth;


should be req.cookies[JWT_COOKIE_KEY]

williamchong · 2018-06-21T13:43:53Z

server/util/jwt.js

+}
+
+export const jwtSign = (payload) =>
+  jwt.sign(payload, secret, { expiresIn: `${JWT_EXPIRE_DAYS}d` });


should not assume it must be d (days), whole string should be configurable

williamchong · 2018-06-21T13:45:54Z

server/util/jwt.js

+      next(e);
+    });
+  };
+  return auth;


just return a arrow function without declaring auth

mckingho · 2018-06-22T08:01:24Z

Connects #37

rickmak

Various comments but not a merge blocker.

rickmak · 2018-06-25T06:31:21Z

server/util/auth.js

+
+export async function getUserChallenge(queryWallet) {
+  let data;
+  if (process.env.NODE_ENV === 'production' || LIKECOIN_AUTH_URL) {


After introducing of config/server.js, the rule should be no process.env in application code.

rickmak · 2018-06-25T06:33:05Z

server/util/auth.js

+      likecoinId,
+      wallet: responseWallet,
+    };
+  } catch (err) {


The try catch scope it too big. It will make debuging very difficult.

For example, we don't know it is like api fails or our application fails to handle edge case.

rickmak · 2018-06-25T07:45:43Z

server/util/jwt.js

+  res.setHeader('Expires', '0');
+}
+
+export const jwtSign = (payload) =>


It is not symmetric with other API in same apckage. Other are operating on request and request header level. This one is pure function.

I think it should accept a payload and request?

rickmak · 2018-06-25T07:46:56Z

server/util/jwt.js

+  crypto
+    .randomBytes(64)
+    .toString('hex')
+    .slice(0, 64);


This default should go to the config/server

rickmak · 2018-06-25T07:47:52Z

server/util/jwt.js

+  });
+};
+
+export const AUTH_COOKIE_OPTION = {


No need to expose it and the options logic should belong to this package.

mckingho added the review label Jun 20, 2018

mckingho requested a review from williamchong June 20, 2018 12:14

mckingho force-pushed the master branch from 7a2aa6f to 709f0b5 Compare June 21, 2018 03:47

williamchong requested changes Jun 21, 2018

View reviewed changes

mckingho force-pushed the master branch 3 times, most recently from 6d8e757 to 8bf8d48 Compare June 21, 2018 13:17

williamchong reviewed Jun 21, 2018

View reviewed changes

mckingho force-pushed the master branch from 702eb6c to 3050377 Compare June 21, 2018 14:13

mckingho requested a review from rickmak June 22, 2018 02:34

mckingho force-pushed the master branch from 3050377 to 1bf0bf2 Compare June 22, 2018 07:20

mckingho added 9 commits June 22, 2018 15:27

✨ Add jwt in login api

63653ac

🗃 Add avatar in user model

42c17ef

✨ Add user login api

004ffe4

🐛 Fix user api test

69ec6fe

🎨 Rewrite jwt files with configurable variables

e7094d6

🎨 Wrap auth request in try catch

37dc39a

✅ Add tests for login api

120ab89

🐛 Fix user login test case and linter

bc56b3b

🐛 Remove avatar data url in server side api; Fix review

5807a17

mckingho force-pushed the master branch from 1bf0bf2 to 5807a17 Compare June 22, 2018 07:28

williamchong approved these changes Jun 22, 2018

View reviewed changes

✅ Increase jest timeout in some testcases

b3a9562

rickmak reviewed Jun 25, 2018

View reviewed changes

rickmak merged commit b3a9562 into likecoin:master Jun 25, 2018

rickmak removed the review label Jun 25, 2018

This was referenced Jun 27, 2018

Restrict the usage of process.env to server/config only #79

Open

Improve the server/util/jwt.js expose API to make is symmetry #80

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔀 Add login flow and update user model #64

🔀 Add login flow and update user model #64

mckingho commented Jun 20, 2018

williamchong left a comment

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

williamchong Jun 21, 2018

mckingho commented Jun 22, 2018

rickmak left a comment

rickmak Jun 25, 2018

rickmak Jun 25, 2018

rickmak Jun 25, 2018

rickmak Jun 25, 2018

rickmak Jun 25, 2018

🔀 Add login flow and update user model #64

🔀 Add login flow and update user model #64

Conversation

mckingho commented Jun 20, 2018

williamchong 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckingho commented Jun 22, 2018

rickmak 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