-
Notifications
You must be signed in to change notification settings - Fork 7k
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
[MM-58492][MM-58523] Fixed some access control bugs around archived channels by replacing the permission check with HasPermissionToReadChannel #27409
base: master
Are you sure you want to change the base?
Changes from all commits
d9a6cc0
477c1c1
49ed008
32907e2
d3b3ac8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -257,15 +257,20 @@ func getPostsForChannel(c *Context, w http.ResponseWriter, r *http.Request) { | |
return | ||
} | ||
|
||
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channelId, model.PermissionReadChannelContent) { | ||
channel, err := c.App.GetChannel(c.AppContext, channelId) | ||
if err != nil { | ||
c.Err = err | ||
return | ||
} | ||
if !c.App.SessionHasPermissionToReadChannel(c.AppContext, *c.AppContext.Session(), channel) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I am not mistaken, from the implementation of 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's bring this up in the sync meeting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
We don't have per-user group permissions to view, only to archive 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided not to make an exception for system admins: https://hub.mattermost.com/private-core/pl/iopz1gkws7yjbqpqzq8k6kbacy |
||
c.SetPermissionError(model.PermissionReadChannelContent) | ||
return | ||
} | ||
devinbinnie marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if !*c.App.Config().TeamSettings.ExperimentalViewArchivedChannels { | ||
channel, err := c.App.GetChannel(c.AppContext, channelId) | ||
if err != nil { | ||
c.Err = err | ||
channel, appErr := c.App.GetChannel(c.AppContext, channelId) | ||
if appErr != nil { | ||
c.Err = appErr | ||
return | ||
} | ||
if channel.DeleteAt != 0 { | ||
|
@@ -275,7 +280,6 @@ func getPostsForChannel(c *Context, w http.ResponseWriter, r *http.Request) { | |
} | ||
|
||
var list *model.PostList | ||
var err *model.AppError | ||
etag := "" | ||
|
||
if since > 0 { | ||
|
@@ -341,7 +345,12 @@ func getPostsForChannelAroundLastUnread(c *Context, w http.ResponseWriter, r *ht | |
} | ||
|
||
channelId := c.Params.ChannelId | ||
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channelId, model.PermissionReadChannelContent) { | ||
channel, err := c.App.GetChannel(c.AppContext, channelId) | ||
if err != nil { | ||
c.Err = err | ||
Comment on lines
+348
to
+350
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and in a few other places I'd recommend to use |
||
return | ||
} | ||
if !c.App.SessionHasPermissionToReadChannel(c.AppContext, *c.AppContext.Session(), channel) { | ||
c.SetPermissionError(model.PermissionReadChannelContent) | ||
return | ||
} | ||
|
@@ -423,6 +432,20 @@ func getFlaggedPostsForUser(c *Context, w http.ResponseWriter, r *http.Request) | |
return | ||
} | ||
|
||
channelMap := make(map[string]*model.Channel) | ||
channelIds := []string{} | ||
for _, post := range posts.Posts { | ||
channelIds = append(channelIds, post.ChannelId) | ||
} | ||
channels, err := c.App.GetChannels(c.AppContext, channelIds) | ||
if err != nil { | ||
c.Err = err | ||
return | ||
} | ||
for _, channel := range channels { | ||
channelMap[channel.Id] = channel | ||
} | ||
|
||
pl := model.NewPostList() | ||
channelReadPermission := make(map[string]bool) | ||
|
||
|
@@ -432,7 +455,11 @@ func getFlaggedPostsForUser(c *Context, w http.ResponseWriter, r *http.Request) | |
if !ok { | ||
allowed = false | ||
|
||
if c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), post.ChannelId, model.PermissionReadChannelContent) { | ||
channel, ok := channelMap[post.ChannelId] | ||
if !ok { | ||
continue | ||
} | ||
if c.App.SessionHasPermissionToReadChannel(c.AppContext, *c.AppContext.Session(), channel) { | ||
allowed = true | ||
} | ||
|
||
|
@@ -523,23 +550,28 @@ func getPostsByIds(c *Context, w http.ResponseWriter, r *http.Request) { | |
return | ||
} | ||
|
||
var posts = []*model.Post{} | ||
channelMap := make(map[string]*model.Channel) | ||
channelIds := []string{} | ||
for _, post := range postsList { | ||
channelIds = append(channelIds, post.ChannelId) | ||
} | ||
channels, appErr := c.App.GetChannels(c.AppContext, channelIds) | ||
if appErr != nil { | ||
c.Err = appErr | ||
return | ||
} | ||
for _, channel := range channels { | ||
channelMap[channel.Id] = channel | ||
} | ||
|
||
var posts = []*model.Post{} | ||
for _, post := range postsList { | ||
var channel *model.Channel | ||
if val, ok := channelMap[post.ChannelId]; ok { | ||
channel = val | ||
} else { | ||
channel, appErr = c.App.GetChannel(c.AppContext, post.ChannelId) | ||
if appErr != nil { | ||
c.Err = appErr | ||
return | ||
} | ||
channelMap[channel.Id] = channel | ||
channel, ok := channelMap[post.ChannelId] | ||
if !ok { | ||
continue | ||
} | ||
|
||
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), channel.Id, model.PermissionReadChannelContent) { | ||
if !c.App.SessionHasPermissionToReadChannel(c.AppContext, *c.AppContext.Session(), channel) { | ||
if channel.Type != model.ChannelTypeOpen || (channel.Type == model.ChannelTypeOpen && !c.App.SessionHasPermissionToTeam(*c.AppContext.Session(), channel.TeamId, model.PermissionReadPublicChannel)) { | ||
continue | ||
} | ||
|
@@ -1031,7 +1063,12 @@ func saveIsPinnedPost(c *Context, w http.ResponseWriter, isPinned bool) { | |
auditRec.AddEventPriorState(post) | ||
auditRec.AddEventObjectType("post") | ||
|
||
if !c.App.SessionHasPermissionToChannel(c.AppContext, *c.AppContext.Session(), post.ChannelId, model.PermissionReadChannelContent) { | ||
channel, err := c.App.GetChannel(c.AppContext, post.ChannelId) | ||
if err != nil { | ||
c.Err = err | ||
return | ||
} | ||
if !c.App.SessionHasPermissionToReadChannel(c.AppContext, *c.AppContext.Session(), channel) { | ||
c.SetPermissionError(model.PermissionReadChannelContent) | ||
return | ||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I'd prefer if we added a new test case rather than changing an existing one.