7
\$\begingroup\$

I updated my Instagram API and I read that HttpClient is now the preferred way in .NET Core 3.1. I would like to get a code review, because there are too many examples and too many ways and I'm not quite sure if I did the migration correctly.

One more question: The reason I added _client.Timeout is in case the user is using proxy and it is not responding. I think that it's needed only on the first request. Can I somehow restrict it to the first request? Or a better way?

New code:

public class InstagramAPI
{
    private static HttpClient _client;

    public string Username { get; private set; }
    public string Password { get; private set; }

    public InstagramAPI(string username, string password, string proxy = null)
    {
        if (_client == null)
        {
            var handler = new HttpClientHandler();

            if (proxy != null)
            {
                handler.Proxy = new WebProxy(proxy, true);
            }

            if (handler.SupportsAutomaticDecompression)
            {
                handler.AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate;
            }

            _client = new HttpClient(handler)
            {
                Timeout = TimeSpan.FromSeconds(10)
            };
        }

        if (string.IsNullOrWhiteSpace(username))
            throw new ArgumentException("Username cannot be empty or null.");

        if (string.IsNullOrWhiteSpace(password))
            throw new ArgumentException("Password cannot be empty or null.");

        Username = username;
        Password = password;
    }

    public void Login()
    {
        try
        {
            // 1st request
            HttpRequestMessage httpRequest = new HttpRequestMessage(HttpMethod.Get, "https://www.instagram.com/");

            httpRequest.Headers.Host = "www.instagram.com";
            httpRequest.Headers.UserAgent.ParseAdd("Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0");
            httpRequest.Headers.Accept.ParseAdd("text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8");
            httpRequest.Headers.AcceptLanguage.ParseAdd("en-US");
            httpRequest.Headers.AcceptEncoding.ParseAdd("gzip, deflate, br");

            httpRequest.Headers.Add("Upgrade-Insecure-Requests", "1");

            var getsResponse = _client.SendAsync(httpRequest);
            var getResult = getsResponse.Result.Content.ReadAsStringAsync().Result;

            string web_key_id = Regex.Match(getResult, "\"key_id\":\"([^\"]*)").Groups[1].Value;
            string web_pub_key = Regex.Match(getResult, "\"public_key\":\"([^\"]*)").Groups[1].Value;
            string web_key_version = Regex.Match(getResult, "\"version\":\"([^\"]*)").Groups[1].Value;
            string csrfToken = Regex.Match(getResult, "\"csrf_token\":\"([^\"]*)").Groups[1].Value;
            string rolloutHash = Regex.Match(getResult, "\"rollout_hash\":\"([^\"]*)").Groups[1].Value;

            // 2nd request
            string encryptedPassword = EncryptPassword(Password, web_pub_key, web_key_id, web_key_version);
            HttpRequestMessage httpRequest2 = new HttpRequestMessage(HttpMethod.Post, "https://www.instagram.com/accounts/login/ajax/");

            httpRequest2.Content = new FormUrlEncodedContent(new[]
            {
                new KeyValuePair<string, string>("username", Username),
                new KeyValuePair<string, string>("enc_password", encryptedPassword),
                new KeyValuePair<string, string>("queryParams", "{}"),
                new KeyValuePair<string, string>("optIntoOneTap", "false")
            });

            httpRequest2.Headers.Host = "www.instagram.com";
            httpRequest2.Headers.UserAgent.ParseAdd("Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0");
            httpRequest2.Headers.Accept.ParseAdd("*/*");
            httpRequest2.Headers.AcceptLanguage.ParseAdd("en-US");
            httpRequest2.Headers.AcceptEncoding.ParseAdd("gzip, deflate, br");

            httpRequest2.Headers.Add("X-CSRFToken", csrfToken);
            httpRequest2.Headers.ConnectionClose = true;

            var postResponse = _client.SendAsync(httpRequest2);
            var postResult = postResponse.Result.Content.ReadAsStringAsync().Result;
            
            ...
        }
        catch (AggregateException ex)
        {
            if (ex.InnerException is TaskCanceledException)
            {
                // return proxy error
                MessageBox.Show("We could not connect. It could a proxy problem, if you were using one.", "Connection error", MessageBoxButtons.OK, MessageBoxIcon.Error);
            }
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message, "Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
        }
    }
}

Old code:

void Login(string postData)
{
    // 1st request
    HttpWebRequest request = WebRequest.Create("https://www.instagram.com/") as HttpWebRequest;
    request.Method = "GET";
    request.AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate;

    if (proxyAddress != null)
    {
        request.Proxy = new WebProxy(proxyAddress);
    }
    else
    {
        // The first request takes an abnormally long time: This occurs because WebRequest by default detects and loads proxy settings the first time it starts,
        // which can take quite a while.To stop this, simply set the proxy property (WebRequest.Proxy) to null and it'll bypass the check
        request.Proxy = null;
    }

    request.Host = "www.instagram.com";
    request.UserAgent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0";
    request.Accept = "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8";

    request.Headers.Add("Accept-Language", "en-US");
    request.Headers.Add("Accept-Encoding", "gzip, deflate, br");

    CookieCollection cookies = new CookieCollection();

    IAsyncResult response = request.BeginGetResponse(new AsyncCallback(ReadCallback), request);

    // this line implements the timeout, if there is a timeout, the callback fires and the request becomes aborted
    ThreadPool.RegisterWaitForSingleObject(response.AsyncWaitHandle, new WaitOrTimerCallback(TimeoutCallback), request, 2000, true);

    // The response came in the allowed time. The work processing will happen in the callback function
    _manualResetEvent.WaitOne();

    if (RequestResult == null)
    {
        return ResultType.ProxyError;
    }

    string web_key_id = Regex.Match(RequestResult, "\"key_id\":\"([^\"]*)").Groups[1].Value;
    string web_pub_key = Regex.Match(RequestResult, "\"public_key\":\"([^\"]*)").Groups[1].Value;
    string web_key_version = Regex.Match(RequestResult, "\"version\":\"([^\"]*)").Groups[1].Value;
    string csrfToken = Regex.Match(RequestResult, "\"csrf_token\":\"([^\"]*)").Groups[1].Value;
    string rolloutHash = Regex.Match(RequestResult, "\"rollout_hash\":\"([^\"]*)").Groups[1].Value;

    // 2nd request
    string encryptedPassword = GenerateEncPassword(password, web_pub_key, web_key_id, web_key_version);

    byte[] data = Encoding.UTF8.GetBytes(postData);

    HttpWebRequest request2 = WebRequest.Create("https://www.instagram.com/accounts/login/ajax/") as HttpWebRequest;
    request2.Method = "POST";
    request2.AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate;

    if (proxyAddress != null)
    {
        request2.Proxy = new WebProxy(proxyAddress);
    }
    else
    {
        request2.Proxy = null;
    }

    request2.Host = "www.instagram.com";
    request2.UserAgent = "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0";
    request2.Accept = "*/*";
    request2.ContentType = "application/x-www-form-urlencoded";
    request2.Referer = "https://www.instagram.com/";

    request2.Headers.Add("Accept-Language", "en-US");
    request2.Headers.Add("Accept-Encoding", "gzip, deflate, br");
    request2.Headers.Add("X-CSRFToken", csrfToken);

    request2.ContentLength = data.Length;

    try
    {
        using (Stream stream = request2.GetRequestStream())
        {
            stream.Write(data, 0, data.Length);
        }

        using (HttpWebResponse response2 = request2.GetResponseWithoutException() as HttpWebResponse)
        {
            using (StreamReader reader = new StreamReader(response2.GetResponseStream()))
            {
                RequestResult = reader.ReadToEnd();
            }
        }
    }
    catch (WebException e)
    {
        if (e.InnerException is SocketException)
        {
            return ResultType.ProxyError;
        }

        return ResultType.ProxyError;
    }
    
    ...
}

private ManualResetEvent _manualResetEvent = new ManualResetEvent(false);

public string RequestResult { get; private set; }

private void TimeoutCallback(object state, bool timedOut)
{
    if (timedOut)
    {
        HttpWebRequest request = (HttpWebRequest)state;
        if (request != null)
        {
            request.Abort();
        }
    }
}

