brockallen / BrockAllen.MembershipReboot

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

Custom e-mail in admin user creation flow #607

Closed kryptx closed 8 years ago

kryptx commented 8 years ago

We have built an admin area to manage users for MR. In this area an administrator can, among other things, create an account. When this happens, the user should receive a single e-mail with appropriate verbiage, which contains a link to enter a password for their new account. At least for now, all other actions should still send the usual e-mails sent by MR.

We have taken two approaches to attempt to solve this problem. Both of them are functional but we're not really satisfied with either:

  1. Create custom e-mail event handlers for these events which do not call Process() if the user is currently being created by an admin (except for the password reset, since PasswordResetRequestedEvent contains the key we need to build a valid link) and a custom formatter to select the template based on the situation. However, the only way we could find to expose the current workflow to the event handler was by creating a temporary claim. This really feels like a hack, as it's obviously not at all what a claim is intended for.

    // snippet from an MVC controller
    HierarchicalUserAccount account = _userAccountService.CreateAccount(
       model.Username, 
       password,
       model.Email, 
       null, 
       null, 
       // this claim will be used to determine whether to send e-mails and what their content will be.
       new Claim[] { new Claim("companyname:userorigin:admin", "true") }
    );
    _userAccountService.SetConfirmedEmail(account.ID, model.Email);
    _userAccountService.ResetPassword(account.ID);
    
    // claim needs to be removed so that password resets will work
    _userAccountService.RemoveClaim(account.ID, "companyname:userorigin:admin", "true");
  2. Since this is only for admin account creation, we can extend UserAccountService<T>, overload (or override) one or more CreateAccount methods, and inject only into the desired controller. It can fire only our custom event so we do not need to replace any handlers or formatters; we can just add one.

    Originally our plan was just to copy the existing method and add a call to ((IEventSource)this).Clear() after the call to Init to clear the event. But the event fires even sooner than that, so we wound up having to interact with the repository directly and do things a lot more manually and explicitly. Since this is now a combination of our code and yours, changes that you've vetted against your code base may very well break this:

     public TAccount CreateAccount(string tenant, string username, string email, Guid? id = null, DateTime? dateCreated = null, TAccount account = null, IEnumerable<Claim> claims = null)
     {
         ... (Tracing, EmailIsUsername, MultiTenant) ...
    
         IEventSource source = this;
         account = account ?? CreateUserAccount();
    
         Init(account, tenant, username, password, email, id, dateCreated, claims);
         ValidateEmail(account, email);
         ValidateUsername(account, username);
    
         // Remove the AccountCreatedEvent
         source.Clear();
    
         // repo is a private field, of type EventBusUserAccountRepository<TAccount>
         repo.Add(account);
         SetConfirmedEmail(account.ID, email);
    
         // this is important, or we reset the IsAccountVerified flag to false in our update.
         account = GetByID(account.ID);
    
         var accountCreatedEvent = new UserAccountCreatedByAdminEvent<TAccount>
         {
             Account = account,
             VerificationKey = SetVerificationKey(account, VerificationKeyPurpose.ResetPassword)
         };
    
         AddEvent(accountCreatedEvent);
         // this will actually fire the event; we have no more changes to save
         repo.Update(account);
    
         Tracing.Verbose("[UserAccountService.CreateAccount] success");
    
         return account;
     }

I feel like we must be missing something. Is there a generic recommended approach to solve this? Thanks!

brockallen commented 8 years ago

Usually when you have different scenarios where you want different event behavior, I recommend having different configs that have the different event handlers for those scenarios. Then you use the one you want/need for those scenarios. It's only really tricky when you have multiple scenarios in the same project (and thus complexity of DI).

kryptx commented 8 years ago

@brockallen We were able to make that work. Thank you!