IdentityServer / IdentityServer3.AspNetIdentity

ASP.NET Identity support for Thinktecture IdentityServer3
Apache License 2.0
64 stars 51 forks source link

Nuget install adds App_Package folder containing UserService #47

Closed scottbrady91 closed 9 years ago

scottbrady91 commented 9 years ago

Installed this package via nuget command line this morning and it added the following folders & class to my project: App_Package / IdentityServer3.AspNetIdentity / IdentityServer3.AspNetIdentity.cs

This class contains the AspNetIdentityUserService.

Confirmed using a fresh install in both web and console apps, typing only: install-package IdentityServer3.AspNetIdentity

brockallen commented 9 years ago

Yep, this is by design.

scottbrady91 commented 9 years ago

Do you have a reason why anywhere?

brockallen commented 9 years ago

Because it made it more obvious what the base was doing, and so that you could change it as needed, or just decide to skip it all together.

scottbrady91 commented 9 years ago

This package is a client library that allows me to use an ASP.NET Identity user store.

If I wanted to start messing with your default behaviour, I would implement and override the AspNetIdentityUserService, not directly start modifying it.

What happens if I modify this UserService and I hit update package?

brockallen commented 9 years ago

Sorry if you're unhappy with the design change. The point, though, is that the user service is by far the most customized piece of IdSvr and people want the behavior to work differently for various reasons. This approach provides a starting point for an application's customization.

scottbrady91 commented 9 years ago

Customisation is understandable, but not by directly editing the source code. If a user has such a special use case that they cannot extend the class, surely they should take advantage of the open source project and fork it.

Again, I have to reiterate, this is going to cause issues if I modify this class and then update the package.

You obviously know your target audience, but surely if they're struggling with extensibility and source control, taking this approach is going to cause many more problems for them.

brockallen commented 9 years ago

Well, this was the decision when faced with the alternative to make an uber-flexible (and thus complicated) base class to accommodate all the variations people wanted. In a sense, it's meant as a starting point and to force you to decide how you want the user service implemented.

I certainly understand your concern about future upgrades. We shall see how it plays out. Thanks.

scottbrady91 commented 9 years ago

Can I just confirm that I'm not missing something here.

You have a AspNetIdentityUserService that extends the base UserServiceBase which implements the IUserService interface.

Are there methods that I can't override in the AspNetIdentityUserService that requires me to modify the AspNetIdentityUserService source code directly instead of extending it?

brockallen commented 9 years ago

You can still derive from the AspNetIdentityUserService and override whatever you want. You don't need to modify the source. It's just an option.

scottbrady91 commented 9 years ago

I don't want to keep banging on about this, but just so I know I've done everything I can:

No one else does this and there's no reason this shouldn't be a dll. A user can extend and override all of the logic in the AspNetIdentityUserService to their hearts content and the source code is available on GitHub for them to learn from and fork if necessary.

By offering them direct access to the source code in their project you're only going to confuse people and anger them when it's time for a package update and you overwrite their implementations. If they have source control they will have to sit there manually diffing through the code to add their changes back in and understand what you have updated. And lets face it, many people still don't have source control... I'm really struggling to see any positives to this approach.

This is a client library. No one else does this. Okay, maybe the JQuery nuget package does but that's JavaScript and again, that upsets everyone when it's time to update.

This is not an option, there's no way for me to remove this and treat it like the client library / dll that it is.

Sorry to go on so much and I'm at risk of sounding like an old woman but: I'm not angry, I'm just disappointed.

brockallen commented 9 years ago

As I said, I'm sorry you're unhappy with the decision.

scottbrady91 commented 9 years ago

Sorted! https://www.nuget.org/packages/IdentityServer3.AspNetIdentity.dll/2.0.0

I'll tart it up sometime tomorrow. Let me know if I have to point anything back at you guys...

RaYell commented 9 years ago

While I appreciate the good work you did creating the original AspNetIdentity package I have to agree with @scottbrady91 on this.

On top of everything that has been said I'd like to add that in my case it was driving me mad to have this file added to my solution as it was breaking loads of StyleCop, R# and Code Analysis rules I had set in a project.

Again, I was also using this class as a client, so I felt no need to keep it in the solution as source file. Also one of the principles of SOLID is to not allow modification of a code, but instead allow to extend it. With your design choice you are encouraging quite the opposite.

Thanks to @scottbrady91 DLL package I was able to replace your classes injecting my project with NuGet package reference and I no longer have problems with my quality tools.

brockallen commented 9 years ago

Again, the real motivation was that the base class wasn't really (IMO) a full implementation because different application requirements can't be done in a single user service base class. This point was to make it more obvious what sort of thing can be done and when using our somewhat pre-built base you can take it or leave it. But now it's more obvious (to the average dev) what a user service needs to do.