Refactoring public API to reduce security risks

Question

In our application, the PublicManagerService class in the WCF layer is showing signs of exponential growth, which is resulting in methods being exposed in the public API which can lead to increased risks to our security, a doorway for malicious attacks and also a decrease in performance.

An example of this can be seen in our AccountController class, in the Accept and Decline methods which are used upon a user clicking on a hyperlink in an email. The following calls are made for methods in the public API:

• GetTransactionEmailLogByGUID
• UpdateTransactionHitCount
• SaveApprovalLog
• GeneratePDF
• UploadBlobByType
• SetCurrentCulture (this is used by other methods, however, so may remain)
• SendEmail

We should aim to only have one method exposed publicly to perform the Accept process, and likewise for the Decline method, which utilises the exact same calls.

    /// <summary>
    ///  Response to Accept 
    /// </summary>
    /// <param name="guid"></param>
    /// <param name="comment"></param>
    /// <returns></returns>
    public ActionResult Accept(string guid,string comment)
    {
        Gateway.Instance.Logger.LogDebug("[Accept] method entered.");
        IEnumerable<TransactionEmailLog> transactionEmailLogs = _publicServiceManager.GetTransactionEmailLogByGUID(guid.Trim());
        ViewBag.GUID = guid.Trim();
        if (transactionEmailLogs != null && transactionEmailLogs.Count() > 0)
        {
            var transactionEmailLog = transactionEmailLogs.FirstOrDefault();
            if (transactionEmailLog.HitCount < 50)
            {
                _publicServiceManager.UpdateTransactonHitCount(transactionEmailLog.GUID);
                var transaction = transactionEmailLog.Transaction;

                if (transaction != null)
                {
                    ViewBag.InvoiceDate = transaction.InvoiceIssueDate != null ? Convert.ToDateTime(transaction.InvoiceIssueDate).ToShortDateString() : "";
                    ViewBag.InvoiceNumber = transaction.InvoiceNumber != null ? Convert.ToString(transaction.InvoiceNumber) : "";
                    ViewBag.DueDate = transaction.PaymentDueDate != null ? Convert.ToDateTime(transaction.PaymentDueDate).ToShortDateString() : "";
                    ViewBag.AmountDue = transaction.PaymentDueDate != null ? transaction.CurrencyCode + " " + Convert.ToDecimal(transaction.InvoiceTotalPayable, CultureInfo.CurrentCulture.NumberFormat).ToString("N") : "";
                }

                ViewBag.IsAcceptOrDecline = "True";
                TransactionApprovalLog approvalLog = new TransactionApprovalLog { IsAccepted = true, CreatedBy = transactionEmailLog.ReferredTo, CreatedDate = DateTime.UtcNow, Transaction = transaction };
                if (!string.IsNullOrWhiteSpace(comment))
                {
                    approvalLog.Comments = comment;
                }
                this._publicServiceManager.SaveApprovalLog(approvalLog);

                if (transaction != null)
                {
                    this.SendEmailNotficationOfInvoiceVerification(transaction, guid, comment, Language.Accepted, transactionEmailLog);
                }
            }
        }
        ViewBag.Message =Language.InvoiceVerificationAcceptMessage;

        Gateway.Instance.Logger.LogDebug("[Accept] method exited.");

        if (Request.IsAjaxRequest())
        {
            return Json(new { result = "success" }, JsonRequestBehavior.AllowGet);
        }
        return View("InvoiceVerification");
    }

As you can see, there are many different exposed methods in the '_publicServiceManager'. What I have done is create a DataContract object that contains information such as GUID, CultureName, etc that will be needed and I aim to pass this into a new method in the PublicManagerService.

public void VerifyInvoice (InvoiceVerificationDataContract invoiceVerificationDetails){}

My question is, how would I best approach the refactoring of this code? Should I have any logic in the WCF layer or should this all remain in the business layer? Many thanks for any and all suggestions in advance.

Answer 1

I think the first step in refactoring this code is to wrap your calls to the Accept and Decline operations in some kind of integration test.

This will allow you to muck about with the composition of the service with the knowledge that you are not regressing or compromising the overall process.

This process looks as if it models some kind of business process, so to munge this into a set of consolidated public operations I guess you'll need to work out the following:

• Does each step need to execute successfully for the overall operation to succeed?
• Does each step have to execute in a specific order?
• How will you handle failure/recovery in one or more of the steps?
• Does it even make sense to combine what were separate functions?
• Do the overall business processes need to be called synchronously?

You may well have already considered all of these points and more besides, so this answer may not provide much value, but it's difficult to answer your question with specifics in it's current form. You could look at the command-processor pattern as a way of decoupling the various steps in your process.

Appreciate this does not answer your question directly.