Upgrading to IdentityServer4 v 4.0.4

ravendb / IdentityServer4.Contrib.RavenDB

MIT License

9 stars 3 forks source link

Upgrading to IdentityServer4 v 4.0.4 #3

Closed lahma closed 3 years ago

lahma commented 3 years ago

Hello folks. I'm the process of evaluating the use of RavenDB as backend storage for IDS4. Fortunately found about this project just before I started to work on porting some older project 😌 I'm going to migrate to latest IDS v 4.0.4 API to get the latest and greatest while I'm just starting.

Couple questions:

Would you accept PR for such and upgrade?
Do you you have plans to publish on NuGet?
- I see the package id IdentityServer4.Contrib.RavenDB a bit problematic as it's reserved on NuGet and then again does RavenDB as product give the prefix which would somewhat imply some kind of official support (even if there wasn't any)

I've already started the progress so I could offer a PR for review scrutiny, if you would like one.

ayende commented 3 years ago

Yes, we'll gladly accept PRs. We can probably put a NuGet package under RavenDB.Contrib

lahma commented 3 years ago

So I've made progress in PR https://github.com/ravendb/IdentityServer4.Contrib.RavenDB/pull/4 which is now somewhat minimal version of the upgrade that I was able to get up and running. Some things that might divide opinions so I'd like some feedback:

I've moved the extension methods to main project, I don't see much benefit for extra NuGet dependency just to get couple builder methods
documents should use logic-assigned ids. We cannot have client definitions with same names for example, this would make it logical to use document id Clients/my-client-id - this is probably a breaking change, I don't know how widely this solution is in use
currently auto-indexes are in use, I'd prefer to use static indexes for better predictability and maintenance. We don't allow auto-indexes in our development environments so that we don't get any indexing surprises in production, we always declare index to use when querying

ayende commented 3 years ago

I think we can afford to make a breaking change.
No issue with either static indexes or auto ones.

lahma commented 3 years ago

now using static indexes that end user can deploy, relevant ones waited on SaveChanges
now forwarding api resource names to Id, client id to Id to ensure uniqueness (not cluster safe though)

lahma commented 3 years ago

Hey thanks for accepting the PR! I might be doing hopefully much smaller improvement suggestion later on as I'm still at the early stages 👍

lahma commented 3 years ago

And for crying out loud, they just released a new version yesterday... https://github.com/IdentityServer/IdentityServer4/releases