Rocket.Chat icon indicating copy to clipboard operation
Rocket.Chat copied to clipboard

fix: login with third-party apps wasn't working without "Manage Oauth Apps"-permission

Open mbreslein-thd opened this issue 1 year ago • 2 comments

This change was made in https://github.com/RocketChat/Rocket.Chat/commit/5bb039037015d7d4cd3dcd255ac89e63dbe5c84c and broke third-party apps. (see #31749)

Proposed changes (including videos or screenshots)

Make third-party apps work again without giving users permission to edit/add them.

Issue(s)

#31749

Steps to test or reproduce

Further comments

mbreslein-thd avatar Feb 16 '24 09:02 mbreslein-thd

🦋 Changeset detected

Latest commit: d980e18867b0a07a43ddef97b6e9c011c80adac6

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/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client 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

changeset-bot[bot] avatar Feb 16 '24 09:02 changeset-bot[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 54.60%. Comparing base (1ac48ae) to head (4430b45). Report is 1 commits behind head on develop.

:exclamation: Current head 4430b45 differs from pull request most recent head d980e18

Please upload reports for the commit d980e18 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #31771      +/-   ##
===========================================
- Coverage    56.32%   54.60%   -1.72%     
===========================================
  Files         2434     2279     -155     
  Lines        53708    50208    -3500     
  Branches     11057    10240     -817     
===========================================
- Hits         30251    27417    -2834     
+ Misses       20812    20310     -502     
+ Partials      2645     2481     -164     
Flag Coverage Δ
e2e 53.56% <ø> (-2.44%) :arrow_down:
e2e-api 40.07% <ø> (-1.27%) :arrow_down:
unit 76.44% <ø> (+4.23%) :arrow_up:

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

codecov[bot] avatar Feb 16 '24 09:02 codecov[bot]

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

  • This PR has conflicts, please resolve them before merging
  • This PR is missing the 'stat: QA assured' label
  • This PR is not mergeable
  • This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

dionisio-bot[bot] avatar Apr 08 '24 09:04 dionisio-bot[bot]

Hi @sampaiodiego , when can this fix be merged?

AndrewChau avatar May 29 '24 01:05 AndrewChau

is it possible to add some tests validating that the login with third party apps is working even without the 'manage-oauth-apps' permission?

Gustrb avatar May 29 '24 12:05 Gustrb

is it possible to add some tests validating that the login with third party apps is working even without the 'manage-oauth-apps' permission?

It would probably be a good idea to add that check where the old one was: https://github.com/RocketChat/Rocket.Chat/pull/31771/files#diff-54d2e43cb32ca66b463865e1af3547e2b1d5fdf9460c9c2153725b10e962167eL78

I don't have any experience with the used testing-software, so it's probably better if someone else does that.

mbreslein-thd avatar May 29 '24 12:05 mbreslein-thd

is it possible to add some tests validating that the login with third party apps is working even without the 'manage-oauth-apps' permission?

It would probably be a good idea to add that check where the old one was: https://github.com/RocketChat/Rocket.Chat/pull/31771/files#diff-54d2e43cb32ca66b463865e1af3547e2b1d5fdf9460c9c2153725b10e962167eL78

I don't have any experience with the used testing-software, so it's probably better if someone else does that.

About the testing, I took a closer look, and, altough I think we should add a testing flow to validate the login is working properly, it would take a lot of effort and I believe we don't even have the testing capabilities to test that now. So I think we can just add the permission to fetch OAuth apps and keep the old tests (just changing the permission name)

So I believe we can skip that for this PR

Gustrb avatar May 29 '24 13:05 Gustrb

Just pushed the new permission to the branch, please update the tests :P

Gustrb avatar May 29 '24 14:05 Gustrb

Just pushed the new permission to the branch, please update the tests :P

Thank you.

Done: https://github.com/RocketChat/Rocket.Chat/pull/31771/commits/ffd96c382f3aacb3f9b8b5ff8da35b8c976a8343

mbreslein-thd avatar May 29 '24 14:05 mbreslein-thd

Hey @mbreslein-thd ! I brought the solution proposed by this PR for the consideration of our team (internally) and we considered that we must have a restriction regarding the info returned by the endpoint, since some of the info returned by it are sensitive. 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)

matheusbsilva137 avatar Aug 05 '24 20:08 matheusbsilva137