fix: Unable to log in with third-party apps without "Manage OAuth Apps" permission #32782

verdel · 2024-07-13T10:49:31Z

Added a new REST API method oauth-apps.info that returns a limited set of attributes (clientId, name) necessary for rendering the consent screen

Proposed changes (including videos or screenshots)

The issue is detailed in my comment on the #31749 .

The attributes clientId and name of the OAuth application are used in the process of rendering the consent screen. The API method oauth-apps.get is used to obtain these attributes. This method also returns the clientSecret attribute.

To prevent regular users from obtaining this sensitive data, the user must have manage-oauth-apps permissions to call the method. Therefore, regular users cannot use the third-party login process.

I am adding a separate API method oauth-apps.info that only returns the clientId and name attributes needed in the process of rendering the consent screen.

Issue(s)

Closes #31749

CORE-473

changeset-bot · 2024-07-13T10:49:35Z

🦋 Changeset detected

Latest commit: 0edac8e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages

Name	Type
@rocket.chat/meteor	Patch
@rocket.chat/core-typings	Patch
@rocket.chat/rest-typings	Patch
@rocket.chat/apps	Patch
@rocket.chat/core-services	Patch
@rocket.chat/cron	Patch
@rocket.chat/fuselage-ui-kit	Patch
@rocket.chat/gazzodown	Patch
@rocket.chat/livechat	Patch
@rocket.chat/model-typings	Patch
@rocket.chat/ui-contexts	Patch
@rocket.chat/account-service	Patch
@rocket.chat/authorization-service	Patch
@rocket.chat/ddp-streamer	Patch
@rocket.chat/omnichannel-transcript	Patch
@rocket.chat/presence-service	Patch
@rocket.chat/queue-worker	Patch
@rocket.chat/stream-hub-service	Patch
@rocket.chat/api-client	Patch
@rocket.chat/ddp-client	Patch
@rocket.chat/license	Patch
@rocket.chat/omnichannel-services	Patch
@rocket.chat/pdf-worker	Patch
@rocket.chat/presence	Patch
rocketchat-services	Patch
@rocket.chat/uikit-playground	Patch
@rocket.chat/models	Patch
@rocket.chat/ui-avatar	Patch
@rocket.chat/ui-client	Patch
@rocket.chat/ui-video-conf	Patch
@rocket.chat/web-ui-registration	Patch
@rocket.chat/instance-status	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

CLAassistant · 2024-07-13T10:49:36Z

All committers have signed the CLA.

…Auth Apps permission Added a new REST API method oauth-apps.info that returns a limited set of attributes (clientId, name) necessary for rendering the consent screen Signed-off-by: Vadim Aleksandrov <[email protected]>

matheusbsilva137

Thanks for creating this PR! 🚀 I'll bring this for our team to check too

packages/rest-typings/src/v1/oauthapps/OAuthAppsInfoParamsGET.ts

apps/meteor/tests/end-to-end/api/oauthapps.ts

codecov · 2024-07-29T23:29:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.28%. Comparing base (851d60a) to head (ce358c0).

❗ There is a different number of reports uploaded between BASE (851d60a) and HEAD (ce358c0). Click for more details.

HEAD has 8 uploads less than BASE

Flag BASE (851d60a) HEAD (ce358c0)

e2e 10 2

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #32782      +/-   ##
===========================================
- Coverage    55.66%   48.28%   -7.39%     
===========================================
  Files         2637     2219     -418     
  Lines        57399    50179    -7220     
  Branches     11892    10268    -1624     
===========================================
- Hits         31954    24231    -7723     
- Misses       22731    23843    +1112     
+ Partials      2714     2105     -609

