Merge pull request #5478 from boundlessgeo/auth-viable

[auth][api] Remove deprecated QSslCertificate::isValid()
qgis · Oct 27, 2017 · 071099c · 071099c
2 parents 88a80e6 + 4b9898b
commit 071099c
Show file tree

Hide file tree

Showing 17 changed files with 177 additions and 36 deletions.
diff --git a/python/core/auth/qgsauthcertutils.sip b/python/core/auth/qgsauthcertutils.sip
@@ -294,6 +294,29 @@ Get short strings describing an SSL error
 %End
 
 
+    static bool certIsCurrent( const QSslCertificate &cert );
+%Docstring
+ certIsCurrent checks if ``cert`` is viable for its not before and not after dates
+ \param cert certificate to be checked
+ :rtype: bool
+%End
+
+    static QList<QSslError> certViabilityErrors( const QSslCertificate &cert );
+%Docstring
+ certViabilityErrors checks basic characteristics (validity dates, blacklisting, etc.) of given ``cert``
+ \param cert certificate to be checked
+ :return: list of QSslError (will return NO ERRORS if a null QSslCertificate is passed)
+ :rtype: list of QSslError
+%End
+
+    static bool certIsViable( const QSslCertificate &cert );
+%Docstring
+ certIsViable checks for viability errors of ``cert`` and whether it is NULL
+ \param cert certificate to be checked
+ :return: false if cert is NULL or has viability errors
+ :rtype: bool
+%End
+
     static QList<QSslError> validateCertChain( const QList<QSslCertificate> &certificateChain,
         const QString &hostName = QString(),
         bool trustRootCa = false ) ;

diff --git a/src/auth/identcert/qgsauthidentcertmethod.cpp b/src/auth/identcert/qgsauthidentcertmethod.cpp
@@ -251,9 +251,9 @@ QgsPkiConfigBundle *QgsAuthIdentCertMethod::getPkiConfigBundle( const QString &a
   // init client cert
   // Note: if this is not valid, no sense continuing
   QSslCertificate clientcert( cibundle.first );
-  if ( !clientcert.isValid() )
+  if ( !QgsAuthCertUtils::certIsViable( clientcert ) )
   {
-    QgsDebugMsg( QString( "PKI bundle for authcfg %1: insert FAILED, client cert is not valid" ).arg( authcfg ) );
+    QgsDebugMsg( QString( "PKI bundle for authcfg %1: insert FAILED, client cert is not viable" ).arg( authcfg ) );
     return bundle;
   }
 

diff --git a/src/auth/pkipaths/qgsauthpkipathsedit.cpp b/src/auth/pkipaths/qgsauthpkipathsedit.cpp
@@ -24,6 +24,7 @@
 #include <QSslKey>
 
 #include "qgsapplication.h"
+#include "qgsauthcertutils.h"
 #include "qgsauthmanager.h"
 #include "qgsauthguiutils.h"
 #include "qgslogger.h"
@@ -97,21 +98,21 @@ bool QgsAuthPkiPathsEdit::validateConfig()
     return validityChange( false );
   }
 
-  bool certvalid = cert.isValid();
   QDateTime startdate( cert.effectiveDate() );
   QDateTime enddate( cert.expiryDate() );
 
   writePkiMessage( lePkiPathsMsg,
                    tr( "%1 thru %2" ).arg( startdate.toString(), enddate.toString() ),
-                   ( certvalid ? Valid : Invalid ) );
+                   ( QgsAuthCertUtils::certIsCurrent( cert ) ? Valid : Invalid ) );
 
-  bool showCas( certvalid && populateCas() );
+  bool certviable = QgsAuthCertUtils::certIsViable( cert );
+  bool showCas( certviable && populateCas() );
   lblCas->setVisible( showCas );
   twCas->setVisible( showCas );
   cbAddCas->setVisible( showCas );
   cbAddRootCa->setVisible( showCas );
 
-  return validityChange( certvalid );
+  return validityChange( certviable );
 }
 
 QgsStringMap QgsAuthPkiPathsEdit::configMap() const

