Logging into WebAPI 2 site from c# desktop application

Question

I'm trying to login into a webapi2 site from a desktop application. After a lot of googling, I cobbled together a working prototype. Since we are talking about security I wanted to do a peer review. I'm just starting with security design and I'm not exactly sure about my design.

The WebAPI site that is targeted is a wepapi 2 standard template out of visual studio.

I'm mainly looking for security issuesut general issues are also greatly appreciated.

One issue I'm aware of is that SSL should be added and with that ssl validation. I haven't implemented it yet.

static internal async Task<string> GetWebAPIReguest(string siteUrl, string APIController, string userName, string Password)
{
    TokenResponseModel Token = await GetBearerToken(siteUrl, userName, Password);

    HttpClient client = new HttpClient();
    client.BaseAddress = new Uri(siteUrl);
    client.DefaultRequestHeaders.Accept.Clear();
    client.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
    client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", Token.AccessToken);

    return await client.GetStringAsync(APIController);
}

static internal async Task<TokenResponseModel> GetBearerToken(string siteUrl, string Username, string Password)
{
    HttpClient client = new HttpClient();
    client.BaseAddress = new Uri(siteUrl);
    client.DefaultRequestHeaders.Accept.Clear();

    HttpContent requestContent = new StringContent("grant_type=password&username=" + Username + "&password=" + Password, Encoding.UTF8, "application/x-www-form-urlencoded");

    HttpResponseMessage responseMessage = await client.PostAsync("Token", requestContent);

    if (responseMessage.IsSuccessStatusCode)
    {
        string jsonMessage;
        using (Stream responseStream = await responseMessage.Content.ReadAsStreamAsync())
        {
            jsonMessage = new StreamReader(responseStream).ReadToEnd();
        }

        TokenResponseModel tokenResponse = (TokenResponseModel)JsonConvert.DeserializeObject(jsonMessage, typeof(TokenResponseModel));

        return tokenResponse;
    }
    else
    {
        return null;
    }
}

class TokenResponseModel
{
    [JsonProperty("access_token")]
    public string AccessToken { get; set; }

    [JsonProperty("token_type")]
    public string TokenType { get; set; }

    [JsonProperty("expires_in")]
    public int ExpiresIn { get; set; }

    [JsonProperty("userName")]
    public string Username { get; set; }

    [JsonProperty(".issued")]
    public string IssuedAt { get; set; }

    [JsonProperty(".expires")]
    public string ExpiresAt { get; set; }
}

Answer 1

I was going to make a very large post with altered code, but I don't fully understand what your code is doing so all I will suggest is that

1. You need to use a consistent code style. camelCase for variables/arguments, PascalCase for runtime constants (types/constants) and properties, and _camelCase for private fields on an object.
2. Your methods are simply doing way too much. Your 2 methods there are connecting to two different web pages, composing two URLs, parsing JSON into a POCO (and then back out again), and doing authentication on top of all of that.
3. You aren't managing the lifecycle of HttpClient. There is a general consensus that creating and disposing HttpClients is not the best use of a class. At the very least you should be Disposing of your HttpClients but all you're doing in your current code is letting your HttpClients leak. Consider that your functions do not need to know how to create a HttpClient, they just need to know how to connect to a Url - let the caller of the function do the heavy lifting for you.

Once again I'd give you a code sample but I just don't understand what your code is trying to do, which is my own fault, because I haven't really used much HTTP in .NET.

Here are the responsibilities of your two methods so far:

• Create a HttpClient
• Decode JSON into a POCO
• Create authentication token
• Compose url from base url, secondary url and a query string
• Apply authentication token to HttpClient
• Handle the response of one of the HttpClients
• Prepare HttpClient headers

The majority of these should be in their own function.

Answer 2

I don't like that the password is hard coded here

HttpContent requestContent = new StringContent("grant_type=password&username=" + Username + "&password=" + Password, Encoding.UTF8, "application/x-www-form-urlencoded");

I am sure that you could move it into a configuration file or something.

---

and this is a bit wordy

if (responseMessage.IsSuccessStatusCode)
{
    string jsonMessage;
    using (Stream responseStream = await responseMessage.Content.ReadAsStreamAsync())
    {
        jsonMessage = new StreamReader(responseStream).ReadToEnd();
    }

    TokenResponseModel tokenResponse = (TokenResponseModel)JsonConvert.DeserializeObject(jsonMessage, typeof(TokenResponseModel));

    return tokenResponse;
}
else
{
    return null;
}

I would wrap more inside the using statement so that I could combine some things like this

if (responseMessage.IsSuccessStatusCode)
{
    using (Stream responseStream = await responseMessage.Content.ReadAsStreamAsync())
    {
        return (TokenResponseModel)JsonConvert.DeserializeObject(new StreamReader(responseStream).ReadToEnd(), typeof(TokenResponseModel));
    }
}
else
{
    return null;
}

---

@Rolfl made a very good observation/point.

If we negate the condition and return null we can make it so that the using block is not nested inside of an if statement. so it would look like this

if (!responseMessage.IsSuccessStatusCode) { return null;}

using (Stream responseStream = await responseMessage.Content.ReadAsStreamAsync())
{
    return (TokenResponseModel)JsonConvert.DeserializeObject(new StreamReader(responseStream).ReadToEnd(), typeof(TokenResponseModel));
}

This is far cleaner than the OP code.