NHibernate.AspNet.Identity icon indicating copy to clipboard operation
NHibernate.AspNet.Identity copied to clipboard

Current threads of different cultures may cause potential problems.

Open maliming opened this issue 6 years ago • 4 comments

see https://github.com/nhibernate/nhibernate-core/issues/1967

maliming avatar Jan 18 '19 03:01 maliming

I do not see how should this be a concern NHibernate.AspNet.Identity.

Can you elaborate? Explain what it may cause for NHibernate.AspNet.Identity features and why should it by handled by it rather than by the application code. Using adequate functions, culture sensitive or not, according to the application needs, is the application responsibility. Now if you report this because some features of NHibernate.AspNet.Identity use inadequate culture sensitive/insensitive functions, please point them.

fredericDelaporte avatar Jan 18 '19 08:01 fredericDelaporte

Changing the culture of the unit test thread context will reproduce this problem. In the current culture, the uppercase of admin is no longer ADMIN but ADMİN,

https://github.com/nhibernate/NHibernate.AspNet.Identity/blob/28b5907beed2c8aa05cbb1785c23210fa855a0f4/source/NHibernate.AspNet.Identity.Tests/UserStoreTest.cs#L237-L244

[TestMethod]
public void FindByName()
{
	var userManager = new UserManager<ApplicationUser>(new UserStore<ApplicationUser>(this._session));
	userManager.Create(new ApplicationUser() { UserName = "admin", Email = "[email protected]", EmailConfirmed = true }, "Welcome");

	Thread.CurrentThread.CurrentCulture = new CultureInfo("tr");

	var x = userManager.FindByName("admin");
	Assert.IsNotNull(x);
	Assert.IsTrue(userManager.IsEmailConfirmed(x.Id));
}


Expected: not null
But was:  null

at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
at NUnit.Framework.Assert.IsNotNull(Object anObject)
at NHibernate.AspNet.Identity.Tests.UserStoreTest.FindByName() 

maliming avatar Jan 18 '19 08:01 maliming

https://github.com/nhibernate/NHibernate.AspNet.Identity/blob/28b5907beed2c8aa05cbb1785c23210fa855a0f4/source/NHibernate.AspNet.Identity/UserStore%601.cs#L51

If the user's current thread culture changes, NHibernate.AspNet.Identity may have problems. Also exists in RoleStore.

The specific content is in the issue I mentioned earlier. https://github.com/nhibernate/nhibernate-core/issues/1967

https://github.com/nhibernate/NHibernate.AspNet.Identity/blob/28b5907beed2c8aa05cbb1785c23210fa855a0f4/source/NHibernate.AspNet.Identity/RoleStore%601.cs#L39

maliming avatar Jan 18 '19 09:01 maliming

A proper fix for this case could be to use something like Criteria/QueryOver InsensitiveLike, which Linq/Hql currently lacks. Using ToUpper or ToLower for emulating case insensitive comparisons is anyway a hack. (Hack which, depending on the dialect may still be used by InsensitiveLike, but at least for PostgreSQL, it will use its built-in support, ilike, instead. But its current implementation will still have the issue in case of uppercase values, since it will use a runtime culture sensitive .ToLower on the parameter value, even with PostgreSQL.)

In fact, I think it is an error for NHibernate.AspNet.Identity to try forcing an insensitive comparison for user names and roles, as illustrated by this issue. It should let the database be responsible for this, since this behavior can in many cases be adjusted in the database (by example, with SQL-Server, by adjusting the collation of columns).

Of course, removing these offending ToUpper would be a breaking change for those currently relying on them. (Those using values differing by the case with case-sensitive comparisons on the db side, while not having a culture mismatch trouble between their application and the database.)

fredericDelaporte avatar Jan 18 '19 09:01 fredericDelaporte