Skip to content

Commit 4f22e27

Browse files
alloydsaalloydsa
authored
Changes to fix the Everyone ACL when Invalid AD user is passed
1 parent 9a68dc6 commit 4f22e27

File tree

5 files changed

+65
-91
lines changed

5 files changed

+65
-91
lines changed

‎storage_drivers/ontap/api/abstraction_error.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ package api
88
const (
99
ENTRY_DOESNT_EXIST = "4"
1010
DUPLICATE_ENTRY = "1"
11+
INVALID_ENTRY = "655446"
1112
DP_VOLUME_NOT_INITIALIZED = "917536"
1213
SNAPMIRROR_TRANSFER_IN_PROGRESS = "13303812"
1314
SNAPMIRROR_TRANSFER_IN_PROGRESS_BROKEN_OFF = "13303808" // Transition to broken_off state failed. Reason:Another transfer is in progress

‎storage_drivers/ontap/api/ontap_rest.go‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6271,9 +6271,22 @@ func (c *RestClient) SMBShareAccessControlCreate(ctx context.Context, shareName
62716271
result, err := c.api.Nas.CifsShareACLCreate(params, c.authInfo)
62726272
if err != nil {
62736273
if restErr, extractErr := ExtractErrorResponse(ctx, err); extractErr == nil {
6274-
if restErr.Error != nil && restErr.Error.Code != nil && *restErr.Error.Code != DUPLICATE_ENTRY &&
6275-
restErr.Error.Message != nil {
6274+
if restErr.Error != nil && restErr.Error.Code != nil {
6275+
switch *restErr.Error.Code {
6276+
case DUPLICATE_ENTRY:
6277+
Logc(ctx).WithField("userOrGroup", userOrGroup).Debug("Duplicate SMB share ACL entry, ignoring.")
6278+
case INVALID_ENTRY:
6279+
Logc(ctx).WithField("userOrGroup", userOrGroup).Warn("Invalid user or group specified for SMB share access control")
6280+
default:
6281+
if restErr.Error.Message != nil {
6282+
return fmt.Errorf(*restErr.Error.Message)
6283+
}
6284+
return fmt.Errorf("ONTAP REST API error code %v with no message", *restErr.Error.Code)
6285+
}
6286+
} else if restErr.Error != nil && restErr.Error.Message != nil {
62766287
return fmt.Errorf(*restErr.Error.Message)
6288+
} else {
6289+
return fmt.Errorf("unknown ONTAP REST API error")
62776290
}
62786291
} else {
62796292
return err

‎storage_drivers/ontap/api/ontap_zapi.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3369,6 +3369,9 @@ func (c Client) SMBShareAccessControlCreate(shareName string,
33693369
ExecuteUsing(c.zr)
33703370
if err != nil {
33713371
return nil, err
3372+
} else if response.Result.ResultErrnoAttr == azgo.EAPIERROR {
3373+
Logc(context.Background()).WithField("userOrGroup",
3374+
userOrGroup).Warn("Invalid user or group specified for SMB share access control")
33723375
} else if response.Result.ResultStatusAttr != "passed" && response.Result.ResultErrnoAttr != azgo.EDUPLICATEENTRY {
33733376
return nil, fmt.Errorf("failed to set SMB share ACL: %s", response.Result)
33743377
}

‎storage_drivers/ontap/ontap_nas.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1863,12 +1863,12 @@ func (d *NASStorageDriver) EnsureSMBShare(
18631863

18641864
// If secure SMB is enabled, configure access control
18651865
if secureSMBEnabled {
1866-
if err = d.API.SMBShareAccessControlCreate(ctx, shareName, smbShareACL); err != nil {
1866+
// Delete the Everyone Access Control created by default during share creation by ONTAP
1867+
if err = d.API.SMBShareAccessControlDelete(ctx, shareName, smbShareDeleteACL); err != nil {
18671868
return err
18681869
}
18691870

1870-
// Delete the Everyone Access Control created by default during share creation by ONTAP
1871-
if err = d.API.SMBShareAccessControlDelete(ctx, shareName, smbShareDeleteACL); err != nil {
1871+
if err = d.API.SMBShareAccessControlCreate(ctx, shareName, smbShareACL); err != nil {
18721872
return err
18731873
}
18741874
}

‎storage_drivers/ontap/ontap_nas_test.go‎

Lines changed: 43 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -1595,9 +1595,11 @@ func TestOntapNasStorageDriverVolumeClone_SecureSMBAccessControlCreatefail(t *te
15951595
mockAPI.EXPECT().VolumeSetComment(ctx, volConfig.InternalName, volConfig.InternalName, "{\"provisioning\":{\"type\":\"clone\"}}").Return(nil)
15961596
mockAPI.EXPECT().VolumeMount(ctx, volConfig.InternalName, "/"+volConfig.InternalName).Return(nil)
15971597
mockAPI.EXPECT().VolumeModifyExportPolicy(ctx, volConfig.InternalName, "").Return(nil)
1598-
mockAPI.EXPECT().SMBShareExists(ctx, "").Return(false, nil)
1599-
mockAPI.EXPECT().SMBShareCreate(ctx, "", "/").Return(nil)
1600-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "", volConfig.SMBShareACL).Return(fmt.Errorf("cannot create SMB Share Access Control rule"))
1598+
mockAPI.EXPECT().SMBShareExists(ctx, volConfig.InternalName).Return(false, nil)
1599+
mockAPI.EXPECT().SMBShareCreate(ctx, volConfig.InternalName, "/"+volConfig.InternalName).Return(nil)
1600+
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, volConfig.InternalName, smbShareDeleteACL).Return(nil)
1601+
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, volConfig.InternalName, volConfig.SMBShareACL).
1602+
Return(fmt.Errorf("cannot create volume"))
16011603

16021604
result := driver.CreateClone(ctx, nil, volConfig, pool1)
16031605

@@ -1643,10 +1645,9 @@ func TestOntapNasStorageDriverVolumeClone_SecureSMBAccessControlDeletefail(t *te
16431645
mockAPI.EXPECT().VolumeMount(ctx, volConfig.InternalName, "/"+volConfig.InternalName).Return(nil)
16441646
mockAPI.EXPECT().VolumeModifyExportPolicy(ctx, volConfig.InternalName, "").Return(nil)
16451647

1646-
mockAPI.EXPECT().SMBShareExists(ctx, "").Return(false, nil)
1647-
mockAPI.EXPECT().SMBShareCreate(ctx, "", "/").Return(nil)
1648-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "", volConfig.SMBShareACL).Return(nil)
1649-
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, "", smbShareDeleteACL).Return(fmt.Errorf("cannot delete SMB Share Access Control rule"))
1648+
mockAPI.EXPECT().SMBShareExists(ctx, volConfig.InternalName).Return(false, nil)
1649+
mockAPI.EXPECT().SMBShareCreate(ctx, volConfig.InternalName, "/"+volConfig.InternalName).Return(nil)
1650+
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, volConfig.InternalName, smbShareDeleteACL).Return(fmt.Errorf("cannot create volume"))
16501651

16511652
result := driver.CreateClone(ctx, nil, volConfig, pool1)
16521653

@@ -1686,10 +1687,11 @@ func TestOntapNasStorageDriverVolumeClone_ROCloneSecureSMBAccessControlCreateFai
16861687
mockAPI.EXPECT().SVMName().AnyTimes().Return("SVM1")
16871688
mockAPI.EXPECT().VolumeInfo(ctx, volConfig.CloneSourceVolumeInternal).Return(&flexVol, nil)
16881689

1689-
mockAPI.EXPECT().SMBShareExists(ctx, "").Return(false, nil)
1690-
mockAPI.EXPECT().SMBShareCreate(ctx, "", "/").Return(nil)
1691-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "", volConfig.SMBShareACL).
1692-
Return(fmt.Errorf("cannot create SMB Share Access Control rule"))
1690+
mockAPI.EXPECT().SMBShareExists(ctx, volConfig.InternalName).Return(false, nil)
1691+
mockAPI.EXPECT().SMBShareCreate(ctx, volConfig.InternalName, "/"+volConfig.InternalName).Return(nil)
1692+
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, volConfig.InternalName, smbShareDeleteACL).Return(nil)
1693+
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, volConfig.InternalName, volConfig.SMBShareACL).
1694+
Return(fmt.Errorf("cannot create volume"))
16931695

16941696
result := driver.CreateClone(ctx, nil, volConfig, pool1)
16951697

@@ -1728,10 +1730,9 @@ func TestOntapNasStorageDriverVolumeClone_ROCloneSecureSMBAccessControlDeleteFai
17281730

17291731
mockAPI.EXPECT().SVMName().AnyTimes().Return("SVM1")
17301732
mockAPI.EXPECT().VolumeInfo(ctx, volConfig.CloneSourceVolumeInternal).Return(&flexVol, nil)
1731-
mockAPI.EXPECT().SMBShareExists(ctx, "").Return(false, nil)
1732-
mockAPI.EXPECT().SMBShareCreate(ctx, "", "/").Return(nil)
1733-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "", volConfig.SMBShareACL).Return(nil)
1734-
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, "", smbShareDeleteACL).Return(fmt.Errorf("cannot delete SMB Share Access Control rule"))
1733+
mockAPI.EXPECT().SMBShareExists(ctx, volConfig.InternalName).Return(false, nil)
1734+
mockAPI.EXPECT().SMBShareCreate(ctx, volConfig.InternalName, "/"+volConfig.InternalName).Return(nil)
1735+
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, volConfig.InternalName, smbShareDeleteACL).Return(fmt.Errorf("cannot create volume"))
17351736

