2
\$\begingroup\$

I am new to Azure and SQL. I have the following function that is part of a larger Web-API which is hosted on Azure and is querying an SQL Server database that is also hosted on the Azure platform:

[WebMethod]
public string RetrieveUserFriends(string userId)
{

    //Ensure there is a user id
    if (userId == null)
    {
        return "UserId is missing.";
    }

    //Build the Connection String
    System.Data.SqlClient.SqlConnectionStringBuilder builder = BuildConnection();

    //Build the query
    string query = string.Format("SELECT  u2.UserId, u2.[first name], u2.[last name], u2.[username] FROM       Users u1 INNER JOIN Friends f1 ON u1.userid = f1.userid INNER JOIN Users u2 ON u2.userid = f1.FriendId INNER JOIN Friends f2 ON u2.userid = f2.friendid WHERE   ( f2.userid = '{0}'    OR f1.friendid = '{0}') AND u2.userid != '{0}';", userId);

    try
    {

        //1. Define an Exponential Backoff retry strategy for Azure SQL Database throttling (ExponentialBackoff Class). An exponential back-off strategy will gracefully back off the load on the service.
        int retryCount = 4;
        int minBackoffDelayMilliseconds = 2000;
        int maxBackoffDelayMilliseconds = 8000;
        int deltaBackoffMilliseconds = 2000;

        ExponentialBackoff exponentialBackoffStrategy =
          new ExponentialBackoff("exponentialBackoffStrategy",
              retryCount,
              TimeSpan.FromMilliseconds(minBackoffDelayMilliseconds),
              TimeSpan.FromMilliseconds(maxBackoffDelayMilliseconds),
              TimeSpan.FromMilliseconds(deltaBackoffMilliseconds));

        //2. Set a default strategy to Exponential Backoff.
        RetryManager manager = new RetryManager(new List<RetryStrategy>
{  
exponentialBackoffStrategy 
}, "exponentialBackoffStrategy");

        //3. Set a default Retry Manager. A RetryManager provides retry functionality, or if you are using declarative configuration, you can invoke the RetryPolicyFactory.CreateDefault
        RetryManager.SetDefault(manager, false);

        //4. Define a default SQL Connection retry policy and SQL Command retry policy. A policy provides a retry mechanism for unreliable actions and transient conditions.
        RetryPolicy retryConnectionPolicy = manager.GetDefaultSqlConnectionRetryPolicy();
        RetryPolicy retryCommandPolicy = manager.GetDefaultSqlCommandRetryPolicy();

        string json = string.Empty;

        //5. Create a function that will retry the connection using a ReliableSqlConnection.
        retryConnectionPolicy.ExecuteAction(() =>
        {

            using (ReliableSqlConnection connection = new ReliableSqlConnection(builder.ConnectionString))
            {

                connection.Open();

                IDbCommand command = connection.CreateCommand();
                command.CommandText = query;

                //6. Create a function that will retry the command calling ExecuteCommand() from the ReliableSqlConnection
                retryCommandPolicy.ExecuteAction(() =>
                {

                    using (IDataReader reader = connection.ExecuteCommand<IDataReader>(command))
                    {


                        List<GameOnUser> users = new List<GameOnUser>();

                        int count = reader.FieldCount;


                        while (reader.Read())
                        {

                            GameOnUser user = new GameOnUser();

                            for (int i = 0; i < count; i++)
                            {

                                if (i == 0)
                                {
                                    user.ID = reader.GetGuid(i).ToString();
                                }
                                else if (i == 3)
                                {
                                    user.Username = reader.GetString(i);
                                }

                            }

                            users.Add(user);
                        }

                        json = new JavaScriptSerializer().Serialize(users);
                    }
                });
            }
        });

        return json;
    }
    catch (Exception e)
    {

        return "ERROR (RetrieveUserFriends): " + e.Message;
    }
}

NOTE: I am aware of the poorly named columns. We intend to fix these in the near future.

My main concern lies in the area of the returning JSON:

