Merge remote-tracking branch 'origin/adreed/fix-acl-api' into adreed/fix-acl-api

Author: adreed-msft

cmd/copy.go

@@ -123,6 +123,8 @@ type rawCopyCmdArgs struct {
 	preserveSMBPermissions bool
 	preservePermissions    bool // Separate flag so that we don't get funkiness with two "flags" targeting the same boolean
 	preserveOwner          bool // works in conjunction with preserveSmbPermissions
+	// Default true; false indicates that the destination is the target directory, rather than something we'd put a directory under (e.g. a container)
+	asSubdir bool
 	// Opt-in flag to persist additional SMB properties to Azure Files. Named ...info instead of ...properties
 	// because the latter was similar enough to preserveSMBPermissions to induce user error
 	preserveSMBInfo bool
@@ -652,6 +654,9 @@ func (raw rawCopyCmdArgs) cook() (CookedCopyCmdArgs, error) {
 		cooked.isHNStoHNS = true // override HNS settings, since if a user is tx'ing blob->blob and copying permissions, it's DEFINITELY going to be HNS (since perms don't exist w/o HNS).
 	}
 
+	// --as-subdir is OK on all sources and destinations, but additional verification has to be done down the line. (e.g. https://account.blob.core.windows.net is not a valid root)
+	cooked.asSubdir = raw.asSubdir
+
 	cooked.IncludeDirectoryStubs = raw.includeDirectoryStubs || (cooked.isHNStoHNS && cooked.preservePermissions.IsTruthy())
 
 	if err = crossValidateSymlinksAndPermissions(cooked.FollowSymlinks, cooked.preservePermissions.IsTruthy()); err != nil {
@@ -1093,6 +1098,9 @@ type CookedCopyCmdArgs struct {
 	// Whether to enable Windows special privileges
 	backupMode bool
 
+	// Whether to rename/share the root
+	asSubdir bool
+
 	// whether user wants to preserve full properties during service to service copy, the default value is true.
 	// For S3 and Azure File non-single file source, as list operation doesn't return full properties of objects/files,
 	// to preserve full properties AzCopy needs to send one additional request per object/file.
@@ -1846,6 +1854,7 @@ func init() {
 	cpCmd.PersistentFlags().BoolVar(&raw.noGuessMimeType, "no-guess-mime-type", false, "Prevents AzCopy from detecting the content-type based on the extension or content of the file.")
 	cpCmd.PersistentFlags().BoolVar(&raw.preserveLastModifiedTime, "preserve-last-modified-time", false, "Only available when destination is file system.")
 	cpCmd.PersistentFlags().BoolVar(&raw.preserveSMBPermissions, "preserve-smb-permissions", false, "False by default. Preserves SMB ACLs between aware resources (Windows and Azure Files). For downloads, you will also need the --backup flag to restore permissions where the new Owner will not be the user running AzCopy. This flag applies to both files and folders, unless a file-only filter is specified (e.g. include-pattern).")
+	cpCmd.PersistentFlags().BoolVar(&raw.asSubdir, "as-subdir", true, "True by default. Places folder sources as subdirectories under the destination.")
 	cpCmd.PersistentFlags().BoolVar(&raw.preserveOwner, common.PreserveOwnerFlagName, common.PreserveOwnerDefault, "Only has an effect in downloads, and only when --preserve-smb-permissions is used. If true (the default), the file Owner and Group are preserved in downloads. If set to false, --preserve-smb-permissions will still preserve ACLs but Owner and Group will be based on the user running AzCopy")
 	cpCmd.PersistentFlags().BoolVar(&raw.preserveSMBInfo, "preserve-smb-info", true, "For SMB-aware locations, flag will be set to true by default. Preserves SMB property info (last write time, creation time, attribute bits) between SMB-aware resources (Windows and Azure Files). Only the attribute bits supported by Azure Files will be transferred; any others will be ignored. This flag applies to both files and folders, unless a file-only filter is specified (e.g. include-pattern). The info transferred for folders is the same as that for files, except for Last Write Time which is never preserved for folders.")
 	cpCmd.PersistentFlags().BoolVar(&raw.forceIfReadOnly, "force-if-read-only", false, "When overwriting an existing file on Windows or Azure Files, force the overwrite to work even if the existing file has its read-only attribute set")

cmd/copyEnumeratorInit.go

@@ -76,9 +76,12 @@ func (cca *CookedCopyCmdArgs) initEnumerator(jobPartOrder common.CopyJobPartOrde
 		return nil, err
 	}
 
-	// Ensure we're only copying from a directory with a trailing wildcard or recursive.
+	// Ensure we're only copying a directory under valid conditions
 	isSourceDir := traverser.IsDirectory(true)
-	if isSourceDir && !cca.Recursive && !cca.StripTopDir {
+	if isSourceDir &&
+		!cca.Recursive && // Copies the folder & everything under it
+		!cca.StripTopDir { // Copies only everything under it
+		// todo: dir only transfer, also todo: support syncing the root folder's acls on sync.
 		return nil, errors.New("cannot use directory as source without --recursive or a trailing wildcard (/*)")
 	}
 
@@ -113,6 +116,10 @@ func (cca *CookedCopyCmdArgs) initEnumerator(jobPartOrder common.CopyJobPartOrde
 		return nil, errors.New("cannot transfer individual files/folders to the root of a service. Add a container or directory to the destination URL")
 	}
 
+	if srcLevel == ELocationLevel.Container() && dstLevel == ELocationLevel.Service() && !cca.asSubdir {
+		return nil, errors.New("cannot use --as-subdir=false with a service level destination")
+	}
+
 	// When copying a container directly to a container, strip the top directory
 	if srcLevel == ELocationLevel.Container() && dstLevel == ELocationLevel.Container() && cca.FromTo.From().IsRemote() && cca.FromTo.To().IsRemote() {
 		cca.StripTopDir = true
@@ -255,8 +262,8 @@ func (cca *CookedCopyCmdArgs) initEnumerator(jobPartOrder common.CopyJobPartOrde
 			}
 		}
 
-		srcRelPath := cca.MakeEscapedRelativePath(true, isDestDir, object)
-		dstRelPath := cca.MakeEscapedRelativePath(false, isDestDir, object)
+		srcRelPath := cca.MakeEscapedRelativePath(true, isDestDir, cca.asSubdir, object)
+		dstRelPath := cca.MakeEscapedRelativePath(false, isDestDir, cca.asSubdir, object)
 
 		transfer, shouldSendToSte := object.ToNewCopyTransfer(
 			cca.autoDecompress && cca.FromTo.IsDownload(),
@@ -572,7 +579,7 @@ func pathEncodeRules(path string, fromTo common.FromTo, disableAutoDecoding bool
 	return path
 }
 
-func (cca *CookedCopyCmdArgs) MakeEscapedRelativePath(source bool, dstIsDir bool, object StoredObject) (relativePath string) {
+func (cca *CookedCopyCmdArgs) MakeEscapedRelativePath(source bool, dstIsDir bool, asSubdir bool, object StoredObject) (relativePath string) {
 	// write straight to /dev/null, do not determine a indirect path
 	if !source && cca.Destination.Value == common.Dev_Null {
 		return "" // ignore path encode rules
@@ -602,8 +609,12 @@ func (cca *CookedCopyCmdArgs) MakeEscapedRelativePath(source bool, dstIsDir bool
 		return pathEncodeRules(relativePath, cca.FromTo, cca.disableAutoDecoding, source)
 	}
 
-	// If it's out here, the object is contained in a folder, or was found via a wildcard, or object.isSourceRootFolder == true
+	// user is not placing the source as a subdir
+	if object.isSourceRootFolder() && !asSubdir {
+		relativePath = ""
+	}
 
+	// If it's out here, the object is contained in a folder, or was found via a wildcard, or object.isSourceRootFolder == true
 	if object.isSourceRootFolder() {
 		relativePath = "" // otherwise we get "/" from the line below, and that breaks some clients, e.g. blobFS
 	} else {
@@ -612,7 +623,7 @@ func (cca *CookedCopyCmdArgs) MakeEscapedRelativePath(source bool, dstIsDir bool
 
 	if common.IffString(source, object.ContainerName, object.DstContainerName) != "" {
 		relativePath = `/` + common.IffString(source, object.ContainerName, object.DstContainerName) + relativePath
-	} else if !source && !cca.StripTopDir {
+	} else if !source && !cca.StripTopDir && cca.asSubdir { // Avoid doing this where the root is shared or renamed.
 		// We ONLY need to do this adjustment to the destination.
 		// The source SAS has already been removed. No need to convert it to a URL or whatever.
 		// Save to a directory

cmd/zt_copy_s2smigration_test.go

@@ -89,6 +89,7 @@ func getDefaultRawCopyInput(src, dst string) rawCopyCmdArgs {
 		s2sInvalidMetadataHandleOption: defaultS2SInvalideMetadataHandleOption.String(),
 		forceWrite:                     common.EOverwriteOption.True().String(),
 		preserveOwner:                  common.PreserveOwnerDefault,
+		asSubdir:                       true,
 	}
 }
 

cmd/zt_scenario_helpers_for_test.go

@@ -36,7 +36,7 @@ import (
 	gcpUtils "cloud.google.com/go/storage"
 
 	"github.com/Azure/azure-storage-azcopy/v10/azbfs"
-	minio "github.com/minio/minio-go"
+	"github.com/minio/minio-go"
 
 	"github.com/Azure/azure-storage-azcopy/v10/common"
 	"github.com/Azure/azure-storage-blob-go/azblob"
@@ -848,6 +848,7 @@ func getDefaultCopyRawInput(src string, dst string) rawCopyCmdArgs {
 		s2sInvalidMetadataHandleOption: defaultS2SInvalideMetadataHandleOption.String(),
 		forceWrite:                     common.EOverwriteOption.True().String(),
 		preserveOwner:                  common.PreserveOwnerDefault,
+		asSubdir: true,
 	}
 }
 

e2etest/declarativeHelpers.go

@@ -140,6 +140,7 @@ func (a *testingAsserter) CompactScenarioName() string {
 
 type params struct {
 	recursive                 bool
+	invertedAsSubdir          bool // this flag is INVERTED, because it is TRUE by default. todo: use pointers instead?
 	includePath               string
 	includePattern            string
 	includeAfter              string

e2etest/declarativeScenario.go

@@ -187,7 +187,8 @@ func (s *scenario) runAzCopy() {
 	result, wasClean, err := r.ExecuteAzCopyCommand(
 		s.operation,
 		s.state.source.getParam(s.stripTopDir, srcUseSas, tf.objectTarget),
-		s.state.dest.getParam(false, useSas, tf.objectTarget),
+		// Prefer the destination target over the object target itself.
+		s.state.dest.getParam(false, useSas, common.IffString(tf.destTarget != "", tf.destTarget, tf.objectTarget)),
 		afterStart, s.chToStdin)
 
 	if !wasClean {
@@ -296,20 +297,26 @@ func (s *scenario) getTransferInfo() (srcRoot string, dstRoot string, expectFold
 	// compute dest, taking into account our stripToDir rules
 	addedDirAtDest = ""
 	areBothContainerLike := s.state.source.isContainerLike() && s.state.dest.isContainerLike()
+
+	tf := s.GetTestFiles()
 	if s.stripTopDir || s.operation == eOperation.Sync() || areBothContainerLike {
 		// Sync always acts like stripTopDir is true.
 		// For copies between two container-like locations, we don't expect the root directory to be transferred, regardless of stripTopDir.
 		// Yes, this is arguably inconsistent. But its the way its always been, and it does seem to match user expectations for copies
 		// of that kind.
 		expectRootFolder = false
 	} else if s.fromTo.From().IsLocal() {
-		if s.GetTestFiles().objectTarget == "" {
+		if tf.objectTarget == "" && tf.destTarget == "" {
 			addedDirAtDest = filepath.Base(srcRoot)
+		} else if tf.destTarget != "" {
+			addedDirAtDest = tf.destTarget
 		}
 		dstRoot = fmt.Sprintf("%s%c%s", dstRoot, os.PathSeparator, addedDirAtDest)
 	} else {
-		if s.GetTestFiles().objectTarget == "" {
+		if tf.objectTarget == "" && tf.destTarget == "" {
 			addedDirAtDest = path.Base(srcRoot)
+		} else if tf.destTarget != "" {
+			addedDirAtDest = tf.destTarget
 		}
 		dstRoot = fmt.Sprintf("%s/%s", dstRoot, addedDirAtDest)
 	}

e2etest/declarativeTestFiles.go

@@ -307,6 +307,7 @@ func folder(n string, properties ...withPropertyProvider) *testObject {
 type testFiles struct {
 	defaultSize  string                  // how big should the files be? Applies to those files that don't specify individual sizes. Uses the same K, M, G suffixes as benchmark mode's size-per-file
 	objectTarget string                  // should we target only a single file/folder?
+	destTarget   string                  // do we want to copy under a folder or rename?
 	sourcePublic azblob.PublicAccessType // should the source blob container be public? (ONLY APPLIES TO BLOB.)
 
 	// The files/folders that we expect to be transferred. Elements of the list must be strings or testObject's.
@@ -329,6 +330,7 @@ func (tf testFiles) cloneShouldTransfers() testFiles {
 	return testFiles{
 		defaultSize:    tf.defaultSize,
 		objectTarget:   tf.objectTarget,
+		destTarget:     tf.destTarget,
 		sourcePublic:   tf.sourcePublic,
 		shouldTransfer: tf.shouldTransfer,
 	}
@@ -339,6 +341,7 @@ func (tf testFiles) DeepCopy() testFiles {
 	ret.defaultSize = tf.defaultSize
 
 	ret.objectTarget = tf.objectTarget
+	ret.destTarget = tf.destTarget
 	ret.sourcePublic = tf.sourcePublic
 	ret.shouldTransfer = tf.copyList(tf.shouldTransfer)
 	ret.shouldIgnore = tf.copyList(tf.shouldIgnore)

e2etest/runner.go

@@ -68,6 +68,7 @@ func (t *TestRunner) SetAllFlags(p params) {
 
 	// TODO: TODO: nakulkar-msft there will be many more to add here
 	set("recursive", p.recursive, false)
+	set("as-subdir", !p.invertedAsSubdir, true)
 	set("include-path", p.includePath, "")
 	set("exclude-path", p.excludePath, "")
 	set("include-pattern", p.includePattern, "")

e2etest/zt_basic_copy_sync_remove_test.go

@@ -306,3 +306,30 @@ func TestBasic_CopyToWrongBlobType(t *testing.T) {
 		}
 	}
 }
+
+func TestBasic_CopyWithShareRoot(t *testing.T) {
+	RunScenarios(
+		t,
+		eOperation.Copy(), // Sync already shares the root by default.
+		eTestFromTo.AllUploads(),
+		eValidate.Auto(),
+		params{
+			recursive:        true,
+			invertedAsSubdir: true,
+		},
+		nil,
+		testFiles{
+			defaultSize: "1K",
+			destTarget:  "newName",
+
+			shouldTransfer: []interface{}{
+				folder(""),
+				f("asdf.txt"),
+				folder("a"),
+				f("a/asdf.txt"),
+			},
+		},
+		EAccountType.Standard(),
+		"",
+	)
+}

e2etest/zt_preserve_smb_properties_test.go

@@ -215,3 +215,49 @@ func TestProperties_SMBPermsAndFlagsWithSync(t *testing.T) {
 		shouldIgnore:   recreateFiles,
 	}, EAccountType.Standard(), "")
 }
+
+func TestProperties_SMBWithCopyWithShareRoot(t *testing.T) {
+	fileSDDL, err := AdjustSDDLToLocal(SampleSDDL, SampleSDDLPlaceHolder)
+	if err != nil {
+		t.Error(err)
+	}
+
+	rootSDDL, err := AdjustSDDLToLocal(RootSampleSDDL, SampleSDDLPlaceHolder)
+	if err != nil {
+		t.Error(err)
+	}
+
+	folderSDDL, err := AdjustSDDLToLocal(FolderSampleSDDL, SampleSDDLPlaceHolder)
+	if err != nil {
+		t.Error(err)
+	}
+
+	RunScenarios(
+		t,
+		eOperation.Copy(), // Sync already shares the root by default.
+		eTestFromTo.Other(common.EFromTo.LocalFile()),
+		eValidate.Auto(),
+		params{
+			recursive:              true,
+			invertedAsSubdir:       true,
+			preserveSMBPermissions: true,
+			preserveSMBInfo:        true,
+		},
+		nil,
+		testFiles{
+			defaultSize: "1K",
+			destTarget:  "newName",
+
+			shouldTransfer: []interface{}{
+				folder("", with{smbAttributes: 2, smbPermissionsSddl: rootSDDL}),
+				f("asdf.txt", with{smbAttributes: 2, smbPermissionsSddl: fileSDDL}),
+				folder("a", with{smbAttributes: 2, smbPermissionsSddl: folderSDDL}),
+				f("a/asdf.txt", with{smbAttributes: 2, smbPermissionsSddl: fileSDDL}),
+				folder("a/b", with{smbAttributes: 2, smbPermissionsSddl: folderSDDL}),
+				f("a/b/asdf.txt", with{smbAttributes: 2, smbPermissionsSddl: fileSDDL}),
+			},
+		},
+		EAccountType.Standard(),
+		"",
+	)
+}

ste/downloader-azureFiles_windows.go

@@ -11,6 +11,8 @@ import (
 	"syscall"
 	"unsafe"
 
+	"github.com/Azure/azure-storage-file-go/azfile"
+
 	"github.com/Azure/azure-storage-azcopy/v10/common"
 	"github.com/Azure/azure-storage-azcopy/v10/sddl"
 	"github.com/Azure/azure-storage-file-go/azfile"
@@ -94,6 +96,12 @@ var globalSetAclMu = &sync.Mutex{}
 func (a *azureFilesDownloader) PutSDDL(sip ISMBPropertyBearingSourceInfoProvider, txInfo TransferInfo) error {
 	// Let's start by getting our SDDL and parsing it.
 	sddlString, err := sip.GetSDDL()
+
+	// There's nothing to set.
+	if sddlString == "" {
+		return nil
+	}
+
 	// TODO: be better at handling these errors.
 	// GetSDDL will fail on a file-level SAS token.
 	if err != nil {

ste/sender-azureFile.go

@@ -272,7 +272,13 @@ func (u *azureFileSenderBase) addPermissionsToHeaders(info TransferInfo, destUrl
 		// If we didn't do the workaround, then let's get the SDDL and put it later.
 		if u.headersToApply.PermissionKey == nil || *u.headersToApply.PermissionKey == "" {
 			pString, err := sddlSIP.GetSDDL()
-			u.headersToApply.PermissionString = &pString
+
+			// Sending "" to the service is invalid, but the service will return it sometimes (e.g. on file shares)
+			// Thus, we'll let the files SDK fill in "inherit" for us, so the service is happy.
+			if pString != "" {
+				u.headersToApply.PermissionString = &pString
+			}
+
 			if err != nil {
 				return "Getting permissions", err
 			}