Best way to identify a computer in LAN C#

Question

I have a web application to manage computers. Every registered computer is locally querying the latest information and updates the database with it in a 10 minute cycle.

But the computer is only allowed to update its information and nothing else. Currently I'm identifying the computer like this:

I get its hostname from the "remote_addr" variable

string senderHostname = (Dns.GetHostEntry(Request.ServerVariables["remote_addr"]).HostName).Split('.')[0];

Then I compare the hostname of the sender computer with the hostname of the computer which should be updated.

if (senderHostname.Equals(targetHostname, StringComparison.InvariantCultureIgnoreCase)) {
 // allowed
}
else {
 // forbidden
}

If they are the same, then I'm allowing the update, otherwise not.

I know that this code is very very ugly and I would be interested in a best practice to do this.

All the code:

protected void Page_Load(object sender, EventArgs e)
{
    try
    {
        clientMachineName = (Dns.GetHostEntry(Request.ServerVariables["remote_addr"]).HostName).Split('.')[0];
    }
    catch (Exception)
    {
        clientMachineName = "n/a";
    }

    if (!String.IsNullOrEmpty(Request.QueryString["status"]) && !String.IsNullOrEmpty(Request.QueryString["value"]))
    {
        String macAddress = Request.QueryString["status"];
        String statusCode = Request.QueryString["value"];

        if (clientMachineName.Equals(getHostName(macAddress), StringComparison.InvariantCultureIgnoreCase))
        {
            ComputerDbCommunicator communicator = new ComputerDbCommunicator();
            communicator.insertIntoComputerDb("UPDATE [Computer] SET [statuscode] = " + statusCode + " WHERE [macaddress] = '" + macAddress + "'");

            communicator.closeConnections();
            communicator = null;

            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write(statusCode);
            Response.Flush();
            Response.End();
        }
        else
        {
            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write("Operation not allowed");
            Response.Flush();
            Response.End();
        }
    } 
    else if (!String.IsNullOrEmpty(Request.QueryString["status"]))
    {
        String macAddress = Request.QueryString["status"];
        String statusCode = null;
        ComputerDbCommunicator communicator = new ComputerDbCommunicator();
        SqlDataReader dr = communicator.queryComputerDb("SELECT [statuscode] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
        if (dr.HasRows)
        {
            while (dr.Read())
            {
                statusCode = dr[0].ToString();
            }
            dr.Close();

            Response.Clear();
            Response.ClearHeaders();
            Response.AddHeader("Content-Type", "text/plain");
            Response.Write(statusCode);
            Response.Flush();
            Response.End();

            communicator.closeConnections();
            communicator = null; 
        }
    }
}

protected String getHostName(String macAddress)
{
    String hostName = null;
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    SqlDataReader dr = communicator.queryComputerDb("SELECT [hostname] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
    while (dr.Read())
    {
        hostName = dr[0].ToString();
    }
    dr.Close();

    communicator.closeConnections();
    communicator = null;

    return hostName;
}

Answer 1

Instead of reading the querystrings multiple times, you should consider to read them once and store them in a variable.

---

You should use Parameter's for your query to avoid sql injection.

---

Instead of Request.ServerVariables["remote_addr"]).HostName you should give Request.UserHostAddress a try.

See also: https://stackoverflow.com/q/13994582/2655508

---

Using a guard clause for the case that macAddress is null or empty will add readability to your code.

---

The selecting and updating of the statuscode should be extracted to separate methods.

---

After implementing the changes mentioned above (except the parameterized query) and the change @Pimgd suggested we will get

protected void Page_Load(object sender, EventArgs e)
{
    try
    {
        clientMachineName = (Request.UserHostAddress.HostName).Split('.')[0];
    }
    catch (Exception)
    {
        clientMachineName = "n/a";
    }

    String macAddress = Request.QueryString["status"];
    String statusCode = Request.QueryString["value"];

    if (String.IsNullOrEmpty(macAddress)) { return; }

    if (!String.IsNullOrEmpty(statusCode))
    {
        if (clientMachineName.Equals(getHostName(macAddress), StringComparison.InvariantCultureIgnoreCase))
        {
            UpdateStatusCode(statusCode);
            CreateResponse(statusCode);
        }
        else
        {
            CreateResponse("Operation not allowed");
        }
    }
    else
    {
        statusCode = GetStatusCode(macAddress);
        if (!String.IsNullOrEmpty(statusCode))
        {
            CreateResponse(statusCode);
        }
    }
}

protected void UpdateStatusCode(string statusCode)
{
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    communicator.insertIntoComputerDb("UPDATE [Computer] SET [statuscode] = " + statusCode + " WHERE [macaddress] = '" + macAddress + "'");

    communicator.closeConnections();
    communicator = null;
}

protected string GetStatusCode(String macAddress)
{
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    SqlDataReader dr = communicator.queryComputerDb("SELECT [statuscode] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
    if (!dr.HasRows) { return String.Empty; }

    string statusCode = String.Empty;
    while (dr.Read())
    {
        statusCode = dr[0].ToString();
    }
    dr.Close();

    communicator.closeConnections();
    communicator = null;

    return statusCode;
}

protected void CreateResponse(string response)
{
    Response.Clear();
    Response.ClearHeaders();
    Response.AddHeader("Content-Type", "text/plain");
    Response.Write(response);
    Response.Flush();
    Response.End();
}

protected String getHostName(String macAddress)
{
    String hostName = null;
    ComputerDbCommunicator communicator = new ComputerDbCommunicator();
    SqlDataReader dr = communicator.queryComputerDb("SELECT [hostname] FROM [Computer] WHERE [macaddress] = '" + macAddress + "'");
    while (dr.Read())
    {
        hostName = dr[0].ToString();
    }
    dr.Close();

    communicator.closeConnections();
    communicator = null;

    return hostName;
}

Answer 2

You've got some duplication in there;

Perhaps you can create a function with a string argument that will handle response writing?

protected void CreateResponse(string response){
    Response.Clear();
    Response.ClearHeaders();
    Response.AddHeader("Content-Type", "text/plain");
    Response.Write(response);
    Response.Flush();
    Response.End();
}

To me that seems far more concerning than the way in which you retrieve the hostname.