Skip to content

Conversation

@KevLehman
Copy link
Member

@KevLehman KevLehman commented Jul 29, 2022

Note: most files changed are conversions to use the implemented permissions param 👀 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 KevLehman changed the title Allow to check permissions per method and specify if all or any are r… [IMPROVE] Permissions check per endpoint/method Jul 29, 2022
@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

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

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

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

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 1 alert when merging c621332 into f555889 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 1 alert when merging 2224425 into f555889 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@codecov
Copy link

codecov bot commented Aug 2, 2022

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%) ⬆️

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

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 1 alert when merging f93d50a into 0343424 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 1 alert when merging 272ce1b into 0343424 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 1 alert when merging 7ddd888 into 0343424 - view on LGTM.com

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2022

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

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2022

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

new alerts:

  • 1 for Superfluous trailing arguments

@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2022

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

new alerts:

  • 1 for Superfluous trailing arguments

@KevLehman KevLehman added this to the 5.1.0 milestone Aug 19, 2022
murtaza98
murtaza98 previously approved these changes Aug 22, 2022
sampaiodiego
sampaiodiego previously approved these changes Aug 22, 2022
@KevLehman KevLehman dismissed stale reviews from sampaiodiego and murtaza98 via e5893b1 August 22, 2022 15:03
@KevLehman KevLehman requested review from murtaza98 and sampaiodiego and removed request for murtaza98 August 22, 2022 20:07
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR ended up adding a lot of other omnichannel changes, right? should the title be changed?

@ggazzo ggazzo added stat: QA skipped stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Aug 23, 2022
@kodiakhq kodiakhq bot merged commit 86797fb into develop Aug 23, 2022
@kodiakhq kodiakhq bot deleted the chore/permissions-on-api branch August 23, 2022 21:18
gabriellsh added a commit that referenced this pull request Aug 24, 2022
…ove/otr-message

* 'develop' of github.com:RocketChat/Rocket.Chat:
  Regression: Add alsoSendThreadToChannel to user settings api (#26663)
  [IMPROVE] Spotlight search user results (#26599)
  [FIX] Slack User CSV importer not working (#26629)
  Chore: Importer rest types, meteor methods to TS and API unit tests (#26284)
  [NEW] Adding oauth crud on the rocket.chat side (#26220)
  [NEW] allow ephemeral messages to receive a specific id (#26118)
  [FIX] MDM content alignment (#26665)
  Chore: Permissions check per endpoint/method (#26419)
  Regression: CI (#26658)
  [FIX] Not allowed error in discussion room with a private parent channel (#26394)
  Chore: Fix grammatical typo when only one message is pruned (#21902)
  [FIX] Agents (with user status offline & omni-status as available) not able to take or forward chat (#26575)
  i18n: Language update from LingoHub 🤖 on 2022-08-22Z (#26645)
  Chore: Add license env var to ee tests (#26650)
  Chore: Move `Card` and related components to `@rocket.chat/ui-client` (#26653)
  Regression: Custom status loading forever in Usercard (#26656)
  [FIX] Current Chat Custom Field Filter (#26200)
  Chore: Migrate modules related to `room` template to TypeScript (#25881)
  Chore: Create teams management tests (#26578)
csuadev pushed a commit that referenced this pull request Aug 26, 2022
Co-authored-by: Murtaza Patrawala <34130764+murtaza98@users.noreply.github.com>
@murtaza98 murtaza98 mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants