brockallen / BrockAllen.MembershipReboot

MembershipReboot is a user identity management and authentication library.
Other
742 stars 238 forks source link

Can use this project with ASP.NET 5 and MVC6? #568

Closed mikeandersun closed 8 years ago

mikeandersun commented 9 years ago

If No. Is there any plan?

vinneyk commented 9 years ago

+1

brockallen commented 9 years ago

No idea right now. Is there something that prevents you from using it now in ASP.NET 5?

vinneyk commented 8 years ago

I'm working out the differences but it stems from the divergence from IAppBuilder in the new pipeline builder. Admittedly, this can be reverted back to the "old" OWIN pipeline but, let's face it, if you're on vNext, you're not about reverting back ;)

I'll report back with the specific findings when I get to a working implementation.

brockallen commented 8 years ago

Well, the core of MR is separate from whatever cookie middleware you use. So in theory MR should work just fine w/ ASP.NET 5 -- you'd just need to bridge the gap with a ASP.NET 5 specific AuthenticationService (the thing that manages the cookie).

Also, though, to make MR more ASP.NET 5 friendly, I'd really want to make some breaking changes and make everything Task-centric. That's more work/time I don't have right now.

vinneyk commented 8 years ago

That's what I'm seeing as well: it's just the configuration helpers that are antiquated. I appreciate your ambitions and hesitations for the vNext targeted revamp. What would you think about a shim project to include the new helpers in the meanwhile? Either way, I'll share my code with you (when I have the opportunity to write it, that is!)

vinneyk commented 8 years ago

I finally got around to getting MR working with ASP.NET 5 / MVC 6. I intend to document my findings but the most jarring diff is due to reading settings from ConfigurationManager. I've been able to sidestep it via dependency injection using the following:

// reads configuration from appsettings.json
services.AddSingleton(p => new MembershipRebootConfiguration<ApplicationUser>(p.GetService<IOptions<SecuritySettings>>().Value));

However, I just hit an unavoidable wall in the SmtpMessageDelivery due to the fact that it's reading settings from ConfigurationManager without constructor a override for SmtpSection. It's not a huge setback since the class is small and unlikely to change but, to my knowledge, I'll have to replace the class with a version that doesn't reference ConfigurationManager directly.

This dependency on ConfigurationManager seems to be the biggest issue in utilizing MR in vNext; which is not too bad!

brockallen commented 8 years ago

Well, if you can come up with a proposal then I'm happy to consider it. Maybe just another ctor param for the from?

vinneyk commented 8 years ago

Yeah, that's exactly what I came up with:

[MembershipRebootReplacementNote(typeof(BrockAllen.MembershipReboot.SmtpMessageDelivery), "Dependency of ConfigurationManager for retrieval of SmtpSection values")]
public class SmtpMessageDelivery : IMessageDelivery
{
    private readonly StmpDeliveryConfig _config;
    private readonly bool _sendAsHtml;

    public SmtpMessageDelivery(StmpDeliveryConfig config, bool sendAsHtml = false)
    {
        _config = config;
        _sendAsHtml = sendAsHtml;
    }

    public void Send(Message msg)
    {
        Tracing.Information("[SmtpMessageDelivery.Send] sending mail to " + msg.To);
        if (string.IsNullOrWhiteSpace(msg.From))
        {
            msg.From = _config.FromEmailAddress;
        }
        using (var smtpClient = new SmtpClient())
        {
            smtpClient.Timeout = 5000;
            try
            {
                var message = new MailMessage(msg.From, msg.To, msg.Subject, msg.Body)
                {
                    IsBodyHtml = _sendAsHtml
                };
                smtpClient.UseDefaultCredentials = false;
                smtpClient.Host = _config.Host;
                smtpClient.Credentials = new NetworkCredential(_config.UserName, _config.Password);
                smtpClient.Port = _config.Port;
                smtpClient.EnableSsl = _config.EnableSsl;

                smtpClient.Send(message);
            }
            catch (SmtpException ex)
            {
                Tracing.Error("[SmtpMessageDelivery.Send] SmtpException: " + ex.Message);
            }
            catch (Exception ex)
            {
                Tracing.Error("[SmtpMessageDelivery.Send] Exception: " + ex.Message);
            }
        }
    }
}
brockallen commented 8 years ago

Sure, send a PR if you'd like.

vinneyk commented 8 years ago

I'd be glad to. Are you wanting to maintain backward compatibility or would this be an acceptable breaking change?

I have identified at least one more non-vNext compatible concept regarding the RelativePathApplicationInformation which uses HttpContext.Current within the GetApplicationUrl method. I have a replacement method for this as well. Would you like me to continue posting such findings on this thread or save them for a PR?

brockallen commented 8 years ago

I think a breaking change would be ok since it's work to support ASP.NET 5. I should start a new branch for it for the time being.

brockallen commented 8 years ago

Since we're talking about breaking changes, I think I want to have the various APIs that gen a key (email, sms) to return that string as a return value. To me this is one of the most confusing parts for people using MR (rather than using a event handler).

nikcub commented 8 years ago

hey @vinneyk i'm currently also working on this - do you want to create a pull request and i'll add my changes as well?

It might be a good idea to keep this in a separate branch for now while we have a chance to test

vinneyk commented 8 years ago

Hey @nikcub I probably won't get to it before the weekend. If you are quicker on the trigger, I'll follow your lead. Otherwise, yes, I'll fork and add my changes. Looking forward to working with you :)

brockallen commented 8 years ago

Another thing I thought of -- all async. If async is ever going to get into MR, then this would be the time.

Thoughts?

vinneyk commented 8 years ago

I agree 100%.Do you care to have non-async options as well?

vinneyk commented 8 years ago

What are your thoughts about rebuilding the project on the new ASP.NET project templates? I'm thinking we'd start a new solution and just start porting projects into it one at a time. It would obviously make it more difficult to diff so your review may become more cumbersome. I don't know. What do you think? Would it be easier for you if we upgrade in place and then port to the new structure later (if at all)?

vinneyk commented 8 years ago

Put another way, is the aim to support ASP.NET 5 or embrace it?

vinneyk commented 8 years ago

@nikcub, I branched and added my few modifications in my fork. I've submitted a pull request to @brockallen. There were only two objects I've had to modify so far in order to get MR working in my MVC 6 application. In addition, I have had to include a couple settings for IServiceCollection and IApplicationBuilder configuration. That'll likely change be submitted, in some form, soon.

brockallen commented 8 years ago

Sorry -- I missed your recent questions. Yea, I'm not sure about ASP.NET 5. If we do, then perhaps a new project name/assembly.etc -- I've not had time to think about it fully. Also, RC2 will bring about some changes... though, not sure how many would affect us at this level.

VanLePham commented 8 years ago

I saw @vinneyk pull requests about changing the target framework from 4.5 to 4.6.1, but not to the new light weight dotnet5.4. Do you have any plan to port the projects, or create new ones to support the new .Net platform standard (dotnet5.4) framework?

nikcub commented 8 years ago

@vinneyk thanks for that - i've pulled it into my fork and i'm going through it now - plan to work on it this weekend

To answer the question about how to setup the project - most projects that i've come across that are looking to port are splitting into separate projects.

and +1 to async

nikcub commented 8 years ago

@VanLePham will eventually be able to target coreclr

brockallen commented 8 years ago

So do we need a new repo, or will these changes be usable from an old ASP.NET/.NET project? IOW, will these changes move MR into being more like a PCL?

vinneyk commented 8 years ago

From what I've seen thus far, we don't have to loose support for previous .NET versions. If we take the path of least destruction, we can simply replace dependencies on antiquated concepts and potentially support both (at least the non-core frameworks).

Admittedly, I've not had much opportunity to play with coreclr since I tend to have other dependencies which prevent it. That said, @VanLePham, I don't see any dependencies in MR which would prevent support so it should be doable. Though, as I understand it, we'd need to decide to draw a line in the sand and port to a completely new project. I could be wrong but it seems like that's the ultimate answer to your question, @brockallen: do we want coreclr support?

brockallen commented 8 years ago

If we're going to rip off the bandaid, then why not support both.

To be honest, I was on the fence if I wanted to continue MR into ASP.NET 5/Core. ASP.NET Identity 3 technically has all the features necessary. The only real reason to keep working on MR was to help people support existing use. And maybe this would even mean a new project to help migrate from MR to the new ASP.NET Identity 3.

Are there features in MR you feel are more useful than what's in ASP.NET Identity 3, such that it warrants the effort porting this to the new runtime?

vinneyk commented 8 years ago

You're right, Identity 3 does technically have the same features; on the surface at least.

Since I've really started to get to know MR, I've been more excited about what it has at it's core. The capability to customize the membership pipeline is a really great asset! For example, I'm writing a multi-tenant application to which users can be invited by another user. I really like the possibility of composing new membership features out of the event components. In this case, perhaps invited users aren't created using the same path as regular sign-ups. This is not something I've seen supported by ASP.NET Identity.

If you decide to deprecate support moving forward, then we can make the minimal changes required in order to add support for the full framework beyond 4.5.1. In fact, I've been able to get the current MR release working in 4.6.1 by providing a couple classes and a few lines of configuration so perhaps I could contribute that to your wiki. However, if you haven't already, I suggest you write an app on the new architecture and Identity library, or perhaps adapt an existing app with slightly more complex logic, and see for yourself if it's "good enough" for your liking. I would respect your insights if you said it was me who was missing the picture regarding Identity 3.

brockallen commented 8 years ago

Maybe we start with a rev to support the minimal changes for ASP.NET Core. After that then we could maybe discuss having you contribute/fork to an ASP.NET Core version.

vinneyk commented 8 years ago

Did you mean to reference Core in both statements there?

brockallen commented 8 years ago

I think I did :)

