From 2688a7c8d1a7903d012a1dea13b32d634edf536d Mon Sep 17 00:00:00 2001 From: Matheus Barbosa Silva <36537004+matheusbsilva137@users.noreply.github.com> Date: Mon, 6 May 2024 21:24:13 -0300 Subject: [PATCH] chore: Improve permissions check on users endpoints (#32353) --- apps/meteor/app/api/server/v1/users.ts | 38 ++++++-------------- apps/meteor/tests/end-to-end/api/01-users.js | 11 +++--- 2 files changed, 18 insertions(+), 31 deletions(-) diff --git a/apps/meteor/app/api/server/v1/users.ts b/apps/meteor/app/api/server/v1/users.ts index ccca23f8ea82..2fd9030690d6 100644 --- a/apps/meteor/app/api/server/v1/users.ts +++ b/apps/meteor/app/api/server/v1/users.ts @@ -308,13 +308,9 @@ API.v1.addRoute( API.v1.addRoute( 'users.delete', - { authRequired: true }, + { authRequired: true, permissionsRequired: ['delete-user'] }, { async post() { - if (!(await hasPermissionAsync(this.userId, 'delete-user'))) { - return API.v1.unauthorized(); - } - const user = await getUserFromParams(this.bodyParams); const { confirmRelinquish = false } = this.bodyParams; @@ -349,16 +345,15 @@ API.v1.addRoute( API.v1.addRoute( 'users.setActiveStatus', - { authRequired: true, validateParams: isUserSetActiveStatusParamsPOST }, + { + authRequired: true, + validateParams: isUserSetActiveStatusParamsPOST, + permissionsRequired: { + POST: { permissions: ['edit-other-user-active-status', 'manage-moderation-actions'], operation: 'hasAny' }, + }, + }, { async post() { - if ( - !(await hasPermissionAsync(this.userId, 'edit-other-user-active-status')) && - !(await hasPermissionAsync(this.userId, 'manage-moderation-actions')) - ) { - return API.v1.unauthorized(); - } - const { userId, activeStatus, confirmRelinquish = false } = this.bodyParams; await Meteor.callAsync('setUserActiveStatus', userId, activeStatus, confirmRelinquish); @@ -375,13 +370,9 @@ API.v1.addRoute( API.v1.addRoute( 'users.deactivateIdle', - { authRequired: true, validateParams: isUserDeactivateIdleParamsPOST }, + { authRequired: true, validateParams: isUserDeactivateIdleParamsPOST, permissionsRequired: ['edit-other-user-active-status'] }, { async post() { - if (!(await hasPermissionAsync(this.userId, 'edit-other-user-active-status'))) { - return API.v1.unauthorized(); - } - const { daysIdle, role = 'user' } = this.bodyParams; const lastLoggedIn = new Date(); @@ -452,13 +443,10 @@ API.v1.addRoute( { authRequired: true, queryOperations: ['$or', '$and'], + permissionsRequired: ['view-d-room'], }, { async get() { - if (!(await hasPermissionAsync(this.userId, 'view-d-room'))) { - return API.v1.unauthorized(); - } - if ( settings.get('API_Apply_permission_view-outside-room_on_users-list') && !(await hasPermissionAsync(this.userId, 'view-outside-room')) @@ -804,13 +792,9 @@ API.v1.addRoute( API.v1.addRoute( 'users.getPersonalAccessTokens', - { authRequired: true }, + { authRequired: true, permissionsRequired: ['create-personal-access-tokens'] }, { async get() { - if (!(await hasPermissionAsync(this.userId, 'create-personal-access-tokens'))) { - throw new Meteor.Error('not-authorized', 'Not Authorized'); - } - const user = (await Users.getLoginTokensByUserId(this.userId).toArray())[0] as unknown as IUser | undefined; const isPersonalAccessToken = (loginToken: ILoginToken | IPersonalAccessToken): loginToken is IPersonalAccessToken => diff --git a/apps/meteor/tests/end-to-end/api/01-users.js b/apps/meteor/tests/end-to-end/api/01-users.js index d973f1bd3ff0..a16cf30f7843 100644 --- a/apps/meteor/tests/end-to-end/api/01-users.js +++ b/apps/meteor/tests/end-to-end/api/01-users.js @@ -2616,7 +2616,7 @@ describe('[Users]', function () { .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error', 'unauthorized'); + expect(res.body).to.have.property('error', 'User does not have the permissions required for this action [error-unauthorized]'); }); }); @@ -2879,10 +2879,10 @@ describe('[Users]', function () { .get(api('users.getPersonalAccessTokens')) .set(credentials) .expect('Content-Type', 'application/json') - .expect(400) + .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); - expect(res.body.errorType).to.be.equal('not-authorized'); + expect(res.body.error).to.be.equal('User does not have the permissions required for this action [error-unauthorized]'); }) .end(done); }); @@ -3003,6 +3003,7 @@ describe('[Users]', function () { .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('error', 'User does not have the permissions required for this action [error-unauthorized]'); }) .end(done); }); @@ -3019,6 +3020,7 @@ describe('[Users]', function () { .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('error', 'User does not have the permissions required for this action [error-unauthorized]'); }) .end(done); }); @@ -3035,6 +3037,7 @@ describe('[Users]', function () { .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('error', 'User does not have the permissions required for this action [error-unauthorized]'); }) .end(done); }); @@ -3249,7 +3252,7 @@ describe('[Users]', function () { .expect(403) .expect((res) => { expect(res.body).to.have.property('success', false); - expect(res.body).to.have.property('error', 'unauthorized'); + expect(res.body).to.have.property('error', 'User does not have the permissions required for this action [error-unauthorized]'); }) .end(done); });