Skip to content

Commit

Permalink
Add parsing and validation for old filenames migration (#12827)
Browse files Browse the repository at this point in the history 
(cherry picked from commit 3c06fc2)
  • Loading branch information
@streamer45
streamer45 committed Oct 19, 2019
1 parent 2ad85bf commit 243500a
Show file tree
Hide file tree
Showing 2 changed files with 165 additions and 48 deletions.
78 changes: 49 additions & 29 deletions app/file.go
View file
Expand Up @@ -18,6 +18,7 @@ import (
"net/http"
"net/url"
"path/filepath"
"regexp"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -123,29 +124,21 @@ func (a *App) RemoveFile(path string) *model.AppError {
return backend.RemoveFile(path)
}

func (a *App) GetInfoForFilename(post *model.Post, teamId string, filename string) *model.FileInfo {
// Find the path from the Filename of the form /{channelId}/{userId}/{uid}/{nameWithExtension}
split := strings.SplitN(filename, "/", 5)
if len(split) < 5 {
mlog.Error("Unable to decipher filename when migrating post to use FileInfos", mlog.String("post_id", post.Id), mlog.String("filename", filename))
return nil
func (a *App) ListDirectory(path string) ([]string, *model.AppError) {
backend, err := a.FileBackend()
if err != nil {
return nil, err
}

channelId := split[1]
userId := split[2]
oldId := split[3]
name, _ := url.QueryUnescape(split[4])

if split[0] != "" || split[1] != post.ChannelId || split[2] != post.UserId || strings.Contains(split[4], "/") {
mlog.Warn(
"Found an unusual filename when migrating post to use FileInfos",
mlog.String("post_id", post.Id),
mlog.String("channel_id", post.ChannelId),
mlog.String("user_id", post.UserId),
mlog.String("filename", filename),
)
paths, err := backend.ListDirectory(path)
if err != nil {
return nil, err
}

return *paths, nil
}

func (a *App) getInfoForFilename(post *model.Post, teamId, channelId, userId, oldId, filename string) *model.FileInfo {
name, _ := url.QueryUnescape(filename)
pathPrefix := fmt.Sprintf("teams/%s/channels/%s/users/%s/%s/", teamId, channelId, userId, oldId)
path := pathPrefix + name

Expand Down Expand Up @@ -187,10 +180,8 @@ func (a *App) GetInfoForFilename(post *model.Post, teamId string, filename strin
return info
}

func (a *App) FindTeamIdForFilename(post *model.Post, filename string) string {
split := strings.SplitN(filename, "/", 5)
id := split[3]
name, _ := url.QueryUnescape(split[4])
func (a *App) findTeamIdForFilename(post *model.Post, id, filename string) string {
name, _ := url.QueryUnescape(filename)

// This post is in a direct channel so we need to figure out what team the files are stored under.
result := <-a.Srv.Store.Team().GetTeamsByUserId(post.UserId)
Expand All @@ -207,7 +198,7 @@ func (a *App) FindTeamIdForFilename(post *model.Post, filename string) string {

for _, team := range teams {
path := fmt.Sprintf("teams/%s/channels/%s/users/%s/%s/%s", team.Id, post.ChannelId, post.UserId, id, name)
if _, err := a.ReadFile(path); err == nil {
if ok, err := a.FileExists(path); ok && err == nil {
// Found the team that this file was posted from
return team.Id
}
Expand All @@ -217,6 +208,27 @@ func (a *App) FindTeamIdForFilename(post *model.Post, filename string) string {
}

var fileMigrationLock sync.Mutex
var oldFilenameMatchExp *regexp.Regexp = regexp.MustCompile(`^\/([a-z\d]{26})\/([a-z\d]{26})\/([a-z\d]{26})\/([^\/]+)$`)

// Parse the path from the Filename of the form /{channelId}/{userId}/{uid}/{nameWithExtension}
func parseOldFilenames(filenames []string, channelId, userId string) [][]string {
parsed := [][]string{}
for _, filename := range filenames {
matches := oldFilenameMatchExp.FindStringSubmatch(filename)
if len(matches) != 5 {
mlog.Error("Failed to parse old Filename", mlog.String("filename", filename))
continue
}
if matches[1] != channelId {
mlog.Error("ChannelId in Filename does not match", mlog.String("channel_id", channelId), mlog.String("matched", matches[1]))
} else if matches[2] != userId {
mlog.Error("UserId in Filename does not match", mlog.String("user_id", userId), mlog.String("matched", matches[2]))
} else {
parsed = append(parsed, matches[1:])
}
}
return parsed
}

// Creates and stores FileInfos for a post created before the FileInfos table existed.
func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo {
Expand All @@ -241,11 +253,19 @@ func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo {
}
channel := result.Data.(*model.Channel)

// Parse and validate filenames before further processing
parsedFilenames := parseOldFilenames(filenames, post.ChannelId, post.UserId)

if len(parsedFilenames) == 0 {
mlog.Error("Unable to parse filenames")
return []*model.FileInfo{}
}

// Find the team that was used to make this post since its part of the file path that isn't saved in the Filename
var teamId string
if channel.TeamId == "" {
// This post was made in a cross-team DM channel so we need to find where its files were saved
teamId = a.FindTeamIdForFilename(post, filenames[0])
// This post was made in a cross-team DM channel, so we need to find where its files were saved
teamId = a.findTeamIdForFilename(post, parsedFilenames[0][2], parsedFilenames[0][3])
} else {
teamId = channel.TeamId
}
Expand All @@ -258,8 +278,8 @@ func (a *App) MigrateFilenamesToFileInfos(post *model.Post) []*model.FileInfo {
mlog.String("post_id", post.Id),
)
} else {
for _, filename := range filenames {
info := a.GetInfoForFilename(post, teamId, filename)
for _, parsed := range parsedFilenames {
info := a.getInfoForFilename(post, teamId, parsed[0], parsed[1], parsed[2], parsed[3])
if info == nil {
continue
}
Expand Down
135 changes: 116 additions & 19 deletions app/file_test.go
View file
Expand Up @@ -51,18 +51,14 @@ func TestDoUploadFile(t *testing.T) {
data := []byte("abcd")

info1, err := th.App.DoUploadFile(time.Date(2007, 2, 4, 1, 2, 3, 4, time.Local), teamId, channelId, userId, filename, data)
if err != nil {
t.Fatal(err)
} else {
defer func() {
<-th.App.Srv.Store.FileInfo().PermanentDelete(info1.Id)
th.App.RemoveFile(info1.Path)
}()
}
require.Nil(t, err, "DoUploadFile should succeed with valid data")
defer func() {
th.App.Srv.Store.FileInfo().PermanentDelete(info1.Id)
th.App.RemoveFile(info1.Path)
}()

if info1.Path != fmt.Sprintf("20070204/teams/%v/channels/%v/users/%v/%v/%v", teamId, channelId, userId, info1.Id, filename) {
t.Fatal("stored file at incorrect path", info1.Path)
}
value := fmt.Sprintf("20070204/teams/%v/channels/%v/users/%v/%v/%v", teamId, channelId, userId, info1.Id, filename)
assert.Equal(t, value, info1.Path, "stored file at incorrect path")

info2, err := th.App.DoUploadFile(time.Date(2007, 2, 4, 1, 2, 3, 4, time.Local), teamId, channelId, userId, filename, data)
if err != nil {
Expand Down Expand Up @@ -131,31 +127,125 @@ func TestUploadFile(t *testing.T) {
}
}

func TestParseOldFilenames(t *testing.T) {
th := Setup().InitBasic()
defer th.TearDown()

fileId := model.NewId()

tests := []struct {
description string
filenames []string
channelId string
userId string
expected [][]string
}{
{
description: "Empty input should result in empty output",
filenames: []string{},
channelId: th.BasicChannel.Id,
userId: th.BasicUser.Id,
expected: [][]string{},
},
{
description: "Filename with invalid format should not parse",
filenames: []string{"/path/to/some/file.png"},
channelId: th.BasicChannel.Id,
userId: th.BasicUser.Id,
expected: [][]string{},
},
{
description: "ChannelId in Filename should not match",
filenames: []string{
fmt.Sprintf("/%v/%v/%v/file.png", model.NewId(), th.BasicUser.Id, fileId),
},
channelId: th.BasicChannel.Id,
userId: th.BasicUser.Id,
expected: [][]string{},
},
{
description: "UserId in Filename should not match",
filenames: []string{
fmt.Sprintf("/%v/%v/%v/file.png", th.BasicChannel.Id, model.NewId(), fileId),
},
channelId: th.BasicChannel.Id,
userId: th.BasicUser.Id,
expected: [][]string{},
},
{
description: "../ in filename should not parse",
filenames: []string{
fmt.Sprintf("/%v/%v/%v/../../../file.png", th.BasicChannel.Id, th.BasicUser.Id, fileId),
},
channelId: th.BasicChannel.Id,
userId: th.BasicUser.Id,
expected: [][]string{},
},
{
description: "Should only parse valid filenames",
filenames: []string{
fmt.Sprintf("/%v/%v/%v/../otherfile.png", th.BasicChannel.Id, th.BasicUser.Id, fileId),
fmt.Sprintf("/%v/%v/%v/file.png", th.BasicChannel.Id, th.BasicUser.Id, fileId),
},
channelId: th.BasicChannel.Id,
userId: th.BasicUser.Id,
expected: [][]string{
{
th.BasicChannel.Id,
th.BasicUser.Id,
fileId,
"file.png",
},
},
},
{
description: "Valid Filename should parse",
filenames: []string{
fmt.Sprintf("/%v/%v/%v/file.png", th.BasicChannel.Id, th.BasicUser.Id, fileId),
},
channelId: th.BasicChannel.Id,
userId: th.BasicUser.Id,
expected: [][]string{
{
th.BasicChannel.Id,
th.BasicUser.Id,
fileId,
"file.png",
},
},
},
}

for _, test := range tests {
t.Run(test.description, func(tt *testing.T) {
result := parseOldFilenames(test.filenames, test.channelId, test.userId)
require.Equal(tt, result, test.expected)
})
}
}

func TestGetInfoForFilename(t *testing.T) {
th := Setup().InitBasic()
defer th.TearDown()

post := th.BasicPost
teamId := th.BasicTeam.Id

info := th.App.GetInfoForFilename(post, teamId, "sometestfile")
assert.Nil(t, info, "Test bad filename")

info = th.App.GetInfoForFilename(post, teamId, "/somechannel/someuser/someid/somefile.png")
info := th.App.getInfoForFilename(post, teamId, post.ChannelId, post.UserId, "someid", "somefile.png")
assert.Nil(t, info, "Test non-existent file")
}

func TestFindTeamIdForFilename(t *testing.T) {
th := Setup().InitBasic()
defer th.TearDown()

teamId := th.App.FindTeamIdForFilename(th.BasicPost, fmt.Sprintf("/%v/%v/%v/blargh.png", th.BasicChannel.Id, th.BasicUser.Id, "someid"))
teamId := th.App.findTeamIdForFilename(th.BasicPost, "someid", "somefile.png")
assert.Equal(t, th.BasicTeam.Id, teamId)

_, err := th.App.CreateTeamWithUser(&model.Team{Email: th.BasicUser.Email, Name: "zz" + model.NewId(), DisplayName: "Joram's Test Team", Type: model.TEAM_OPEN}, th.BasicUser.Id)
require.Nil(t, err)

teamId = th.App.FindTeamIdForFilename(th.BasicPost, fmt.Sprintf("/%v/%v/%v/blargh.png", th.BasicChannel.Id, th.BasicUser.Id, "someid"))
teamId = th.App.findTeamIdForFilename(th.BasicPost, "someid", "somefile.png")
assert.Equal(t, "", teamId)
}

Expand All @@ -176,14 +266,21 @@ func TestMigrateFilenamesToFileInfos(t *testing.T) {
require.Nil(t, fileErr)
defer file.Close()

fpath := fmt.Sprintf("/teams/%v/channels/%v/users/%v/%v/test.png", th.BasicTeam.Id, th.BasicChannel.Id, th.BasicUser.Id, "someid")
fileId := model.NewId()
fpath := fmt.Sprintf("/teams/%v/channels/%v/users/%v/%v/test.png", th.BasicTeam.Id, th.BasicChannel.Id, th.BasicUser.Id, fileId)
_, err := th.App.WriteFile(file, fpath)
require.Nil(t, err)
rpost, err := th.App.CreatePost(&model.Post{UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, Filenames: []string{fmt.Sprintf("/%v/%v/%v/test.png", th.BasicChannel.Id, th.BasicUser.Id, "someid")}}, th.BasicChannel, false)
rpost, err := th.App.CreatePost(&model.Post{UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, Filenames: []string{fmt.Sprintf("/%v/%v/%v/test.png", th.BasicChannel.Id, th.BasicUser.Id, fileId)}}, th.BasicChannel, false)
require.Nil(t, err)

infos = th.App.MigrateFilenamesToFileInfos(rpost)
assert.Equal(t, 1, len(infos))

rpost, err = th.App.CreatePost(&model.Post{UserId: th.BasicUser.Id, ChannelId: th.BasicChannel.Id, Filenames: []string{fmt.Sprintf("/%v/%v/%v/../../test.png", th.BasicChannel.Id, th.BasicUser.Id, fileId)}}, th.BasicChannel, false)
require.Nil(t, err)

infos = th.App.MigrateFilenamesToFileInfos(rpost)
assert.Equal(t, 0, len(infos))
}

func TestCopyFileInfos(t *testing.T) {
Expand Down

0 comments on commit 243500a

Please sign in to comment.