Is partial mocking of a security a good practice?

Question

I am introducing automatic testing using NUnit, NSubstitute for a project which uses Ninject and generic repositories.

For regression testing, I am replacing generic repositories with in memory ones to prevent working with a database.

Also, to test security constraints of services, I am mocking the security service which looks like this:

public class SecurityService : ISecurityService
{
    #region Properties
    private IScopedDataAccess DataAccess { get; }
    private IMappingService MappingService { get; }
    #endregion

    #region Constructor
    public SecurityService(IScopedDataAccess scopedDataAccess, IMappingService mappingService)
    {
        DataAccess = scopedDataAccess;
        MappingService = mappingService;
    }

    #endregion

    #region Methods
    public virtual string GetUsername()
    {
        return HttpContext.Current.User.Identity.Name;
    }

    public AppUserSecurityProfileServiceModel GetCurrentUserData()
    {
        var username = GetUsername();
        var userDataModel = DataAccess.AppUserRepository.AllNoTracking.FirstOrDefault(u => u.Username == username);
        if (userDataModel == null)
            return null;

        var ret = MappingService.Mapper.Map<AppUserSecurityProfileServiceModel>(userDataModel);
        return ret;
    }

    public virtual int GetCurrentUserId()
    {
        var userData = GetCurrentUserData();
        if (userData == null)
            throw new SecurityException($"No user data could be fetched for - {GetUsername()}");

        return userData.AppUserId;
    }

    public bool IsInRole(UserRoleEnum role, int? userId = null)
    {
        int actualUserId = userId ?? GetCurrentUserId();

        var hasRole = DataAccess.AppUserXUserRoleRepository.AllNoTracking.Any(x => x.AppUserId == actualUserId && x.UserRoleId == (int) role);
        return hasRole;
    }

    public bool CanPerformAction(UserActionEnum action, int? userId = null)
    {
        int actualUserId = userId ?? GetCurrentUserId();

        var hasAction = DataAccess.AppUserXUserRoleRepository.AllNoTracking
            .Where(x => x.AppUserId == actualUserId)
            .Join(DataAccess.UserRoleRepository.AllNoTracking, xRole => xRole.UserRoleId, role => role.UserRoleId, (xRole, role) => role)
            .Join(DataAccess.UserRoleXUserActionRepository.AllNoTracking, xRole => xRole.UserRoleId, xAction => xAction.UserRoleId,
                (role, xAction) => xAction.UserActionId)
            .Contains((int) action);

        return hasAction;
    }

    // other methods can appear here in the future
    #endregion
}

Each regression test fakes current user like this:

public void FakeCurrentUser(int userId)
{
    var userRef = DataAccess.AppUserRepository.AllNoTracking.FirstOrDefault(u => u.AppUserId == userId);

    var securitySubstitude = Substitute.ForPartsOf<SecurityService>(Kernel.Get<IScopedDataAccess>(), Kernel.Get<IMappingService>());

    securitySubstitude.When(x => x.GetUsername()).DoNotCallBase();
    securitySubstitude.GetUsername().Returns(userRef?.Username ?? "<none>");
    securitySubstitude.When(x => x.GetCurrentUserId()).DoNotCallBase();
    securitySubstitude.GetCurrentUserId().Returns(userId);

    Kernel.Rebind<ISecurityService>().ToConstant(securitySubstitude);
}

Basically, it takes care to replace methods that are based on the context (i.e. HttpContext in my case), but leaves other method intact.

Each tested service will be instantiated after this initialization, so I am sure that the appropriate instance is injected.

Question: is it OK to mock the service like this or is it an anti-pattern?

Answer 1

Do you have a particular concern with this approach? It seems workable in this case.

Personally I like to avoid partial mocks because then I have to keep a more careful track on which parts are real / going to call real code vs which parts are faked out. If you have the flexibility to change the code here, you could push the HttpContext related stuff to another dependency (Strategy pattern I think) and then fake that out instead.

Something like:

public interface IUserInfo {
    string GetUsername();
    int GetCurrentUserId();
}

public class HttpContextUserInfo : IUserInfo {
    public string GetUsername() { return HttpContext.Current.User.Identity.Name; }
    public int GetCurrentUserId() { ... }
}

public class SecurityService : ISecurityService
{
    private IScopedDataAccess DataAccess { get; }
    private IMappingService MappingService { get; }
    // New field:
    private IUserInfo UserInfo { get; }

    // Added ctor argument:
    public SecurityService(IScopedDataAccess scopedDataAccess, IMappingService mappingService, IUserInfo userInfo)
    { ... }

    public AppUserSecurityProfileServiceModel GetCurrentUserData()
    {
        var username = UserInfo.GetUsername();
        var userDataModel = DataAccess.AppUserRepository.AllNoTracking.FirstOrDefault(u => u.Username == username);
        ...
        return ret;
    }

    public bool IsInRole(UserRoleEnum role, int? userId = null)
    {
        int actualUserId = userId ?? UserInfo.GetCurrentUserId();
        var hasRole = ...;
        return hasRole;
    }

    public bool CanPerformAction(UserActionEnum action, int? userId = null)
    {
        int actualUserId = userId ?? UserInfo.GetCurrentUserId();
        var hasAction = ...;
        return hasAction;
    }    
}

Now you're free to pass in an alternate implementation of IUserInfo for your test (can implement by hand or use a mocking library). This addresses my initial concern with partial mocks because I know that all of the SecurityService being tested is calling its real code, and I can manipulate the test dependencies to exercise different parts of that code. The cost is that we now have another class to worry about (and potentially another interface; I've used one but you could stick to a single class with virtual methods) which increases the complexity of the solution a little.

Hope this helps.