From dbb3bd486dae5edb22320d36450a687580f56b18 Mon Sep 17 00:00:00 2001 From: Keepers Date: Tue, 9 May 2023 14:12:08 -0600 Subject: [PATCH] handle large item client redirects (#3350) adds the khttp redirectMiddleware to the http wrapper used for large item downloads, to ensure that proxy servers and other 3xx class responses appropriately follow their redirection. --- #### Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included #### Type of change - [x] :bug: Bugfix #### Issue(s) * #3344 #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- CHANGELOG.md | 1 + src/internal/connector/graph/http_wrapper.go | 9 ++- .../connector/graph/http_wrapper_test.go | 69 +++++++++++++++++++ src/internal/connector/graph/service.go | 14 ++++ 4 files changed, 92 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 259289b6a..39811f5cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Improve restore time on large restores by optimizing how items are loaded from the remote repository. - Remove exchange item filtering based on m365 item ID via the CLI. - OneDrive backups no longer include a user's non-default drives. +- OneDrive and SharePoint file downloads will properly redirect from 3xx responses. ## [v0.7.0] (beta) - 2023-05-02 diff --git a/src/internal/connector/graph/http_wrapper.go b/src/internal/connector/graph/http_wrapper.go index bc469c5f2..55e9f9556 100644 --- a/src/internal/connector/graph/http_wrapper.go +++ b/src/internal/connector/graph/http_wrapper.go @@ -140,13 +140,20 @@ func defaultTransport() http.RoundTripper { } func internalMiddleware(cc *clientConfig) []khttp.Middleware { - return []khttp.Middleware{ + mw := []khttp.Middleware{ &RetryMiddleware{ MaxRetries: cc.maxRetries, Delay: cc.minDelay, }, + khttp.NewRedirectHandler(), &LoggingMiddleware{}, &ThrottleControlMiddleware{}, &MetricsMiddleware{}, } + + if len(cc.appendMiddleware) > 0 { + mw = append(mw, cc.appendMiddleware...) + } + + return mw } diff --git a/src/internal/connector/graph/http_wrapper_test.go b/src/internal/connector/graph/http_wrapper_test.go index 483a5f0ba..d5edaf27d 100644 --- a/src/internal/connector/graph/http_wrapper_test.go +++ b/src/internal/connector/graph/http_wrapper_test.go @@ -2,9 +2,11 @@ package graph import ( "net/http" + "strings" "testing" "github.com/alcionai/clues" + khttp "github.com/microsoft/kiota-http-go" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -43,3 +45,70 @@ func (suite *HTTPWrapperIntgSuite) TestNewHTTPWrapper() { require.NotNil(t, resp) require.Equal(t, http.StatusOK, resp.StatusCode) } + +type mwForceResp struct { + err error + resp *http.Response + alternate func(*http.Request) (bool, *http.Response, error) +} + +func (mw *mwForceResp) Intercept( + pipeline khttp.Pipeline, + middlewareIndex int, + req *http.Request, +) (*http.Response, error) { + ok, r, e := mw.alternate(req) + if ok { + return r, e + } + + return mw.resp, mw.err +} + +type HTTPWrapperUnitSuite struct { + tester.Suite +} + +func TestHTTPWrapperUnitSuite(t *testing.T) { + suite.Run(t, &HTTPWrapperUnitSuite{Suite: tester.NewUnitSuite(t)}) +} + +func (suite *HTTPWrapperUnitSuite) TestNewHTTPWrapper_redirectMiddleware() { + ctx, flush := tester.NewContext() + defer flush() + + var ( + t = suite.T() + uri = "https://graph.microsoft.com" + path = "/fnords/beaux/regard" + url = uri + path + ) + + // can't use gock for this, or else it'll short-circuit the transport, + // and thus skip all the middleware + hdr := http.Header{} + hdr.Set("Location", "localhost:99999999/smarfs") + toResp := &http.Response{ + StatusCode: 302, + Header: hdr, + } + mwResp := mwForceResp{ + resp: toResp, + alternate: func(req *http.Request) (bool, *http.Response, error) { + if strings.HasSuffix(req.URL.String(), "smarfs") { + return true, &http.Response{StatusCode: http.StatusOK}, nil + } + + return false, nil, nil + }, + } + + hw := NewHTTPWrapper(appendMiddleware(&mwResp)) + + resp, err := hw.Request(ctx, http.MethodGet, url, nil, nil) + + require.NoError(t, err, clues.ToCore(err)) + require.NotNil(t, resp) + // require.Equal(t, 1, calledCorrectly, "test server was called with expected path") + require.Equal(t, http.StatusOK, resp.StatusCode) +} diff --git a/src/internal/connector/graph/service.go b/src/internal/connector/graph/service.go index 9db7fb825..288725831 100644 --- a/src/internal/connector/graph/service.go +++ b/src/internal/connector/graph/service.go @@ -173,6 +173,8 @@ type clientConfig struct { // The minimum delay in seconds between retries minDelay time.Duration overrideRetryCount bool + + appendMiddleware []khttp.Middleware } type Option func(*clientConfig) @@ -225,6 +227,14 @@ func MinimumBackoff(dur time.Duration) Option { } } +func appendMiddleware(mw ...khttp.Middleware) Option { + return func(c *clientConfig) { + if len(mw) > 0 { + c.appendMiddleware = mw + } + } +} + // --------------------------------------------------------------------------- // Middleware Control // --------------------------------------------------------------------------- @@ -257,5 +267,9 @@ func kiotaMiddlewares( &MetricsMiddleware{}, }...) + if len(cc.appendMiddleware) > 0 { + mw = append(mw, cc.appendMiddleware...) + } + return mw }