I did not meant to say that the next minor update of MR (mainly the one you're working on) would work on .NET Core. If we or someone wanted .NET Core, then that would require the "latter" option I suggested of forking and making a new repo specifically for that platform.

vinneyk commented 8 years ago

Just wanted to let you know I'm still here. Big push for a demo this week so I'll continue to be quiet for a little while longer.

brockallen commented 8 years ago

It's not a problem -- I don't have any cycles to put into this, so it's no rush for me. Plus there will be many changes in RC2 so it's ok to wait for that.

vinneyk commented 8 years ago

Do you have any information of when that might drop?

brockallen commented 8 years ago

https://github.com/aspnet/Home/wiki/Roadmap

vinneyk commented 8 years ago

Just heard your DNR interview with Dominick Baier. I'm guessing that roadmap doesn't fully encompass that 30 minute explanation of the versioning diagram but I did find the article linked on the RC2 release to be insightful. I respect this statement most of all:

It’s clear to us now that we were running too fast and didn’t take the time to fully think this one all the way through.

I agree with your earlier assessment of waiting for RC2 as it seems that does indeed introduce some pretty radical changes. Are you getting much requests for support on using MR in RC1? If so, I could go ahead and write a wiki detailing how I got up and running. It's not altogether too involved but it did take me some time to get all the pieces in place. LMK

brockallen commented 8 years ago

Are you getting much requests for support on using MR in RC1

Nope; this thread is it.

If so, I could go ahead and write a wiki detailing how I got up and running. It's not altogether too involved but it did take me some time to get all the pieces in place. LMK

Only if you really feel you need to or someone else here asks. Otherwise I'd save your energy.

vinneyk commented 8 years ago

I can certainly live with that :) For the most part, the necessary changes are already documented within this thread. That should be enough for anyone brave enough to be using RC1 anyways.

rondefreitas commented 8 years ago

I was curious about it myself (how I found this issue), but like @brockallen said I don't really see what problems MR solves that ASP.NET Identity 3 doesn't.

brockallen commented 8 years ago

I still think there are many ways in ASP.NET Identity to shoot yourself in the foot that MR prevents.

j-hurst commented 8 years ago

I too was curious about adding the support for async/await in the repo. I am currently using MR with identityserver and am working on calling the repositories through web api service calls.

I could not tell from this thread, is this a change that you are looking to make MR, or do you not feel it is worth it?

brockallen commented 8 years ago

I think it's a good idea. It would certainly be a major breaking change.

j-hurst commented 8 years ago

Yea, I was going to look at how big of change it would be, but I did not know based on your guys' discussion above, if some of it was already being looked into.

brockallen commented 8 years ago

Sure, and I know I was vague :)

I just honestly don't know if I have time in the next ~6 months. Is someone here interested in stepping up and helping maintain?

rondefreitas commented 8 years ago

I'm interested in porting it and doing a deep comparison feature-wise as I do so, but i'm wondering if I should wait for RC2 before I tackle this.

brockallen commented 8 years ago

I just spent a day going over all of the new AspNet Identity 3 code and features. Some of the messiness of AspId 1&2 has now been cleaned up and simplified, IMO (much of that is due to a standard DI system in ASP.NET Core).

I then went and looked at the readme here to see what the purported feature list was for comparison. AspId3 has all of those features covered except for the event-bus feature, nested role definitions, and multi-tenancy. On top of it, AspId3 is async, has explicit storage for tokens (minor feature IMO), security stamp validation, and arbitrary claims on roles (which somewhat makes up for the nested roles feature).

At this point, I don't think it makes sense to try to push MR forward into ASP.NET Core now that AspId3 has finally caught up with (and in many regards surpassed) MR.

What I could see going forward is: 1) a MR user store in AspId3 to maintain the existing MR DB/schema/data, and/or 2) a migration tool to move the MR data to AspId3. In either case there is a need to improve on AspId3 with an iterative password hasher (as I did with IdentityReboot), especially if existing MR data needs to be moved to AspId3. Multi-tenancy is easily added by tweaking the AspId3 code in the DI system.

In a sense, MR has served its purpose -- I'd like to think it somewhat forced Microsoft (and perhaps guided them even) to address the state of their identity management and bring it into a more modern time. I appreciate all the of kind and encouraging words I've heard about MR.

ccrowhurstram commented 8 years ago

Brok, another feature of MR is that it provides guidance with regard to higher level workflows around password reset, change of email, etc. Specifically I'm thinking of the methods on the UserAccountService class for executing these workflows.

In your exploration of AspId3 did you find a comparable set of operations implemented?

brockallen commented 8 years ago

No they don't force the same set of requirements around those areas.

ccrowhurstram commented 8 years ago

So in summary:

I think you're right. I also think that it makes sense for a new project to be created "on top of" AspId3 that provides some of the features from MR.

You already hinted at something like this. But correct me if I 'm wrong, you have thought of this as more of a migration project with the implication that only existing MR consumers would use. Whereas I see this as providing value to anyone using AspId3.

From me the important features from MR that would be included in this new project:

C

brockallen commented 8 years ago

Yep, all correct and agreed.

brockallen commented 8 years ago

My opinion is that if any of these features are desired in .NET Core, then they should be added on top of ASP.NET Identity 3.