brockallen / BrockAllen.MembershipReboot

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

Adding custom event for ExternalLoginEvent #627

Closed parkinsona closed 7 years ago

parkinsona commented 8 years ago

Hi,

I can't seem to see an event for when a user logins in using an external account. The closest I can find is the one where a LinkedAccount is created.

Is there a way I can create my own event and add it?

Below is what i've tried. It runs, however nothing is ending up in the log.

the Custom Event class

 public class SuccessfulExternalAccountLoginEvent<T> : SuccessfulLoginEvent<T>
    {
        public SuccessfulExternalAccountLoginEvent()
            :base()
        { }

        public string ProviderAccountId { get; set; }
        public string ProviderName { get; set; }
    }

The CustomUserService call to add the event

  public class CustomUserService : MembershipRebootUserService<CustomUser>
    {
        public CustomUserService(CustomUserAccountService userSvc)
            : base(userSvc)
        {
        }

 protected override async Task<AuthenticateResult> ProcessExistingExternalAccountAsync(Guid accountID, string provider, string providerId, IEnumerable<Claim> claims)
        {
            //UpdatedLinkedAccount
            var user = userAccountService.GetByID(accountID);

var cuas = (userAccountService as CustomUserAccountService);
            cuas.AddEvent(new SuccessfulExternalAccountLoginEvent<CustomUser> { Account = account, ProviderAccountId = providerId, ProviderName = provider });

            return await SignInFromExternalProviderAsync(user, provider, providerId, claims);
        }

I also had to expose the AddEvent method because I couldn't see another way to add it to my user account service.

 public class CustomUserAccountService : UserAccountService<CustomUser>
    {
        public CustomUserAccountService(CustomConfig config, CustomUserRepository repo)
            : base(config, repo)
        {
        }

        public void AddEvent<E>(E evt) where E : IEvent            
        {
            base.AddEvent<E>(evt);
        }

    }

Handler for the Events

 public class AuditEventHandler :
         IEventHandler<SuccessfulLoginEvent<CustomUser>>,
         IEventHandler<FailedLoginEvent<CustomUser>>,
         IEventHandler<PasswordChangedEvent<CustomUser>>,
         IEventHandler<UserAccountEvent<CustomUser>>,
...............
parkinsona commented 8 years ago

I have also tried adding in different event types, but with no success.

brockallen commented 8 years ago

Well, that was certainly never the design (meaning custom events) but if you can get it to work, then great.

parkinsona commented 8 years ago

The only reason I'm really looking at a custom event is because I can't see a way of logging when external logins occur. So I guess, is it something I should be doing, or can we add an event for when someone logs in externally? Not sure if this is feasible or not.

parkinsona commented 8 years ago

Ok, I found out where I was going wrong. RaiseEvents was not being fired after I added my customEvents. Moving them prior to AddOrUpdateLinkedAccount gets around this because it calls Update(account), which in turn calls RaiseEvents.

This is what my ProcessExternal looks like now:

 protected override async Task<AuthenticateResult> ProcessExistingExternalAccountAsync(Guid accountID, string provider, string providerId, IEnumerable<Claim> claims)
        {
            //UpdatedLinkedAccount
            var user = userAccountService.GetByID(accountID);

var cuas = (userAccountService as CustomUserAccountService);
            cuas.AddEvent(new SuccessfulExternalAccountLoginEvent<CustomUser> { Account = account, ProviderAccountId = providerId, ProviderName = provider });

            //Remove any duplicate claims to avoid any DB constraint issues when adding them
            var distinctExternalClaims = claims.GroupBy(t => t.Type + t.Value).Select(t => t.First());

            //Create a linked Account record for the external user and update linked claims. The following will remove all existing linked claims and add the newly supplied ones.
            userAccountService.AddOrUpdateLinkedAccount(account, provider, providerId, distinctExternalClaims);

            return await SignInFromExternalProviderAsync(user, provider, providerId, claims);
        }

Now having said that. Wouldn't it make more sense to add a "SuccessfulLoginFromExternalProviderEvent" that is raised in AddOrUpdateLinkedAccount ?

brockallen commented 8 years ago

Wouldn't it make more sense to add a "SuccessfulLoginFromExternalProviderEvent" that is raised in AddOrUpdateLinkedAccount ?

Possibly -- so you're looking for a native event from MR for this activity? Open an issue in MR.

parkinsona commented 8 years ago

Yes that would be great. This is MR ;)

brockallen commented 8 years ago

This is MR ;)

Oh jeeze. #embarassed #toomanyOSSprojects

parkinsona commented 8 years ago

No worries. I still think its amazing you can manage all these things.

In the new event, could we include ProviderName and ProviderAccountID in the object?

brockallen commented 8 years ago

Yea, those seem to make sense.

parkinsona commented 8 years ago

I think I was going to be stuck raising my own event without some form of update to the UserAccount table. I think to get around this, if EventBusUserAccountRepository userRepository; in UserAccountService was made protected, then I could call RaiseEvents myself for any other custom events I may have.

I wouldn't need this for ExternalProvider Logins. However, in my UserManagement application, I may wish to capture other update events that are specific to my application.

parkinsona commented 8 years ago

Hi, I've come across another scenario where I would want to write my own events.

Is it possible to expose EventBusUserAccountRepository from UserAccountService so that I can call RaiseEvents? Or, could you add a RaiseEvents & AddEvent method to UserService?

parkinsona commented 8 years ago

Exposing IAllowMultiple would also be helpful.

brockallen commented 7 years ago

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