Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the class below to manage my specific implementation of a membership system using entity framework 6.1

Is the use of static access correct in this case? as far as I understood that EF context is not thread-safe so I'm instantiating one with every call through the using statement. My second question, is the use of Boolean variable okay to signal failure in db transactions? an important point about this class is that it's going to be used quite frequently as the authentication wont rely on any session established with the user, it's also important to note that the users of this system will be quite limited.

Edit: I'm aware I can use the new identity system or any of the membership providers offered by ASP.NET but this custom provider is quite specific to a Windows authentication system and the use of any cookie-based system is not possible.

public static class AccountManager
{
    private enum Role { User, Comment, CommentControl, Sysadmin }

    public static bool IsUserInRole(string username, string role)
    {
        using (var ctx = new AccountsDbCtx())
        {
            var status = false;
            var foundAcc = ctx.Accounts.Where(x => x.Username == username);
            if (foundAcc.Any())
            {
                var s = foundAcc.SingleOrDefault();
                if (s != null)
                {
                    var exitingRole = s.Roles.Where(x => x.Name.Equals(role, StringComparison.InvariantCultureIgnoreCase));
                    if (exitingRole.Any())
                    {
                        status = true;
                    }
                }
            }
            return status;
        } 
    }

    public static bool IsUserCached(string username)
    {
        using (var ctx = new AccountsDbCtx())
        {
            var result = true;
            var foundAcc = ctx.Accounts.Where(x => x.Username == username);
            if (!foundAcc.Any())
            {
                result = false;
            }
            return result;
        }
    }

    public static bool CacheUser(string username)
    {
        using (var ctx = new AccountsDbCtx())
        {
            var status = false;

            try
            {
                var newAcc = new Account
                {
                    Active = true,
                    Username = username,
                };
                ctx.Accounts.Add(newAcc);
                ctx.SaveChanges();

                AssignRole(newAcc.Username, Role.User);
                status = true;
            }
            catch
            { }

            return status;
        }
    }

    public static bool AssignRoleComment(string username)
    {
        return AssignRole(username, Role.Comment);
    }

    public static bool AssignRoleCommentControl(string username)
    {
        return AssignRole(username, Role.CommentControl);
    }

    public static bool AssignRoleSysAdmin(string username)
    {
        return AssignRole(username, Role.Sysadmin);
    }

    private static bool AssignRole(string accountName, Role role)
    {
        using (var ctx = new AccountsDbCtx())
        {
            var completed = false;
            var roleToAssign = ctx.Roles.SingleOrDefault(x => x.Role1 == (int)role);

            try
            {
                var currentAccount = ctx.Accounts.SingleOrDefault(x => x.Username.Contains(accountName));

                if (currentAccount != null && roleToAssign != null) currentAccount.Roles.Add(roleToAssign);

                ctx.SaveChanges();

                completed = true;

            }
            catch
            {}

            return completed;
        }
    }
}
share|improve this question
    
If you're using .NET 4.5, then there are new ASync methods for all of your query methods, which will allow you to multi-thread calls to the database. –  krillgar Apr 14 at 2:52

1 Answer 1

up vote 3 down vote accepted

umm yes, you could do it that way.

as far as I understood that EF context is not thread-safe so I'm instantiating one with every call through the using statement

Where did you read this? I'm not denying it it's just that I have not read this before and I haven't experienced too many problems with using EF in the case of one instance per request.

My second question, is the use of Boolean variable ok to signal failure in db transactions

Maybe. But how are you going to know what actually went wrong? All that's going to happen is you know something went wrong but when it comes to diagnosing it you will be at a dead end because you have swallowed the exception (so to speak). Given that your exceptions seeming to be handling cases such as the database offline, or connection issues I think it would be best to bubble those up as that would be a core system fault and you would want to know more info.

Here's an alternative solution using a non-static class and sharing the Database context between methods.

NOTE: I would actually consider removing the default empty constructor and IDisposable if you were using an IOC framework to instantiate the context as that would be responsible for your instance lifetime. However I'm not sure how your Account Manager would be created.

public class AccountManager : IDisposable
{
    private enum Role { User, Comment, CommentControl, Sysadmin }
    private readonly AccountsDbCtx dbContext;

    public AccountManager(AccountsDbCtx dbContext)
    {
        this.dbContext = dbContext;
    }

    public AccountManager() : this(new AccountsDbCtx())
    {
    }

    public void Dispose() 
    {
        dbContext.Dispose();
    {

    public bool IsUserInRole(string username, string role)
    {
        var account = dbContext.Accounts.SingleOrDefault(x => x.Username == username);

        if(account == null)
        {
            return false;
        }

        var existingRole = account.Roles.Where(x => x.Name.Equals(role, StringComparison.InvariantCultureIgnoreCase));

        return existingRole.Any();      
    }

    public bool IsUserCached(string username)
    {
        return dbContext.Accounts.Any(x => x.Username == username); 
    }

    public void CacheUser(string username)
    {
        dbContext.Accounts.Add(new Account
        {
            Active = true,
            Username = username
        });

        dbContext.Accounts.Add(newAcc);
        dbContext.SaveChanges();

        AssignRoleUser(username);       
    }

    public bool AssignRoleUser(string username)
    {
        return AssignRole(username, Role.User);
    }

    public bool AssignRoleComment(string username)
    {
        return AssignRole(username, Role.Comment);
    }

    public bool AssignRoleCommentControl(string username)
    {
        return AssignRole(username, Role.CommentControl);
    }

    public bool AssignRoleSysAdmin(string username)
    {
        return AssignRole(username, Role.Sysadmin);
    }

    private bool AssignRole(string accountName, Role role)
    {
        // Why would a role not exist in our database?
        var roleToAssign = dbContext.Roles.Single(x => x.Role1 == (int)role);               
        var currentAccount = dbContext.Accounts.SingleOrDefault(x => x.Username.Contains(accountName));

        if (currentAccount == null)
            return false;

        currentAccount.Roles.Add(roleToAssign);
        dbContext.SaveChanges();

        return IsUserInRole(accountName, role); 
    }
}
share|improve this answer
    
Great points sir, thanks a lot for your help. –  Meldar Apr 13 at 23:21
    
about the pre-fetch of the role object, I have the following schema [Account]1--m[AccountRole]m--1[Role] . EF did not simply allow me to create my role object and submit it with the account object or that's at least what happened when I tried it, the record simply does not get inserted. I had to pre-fetch my role before hand. –  Meldar Apr 13 at 23:34
    
@Meldar Sorry I don't quite understand what you mean? Is something I suggested not working (Which could very well be the case) –  dreza Apr 13 at 23:53
    
@Meldar oh, and just for your reference. If you let the question sit un-accepted for a few days you are more likely to get a few other people commenting who might also have great ideas, thoughts or even better comments. Might help you out even more.... –  dreza Apr 13 at 23:54
1  
@Meldar oh, yes My comment was around did we need to use SingleOrDefault. Cause if the role is always going to exist using Single would be clearer in that we expect it to always be there. Sorry if I wasn't clear in my query. –  dreza Apr 14 at 0:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.