Skip to content

Commit

Permalink
Use tenant domain instead of siteUrl in /shares api in ODSP driver
Browse files Browse the repository at this point in the history
  • Loading branch information
@jatgarg
jatgarg authored and Jatin Garg committed Apr 21, 2024
1 parent 906dd1f commit b49db3c
Show file tree
Hide file tree
Showing 2 changed files with 161 additions and 30 deletions.
86 changes: 62 additions & 24 deletions packages/drivers/odsp-driver/src/fetchSnapshot.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
ITelemetryLoggerExt,
isFluidError,
PerformanceEvent,
loggerToMonitoringContext,
wrapError,
} from "@fluidframework/telemetry-utils";
import { fromUtf8ToBase64 } from "@fluid-internal/client-utils";
Expand Down Expand Up @@ -205,34 +206,71 @@ async function redeemSharingLink(
storageTokenFetcher: InstrumentedStorageTokenFetcher,
logger: ITelemetryLoggerExt,
forceAccessTokenViaAuthorizationHeader: boolean,
) {
return PerformanceEvent.timedExecAsync(
): Promise<void> {
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<void> {
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);
},
);
}

Expand Down
105 changes: 99 additions & 6 deletions packages/drivers/odsp-driver/src/test/fetchSnapshot.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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();

Expand Down Expand Up @@ -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: {
Expand All @@ -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,
{
Expand All @@ -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",
Expand Down Expand Up @@ -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<Response> => okResponse({}, {}) as unknown as Response,
async (): Promise<Response> => {
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<Response> => okResponse({}, {}) as unknown as Response,
async (): Promise<Response> => {
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" },
]),
);
});
});

0 comments on commit b49db3c

Please sign in to comment.