Resolved/removed fixme-s in code

Signed-off-by: Lauris Kaplinski <[email protected]>

src/controller/command-handlers/authenticate.cpp

@@ -30,6 +30,7 @@
 #include <QJsonDocument>
 #include <QCryptographicHash>
 #include <QDir>
+#include <QScopeGuard>
 
 #include <map>
 
@@ -122,17 +123,17 @@ QVariantMap Authenticate::onConfirm(WebEidUI* window,
     const auto signatureAlgorithm =
         QString::fromStdString(cardCertAndPin.cardInfo->eid().authSignatureAlgorithm());
 
-    auto pin = getPin(cardCertAndPin.cardInfo->eid(), window);
+    pcsc_cpp::byte_vector pin;
+    getPin(pin, cardCertAndPin.cardInfo->eid().smartcard(), window);
+    auto pin_cleanup = qScopeGuard([&pin] {
+        // Erase PIN memory.
+        std::fill(pin.begin(), pin.end(), '\0');
+    });
 
     try {
         const auto signature =
             createSignature(origin.url(), challengeNonce, cardCertAndPin.cardInfo->eid(), pin);
 
-        // Erase the PIN memory.
-        // TODO: Use a scope guard. Verify that the buffers are actually zeroed and no copies
-        // remain.
-        std::fill(pin.begin(), pin.end(), '\0');
-
         return createAuthenticationToken(signatureAlgorithm, cardCertAndPin.certificateBytesInDer,
                                          signature);
 

src/controller/command-handlers/sign.cpp

@@ -25,6 +25,8 @@
 #include "signauthutils.hpp"
 #include "utils/utils.hpp"
 
+#include <QScopeGuard>
+
 using namespace electronic_id;
 
 namespace
@@ -95,16 +97,16 @@ void Sign::emitCertificatesReady(const std::vector<CardCertificateAndPinInfo>& c
 
 QVariantMap Sign::onConfirm(WebEidUI* window, const CardCertificateAndPinInfo& cardCertAndPin)
 {
-    auto pin = getPin(cardCertAndPin.cardInfo->eid(), window);
+    pcsc_cpp::byte_vector pin;
+    getPin(pin, cardCertAndPin.cardInfo->eid().smartcard(), window);
+    auto pin_cleanup = qScopeGuard([&pin] {
+        // Erase PIN memory.
+        std::fill(pin.begin(), pin.end(), '\0');
+    });
 
     try {
         const auto signature = signHash(cardCertAndPin.cardInfo->eid(), pin, docHash, hashAlgo);
 
-        // Erase PIN memory.
-        // TODO: Use a scope guard. Verify that the buffers are actually zeroed
-        // and no copies remain.
-        std::fill(pin.begin(), pin.end(), '\0');
-
         return {{QStringLiteral("signature"), signature.first},
                 {QStringLiteral("signatureAlgorithm"), signature.second}};
 

src/controller/command-handlers/signauthutils.cpp

@@ -24,7 +24,6 @@
 
 #include "ui.hpp"
 #include "commandhandler.hpp"
-#include "utils/utils.hpp"
 
 using namespace electronic_id;
 
@@ -78,29 +77,28 @@ inline void eraseData(T& data)
     }
 }
 
-pcsc_cpp::byte_vector getPin(const ElectronicID& card, WebEidUI* window)
+int getPin(pcsc_cpp::byte_vector& pin, const pcsc_cpp::SmartCard& card, WebEidUI* window)
 {
     // Doesn't apply to PIN pads.
-    if (card.smartcard().readerHasPinPad() || card.providesExternalPinDialog()) {
-        return {};
+    if (card.readerHasPinPad()) {
+        return 0;
     }
 
     REQUIRE_NON_NULL(window)
 
-    auto pin = window->getPin();
-    if (pin.isEmpty()) {
+    QString pinqs = window->getPin();
+    if (pinqs.isEmpty()) {
         THROW(ProgrammingError, "Empty PIN");
     }
+    int len = (int)pinqs.length();
 
-    // TODO: Avoid making copies of the PIN in memory.
-    auto pinQByteArray = pin.toUtf8();
-    pcsc_cpp::byte_vector pinBytes {pinQByteArray.begin(), pinQByteArray.end()};
-
-    // TODO: Verify that the buffers are actually zeroed and no copies remain.
-    eraseData<QString, QChar>(pin);
-    eraseData<QByteArray, char>(pinQByteArray);
+    pin.resize(len);
+    for (int i = 0; i < len; i++) {
+        pin[i] = pinqs[i].cell();
+    }
+    eraseData<QString, QChar>(pinqs);
 
-    return pinBytes;
+    return len;
 }
 
 QVariantMap signatureAlgoToVariantMap(const SignatureAlgorithm signatureAlgo)

src/controller/command-handlers/signauthutils.hpp

@@ -45,6 +45,6 @@ extern template QString validateAndGetArgument<QString>(const QString& argName,
 extern template QByteArray
 validateAndGetArgument<QByteArray>(const QString& argName, const QVariantMap& args, bool allowNull);
 
-pcsc_cpp::byte_vector getPin(const electronic_id::ElectronicID& card, WebEidUI* window);
+int getPin(pcsc_cpp::byte_vector& pin, const pcsc_cpp::SmartCard& card, WebEidUI* window);
 
 QVariantMap signatureAlgoToVariantMap(const electronic_id::SignatureAlgorithm signatureAlgo);

src/controller/controller.cpp

@@ -101,6 +101,15 @@ void Controller::run()
 
         startCommandExecution();
 
+    } catch (const std::invalid_argument& exc) {
+        if (isInStdinMode) {
+            // Pass invalid argument message to the caller just in case it may be interested
+            // The result will be {"invalid-argument" : message}
+            // Command parameter is only used if exception will be raised during json creation
+            writeResponseToStdOut(isInStdinMode, {{QStringLiteral("invalid-argument"), exc.what()}},
+                                  "invalid-argument");
+        }
+        onCriticalFailure(exc.what());
     } catch (const std::exception& error) {
         onCriticalFailure(error.what());
     }

src/controller/inputoutputmode.cpp

@@ -71,7 +71,6 @@ CommandWithArgumentsPtr readCommandFromStdin()
     const auto messageLength = readMessageLength(std::cin);
 
     if (messageLength < 5) {
-        // FIXME: Pass errors back up to caller in stdin mode.
         throw std::invalid_argument("readCommandFromStdin: Message length is "
                                     + std::to_string(messageLength) + ", at least 5 required");
     }
@@ -88,7 +87,6 @@ CommandWithArgumentsPtr readCommandFromStdin()
     const auto json = QJsonDocument::fromJson(message);
 
     if (!json.isObject()) {
-        // FIXME: Pass errors back up to caller in stdin mode.
         throw std::invalid_argument("readCommandFromStdin: Invalid JSON, not an object");
     }
 
@@ -97,7 +95,6 @@ CommandWithArgumentsPtr readCommandFromStdin()
     const auto arguments = jsonObject["arguments"];
 
     if (!command.isString() || !arguments.isObject()) {
-        // FIXME: Pass errors back up to caller in stdin mode.
         throw std::invalid_argument("readCommandFromStdin: Invalid JSON, the main object does not "
                                     "contain a 'command' string and 'arguments' object");
     }

src/ui/webeiddialog.cpp

@@ -284,7 +284,10 @@ QString WebEidDialog::getPin()
 {
     // getPin() is called from background threads and must be thread-safe.
     // QString uses QAtomicPointer internally and is thread-safe.
-    return pin;
+    // There should be only single reference and this is transferred to the caller for safety
+    QString ret = pin;
+    pin.clear();
+    return ret;
 }
 
 void WebEidDialog::onSmartCardStatusUpdate(const RetriableError status)
@@ -332,7 +335,8 @@ void WebEidDialog::onMultipleCertificatesReady(
         ui->selectAnotherCertificate->setVisible(certificateAndPinInfos.size() > 1);
         connect(ui->selectAnotherCertificate, &QPushButton::clicked, this,
                 [this, origin, certificateAndPinInfos] {
-                    ui->pinInput->clear();
+                    // We set pinInput to empty text instead of clear() to also reset undo buffer
+                    ui->pinInput->setText({});
                     onMultipleCertificatesReady(origin, certificateAndPinInfos);
                 });
         setupOK([this, origin] {
@@ -443,7 +447,6 @@ void WebEidDialog::onVerifyPinFailed(const VerifyPinFailed::Status status, const
 
     std::function<QString()> message;
 
-    // FIXME: don't allow retry in case of UNKNOWN_ERROR
     switch (status) {
     case Status::RETRY_ALLOWED:
         message = [retriesLeft]() -> QString {
@@ -465,8 +468,11 @@ void WebEidDialog::onVerifyPinFailed(const VerifyPinFailed::Status status, const
         message = [] { return tr("PIN entry cancelled."); };
         break;
     case Status::PIN_ENTRY_DISABLED:
+        message = [] { return tr("PIN entry disabled"); };
+        break;
     case Status::UNKNOWN_ERROR:
         message = [] { return tr("Technical error"); };
+        displayFatalError(message);
         break;
     }
 
@@ -678,6 +684,20 @@ void WebEidDialog::displayPinBlockedError()
     ui->helpButton->show();
 }
 
+void WebEidDialog::displayFatalError(std::function<QString()> message)
+{
+    ui->pinTitleLabel->hide();
+    ui->pinInput->hide();
+    ui->pinTimeoutTimer->stop();
+    ui->pinTimeRemaining->hide();
+    ui->pinEntryTimeoutProgressBar->hide();
+    setTrText(ui->pinErrorLabel, message);
+    ui->pinErrorLabel->show();
+    ui->okButton->hide();
+    ui->cancelButton->setEnabled(true);
+    ui->cancelButton->show();
+}
+
 void WebEidDialog::showPinInputWarning(bool show)
 {
     style()->unpolish(ui->pinInput);

src/ui/webeiddialog.hpp

@@ -104,6 +104,7 @@ class WebEidDialog final : public WebEidUI
     template <typename Func>
     void setupOK(Func func, const char* text = {}, bool enabled = false);
     void displayPinBlockedError();
+    void displayFatalError(std::function<QString()> message);
 
     void showPinInputWarning(bool show);