#166265850 Implements role based functionality #31

encodedBicoding · 2019-06-05T16:47:00Z

What does this PR do?

Implements role-based functionality.
This creates a new super admin, also creates a route that will enable the super admin assign role to other users.
This also creates a middle-ware which can be added to protect routes from unauthorized users

Description of Task to be completed?

N/A

How should this be manually tested?

N/A

Any background context you want to provide?

The middle-ware used to check if a user is an admin or a regular user before they can access a certain feature has been commented out. Please uncomment it only when you want to use it.

What are the relevant pivotal tracker stories?

#166265850

Screenshots (if appropriate)

Questions:

houndci-bot · 2019-06-05T16:47:09Z

server/controllers/userController.js

+    if(!user) return util.errorStatus(res, 401, 'Not authorized');
+    if(user['role'] !== 'admin'){
+      return util.errorStatus(res, 401, 'Not authorized');
+    } else {


Unnecessary 'else' after 'return' no-else-return

server/controllers/userController.js

server/__test__/user.spec.js

server/db/seeders/20190528204615-demo-user.js

coveralls · 2019-06-05T16:50:06Z

Coverage decreased (-0.3%) to 90.669% when pulling 437f769 on feat/166265850/role-based-functionality into 3a7afc0 on develop.

emp-daisy · 2019-06-05T18:13:38Z

@encodedBicoding Fix all your hound issues and coverage

emp-daisy · 2019-06-05T22:48:40Z

server/db/models/user.js

        allowNull: true
+      },
+      role: {
+        type: DataTypes.ENUM('admin', 'user'),


I can see you defined two roles while you use three. Where was "super_admin" defined?

emp-daisy · 2019-06-05T22:50:42Z

server/db/seeders/20190528204615-demo-user.js

        profilePic: null
+      },
+      {
+          firstName: process.env.SUPER_ADMIN_FIRSTNAME,


Why is this coming from the environment variable?

To protect data?

I doubt it matters for seeded users

server/db/migrations/20190528151100-create-user.js

emp-daisy · 2019-06-05T22:51:44Z

server/controllers/userController.js

+    }
+  }
+
+  // static async checkUserRole(req, res, next) {


Remove unused comments

Please format your code well and also add a few line spaces to make them readable.

emp-daisy · 2019-06-05T22:53:34Z

server/controllers/userController.js

    return util.successStatus(res, 200, 'Password reset successfully');
  }