Flag	Coverage Δ
e2e	`38.28% <ø> (-16.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

…th-apps.get

verdel · 2024-07-30T09:17:36Z

@matheusbsilva137, I also fixed the type errors that were discovered after running the GitHub Workflow.

dionisio-bot · 2024-07-30T15:44:36Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label
This PR is targeting the wrong base branch. It should target 6.12.0, but it targets 6.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

KevLehman · 2024-07-30T18:07:31Z

apps/meteor/app/api/server/v1/oauthapps.ts

+	{ authRequired: true, validateParams: isOauthAppsInfoParams },
+	{
+		async get() {
+			const oauthApp = await OAuthApps.findOneAuthAppByIdOrClientId(this.queryParams);


Add a projection since you're returning just clientId/name

(this will imply updating the model typings for allowing the projection to go as the current method doesn't allow it)

I added projection to the findOneAuthAppByIdOrClientId method of the IOAuthAppsModel and OAuthAppsRaw models. I hope I changed the code everywhere it was needed and didn't forget anything.

apps/meteor/client/views/oauth/hooks/useOAuthAppQuery.ts

apps/meteor/tests/end-to-end/api/oauthapps.ts

KevLehman · 2024-07-30T18:11:18Z

packages/core-typings/src/IOAuthApps.ts

+export interface IOAuthAppsInfo {
+	clientId: string;
 	name: string;
+}


Personal pref: do we need this to be an interface? Can't we just export type OauthAppsInfo = Pick<IOAuthApps, 'clientId' | 'name'> ?

I agree, that will be better.

Also added a check for the case when the OAuthApp is not found

…ppsModel

apps/meteor/tests/end-to-end/api/oauthapps.ts

…fo api method

…ttier warning

verdel · 2024-08-02T16:49:13Z

@matheusbsilva137, sorry for the inconvenience, but I added a commit to fix the Prettier warning that appeared during code linting in the GitHub workflow. The tests need to be run again.

matheusbsilva137 · 2024-08-05T20:53:06Z

Hey @verdel !
I brought the solution proposed by this PR for the consideration of our team (internally) and we considered that a new endpoint to fix this specific issue may be too much since the current endpoint can be adapted to return only proper info based on user permission. Due to this decision, I created #32986, which we'll use to fix this issue
Thanks for contributing and for bringing this issue to us. Please check #32986 to be aware of the next steps regarding this fix (we'll push this so that this fix can be included in 6.12)

verdel · 2024-08-05T21:29:08Z

@matheusbsilva137, I'm sorry that I initially went a bit off track. In my comment on the issue, I mentioned two possible ways to solve the problem: modifying the oauth-apps.get method or adding a new method.

@scuciatto's comment eventually led me to choose the second option. And I still lean towards the idea that using a separate method (which returns the minimal necessary number of OAuth App attributes) within the OAuth2 Authorization Flow, regardless of user permissions, is a more correct approach from a security perspective, even though it might be less correct from an API method semantics standpoint.

In any case, if the core product development team has decided that the solution proposed in your PR is more appropriate, then I can't influence that decision. At least I was able to help in some way with the final implementation of the idea. Moreover, your solution addresses the original problem mentioned in the issue.

Thank you and everyone who reviewed this PR for your time.

verdel requested review from a team as code owners July 13, 2024 10:49

verdel force-pushed the fix-oauth-flow-for-non-privileged-user branch from 7f9cc44 to 918fe12 Compare July 14, 2024 21:32

verdel force-pushed the fix-oauth-flow-for-non-privileged-user branch from 918fe12 to e87d8d5 Compare July 14, 2024 21:34

scuciatto assigned matheusbsilva137 Jul 29, 2024

matheusbsilva137 requested changes Jul 29, 2024

View reviewed changes

packages/rest-typings/src/v1/oauthapps/OAuthAppsInfoParamsGET.ts Outdated Show resolved Hide resolved

apps/meteor/tests/end-to-end/api/oauthapps.ts Outdated Show resolved Hide resolved

Merge branch 'develop' into fix-oauth-flow-for-non-privileged-user

cfcd0d2

verdel added 2 commits July 30, 2024 12:09

fix: resolve review comments in the PR

f2aafc8

fix: resolve type errors that occurred when adding the API method oau…

ce358c0

…th-apps.get

verdel force-pushed the fix-oauth-flow-for-non-privileged-user branch from 474fa7d to ce358c0 Compare July 30, 2024 09:09

verdel requested a review from matheusbsilva137 July 30, 2024 09:17

matheusbsilva137 previously approved these changes Jul 30, 2024

View reviewed changes

matheusbsilva137 added this to the 6.12 milestone Jul 30, 2024

matheusbsilva137 changed the title ~~fix: Restore ability to log in with third-party apps without Manage OAuth Apps permission~~ fix: Unable to log in with third-party apps without "Manage OAuth Apps" permission Jul 30, 2024

Create changeset

ae7e1c9

matheusbsilva137 dismissed their stale review via ae7e1c9 July 30, 2024 15:52

Merge branch 'develop' into fix-oauth-flow-for-non-privileged-user

c1c2168

matheusbsilva137 previously approved these changes Jul 30, 2024

View reviewed changes

KevLehman requested changes Jul 30, 2024

View reviewed changes

fix: move from IOAuthAppsInfo interface to OAuthAppsInfo type

ba2c5e9

verdel dismissed matheusbsilva137’s stale review via f2127a5 July 30, 2024 20:22

verdel requested a review from KevLehman July 30, 2024 21:47

fix: remove manage-oauth-apps permission in oauth-apps.info e2e

5a3401d

Also added a check for the case when the OAuthApp is not found

fix: add projection to findOneAuthAppByIdOrClientId method in IOAuthA…

c3348d7

…ppsModel

verdel force-pushed the fix-oauth-flow-for-non-privileged-user branch from 18dbc0f to c3348d7 Compare July 30, 2024 21:51

MarcosSpessatto previously approved these changes Jul 31, 2024

View reviewed changes

MarcosSpessatto requested a review from matheusbsilva137 July 31, 2024 11:53

matheusbsilva137 requested changes Aug 1, 2024

View reviewed changes

apps/meteor/tests/end-to-end/api/oauthapps.ts Show resolved Hide resolved

apps/meteor/tests/end-to-end/api/oauthapps.ts Outdated Show resolved Hide resolved

apps/meteor/tests/end-to-end/api/oauthapps.ts Outdated Show resolved Hide resolved

fix: use hooks to update permissions in the e2e for the oauth-apps.in…

81ce26a

…fo api method

verdel dismissed MarcosSpessatto’s stale review via 81ce26a August 2, 2024 11:38

verdel requested a review from matheusbsilva137 August 2, 2024 11:40

matheusbsilva137 and others added 3 commits August 2, 2024 10:42

Merge branch 'develop' into fix-oauth-flow-for-non-privileged-user

fefdf76

fix: move OAuthAppsInfo type to a separate file

98f6a8f

fix: format the parameters of findOneAuthAppByIdOrClientId to fix Pre…

0edac8e

…ttier warning

verdel requested a review from MarcosSpessatto August 2, 2024 19:33

matheusbsilva137 closed this Aug 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Unable to log in with third-party apps without "Manage OAuth Apps" permission #32782

fix: Unable to log in with third-party apps without "Manage OAuth Apps" permission #32782

verdel commented Jul 13, 2024 •

edited by dougfabris

Loading

changeset-bot bot commented Jul 13, 2024 •

edited

Loading

CLAassistant commented Jul 13, 2024 •

edited

Loading

matheusbsilva137 left a comment

codecov bot commented Jul 29, 2024 •

edited

Loading

verdel commented Jul 30, 2024

dionisio-bot bot commented Jul 30, 2024

KevLehman Jul 30, 2024

KevLehman Jul 30, 2024

verdel Jul 30, 2024

KevLehman Jul 30, 2024

verdel Jul 30, 2024

verdel commented Aug 2, 2024

matheusbsilva137 commented Aug 5, 2024

verdel commented Aug 5, 2024

fix: Unable to log in with third-party apps without "Manage OAuth Apps" permission #32782

fix: Unable to log in with third-party apps without "Manage OAuth Apps" permission #32782

Conversation

verdel commented Jul 13, 2024 • edited by dougfabris Loading

Proposed changes (including videos or screenshots)

Issue(s)

changeset-bot bot commented Jul 13, 2024 • edited Loading

🦋 Changeset detected

CLAassistant commented Jul 13, 2024 • edited Loading

matheusbsilva137 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2024 • edited Loading

Codecov Report

verdel commented Jul 30, 2024

dionisio-bot bot commented Jul 30, 2024

KevLehman Jul 30, 2024

Choose a reason for hiding this comment

KevLehman Jul 30, 2024

Choose a reason for hiding this comment

verdel Jul 30, 2024

Choose a reason for hiding this comment

KevLehman Jul 30, 2024

Choose a reason for hiding this comment

verdel Jul 30, 2024

Choose a reason for hiding this comment

verdel commented Aug 2, 2024

matheusbsilva137 commented Aug 5, 2024

verdel commented Aug 5, 2024

verdel commented Jul 13, 2024 •

edited by dougfabris

Loading

changeset-bot bot commented Jul 13, 2024 •

edited

Loading

CLAassistant commented Jul 13, 2024 •

edited

Loading

codecov bot commented Jul 29, 2024 •

edited

Loading