6
\$\begingroup\$

I wrote a method early of our development last year about getting the content of a webrequest where it contains an xml data that we need to process and insert to database. I wanted to improve this, so I'm asking a feedback on these code snippets.

public async Task<BulkStatusCode> AuthenticateConnection(string url, string user, string access, string recordType)
{
    try
    {
        // TODO : this is just a quickfix, create a logic that will determine if the url is using ssl or not
        url = url.Replace("http://", "https://");

        var uri = new Uri(url);
        //ServicePointManager.ServerCertificateValidationCallback = ((sender, certificate, chain, SslPolicyErrors) => true);

        var request = WebRequest.Create(uri) as HttpWebRequest;

        request.PreAuthenticate = true;
        //will ignore certificate validation
        request.ServerCertificateValidationCallback = ((sender, certificate, chain, SslPolicyErrors) => true);
        request.Method = WebRequestMethods.Http.Get;
        request.Credentials = BulkAuthentication.GetCredentialCache(uri, user, access);

        //check if status is 200 and content type is correct
        using (var response = await request.GetResponseAsync() as HttpWebResponse)
        {
            if (response.StatusCode.Equals(HttpStatusCode.OK))
            {
                if (!ReferenceEquals(response.ContentType, null) && IsValidContentType(response.ContentType))
                {
                    using (var stream = response.GetResponseStream())
                    {
                        //store stream to variable
                        using (var ms = new MemoryStream())
                        {
                            stream.CopyTo(ms);
                            var storeStreamToCache = ms.ToArray();
                            var storeStreamToValidate = ms.ToArray();

                            using (Stream streamToValidate = new MemoryStream(storeStreamToValidate))
                            {
                                using (var reader2 = new StreamReader(streamToValidate))
                                {
                                    if (!XmlValidator.IsValidXml(reader2, XmlSchemaType(recordType)))
                                        return BulkStatusCode.UnsupportedXML;
                                }
                            }

                            using (Stream streamToCache = new MemoryStream(storeStreamToCache))
                            {
                                using (var reader = new StreamReader(streamToCache))
                                {
                                    if (recordType.Equals("Foo"))
                                    {
                                        var serializer = new XmlSerializer(typeof(BulkFooRoot));
                                        userXmlData = (BulkFooRoot)serializer.Deserialize(reader);
                                        _cache.Add("BulkFooUserXml", fooXmlData);
                                    }
                                    else if (recordType.Equals("Bar"))
                                    {
                                        var serializer = new XmlSerializer(typeof(BulkBarRoot));
                                        projectXmlData = (BulkBarRoot)serializer.Deserialize(reader);
                                        _cache.Add("BulkUploadBarXml", projectXmlData);
                                    }
                                }
                            }
                        }
                    }
                    //valid content type but has invalid xml content
                    return BulkStatusCode.OK;
                }
                //invalid content type
                return BulkStatusCode.UnsupportedMediaType;
            }
        }
    }
    catch (WebException ex)
    {
        var responseStatusCode = ((HttpWebResponse)ex.Response).StatusCode;
        if (ReferenceEquals(ex.Response, null) || !responseStatusCode.Equals(HttpStatusCode.Unauthorized))
            //invalid request (http not found)
            return BulkStatusCode.NotFound;
    }
    // retry status to retry to login again
    return BulkStatusCode.Retry;
}

private static bool IsValidContentType(string contentType)
{
    switch (contentType)
    {
        case "text/xml":
        case "application/xml; charset=utf-8":
            return true;
    }
    return false;
}
\$\endgroup\$
3
\$\begingroup\$

A few notes to kick you off:

// TODO : this is just a quickfix, create a logic that will determine if the url is using ssl or not
url = url.Replace("http://", "https://");
var uri = new Uri(url);

I consider updating input parameters to be a really bad idea... Factor out a method to handle this:

var requestUri = GetSecureUri(url);

which could look something like:

public Uri GetSecureUri(string url)
{
   if (string.IsNullOrEmpty(url))
   {
       throw new ArgumentNullException("url");
   }
   Uri requestUri;
   if (!Uri.TryCreate(url, UriKind.Absolute, out requestUri))
   {
       throw new ArgumentException("url must be a valid absolute URI.", "url");
   }
   if (requestUri.Scheme == Uri.UriSchemeHttps)
   {
       return requestUri;
   }
   return new UriBuilder(requestUri)
   {
       Port = requestUri.IsDefaultPort ? -1 : requestUri.Port,
       Scheme = Uri.UriSchemeHttps

   }.Uri;
}

Is that "better"? Maybe - but probably not worth all the extra effort.


//will ignore certificate validation
request.ServerCertificateValidationCallback = ((sender, certificate, chain, SslPolicyErrors) => true);

No! NO! NO! NO! NO! NO!. If you're going to force TLS (a good thing for sensitive information) you need to check the certificate. If you ignore all the validation then someone could give you a self signed cert for a service they don't own and you wouldn't even know. Don't do this. I can't even find words to use to fully convey the amount I've just lost faith in humanity.


Use the normal cast when you know it's going to succeed: WebRequest.Create(uri) as HttpWebRequest should be (HttpWebRequest)WebRequest.Create(uri).

I'll probably come back later and review a bit more.

\$\endgroup\$
  • \$\begingroup\$ I really appreciate that you did the method for https thanks you for this and also thanks for the codes you point out so I can prevent doing this in the future. \$\endgroup\$ – rpmansion Aug 26 '16 at 6:58
  • \$\begingroup\$ @rpmansion - no problem. Sorry if I was a bit hard on you about the certificate validation. I just wanted to be sure you'd realise it was bad idea :) \$\endgroup\$ – RobH Aug 26 '16 at 9:24
  • \$\begingroup\$ No worries it was worth pointing out so I wont do it next time \$\endgroup\$ – rpmansion Aug 26 '16 at 9:52
  • \$\begingroup\$ Quick question in this code, Port = requestUri.IsDefaultPort ? -1 : requestUri.Port, Scheme = Uri.UriSchemeHttps. Why do you set it to -1 if it is true? \$\endgroup\$ – rpmansion Aug 26 '16 at 9:52
  • \$\begingroup\$ @rpmansion -1 means use the default port for the scheme. \$\endgroup\$ – RobH Aug 26 '16 at 10:05
1
\$\begingroup\$

Your nesting is so deep that I need a scuba tank to keep diving ;-)

To reduce it you should return from the method as soon as the condition is false and there is no else rather then nesting another if inside.

if (!response.StatusCode.Equals(HttpStatusCode.OK))
{
    ... return
}

next line

if (ReferenceEquals(response.ContentType, null) || !IsValidContentType(response.ContentType))
{
    ... return
}

and so on...

You can also reduce using nesting by putting them one below the other:

using (var stream = response.GetResponseStream())
using (var ms = new MemoryStream())//store stream to variable
{
    ... do stuff
}

here you can even remove one using as the MemoryStream can take the response stream as a parameter in its constructor.

I don't understand why you are copying this stream twice - for caching and for validating. You can do both, just rewind it after you've validated it with MemoryStream.Seek.

\$\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.