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.

Sign up for GitHub

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

Jump to bottom

[MM-57988] Fix move thread logic to not block channel admins #27061

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[MM-57988] Fix move thread logic to not block channel admins #27061

wants to merge 6 commits into from
+97 −21

Conversation

MattSilvaa
Copy link
Contributor

Summary

This PR concats a user's roles with their channel roles so that we correctly check all current user roles against the permitted roles for move thread. Doing this allows for channel admins to be able to move threads.

For testing, I added a unit test in the api tests as well as doing some manual testing.

Ticket Link

Fixes #26959
Jira https://mattermost.atlassian.net/browse/MM-57988

Demo video

Screencast.from.2024-05-19.09-47-32.webm

Release Note

NONE

@mm-cloud-bot mm-cloud-bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 19, 2024
@mattermost-build
Copy link
Contributor

Hello @MattSilvaa,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@marianunez marianunez added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review and removed Lifecycle/1:stale labels May 31, 2024
@unified-ci-app
Copy link
Contributor

E2E test triggered successfully for PR #27061. The corresponding commit's status check will be available shortly.

@github-actions GitHub Actions
Copy link

E2E test run is starting for commit ed9499dd3e1f14e38a13d7210259bfe5f8039446.
You can check its progress by either:

@devinbinnie
Copy link
Member

/update-branch

nickmisasi
nickmisasi approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@nickmisasi nickmisasi left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @MattSilvaa!

@nickmisasi
Copy link
Contributor

@esarafianou is there someone from your team who'd be open to giving this change a quick review?

@nickmisasi nickmisasi added the 3: Security Review Review requested from Security Team label Jun 3, 2024
@esarafianou
Copy link
Contributor

@nickmisasi I'm good with the change. We'd ideally though have the authorization checks all in one place. Now we check for permitted roles in the API layer and then check for additional permissions on the app layer:

mattermost/server/channels/app/post.go

Line 2403 in 91741a7

func (a *App) ValidateMoveOrCopy(c request.CTX, wpl *model.WranglerPostList, originalChannel *model.Channel, targetChannel *model.Channel, user *model.User) error {

This makes it difficult to follow and audit that all permissions are correctly in place.

devinbinnie
devinbinnie requested changes Jun 7, 2024
View reviewed changes
Copy link
Member

@devinbinnie devinbinnie left a comment

Choose a reason for hiding this comment

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

LGTM other than one small request.
Thanks for the PR @MattSilvaa!

server/channels/api4/post.go Outdated
@@ -1267,3 +1272,19 @@ func getPostInfo(c *Context, w http.ResponseWriter, r *http.Request) {

w.Write(js)
}

func hasPermittedRole(c *Context, user *model.User, channelMember *model.ChannelMember) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Can we be a bit more specific on the naming here, since this is specific to Wrangler, something like:

Suggested change
func hasPermittedRole(c *Context, user *model.User, channelMember *model.ChannelMember) bool {
func hasPermittedWranglerRole(c *Context, user *model.User, channelMember *model.ChannelMember) bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Done.

server/channels/api4/post.go Outdated
}

userHasRole := hasPermittedRole(c, user, channelMember)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
userHasRole := hasPermittedRole(c, user, channelMember)
userHasRole := hasPermittedWranglerRole(c, user, channelMember)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nickmisasi
Copy link
Contributor

@esarafianou good call out for sure - this was brought up in initial review as well. The long term goal was initially intended to be to integrate with Mattermost's permission schemes rather than this sort of thing. There's currently no owner for this feature as its experimental and no team has bandwidth to pick it up, so your suggestions would hopefully be addressed in the future.

devinbinnie
devinbinnie approved these changes Jun 10, 2024
View reviewed changes
Copy link
Member

@devinbinnie devinbinnie left a comment

Choose a reason for hiding this comment

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

Thanks @MattSilvaa, this looks good now :)

@devinbinnie devinbinnie added Setup Cloud Test Server Setup an on-prem test server and removed 2: Dev Review Requires review by a developer labels Jun 10, 2024
@mm-cloud-bot
Copy link

Creating a new SpinWick test server using Mattermost Cloud.

@mm-cloud-bot
Copy link

Enterprise Edition Image not available in the 30 minutes timeframe, checking the Team Edition Image and if available will use that.

@yasserfaraazkhan yasserfaraazkhan removed the Setup Cloud Test Server Setup an on-prem test server label Jun 11, 2024
@yasserfaraazkhan
Copy link
Contributor

/update-branch

@yasserfaraazkhan yasserfaraazkhan added the Setup Cloud Test Server Setup an on-prem test server label Jun 11, 2024
@mm-cloud-bot
Copy link

Creating a new SpinWick test server using Mattermost Cloud.

@mm-cloud-bot
Copy link

Mattermost test server created! 🎉

Access here: https://mattermost-pr-27061.test.mattermost.cloud

Account Type Username Password
Admin sysadmin Sys@dmin123
User user-1 User-1@123

Your Spinwick's installation ID is: w9w1y6ygq7n1teejrnajo3ztrw
To access the logs, please click here

yasserfaraazkhan
yasserfaraazkhan approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a comment

Choose a reason for hiding this comment

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

Verified that when permitted role to move thread is set to Channel_admin,

the channel_admins get option to move thread. if User is demoted, the option is not shown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickmisasi nickmisasi nickmisasi approved these changes

@yasserfaraazkhan yasserfaraazkhan yasserfaraazkhan approved these changes

@devinbinnie devinbinnie devinbinnie approved these changes

Assignees
No one assigned
Labels
3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review 3: Security Review Review requested from Security Team Contributor release-note-none Denotes a PR that doesn't merit a release note. Setup Cloud Test Server Setup an on-prem test server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Thread move feature is incorrectly limited to system admins
8 participants
@MattSilvaa @mattermost-build @devinbinnie @nickmisasi @esarafianou @mm-cloud-bot @yasserfaraazkhan @marianunez