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.

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

feat!: Add permissions to create rooms in teams #31117

Merged
merged 30 commits into from
Oct 17, 2024

Conversation

matheusbsilva137
Copy link
Member

@matheusbsilva137 matheusbsilva137 commented Nov 30, 2023

Proposed changes (including videos or screenshots)

  • Add create-team-channel and create-team-group permissions to provide more control for the creation of rooms inside teams (the new permissions are checked within the team's main room scope);
  • Update client to use the new permissions within the team's main room scope;
  • Fix client to use existing team rooms' editing permissions within the team's main room scope (since auto-join and "remove team channel" options were being incorrectly displayed to room owners that are not team owners/moderators).

Issue(s)

Steps to test or reproduce

Further comments

This is a BREAKING CHANGE
SUP-356

Copy link

changeset-bot bot commented Nov 30, 2023

🦋 Changeset detected

Latest commit: 5e395a5

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

This PR includes changesets to release 35 packages
Name Type
@rocket.chat/meteor Major
@rocket.chat/core-typings Major
@rocket.chat/rest-typings Major
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Major
@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/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/network-broker Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-video-conf Major
@rocket.chat/ui-voip Major
@rocket.chat/web-ui-registration Major
@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

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (release-7.0.0@286e1e0). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##             release-7.0.0   #31117   +/-   ##
================================================
  Coverage                 ?   74.79%           
================================================
  Files                    ?      467           
  Lines                    ?    20641           
  Branches                 ?     5262           
================================================
  Hits                     ?    15439           
  Misses                   ?     4590           
  Partials                 ?      612           
Flag Coverage Δ
unit 74.79% <ø> (?)

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

@matheusbsilva137 matheusbsilva137 added this to the 7.0 milestone Dec 5, 2023
@matheusbsilva137 matheusbsilva137 marked this pull request as ready for review December 5, 2023 12:53
@matheusbsilva137 matheusbsilva137 requested review from a team as code owners December 5, 2023 12:53
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc left a comment

Choose a reason for hiding this comment

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

One "problem" with this feature is that team owners will be able to create rooms inside teams, but there won't be any way for them to delete those rooms unless they have the global delete permission. Ideally anyone who can create a room should be able to also delete it afterwards.

But in this case we can't simply add a delete-team-channel permission either, because team owners would then be able to delete any global room by simply adding it to their team first.

I don't know what the ideal solution is, but I believe it's just a matter of time until we get people requesting a delete-team-channel permission after this one is merged.

Copy link
Member Author

@matheusbsilva137 matheusbsilva137 left a comment

Choose a reason for hiding this comment

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

@pierre-lehnen-rc with the PR as it is right now, users who have the new permission to add channels/groups would only be able to delete the channels they own (since we check the delete-c and delete-p permissions in the deleted room scope).

I thought of two ways these two new delete-team-channel / delete-team-group permissions could behave if we introduce them:

  • They could "override" the global permissions, so that anyone that has this permission in a team would be able to delete ANY room in it (even the ones they don't own);
  • They could complement the permission to delete a room. So a user would only be able to delete a room that is part of a team if they own it (delete-c/delete-p permissions, which are currently checked within the deleted room's scope) and are also allowed to delete it in the team.

I'll check what's the behavior we'd like to have in the product. Maybe we should also make this clear somewhere if we decide to implement it.

guijun13
guijun13 previously approved these changes Dec 13, 2023
@casalsgh casalsgh requested a review from a team December 20, 2023 14:02
apps/meteor/app/lib/server/methods/createChannel.ts Outdated Show resolved Hide resolved
apps/meteor/app/lib/server/methods/createPrivateGroup.ts Outdated Show resolved Hide resolved
apps/meteor/packages/rocketchat-i18n/i18n/pl.i18n.json Outdated Show resolved Hide resolved
apps/meteor/packages/rocketchat-i18n/i18n/ru.i18n.json Outdated Show resolved Hide resolved
apps/meteor/packages/rocketchat-i18n/i18n/sv.i18n.json Outdated Show resolved Hide resolved
apps/meteor/server/startup/migrations/v305.ts Outdated Show resolved Hide resolved
apps/meteor/server/startup/migrations/v305.ts Outdated Show resolved Hide resolved
apps/meteor/tests/end-to-end/api/02-channels.js Outdated Show resolved Hide resolved
MarcosSpessatto
MarcosSpessatto previously approved these changes Jan 3, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jan 9, 2024
@kodiakhq kodiakhq bot removed the stat: ready to merge PR tested and approved waiting for merge label Jan 15, 2024
Copy link
Contributor

kodiakhq bot commented Jan 15, 2024

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

@matheusbsilva137 matheusbsilva137 added the do not merge Prevent a PR from being automatically merged label Jan 31, 2024
@matheusbsilva137 matheusbsilva137 force-pushed the new/teams-room-creation-permissions branch from 31902b7 to 5e395a5 Compare October 16, 2024 18:58
@ggazzo ggazzo merged commit bc3a19b into release-7.0.0 Oct 17, 2024
50 checks passed
@ggazzo ggazzo deleted the new/teams-room-creation-permissions branch October 17, 2024 00:30
This was referenced Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad: team-collab stat: QA assured Means it has been tested and approved by a company insider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants