From a14266077d3541c7a33b7635efa4208335e73519 Mon Sep 17 00:00:00 2001 From: Sabir Hassan Date: Fri, 30 Sep 2022 14:41:09 +0500 Subject: [PATCH] fix: no need to restrict api endpoints when ldap auth is applied --- api/src/controllers/group.ts | 35 +++++++++++++++--- api/src/controllers/user.ts | 54 ++++++++++++++++++++++++---- api/src/controllers/web.ts | 2 +- api/src/middlewares/desktop.ts | 14 +------- api/src/routes/api/group.ts | 52 +++++---------------------- api/src/routes/api/spec/user.spec.ts | 42 +++++++++++----------- api/src/routes/api/user.ts | 47 ++++++++++-------------- 7 files changed, 126 insertions(+), 120 deletions(-) diff --git a/api/src/controllers/group.ts b/api/src/controllers/group.ts index c5b8681..990d0f1 100644 --- a/api/src/controllers/group.ts +++ b/api/src/controllers/group.ts @@ -12,6 +12,7 @@ import { import Group, { GroupPayload, PUBLIC_GROUP_NAME } from '../model/Group' import User from '../model/User' +import { AuthProviderType } from '../utils' import { UserResponse } from './user' export interface GroupResponse { @@ -147,12 +148,22 @@ export class GroupController { @Delete('{groupId}') public async deleteGroup(@Path() groupId: number) { const group = await Group.findOne({ groupId }) - if (group) return await group.remove() - throw { - code: 404, - status: 'Not Found', - message: 'Group not found.' + if (!group) + throw { + code: 404, + status: 'Not Found', + message: 'Group not found.' + } + + if (group.authProvider !== AuthProviderType.Internal) { + throw { + code: 405, + status: 'Method Not Allowed', + message: 'Can not delete group created by an external auth provider.' + } } + + return await group.remove() } } @@ -248,6 +259,13 @@ const updateUsersListInGroup = async ( message: `Can't add/remove user to '${PUBLIC_GROUP_NAME}' group.` } + if (group.authProvider !== AuthProviderType.Internal) + throw { + code: 405, + status: 'Method Not Allowed', + message: `Can't add/remove user to group created by external auth provider.` + } + const user = await User.findOne({ id: userId }) if (!user) throw { @@ -256,6 +274,13 @@ const updateUsersListInGroup = async ( message: 'User not found.' } + if (user.authProvider !== AuthProviderType.Internal) + throw { + code: 405, + status: 'Method Not Allowed', + message: `Can't add/remove user to group created by external auth provider.` + } + const updatedGroup = action === 'addUser' ? await group.addUser(user) diff --git a/api/src/controllers/user.ts b/api/src/controllers/user.ts index f410853..0c96b55 100644 --- a/api/src/controllers/user.ts +++ b/api/src/controllers/user.ts @@ -17,7 +17,12 @@ import { import { desktopUser } from '../middlewares' import User, { UserPayload } from '../model/User' -import { getUserAutoExec, updateUserAutoExec, ModeType } from '../utils' +import { + getUserAutoExec, + updateUserAutoExec, + ModeType, + AuthProviderType +} from '../utils' import { GroupResponse } from './group' export interface UserResponse { @@ -211,7 +216,11 @@ const createUser = async (data: UserPayload): Promise => { // Checking if user is already in the database const usernameExist = await User.findOne({ username }) - if (usernameExist) throw new Error('Username already exists.') + if (usernameExist) + throw { + code: 409, + message: 'Username already exists.' + } // Hash passwords const hashPassword = User.hashPassword(password) @@ -255,7 +264,11 @@ const getUser = async ( 'groupId name description -_id' )) as unknown as UserDetailsResponse - if (!user) throw new Error('User is not found.') + if (!user) + throw { + code: 404, + message: 'User is not found.' + } return { id: user.id, @@ -284,6 +297,19 @@ const updateUser = async ( const params: any = { displayName, isAdmin, isActive, autoExec } + const user = await User.findOne(findBy) + + if ( + user?.authProvider !== AuthProviderType.Internal && + (username !== user?.username || displayName !== user?.displayName) + ) { + throw { + code: 405, + message: + 'Can not update username and display name of user that is created by an external auth provider.' + } + } + if (username) { // Checking if user is already in the database const usernameExist = await User.findOne({ username }) @@ -292,7 +318,10 @@ const updateUser = async ( (findBy.id && usernameExist.id != findBy.id) || (findBy.username && usernameExist.username != findBy.username) ) - throw new Error('Username already exists.') + throw { + code: 409, + message: 'Username already exists.' + } } params.username = username } @@ -305,7 +334,10 @@ const updateUser = async ( const updatedUser = await User.findOneAndUpdate(findBy, params, { new: true }) if (!updatedUser) - throw new Error(`Unable to find user with ${findBy.id || findBy.username}`) + throw { + code: 404, + message: `Unable to find user with ${findBy.id || findBy.username}` + } return { id: updatedUser.id, @@ -332,11 +364,19 @@ const deleteUser = async ( { password }: { password?: string } ) => { const user = await User.findOne(findBy) - if (!user) throw new Error('User is not found.') + if (!user) + throw { + code: 404, + message: 'User is not found.' + } if (!isAdmin) { const validPass = user.comparePassword(password!) - if (!validPass) throw new Error('Invalid password.') + if (!validPass) + throw { + code: 401, + message: 'Invalid password.' + } } await User.deleteOne(findBy) diff --git a/api/src/controllers/web.ts b/api/src/controllers/web.ts index b3b7d08..e496235 100644 --- a/api/src/controllers/web.ts +++ b/api/src/controllers/web.ts @@ -86,7 +86,7 @@ const login = async ( if (!user) throw new Error('Username is not found.') if ( - process.env.AUTH_MECHANISM === AuthProviderType.LDAP && + process.env.AUTH_PROVIDERS === AuthProviderType.LDAP && user.authProvider === AuthProviderType.LDAP ) { const ldapClient = await LDAPClient.init() diff --git a/api/src/middlewares/desktop.ts b/api/src/middlewares/desktop.ts index 4db2396..b2935fd 100644 --- a/api/src/middlewares/desktop.ts +++ b/api/src/middlewares/desktop.ts @@ -1,7 +1,7 @@ import { RequestHandler, Request } from 'express' import { userInfo } from 'os' import { RequestUser } from '../types' -import { ModeType, AuthProviderType } from '../utils' +import { ModeType } from '../utils' const regexUser = /^\/SASjsApi\/user\/[0-9]*$/ // /SASjsApi/user/1 @@ -27,18 +27,6 @@ export const desktopRestrict: RequestHandler = (req, res, next) => { next() } -export const ldapRestrict: RequestHandler = (req, res, next) => { - const { AUTH_MECHANISM } = process.env - - if (AUTH_MECHANISM === AuthProviderType.LDAP) { - return res - .status(403) - .send(`Not Allowed while AUTH_MECHANISM is '${AuthProviderType.LDAP}'.`) - } - - next() -} - export const desktopUser: RequestUser = { userId: 12345, clientId: 'desktop_app', diff --git a/api/src/routes/api/group.ts b/api/src/routes/api/group.ts index 7e83dff..f8712ee 100644 --- a/api/src/routes/api/group.ts +++ b/api/src/routes/api/group.ts @@ -1,17 +1,12 @@ import express from 'express' import { GroupController } from '../../controllers/' -import { - ldapRestrict, - authenticateAccessToken, - verifyAdmin -} from '../../middlewares' +import { authenticateAccessToken, verifyAdmin } from '../../middlewares' import { getGroupValidation, registerGroupValidation } from '../../utils' const groupRouter = express.Router() groupRouter.post( '/', - ldapRestrict, authenticateAccessToken, verifyAdmin, async (req, res) => { @@ -23,11 +18,7 @@ groupRouter.post( const response = await controller.createGroup(body) res.send(response) } catch (err: any) { - const statusCode = err.code - - delete err.code - - res.status(statusCode).send(err.message) + res.status(err.code).send(err.message) } } ) @@ -38,11 +29,7 @@ groupRouter.get('/', authenticateAccessToken, async (req, res) => { const response = await controller.getAllGroups() res.send(response) } catch (err: any) { - const statusCode = err.code - - delete err.code - - res.status(statusCode).send(err.message) + res.status(err.code).send(err.message) } }) @@ -54,11 +41,7 @@ groupRouter.get('/:groupId', authenticateAccessToken, async (req, res) => { const response = await controller.getGroup(parseInt(groupId)) res.send(response) } catch (err: any) { - const statusCode = err.code - - delete err.code - - res.status(statusCode).send(err.message) + res.status(err.code).send(err.message) } }) @@ -76,18 +59,13 @@ groupRouter.get( const response = await controller.getGroupByGroupName(name) res.send(response) } catch (err: any) { - const statusCode = err.code - - delete err.code - - res.status(statusCode).send(err.message) + res.status(err.code).send(err.message) } } ) groupRouter.post( '/:groupId/:userId', - ldapRestrict, authenticateAccessToken, verifyAdmin, async (req, res) => { @@ -101,18 +79,13 @@ groupRouter.post( ) res.send(response) } catch (err: any) { - const statusCode = err.code - - delete err.code - - res.status(statusCode).send(err.message) + res.status(err.code).send(err.message) } } ) groupRouter.delete( '/:groupId/:userId', - ldapRestrict, authenticateAccessToken, verifyAdmin, async (req, res) => { @@ -126,18 +99,13 @@ groupRouter.delete( ) res.send(response) } catch (err: any) { - const statusCode = err.code - - delete err.code - - res.status(statusCode).send(err.message) + res.status(err.code).send(err.message) } } ) groupRouter.delete( '/:groupId', - ldapRestrict, authenticateAccessToken, verifyAdmin, async (req, res) => { @@ -148,11 +116,7 @@ groupRouter.delete( await controller.deleteGroup(parseInt(groupId)) res.status(200).send('Group Deleted!') } catch (err: any) { - const statusCode = err.code - - delete err.code - - res.status(statusCode).send(err.message) + res.status(err.code).send(err.message) } } ) diff --git a/api/src/routes/api/spec/user.spec.ts b/api/src/routes/api/spec/user.spec.ts index 12e68d5..b2e000f 100644 --- a/api/src/routes/api/spec/user.spec.ts +++ b/api/src/routes/api/spec/user.spec.ts @@ -110,16 +110,16 @@ describe('user', () => { expect(res.body).toEqual({}) }) - it('should respond with Forbidden if username is already present', async () => { + it('should respond with Conflict if username is already present', async () => { await controller.createUser(user) const res = await request(app) .post('/SASjsApi/user') .auth(adminAccessToken, { type: 'bearer' }) .send(user) - .expect(403) + .expect(409) - expect(res.text).toEqual('Error: Username already exists.') + expect(res.text).toEqual('Username already exists.') expect(res.body).toEqual({}) }) @@ -254,7 +254,7 @@ describe('user', () => { expect(res.body).toEqual({}) }) - it('should respond with Forbidden if username is already present', async () => { + it('should respond with Conflict if username is already present', async () => { const dbUser1 = await controller.createUser(user) const dbUser2 = await controller.createUser({ ...user, @@ -265,9 +265,9 @@ describe('user', () => { .patch(`/SASjsApi/user/${dbUser1.id}`) .auth(adminAccessToken, { type: 'bearer' }) .send({ username: dbUser2.username }) - .expect(403) + .expect(409) - expect(res.text).toEqual('Error: Username already exists.') + expect(res.text).toEqual('Username already exists.') expect(res.body).toEqual({}) }) @@ -349,7 +349,7 @@ describe('user', () => { expect(res.body).toEqual({}) }) - it('should respond with Forbidden if username is already present', async () => { + it('should respond with Conflict if username is already present', async () => { const dbUser1 = await controller.createUser(user) const dbUser2 = await controller.createUser({ ...user, @@ -360,9 +360,9 @@ describe('user', () => { .patch(`/SASjsApi/user/by/username/${dbUser1.username}`) .auth(adminAccessToken, { type: 'bearer' }) .send({ username: dbUser2.username }) - .expect(403) + .expect(409) - expect(res.text).toEqual('Error: Username already exists.') + expect(res.text).toEqual('Username already exists.') expect(res.body).toEqual({}) }) }) @@ -446,7 +446,7 @@ describe('user', () => { expect(res.body).toEqual({}) }) - it('should respond with Forbidden when user himself requests and password is incorrect', async () => { + it('should respond with Unauthorized when user himself requests and password is incorrect', async () => { const dbUser = await controller.createUser(user) const accessToken = await generateAndSaveToken(dbUser.id) @@ -454,9 +454,9 @@ describe('user', () => { .delete(`/SASjsApi/user/${dbUser.id}`) .auth(accessToken, { type: 'bearer' }) .send({ password: 'incorrectpassword' }) - .expect(403) + .expect(401) - expect(res.text).toEqual('Error: Invalid password.') + expect(res.text).toEqual('Invalid password.') expect(res.body).toEqual({}) }) @@ -528,7 +528,7 @@ describe('user', () => { expect(res.body).toEqual({}) }) - it('should respond with Forbidden when user himself requests and password is incorrect', async () => { + it('should respond with Unauthorized when user himself requests and password is incorrect', async () => { const dbUser = await controller.createUser(user) const accessToken = await generateAndSaveToken(dbUser.id) @@ -536,9 +536,9 @@ describe('user', () => { .delete(`/SASjsApi/user/by/username/${dbUser.username}`) .auth(accessToken, { type: 'bearer' }) .send({ password: 'incorrectpassword' }) - .expect(403) + .expect(401) - expect(res.text).toEqual('Error: Invalid password.') + expect(res.text).toEqual('Invalid password.') expect(res.body).toEqual({}) }) }) @@ -652,16 +652,16 @@ describe('user', () => { expect(res.body).toEqual({}) }) - it('should respond with Forbidden if userId is incorrect', async () => { + it('should respond with Not Found if userId is incorrect', async () => { await controller.createUser(user) const res = await request(app) .get('/SASjsApi/user/1234') .auth(adminAccessToken, { type: 'bearer' }) .send() - .expect(403) + .expect(404) - expect(res.text).toEqual('Error: User is not found.') + expect(res.text).toEqual('User is not found.') expect(res.body).toEqual({}) }) @@ -731,16 +731,16 @@ describe('user', () => { expect(res.body).toEqual({}) }) - it('should respond with Forbidden if username is incorrect', async () => { + it('should respond with Not Found if username is incorrect', async () => { await controller.createUser(user) const res = await request(app) .get('/SASjsApi/user/by/username/randomUsername') .auth(adminAccessToken, { type: 'bearer' }) .send() - .expect(403) + .expect(404) - expect(res.text).toEqual('Error: User is not found.') + expect(res.text).toEqual('User is not found.') expect(res.body).toEqual({}) }) }) diff --git a/api/src/routes/api/user.ts b/api/src/routes/api/user.ts index 2baa54f..d0ee7cc 100644 --- a/api/src/routes/api/user.ts +++ b/api/src/routes/api/user.ts @@ -3,8 +3,7 @@ import { UserController } from '../../controllers/' import { authenticateAccessToken, verifyAdmin, - verifyAdminIfNeeded, - ldapRestrict + verifyAdminIfNeeded } from '../../middlewares' import { deleteUserValidation, @@ -15,24 +14,18 @@ import { const userRouter = express.Router() -userRouter.post( - '/', - ldapRestrict, - authenticateAccessToken, - verifyAdmin, - async (req, res) => { - const { error, value: body } = registerUserValidation(req.body) - if (error) return res.status(400).send(error.details[0].message) +userRouter.post('/', authenticateAccessToken, verifyAdmin, async (req, res) => { + const { error, value: body } = registerUserValidation(req.body) + if (error) return res.status(400).send(error.details[0].message) - const controller = new UserController() - try { - const response = await controller.createUser(body) - res.send(response) - } catch (err: any) { - res.status(403).send(err.toString()) - } + const controller = new UserController() + try { + const response = await controller.createUser(body) + res.send(response) + } catch (err: any) { + res.status(err.code).send(err.message) } -) +}) userRouter.get('/', authenticateAccessToken, async (req, res) => { const controller = new UserController() @@ -40,7 +33,7 @@ userRouter.get('/', authenticateAccessToken, async (req, res) => { const response = await controller.getAllUsers() res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + res.status(err.code).send(err.message) } }) @@ -58,7 +51,7 @@ userRouter.get( const response = await controller.getUserByUsername(req, username) res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + res.status(err.code).send(err.message) } } ) @@ -71,13 +64,12 @@ userRouter.get('/:userId', authenticateAccessToken, async (req, res) => { const response = await controller.getUser(req, parseInt(userId)) res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + res.status(err.code).send(err.message) } }) userRouter.patch( '/by/username/:username', - ldapRestrict, authenticateAccessToken, verifyAdminIfNeeded, async (req, res) => { @@ -99,14 +91,13 @@ userRouter.patch( const response = await controller.updateUserByUsername(username, body) res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + res.status(err.code).send(err.message) } } ) userRouter.patch( '/:userId', - ldapRestrict, authenticateAccessToken, verifyAdminIfNeeded, async (req, res) => { @@ -122,14 +113,13 @@ userRouter.patch( const response = await controller.updateUser(parseInt(userId), body) res.send(response) } catch (err: any) { - res.status(403).send(err.toString()) + res.status(err.code).send(err.message) } } ) userRouter.delete( '/by/username/:username', - ldapRestrict, authenticateAccessToken, verifyAdminIfNeeded, async (req, res) => { @@ -151,14 +141,13 @@ userRouter.delete( await controller.deleteUserByUsername(username, data, user!.isAdmin) res.status(200).send('Account Deleted!') } catch (err: any) { - res.status(403).send(err.toString()) + res.status(err.code).send(err.message) } } ) userRouter.delete( '/:userId', - ldapRestrict, authenticateAccessToken, verifyAdminIfNeeded, async (req, res) => { @@ -174,7 +163,7 @@ userRouter.delete( await controller.deleteUser(parseInt(userId), data, user!.isAdmin) res.status(200).send('Account Deleted!') } catch (err: any) { - res.status(403).send(err.toString()) + res.status(err.code).send(err.message) } } )