IdentityServer / IdentityServer3.EntityFramework

EntityFramework persistence layer for IdentityServer3
Apache License 2.0
68 stars 97 forks source link

Problem using MySql as backing store #80

Closed FireFly26 closed 8 years ago

FireFly26 commented 8 years ago

Dear,

I'm building an STS using Identityserver3 and Identityserver3.Entityframework packages but my requirements include MySql for the db backing store.

And here I ran into a problem: in the ClientSecret entity the property Expiration is of type DateTimeOffset and MySql doesn't support that type for a column.

Any change you could change the type to DateTime and convert it in the Automapper mappings to DateTimeOffset?

Kind Regards, Steven.

buybackoff commented 8 years ago

See #62 and this commit for Automapper changes However, it is 57 commits behind the current upstream master.

nicholi commented 8 years ago

My fork also has similar fixes in place. Successfully used with MySQL EF6. I am noticing one problem with TokenCleanup currently...still figuring it out.

brockallen commented 8 years ago

If one of you wants to manage a MySql IdSvr repo, then we can link to it from our docs.

buybackoff commented 8 years ago

I believe changing DateTimeOffset to DateTime in upstream is the only long-term option, because DateTimeOffset usage has no valid reason. It is not MySQL specific, but Postgres and other databases as well. As discussed in #62.

leastprivilege commented 8 years ago

@jageall - what was the reason for DateTimeOffset again?

nicholi commented 8 years ago

My guess the intention was to allow to unambiguously always refer to a DateTime + the exact TimeZone. That's usually the reason to use DateTimeOffset. Whereas DateTime can only refer to UTC and "Local" (which is still ambiguous at best if server moves). However in Ids you always store the time as UTC anyways, so really either should do. And whenever you create a new time/operate on it, it seems to always be converted ToUniversal first.

I wouldn't having a problem managing a MySql repo...however this isn't truly a new library, it's just a total fork of the EntityFramework package with the necessary changes. Could rename it to IdentityServer3.EntityFramework.MySql.

Here are the three necessary commits I've identified, split up between 2.0 and 2.2.

2.0 #1 #2

2.2 #3

Edit: Whoops forgot 1 additional change that was fixed in my 2.2 merge. Source/Core.EntityFramework/Stores/RefreshTokenStore.cs

jageall commented 8 years ago

DateTime.Kind doesn't always support serialization including by some database persistence layers resulting in the DateTime being deserialized as the wrong value. I guess we could have created a datetimeoffset internally and assumed that it was always going to be in utc, but I would prefer to err on the side of caution and only ditch the info in persistence layers that don't support DateTimeOffset than find out the assumption was wrong in the middle of the night when daylight savings get applied.

On 3 December 2015 at 16:14, Dominick Baier notifications@github.com wrote:

@jageall https://github.com/jageall - what was the reason for DateTimeOffset again?

— Reply to this email directly or view it on GitHub https://github.com/IdentityServer/IdentityServer3.EntityFramework/issues/80#issuecomment-161695921 .

jageall commented 8 years ago

ok, so reviewing this not on my phone so maybe I can make more sense. Most of what follows is just my opinion :)

nicholi commented 8 years ago

I'll have to give the MySql.Data Connection + Entity conversion to assure it's setting the DateTimeKind correctly on their end. They've actually had a few bugs I reported in the past even for the plain ADO.NET Connection setting it incorrectly for DateTime's.

I agree DateTime is a bit ugly, not nearly as bad as Java's Date, but yeah. So is there a clear solution we should be taking for all the EntityFramework compatible RDBMS that don't support DateTimeOffset (as far as I know MS SQL is the only one that does)? Or...everyone just has to fork and edit all the DateTimeOffset lines as we move along?

buybackoff commented 8 years ago

@jageall servers always run in Utc, otherwise you have a million other problems not even related to IdSrv... In IdSrv.EF, which is already a persistence layer to IdSrv, there is an explicit use of Utc time everywhere DayTimeOffset is used. DateTime is perfectly fine data structure (other that it is non-blittable for no reason), and when used as Utc it is just an Int64 number of ticks from the start zero year. All world outside .NET uses Unix epoch for this and somehow cope with Int32 number of milliseconds since 1970 for 99% use cases. Another solution could be to use the number of ticks as Int64, not DateTime, but this is effectively naming the same thing differently.

TimeZone is a UI/UX thing, not a server thing. You could have one server and clients from across the world. Are you going to store user Id data in their zoned time!? Obviously not. The only place when the user's TZ data could be needed is in UI, e.g. last login time and similar things. And even here you have an option to detect TZ from a browser, or use it from user settings.

jageall commented 8 years ago

@buybackoff I do agree with some of what you are saying.

The real issue here is where the line should be drawn. If identityserver uses a type that is unambiguous then the persistence layer can make the call whether to use that type or some other. If not the persistence layer has to make an assumption about the value the type is representing and usually someone will make the wrong assumption at some point. I personally favour the use of an unambiguous type and making the decision about how to represent that data for persistence in the persistence layer, but I appreciate that you may not.

Changing it to datetime would be a breaking change to identityserver so probably not something that would happen until version 3 so in the meantime the persistence layers will have to convert to a supported type

I apologise for my somewhat liberal use of the word "server", but there are certainly times when identityserver is running an a computer with a timezone set. During development is a prime example. Some of us do most of our work on laptops and travel a lot, the timezone changes regularly. I also don't buy that every server is perfectly configured including it's configured timezone.

There are plenty of databases out there where the drivers do stupid things with datetime when simply round tripping it. This is specifically a problem with the .NET implementation of the datetime struct and it's serialization, if they had used the Unix epoch in the first place we wouldn't even be having.this conversation.

brockallen commented 8 years ago

I don't foresee making this change. Sorry. If you build your own MySql store, and if you open source it, feel free to let us know ans we can link to it from our docs. Thx.

buybackoff commented 8 years ago

A couple of weeks ago I changed DTO to DT again, it took me less than 10 mins. My fork was too many commits behind and I had many merge conflicts. So I scanned for all usages of DTO, changed them to DT and added two Automapper rules. That is it. So in general this is not a problem and maintaining a separate fork up to date requires more work.

buybackoff commented 8 years ago

This is the DateTime branch, updated for 2.4. I do not pretend that I maintain a fork, the changes are just trivial.

DerekJarvis commented 7 years ago

Has there been any further consideration for improving IdSrv's support for MySQL? MSSQL is too heavy to use for standalone cloud-hosted apps. It seems a bit strange that the authentication framework I use would dictate the backing database for my entire application.

brockallen commented 7 years ago

It seems a bit strange that the authentication framework I use would dictate the backing database for my entire application.

We don't prohibit you using MySql -- you just need to build this yourself. You could then even OSS it as well -- that'd be a way to contribute back to the community.

buybackoff commented 7 years ago

Just want to confirm that my link above works well for the entire year without any single change, and there are several others on GitHub, probably more recently updated.

@brockallen I haven't started migrating to the version 4 yet (standalone IdSrv v.3 works great with .NET Core MVC client), when I had a look there were no stable MySQL provider for EF7. You mentioned here that there is no more DateTimeOffsets in v.4, do you know if people run v.4 with MySQL and EF7 without problems?

brockallen commented 7 years ago

do you know if people run v.4 with MySQL and EF7 without problems

No, I don't. Sorry