Skip to content

Commit

Permalink
SNOW-1706569: Fix file permissions (#1089)
Browse files Browse the repository at this point in the history
  • Loading branch information
sfc-gh-pbulawa authored Jan 29, 2025
1 parent cab672d commit 89d91e8
Show file tree
Hide file tree
Showing 23 changed files with 699 additions and 186 deletions.
7 changes: 6 additions & 1 deletion Snowflake.Data.Tests/IntegrationTests/MaxLobSizeIT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Data.Common;
using System.IO;
using System.Linq;
using Snowflake.Data.Core.Tools;

namespace Snowflake.Data.Tests.IntegrationTests
{
Expand Down Expand Up @@ -301,7 +302,11 @@ void PrepareTest()
t_inputFilePath = Path.GetTempPath() + t_fileName;

var data = $"{t_colData[0]},{t_colData[1]},{t_colData[2]}";
File.WriteAllText(t_inputFilePath, data);
using (var stream = FileOperations.Instance.Create(t_inputFilePath))
using (var writer = new StreamWriter(stream))
{
writer.Write(data);
}

t_outputFilePath = $@"{s_outputDirectory}/{t_fileName}";
t_filesToDelete.Add(t_inputFilePath);
Expand Down
8 changes: 7 additions & 1 deletion Snowflake.Data.Tests/IntegrationTests/SFPutGetTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Data.Common;
using System.IO.Compression;
using System.Text;
using Snowflake.Data.Core.Tools;
using Snowflake.Data.Tests.Util;

namespace Snowflake.Data.Tests.IntegrationTests
Expand Down Expand Up @@ -807,7 +808,12 @@ private static void PrepareFileData(string file)
var rawDataRow = string.Join(",", s_colData) + "\n";
var rawData = string.Concat(Enumerable.Repeat(rawDataRow, NumberOfRows));

File.WriteAllText(file, rawData);

using (var stream = FileOperations.Instance.Create(file))
using (var writer = new StreamWriter(stream))
{
writer.Write(rawData);
}
t_filesToDelete.Add(file);
}

Expand Down
39 changes: 39 additions & 0 deletions Snowflake.Data.Tests/Mock/MockUnixOperations.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
using Mono.Unix;
using Snowflake.Data.Core.Tools;

namespace Snowflake.Data.Tests.Mock
{
internal class MockUnixOperations : UnixOperations
{
public long CurrentUserId { get; set; } = 0;
public long FileOwnerId { get; set; } = 0;
public long DirectoryOwnerId { get; set; } = 0;
public bool DirectoryPermissionsReturn { get; set; } = false;
public bool FilePermissionsReturn { get; set; } = false;

public override bool CheckDirectoryHasAnyOfPermissions(string path, FileAccessPermissions permissions)
{
return DirectoryPermissionsReturn;
}

public override bool CheckFileHasAnyOfPermissions(string path, FileAccessPermissions permissions)
{
return FilePermissionsReturn;
}

public override long GetCurrentUserId()
{
return CurrentUserId;
}

public override long GetOwnerIdOfDirectory(string path)
{
return DirectoryOwnerId;
}

public override long GetOwnerIdOfFile(string path)
{
return FileOwnerId;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.IO;
using System.Security;
using Mono.Unix;
using Mono.Unix.Native;
using Moq;
using NUnit.Framework;
using Snowflake.Data.Core.CredentialManager.Infrastructure;
Expand Down Expand Up @@ -66,8 +65,8 @@ public void TestThatThrowsErrorWhenCacheFailToCreateCacheFile()
.Returns(false);
t_unixOperations
.Setup(u => u.CreateFileWithPermissions(s_customJsonPath,
FilePermissions.S_IRUSR | FilePermissions.S_IWUSR))
.Returns(-1);
FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite))
.Throws<IOException>();
t_environmentOperations
.Setup(e => e.GetEnvironmentVariable(SFCredentialManagerFileStorage.CredentialCacheDirectoryEnvironmentName))
.Returns(CustomJsonDir);
Expand All @@ -83,6 +82,9 @@ public void TestThatThrowsErrorWhenCacheFailToCreateCacheFile()
t_directoryOperations
.Setup(d => d.GetDirectoryInfo(s_customLockPath))
.Returns(new DirectoryInformation(false, null));
t_unixOperations
.Setup(u => u.CreateDirectoryWithPermissionsMkdir(s_customLockPath, FileAccessPermissions.UserRead))
.Returns(0);
_credentialManager = new SFCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object);

// act
Expand All @@ -104,7 +106,7 @@ public void TestThatThrowsErrorWhenCacheFileCanBeAccessedByOthers()
try
{
DirectoryOperations.Instance.CreateDirectory(tempDirectory);
UnixOperations.Instance.CreateFileWithPermissions(Path.Combine(tempDirectory, SFCredentialManagerFileStorage.CredentialCacheFileName), FilePermissions.ALLPERMS);
UnixOperations.Instance.CreateFileWithPermissions(Path.Combine(tempDirectory, SFCredentialManagerFileStorage.CredentialCacheFileName), FileAccessPermissions.AllPermissions);

// act
var thrown = Assert.Throws<SecurityException>(() => _credentialManager.SaveCredentials("key", "token"));
Expand All @@ -124,8 +126,7 @@ public void TestThatJsonFileIsCheckedIfAlreadyExists()
// arrange
t_unixOperations
.Setup(u => u.CreateFileWithPermissions(s_customJsonPath,
FilePermissions.S_IRUSR | FilePermissions.S_IWUSR))
.Returns(0);
FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite));
t_unixOperations
.Setup(u => u.GetFilePermissions(s_customJsonPath))
.Returns(FileAccessPermissions.UserRead | FileAccessPermissions.UserWrite);
Expand Down Expand Up @@ -184,7 +185,7 @@ public void TestWritingIsUnavailableIfFailedToCreateDirLock()
.Setup(u => u.GetCurrentUserId())
.Returns(UserId);
t_unixOperations
.Setup(u => u.CreateDirectoryWithPermissions(s_customLockPath, SFCredentialManagerFileImpl.CredentialCacheLockDirPermissions))
.Setup(u => u.CreateDirectoryWithPermissionsMkdir(s_customLockPath, SFCredentialManagerFileImpl.CredentialCacheLockDirPermissions))
.Returns(-1);
_credentialManager = new SFCredentialManagerFileImpl(t_fileOperations.Object, t_directoryOperations.Object, t_unixOperations.Object, t_environmentOperations.Object);

