Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid adding duplicate table prefix into the same database #11846

Closed
wants to merge 6 commits into from
Closed

Avoid adding duplicate table prefix into the same database #11846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
49c4286
Avoid adding duplicate table prefix into the same database
hishamco Jun 10, 2022
70b8ceb
Fix NRE
hishamco Jun 10, 2022
246e865
Use indexer
hishamco Jun 10, 2022
71929cb
Check againts the connection string too
hishamco Jun 11, 2022
574d5d0
Don't filter unless the tenant to be added has a connection string
hishamco Jun 11, 2022
c5c8197
Add unit tests
hishamco Jun 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Tenants/Services/TenantValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,29 @@ public async Task<IEnumerable<ModelError>> ValidateAsync(TenantViewModel model)
errors.Add(new ModelError(nameof(model.Name), S["A tenant with the same name already exists."]));
}

var allOtherShells = allSettings.Where(t => !String.Equals(t.Name, model.Name, StringComparison.OrdinalIgnoreCase));
var allOtherShells = allSettings.Where(tenant => !String.Equals(tenant.Name, model.Name, StringComparison.OrdinalIgnoreCase));

if (allOtherShells.Any(tenant => String.Equals(tenant.RequestUrlPrefix, model.RequestUrlPrefix?.Trim(), StringComparison.OrdinalIgnoreCase) && DoesUrlHostExist(tenant.RequestUrlHost, model.RequestUrlHost)))
{
errors.Add(new ModelError(nameof(model.RequestUrlPrefix), S["A tenant with the same host and prefix already exists."]));
}

if (selectedProvider.HasConnectionString)
{
var allOtherShellsHaveConnectionString = allOtherShells
.Where(tenant => !String.IsNullOrEmpty(tenant.ShellConfiguration?["ConnectionString"]))
.ToList();

if (allOtherShellsHaveConnectionString.Any() &&
allOtherShellsHaveConnectionString.Any(
tenant => String.Equals(model.ConnectionString, tenant.ShellConfiguration["ConnectionString"], StringComparison.OrdinalIgnoreCase) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hishamco
I don't think this is a good check. For example, look a the following variation of SQL-Server strings

Data Source=localhost;Database=Orchard;Integrated Security=True;
Server=localhost;Database=Orchard;Integrated Security=True;
Server=localhost;Database=Orchard;User Id=username1;Password=password
User Id=username1;Server=localhost;Database=Orchard;Password=password

All the above should be treated the same since they point to the same exact thing when looking at the combination of 3 (Provider, Server, and Database) + prefix.

The way to do it here is to let the provider parse the connection string and give you a common DbConnection object. Like create IDbConnectionFactory that would return a DbConnection from a given screen and then use the common info in the DbConnection to create a unique key for each connection and see if these exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already provider a sample for each connection string ;) nothing but I can handle this easliy if we need to support such variations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think that is enough check. Not everyone follow the sample string and even then, sometimes people use different username to to connect to the same database.

Do keep in mind that my original PR was just to inspect all connections string in the configuration "after paring them". But during the last meeting, @sebastienros suggest that we take it one step further and actually validation the database connection to make sure the the tables do not physically exists on the server. So if the recommendation not to do connection validation, I can tweak the PR #11822 and eliminate the physical connection validation step and just reply on the connection settings like how the PR original started. But I personally like @sebastienros recommendation which is why I tooks the PR one step further.

I would suggest we wait and see more feedback from @jtkech , @Skrypt and @agriffard so we are not wasting time making changes into both placed. Meanwhile I suggest you tag this PR with do not merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in other PR, let your check the table existance to avoid invalid serial instead of table prefix issue, this way it's clear for all what's going on

String.Equals(model.TablePrefix, tenant.ShellConfiguration["TablePrefix"], StringComparison.OrdinalIgnoreCase))
)
{
errors.Add(new ModelError(nameof(model.TablePrefix), S["A tenant with the same connection string and table prefix already exists."]));
}
}

return errors;
}

Expand Down
62 changes: 59 additions & 3 deletions test/OrchardCore.Tests/Modules/OrchardCore.Tenants/Services/TenantValidatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public async Task TenantValidationFailsIfInvalidConfigurationsWasProvided(string
RequestUrlPrefix = urlPrefix,
RequestUrlHost = hostName,
FeatureProfile = featureProfile,
DatabaseProvider = "Sqlite",
IsNewTenant = true
};

Expand Down Expand Up @@ -76,6 +77,7 @@ public async Task DuplicateTenantHostOrPrefixShouldFailValidation(bool isNewTena
RequestUrlPrefix = "tenant4",
RequestUrlHost = "example5.com",
FeatureProfile = "Feature Profile",
DatabaseProvider = "Sqlite",
IsNewTenant = isNewTenant
};