private void ReadCallback(IAsyncResult asyncResult)
{
    HttpWebRequest request = (HttpWebRequest)asyncResult.AsyncState;

    try
    {
        using (HttpWebResponse response = (HttpWebResponse)request.EndGetResponse(asyncResult))
        {
            using (StreamReader reader = new StreamReader(response.GetResponseStream()))
            {
                RequestResult = reader.ReadToEnd();
            }
        }
    }
    catch
    {
    }

    _manualResetEvent.Set();
}
```
\$\endgroup\$
4
13
\$\begingroup\$
var postResponse = _client.SendAsync(httpRequest2);
var postResult = postResponse.Result.Content.ReadAsStringAsync().Result;

Never use tasks like this, it leads to dead-locks very easily. Plus it's wasteful, you have an entire framework (HttpClient) designed to keep your application responsive, and instead you freeze it for no good reason.

  1. Make your Login function async and have it return a Task<bool> (or even better, an enum that describes exactly why it failed if it did)
  2. Properly await those async calls (SendAsync and ReadAsStringAsync)
  3. Change Login to LoginAsync to follow the framework naming conventions
  4. Get rid of those random MessageBox calls in your class, they do nothing but prevent unit testing and introduce WinForms dependencies in what could otherwise be a nice service class.
\$\endgroup\$
7
  • 1
    \$\begingroup\$ First of all, thank you for your answer! I actually did the await part right after I wrote the question. Fair point about the MessageBox's. Updated the code in the question, thanks again and I'm open for more corrections! \$\endgroup\$ – nop Jul 28 '20 at 18:20
  • 1
    \$\begingroup\$ You still have a postResponse.Content.ReadAsStringAsync().Result! \$\endgroup\$ – Blindy Jul 28 '20 at 18:22
  • 1
    \$\begingroup\$ Also instead of doing those repeated and error-prone Contains calls, decode the response (I'm assuming it's JSON) and read the structured properties directly. \$\endgroup\$ – Blindy Jul 28 '20 at 18:25
  • 1
    \$\begingroup\$ No, I mean use JsonConverter.Deserialize<T>(jsonString), where T is a class you write that contains the fields you're interested in from the response (like string Message; etc), and then use the class to determine the return value instead of doing string operations. \$\endgroup\$ – Blindy Jul 28 '20 at 18:52
  • 1
    \$\begingroup\$ And if you're not familiar with JSON parsing, make sure you define a clear enum for the status, don't parse it as a string, that will lead to more if..else if..else if.. \$\endgroup\$ – Blindy Jul 28 '20 at 18:54
3
\$\begingroup\$

I don't want to repeat Blindly's observations (and those that have spotted under the comment section).

I would like to suggest other improvement areas:

Constructor

You are using instance constructor to initialize static member.

  • Static constructor should be used to initialize your HttpClient with(out) proxy
  • There is no need to use instance constructor
    • UserName and Password should be a parameter of the Login

Login - In general

It does seems to me that you want to implement something on the server-side which was designed to support client-side authentication.

  • Please check that does Instagram support server-side auth

Login - Error handling

This error handling is really naive and thin.

  • You should definitely look for HttpRequestException.
  • You should check the responses' StatusCode before any further processing

Login - Flow

Separation helps the understanding and maintainability of a function. Smaller chunks can be easier tested, etc.

  • I suggest to separate the two separate requests into two separate functions
  • Your Login should be the coordinator for the two functions

HttpClient - Header

You are using a lot of strings to setup requests. Most of them are duplicated.

  • Please consider to use constants instead
  • Consider to set the Host on the HttpClient level instead of on the HttpWebRequest level

HttpClient - Timeout

In case of .NET Core the recommended approach to enforce timeout is to use the Polly library. It is a quite neat utility and it exposes other useful policies as well (like circuit-breaker, bulkhead)

\$\endgroup\$
6
  • \$\begingroup\$ Thank you for your suggestions! I applied your constructor and error handling suggestions, excluding the status codes because Instagram returns kinda random status codes in case of two-factor authentication/checkpoint required or so. They are returning the response as JSON. I like the single responsibility concept (S.O.L.I.D.) for the login function. Constants would also be awesome as you mentioned. Do you recommend me to put the constants into that class or a different class with singleton pattern implementation? I will definitely try out Polly. \$\endgroup\$ – nop Jul 29 '20 at 9:26
  • 1
    \$\begingroup\$ @nop 1) It does not matter from where the status code is coming. It is the primary indicator of the request processing successfulness. For example if it is 5xx then you shouldn't looking for key_id in the response body. 2) SRP is a kind a good guidance for every length function :D 3) If you need to reuse these constants across multiple classes then move them into a separate class and mark it as static. You don't have to make it as singleton. \$\endgroup\$ – Peter Csala Jul 29 '20 at 10:20
  • \$\begingroup\$ Fair point. Btw, is it only me who cannot find a way to set up proxy only for HttpRequestMessage instead of the whole HttpClient object? In case it is like that, I will probably have to move the HttpClient instance into the LoginAsync method. \$\endgroup\$ – nop Jul 29 '20 at 10:33
  • 1
    \$\begingroup\$ What are the advantages of using Polly to set the timeout instead of simply using the property? Polly is nice if you're using the more advanced features (at which point an argument could be made to be consistent and use it for everything), but in this case it's overkill with no benefits. Sidenote: Static constructors are pretty awful from a performance PoV so if one can avoid it, that's a good idea - just use Lazy instead. \$\endgroup\$ – Voo Jul 29 '20 at 12:20
  • 1
    \$\begingroup\$ @Peter Sure if you're going to use the extra features, it makes sense to be consistent. But if nothing but a simple timeout is needed the property will do just fine imo. \$\endgroup\$ – Voo Jul 29 '20 at 12:31

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.