Subclassing User (Adding Multiple user types) #8153

tusksoft created 4 years ago

For our app we are segregating users by type as they have data that needs to be segregated. Roles will not suffice as the difference between users involves data tied to the user and IExtendableObject pattern would suffer from the same issues as adding these segregated properties to the User directly.

Consider the following:

public class User : AbpUser<User>
{
    ....
}

public class Foo : User
{
    public object FooProperty { get; set; }
    ....
}

public class Bar : User
{
    public IEnumerable<object> BarProperty { get; set; }
    ....
}

The issue with the above subclassing is that it seems nearly 90% of the code from UserManager and UserAppService would have to be duplicated with mostly just type updates and permissions changes; which makes me feel like I am missing something.

An alternative approach that I considered was creating Foo and Bar with navigation properties to an optional user account. But that introduces more complexity around permissions since it would always have to be checked from the Foo or Bar object as User wouldn't have any knowledge of them. Code for this as follows:

public interface IHaveUser
{
    public User User { get; }
}

public class Foo : IHaveUser
{
    public User User { get; }
    public object FooProperty { get; set; }
    ....
}

public class Bar : IHaveUser
{
    public User User { get; }
    public IEnumerable<object> BarProperty { get; set; }
    ....
}

So to summarize, what is the best approach for creating multiple user types that invloves the least duplication of user management code and is the most compatible with upgrades (minimal modification to AspNetZero demo code)? I prefer the subclassing pattern as it makes the most sense for our needs, just need some direction on making sure we don't duplicate more code than we need to. The intent is to also subclass the existing UserDtos so that we can reuse the Angular components too.

4 Answer(s)

0
maliming created 4 years ago
Support Team
I think inheritance is the right solution.

https://docs.aspnetzero.com/en/common/latest/Extending-Existing-Entities#extending-non-abstract-entities https://docs.microsoft.com/en-us/ef/core/modeling/relational/inheritance

public class User : AbpUser<User> { .... } public class Foo : User { public object FooProperty { get; set; } .... } public class Bar : User { public IEnumerable<object> BarProperty { get; set; } .... }

The issue with the above subclassing is that it seems nearly 90% of the code from UserManager and UserAppService would have to be duplicated with mostly just type updates and permissions changes; which makes me feel like I am missing something.

I think TPH does not have this problem, Like MyEdition, you can use editionManager to manage it.

https://github.com/aspnetzero/aspnet-zero-samples/blob/master/ExtendEntitiesDemo/ExtendEntitiesDemo.Application/Editions/EditionAppService.cs#L109

https://github.com/aspnetzero/aspnet-zero-samples/blob/master/ExtendEntitiesDemo/ExtendEntitiesDemo.Application/Editions/EditionAppService.cs#L122

tusksoft created 4 years ago

Thank you for the direction. Unfortunately edition is a relatively simple compared to User where there is substantially more process around it. I have marked below in the original User creation method from UserAppService.cs at the start of every block that will be duplicated in a create method on StaffMember with ///DUPLICATED to indicate the level of duplication im concerned about.

[AbpAuthorize(AppPermissions.Pages_Administration_Users_Create)]
protected virtual async Task CreateUserAsync(CreateOrUpdateUserInput<UserEditDto> input)
{
    ///DUPLICATED
    if (AbpSession.TenantId.HasValue)
    {
        await _userPolicy.CheckMaxUserCountAsync(AbpSession.GetTenantId());
    }

    /// This will be different because we have to use custom mappings
    var user = ObjectMapper.Map<User>(input.User); //Passwords is not mapped (see mapping configuration)
    user.TenantId = AbpSession.TenantId;

    ///DUPLICATED
    //Set password
    if (input.SetRandomPassword)
    {
        var randomPassword = await _userManager.CreateRandomPassword();
        user.Password = _passwordHasher.HashPassword(user, randomPassword);
        input.User.Password = randomPassword;
    }
    else if (!input.User.Password.IsNullOrEmpty())
    {
        await UserManager.InitializeOptionsAsync(AbpSession.TenantId);
        foreach (var validator in _passwordValidators)
        {
            CheckErrors(await validator.ValidateAsync(UserManager, user, input.User.Password));
        }

        user.Password = _passwordHasher.HashPassword(user, input.User.Password);
    }

    ///DUPLICATED
    user.ShouldChangePasswordOnNextLogin = input.User.ShouldChangePasswordOnNextLogin;

    ///DUPLICATED (with minor changes if our derived users end up having dedicated roles)
    //Assign roles
    user.Roles = new Collection<UserRole>();
    foreach (var roleName in input.AssignedRoleNames)
    {
        var role = await _roleManager.GetRoleByNameAsync(roleName);
        user.Roles.Add(new UserRole(AbpSession.TenantId, user.Id, role.Id));
    }

    ///DUPLICATED
    CheckErrors(await UserManager.CreateAsync(user));
    await CurrentUnitOfWork.SaveChangesAsync(); //To get new user's Id.

    ///DUPLICATED (I can see a reason this wouldnt be duplicated for some subclassed users)
    //Notifications
    await _notificationSubscriptionManager.SubscribeToAllAvailableNotificationsAsync(user.ToUserIdentifier());
    await _appNotifier.WelcomeToTheApplicationAsync(user);

    ///DUPLICATED (I can see a reason this wouldnt be duplicated for some subclassed users)
    //Organization Units
    await UserManager.SetOrganizationUnitsAsync(user, input.OrganizationUnits.ToArray());

    ///DUPLICATED (I can see a reason this wouldnt be duplicated for some subclassed users)
    //Send activation email
    if (input.SendActivationEmail)
    {
        user.SetNewEmailConfirmationCode();
        await _userEmailer.SendEmailActivationLinkAsync(
            user,
            AppUrlService.CreateEmailActivationUrlFormat(AbpSession.TenantId),
            input.User.Password
        );
    }
}

That is just for one method in the UserAppService. So if there are updates to the framework, I'm worried that unless we go through the diffs we would miss those updates (because we aren't subclassing the service, it could go completely unnoticed)

Maybe this is unavoidable, but I'm just worried that there is a code-smell here where subclassing involves duplication a lot of code. Was AspNetZero/ABP built with subclassing User as a supported pattern? It seems like some of this behavior could be consolidated or handled by events. (That would make creating services for subclassed users a much smaller undertaking without worrying about losing other behaviours like notifications or welcome emails because they were overlooked)

I think the solution proposed will work for us, I just want to be absolutely certain before I develop our subclassed user app services such that they are detached from the original UserAppService as that will be a non-trivial time sink.

0

ismcagdas created 4 years ago
Support Team

Hi,

You are right, this is unavoidable when you change the given source code. You can take a look at https://github.com/aspnetzero/aspnet-zero/issues/96 to update your code with the latest version.
0

ismcagdas created 4 years ago
Support Team

This issue is closed because it has not had recent activity for a long time.

Have an answer to this question? Log in and write your answer.