2
\$\begingroup\$

I created a bunch of small and mostly internal WPF applications for my company - usually for some file manipulation etc. For these programs, I would like to start collecting user info - who uses them, how often, most importantly - which version is used.

It's partly for my personal skill development, but I also want to introduce auto-updating for my apps - some of them have this feature already, but they rely on text files located on shared drive so obviously not a good solution.

The intended audience is not a large crowd, probably fewer than 50 users. The calls to the database will most likely happen only on the application launch.

The idea is that I have a generic 'helper' dll, which I reference in my projects so that in each of the programs I call a simple method and have the repetitive registration handling out of my mind. All the data is stored in a single Azure database called MyDB, and all the programs should connect through the same API (also hosted in Azure), called e.g. MyApi (myapi.azuresites.net).

So, looking from the point of view of a single client app:

 public MainWindow()
        {
            InitializeComponent();
            Register = new RegistrationHelper();
            var progressInfo = new Progress<string>(p => StatusInfo = p);
            Register.HandleRegistration(progressInfo);
            //and that's it
    }

Now, the helper library contains a smart constructor that will deduct all the info I need for my database (email is optional only for some programs with explicit registration):

private const string DefaultApiAddress = "https://MyApi.azuresites.net";
public RegistrationHelper(string email = null)
{
    ApiUri = new Uri(DefaultApiAddress);
    UserName = Environment.UserName;
    MachineName = Environment.MachineName;
    //email is not needed for registration
    UserProvidedEmail = email;
    CurrentAssembly = Assembly.GetEntryAssembly();
    ProgramName = CurrentAssembly.GetName().Name;
    ProgramVersion = FileVersionInfo.GetVersionInfo(CurrentAssembly.Location).FileVersion;
}

Then it has a public method for running handling the registration (which obviously should not block program execution):

public void HandleRegistration(IProgress<string> progressInfo)
{
    Task.Run((() => HandleRegistrationAsync(progressInfo)));
}

Which calls a private handler:

