fix: Users without the "Manage OAuth Apps" permission can't log in wi…

…th third-party apps (#32986)
RocketChat · Aug 22, 2024 · 7f88158 · 7f88158
1 parent 150c7b1
commit 7f88158
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 44 deletions.
diff --git a/.changeset/cool-rocks-remember.md 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
@@ -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
@@ -13,12 +13,18 @@ export class OAuthAppsRaw extends BaseRaw<IOAuthApps> implements IOAuthAppsModel
 		return [{ key: { clientId: 1, clientSecret: 1 } }, { key: { appId: 1 } }];
 	}
 
-	findOneAuthAppByIdOrClientId(props: { clientId: string } | { appId: string } | { _id: string }): Promise<IOAuthApps | null> {
-		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<IOAuthApps>,
+	): Promise<IOAuthApps | null> {
+		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<IOAuthApps>): Promise<IOAuthApps | null> {

diff --git 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
@@ -11,6 +11,7 @@ export interface IOAuthAppsModel extends IBaseModel<IOAuthApps> {
 			| {
 					_id: string;
 			  },
+		options?: FindOptions<IOAuthApps>,
 	): Promise<IOAuthApps | null>;
 
 	findOneActiveByClientId(clientId: string, options?: FindOptions<IOAuthApps>): Promise<IOAuthApps | null>;