From b49db3c2b7db2bbb53fcefa649caeaa321821bac Mon Sep 17 00:00:00 2001 From: Jatin Garg <48029724+jatgarg@users.noreply.github.com> Date: Sun, 21 Apr 2024 21:44:03 +0530 Subject: [PATCH] Use tenant domain instead of siteUrl in /shares api in ODSP driver --- .../drivers/odsp-driver/src/fetchSnapshot.ts | 86 ++++++++++---- .../src/test/fetchSnapshot.spec.ts | 105 +++++++++++++++++- 2 files changed, 161 insertions(+), 30 deletions(-) diff --git a/packages/drivers/odsp-driver/src/fetchSnapshot.ts b/packages/drivers/odsp-driver/src/fetchSnapshot.ts index f7282c2fc17c..4b3768c3e99b 100644 --- a/packages/drivers/odsp-driver/src/fetchSnapshot.ts +++ b/packages/drivers/odsp-driver/src/fetchSnapshot.ts @@ -8,6 +8,7 @@ import { ITelemetryLoggerExt, isFluidError, PerformanceEvent, + loggerToMonitoringContext, wrapError, } from "@fluidframework/telemetry-utils"; import { fromUtf8ToBase64 } from "@fluid-internal/client-utils"; @@ -205,34 +206,71 @@ async function redeemSharingLink( storageTokenFetcher: InstrumentedStorageTokenFetcher, logger: ITelemetryLoggerExt, forceAccessTokenViaAuthorizationHeader: boolean, -) { - return PerformanceEvent.timedExecAsync( +): Promise { + await PerformanceEvent.timedExecAsync( logger, { eventName: "RedeemShareLink", }, - async () => - getWithRetryForTokenRefresh(async (tokenFetchOptions) => { - assert( - !!odspResolvedUrl.shareLinkInfo?.sharingLinkToRedeem, - 0x1ed /* "Share link should be present" */, - ); - const storageToken = await storageTokenFetcher( - tokenFetchOptions, - "RedeemShareLink", - ); - const encodedShareUrl = getEncodedShareUrl( - odspResolvedUrl.shareLinkInfo?.sharingLinkToRedeem, - ); - const redeemUrl = `${odspResolvedUrl.siteUrl}/_api/v2.0/shares/${encodedShareUrl}`; - const { url, headers } = getUrlAndHeadersWithAuth( - redeemUrl, - storageToken, - forceAccessTokenViaAuthorizationHeader, - ); - headers.prefer = "redeemSharingLink"; - return fetchAndParseAsJSONHelper(url, { headers }); - }), + async () => { + assert( + !!odspResolvedUrl.shareLinkInfo?.sharingLinkToRedeem, + 0x1ed /* "Share link should be present" */, + ); + + const encodedShareUrl = getEncodedShareUrl( + odspResolvedUrl.shareLinkInfo?.sharingLinkToRedeem, + ); + + let redeemUrl: string | undefined; + async function callSharesAPI(baseUrl: string): Promise { + await getWithRetryForTokenRefresh(async (tokenFetchOptions) => { + const storageToken = await storageTokenFetcher( + tokenFetchOptions, + "RedeemShareLink", + ); + redeemUrl = `${baseUrl}/_api/v2.0/shares/${encodedShareUrl}`; + const { url, headers } = getUrlAndHeadersWithAuth( + redeemUrl, + storageToken, + forceAccessTokenViaAuthorizationHeader, + ); + headers.prefer = "redeemSharingLink"; + await fetchAndParseAsJSONHelper(url, { headers }); + }); + } + + const disableUsingTenantDomain = loggerToMonitoringContext(logger).config.getBoolean( + "Fluid.Driver.Odsp.DisableUsingTenantDomainForSharesApi", + ); + // There is an issue where if we use the siteUrl in /shares, then the allowed length of url is just a few hundred characters(300-400) + // and we fail to do the redeem. But if we use the tenant domain in the url, then the allowed length becomes 2048. So, first + // construct the url for /shares using tenant domain but to be on safer side, fallback to using the siteUrl. We get tenant domain + // by getting origin of the siteUrl. + if (!disableUsingTenantDomain) { + try { + await callSharesAPI(new URL(odspResolvedUrl.siteUrl).origin); + return; + } catch (error) { + logger.sendTelemetryEvent( + { + eventName: "ShareLinkRedeemFailedWithTenantDomain", + details: JSON.stringify({ + length: redeemUrl?.length, + shareLinkUrlLength: + odspResolvedUrl.shareLinkInfo?.sharingLinkToRedeem.length, + queryParamsLength: new URL( + odspResolvedUrl.shareLinkInfo?.sharingLinkToRedeem, + ).search.length, + useHeaders: forceAccessTokenViaAuthorizationHeader, + }), + }, + error, + ); + } + } + await callSharesAPI(odspResolvedUrl.siteUrl); + }, ); } diff --git a/packages/drivers/odsp-driver/src/test/fetchSnapshot.spec.ts b/packages/drivers/odsp-driver/src/test/fetchSnapshot.spec.ts index da7bae17a4ed..d18805ad6dae 100644 --- a/packages/drivers/odsp-driver/src/test/fetchSnapshot.spec.ts +++ b/packages/drivers/odsp-driver/src/test/fetchSnapshot.spec.ts @@ -7,9 +7,13 @@ import { strict as assert } from "assert"; import { stub } from "sinon"; import { DriverErrorType } from "@fluidframework/driver-definitions"; import { IOdspResolvedUrl } from "@fluidframework/odsp-driver-definitions"; -import { createChildLogger } from "@fluidframework/telemetry-utils"; +import { + ITelemetryLoggerExt, + MockLogger, + createChildLogger, +} from "@fluidframework/telemetry-utils"; import { EpochTracker } from "../epochTracker"; -import { HostStoragePolicyInternal } from "../contracts"; +import { HostStoragePolicyInternal, IOdspSnapshot } from "../contracts"; import * as fetchSnapshotImport from "../fetchSnapshot"; import { LocalPersistentCache, NonPersistentCache } from "../odspCache"; import { INewFileInfo, IOdspResponse } from "../odspUtils"; @@ -18,7 +22,7 @@ import { getHashedDocumentId, ISnapshotContents } from "../odspPublicUtils"; import { OdspDriverUrlResolver } from "../odspDriverUrlResolver"; import { ISnapshotRequestAndResponseOptions } from "../fetchSnapshot"; import { OdspDocumentStorageService } from "../odspDocumentStorageManager"; -import { createResponse } from "./mockFetch"; +import { createResponse, mockFetchMultiple, notFound, okResponse } from "./mockFetch"; const createUtLocalCache = () => new LocalPersistentCache(); @@ -56,8 +60,10 @@ describe("Tests for snapshot fetch", () => { const resolver = new OdspDriverUrlResolver(); const nonPersistentCache = new NonPersistentCache(); - const logger = createChildLogger(); + let logger: ITelemetryLoggerExt; + let mockLogger: MockLogger; const odspUrl = createOdspUrl({ ...newFileParams, itemId, dataStorePath: "/" }); + let resolved: IOdspResolvedUrl; const content: ISnapshotContents = { snapshotTree: { @@ -70,13 +76,26 @@ describe("Tests for snapshot fetch", () => { sequenceNumber: 0, latestSequenceNumber: 0, }; + + const odspSnapshot: IOdspSnapshot = { + id: "id", + trees: [ + { + entries: [{ path: "path", type: "tree" }], + id: "id", + sequenceNumber: 1, + }, + ], + blobs: [], + }; before(async () => { hashedDocumentId = await getHashedDocumentId(driveId, itemId); }); beforeEach(async () => { localCache = createUtLocalCache(); - // use null logger here as we expect errors + mockLogger = new MockLogger(); + logger = createChildLogger({ logger: mockLogger }); epochTracker = new EpochTracker( localCache, { @@ -86,7 +105,7 @@ describe("Tests for snapshot fetch", () => { logger, ); epochTracker.setEpoch("epoch1", true, "test"); - const resolved = await resolver.resolve({ url: odspUrl }); + resolved = await resolver.resolve({ url: odspUrl }); service = new OdspDocumentStorageService( resolved, async (_options) => "token", @@ -188,4 +207,78 @@ describe("Tests for snapshot fetch", () => { assert.strictEqual(error.contentType, "unknown", "content type should be unknown"); } }); + + it("RedeemFallback behavior when fallback succeeds with using tenant domain", async () => { + resolved.shareLinkInfo = { + sharingLinkToRedeem: "https://microsoft.sharepoint-df.com/sharelink", + }; + hostPolicy.enableRedeemFallback = true; + + const response = (await createResponse( + { "x-fluid-epoch": "epoch1", "content-type": "application/json" }, + odspSnapshot, + 200, + )) as unknown as Response; + + await assert.doesNotReject( + async () => + mockFetchMultiple( + async () => service.getVersions(null, 1), + [ + notFound, + async (): Promise => okResponse({}, {}) as unknown as Response, + async (): Promise => { + return response; + }, + ], + ), + "Should succeed", + ); + assert( + mockLogger.matchEvents([ + { eventName: "TreesLatest_cancel", shareLinkPresent: true }, + { eventName: "RedeemShareLink_end" }, + { eventName: "RedeemFallback", errorType: "fileNotFoundOrAccessDeniedError" }, + { eventName: "TreesLatest_end" }, + ]), + ); + }); + + it("RedeemFallback behavior when fallback succeeds with using siteUrl", async () => { + resolved.shareLinkInfo = { + sharingLinkToRedeem: "https://microsoft.sharepoint-df.com/sharelink", + }; + hostPolicy.enableRedeemFallback = true; + + const response = (await createResponse( + { "x-fluid-epoch": "epoch1", "content-type": "application/json" }, + odspSnapshot, + 200, + )) as unknown as Response; + + await assert.doesNotReject( + async () => + mockFetchMultiple( + async () => service.getVersions(null, 1), + [ + notFound, + notFound, + async (): Promise => okResponse({}, {}) as unknown as Response, + async (): Promise => { + return response; + }, + ], + ), + "Should succeed", + ); + assert( + mockLogger.matchEvents([ + { eventName: "TreesLatest_cancel", shareLinkPresent: true }, + { eventName: "ShareLinkRedeemFailedWithTenantDomain", statusCode: 404 }, + { eventName: "RedeemShareLink_end" }, + { eventName: "RedeemFallback", errorType: "fileNotFoundOrAccessDeniedError" }, + { eventName: "TreesLatest_end" }, + ]), + ); + }); });