Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Users without the "Manage OAuth Apps" permission can't log in with third-party apps #32986

Merged
merged 8 commits into from
Aug 22, 2024
Merged

fix: Users without the "Manage OAuth Apps" permission can't log in with third-party apps #32986

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e6392f4
fix: allow returning public oauth-apps info to non privileged users
matheusbsilva137 Aug 5, 2024
f747df2
tests: update end-to-end tests
matheusbsilva137 Aug 5, 2024
cd1aa64
Create cool-rocks-remember.md
matheusbsilva137 Aug 5, 2024
4de1096
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
matheusbsilva137 Aug 13, 2024
bd92e58
tests: add test case to assure nosql injections aren't possible
matheusbsilva137 Aug 13, 2024
7c8b9aa
Update apps/meteor/tests/end-to-end/api/oauthapps.ts
matheusbsilva137 Aug 14, 2024
a8ee197
Merge branch 'develop' into fix/oauth-apps-not-working
kodiakhq[bot] Aug 21, 2024
5154ee1
Merge branch 'develop' into fix/oauth-apps-not-working
kodiakhq[bot] Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/cool-rocks-remember.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
9 changes: 5 additions & 4 deletions apps/meteor/app/api/server/v1/oauthapps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
matheusbsilva137 marked this conversation as resolved.
Show resolved Hide resolved
!isOAuthAppsManager ? { projection: { clientSecret: 0 } } : {},
);

if (!oauthApp) {
return API.v1.failure('OAuth app not found.');
Expand Down
18 changes: 12 additions & 6 deletions apps/meteor/server/models/raw/OAuthApps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
73 changes: 39 additions & 34 deletions apps/meteor/tests/end-to-end/api/oauthapps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -74,36 +77,38 @@ 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 () => {
KevLehman marked this conversation as resolved.
Show resolved Hide resolved
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 clientId', async () => {
matheusbsilva137 marked this conversation as resolved.
Show resolved Hide resolved
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');
});
});
});

Expand Down
1 change: 1 addition & 0 deletions packages/model-typings/src/models/IOAuthAppsModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
Loading