Expand All @@ -87,6 +89,41 @@ public async Task DuplicateTenantHostOrPrefixShouldFailValidation(bool isNewTena
Assert.Equal("A tenant with the same host and prefix already exists.", errors.Single().Message);
}

[Theory]
[InlineData("Tenant10", "tenant10", "Server=.;Database=OrchardCore1;User Id=oc;Password=admin@OC", "OC", "A tenant with the same connection string and table prefix already exists.")]
[InlineData("Tenant20", "tenant20", "Server=.;Database=OrchardCore2;User Id=oc;Password=admin@OC", "OC", "")]
[InlineData("Tenant20", "tenant30", "Server=.;Database=OrchardCore1;User Id=oc;Password=admin@OC", "OCC", "")]
public async Task DuplicateConnectionStringAndTablePrefixShouldFailValidation(string name, string urlPrefix, string connectionString, string tablePrefix, string errorMessage)
{
// Arrange
var tenantValidator = CreateTenantValidator();

var viewModel = new EditTenantViewModel
{
Name = name,
RequestUrlPrefix = urlPrefix,
FeatureProfile = "Feature Profile",
ConnectionString = connectionString,
TablePrefix = tablePrefix,
DatabaseProvider = "SqlConnection",
IsNewTenant = true
};

// Act
var errors = await tenantValidator.ValidateAsync(viewModel);

// Asserts
if (errorMessage == String.Empty)
{
Assert.False(errors.Any());
}
else
{
Assert.Single(errors);
Assert.Equal(errorMessage, errors.Single().Message);
}
}

private TenantValidator CreateTenantValidator(bool defaultTenant = true)
{
var shellHostMock = new Mock<IShellHost>();
Expand All @@ -107,22 +144,41 @@ private TenantValidator CreateTenantValidator(bool defaultTenant = true)
.Setup(l => l[It.IsAny<string>(), It.IsAny<object[]>()])
.Returns<string, object[]>((n, a) => new LocalizedString(n, n));

var dataProviders = new List<DatabaseProvider>
{
new DatabaseProvider { Name = "Sql Server", Value = "SqlConnection", HasConnectionString = true, SampleConnectionString = "Server=localhost;Database=Orchard;User Id=username;Password=password", HasTablePrefix = true, IsDefault = false },
new DatabaseProvider { Name = "Sqlite", Value = "Sqlite", HasConnectionString = false, HasTablePrefix = false, IsDefault = true },
new DatabaseProvider { Name = "MySql", Value = "MySql", HasConnectionString = true, SampleConnectionString = "Server=localhost;Database=Orchard;Uid=username;Pwd=password", HasTablePrefix = true, IsDefault = false },
new DatabaseProvider { Name = "Postgres", Value = "Postgres", HasConnectionString = true, SampleConnectionString = "Server=localhost;Port=5432;Database=Orchard;User Id=username;Password=password", HasTablePrefix = true, IsDefault = false }
};

var shellSettings = defaultTenant
? _shellSettings.First()
: new ShellSettings();

return new TenantValidator(
shellHostMock.Object,
featureProfilesServiceMock.Object,
Enumerable.Empty<DatabaseProvider>(),
dataProviders,
shellSettings,
stringLocalizerMock.Object);
}

private void SeedTenants()
{
_shellSettings.Add(new ShellSettings{ Name = ShellHelper.DefaultShellName });
_shellSettings.Add(new ShellSettings { Name = "Tenant1" });
_shellSettings.Add(new ShellSettings { Name = ShellHelper.DefaultShellName });

var settings = new ShellSettings { Name = "Tenant1" };
settings.ShellConfiguration["ConnectionString"] = "Server=.;Database=OrchardCore1;User Id=oc;Password=admin@OC";
settings.ShellConfiguration["TablePrefix"] = "OC";

_shellSettings.Add(settings);

settings = new ShellSettings { Name = "Tenant2", RequestUrlPrefix = String.Empty, RequestUrlHost = "example2.com" };
settings.ShellConfiguration["ConnectionString"] = "Server=.;Database=OrchardCore2;User Id=oc;Password=admin@OC";

_shellSettings.Add(settings);

_shellSettings.Add(new ShellSettings { Name = "Tenant2", RequestUrlPrefix = String.Empty, RequestUrlHost = "example2.com" });
_shellSettings.Add(new ShellSettings { Name = "Tenant3", RequestUrlPrefix = "tenant3", RequestUrlHost = String.Empty });
_shellSettings.Add(new ShellSettings { Name = "Tenant4", RequestUrlPrefix = "tenant4", RequestUrlHost = "example4.com,example5.com" });
Expand Down