# Authenticating web request to get the xml data from it

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))
{
{
return BulkStatusCode.UnsupportedXML;
}
}

using (Stream streamToCache = new MemoryStream(storeStreamToCache))
{
{
if (recordType.Equals("Foo"))
{
var serializer = new XmlSerializer(typeof(BulkFooRoot));
}
else if (recordType.Equals("Bar"))
{
var serializer = new XmlSerializer(typeof(BulkBarRoot));
}
}
}
}
}
//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))
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;
}


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.

• 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. – rpmansion Aug 26 '16 at 6:58
• @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 :) – RobH Aug 26 '16 at 9:24
• No worries it was worth pointing out so I wont do it next time – rpmansion Aug 26 '16 at 9:52
• 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? – rpmansion Aug 26 '16 at 9:52
• @rpmansion -1 means use the default port for the scheme. – RobH Aug 26 '16 at 10:05

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.