0
\$\begingroup\$

I have a malware detection system that includes a subsystem responsible for IBAN checks. Initially, this subsystem needs to load and parse data from a specific URL. Subsequently, it checks whether the IBANs are present in the blacklist.

I opted to write separate utility classes for the blacklist and IBAN checks. These utility classes are then utilized within the service layer.

Currently, only two exceptions are thrown from the controller. However, it might be necessary to introduce additional exceptions in the future, and there could be enhancements in managing the flow within the service layer.

I look forward to your evaluation and feedback.

Thank you.

public ResponseEntity<MalwareCheckResource> malwareCheck(@RequestBody MalwareCheckRequest request) throws BlackListException, PdfExtractionException {
try {

    String url = request.getUrl();

    logger.info("Malware check request received for URL: {}", request.getUrl());

    MalwareCheckResource malwareCheckResource = malwareCheckService.checkMalware(url);

    return new ResponseEntity<>(malwareCheckResource, HttpStatus.OK);

} catch (BlackListException e) {
    throw new BlackListException("Blacklist check failed for URL: " + request.getUrl() + ": " + e.getMessage(), e.getBlacklistedIbans());

} catch (PdfExtractionException e) {
    throw new PdfExtractionException("PDF extraction failed for URL: ", request.getUrl() + ": " + e.getMessage());
}

}

package ...;

imports..

@Service
public class MalwareCheckServiceImpl implements MalwareCheckService {

    private final BlackListCheckService blackListCheckService;

    private final PdfProcessorService pdfProcessorService;

    private final MalwareCheckMapper malwareCheckMapper;

    public MalwareCheckServiceImpl(BlackListCheckService blackListCheckService, PdfProcessorService pdfProcessorService, MalwareCheckMapper malwareCheckMapper) {
        this.blackListCheckService = blackListCheckService;
        this.pdfProcessorService = pdfProcessorService;
        this.malwareCheckMapper = malwareCheckMapper;
    }

    @Override
    public MalwareCheckResource checkMalware(String url) throws BlackListException, PdfExtractionException  {

        List<String> ibanList;

        try {
            ibanList = pdfProcessorService.extractIbansFromPdf(url);
        } catch(PdfExtractionException ex)  {
            throw new PdfExtractionException("Exception while parsing url : " + ex.getMessage(), null);
        }

        List<String> blackListedIbans = new ArrayList<>();

        boolean failed = false;
        for(String iban: ibanList) {
           try {
               blackListCheckService.checkBlacklistInfo(iban);
           } catch (Exception e) {
               failed = true;
               blackListedIbans.add(iban);
           }
        }

        if (failed) {
           throw new BlackListException("Malware check failed due to blacklisted IBANs.", blackListedIbans);
        }

        MalwareCheckResource malwareCheckResource = MalwareCheckResource.builder()
                .url(url)
                .validity(true)
                .checkTime(LocalDateTime.now())
                .build();

        return malwareCheckMapper.toResource(malwareCheckResource);

    }

}


package...

impors....

@Service
public class BlackListCheckServiceImpl implements BlackListCheckService {

    private final BlacklistUtil blacklistUtil;

    public BlackListCheckServiceImpl(BlacklistUtil blacklistUtil) {
        this.blacklistUtil = blacklistUtil;
    }

    @Override
    public Boolean checkBlacklistInfo(String iban) throws BlackListException {
        try {
            // Check if IBAN has valid format
            IbanUtil.validate(iban);

            // Check if IBAN is in the blacklist
            if (isBlacklistedIban(iban)) {
                throw new BlackListException("IBAN is listed in Blacklist: ", Collections.singletonList(iban));
            }

            return true;
        } catch (IbanFormatException | InvalidCheckDigitException | UnsupportedCountryException e) {
            throw new BlackListException("Invalid IBAN format/Blacklisted: ", Collections.singletonList(iban));
        }
    }

    private boolean isBlacklistedIban(String iban) {
        return blacklistUtil.contains(iban);
    }
}

package ...;

import ...

@Service
public class PdfProcessorServiceImpl implements PdfProcessorService {

