[MM-58492][MM-58523] Fixed some access control bugs around archived channels by replacing the permission check with HasPermissionToReadChannel #27409
Conversation
devinbinnie
added
2: Dev Review
mm-cloud-bot
added
the
release-note
|
Not triggering E2E tests: this PR has 4 commit status(es) or check-runs that are not passing. Ensure all statuses aside from the E2E testing ones are green, before triggering E2E tests. |
|
Creating a new SpinWick test server using Mattermost Cloud. |
|
Enterprise Edition Image not available in the 30 minutes timeframe, checking the Team Edition Image and if available will use that. |
|
Test server creation failed. Review the error details here. |
devinbinnie
added
Setup Cloud Test Server
|
Creating a new SpinWick test server using Mattermost Cloud. |
|
Mattermost test server created! 🎉 Access here: https://mattermost-pr-27409.test.mattermost.cloud
Your Spinwick's installation ID is: |
|
/e2e-tests |
|
E2E test triggered successfully for PR #27409. The corresponding commit's status check will be available shortly. |
|
E2E test run is starting for commit
|
server/channels/api4/preference.go
Outdated
| @@ -122,7 +122,13 @@ func updatePreferences(c *Context, w http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), post.ChannelId, model.PermissionReadChannelContent) { | |||
| channel, err := c.App.GetChannel(c.AppContext, post.ChannelId) | |||
There was a problem hiding this comment.
We may want to revisit this. You are getting a channel for each flagged post, which may have performance implications. A "not great but better" solution would be to keep a map of channels that you already checked whether you have permissions, so you don't have to do this several times for several flagged posts in the same channel.
Not sure what is the maximum number of flagged posts we can have, and maybe this is not an issue here because of that. 0/5
Nevertheless, please take another look to the changes to see if we are doing this in any loop that may have a similar performance concern.
There was a problem hiding this comment.
Yeah I guess we could add the channel mapping concept here as well to be safe, though I don't think there would be as much of a concern here vs. some of the other APIs since the number of flagged posts likely wouldn't be high, and we are already getting each post one by one here. I guess best not to make it worse :P
| @@ -271,6 +271,7 @@ func (a *App) UploadData(c request.CTX, us *model.UploadSession, rd io.Reader) ( | |||
| } | |||
|
|
|||
| info.CreatorId = us.UserId | |||
| info.ChannelId = us.ChannelId | |||
There was a problem hiding this comment.
I am pretty surprised we didn't assign this value before. Have you found if we assign it somewhere else or if there is any strong reason not to assign it?
There was a problem hiding this comment.
@larkox I think the reason is merely that at the time of implementation we didn't have a FileInfo.ChannelID. The mapping and permissions would happen indirectly through any attached post ids.
There was a problem hiding this comment.
As long as we don't think that this will break anything, this probably should be assigned here. I found it very odd that it was missing in the first place. I'm not sure where exact it was assigned elsewhere, but since we do provide the channel scope now, we should assign it here.
| c.Err = err | ||
| return | ||
| } | ||
| if !c.App.SessionHasPermissionToReadChannel(c.AppContext, *c.AppContext.Session(), channel) { |
There was a problem hiding this comment.
If I am not mistaken, from the implementation of HasPermissionToReadChannel, we check first the channel being archived, before checking whether the user is a sysadmin or not.
This means that now sysadmins won't be able to get the posts for an archived channel. Is that the desired behavior, or we have to rethink the HasPermissionToReadChannel function?
There was a problem hiding this comment.
Let's bring this up in the sync meeting.
There was a problem hiding this comment.
As a note, anywhere else we check for archived channels we don't check if they're a system admin, so I think it actually makes sense to deny if it's off.
Technically that is a system-wide configuration item so as I see it the proper way to configure access control would be to flip the global setting on and then control it separately per user group using the other permissions controls.
We don't have per-user group permissions to view, only to archive 😅
There was a problem hiding this comment.
Decided not to make an exception for system admins: https://hub.mattermost.com/private-core/pl/iopz1gkws7yjbqpqzq8k6kbacy
|
New commit detected. SpinWick will upgrade if the updated docker image is available. |
|
Mattermost test server updated with git commit Access here: https://mattermost-pr-27409.test.mattermost.cloud |
Summary
There are a lot of checks for permissions in the API using
SessionHasPermissionToChannel, which does not check for archived channels or compliance requirements. There is another call specifically around reading channel content (including posts, files, etc.) calledHasPermissionToReadChannelthat does account for this, but has not yet been widely implemented across the API.This PR addresses a lot of small access control issues caused by this broken permission check by using
HasPermissionToReadChannelinstead. A supplemental method that takes in the fullSessionobject was also added so that we can still check for local mode correctly. A few tests were also patched to work correctly.Note to QA: Definitely need to run the E2E tests against this to make sure we check that all cases around being able to access (or not access) archived channels are accounted for.
Ticket Link
https://mattermost.atlassian.net/browse/MM-58492
https://mattermost.atlassian.net/browse/MM-58523