17361737
result := driver.CreateClone(ctx, nil, volConfig, pool1)
17371738

@@ -4666,43 +4667,22 @@ func TestOntapNasStorageDriverVolumeCreate_SecureSMBAccessControlCreatefail(t *t
46664667
driver.Config.NASType = sa.SMB
46674668
volAttrs := map[string]sa.Request{}
46684669

4669-
tests := []struct {
4670-
smbShare string
4671-
}{
4672-
{"vol1"},
4673-
{""},
4674-
}
4675-
4676-
for _, test := range tests {
4677-
t.Run(test.smbShare, func(t *testing.T) {
4678-
driver.Config.SMBShare = test.smbShare
4679-
4680-
mockAPI.EXPECT().ExportPolicyCreate(ctx, "test_empty").Return(nil)
4681-
mockAPI.EXPECT().SVMName().AnyTimes().Return("fakesvm")
4682-
mockAPI.EXPECT().VolumeExists(ctx, "vol1").Return(false, nil)
4683-
mockAPI.EXPECT().GetSVMPeers(ctx).Return([]string{"fakesvm2"}, nil)
4684-
mockAPI.EXPECT().TieringPolicyValue(ctx).Return("none")
4685-
mockAPI.EXPECT().VolumeCreate(ctx, gomock.Any()).Return(nil)
4686-
mockAPI.EXPECT().VolumeMount(ctx, "vol1", "/vol1").Return(nil)
4687-
4688-
if test.smbShare != "" {
4689-
mockAPI.EXPECT().SMBShareExists(ctx, "vol1").Return(false, nil)
4690-
mockAPI.EXPECT().SMBShareCreate(ctx, "vol1", "/vol1").Return(nil)
4691-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "vol1", volConfig.SMBShareACL).
4692-
Return(fmt.Errorf("cannot create volume"))
4693-
4694-
} else {
4695-
mockAPI.EXPECT().SMBShareExists(ctx, "vol1").Return(false, nil)
4696-
mockAPI.EXPECT().SMBShareCreate(ctx, "vol1", "/vol1").Return(nil)
4697-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "vol1", volConfig.SMBShareACL).
4698-
Return(fmt.Errorf("cannot create volume"))
4699-
}
4670+
mockAPI.EXPECT().ExportPolicyCreate(ctx, "test_empty").Return(nil)
4671+
mockAPI.EXPECT().SVMName().AnyTimes().Return("fakesvm")
4672+
mockAPI.EXPECT().VolumeExists(ctx, "vol1").Return(false, nil)
4673+
mockAPI.EXPECT().GetSVMPeers(ctx).Return([]string{"fakesvm2"}, nil)
4674+
mockAPI.EXPECT().TieringPolicyValue(ctx).Return("none")
4675+
mockAPI.EXPECT().VolumeCreate(ctx, gomock.Any()).Return(nil)
4676+
mockAPI.EXPECT().VolumeMount(ctx, "vol1", "/vol1").Return(nil)
4677+
mockAPI.EXPECT().SMBShareExists(ctx, "vol1").Return(false, nil)
4678+
mockAPI.EXPECT().SMBShareCreate(ctx, "vol1", "/vol1").Return(nil)
4679+
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, "vol1", smbShareDeleteACL).Return(nil)
4680+
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "vol1", volConfig.SMBShareACL).
4681+
Return(fmt.Errorf("cannot create volume"))
47004682