    public List<String> extractIbansFromPdf(String url) throws PdfExtractionException {
        List<String> ibans = extractIbansFromPdfText(url);

        // Validate each IBAN
        for (String iban : ibans) {
            try {
                IbanUtil.validate(iban);
            } catch (Exception e) {
                throw new PdfExtractionException("Error while validating Ibans", e.getMessage());
            }
        }

        return ibans;
    }

    public List<String> extractIbansFromPdfText(String url) throws PdfExtractionException {
        List<String> ibans = new ArrayList<>();

        try (PDDocument document = PDDocument.load(new URL(url).openStream())) {
            PDFTextStripper pdfStripper = new PDFTextStripper();
            String pdfText = pdfStripper.getText(document);

            List<String> extractedIbans = IbanUtil.extractIbansFromString(pdfText);
            ibans.addAll(extractedIbans);
        } catch (RuntimeException | IOException e) {
            throw new PdfExtractionException("Exception while extracting pdf from url", url);
        }

        return ibans;
    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

I don't have the time to do a full review, but few quick remarks on exception handling:

You are discarding the original exceptions, which can contain important information for debugging. The base Exception class constructor has a parameter to add the original exception and basically chain them called cause. Make sure your custom exception classes override that constructor and pass in the original exception:

class PdfExtractionException extends Exception {
   public PdfExtractionException(String message, String url, Throwable cause) {
      super(message, cause);
      // ...
   }
}

// ...
} catch (RuntimeException | IOException e) {
    throw new PdfExtractionException("Exception while extracting pdf from url", url, e);
}

Especially don't catch exceptions just to throw away the original exception and recreate basically the same one, unless you really need to add new information. These catch/throws look are absolutely pointless:

} catch (BlackListException e) {
    throw new BlackListException("Blacklist check failed for URL: " + request.getUrl() + ": " + e.getMessage(), e.getBlacklistedIbans());

} catch (PdfExtractionException e) {
    throw new PdfExtractionException("PDF extraction failed for URL: ", request.getUrl() + ": " + e.getMessage());
}

Finally, never catch Exception. Always only catch the specific exceptions you know to be throw.

\$\endgroup\$
0
\$\begingroup\$

The use of terms such as whitelist and blacklist is discouraged. The use of more inclusive and descriptive terms, such as allowList and denyList is encouraged. In your specific case: allowedIbanList and deniedIbanList.

If you are uncomfortable with the racial connotations, you can explain it to yourself by saying simply that "black" and "white" are ambiguous as the colours themselves do not explain what the lists are used for. It is always better to use terminology that directly communicates intention instead of relying on the reader knowing some ancient social conventions. Your intention is to have a list of IBAN numbers that are not allowed to be used. It's better to put that directly into the variable name.

Using exceptions to control program flow is a well known code smell. Having a boolean return value and a dedicated exception to communicate the same thing is also redundant. And since you are always operating on a set of IBAN numbers, having a method that only processes single IBANs is cumbersome to use (and somewhat inefficient, although that is probably bordering on "an unmeasurable gain"). Thus, you should refactor the method signature to:

/**
 * Return the IBAN numbers from <code>accounts</code> that are
 * listed as denied accounts. If this returns an empty set,
 * then none of accounts were denied.
 */
public Set<IBAN> filterDeniedAccounts(Collection<IBAN> accounts) {
    ...
}

And the code becomes:

final Collection<String> deniedIbans = ibanService.filterDeniedAccounts(ibanList);

if (! deniedIbans.isEmpty()) {
    throw new MalwareException("Malware check failed due to denied IBANs.", deniedIbans);
}

I have used Collections in the parameters here instead of Lists. You should use as generic types as possible in order to not put unnecessary restrictions on the programmer who uses your API. Since the order of the elements in the list of IBANs is not important, there is no need to force the user to use a List. They may be working with a Set if they are operating on a unique set of IBAN numbers, which to me is a not at all far fetched concept.

The term "malware" when checking account numbers against allowed/denied-lists is a bit odd choice. In the banking domain this kind of stuff is usually associated with "fraud prevention" and "sanctions screening". This suggests that the IBAN checking may have been placed into the wrong component and the application architecture might benefit from some refactoring.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.