From 6e2d72509c1517c9a6e86ebef11953787e4b50d0 Mon Sep 17 00:00:00 2001
From: Abin Simon <abinsimon10@gmail.com>
Date: Wed, 14 Jun 2023 10:39:07 +0530
Subject: [PATCH] Skip any attachment fetches that fail with OLE conversion
 error (#3607)

These files from what we can understand are not available and thus can't be fetched. This change ensures that we don't fail the backup just because of this error.

<!-- PR description-->

---

#### Does this PR need a docs update or release note?

- [x] :white_check_mark: Yes, it's included
- [ ] :clock1: Yes, but in a later PR
- [ ] :no_entry: No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] :sunflower: Feature
- [x] :bug: Bugfix
- [ ] :world_map: Documentation
- [ ] :robot: Supportability/Tests
- [ ] :computer: CI/Deployment
- [ ] :broom: Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* #<issue>

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] :muscle: Manual
- [x] :zap: Unit test
- [ ] :green_heart: E2E
---
 CHANGELOG.md                           |  1 +
 src/internal/m365/graph/errors.go      |  8 ++++++
 src/internal/m365/graph/errors_test.go | 39 ++++++++++++++++++++++++++
 src/pkg/services/m365/api/mail.go      | 14 +++++++++
 4 files changed, 62 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 99d92b4fd..d528b18c8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix Exchange folder cache population error when parent folder isn't found.
 - Fix Exchange backup issue caused by incorrect json serialization
 - Fix issues with details model containing duplicate entry for api consumers
+- Handle OLE conversion errors when trying to fetch attachments
 
 ### Changed
 - Do not display all the items that we restored at the end if there are more than 15. You can override this with `--verbose`.
diff --git a/src/internal/m365/graph/errors.go b/src/internal/m365/graph/errors.go
index 2f2427b6c..65150868b 100644
--- a/src/internal/m365/graph/errors.go
+++ b/src/internal/m365/graph/errors.go
@@ -44,6 +44,10 @@ const (
 	// the same name as another folder in the same parent. Such duplicate folder
 	// names are not allowed by graph.
 	folderExists errorCode = "ErrorFolderExists"
+	// cannotOpenFileAttachment happen when an attachment is
+	// inaccessible. The error message is usually "OLE conversion
+	// failed for an attachment."
+	cannotOpenFileAttachment errorCode = "ErrorCannotOpenFileAttachment"
 )
 
 type errorMessage string
@@ -126,6 +130,10 @@ func IsErrResourceNotFound(err error) bool {
 	return hasErrorCode(err, resourceNotFound)
 }
 
+func IsErrCannotOpenFileAttachment(err error) bool {
+	return hasErrorCode(err, cannotOpenFileAttachment)
+}
+
 func IsErrAccessDenied(err error) bool {
 	return hasErrorCode(err, errorAccessDenied) || clues.HasLabel(err, LabelStatus(http.StatusForbidden))
 }
diff --git a/src/internal/m365/graph/errors_test.go b/src/internal/m365/graph/errors_test.go
index a0095dc1e..714677179 100644
--- a/src/internal/m365/graph/errors_test.go
+++ b/src/internal/m365/graph/errors_test.go
@@ -384,3 +384,42 @@ func (suite *GraphErrorsUnitSuite) TestIsErrFolderExists() {
 		})
 	}
 }
+
+func (suite *GraphErrorsUnitSuite) TestIsErrCannotOpenFileAttachment() {
+	table := []struct {
+		name   string
+		err    error
+		expect assert.BoolAssertionFunc
+	}{
+		{
+			name:   "nil",
+			err:    nil,
+			expect: assert.False,
+		},
+		{
+			name:   "non-matching",
+			err:    assert.AnError,
+			expect: assert.False,
+		},
+		{
+			name:   "as",
+			err:    ErrInvalidDelta,
+			expect: assert.False,
+		},
+		{
+			name:   "non-matching oDataErr",
+			err:    odErr("fnords"),
+			expect: assert.False,
+		},
+		{
+			name:   "quota-exceeded oDataErr",
+			err:    odErr(string(cannotOpenFileAttachment)),
+			expect: assert.True,
+		},
+	}
+	for _, test := range table {
+		suite.Run(test.name, func() {
+			test.expect(suite.T(), IsErrCannotOpenFileAttachment(test.err))
+		})
+	}
+}
diff --git a/src/pkg/services/m365/api/mail.go b/src/pkg/services/m365/api/mail.go
index 441198c2a..4c608ad28 100644
--- a/src/pkg/services/m365/api/mail.go
+++ b/src/pkg/services/m365/api/mail.go
@@ -406,6 +406,20 @@ func (c Mail) GetItem(
 			ByAttachmentId(ptr.Val(a.GetId())).
 			Get(ctx, attachConfig)
 		if err != nil {
+			if graph.IsErrCannotOpenFileAttachment(err) {
+				logger.CtxErr(ctx, err).
+					With(
+						"skipped_reason", fault.SkipNotFound,
+						"attachment_id", ptr.Val(a.GetId()),
+						"attachment_size", ptr.Val(a.GetSize()),
+					).Info("attachment not found")
+				// TODO This should use a `AddSkip` once we have
+				// figured out the semantics for skipping
+				// subcomponents of an item
+
+				continue
+			}
+
 			return nil, nil, graph.Wrap(ctx, err, "getting mail attachment").
 				With("attachment_id", ptr.Val(a.GetId()), "attachment_size", ptr.Val(a.GetSize()))
 		}