SQL Permission Handling in WinForms

Question

For my application, I've opted to use Integrated Security with Windows Authentication to access the SQL database. I've created AD security groups for each role and given them GRANT permissions on the tables and views they need to do their job.

To further keep the end users from getting to the controls I'm checking the SQL permissions using HAS_PERMS_BY_NAME() and then disabling the relevant controls.

In a static class, I have defined astatic method to get a dictionary of permissions.

namespace PartDataManager
{
 public static class Globals
 {
 public static Dictionary<string,bool> GetPermissions()
 {
 Dictionary<string,bool> Permissions = new Dictionary<string,bool>();
 string query = "SELECT \r\n";
 foreach( string table in new string[]{"appearance","appearanceSpecs","cutTemplates","cutVinyl"} )
 {
 foreach( string perm in new string[]{"INSERT","UPDATE","DELETE"} )
 {
 query += String.Format("\t{0}HAS_PERMS_BY_NAME('{1}','OBJECT','{2}') AS {1}_{2}\r\n",(query.Length>10?",":""),table,perm);
 }
 }
 try
 {
 using( SqlConnection sql = new SqlConnection(ConnectionString) )
 using( SqlCommand cmd = new SqlCommand(query,sql) )
 {
 sql.Open();
 using( SqlDataReader data = cmd.ExecuteReader() )
 {
 data.Read();
 for( int i = 0; i < data.FieldCount; i++ )
 Permissions.Add(data.GetName(i), data[i].Equals(1));
 }
 }
 }
 catch
 {
 throw;
 }
 return Permissions;
 }
 // ...
 }
}

I then use the method on loading of the form to disable the relevant controls like so (wrapped in a try... catch)...

Dictionary<string, bool> Permissions = Globals.GetPermissions();
tsmiNewApprSpec.Enabled = Permissions["appearance_INSERT"];
tsmiNewCutTemplate.Enabled = Permissions["cutTemplates_INSERT"];
tsmiEditAppr.Enabled = Permissions["appearance_UPDATE"];
//...

Is this a solid approach to handling security? Is there room for improvement without increasing complexity?

Note: When I write code, I like to do things in as few lines as possible. I have a horrible memory, so when I go back and look at it, it's like I've never seen it before and I like to be able to see a significant chunk without scrolling, or matching braces, and be able to quickly tell what it does. So, any tips that reduce lines of code will definitely be appreciated and rewarded!

Answer 1

So, any tips that reduce lines of code..

As a start you can remove this useless try..catch because you are only rethrowing the exception which is senseless.

---

Using += with strings in a loop is a bad habit because each time a new string will be created so better use a StringBuilder.
Speeking about the composition of the query variable you don't need to add new lines and tabs in it. You won't show it, you only execute it as a sql query.

---

In its current state this method is not flexible enough. Each time you want to query different tables or need only specific permissions you need to change this method.

By passing IEnumerable<string> tables and IEnumerable<string> permissionType you can beautify your method and add a lot flexibility to it.

---

Let us talk about style. If it comes to style one should stick to a choosen style.

If you choose to use braces {} for loops you should always use them. Mixing styles is a bad habit, don't do it.

Method scoped variables should be named using camelCase casing. PascalCase casing is used for class-, method- and propertynames. So Dictionary<string,bool> Permissions -> Dictionary<string,bool> permissions.

---

EDIT Based on the great comment of @d347hm4n stating

In the above code, in all cases, there will be a comma on the end that needs to be removed. So removing the last character from the string builder is sufficient. If the sql generated does change in the future, using a TrimEnd instead gives you the extra intelligence to not remove the comma on the end if it's not there.

I changed the former removing of the comma from

sb.Length -= 1; 
string query = sb.ToString(); 

to

string query = sb.ToString().TrimEnd(','); 

Applying the mentioned points lead to

public static Dictionary<string, bool> GetPermissions(IEnumerable<string> tables, IEnumerable<string> permissionTypes)
{
 if (!tables.Any() || !permissionTypes.Any())
 {
 return new Dictionary<string, bool>();
 }
 StringBuilder sb = new StringBuilder(1024);
 sb.Append("SELECT ");
 foreach (string table in tables)
 {
 foreach (string perm in permissionTypes)
 {
 sb.AppendFormat("HAS_PERMS_BY_NAME('{0}','OBJECT','{1}') AS {0}_{1},", table, perm);
 }
 }
 string query = sb.ToString().TrimEnd(',');
 Dictionary<string, bool> permissions = new Dictionary<string, bool>();
 using (SqlConnection sql = new SqlConnection(ConnectionString))
 using (SqlCommand cmd = new SqlCommand(query, sql))
 {
 sql.Open();
 using (SqlDataReader data = cmd.ExecuteReader())
 {
 data.Read();
 for (int i = 0; i < data.FieldCount; i++)
 {
 permissions.Add(data.GetName(i), data[i].Equals(1));
 }
 }
 }
 return permissions;
} 

and can be then called like this

IEnumerable<string> tables = new string[] { "appearance", "appearanceSpecs", "cutTemplates", "cutVinyl" };
IEnumerable<string> permissionTypes = new string[] { "INSERT", "UPDATE", "DELETE" };
Dictionary<string, bool> permissions = GetPermissions(tables, permissionTypes);

• aydjay: I might use: sb.ToString().TrimEnd(',') instead
• Heslacher: That comes with the cost of an extra string object.
• aydjay: You gain the intelligence of not mindlessly removing the last character, incase the SQL changes in the meantime.
• Heslacher: I don't understand what you mean.
• aydjay: In the above code, in all cases, there will be a comma on the end that needs to be removed. So removing the last character from the string builder is sufficient. If the sql generated does change in the future, using a TrimEnd instead gives you the extra intelligence to not remove the comma on the end if it's not there.