private async Task HandleRegistrationAsync(IProgress<string> progressInfo)
{
    try
    {
        progressInfo.Report("Connecting...");
        RegisteredUserId = await GetUserIdAsync();
        if (RegisteredUserId == 0)
        {
            RegisteredUserId = await RegisterUserByNameAsync();
            if (RegisteredUserId > 0)
            {
                progressInfo.Report(string.Format("Newly registered. Id: {0}", RegisteredUserId));
            }
            else
            {
                progressInfo.Report("Registration failed");
            }
        }
        else if (RegisteredUserId < 0)
        {
            progressInfo.Report(string.Format("Not registered. {0}", RegisteredUserId));
        }
        else
        {
            progressInfo.Report(string.Format("Updating user {0} info...", RegisteredUserId));
            progressInfo.Report("Registered. Id: " + RegisteredUserId);
        }
    }
    catch (Exception ex)
    {
        progressInfo.Report("Connection error");
        if (ThrowExceptions)
            throw ex;
    }

And the methods that call the API are all pretty similar, structured as follows:

public async Task<int> UpdateProgramUsageAsync()
{
    try
    {
        using (var client = new HttpClient())
        {
            client.BaseAddress = ApiUri;
            client.DefaultRequestHeaders.Accept.Clear();
            HttpResponseMessage response = await client.GetAsync(string.Format("api/update/{0}/{1}/{2}/", RegisteredUserId, ProgramName, ProgramVersion));
            return response.IsSuccessStatusCode
                ? Convert.ToInt32(response.Content.ReadAsStringAsync().Result)
                : -1;
        }
    }
    catch (Exception)
    {
        if (ThrowExceptions)
            throw;
        else
            return -666;
    }
}

Now, what's in the web API:

There is a single UserController (apart from the stuff that comes out of the box from the WebApi template). It contains the connection string to the database (hardcoded), plus a bunch of constants for StoredProcedures names and stuff like that.

Then, my controller methods are structured as follows:

private const string ConnectionString = "allthestuffsensitivetoconnecttodatabase";
[Route("api/getid/{username}")]
public async Task<int> GetIdAsync(string username)
{
    using (var conn = new SqlConnection(ConnectionString))
    {
        conn.Open();
        if (conn.State == ConnectionState.Open)
        {
            var result = await ExecuteScalarCommandAsync(GetRegisteredUserByName, conn,
                new[]
                    {
                        new SqlParameter(UserNameCol, username), 
                    });
            if (result != null && !(result is DBNull))
            {
                return Convert.ToInt32(result);
            }
            return 0;
        }
    }
    return -1;
}

private async Task<object> ExecuteScalarCommandAsync(string commandName, SqlConnection conn, SqlParameter[] parameters)
{
    using (var command = new SqlCommand(commandName, conn)
    {
        CommandType = CommandType.StoredProcedure,
    })
    {
        command.Parameters.AddRange(parameters);
        return await command.ExecuteScalarAsync();
    }
}

Also, as for the methods that not only 'get' values, but also update database, I am using the GET as well.

The one below is a match of UpdateProgramUsageAsync() pasted above:

[Route("api/update/{userid}/{programname}/{version}")]
[AcceptVerbs("GET")]
public async Task<int> UpdateProgramUsageAsync(string userid, string programname, string version)
{
    using (var conn = new SqlConnection(ConnectionString))
    {
        conn.Open();
        if (conn.State == ConnectionState.Open)
        {
            var result = await ExecuteScalarCommandAsync(UpdateProgramUsage, conn,
        new[]
            {
                new SqlParameter(UserIdCol, userid), 
                new SqlParameter(ProgramNameCol, programname), 
                new SqlParameter(VersionCol, version)
            }
        );
            if (result != null && !(result is DBNull))
            {
                return Convert.ToInt32(result);
            }
        }
    }
    return 0;
}

As you can see, it relies on stored procedures updating the database and returning some response codes. The procedures are pretty simple (get value, put value into a table, nothing fancy).

\$\endgroup\$
1

1 Answer 1

2
\$\begingroup\$

You shouldn't use const for your website's url nor for the connection string. They should be in an app.config. Azure also offers some kind of online configuration file you can use, which is kind of better than the app.config since it's all encrypted in the cloud.

Register isn't a good name for a RegistrationHelper, you should name it RegistrationHelper. As you can see, it's much clearer :p

The method HandleRegistration is weird. You know this method is async right? Task.Run returns a Task to be awaited and it's a bad plan not to keep the pointer to the Task. You can't ever be sure what happens with your Task once you start it and loose the pointer to the Task. Maybe the database connection will take 3 minutes because of some random latency? Also, throwing your exception in your async task will only stop the Task. You'll never be aware of the exception because the Exception is stored in the Task's object, which you don't have.

If you want HandleRegistration to be sync, that's what you'd need to do :

public void HandleRegistration(IProgress<string> progressInfo)
{
    Task.Run((() => HandleRegistrationAsync(progressInfo))).Wait();
}

If you want it to be async, you'll need to read on the pattern again, because something is wrong.

Finally, your API as a big security flaw. You're using an HttpGet to update your database. What does that mean? It means that if I send http get requests or use Cross-site scripting to inject an HttpGet request on vulnerable websites to your website, you've been hacked. Your data will be corrupted because nothing would stop anyone from sending thousands of thousands of requests to your site. You need to use HttpPost and to setup security measures on your website so this doesn't happen.

Overall, you need to check how to use the async/await and read a little bit on web security! :)

\$\endgroup\$
1
  • \$\begingroup\$ TopinFrassi - Would you mind seeing my answer below? It's partial implementation of your comments, but not large enough to post as a new question I suppose. \$\endgroup\$
    – Bartosz
    Nov 2, 2015 at 10:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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