+  static async assignUserRole(req, res) {
+    const { id } = req.user ? req.user : req.query;


Where is this coming from?

User id should never come from a query in this case, the token used for the request is a better option to prevent using other users details to make changes

cvjude · 2019-06-06T12:57:21Z

server/controllers/userController.js

+    if( !id ) { return util.errorStatus(res, 401, 'Not Authorized'); }
+    try {
+        const superAdmin = await models.Users.findByPk(id);
+        if(!superAdmin || superAdmin.role !== 'super_admin') { return util.errorStatus(res, 401, 'Not Authorized'); }


I think the Authorization should be done in a separate file, to avoid repetition.

houndci-bot · 2019-06-06T18:58:48Z

server/__test__/user.spec.js

+      .set('Authorization', 'bearer kjjodndsfj94mkfdsif0dfdsfmosj')
+      .send({email: 'john.doe@test.com', role: 'admin'})
+      .end((err, res) => {
+        console.log(res.body)


Unexpected console statement no-console

houndci-bot · 2019-06-06T18:58:48Z

server/__test__/user.spec.js

+      .set('Authorization', `bearer ${superAdminToken}`)
+      .send({email: 'john.doe@test.com', role: 'admin'})
+      .end((err, res) => {
+        console.log(res.body)


Unexpected console statement no-console

houndci-bot · 2019-06-07T06:59:21Z

server/middleware/authenticator.js

-    const token = req.headers.authorization.split(' ')[1];
+    let token = req.headers.authorization;
+    if(token.startsWith('bearer ')) {
+      token = req.headers.authorization.split(' ')[1]


Use array destructuring prefer-destructuring

emp-daisy · 2019-06-07T07:43:07Z

server/controllers/userController.js

  }
+
+  static async assignUserRole(req, res) {
+    let token  = req.headers.authorization ? req.headers.authorization : req.query.token;


A middleware already exist for this

Hadeneekeh · 2019-06-07T08:11:35Z

server/middleware/authenticator.js

-
-    const token = req.headers.authorization.split(' ')[1];
+    let token = req.headers.authorization;
+    if(token.startsWith('bearer ')) {


@encodedBicoding please what is the reason for this line?

The token is a string that starts with bearer, or doesn't. The way u implemented it before, it just splits the main token, but that line checks first if there is bearer before the main token before splitting

Well, I think that the if block is not necessary since the split token still gets verified in the try block that follows.

I think I understand what @encodedBicoding is trying to do. He's making a provision to just insert the token without appending the word Bearer. I think it's a good idea to take away the stress of always appending that bearer word but I don't know if it's standard. Maybe I'll look it up after demo today.

Hadeneekeh · 2019-06-07T08:14:39Z

server/controllers/userController.js

  }
+
+  static async assignUserRole(req, res) {
+    let token  = req.headers.authorization ? req.headers.authorization : req.query.token;


houndci-bot · 2019-06-07T13:11:27Z

server/controllers/userController.js

      const { email_confirm_code, email } = user;
-
+
+      if (email_confirm_code === null) return util.errorStatus(res, 403, 'Email already verified');


Identifier 'email_confirm_code' is not in camel case camelcase

emp-daisy · 2019-06-07T13:22:29Z

server/db/models/user.js

+      },
+      role: {
+        type: DataTypes.STRING,
+        allowNull: false


You can also set a default value so it doesn't have to stated everything

true, default value should be user. But it is also set during signup. I just thought to leave it like that.

emp-daisy · 2019-06-07T13:29:04Z

server/docs/hellobooks_api_doc.yaml

+      tags:
+        - Users
+      summary: Assign role to users
+      description: Authenticate by user role before they can perform certain actions


Something doesn't seem right here... Since it is a secured route, I expected to see the required security added

Will sort that after DEMO today.

setup role based functionality [(Finishes) #166265850]

houndci-bot reviewed Jun 5, 2019

View reviewed changes

server/db/seeders/20190528204615-demo-user.js Outdated Show resolved Hide resolved

encodedBicoding requested review from Hadeneekeh, OvieMudi, PelumiAlesh, chuxmykel, cvjude, emp-daisy and ocranbillions June 5, 2019 19:10

emp-daisy reviewed Jun 5, 2019

View reviewed changes

server/db/migrations/20190528151100-create-user.js Show resolved Hide resolved

emp-daisy reviewed Jun 5, 2019

View reviewed changes

cvjude reviewed Jun 6, 2019

View reviewed changes

encodedBicoding force-pushed the feat/166265850/role-based-functionality branch from 148006c to 4ac70aa Compare June 6, 2019 18:58

houndci-bot reviewed Jun 6, 2019

View reviewed changes

encodedBicoding force-pushed the feat/166265850/role-based-functionality branch from 4ac70aa to 9d327da Compare June 7, 2019 06:59

houndci-bot reviewed Jun 7, 2019

View reviewed changes

emp-daisy reviewed Jun 7, 2019

View reviewed changes

Hadeneekeh reviewed Jun 7, 2019

View reviewed changes

encodedBicoding force-pushed the feat/166265850/role-based-functionality branch from 9d327da to 133b7a3 Compare June 7, 2019 13:11

houndci-bot reviewed Jun 7, 2019

View reviewed changes

emp-daisy reviewed Jun 7, 2019

View reviewed changes

encodedBicoding added 3 commits June 7, 2019 17:43

feat(role-based-functionality):

f76eb92

setup role based functionality [(Finishes) #166265850]

Fix HoundCI complaint

1c7ae78

Fix HoundCI complaints

7aade9b

encodedBicoding added 9 commits June 7, 2019 17:43

update logic on role functionality

3ad1c87

update docs with assignrole route

e20a5c1

update login used in role based functionality

d677d85

update login used in role based functionality

f908e3e

update swagfer docs

dc2911b

update swagfer documentation

a060021

refactor role based funct algo

491f573

refactor role based funct algorithm

17145c6

update swagger docs with role based functionality

f4a0595

encodedBicoding force-pushed the feat/166265850/role-based-functionality branch from 133b7a3 to f4a0595 Compare June 7, 2019 16:44

emp-daisy previously approved these changes Jun 10, 2019

View reviewed changes

update assign role functionality

95c5725

encodedBicoding dismissed emp-daisy’s stale review via 437f769 June 10, 2019 10:07

update assign roles functionality

437f769

emp-daisy approved these changes Jun 10, 2019

View reviewed changes

emp-daisy merged commit 055aa37 into develop Jun 10, 2019

emp-daisy deleted the feat/166265850/role-based-functionality branch June 10, 2019 10:13

		const { email_confirm_code, email } = user;


		if (email_confirm_code === null) return util.errorStatus(res, 403, 'Email already verified');

#166265850 Implements role based functionality #31

#166265850 Implements role based functionality #31

Uh oh!

Conversation

encodedBicoding commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Description of Task to be completed?

How should this be manually tested?

Any background context you want to provide?

What are the relevant pivotal tracker stories?

Screenshots (if appropriate)

Questions:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emp-daisy commented Jun 5, 2019

Uh oh!

emp-daisy Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emp-daisy Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

encodedBicoding Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

encodedBicoding commented Jun 5, 2019 •

edited

Loading

coveralls commented Jun 5, 2019 •

edited

Loading

emp-daisy Jun 5, 2019 •

edited

Loading

emp-daisy Jun 5, 2019 •

edited

Loading

encodedBicoding Jun 7, 2019 •

edited

Loading