Skip to content

Commit

Permalink
chore!: Improve permissions check on oauth-apps endpoints (#32338)
Browse files Browse the repository at this point in the history 
Co-authored-by: Marcos Spessatto Defendi <[email protected]>
  • Loading branch information
@matheusbsilva137 @MarcosSpessatto @ggazzo
2 people authored and ggazzo committed Jul 16, 2024
1 parent b28591d commit a196874
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 67 deletions.
28 changes: 5 additions & 23 deletions apps/meteor/app/api/server/v1/oauthapps.ts
View file
Original file line number Diff line number Diff line change
@@ -1,20 +1,15 @@
import { OAuthApps } from '@rocket.chat/models';
import { isUpdateOAuthAppParams, isOauthAppsGetParams, isOauthAppsAddParams, isDeleteOAuthAppParams } from '@rocket.chat/rest-typings';

import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission';
import { apiDeprecationLogger } from '../../../lib/server/lib/deprecationWarningLogger';
import { addOAuthApp } from '../../../oauth2-server-config/server/admin/functions/addOAuthApp';
import { API } from '../api';

API.v1.addRoute(
'oauth-apps.list',
{ authRequired: true },
{ authRequired: true, permissionsRequired: ['manage-oauth-apps'] },
{
async get() {
if (!(await hasPermissionAsync(this.userId, 'manage-oauth-apps'))) {
throw new Error('error-not-allowed');
}

return API.v1.success({
oauthApps: await OAuthApps.find().toArray(),
});
Expand All @@ -24,13 +19,9 @@ API.v1.addRoute(

API.v1.addRoute(
'oauth-apps.get',
{ authRequired: true, validateParams: isOauthAppsGetParams },
{ authRequired: true, validateParams: isOauthAppsGetParams, permissionsRequired: ['manage-oauth-apps'] },
{
async get() {
if (!(await hasPermissionAsync(this.userId, 'manage-oauth-apps'))) {
return API.v1.unauthorized();
}

const oauthApp = await OAuthApps.findOneAuthAppByIdOrClientId(this.queryParams);

if (!oauthApp) {
Expand All @@ -53,13 +44,10 @@ API.v1.addRoute(
{
authRequired: true,
validateParams: isUpdateOAuthAppParams,
permissionsRequired: ['manage-oauth-apps'],
},
{
async post() {
if (!(await hasPermissionAsync(this.userId, 'manage-oauth-apps'))) {
return API.v1.unauthorized();
}

const { appId } = this.bodyParams;

const result = await Meteor.callAsync('updateOAuthApp', appId, this.bodyParams);
Expand All @@ -74,13 +62,10 @@ API.v1.addRoute(
{
authRequired: true,
validateParams: isDeleteOAuthAppParams,
permissionsRequired: ['manage-oauth-apps'],
},
{
async post() {
if (!(await hasPermissionAsync(this.userId, 'manage-oauth-apps'))) {
return API.v1.unauthorized();
}

const { appId } = this.bodyParams;

const result = await Meteor.callAsync('deleteOAuthApp', appId);
Expand All @@ -95,13 +80,10 @@ API.v1.addRoute(
{
authRequired: true,
validateParams: isOauthAppsAddParams,
permissionsRequired: ['manage-oauth-apps'],
},
{
async post() {
if (!(await hasPermissionAsync(this.userId, 'manage-oauth-apps'))) {
return API.v1.unauthorized();
}

const application = await addOAuthApp(this.bodyParams, this.userId);

return API.v1.success({ application });
Expand Down
131 changes: 87 additions & 44 deletions apps/meteor/tests/end-to-end/api/oauthapps.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ describe('[OAuthApps]', () => {
void request
.get(api('oauth-apps.list'))
.set(credentials)
.expect(400)
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('error-not-allowed');
expect(res.body.error).to.be.equal('User does not have the permissions required for this action [error-unauthorized]');
})
.end(done);
});
Expand Down Expand Up @@ -77,33 +77,29 @@ describe('[OAuthApps]', () => {
})
.end(done);
});
it('should return a 403 Forbidden error when the user does not have the necessary permission by client id', (done) => {
void updatePermission('manage-oauth-apps', []).then(() => {
void request
.get(api('oauth-apps.get'))
.query({ clientId: 'zapier' })
.set(credentials)
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('unauthorized');
})
.end(done);
});
it('should return a 403 Forbidden error when the user does not have the necessary permission by client id', async () => {
await updatePermission('manage-oauth-apps', []);
await request
.get(api('oauth-apps.get'))
.query({ clientId: 'zapier' })
.set(credentials)
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('User does not have the permissions required for this action [error-unauthorized]');
});
});
it('should return a 403 Forbidden error when the user does not have the necessary permission by app id', (done) => {
void updatePermission('manage-oauth-apps', []).then(() => {
void request
.get(api('oauth-apps.get'))
.query({ appId: 'zapier' })
.set(credentials)
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('unauthorized');
})
.end(done);
});
it('should return a 403 Forbidden error when the user does not have the necessary permission by app id', async () => {
await updatePermission('manage-oauth-apps', []);
await request
.get(api('oauth-apps.get'))
.query({ appId: 'zapier' })
.set(credentials)
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('User does not have the permissions required for this action [error-unauthorized]');
});
});
});

Expand All @@ -120,7 +116,11 @@ describe('[OAuthApps]', () => {
active: false,
})
.expect('Content-Type', 'application/json')
.expect(403);
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('User does not have the permissions required for this action [error-unauthorized]');
});
await updatePermission('manage-oauth-apps', ['admin']);
});

Expand Down Expand Up @@ -203,11 +203,12 @@ describe('[OAuthApps]', () => {
describe('[/oauth-apps.update]', () => {
let appId: IOAuthApps['_id'];

before((done) => {
before(async () => {
await updatePermission('manage-oauth-apps', ['admin']);
const name = 'test-oauth-app';
const redirectUri = 'https://test.com';
const active = true;
void request
const res = await request
.post(api('oauth-apps.create'))
.set(credentials)
.send({
Expand All @@ -216,12 +217,13 @@ describe('[OAuthApps]', () => {
active,
})
.expect('Content-Type', 'application/json')
.expect(200)
.end((_err, res) => {
appId = res.body.application._id;
createdAppsIds.push(appId);
done();
});
.expect(200);
appId = res.body.application._id;
createdAppsIds.push(appId);
});

after(async () => {
await updatePermission('manage-oauth-apps', ['admin']);
});

it("should update an app's name, its Active and Redirect URI fields correctly by its id", async () => {
Expand All @@ -247,16 +249,40 @@ describe('[OAuthApps]', () => {
expect(res.body).to.have.property('name', name);
});
});

it('should fail updating an app if user does NOT have the manage-oauth-apps permission', async () => {
const name = `new app ${Date.now()}`;
const redirectUri = 'http://localhost:3000';
const active = false;

await updatePermission('manage-oauth-apps', []);
await request
.post(api(`oauth-apps.update`))
.set(credentials)
.send({
appId,
name,
redirectUri,
active,
})
.expect('Content-Type', 'application/json')
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('User does not have the permissions required for this action [error-unauthorized]');
});
});
});

describe('[/oauth-apps.delete]', () => {
let appId: IOAuthApps['_id'];

before((done) => {
before(async () => {
await updatePermission('manage-oauth-apps', ['admin']);
const name = 'test-oauth-app';
const redirectUri = 'https://test.com';
const active = true;
void request
const res = await request
.post(api('oauth-apps.create'))
.set(credentials)
.send({
Expand All @@ -265,11 +291,12 @@ describe('[OAuthApps]', () => {
active,
})
.expect('Content-Type', 'application/json')
.expect(200)
.end((_err, res) => {
appId = res.body.application._id;
done();
});
.expect(200);
appId = res.body.application._id;
});

after(async () => {
await updatePermission('manage-oauth-apps', ['admin']);
});

it('should delete an app by its id', async () => {
Expand All @@ -285,5 +312,21 @@ describe('[OAuthApps]', () => {
expect(res.body).to.equals(true);
});
});

it('should fail deleting an app by its id if user does NOT have the manage-oauth-apps permission', async () => {
await updatePermission('manage-oauth-apps', []);
await request
.post(api(`oauth-apps.delete`))
.set(credentials)
.send({
appId,
})
.expect('Content-Type', 'application/json')
.expect(403)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body.error).to.be.equal('User does not have the permissions required for this action [error-unauthorized]');
});
});
});
});

0 comments on commit a196874

Please sign in to comment.