Expand All @@ -210,7 +211,7 @@ public void TestReadingIsUnavailableIfFailedToCreateDirLock()
.Returns(false)
.Returns(true);
t_unixOperations
.Setup(u => u.CreateDirectoryWithPermissions(s_customLockPath, SFCredentialManagerFileImpl.CredentialCacheLockDirPermissions))
.Setup(u => u.CreateDirectoryWithPermissionsMkdir(s_customLockPath, SFCredentialManagerFileImpl.CredentialCacheLockDirPermissions))
.Returns(-1);
t_directoryOperations
.Setup(d => d.GetParentDirectoryInfo(CustomJsonDir))
Expand Down
16 changes: 9 additions & 7 deletions Snowflake.Data.Tests/UnitTests/Logger/EasyLoggingStarterTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System.IO;
using System.Runtime.InteropServices;
using Mono.Unix;
using Mono.Unix.Native;
using Moq;
using NUnit.Framework;
using Snowflake.Data.Configuration;
Expand Down Expand Up @@ -157,7 +156,7 @@ public void TestThatDoesNotFailWhenLogDirectoryPermissionIsNot700()

// assert
t_unixOperations.Verify(u => u.CreateDirectoryWithPermissions(s_expectedLogPath,
FilePermissions.S_IRUSR | FilePermissions.S_IWUSR | FilePermissions.S_IXUSR), Times.Never);
FileAccessPermissions.UserReadWriteExecute), Times.Never);
}

[Test]
Expand All @@ -172,9 +171,12 @@ public void TestFailIfDirectoryCreationFails()
t_easyLoggingProvider
.Setup(provider => provider.ProvideConfig(ConfigPath))
.Returns(s_configWithErrorLevel);
t_directoryOperations
.Setup(d => d.CreateDirectory(s_expectedLogPath))
.Throws(() => new Exception("Unable to create directory"));
t_unixOperations
.Setup(u => u.CreateDirectoryWithPermissions(s_expectedLogPath, FilePermissions.S_IRUSR | FilePermissions.S_IWUSR | FilePermissions.S_IXUSR))
.Returns((int)Errno.EPERM);
.Setup(u => u.CreateDirectoryWithPermissions(s_expectedLogPath, FileAccessPermissions.UserReadWriteExecute))
.Throws(() => new Exception("Unable to create directory"));

