diff --git a/.changeset/cool-rocks-remember.md b/.changeset/cool-rocks-remember.md new file mode 100644 index 000000000000..97af36e94320 --- /dev/null +++ b/.changeset/cool-rocks-remember.md @@ -0,0 +1,6 @@ +--- +"@rocket.chat/meteor": patch +"@rocket.chat/model-typings": patch +--- + +Fixed login with third-party apps not working without the "Manage OAuth Apps" permission diff --git a/apps/meteor/app/api/server/v1/oauthapps.ts b/apps/meteor/app/api/server/v1/oauthapps.ts index 034a73f54104..4113b945a4db 100644 --- a/apps/meteor/app/api/server/v1/oauthapps.ts +++ b/apps/meteor/app/api/server/v1/oauthapps.ts @@ -27,11 +27,12 @@ API.v1.addRoute( { authRequired: true, validateParams: isOauthAppsGetParams }, { async get() { - if (!(await hasPermissionAsync(this.userId, 'manage-oauth-apps'))) { - return API.v1.unauthorized(); - } + const isOAuthAppsManager = await hasPermissionAsync(this.userId, 'manage-oauth-apps'); - const oauthApp = await OAuthApps.findOneAuthAppByIdOrClientId(this.queryParams); + const oauthApp = await OAuthApps.findOneAuthAppByIdOrClientId( + this.queryParams, + !isOAuthAppsManager ? { projection: { clientSecret: 0 } } : {}, + ); if (!oauthApp) { return API.v1.failure('OAuth app not found.'); diff --git a/apps/meteor/server/models/raw/OAuthApps.ts b/apps/meteor/server/models/raw/OAuthApps.ts index dc9ce4f95159..c8650a407f6e 100644 --- a/apps/meteor/server/models/raw/OAuthApps.ts +++ b/apps/meteor/server/models/raw/OAuthApps.ts @@ -13,12 +13,18 @@ export class OAuthAppsRaw extends BaseRaw implements IOAuthAppsModel return [{ key: { clientId: 1, clientSecret: 1 } }, { key: { appId: 1 } }]; } - findOneAuthAppByIdOrClientId(props: { clientId: string } | { appId: string } | { _id: string }): Promise { - return this.findOne({ - ...('_id' in props && { _id: props._id }), - ...('appId' in props && { _id: props.appId }), - ...('clientId' in props && { clientId: props.clientId }), - }); + findOneAuthAppByIdOrClientId( + props: { clientId: string } | { appId: string } | { _id: string }, + options?: FindOptions, + ): Promise { + return this.findOne( + { + ...('_id' in props && { _id: props._id }), + ...('appId' in props && { _id: props.appId }), + ...('clientId' in props && { clientId: props.clientId }), + }, + options, + ); } findOneActiveByClientId(clientId: string, options?: FindOptions): Promise { diff --git a/apps/meteor/tests/end-to-end/api/oauthapps.ts b/apps/meteor/tests/end-to-end/api/oauthapps.ts index 7bffa3297bfc..db714d1107bd 100644 --- a/apps/meteor/tests/end-to-end/api/oauthapps.ts +++ b/apps/meteor/tests/end-to-end/api/oauthapps.ts @@ -51,8 +51,11 @@ describe('[OAuthApps]', () => { }); describe('[/oauth-apps.get]', () => { - it('should return a single oauthApp by id', (done) => { - void request + before(() => updatePermission('manage-oauth-apps', ['admin'])); + after(() => updatePermission('manage-oauth-apps', ['admin'])); + + it('should return a single oauthApp by id', () => { + return request .get(api('oauth-apps.get')) .query({ appId: 'zapier' }) .set(credentials) @@ -61,11 +64,11 @@ describe('[OAuthApps]', () => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('oauthApp'); expect(res.body.oauthApp._id).to.be.equal('zapier'); - }) - .end(done); + expect(res.body.oauthApp).to.have.property('clientSecret'); + }); }); - it('should return a single oauthApp by client id', (done) => { - void request + it('should return a single oauthApp by client id', () => { + return request .get(api('oauth-apps.get')) .query({ clientId: 'zapier' }) .set(credentials) @@ -74,36 +77,49 @@ describe('[OAuthApps]', () => { expect(res.body).to.have.property('success', true); expect(res.body).to.have.property('oauthApp'); expect(res.body.oauthApp._id).to.be.equal('zapier'); - }) - .end(done); + expect(res.body.oauthApp).to.have.property('clientSecret'); + }); }); - 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 only non sensitive information if user does not have the permission to manage oauth apps when searching by clientId', async () => { + await updatePermission('manage-oauth-apps', []); + await request + .get(api('oauth-apps.get')) + .query({ clientId: 'zapier' }) + .set(credentials) + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('oauthApp'); + expect(res.body.oauthApp._id).to.be.equal('zapier'); + expect(res.body.oauthApp.clientId).to.be.equal('zapier'); + expect(res.body.oauthApp).to.not.have.property('clientSecret'); + }); }); - 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 only non sensitive information if user does not have the permission to manage oauth apps when searching by appId', async () => { + await updatePermission('manage-oauth-apps', []); + await request + .get(api('oauth-apps.get')) + .query({ appId: 'zapier' }) + .set(credentials) + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('oauthApp'); + expect(res.body.oauthApp._id).to.be.equal('zapier'); + expect(res.body.oauthApp.clientId).to.be.equal('zapier'); + expect(res.body.oauthApp).to.not.have.property('clientSecret'); + }); + }); + it('should fail returning an oauth app when an invalid id is provided (avoid NoSQL injections)', () => { + return request + .get(api('oauth-apps.get')) + .query({ _id: '{ "$ne": "" }' }) + .set(credentials) + .expect(400) + .expect((res) => { + expect(res.body).to.have.property('success', false); + expect(res.body).to.have.property('error', 'OAuth app not found.'); + }); }); }); diff --git a/packages/model-typings/src/models/IOAuthAppsModel.ts b/packages/model-typings/src/models/IOAuthAppsModel.ts index 8f21ba16d2b7..859c972e4597 100644 --- a/packages/model-typings/src/models/IOAuthAppsModel.ts +++ b/packages/model-typings/src/models/IOAuthAppsModel.ts @@ -11,6 +11,7 @@ export interface IOAuthAppsModel extends IBaseModel { | { _id: string; }, + options?: FindOptions, ): Promise; findOneActiveByClientId(clientId: string, options?: FindOptions): Promise;