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

[IMPROVE] Permissions check per endpoint/method

Open KevLehman opened this issue 1 year ago • 11 comments

Note: most files changed are conversions to use the implemented permissions param :eyes: so don't worry to see 40 files.

The idea:

  • There are some routes that have multiple handlers per HTTP method. What happens in those routes is that we need to check for permissions on each one of the handlers, which is a bit of boilerplate, and increases the number of code branches. With this change, endpoints will be able to say which permissions apply to each one of the methods, as well as the "operation" that they want the permissions to be.
  • For example, an endpoint could be accessed by any user with 'view-l-room' or 'view-m-room'. With the current implementation, that cannot be done. With the new changes, devs can pass a operation: hasAll | hasAny prop to signal how permissions should be evaluated, if inclusively or exclusively.

:)

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

KevLehman avatar Jul 29 '22 19:07 KevLehman

This pull request introduces 1 alert when merging 3ccbcc22be07223aee8503615f01aa309237b43d into 133aa101d732e18c5accede20b7d5c31a6bd1ffa - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 02 '22 13:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 99f892e2ca35a7c908f6cb8402345cfe2a39e51a into 133aa101d732e18c5accede20b7d5c31a6bd1ffa - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 02 '22 14:08 lgtm-com[bot]

This pull request introduces 1 alert when merging c621332777c5483089289e88993e6b18488f944a into f5558892d479213facc494cc82d93e92c6b5707d - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 02 '22 15:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 22244252722c63dac099210c69d80f2783a5b58c into f5558892d479213facc494cc82d93e92c6b5707d - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 02 '22 16:08 lgtm-com[bot]

Codecov Report

Merging #26419 (4172286) into develop (d326414) will increase coverage by 0.02%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #26419      +/-   ##
===========================================
+ Coverage    38.46%   38.49%   +0.02%     
===========================================
  Files          794      794              
  Lines        19002    19002              
  Branches      1937     1937              
===========================================
+ Hits          7310     7315       +5     
+ Misses       11400    11395       -5     
  Partials       292      292              
Flag Coverage Δ
e2e 38.49% <ø> (+0.02%) :arrow_up:

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

codecov[bot] avatar Aug 02 '22 16:08 codecov[bot]

This pull request introduces 1 alert when merging f93d50a6f74c72cde46fc680f9b028151efe77f8 into 0343424459f376b2c523688113d62e4dd67abb40 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 02 '22 18:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 272ce1b751d117b98c845a83e489647d1aae3a70 into 0343424459f376b2c523688113d62e4dd67abb40 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 02 '22 18:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 7ddd888c38b8ab1775ac69ff82c37cefb56c1aeb into 0343424459f376b2c523688113d62e4dd67abb40 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 02 '22 19:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 62fc70af2c0293b98584389ece16ba14ee21242c into 2ebd4636ad861cc9210ec9ac6bc9ef42c22597aa - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 09 '22 14:08 lgtm-com[bot]

This pull request introduces 1 alert when merging b025cc95c3077087aef04d286c2a4d9b2ae87299 into 37d8f59d552755890b3506e973060f509304bfb6 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 09 '22 14:08 lgtm-com[bot]

This pull request introduces 1 alert when merging 4caff1eca01f8e43707e7f09408d8f319e4f6d89 into 37d8f59d552755890b3506e973060f509304bfb6 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

lgtm-com[bot] avatar Aug 09 '22 15:08 lgtm-com[bot]

Hey @KevLehman This looks really good :tada: I've left just a few suggestions over here https://github.com/RocketChat/Rocket.Chat/pull/26552 - they are mostly removing some permission checks which aren't required now & addressing the issue I mentioned above

murtaza98 avatar Aug 12 '22 07:08 murtaza98

@sampaiodiego @murtaza98 , you'll have to re-approve this one since there's a conflict :cry:

KevLehman avatar Aug 16 '22 16:08 KevLehman

btw, can we skip QA on this one? :eyes:

KevLehman avatar Aug 16 '22 16:08 KevLehman

since the endpoints changed are all omnichannel, I'll let you guys decide if you want to skip QA.

sampaiodiego avatar Aug 16 '22 16:08 sampaiodiego