// act
var thrown = Assert.Throws<Exception>(() => t_easyLoggerStarter.Init(ConfigPath));
Expand Down Expand Up @@ -208,7 +210,7 @@ public void TestThatConfiguresEasyLoggingOnlyOnceWhenInitializedWithConfigPath()
else
{
t_unixOperations.Verify(u => u.CreateDirectoryWithPermissions(s_expectedLogPath,
FilePermissions.S_IRUSR | FilePermissions.S_IWUSR | FilePermissions.S_IXUSR), Times.Once);
FileAccessPermissions.UserReadWriteExecute), Times.Once);
}
t_easyLoggerManager.Verify(manager => manager.ReconfigureEasyLogging(EasyLoggingLogLevel.Error, s_expectedLogPath), Times.Once);

Expand Down Expand Up @@ -241,7 +243,7 @@ public void TestThatConfiguresEasyLoggingOnlyOnceForInitializationsWithoutConfig
else
{
t_unixOperations.Verify(u => u.CreateDirectoryWithPermissions(s_expectedLogPath,
FilePermissions.S_IRUSR | FilePermissions.S_IWUSR | FilePermissions.S_IXUSR), Times.Once);
FileAccessPermissions.UserReadWriteExecute), Times.Once);
}
t_easyLoggerManager.Verify(manager => manager.ReconfigureEasyLogging(EasyLoggingLogLevel.Error, s_expectedLogPath), Times.Once);
}
Expand All @@ -268,7 +270,7 @@ public void TestThatReconfiguresEasyLoggingWithConfigPathIfNotGivenForTheFirstTi
else
{
t_unixOperations.Verify(u => u.CreateDirectoryWithPermissions(s_expectedLogPath,
FilePermissions.S_IRUSR | FilePermissions.S_IWUSR | FilePermissions.S_IXUSR), Times.Once);
FileAccessPermissions.UserReadWriteExecute), Times.Once);
}
t_easyLoggerManager.Verify(manager => manager.ReconfigureEasyLogging(EasyLoggingLogLevel.Error, s_expectedLogPath), Times.Once);