string json = string.Empty;

            //5. Create a function that will retry the connection using a ReliableSqlConnection.
            retryConnectionPolicy.ExecuteAction(() =>
            {

                using (ReliableSqlConnection connection = new ReliableSqlConnection(builder.ConnectionString))
                {

                    connection.Open();

                    IDbCommand command = connection.CreateCommand();
                    command.CommandText = query;

                    //6. Create a function that will retry the command calling ExecuteCommand() from the ReliableSqlConnection
                    retryCommandPolicy.ExecuteAction(() =>
                    {

                        using (IDataReader reader = connection.ExecuteCommand<IDataReader>(command))
                        {


                            List<GameOnUser> users = new List<GameOnUser>();

                            int count = reader.FieldCount;


                            while (reader.Read())
                            {

                                GameOnUser user = new GameOnUser();

                                for (int i = 0; i < count; i++)
                                {

                                    if (i == 0)
                                    {
                                        user.ID = reader.GetGuid(i).ToString();
                                    }
                                    else if (i == 3)
                                    {
                                        user.Username = reader.GetString(i);
                                    }

                                }

                                users.Add(user);
                            }

                            json = new JavaScriptSerializer().Serialize(users);
                        }
                    });
                }
            });

            return json;

I have a feeling that it is prone to failure but I may be wrong. Any advice on this chunk would be much appreciated.

\$\endgroup\$
0
7
\$\begingroup\$
//Build the query
string query = string.Format("SELECT  u2.UserId, u2.[first name], u2.[last name], u2.[username] FROM       Users u1 INNER JOIN Friends f1 ON u1.userid = f1.userid INNER JOIN Users u2 ON u2.userid = f1.FriendId INNER JOIN Friends f2 ON u2.userid = f2.friendid WHERE   ( f2.userid = '{0}'    OR f1.friendid = '{0}') AND u2.userid != '{0}';", userId);

It appears you are using string.Format to insert query parameters. The problem with this is that it opens up your API to SQL injection attacks.

For example, if someone were to somehow pass the following for userId:

' OR 1=1);

and an appropriate suffix so the overall query is valid SQL, it would return all users. Other injected strings could do worse like modify/drop tables.

Instead, you want to use a parameterized query by replacing your format specifier with an @-prefixed variable. Then, you can add parameters to the SqlCommand.Parameters collection:

const string QUERY = 
@"SELECT u2.UserId, u2.[first name], u2.[last name], u2.[username]
FROM Users u1
INNER JOIN Friends f1 ON u1.userid = f1.userid
INNER JOIN Users u2 ON u2.userid = f1.FriendId
INNER JOIN Friends f2 ON u2.userid = f2.friendid
WHERE ( f2.userid = @userId OR f1.friendid = @userId)
   AND u2.userid != @userId;";

using(var command = connection.CreateCommand())
{
    command.CommandText = QUERY;
    command.Parameters.Add("@userID", System.Data.SqlDbType.VarChar);
    command.Parameters[@"userID"].Value = userId;
}

As an aside, IDbCommand implements IDisposable, so you need to put the command object within a using statement.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks Dan I had no idea (the more you know), I'll be sure to make the appropriate changes throughout my code. Do you have any insight into how I am handling the response from the database? It could be correct but that was my main concern until you mentioned the SQL injection hazard. \$\endgroup\$ – Jack Nutkins Apr 8 '15 at 20:09
2
\$\begingroup\$

This loop:

                            for (int i = 0; i < count; i++)
                            {

                                if (i == 0)
                                {
                                    user.ID = reader.GetGuid(i).ToString();
                                }
                                else if (i == 3)
                                {
                                    user.Username = reader.GetString(i);
                                }

                            }

serves no purpose. Insead, just use

if (count > 0)
{
  user.ID = reader.GetGuid(0).ToString();
  if (count > 3)
  {
     user.Username = reader.GetString(3);
  }
}

Also, this seems like the kind of thing that LINQ (specifically LINQ-to-SQL) is tailor-made for.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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