4701-
result := driver.Create(ctx, volConfig, pool1, volAttrs)
4683+
result := driver.Create(ctx, volConfig, pool1, volAttrs)
47024684

4703-
assert.Error(t, result)
4704-
})
4705-
}
4685+
assert.Error(t, result)
47064686
}
47074687

47084688
func TestOntapNasStorageDriverVolumeCreate_SecureSMBAccessControlDeletefail(t *testing.T) {
@@ -4739,43 +4719,20 @@ func TestOntapNasStorageDriverVolumeCreate_SecureSMBAccessControlDeletefail(t *t
47394719
volAttrs := map[string]sa.Request{}
47404720
smbShareDeleteACL := map[string]string{"Everyone": "windows"}
47414721

4742-
tests := []struct {
4743-
smbShare string
4744-
}{
4745-
{"vol1"},
4746-
{""},
4747-
}
4748-
4749-
for _, test := range tests {
4750-
t.Run(test.smbShare, func(t *testing.T) {
4751-
driver.Config.SMBShare = test.smbShare
4752-
4753-
mockAPI.EXPECT().ExportPolicyCreate(ctx, "test_empty").Return(nil)
4754-
mockAPI.EXPECT().SVMName().AnyTimes().Return("fakesvm")
4755-
mockAPI.EXPECT().VolumeExists(ctx, "vol1").Return(false, nil)
4756-
mockAPI.EXPECT().GetSVMPeers(ctx).Return([]string{"fakesvm2"}, nil)
4757-
mockAPI.EXPECT().TieringPolicyValue(ctx).Return("none")
4758-
mockAPI.EXPECT().VolumeCreate(ctx, gomock.Any()).Return(nil)
4759-
mockAPI.EXPECT().VolumeMount(ctx, "vol1", "/vol1").Return(nil)
4760-
4761-
if test.smbShare != "" {
4762-
mockAPI.EXPECT().SMBShareExists(ctx, "vol1").Return(false, nil)
4763-
mockAPI.EXPECT().SMBShareCreate(ctx, "vol1", "/vol1").Return(nil)
4764-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "vol1", volConfig.SMBShareACL).Return(nil)
4765-
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, "vol1", smbShareDeleteACL).Return(fmt.Errorf("cannot create volume"))
4766-
4767-
} else {
4768-
mockAPI.EXPECT().SMBShareExists(ctx, "vol1").Return(false, nil)
4769-
mockAPI.EXPECT().SMBShareCreate(ctx, "vol1", "/vol1").Return(nil)
4770-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, "vol1", volConfig.SMBShareACL).Return(nil)
4771-
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, "vol1", smbShareDeleteACL).Return(fmt.Errorf("cannot create volume"))
4772-
}
4722+
mockAPI.EXPECT().ExportPolicyCreate(ctx, "test_empty").Return(nil)
4723+
mockAPI.EXPECT().SVMName().AnyTimes().Return("fakesvm")
4724+
mockAPI.EXPECT().VolumeExists(ctx, "vol1").Return(false, nil)
4725+
mockAPI.EXPECT().GetSVMPeers(ctx).Return([]string{"fakesvm2"}, nil)
4726+
mockAPI.EXPECT().TieringPolicyValue(ctx).Return("none")
4727+
mockAPI.EXPECT().VolumeCreate(ctx, gomock.Any()).Return(nil)
4728+
mockAPI.EXPECT().VolumeMount(ctx, "vol1", "/vol1").Return(nil)
4729+
mockAPI.EXPECT().SMBShareExists(ctx, "vol1").Return(false, nil)
4730+
mockAPI.EXPECT().SMBShareCreate(ctx, "vol1", "/vol1").Return(nil)
4731+
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, "vol1", smbShareDeleteACL).Return(fmt.Errorf("cannot create volume"))
47734732

