Better code/ Pattern for checking existence of value

Question

I have web.config entries as shown below. This is for controlling access of users in various roles to various pages.

Admin screen can be access by Hiring Manager and CRM1 Logs screen can be access by CRM3 and Transferee

add key="AdminScreenRoles" value ="Hiring Manager,CRM1"
add key="LogsScreenRoles" value ="CRM3,Transferee "

In future new roles can be given access to Admin screen. Also new pages may be introduced.

I need to ensure that the current user has access to at least one of the pages in the config file. I have the following code. It works. Is there any better/concise/scalable code for this functionality?

List<string> authorizedRolesForAdmin = new List<string>((ConfigurationManager.AppSettings["AdminScreenRoles"]).Split(','));
List<string> authorizedRolesForLogs = new List<string>((ConfigurationManager.AppSettings["LogsScreenRoles"]).Split(','));
if ((authorizedRolesForAdmin.Contains(roleName)) || (authorizedRolesForLogs.Contains(roleName)))
{
    //Has access to at least one page
}

REFERENCE:

Scalable C# Code for Creating Array from Config File

This can get really ugly in web.config files, I propose you model this in a database table for more flexibility. — JonH, Aug 02 '12 at 13:58
Two things you can consider. 1. Cache the List<> and clear the cache if you add new Roles, 2. Use Dictionary that is faster on the contain search. — Aristos, Aug 02 '12 at 14:01
Alternatively, if your organization uses Active Directory, use AD roles/groups for managing this information. — Oded, Aug 02 '12 at 14:01
@JonH Thanks. I understand that. But this suggestion is off-topic — LCJ, Aug 02 '12 at 14:01
@Oded Thanks. I understand that. But this suggestion is off-topic — LCJ, Aug 02 '12 at 14:02

Fyodor Soikin · Accepted Answer · 2012-08-02T15:20:45.927

2

You can definitely significantly simplify your existing code like this:

var hasOneRole =
    new [] { "Admin", "Log" }
    .SelectMany( screen => ( ConfigurationManager.AppSettings[ screen + "ScreenRoles" ] ?? "" ).Split( ',' ) )
    .Contains( roleName );

But this is still going to get ugly over time. Web.config just isn't intended for that kind of stuff. I suggest you put your access control settings in the database.

edited Aug 02 '12 at 15:20

answered Aug 02 '12 at 14:06

Fyodor Soikin

78,590
9
125
172

Thank you.. This meets my current need. – LCJ Aug 02 '12 at 14:43
1

I would also suggest protection from null return from AppSettings[] by adding `?? ""` to it. – Fyodor Soikin Aug 02 '12 at 14:47
@FyodorSoikin I normally avoid doing this to explicitly cause an error if it's missing. I'd rather have the site down then muddle around in an invalid state. – asawyer Aug 02 '12 at 14:49
1

@asawyer: while the point its correct, it does not apply in this case. When the list of roles is missing, you want to treat it as an empty list. Besides, if you really want an error to be produced, you better check for this condition explicitly and produce a meaningful error instead if a generic NRE. Simplifies debugging a lot. – Fyodor Soikin Aug 02 '12 at 14:52
@FyodorSoikin In this particular case it would make more sense (to my at least) to read the app settings outside of the lambda just so that you could do that sort of check & throw. With it *inside* the lambda however, is where I would let it throw if missing. – asawyer Aug 02 '12 at 15:01
@asawyer: I fail to see justification. – Fyodor Soikin Aug 02 '12 at 15:06
It would be great if you can provide the syntax for ??"" to check NULL (as an update to the answer) – LCJ Aug 02 '12 at 15:17

Tigran · Answer 2 · 2012-08-02T14:14:20.043

Don't see to much space to make things better here, if not a couple of suggessions, like:

if the amount of lists of roles becomes big

use Dictionary<RoleName..> or HashSet

May be you can control on presence of like, avoiding creation of additional List<T> instance

(ConfigurationManager.AppSettings["AdminScreenRoles"]).
              Contains("roleName,")//tiny optimization....

But as I said before, the code as-is it looks now is best, as it easy to understand and read.

score 0 · Answer 3 · answered Aug 02 '12 at 14:08

You could avoid splitting the string and instead use something like this, which should be slightly faster:

string authorizedRolesForAdmin = string.Concat(",", ConfigurationManager.AppSettings["AdminScreenRoles"]), ",");
string authorizedRolesForLogs = string.Concat(",", ConfigurationManager.AppSettings["LogsScreenRoles"]), ",");
string searchString = string.Concat(",", roleName, ",");

    if ((authorizedRolesForAdmin.Contains(roleName)) || (authorizedRolesForLogs.Contains(roleName)))
    {
        //Has access to at least one page
    }

This avoids the comparitively-expensive string.Split, and also avoids creating the two lists. Worth noting that string.Contains is .NET4 only; in older versions you'd check the value of string.IndexOf instead.

Better code/ Pattern for checking existence of value

3 Answers3

Linked