From ec6862439e23d06bee2aec6170f75c04abf40341 Mon Sep 17 00:00:00 2001 From: Xiaoning Liu Date: Thu, 2 Jul 2020 16:16:45 +0800 Subject: [PATCH] [Storage] Add comments for recursive ACL parameters; Resolve review comments --- .../storage-file-datalake/CHANGELOG.md | 2 +- .../review/storage-file-datalake.api.md | 17 ++- .../storage-file-datalake/src/clients.ts | 15 +- .../storage-file-datalake/src/models.ts | 57 +++++++- .../test/node/pathclient.spec.ts | 128 +++++++++--------- 5 files changed, 132 insertions(+), 87 deletions(-) diff --git a/sdk/storage/storage-file-datalake/CHANGELOG.md b/sdk/storage/storage-file-datalake/CHANGELOG.md index 6a948bf803a0..53a44c5ea20c 100644 --- a/sdk/storage/storage-file-datalake/CHANGELOG.md +++ b/sdk/storage/storage-file-datalake/CHANGELOG.md @@ -5,7 +5,7 @@ - Increased the maximum block size for file from 100MiB to 4000MiB(~4GB). And thereby supporting ~200TB maximum size for file. - Added more mappings for Blob and DFS endpoints. [issue #8744](https://github.com/Azure/azure-sdk-for-js/issues/8744). - Added convenience methods `createIfNotExists`, `deleteIfExists` to `DataLakeFileSystemClient`, `DataLakePathClient`, `DataLakeDirectoryClient`, and `DataLakeFileClient`. -- Supported set access control list recursively. +- Added support to set access control list recursively. ## 12.0.1 (2020.05) diff --git a/sdk/storage/storage-file-datalake/review/storage-file-datalake.api.md b/sdk/storage/storage-file-datalake/review/storage-file-datalake.api.md index f9997cf2c078..13f72fdd5ad3 100644 --- a/sdk/storage/storage-file-datalake/review/storage-file-datalake.api.md +++ b/sdk/storage/storage-file-datalake/review/storage-file-datalake.api.md @@ -56,7 +56,7 @@ export interface AccessControlChanges { aggregateCounters: AccessControlChangeCounters; batchCounters: AccessControlChangeCounters; batchFailures: AccessControlChangeFailure[]; - continuation?: string; + continuationToken?: string; } // @public (undocumented) @@ -874,8 +874,10 @@ export interface PathAccessControl { } // @public (undocumented) -export interface PathAccessControlItem extends RemovePathAccessControlItem { - // (undocumented) +export interface PathAccessControlItem { + accessControlType: AccessControlType; + defaultScope: boolean; + entityId: string; permissions: RolePermissions; } @@ -891,14 +893,14 @@ export interface PathAppendDataHeaders { export interface PathChangeAccessControlRecursiveOptions extends CommonOptions { abortSignal?: AbortSignalLike; batchSize?: number; - continuation?: string; + continuationToken?: string; maxBatches?: number; onProgress?: (progress: AccessControlChanges) => void; } // @public export interface PathChangeAccessControlRecursiveResponse { - continuation?: string; + continuationToken?: string; counters: AccessControlChangeCounters; } @@ -1466,12 +1468,9 @@ export interface RawAccessPolicy { // @public (undocumented) export interface RemovePathAccessControlItem { - // (undocumented) accessControlType: AccessControlType; - // (undocumented) defaultScope: boolean; - // (undocumented) - entityId: string; + entityId?: string; } export { RequestPolicy } diff --git a/sdk/storage/storage-file-datalake/src/clients.ts b/sdk/storage/storage-file-datalake/src/clients.ts index 03cdf8d47c24..a2bac0f9180d 100644 --- a/sdk/storage/storage-file-datalake/src/clients.ts +++ b/sdk/storage/storage-file-datalake/src/clients.ts @@ -61,6 +61,7 @@ import { PathSetMetadataResponse, PathSetPermissionsOptions, PathSetPermissionsResponse, + RemovePathAccessControlItem, } from "./models"; import { newPipeline, Pipeline, StoragePipelineOptions } from "./Pipeline"; import { StorageClient } from "./StorageClient"; @@ -148,11 +149,11 @@ export class DataLakePathClient extends StorageClient { changedDirectoriesCount: 0, changedFilesCount: 0 }, - continuation: undefined + continuationToken: undefined }; try { - let continuation = options.continuation; + let continuationToken = options.continuationToken; let batchCounter = 0; let reachMaxBatches = false; do { @@ -160,14 +161,14 @@ export class DataLakePathClient extends StorageClient { ...options, acl: toAclString(acl as PathAccessControlItem[]), maxRecords: options.batchSize, - continuation, + continuation: continuationToken, spanOptions }); batchCounter++; - continuation = response.continuation; + continuationToken = response.continuation; // Update result - result.continuation = continuation; + result.continuationToken = continuationToken; result.counters.failedChangesCount += response.failureCount || 0; result.counters.changedDirectoriesCount += response.directoriesSuccessful || 0; result.counters.changedFilesCount += response.filesSuccessful || 0; @@ -182,14 +183,14 @@ export class DataLakePathClient extends StorageClient { changedFilesCount: response.filesSuccessful || 0 }, aggregateCounters: result.counters, - continuation + continuationToken: continuationToken }; options.onProgress(progress); } reachMaxBatches = options.maxBatches === undefined ? false : batchCounter >= options.maxBatches; - } while (continuation && !reachMaxBatches); + } while (continuationToken && !reachMaxBatches); return result; } catch (e) { diff --git a/sdk/storage/storage-file-datalake/src/models.ts b/sdk/storage/storage-file-datalake/src/models.ts index 3f68458514ec..4db720ba1fd5 100644 --- a/sdk/storage/storage-file-datalake/src/models.ts +++ b/sdk/storage/storage-file-datalake/src/models.ts @@ -3,7 +3,6 @@ import { HttpResponse, TransferProgressEvent } from "@azure/core-http"; import { LeaseAccessConditions, ModifiedAccessConditions, UserDelegationKeyModel } from "@azure/storage-blob"; import { - FileSystemListPathsHeaders, FileSystemListPathsHeaders, PathCreateResponse, PathDeleteResponse, @@ -404,12 +403,58 @@ export interface PathPermissions { export type AccessControlType = "user" | "group" | "mask" | "other"; export interface RemovePathAccessControlItem { + /** + * Indicates whether this is the default entry for the ACL. + * + * @type {boolean} + * @memberof RemovePathAccessControlItem + */ defaultScope: boolean; + /** + * Specifies which role this entry targets. + * + * @type {AccessControlType} + * @memberof RemovePathAccessControlItem + */ accessControlType: AccessControlType; - entityId: string; + /** + * Specifies the entity for which this entry applies. + * Must be omitted for types mask or other. It must also be omitted when the user or group is the owner. + * + * @type {string} + * @memberof RemovePathAccessControlItem + */ + entityId?: string; } -export interface PathAccessControlItem extends RemovePathAccessControlItem { +export interface PathAccessControlItem { + /** + * Indicates whether this is the default entry for the ACL. + * + * @type {boolean} + * @memberof PathAccessControlItem + */ + defaultScope: boolean; + /** + * Specifies which role this entry targets. + * + * @type {AccessControlType} + * @memberof PathAccessControlItem + */ + accessControlType: AccessControlType; + /** + * Specifies the entity for which this entry applies. + * + * @type {string} + * @memberof PathAccessControlItem + */ + entityId: string; + /** + * Access control permissions. + * + * @type {RolePermissions} + * @memberof PathAccessControlItem + */ permissions: RolePermissions; } @@ -519,7 +564,7 @@ export interface PathChangeAccessControlRecursiveOptions extends CommonOptions { * @type {string} * @memberof PathChangeAccessControlRecursiveOptions */ - continuation?: string; + continuationToken?: string; /** * Callback where caller can track progress of the operation * as well as collect paths that failed to change Access Control. @@ -594,7 +639,7 @@ export interface AccessControlChanges { * @type {string} * @memberof AccessControlChanges */ - continuation?: string; + continuationToken?: string; } /** @@ -644,7 +689,7 @@ export interface PathChangeAccessControlRecursiveResponse { * @type {string} * @memberof PathChangeAccessControlRecursiveResponse */ - continuation?: string; + continuationToken?: string; } export interface PathSetPermissionsOptions extends CommonOptions { diff --git a/sdk/storage/storage-file-datalake/test/node/pathclient.spec.ts b/sdk/storage/storage-file-datalake/test/node/pathclient.spec.ts index e35c0f0bb267..945b678c42fb 100644 --- a/sdk/storage/storage-file-datalake/test/node/pathclient.spec.ts +++ b/sdk/storage/storage-file-datalake/test/node/pathclient.spec.ts @@ -27,7 +27,7 @@ describe("DataLakePathClient Node.js only", () => { let recorder: any; - beforeEach(async function() { + beforeEach(async function () { recorder = record(this, recorderEnvSetup); serviceClient = getDataLakeServiceClient(); fileSystemName = recorder.getUniqueName("filesystem"); @@ -40,7 +40,7 @@ describe("DataLakePathClient Node.js only", () => { await fileClient.flush(content.length); }); - afterEach(async function() { + afterEach(async function () { await fileSystemClient.delete(); recorder.stop(); }); @@ -54,8 +54,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: true, - execute: true - } + execute: true, + }, }, { accessControlType: "group", @@ -64,8 +64,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: false, - execute: true - } + execute: true, + }, }, { accessControlType: "other", @@ -74,9 +74,9 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: false, write: true, - execute: false - } - } + execute: false, + }, + }, ]; await fileClient.setAccessControl(acl); @@ -90,18 +90,18 @@ describe("DataLakePathClient Node.js only", () => { owner: { read: true, write: true, - execute: true + execute: true, }, group: { read: true, write: false, - execute: true + execute: true, }, other: { read: false, write: true, - execute: false - } + execute: false, + }, }); assert.deepStrictEqual(permissions.acl, acl); }); @@ -115,8 +115,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: true, - execute: true - } + execute: true, + }, }, { accessControlType: "group", @@ -125,8 +125,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: false, - execute: true - } + execute: true, + }, }, { accessControlType: "other", @@ -135,13 +135,13 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: false, write: true, - execute: false - } - } + execute: false, + }, + }, ]; await fileClient.setAccessControl(acl, { owner: "$superuser", - group: "$superuser" + group: "$superuser", }); const permissions = await fileClient.getAccessControl(); @@ -154,18 +154,18 @@ describe("DataLakePathClient Node.js only", () => { owner: { read: true, write: true, - execute: true + execute: true, }, group: { read: true, write: false, - execute: true + execute: true, }, other: { read: false, write: true, - execute: false - } + execute: false, + }, }); assert.deepStrictEqual(permissions.acl, acl); }); @@ -179,8 +179,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: true, - execute: false - } + execute: false, + }, }, { accessControlType: "group", @@ -189,8 +189,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: false, - execute: true - } + execute: true, + }, }, { accessControlType: "other", @@ -199,9 +199,9 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: false, write: true, - execute: true - } - } + execute: true, + }, + }, ]; const permissions: PathPermissions = { @@ -210,18 +210,18 @@ describe("DataLakePathClient Node.js only", () => { owner: { read: true, write: true, - execute: false + execute: false, }, group: { read: true, write: false, - execute: true + execute: true, }, other: { read: false, write: true, - execute: false - } + execute: false, + }, }; await fileClient.setPermissions(permissions); @@ -232,7 +232,7 @@ describe("DataLakePathClient Node.js only", () => { assert.deepStrictEqual(response.group, "$superuser"); assert.deepStrictEqual(response.permissions, { ...permissions, - other: { ...permissions.other, execute: true } + other: { ...permissions.other, execute: true }, }); assert.deepStrictEqual(response.acl, acl); }); @@ -246,8 +246,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: true, - execute: false - } + execute: false, + }, }, { accessControlType: "group", @@ -256,8 +256,8 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: true, write: false, - execute: true - } + execute: true, + }, }, { accessControlType: "other", @@ -266,9 +266,9 @@ describe("DataLakePathClient Node.js only", () => { permissions: { read: false, write: true, - execute: true - } - } + execute: true, + }, + }, ]; const permissions: PathPermissions = { @@ -277,18 +277,18 @@ describe("DataLakePathClient Node.js only", () => { owner: { read: true, write: true, - execute: false + execute: false, }, group: { read: true, write: false, - execute: true + execute: true, }, other: { read: false, write: true, - execute: false - } + execute: false, + }, }; await fileClient.setPermissions(permissions, { owner: "$superuser", group: "$superuser" }); @@ -299,7 +299,7 @@ describe("DataLakePathClient Node.js only", () => { assert.deepStrictEqual(response.group, "$superuser"); assert.deepStrictEqual(response.permissions, { ...permissions, - other: { ...permissions.other, execute: true } + other: { ...permissions.other, execute: true }, }); assert.deepStrictEqual(response.acl, acl); }); @@ -335,7 +335,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { let recorder: any; - beforeEach(async function() { + beforeEach(async function () { recorder = record(this, recorderEnvSetup); serviceClient = getDataLakeServiceClient(); fileSystemName = recorder.getUniqueName("filesystem"); @@ -348,7 +348,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { await fileClient.flush(content.length); }); - afterEach(async function() { + afterEach(async function () { await fileSystemClient.delete(); recorder.stop(); }); @@ -388,7 +388,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { assert.deepStrictEqual(3, result.counters.changedDirectoriesCount); assert.deepStrictEqual(4, result.counters.changedFilesCount); assert.deepStrictEqual(0, result.counters.failedChangesCount); - assert.deepStrictEqual(undefined, result.continuation); + assert.deepStrictEqual(undefined, result.continuationToken); }); it("setAccessControlRecursive should work with options - maxBatches", async () => { @@ -427,12 +427,12 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { maxBatches: 1, onProgress: () => { batchCounter++; - } + }, } ); assert.deepStrictEqual(1, batchCounter); - assert.notDeepStrictEqual(undefined, result.continuation); + assert.notDeepStrictEqual(undefined, result.continuationToken); }); it("setAccessControlRecursive should work with options - batchSize", async () => { @@ -465,7 +465,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { const cumulativeCounters: AccessControlChangeCounters = { changedDirectoriesCount: 0, changedFilesCount: 0, - failedChangesCount: 0 + failedChangesCount: 0, }; const result = await directoryClient.setAccessControlRecursive( toAcl( @@ -499,7 +499,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { ); batchCounter++; - } + }, } ); @@ -509,7 +509,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { assert.deepStrictEqual(3, result.counters.changedDirectoriesCount); assert.deepStrictEqual(4, result.counters.changedFilesCount); assert.deepStrictEqual(0, result.counters.failedChangesCount); - assert.deepStrictEqual(undefined, result.continuation); + assert.deepStrictEqual(undefined, result.continuationToken); assert.deepStrictEqual(true, batchCounter > 3); }); @@ -551,10 +551,10 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { batchSize: 2, onProgress: (progress) => { midProgress = progress; - continuation = progress.continuation; + continuation = progress.continuationToken; aborter.abort(); }, - abortSignal: aborter.signal + abortSignal: aborter.signal, } ); } catch (err) { @@ -567,7 +567,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { "user::rwx,user:ec3595d6-2c17-4696-8caa-7e139758d24a:rw-,group::rw-,mask::rwx,other::---" ), { - continuation + continuationToken: continuation, } ); @@ -583,7 +583,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { 0, result.counters.failedChangesCount + midProgress!.batchCounters.failedChangesCount ); - assert.deepStrictEqual(undefined, result.continuation); + assert.deepStrictEqual(undefined, result.continuationToken); }); it("updateAccessControlRecursive should work", async () => { @@ -621,7 +621,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { assert.deepStrictEqual(3, result.counters.changedDirectoriesCount); assert.deepStrictEqual(4, result.counters.changedFilesCount); assert.deepStrictEqual(0, result.counters.failedChangesCount); - assert.deepStrictEqual(undefined, result.continuation); + assert.deepStrictEqual(undefined, result.continuationToken); }); it("removeAccessControlRecursive should work", async () => { @@ -659,7 +659,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { assert.deepStrictEqual(3, result.counters.changedDirectoriesCount); assert.deepStrictEqual(4, result.counters.changedFilesCount); assert.deepStrictEqual(0, result.counters.failedChangesCount); - assert.deepStrictEqual(undefined, result.continuation); + assert.deepStrictEqual(undefined, result.continuationToken); const removeResult = await directoryClient.removeAccessControlRecursive( toRemoveAcl( @@ -673,7 +673,7 @@ describe("DataLakePathClient setAccessControlRecursive Node.js only", () => { assert.deepStrictEqual(3, removeResult.counters.changedDirectoriesCount); assert.deepStrictEqual(4, removeResult.counters.changedFilesCount); assert.deepStrictEqual(0, removeResult.counters.failedChangesCount); - assert.deepStrictEqual(undefined, removeResult.continuation); + assert.deepStrictEqual(undefined, removeResult.continuationToken); }); it("setAccessControlRecursive should work with progress failures - TODO", async () => {});