4774-
result := driver.Create(ctx, volConfig, pool1, volAttrs)
4733+
result := driver.Create(ctx, volConfig, pool1, volAttrs)
47754734

4776-
assert.Error(t, result)
4777-
})
4778-
}
4735+
assert.Error(t, result)
47794736
}
47804737

47814738
func TestOntapNasStorageDriverVolumeImport(t *testing.T) {
@@ -5274,6 +5231,7 @@ func TestOntapNasStorageDriverVolumeImport_SecureSMBAccessControlCreatefail(t *t
52745231
mockAPI.EXPECT().VolumeModifyUnixPermissions(ctx, "vol2", "vol1", DefaultUnixPermissions).Return(nil)
52755232
mockAPI.EXPECT().SMBShareExists(ctx, volConfig.InternalName).Return(false, nil)
52765233
mockAPI.EXPECT().SMBShareCreate(ctx, volConfig.InternalName, flexVol.JunctionPath).Return(nil)
5234+
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, volConfig.InternalName, smbShareDeleteACL).Return(nil)
52775235
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, volConfig.InternalName,
52785236
volConfig.SMBShareACL).Return(fmt.Errorf("cannot create SMB Share Access Control rule"))
52795237
result := driver.Import(ctx, volConfig, "vol1")
@@ -5306,7 +5264,6 @@ func TestOntapNasStorageDriverVolumeImport_SecureSMBAccessControlDeletefail(t *t
53065264
mockAPI.EXPECT().VolumeModifyUnixPermissions(ctx, "vol2", "vol1", DefaultUnixPermissions).Return(nil)
53075265
mockAPI.EXPECT().SMBShareExists(ctx, volConfig.InternalName).Return(false, nil)
53085266
mockAPI.EXPECT().SMBShareCreate(ctx, volConfig.InternalName, flexVol.JunctionPath).Return(nil)
5309-
mockAPI.EXPECT().SMBShareAccessControlCreate(ctx, volConfig.InternalName, volConfig.SMBShareACL).Return(nil)
53105267
mockAPI.EXPECT().SMBShareAccessControlDelete(ctx, volConfig.InternalName,
53115268
smbShareDeleteACL).Return(fmt.Errorf("cannot delete SMB Share Access Control rule"))
53125269
result := driver.Import(ctx, volConfig, "vol1")

0 commit comments

Comments
 (0)