diff --git a/src/auth/pkipaths/qgsauthpkipathsmethod.cpp b/src/auth/pkipaths/qgsauthpkipathsmethod.cpp
@@ -284,9 +284,9 @@ QgsPkiConfigBundle *QgsAuthPkiPathsMethod::getPkiConfigBundle( const QString &au
   // init client cert
   // Note: if this is not valid, no sense continuing
   QSslCertificate clientcert( QgsAuthCertUtils::certFromFile( mconfig.config( QStringLiteral( "certpath" ) ) ) );
-  if ( !clientcert.isValid() )
+  if ( !QgsAuthCertUtils::certIsViable( clientcert ) )
   {
-    QgsDebugMsg( QString( "PKI bundle for authcfg %1: insert FAILED, client cert is not valid" ).arg( authcfg ) );
+    QgsDebugMsg( QString( "PKI bundle for authcfg %1: insert FAILED, client cert is not viable" ).arg( authcfg ) );
     return bundle;
   }
 

diff --git a/src/auth/pkipkcs12/qgsauthpkcs12method.cpp b/src/auth/pkipkcs12/qgsauthpkcs12method.cpp
@@ -292,9 +292,9 @@ QgsPkiConfigBundle *QgsAuthPkcs12Method::getPkiConfigBundle( const QString &auth
   // init client cert
   // Note: if this is not valid, no sense continuing
   QSslCertificate clientcert( bundlelist.at( 0 ).toLatin1() );
-  if ( !clientcert.isValid() )
+  if ( !QgsAuthCertUtils::certIsViable( clientcert ) )
   {
-    QgsDebugMsg( QString( "PKI bundle for authcfg %1: insert FAILED, client cert is not valid" ).arg( authcfg ) );
+    QgsDebugMsg( QString( "PKI bundle for authcfg %1: insert FAILED, client cert is not viable" ).arg( authcfg ) );
     return bundle;
   }
 

diff --git a/src/core/auth/qgsauthcertutils.cpp b/src/core/auth/qgsauthcertutils.cpp
@@ -1241,6 +1241,43 @@ QList<QPair<QSslError::SslError, QString> > QgsAuthCertUtils::sslErrorEnumString
   return errenums;
 }
 
+bool QgsAuthCertUtils::certIsCurrent( const QSslCertificate &cert )
+{
+  if ( cert.isNull() )
+    return false;
+  const QDateTime currentTime = QDateTime::currentDateTime();
+  return cert.effectiveDate() <= currentTime && cert.expiryDate() >= currentTime;
+}
+
+QList<QSslError> QgsAuthCertUtils::certViabilityErrors( const QSslCertificate &cert )
+{
+  QList<QSslError> sslErrors;
+
+  if ( cert.isNull() )
+    return sslErrors;
+
+  const QDateTime currentTime = QDateTime::currentDateTime();
+  if ( cert.expiryDate() <= currentTime )
+  {
+    sslErrors << QSslError( QSslError::SslError::CertificateExpired, cert );
+  }
+  if ( cert.effectiveDate() >= QDateTime::currentDateTime() )
+  {
+    sslErrors << QSslError( QSslError::SslError::CertificateNotYetValid, cert );
+  }
+  if ( cert.isBlacklisted() )
+  {
+    sslErrors << QSslError( QSslError::SslError::CertificateBlacklisted, cert );
+  }
+
+  return sslErrors;
+}
+
+bool QgsAuthCertUtils::certIsViable( const QSslCertificate &cert )
+{
+  return !cert.isNull() && QgsAuthCertUtils::certViabilityErrors( cert ).isEmpty();
+}
+
 QList<QSslError> QgsAuthCertUtils::validateCertChain( const QList<QSslCertificate> &certificateChain,
     const QString &hostName,
     bool trustRootCa )
@@ -1269,20 +1306,7 @@ QList<QSslError> QgsAuthCertUtils::validateCertChain( const QList<QSslCertificat
   const QList<QSslCertificate> constTrustedChain( trustedChain );
   for ( const auto &cert : constTrustedChain )
   {
-    // TODO: move all the checks to QgsAuthCertUtils::certIsViable( )
-    const QDateTime currentTime = QDateTime::currentDateTime();
-    if ( cert.expiryDate() <= currentTime )
-    {
-      sslErrors << QSslError( QSslError::SslError::CertificateExpired, cert );
-    }
-    if ( cert.effectiveDate() >= QDateTime::currentDateTime() )
-    {
-      sslErrors << QSslError( QSslError::SslError::CertificateNotYetValid, cert );
-    }
-    if ( cert.isBlacklisted() )
-    {
-      sslErrors << QSslError( QSslError::SslError::CertificateBlacklisted, cert );
-    }
+    sslErrors << QgsAuthCertUtils::certViabilityErrors( cert );
   }
 
   // Merge in the root CA if present and asked for
@@ -1307,6 +1331,16 @@ QList<QSslError> QgsAuthCertUtils::validateCertChain( const QList<QSslCertificat
 QStringList QgsAuthCertUtils::validatePKIBundle( QgsPkiBundle &bundle, bool useIntermediates, bool trustRootCa )
 {
   QStringList errors;
+  if ( bundle.clientCert().isNull() )
+    errors << QObject::tr( "Client certificate is NULL." );
+
+  if ( bundle.clientKey().isNull() )
+    errors << QObject::tr( "Client certificate key is NULL." );
+
+  // immediately bail out if cert or key is NULL
+  if ( !errors.isEmpty() )
+    return errors;
+
   QList<QSslError> sslErrors;
   if ( useIntermediates )
   {

diff --git a/src/core/auth/qgsauthcertutils.h b/src/core/auth/qgsauthcertutils.h
@@ -330,6 +330,26 @@ class CORE_EXPORT QgsAuthCertUtils
      */
     static QList<QPair<QSslError::SslError, QString> > sslErrorEnumStrings() SIP_SKIP;
 
+    /**
+     * \brief certIsCurrent checks if \a cert is viable for its not before and not after dates
+     * \param cert certificate to be checked
+     */
+    static bool certIsCurrent( const QSslCertificate &cert );
+
+    /**
+     * \brief certViabilityErrors checks basic characteristics (validity dates, blacklisting, etc.) of given \a cert
+     * \param cert certificate to be checked
+     * \return list of QSslError (will return NO ERRORS if a null QSslCertificate is passed)
+     */
+    static QList<QSslError> certViabilityErrors( const QSslCertificate &cert );
+
+    /**
+     * \brief certIsViable checks for viability errors of \a cert and whether it is NULL
+     * \param cert certificate to be checked
+     * \return false if cert is NULL or has viability errors
+     */
+    static bool certIsViable( const QSslCertificate &cert );
+
     /**
      * \brief validateCertChain validates the given \a certificateChain
      * \param certificateChain list of certificates to be checked, with leaf first and with optional root CA last

diff --git a/src/core/auth/qgsauthconfig.cpp b/src/core/auth/qgsauthconfig.cpp
@@ -275,7 +275,7 @@ bool QgsPkiBundle::isNull() const
 
 bool QgsPkiBundle::isValid() const
 {
-  return ( !isNull() && mCert.isValid() );
+  return ( !isNull() && QgsAuthCertUtils::certIsViable( mCert ) );
 }
 
 const QString QgsPkiBundle::certId() const

diff --git a/src/core/auth/qgsauthmanager.cpp b/src/core/auth/qgsauthmanager.cpp
@@ -1784,7 +1784,7 @@ const QPair<QSslCertificate, QSslKey> QgsAuthManager::certIdentityBundle( const
 const QStringList QgsAuthManager::certIdentityBundleToPem( const QString &id )
 {
   QPair<QSslCertificate, QSslKey> bundle( certIdentityBundle( id ) );
-  if ( bundle.first.isValid() && !bundle.second.isNull() )
+  if ( QgsAuthCertUtils::certIsViable( bundle.first ) && !bundle.second.isNull() )
   {
     return QStringList() << QString( bundle.first.toPem() ) << QString( bundle.second.toPem() );
   }
@@ -2719,7 +2719,7 @@ const QList<QSslCertificate> QgsAuthManager::trustedCaCerts( bool includeinvalid
     }
     else if ( defaultpolicy == QgsAuthCertUtils::Trusted && !untrustedids.contains( certid ) )
     {
-      if ( !includeinvalid && !cert.isValid() )
+      if ( !includeinvalid && !QgsAuthCertUtils::certIsViable( cert ) )
         continue;
       trustedcerts.append( cert );
     }

diff --git a/src/gui/auth/qgsauthcertificateinfo.cpp b/src/gui/auth/qgsauthcertificateinfo.cpp
@@ -295,7 +295,7 @@ void QgsAuthCertInfo::updateCurrentCertInfo( int chainindx )
     mCurrentTrustPolicy = trustpolicy;
 
     cmbbxTrust->setTrustPolicy( trustpolicy );
-    if ( !mCurrentQCert.isValid() )
+    if ( !QgsAuthCertUtils::certIsViable( mCurrentQCert ) )
     {
       cmbbxTrust->setDefaultTrustPolicy( QgsAuthCertUtils::Untrusted );
     }
@@ -880,7 +880,7 @@ void QgsAuthCertInfo::decorateCertTreeItem( const QSslCertificate &cert,
     return;
   }
 
-  if ( !cert.isValid() )
+  if ( !QgsAuthCertUtils::certIsViable( cert ) )
   {
     item->setIcon( 0, QgsApplication::getThemeIcon( QStringLiteral( "/mIconCertificateUntrusted.svg" ) ) );
     return;

diff --git a/src/gui/auth/qgsauthidentitieseditor.cpp b/src/gui/auth/qgsauthidentitieseditor.cpp
@@ -205,7 +205,7 @@ void QgsAuthIdentitiesEditor::appendIdentitiesToItem( const QList<QSslCertificat
     QTreeWidgetItem *item( new QTreeWidgetItem( parent, coltxts, ( int )identype ) );
 
     item->setIcon( 0, QgsApplication::getThemeIcon( QStringLiteral( "/mIconCertificate.svg" ) ) );
-    if ( !cert.isValid() )
+    if ( !QgsAuthCertUtils::certIsViable( cert ) )
     {
       item->setForeground( 2, redb );
       item->setIcon( 0, QgsApplication::getThemeIcon( QStringLiteral( "/mIconCertificateUntrusted.svg" ) ) );

diff --git a/src/gui/auth/qgsauthimportcertdialog.cpp b/src/gui/auth/qgsauthimportcertdialog.cpp
@@ -170,7 +170,7 @@ void QgsAuthImportCertDialog::validateCertificates()
 
   Q_FOREACH ( const QSslCertificate &cert, certs )
   {
-    if ( cert.isValid() )
+    if ( QgsAuthCertUtils::certIsViable( cert ) )
       ++validcerts;
 
     if ( filterCAs )

diff --git a/src/gui/auth/qgsauthimportidentitydialog.cpp b/src/gui/auth/qgsauthimportidentitydialog.cpp
@@ -277,12 +277,13 @@ bool QgsAuthImportIdentityDialog::validatePkiPaths()
     ca_certs = certs;
   }
 
-  isvalid = clientcert.isValid();
+  isvalid = QgsAuthCertUtils::certIsViable( clientcert );
+
   QDateTime startdate( clientcert.effectiveDate() );
   QDateTime enddate( clientcert.expiryDate() );
 
   writeValidation( tr( "%1 thru %2" ).arg( startdate.toString(), enddate.toString() ),
-                   ( isvalid ? Valid : Invalid ) );
+                   ( QgsAuthCertUtils::certIsCurrent( clientcert ) ? Valid : Invalid ) );
   //TODO: set enabled on cert info button, relative to cert validity
 
   // check for valid private key and that any supplied password works

diff --git a/src/gui/auth/qgsauthserverseditor.cpp b/src/gui/auth/qgsauthserverseditor.cpp
@@ -24,6 +24,7 @@
 #include "qgssettings.h"
 #include "qgsapplication.h"
 #include "qgsauthcertificateinfo.h"
+#include "qgsauthcertutils.h"
 #include "qgsauthmanager.h"
 #include "qgsauthguiutils.h"
 #include "qgslogger.h"
@@ -206,7 +207,7 @@ void QgsAuthServersEditor::appendSslConfigsToItem( const QList<QgsAuthConfigSslS
     QTreeWidgetItem *item( new QTreeWidgetItem( parent, coltxts, ( int )conftype ) );
 
     item->setIcon( 0, QgsApplication::getThemeIcon( QStringLiteral( "/mIconCertificate.svg" ) ) );
-    if ( !cert.isValid() )
+    if ( !QgsAuthCertUtils::certIsViable( cert ) )
     {
       item->setForeground( 2, redb );
       item->setIcon( 0, QgsApplication::getThemeIcon( QStringLiteral( "/mIconCertificateUntrusted.svg" ) ) );

diff --git a/src/gui/auth/qgsauthtrustedcasdialog.cpp b/src/gui/auth/qgsauthtrustedcasdialog.cpp
@@ -22,6 +22,7 @@
 #include "qgssettings.h"
 #include "qgsapplication.h"
 #include "qgsauthcertificateinfo.h"
+#include "qgsauthcertutils.h"
 #include "qgsauthguiutils.h"
 #include "qgsauthmanager.h"
 #include "qgslogger.h"
@@ -196,7 +197,7 @@ void QgsAuthTrustedCAsDialog::appendCertsToItem( const QList<QSslCertificate> &c
     QTreeWidgetItem *item( new QTreeWidgetItem( parent, coltxts, ( int )catype ) );
 
     item->setIcon( 0, QgsApplication::getThemeIcon( QStringLiteral( "/mIconCertificate.svg" ) ) );
-    if ( !cert.isValid() )
+    if ( !QgsAuthCertUtils::certIsViable( cert ) )
     {
       item->setForeground( 2, redb );
       item->setIcon( 0, QgsApplication::getThemeIcon( QStringLiteral( "/mIconCertificateUntrusted.svg" ) ) );

diff --git a/tests/src/core/testqgsauthcertutils.cpp b/tests/src/core/testqgsauthcertutils.cpp
@@ -39,6 +39,7 @@ class TestQgsAuthCertUtils: public QObject
     void init() {}
     void cleanup() {}
 
+    void testValidationUtils();
     void testPkcsUtils();
 
   private:
@@ -60,6 +61,36 @@ void TestQgsAuthCertUtils::cleanupTestCase()
   QgsApplication::exitQgis();
 }
 
+void TestQgsAuthCertUtils::testValidationUtils()
+{
+  // null cert
+  QSslCertificate cert;
+  QVERIFY( !QgsAuthCertUtils::certIsCurrent( cert ) );
+  QList<QSslError> res = QgsAuthCertUtils::certViabilityErrors( cert );
+  QVERIFY( res.count() == 0 );
+  QVERIFY( !QgsAuthCertUtils::certIsViable( cert ) );
+
+  cert.clear();
+  res.clear();
+  // valid cert
+  cert = QgsAuthCertUtils::certFromFile( sPkiData + "/gerardus_cert.pem" );
+  QVERIFY( QgsAuthCertUtils::certIsCurrent( cert ) );
+  res = QgsAuthCertUtils::certViabilityErrors( cert );
+  QVERIFY( res.count() == 0 );
+  QVERIFY( QgsAuthCertUtils::certIsViable( cert ) );
+
+
+  cert.clear();
+  res.clear();
+  // expired cert
+  cert = QgsAuthCertUtils::certFromFile( sPkiData + "/marinus_cert-EXPIRED.pem" );
+  QVERIFY( !QgsAuthCertUtils::certIsCurrent( cert ) );
+  res = QgsAuthCertUtils::certViabilityErrors( cert );
+  QVERIFY( res.count() > 0 );
+  QVERIFY( res.contains( QSslError( QSslError::SslError::CertificateExpired, cert ) ) );
+  QVERIFY( !QgsAuthCertUtils::certIsViable( cert ) );
+}
+
 void TestQgsAuthCertUtils::testPkcsUtils()
 {
   QByteArray pkcs;