Expand Down
4 changes: 2 additions & 2 deletions Snowflake.Data.Tests/UnitTests/SFAzureClientTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ public void TestDownloadFile(HttpStatusCode httpStatusCode, ResultStatus expecte
.Returns<string>((blobName) =>
{
var mockBlobClient = new Mock<BlobClient>();
mockBlobClient.Setup(client => client.DownloadTo(It.IsAny<string>()))
mockBlobClient.Setup(client => client.DownloadTo(It.IsAny<Stream>()))
.Returns(() =>
{
if (key == HttpStatusCode.OK.ToString())
Expand Down Expand Up @@ -350,7 +350,7 @@ public async Task TestDownloadFileAsync(HttpStatusCode httpStatusCode, ResultSta
.Returns<string>((blobName) =>
{
var mockBlobClient = new Mock<BlobClient>();
mockBlobClient.Setup(client => client.DownloadToAsync(It.IsAny<string>(), It.IsAny<CancellationToken>()))
mockBlobClient.Setup(client => client.DownloadToAsync(It.IsAny<Stream>(), It.IsAny<CancellationToken>()))
.Returns(async () =>
{
if (key == HttpStatusCode.OK.ToString())
Expand Down
1 change: 1 addition & 0 deletions Snowflake.Data.Tests/UnitTests/SFBindUploaderTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
namespace Snowflake.Data.Tests.UnitTests
{
[TestFixture]
[SetCulture("en-US")]
class SFBindUploaderTest
{
private readonly SFBindUploader _bindUploader = new SFBindUploader(null, "test");
Expand Down
123 changes: 123 additions & 0 deletions Snowflake.Data.Tests/UnitTests/Tools/DirectoryOperationsTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
using System.Collections.Generic;
using System.IO;
using Mono.Unix;
using Mono.Unix.Native;
using NUnit.Framework;
using Snowflake.Data.Core.Tools;
using Snowflake.Data.Tests.Mock;

namespace Snowflake.Data.Tests.UnitTests.Tools
{
[TestFixture, NonParallelizable]
public class DirectoryOperationsTest
{
private static DirectoryOperations s_directoryOperations;
private static readonly string s_relativeWorkingDirectory = $"directory_operations_test_{Path.GetRandomFileName()}";
private static readonly string s_workingDirectory = Path.Combine(TempUtil.GetTempPath(), s_relativeWorkingDirectory);
private static readonly string s_dirName = "testdir";
private static readonly string s_dirAbsolutePath = Path.Combine(s_workingDirectory, s_dirName);

[SetUp]
public static void Before()
{
if (!Directory.Exists(s_workingDirectory))
{
Directory.CreateDirectory(s_workingDirectory);
}

s_directoryOperations = new DirectoryOperations();
}

[TearDown]
public static void After()
{
Directory.Delete(s_workingDirectory, true);
}

[Test]
[Platform("Win")]
public void TestDirectoryIsSafeOnWindows()
{
// arrange
var absoluteFilePath = Path.Combine(s_workingDirectory, s_dirName);
Directory.CreateDirectory(absoluteFilePath);

// act and assert
Assert.IsTrue(s_directoryOperations.IsDirectorySafe(absoluteFilePath));
}

[Test]
[Platform(Exclude = "Win")]
public void TestDirectoryIsNotSafeOnNotWindowsWhenPermissionsAreTooBroad(
[ValueSource(nameof(InsecurePermissions))]
FileAccessPermissions permissions)
{
// arrange
Syscall.mkdir(s_dirAbsolutePath, (FilePermissions)permissions);

// act and assert
Assert.IsFalse(s_directoryOperations.IsDirectorySafe(s_dirAbsolutePath));
}

[Test]
public void TestShouldCreateDirectoryWithSafePermissions()
{
// act
s_directoryOperations.CreateDirectory(s_dirAbsolutePath);

// assert
Assert.IsTrue(Directory.Exists(s_dirAbsolutePath));
Assert.IsTrue(s_directoryOperations.IsDirectorySafe(s_dirAbsolutePath));
}

[Test]
[Platform(Exclude = "Win")]
public void TestOwnerIsCurrentUser()
{
// arrange
var mockUnixOperations = new MockUnixOperations{ CurrentUserId = 1, DirectoryOwnerId = 1};
var dirOps = new DirectoryOperations(mockUnixOperations);

// act and assert
Assert.IsTrue(dirOps.IsDirectoryOwnedByCurrentUser(s_dirAbsolutePath));
}

[Test]
[Platform(Exclude = "Win")]
public void TestOwnerIsNotCurrentUser()
{
// arrange
var mockUnixOperations = new MockUnixOperations{ CurrentUserId = 1, DirectoryOwnerId = 2};
var dirOps = new DirectoryOperations(mockUnixOperations);

// act and assert
Assert.IsFalse(dirOps.IsDirectoryOwnedByCurrentUser(s_dirAbsolutePath));
}

[Test]
[Platform(Exclude = "Win")]
public void TestDirectoryIsNotSecureWhenNotOwnedByCurrentUser()
{
// arrange
var mockUnixOperations = new MockUnixOperations{ CurrentUserId = 1, DirectoryOwnerId = 2};
var dirOps = new DirectoryOperations(mockUnixOperations);

// act and assert
Assert.IsFalse(dirOps.IsDirectorySafe(s_dirAbsolutePath));
}

// User permissions are required for all of the tests to be able to access directory information
public static IEnumerable<FileAccessPermissions> InsecurePermissions()
{
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.GroupRead;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.GroupRead | FileAccessPermissions.GroupWrite;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.GroupExecute;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.GroupReadWriteExecute;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.OtherRead;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.OtherRead | FileAccessPermissions.OtherWrite;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.OtherExecute;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.OtherReadWriteExecute;
yield return FileAccessPermissions.UserReadWriteExecute | FileAccessPermissions.AllPermissions;
}
}
}
Loading

0 comments on commit 89d91e8

Please # to comment.