search

brockallen / BrockAllen.MembershipReboot

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

Add time based, Voice and Email options to the TwoFactorAuthMode enum… #649

Closed ericlink closed 7 years ago

ericlink commented 8 years ago

…. This will allow us to use the extended values in our user service and controller in identity server, and in our event listeners that send codes via these methods.

We are extended Idsrv3 (and membership reboot) to perform 2fa via Voice call and email. Later we would like to use TOTP (as in PR 594 - https://github.com/brockallen/BrockAllen.MembershipReboot/pull/594).

We hit a roadblock with this enum, since we need to pass current 2fa type around via the account, and we realized if we extend add the additional enum values, we can use them where we need to for our user service and event handlers. Rather than maintain our own branch, we were hoping you could push this extended enum (or something similar) to allow these extensions?

Thanks for your consideration, - Eric

brockallen commented 8 years ago

I can take a look when I get some time...

ericlink commented 8 years ago

Thanks @brockallen. If it makes it easier, even placeholders would allow us to extend MR without building the the whole thing locally. What I mean is, just 

public enum TwoFactorAuthMode
{        
  {
      None = 0,     
      Mobile = 1,       
      Certificate = 2,
      Extension3 = 3,
      Extension4 = 4, 
      Extension5 = 5, 
      etc...    
 }

}

Then we could use the existing extension points, like custom user service, and access these values from account as intended.

brockallen commented 7 years ago

Sorry I just looked at this today. How are you dealing with this now (without this PR added)?

I'm just worried that others will see this here and expect the core library to implement/support them.

ericlink commented 7 years ago

Ah, yes, that is probably likely!

We made some minor mods to the User Service so that it would fire the two factor events in SMS mode, even if a mobile number was not on the profile. This allows us to run MR with the 2fa enum set to sms, regardless of the actual type we are using. That way MR handles creating, storing and verifying the code for us, which works great.

We added a field to the account to keep track of the users currently selected 2fa mode (using a string "email", "sms", "voice"). We have event handlers for each type of 2fa. They have a guard that checks for currently selected 2fa mode and disregard messages that they are not supposed to handle.

So MR thinks we are in sms mode, but we can handle all three on our side. It makes me wonder if this style would make is easy for contributors to implement new 2fa types as contrib modules?

brockallen commented 7 years ago

It makes me wonder if this style would make is easy for contributors to implement new 2fa types as contrib modules?

Don't know, but I don't foresee having time to curate such additions/modifications.

ericlink commented 7 years ago

Exactly, I was thinking a more open plugin model would mean contributors could maintain those modules outside of identity server. Kind of like ws-fed entity framework nuget, stuff like that. If the 2fa was pluggable, then could someone implement OTAP, push to phone to accept login, etc. without touching MR directly?

brockallen commented 7 years ago

So what changes are you thinking are necessary to MR?

ericlink commented 7 years ago

I could do a PR if you want, just to show what I'm thinking?

brockallen commented 7 years ago

Sure, as long as it's not too intricate -- if we merge this then I'm going to have to understand and maintain it.

ericlink commented 7 years ago

OK, that sounds good. I'll see how simple I can make it.

amd989 commented 7 years ago

I would benefit from something like this as well. Currently the business needs to support more factors/steps of authentication.

SMS is okay, but it seems it is not bulletproof. Certificates are awesome but not widely used. This is further improved with PR #594 by adding something like TOTP.

The problem is extensibility. By following PR #594 example, I was able to add email based 2-Step verification by just overriding methods from UserAccountService, and sending codes to emails instead of SMS. However the main limitation was this enum (TwoFactorAuthMode), I had to add a new "Email" option in order to make the solution work.

Following the proposed PR here, by adding these new options anyone could extend 2FA functionality without touching MembershipReboot's code. To a certain degree.

A better pattern should be considered. A common interface with different implementations should allow more flexibility.

I was thinking something along the lines of:

public interface ITwoStepVerification {
   int Id { get; set; };
}
public class SmsTwoStep : ITwoStepVerification { Id = 1; }
public class CertificateTwoStep : ITwoStepVerification { Id = 2; }
public class TotpTwoStep : ITwoStepVerification { Id = 3; }
public class VoiceTwoStep : ITwoStepVerification { Id = 4; }
public class EmailTwoStep : ITwoStepVerification { Id = 5; }

Code will ship with the current two types, people will be able to add more as needed. The Id would work fine with existing UserAccount table (where the int is the key AccountTwoFactorAuthMode). Each implementation will take care of checking conditions (such as the mobile is not null, cert is not null, etc. ) That way the Authenticate method can be made dynamic instead of explicit.

Just some food for thought! Thanks!

ericlink commented 7 years ago

@amd989 this is right along the lines of what I have in mind. I'll get the PR together; glad there is some interest (and folks to review it!). - Eric

brockallen commented 7 years ago

Given that I don't foresee making this change, I'll close this issue. Thanks.

ericlink commented 7 years ago

Hey Brock, why are you no longer interested? Just curious. We have a need for TOTP and push 2fa, especially since nist deprecated sms for 2fa. Also, sorry i haven't been able to get pr in yet. You understand how busy things can get. - Eric

On Nov 17, 2016 7:50 PM, "Brock Allen" notifications@github.com wrote:

Given that I don't foresee making this change, I'll close this issue. Thanks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/brockallen/BrockAllen.MembershipReboot/pull/649#issuecomment-261426549, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzSxqCwNfQQpsR17QJ26ueEnHf0EmEPks5q_QRYgaJpZM4Iji1K .

brockallen commented 7 years ago

I've not really been involved in MR for some time, so I'm preparing for it to be officially "no longer supported". ASP.NET Identity is more widely used, and now finally (with v3) has (for the most part) comparable features. As a single individual I can't keep up with them. If you need these feature then you're more than welcome to fork, or if you're interesting in taking over maintenance here then I'm willing to consider that.

ericlink commented 7 years ago

Thanks Brock. We may be interested in taking over maintenance. We've invested a lot in apps around MR and if it's not a quick switch to asp.net identity and there is feature parity, then we would want to continue with MR for some time. How would you see a transfer of maintenance responsibility working? Just add some of our folks as repo admins? I have a great architect that could participate as our maintainer. And BTW, thanks for all your contribution to the identity space in .net. Great stuff and we've built our identity solution around it. - Eric

On Thu, Nov 17, 2016 at 8:03 PM, Brock Allen notifications@github.com wrote:

I've not really been involved in MR for some time, so I'm preparing for it to be officially "no longer supported". ASP.NET Identity is more widely used, and now finally (with v3) has (for the most part) comparable features. As a single individual I can't keep up with them. If you need these feature then you're more than welcome to fork, or if you're interesting in taking over maintenance here then I'm willing to consider that.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/brockallen/BrockAllen.MembershipReboot/pull/649#issuecomment-261428595, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzSxrw2wwrVB--8oJqaEaNCBu6C6xTxks5q_Qd-gaJpZM4Iji1K .

Eric Link 214.641.5465

brockallen commented 7 years ago

Send me an email and we can setup a phone/skype call.

ericlink commented 7 years ago

Great I'll set something up after Thanksgiving holiday, - Eric

On Fri, Nov 18, 2016 at 9:50 AM, Brock Allen notifications@github.com wrote:

Send me an email and we can setup a phone/skype call.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/brockallen/BrockAllen.MembershipReboot/pull/649#issuecomment-261565101, or mute the thread https://github.com/notifications/unsubscribe-auth/AAzSxu_2ny0bn_SSHKUj24vblmPBF9q4ks5q_clcgaJpZM4Iji1K .

Eric Link 214.641.5465