MM-11868/MM-12010/MM-12011/MM-12036 Improve post metadata structure (#9693)

* MM-11868/MM-12010/MM-12011/MM-12036 Include dimensions for opengraph and message attachment images in post metadata

* Remove duplicate check from getFirstLinkAndImages

* Add tests for getImagesInMessageAttachments

* Rename PostMetadata.FileInfos to PostMetadata.Files

* Rename Metadata.ImageDimensions to Metadata.Images

app/post_metadata.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 
 	"github.com/dyatlov/go-opengraph/opengraph"
+	"github.com/mattermost/mattermost-server/mlog"
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/utils"
 	"github.com/mattermost/mattermost-server/utils/markdown"
@@ -51,106 +52,134 @@ func (a *App) PreparePostListForClient(originalList *model.PostList) (*model.Pos
 func (a *App) PreparePostForClient(originalPost *model.Post) (*model.Post, *model.AppError) {
 	post := originalPost.Clone()
 
-	needReactionCounts := post.ReactionCounts == nil
-	needEmojis := post.Emojis == nil
-	needOpenGraphData := post.OpenGraphData == nil
-	needImageDimensions := post.ImageDimensions == nil
-
-	// Get reactions to post
-	var reactions []*model.Reaction
-	if needReactionCounts || needEmojis {
-		var err *model.AppError
-		reactions, err = a.GetReactionsForPost(post.Id)
-		if err != nil {
-			return post, err
-		}
-	}
-
-	if needReactionCounts {
-		post.ReactionCounts = model.CountReactions(reactions)
-	}
-
-	// Get emojis for post
-	if needEmojis {
-		emojis, err := a.getCustomEmojisForPost(post.Message, reactions)
-		if err != nil {
-			return post, err
-		}
-
-		post.Emojis = emojis
-	}
-
-	// Get files for post
-	if post.FileInfos == nil {
-		fileInfos, err := a.GetFileInfosForPost(post.Id, false)
-		if err != nil {
-			return post, err
-		}
-
-		post.FileInfos = fileInfos
-	}
-
-	// Proxy image links in post
+	// Proxy image links before constructing metadata so that requests go through the proxy
 	post = a.PostWithProxyAddedToImageURLs(post)
 
-	// Get OpenGraph and image metadata
-	if needOpenGraphData || needImageDimensions {
-		err := a.preparePostWithOpenGraphAndImageMetadata(post, needOpenGraphData, needImageDimensions)
-		if err != nil {
-			return post, err
+	if post.Metadata == nil {
+		post.Metadata = &model.PostMetadata{}
+
+		// Emojis and reaction counts
+		if emojis, reactionCounts, err := a.getEmojisAndReactionCountsForPost(post); err != nil {
+			mlog.Warn("Failed to get emojis and reactions for a post", mlog.String("post_id", post.Id), mlog.Any("err", err))
+		} else {
+			post.Metadata.Emojis = emojis
+			post.Metadata.ReactionCounts = reactionCounts
 		}
+
+		// Files
+		if fileInfos, err := a.GetFileInfosForPost(post.Id, false); err != nil {
+			mlog.Warn("Failed to get files for a post", mlog.String("post_id", post.Id), mlog.Any("err", err))
+		} else {
+			post.Metadata.Files = fileInfos
+		}
+
+		// Embeds and image dimensions
+		firstLink, images := getFirstLinkAndImages(post.Message)
+
+		if embed, err := a.getEmbedForPost(post, firstLink); err != nil {
+			mlog.Warn("Failed to get embedded content for a post", mlog.String("post_id", post.Id), mlog.Any("err", err))
+		} else if embed == nil {
+			post.Metadata.Embeds = []*model.PostEmbed{}
+		} else {
+			post.Metadata.Embeds = []*model.PostEmbed{embed}
+		}
+
+		post.Metadata.Images = a.getImagesForPost(post, images)
 	}
 
 	return post, nil
 }
 
-func (a *App) preparePostWithOpenGraphAndImageMetadata(post *model.Post, needOpenGraphData, needImageDimensions bool) *model.AppError {
-	var appError *model.AppError
-
-	if needOpenGraphData {
-		post.OpenGraphData = []*opengraph.OpenGraph{}
+func (a *App) getEmojisAndReactionCountsForPost(post *model.Post) ([]*model.Emoji, model.ReactionCounts, *model.AppError) {
+	reactions, err := a.GetReactionsForPost(post.Id)
+	if err != nil {
+		return nil, nil, err
 	}
 
-	if needImageDimensions {
-		post.ImageDimensions = []*model.PostImageDimensions{}
+	emojis, err := a.getCustomEmojisForPost(post.Message, reactions)
+	if err != nil {
+		return nil, nil, err
 	}
 
-	firstLink, images := getFirstLinkAndImages(post.Message)
+	return emojis, model.CountReactions(reactions), nil
+}
+
+func (a *App) getEmbedForPost(post *model.Post, firstLink string) (*model.PostEmbed, error) {
+	if _, ok := post.Props["attachments"]; ok {
+		return &model.PostEmbed{
+			Type: model.POST_EMBED_MESSAGE_ATTACHMENT,
+		}, nil
+	}
 
-	// Look at the first link to see if it's a web page or an image
 	if firstLink != "" {
-		og, dimensions, err := a.getLinkMetadata(firstLink, true)
+		og, image, err := a.getLinkMetadata(firstLink, true)
 		if err != nil {
-			// Keep going so that one bad link doesn't prevent other image dimensions from being sent to the client
-			appError = model.NewAppError("PreparePostForClient", "app.post.metadata.link.app_error", nil, err.Error(), http.StatusInternalServerError)
+			return nil, err
 		}
 
-		if needOpenGraphData {
-			post.OpenGraphData = append(post.OpenGraphData, og)
+		if og != nil {
+			return &model.PostEmbed{
+				Type: model.POST_EMBED_OPENGRAPH,
+				URL:  firstLink,
+				Data: og,
+			}, nil
 		}
 
-		if needImageDimensions {
-			post.ImageDimensions = append(post.ImageDimensions, dimensions)
+		if image != nil {
+			// Note that we're not passing the image info here since they'll be part of the PostMetadata.Images field
+			return &model.PostEmbed{
+				Type: model.POST_EMBED_IMAGE,
+				URL:  firstLink,
+			}, nil
 		}
 	}
 
-	if needImageDimensions {
-		// And dimensions for other images
-		for _, image := range images {
-			_, dimensions, err := a.getLinkMetadata(image, true)
-			if err != nil {
-				// Keep going so that one bad link doesn't prevent other image dimensions from being sent to the client
-				appError = model.NewAppError("PreparePostForClient", "app.post.metadata.link.app_error", nil, err.Error(), http.StatusInternalServerError)
-				continue
-			}
+	return nil, nil
+}
 
-			if dimensions != nil {
-				post.ImageDimensions = append(post.ImageDimensions, dimensions)
+func (a *App) getImagesForPost(post *model.Post, imageURLs []string) map[string]*model.PostImage {
+	images := map[string]*model.PostImage{}
+
+	for _, embed := range post.Metadata.Embeds {
+		switch embed.Type {
+		case model.POST_EMBED_IMAGE:
+			// These dimensions will generally be cached by a previous call to getEmbedForPost
+			imageURLs = append(imageURLs, embed.URL)
+
+		case model.POST_EMBED_MESSAGE_ATTACHMENT:
+			imageURLs = append(imageURLs, getImagesInMessageAttachments(post)...)
+
+		case model.POST_EMBED_OPENGRAPH:
+			for _, image := range embed.Data.(*opengraph.OpenGraph).Images {
+				if image.Width != 0 || image.Height != 0 {
+					// The site has already told us the image dimensions
+					images[image.URL] = &model.PostImage{
+						Width:  int(image.Width),
+						Height: int(image.Height),
+					}
+				} else {
+					// The site did not specify its image dimensions
+					imageURLs = append(imageURLs, image.URL)
+				}
 			}
 		}
 	}
 
-	return appError
+	// Removing duplicates isn't strictly since images is a map, but it feels safer to do it beforehand
+	if len(imageURLs) > 1 {
+		imageURLs = model.RemoveDuplicateStrings(imageURLs)
+	}
+
+	for _, imageURL := range imageURLs {
+		if _, image, err := a.getLinkMetadata(imageURL, true); err != nil {
+			mlog.Warn("Failed to get dimensions of an image in a post",
+				mlog.String("post_id", post.Id), mlog.String("image_url", imageURL), mlog.Any("err", err))
+		} else {
+			images[imageURL] = image
+		}
+	}
+
+	return images
 }
 
 func (a *App) getCustomEmojisForPost(message string, reactions []*model.Reaction) ([]*model.Emoji, *model.AppError) {
@@ -200,20 +229,37 @@ func getFirstLinkAndImages(str string) (string, []string) {
 		return true
 	})
 
-	if len(images) > 1 {
-		images = model.RemoveDuplicateStrings(images)
-	}
-
 	return firstLink, images
 }
 
-func (a *App) getLinkMetadata(requestURL string, useCache bool) (*opengraph.OpenGraph, *model.PostImageDimensions, error) {
+func getImagesInMessageAttachments(post *model.Post) []string {
+	var images []string
+
+	for _, attachment := range post.Attachments() {
+		_, imagesInText := getFirstLinkAndImages(attachment.Text)
+		images = append(images, imagesInText...)
+
+		_, imagesInPretext := getFirstLinkAndImages(attachment.Pretext)
+		images = append(images, imagesInPretext...)
+
+		for _, field := range attachment.Fields {
+			if value, ok := field.Value.(string); ok {
+				_, imagesInFieldValue := getFirstLinkAndImages(value)
+				images = append(images, imagesInFieldValue...)
+			}
+		}
+	}
+
+	return images
+}
+
+func (a *App) getLinkMetadata(requestURL string, useCache bool) (*opengraph.OpenGraph, *model.PostImage, error) {
 	// Check cache
 	if useCache {
-		og, dimensions, ok := getLinkMetadataFromCache(requestURL)
+		og, image, ok := getLinkMetadataFromCache(requestURL)
 
 		if ok {
-			return og, dimensions, nil
+			return og, image, nil
 		}
 	}
 
@@ -232,17 +278,17 @@ func (a *App) getLinkMetadata(requestURL string, useCache bool) (*opengraph.Open
 	defer consumeAndClose(res)
 
 	// Parse the data
-	og, dimensions, err := a.parseLinkMetadata(requestURL, res.Body, res.Header.Get("Content-Type"))
+	og, image, err := a.parseLinkMetadata(requestURL, res.Body, res.Header.Get("Content-Type"))
 
 	// Write back to cache
 	if useCache {
-		cacheLinkMetadata(requestURL, og, dimensions)
+		cacheLinkMetadata(requestURL, og, image)
 	}
 
-	return og, dimensions, err
+	return og, image, err
 }
 
-func getLinkMetadataFromCache(requestURL string) (*opengraph.OpenGraph, *model.PostImageDimensions, bool) {
+func getLinkMetadataFromCache(requestURL string) (*opengraph.OpenGraph, *model.PostImage, bool) {
 	cached, ok := linkCache.Get(requestURL)
 	if !ok {
 		return nil, nil, false
@@ -251,28 +297,28 @@ func getLinkMetadataFromCache(requestURL string) (*opengraph.OpenGraph, *model.P
 	switch v := cached.(type) {
 	case *opengraph.OpenGraph:
 		return v, nil, true
-	case *model.PostImageDimensions:
+	case *model.PostImage:
 		return nil, v, true
 	default:
 		return nil, nil, true
 	}
 }
 
-func cacheLinkMetadata(requestURL string, og *opengraph.OpenGraph, dimensions *model.PostImageDimensions) {
+func cacheLinkMetadata(requestURL string, og *opengraph.OpenGraph, image *model.PostImage) {
 	var val interface{}
 	if og != nil {
 		val = og
-	} else if dimensions != nil {
-		val = dimensions
+	} else if image != nil {
+		val = image
 	}
 
 	linkCache.AddWithExpiresInSecs(requestURL, val, LINK_CACHE_DURATION)
 }
 
-func (a *App) parseLinkMetadata(requestURL string, body io.Reader, contentType string) (*opengraph.OpenGraph, *model.PostImageDimensions, error) {
+func (a *App) parseLinkMetadata(requestURL string, body io.Reader, contentType string) (*opengraph.OpenGraph, *model.PostImage, error) {
 	if strings.HasPrefix(contentType, "image") {
-		dimensions, err := parseImageDimensions(requestURL, body)
-		return nil, dimensions, err
+		image, err := parseImages(body)
+		return nil, image, err
 	} else if strings.HasPrefix(contentType, "text/html") {
 		og := a.ParseOpenGraphMetadata(requestURL, body, contentType)
 
@@ -289,17 +335,16 @@ func (a *App) parseLinkMetadata(requestURL string, body io.Reader, contentType s
 	}
 }
 
-func parseImageDimensions(requestURL string, body io.Reader) (*model.PostImageDimensions, error) {
+func parseImages(body io.Reader) (*model.PostImage, error) {
 	config, _, err := image.DecodeConfig(body)
 	if err != nil {
 		return nil, err
 	}
 
-	dimensions := &model.PostImageDimensions{
-		URL:    requestURL,
+	image := &model.PostImage{
 		Width:  config.Width,
 		Height: config.Height,
 	}
 
-	return dimensions, nil
+	return image, nil
 }

app/post_metadata_test.go

@@ -41,20 +41,22 @@ func TestPreparePostForClient(t *testing.T) {
 		clientPost, err := th.App.PreparePostForClient(post)
 		require.Nil(t, err)
 
-		assert.NotEqual(t, clientPost, post, "should've returned a new post")
-		assert.Equal(t, message, post.Message, "shouldn't have mutated post.Message")
-		assert.NotEqual(t, nil, post.ReactionCounts, "shouldn't have mutated post.ReactionCounts")
-		assert.NotEqual(t, nil, post.FileInfos, "shouldn't have mutated post.FileInfos")
-		assert.NotEqual(t, nil, post.Emojis, "shouldn't have mutated post.Emojis")
-		assert.NotEqual(t, nil, post.ImageDimensions, "shouldn't have mutated post.ImageDimensions")
-		assert.NotEqual(t, nil, post.OpenGraphData, "shouldn't have mutated post.OpenGraphData")
+		t.Run("doesn't mutate provided post", func(t *testing.T) {
+			assert.NotEqual(t, clientPost, post, "should've returned a new post")
 
-		assert.Equal(t, clientPost.Message, post.Message, "shouldn't have changed Message")
-		assert.Len(t, post.ReactionCounts, 0, "should've populated ReactionCounts")
-		assert.Len(t, post.FileInfos, 0, "should've populated FileInfos")
-		assert.Len(t, post.Emojis, 0, "should've populated Emojis")
-		assert.Len(t, post.ImageDimensions, 0, "should've populated ImageDimensions")
-		assert.Len(t, post.OpenGraphData, 0, "should've populated OpenGraphData")
+			assert.Equal(t, message, post.Message, "shouldn't have mutated post.Message")
+			assert.Equal(t, (*model.PostMetadata)(nil), post.Metadata, "shouldn't have mutated post.Metadata")
+		})
+
+		t.Run("populates all fields", func(t *testing.T) {
+			assert.Equal(t, message, clientPost.Message, "shouldn't have changed Message")
+			assert.NotEqual(t, nil, clientPost.Metadata, "should've populated Metadata")
+			assert.Len(t, clientPost.Metadata.Embeds, 0, "should've populated Embeds")
+			assert.Len(t, clientPost.Metadata.ReactionCounts, 0, "should've populated ReactionCounts")
+			assert.Len(t, clientPost.Metadata.Files, 0, "should've populated Files")
+			assert.Len(t, clientPost.Metadata.Emojis, 0, "should've populated Emojis")
+			assert.Len(t, clientPost.Metadata.Images, 0, "should've populated Images")
+		})
 	})
 
 	t.Run("metadata already set", func(t *testing.T) {
@@ -83,10 +85,10 @@ func TestPreparePostForClient(t *testing.T) {
 
 		assert.Equal(t, model.ReactionCounts{
 			"smile": 1,
-		}, clientPost.ReactionCounts, "should've populated post.ReactionCounts")
+		}, clientPost.Metadata.ReactionCounts, "should've populated ReactionCounts")
 	})
 
-	t.Run("file infos", func(t *testing.T) {
+	t.Run("files", func(t *testing.T) {
 		th := setup()
 		defer th.TearDown()
 
@@ -105,7 +107,7 @@ func TestPreparePostForClient(t *testing.T) {
 		clientPost, err := th.App.PreparePostForClient(post)
 		require.Nil(t, err)
 
-		assert.Equal(t, []*model.FileInfo{fileInfo}, clientPost.FileInfos, "should've populated post.FileInfos")
+		assert.Equal(t, []*model.FileInfo{fileInfo}, clientPost.Metadata.Files, "should've populated Files")
 	})
 
 	t.Run("emojis without custom emojis enabled", func(t *testing.T) {
@@ -132,10 +134,16 @@ func TestPreparePostForClient(t *testing.T) {
 		clientPost, err := th.App.PreparePostForClient(post)
 		require.Nil(t, err)
 
-		assert.Len(t, clientPost.ReactionCounts, 2, "should've populated post.ReactionCounts")
-		assert.Equal(t, 1, clientPost.ReactionCounts["smile"], "should've populated post.ReactionCounts for smile")
-		assert.Equal(t, 2, clientPost.ReactionCounts["angry"], "should've populated post.ReactionCounts for angry")
-		assert.ElementsMatch(t, []*model.Emoji{}, clientPost.Emojis, "should've populated empty post.Emojis")
+		t.Run("populates emojis", func(t *testing.T) {
+			assert.ElementsMatch(t, []*model.Emoji{}, clientPost.Metadata.Emojis, "should've populated empty Emojis")
+		})
+
+		t.Run("populates reaction counts", func(t *testing.T) {
+			reactionCounts := clientPost.Metadata.ReactionCounts
+			assert.Len(t, reactionCounts, 2, "should've populated ReactionCounts")
+			assert.Equal(t, 1, reactionCounts["smile"], "should've included 'smile' in ReactionCounts")
+			assert.Equal(t, 2, reactionCounts["angry"], "should've included 'angry' in ReactionCounts")
+		})
 	})
 
 	t.Run("emojis with custom emojis enabled", func(t *testing.T) {
@@ -165,11 +173,17 @@ func TestPreparePostForClient(t *testing.T) {
 		clientPost, err := th.App.PreparePostForClient(post)
 		require.Nil(t, err)
 
-		assert.Len(t, clientPost.ReactionCounts, 3, "should've populated post.ReactionCounts")
-		assert.Equal(t, 1, clientPost.ReactionCounts[emoji1.Name], "should've populated post.ReactionCounts for emoji1")
-		assert.Equal(t, 2, clientPost.ReactionCounts[emoji2.Name], "should've populated post.ReactionCounts for emoji2")
-		assert.Equal(t, 1, clientPost.ReactionCounts["angry"], "should've populated post.ReactionCounts for angry")
-		assert.ElementsMatch(t, []*model.Emoji{emoji1, emoji2, emoji3}, clientPost.Emojis, "should've populated post.Emojis")
+		t.Run("pupulates emojis", func(t *testing.T) {
+			assert.ElementsMatch(t, []*model.Emoji{emoji1, emoji2, emoji3}, clientPost.Metadata.Emojis, "should've populated post.Emojis")
+		})
+
+		t.Run("populates reaction counts", func(t *testing.T) {
+			reactionCounts := clientPost.Metadata.ReactionCounts
+			assert.Len(t, reactionCounts, 3, "should've populated ReactionCounts")
+			assert.Equal(t, 1, reactionCounts[emoji1.Name], "should've included emoji1 in ReactionCounts")
+			assert.Equal(t, 2, reactionCounts[emoji2.Name], "should've included emoji2 in ReactionCounts")
+			assert.Equal(t, 1, reactionCounts["angry"], "should've included angry in ReactionCounts")
+		})
 	})
 
 	t.Run("markdown image dimensions", func(t *testing.T) {
@@ -186,20 +200,35 @@ func TestPreparePostForClient(t *testing.T) {
 		clientPost, err := th.App.PreparePostForClient(post)
 		require.Nil(t, err)
 
-		assert.Len(t, clientPost.ImageDimensions, 2)
-		assert.Equal(t, &model.PostImageDimensions{
-			URL:    "https://github.com/hmhealey/test-files/raw/master/logoVertical.png",
-			Width:  1068,
-			Height: 552,
-		}, clientPost.ImageDimensions[0])
-		assert.Equal(t, &model.PostImageDimensions{
-			URL:    "https://github.com/hmhealey/test-files/raw/master/icon.png",
-			Width:  501,
-			Height: 501,
-		}, clientPost.ImageDimensions[1])
+		t.Run("populates image dimensions", func(t *testing.T) {
+			imageDimensions := clientPost.Metadata.Images
+			assert.Len(t, imageDimensions, 2)
+			assert.Equal(t, &model.PostImage{
+				Width:  1068,
+				Height: 552,
+			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/logoVertical.png"])
+			assert.Equal(t, &model.PostImage{
+				Width:  501,
+				Height: 501,
+			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/icon.png"])
+		})
 	})
 
-	t.Run("linked image dimensions", func(t *testing.T) {
+	t.Run("proxy linked images", func(t *testing.T) {
+		th := setup()
+		defer th.TearDown()
+
+		testProxyLinkedImage(t, th, false)
+	})
+
+	t.Run("proxy opengraph images", func(t *testing.T) {
+		th := setup()
+		defer th.TearDown()
+
+		testProxyOpenGraphImage(t, th, false)
+	})
+
+	t.Run("image embed", func(t *testing.T) {
 		th := setup()
 		defer th.TearDown()
 
@@ -214,23 +243,28 @@ func TestPreparePostForClient(t *testing.T) {
 		clientPost, err := th.App.PreparePostForClient(post)
 		require.Nil(t, err)
 
-		// Reminder that only the first link gets dimensions
-		assert.Len(t, clientPost.ImageDimensions, 1)
-		assert.Equal(t, &model.PostImageDimensions{
-			URL:    "https://github.com/hmhealey/test-files/raw/master/logoVertical.png",
-			Width:  1068,
-			Height: 552,
-		}, clientPost.ImageDimensions[0])
+		// Reminder that only the first link gets an embed and dimensions
+
+		t.Run("populates embeds", func(t *testing.T) {
+			assert.ElementsMatch(t, []*model.PostEmbed{
+				{
+					Type: model.POST_EMBED_IMAGE,
+					URL:  "https://github.com/hmhealey/test-files/raw/master/logoVertical.png",
+				},
+			}, clientPost.Metadata.Embeds)
+		})
+
+		t.Run("populates image dimensions", func(t *testing.T) {
+			imageDimensions := clientPost.Metadata.Images
+			assert.Len(t, imageDimensions, 1)
+			assert.Equal(t, &model.PostImage{
+				Width:  1068,
+				Height: 552,
+			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/logoVertical.png"])
+		})
 	})
 
-	t.Run("proxy linked images", func(t *testing.T) {
-		th := setup()
-		defer th.TearDown()
-
-		testProxyLinkedImage(t, th, false)
-	})
-
-	t.Run("opengraph", func(t *testing.T) {
+	t.Run("opengraph embed", func(t *testing.T) {
 		th := setup()
 		defer th.TearDown()
 
@@ -244,30 +278,73 @@ func TestPreparePostForClient(t *testing.T) {
 		clientPost, err := th.App.PreparePostForClient(post)
 		require.Nil(t, err)
 
-		assert.Len(t, clientPost.OpenGraphData, 1)
-		assert.Equal(t, &opengraph.OpenGraph{
-			Description: "Contribute to hmhealey/test-files development by creating an account on GitHub.",
-			SiteName:    "GitHub",
-			Title:       "hmhealey/test-files",
-			Type:        "object",
-			URL:         "https://github.com/hmhealey/test-files",
-			Images: []*opengraph.Image{
+		t.Run("populates embeds", func(t *testing.T) {
+			assert.ElementsMatch(t, []*model.PostEmbed{
 				{
-					URL: "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4",
+					Type: model.POST_EMBED_OPENGRAPH,
+					URL:  "https://github.com/hmhealey/test-files",
+					Data: &opengraph.OpenGraph{
+						Description: "Contribute to hmhealey/test-files development by creating an account on GitHub.",
+						SiteName:    "GitHub",
+						Title:       "hmhealey/test-files",
+						Type:        "object",
+						URL:         "https://github.com/hmhealey/test-files",
+						Images: []*opengraph.Image{
+							{
+								URL: "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4",
+							},
+						},
+					},
 				},
-			},
-		}, clientPost.OpenGraphData[0])
+			}, clientPost.Metadata.Embeds)
+		})
+
+		t.Run("populates image dimensions", func(t *testing.T) {
+			imageDimensions := clientPost.Metadata.Images
+			assert.Len(t, imageDimensions, 1)
+			assert.Equal(t, &model.PostImage{
+				Width:  420,
+				Height: 420,
+			}, imageDimensions["https://avatars1.githubusercontent.com/u/3277310?s=400&v=4"])
+		})
 	})
 
-	t.Run("opengraph image dimensions", func(t *testing.T) {
-		// TODO
-	})
-
-	t.Run("proxy opengraph images", func(t *testing.T) {
+	t.Run("message attachment embed", func(t *testing.T) {
 		th := setup()
 		defer th.TearDown()
 
-		testProxyOpenGraphImage(t, th, false)
+		post, err := th.App.CreatePost(&model.Post{
+			UserId:    th.BasicUser.Id,
+			ChannelId: th.BasicChannel.Id,
+			Props: map[string]interface{}{
+				"attachments": []interface{}{
+					map[string]interface{}{
+						"text": "![icon](https://github.com/hmhealey/test-files/raw/master/icon.png)",
+					},
+				},
+			},
+		}, th.BasicChannel, false)
+		require.Nil(t, err)
+
+		clientPost, err := th.App.PreparePostForClient(post)
+		require.Nil(t, err)
+
+		t.Run("populates embeds", func(t *testing.T) {
+			assert.ElementsMatch(t, []*model.PostEmbed{
+				{
+					Type: model.POST_EMBED_MESSAGE_ATTACHMENT,
+				},
+			}, clientPost.Metadata.Embeds)
+		})
+
+		t.Run("populates image dimensions", func(t *testing.T) {
+			imageDimensions := clientPost.Metadata.Images
+			assert.Len(t, imageDimensions, 1)
+			assert.Equal(t, &model.PostImage{
+				Width:  501,
+				Height: 501,
+			}, imageDimensions["https://github.com/hmhealey/test-files/raw/master/icon.png"])
+		})
 	})
 }
 
@@ -346,15 +423,20 @@ func testProxyOpenGraphImage(t *testing.T, th *TestHelper, shouldProxy bool) {
 		image.URL = "https://avatars1.githubusercontent.com/u/3277310?s=400&v=4"
 	}
 
-	assert.Len(t, clientPost.OpenGraphData, 1)
-	assert.Equal(t, &opengraph.OpenGraph{
-		Description: "Contribute to hmhealey/test-files development by creating an account on GitHub.",
-		SiteName:    "GitHub",
-		Title:       "hmhealey/test-files",
-		Type:        "object",
-		URL:         "https://github.com/hmhealey/test-files",
-		Images:      []*opengraph.Image{image},
-	}, clientPost.OpenGraphData[0])
+	assert.ElementsMatch(t, []*model.PostEmbed{
+		{
+			Type: model.POST_EMBED_OPENGRAPH,
+			URL:  "https://github.com/hmhealey/test-files",
+			Data: &opengraph.OpenGraph{
+				Description: "Contribute to hmhealey/test-files development by creating an account on GitHub.",
+				SiteName:    "GitHub",
+				Title:       "hmhealey/test-files",
+				Type:        "object",
+				URL:         "https://github.com/hmhealey/test-files",
+				Images:      []*opengraph.Image{image},
+			},
+		},
+	}, clientPost.Metadata.Embeds)
 }
 
 func TestGetCustomEmojisForPost_Message(t *testing.T) {
@@ -503,7 +585,7 @@ func TestGetFirstLinkAndImages(t *testing.T) {
 		"multiple images with duplicate": {
 			Input:             "this is a ![our logo](http://example.com/logo) and ![their logo](http://example.com/logo2) and ![my logo which is their logo](http://example.com/logo2)",
 			ExpectedFirstLink: "",
-			ExpectedImages:    []string{"http://example.com/logo", "http://example.com/logo2"},
+			ExpectedImages:    []string{"http://example.com/logo", "http://example.com/logo2", "http://example.com/logo2"},
 		},
 		"reference image": {
 			Input: `this is a ![our logo][logo]
@@ -534,6 +616,166 @@ func TestGetFirstLinkAndImages(t *testing.T) {
 	}
 }
 
+func TestGetImagesInMessageAttachments(t *testing.T) {
+	for _, test := range []struct {
+		Name     string
+		Post     *model.Post
+		Expected []string
+	}{
+		{
+			Name:     "no attachments",
+			Post:     &model.Post{},
+			Expected: []string{},
+		},
+		{
+			Name: "empty attachments",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{},
+				},
+			},
+			Expected: []string{},
+		},
+		{
+			Name: "attachment with no fields that can contain images",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Title: "This is the title",
+						},
+					},
+				},
+			},
+			Expected: []string{},
+		},
+		{
+			Name: "images in text",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Text: "![logo](https://example.com/logo) and ![icon](https://example.com/icon)",
+						},
+					},
+				},
+			},
+			Expected: []string{"https://example.com/logo", "https://example.com/icon"},
+		},
+		{
+			Name: "images in pretext",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Pretext: "![logo](https://example.com/logo1) and ![icon](https://example.com/icon1)",
+						},
+					},
+				},
+			},
+			Expected: []string{"https://example.com/logo1", "https://example.com/icon1"},
+		},
+		{
+			Name: "images in fields",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Fields: []*model.SlackAttachmentField{
+								{
+									Value: "![logo](https://example.com/logo2) and ![icon](https://example.com/icon2)",
+								},
+							},
+						},
+					},
+				},
+			},
+			Expected: []string{"https://example.com/logo2", "https://example.com/icon2"},
+		},
+		{
+			Name: "images in multiple fields",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Fields: []*model.SlackAttachmentField{
+								{
+									Value: "![logo](https://example.com/logo)",
+								},
+								{
+									Value: "![icon](https://example.com/icon)",
+								},
+							},
+						},
+					},
+				},
+			},
+			Expected: []string{"https://example.com/logo", "https://example.com/icon"},
+		},
+		{
+			Name: "non-string field",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Fields: []*model.SlackAttachmentField{
+								{
+									Value: 77,
+								},
+							},
+						},
+					},
+				},
+			},
+			Expected: []string{},
+		},
+		{
+			Name: "images in multiple locations",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Text:    "![text](https://example.com/text)",
+							Pretext: "![pretext](https://example.com/pretext)",
+							Fields: []*model.SlackAttachmentField{
+								{
+									Value: "![field1](https://example.com/field1)",
+								},
+								{
+									Value: "![field2](https://example.com/field2)",
+								},
+							},
+						},
+					},
+				},
+			},
+			Expected: []string{"https://example.com/text", "https://example.com/pretext", "https://example.com/field1", "https://example.com/field2"},
+		},
+		{
+			Name: "multiple attachments",
+			Post: &model.Post{
+				Props: map[string]interface{}{
+					"attachments": []*model.SlackAttachment{
+						{
+							Text: "![logo](https://example.com/logo)",
+						},
+						{
+							Text: "![icon](https://example.com/icon)",
+						},
+					},
+				},
+			},
+			Expected: []string{"https://example.com/logo", "https://example.com/icon"},
+		},
+	} {
+		t.Run(test.Name, func(t *testing.T) {
+			images := getImagesInMessageAttachments(test.Post)
+
+			assert.ElementsMatch(t, images, test.Expected)
+		})
+	}
+}
+
 func TestParseLinkMetadata(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
@@ -565,8 +807,7 @@ func TestParseLinkMetadata(t *testing.T) {
 		assert.Nil(t, err)
 
 		assert.Nil(t, og)
-		assert.Equal(t, &model.PostImageDimensions{
-			URL:    imageURL,
+		assert.Equal(t, &model.PostImage{
 			Width:  408,
 			Height: 336,
 		}, dimensions)
@@ -608,29 +849,25 @@ func TestParseLinkMetadata(t *testing.T) {
 	})
 }
 
-func TestParseImageDimensions(t *testing.T) {
+func TestParseImages(t *testing.T) {
 	for name, testCase := range map[string]struct {
 		FileName       string
-		URL            string
 		ExpectedWidth  int
 		ExpectedHeight int
 		ExpectError    bool
 	}{
 		"png": {
 			FileName:       "test.png",
-			URL:            "https://example.com/test.png",
 			ExpectedWidth:  408,
 			ExpectedHeight: 336,
 		},
 		"animated gif": {
 			FileName:       "testgif.gif",
-			URL:            "http://example.com/test.gif?foo=bar",
 			ExpectedWidth:  118,
 			ExpectedHeight: 118,
 		},
 		"not an image": {
 			FileName:    "README.md",
-			URL:         "https://example.com/test.png",
 			ExpectError: true,
 		},
 	} {
@@ -638,14 +875,13 @@ func TestParseImageDimensions(t *testing.T) {
 			file, err := testutils.ReadTestFile(testCase.FileName)
 			require.Nil(t, err)
 
-			dimensions, err := parseImageDimensions(testCase.URL, bytes.NewReader(file))
+			dimensions, err := parseImages(bytes.NewReader(file))
 			if testCase.ExpectError {
 				require.NotNil(t, err)
 			} else {
 				require.Nil(t, err)
 
 				require.NotNil(t, dimensions)
-				require.Equal(t, testCase.URL, dimensions.URL)
 				require.Equal(t, testCase.ExpectedWidth, dimensions.Width)
 				require.Equal(t, testCase.ExpectedHeight, dimensions.Height)
 			}