Credentials: Simplify credential flow #5728

And as a side effect: don't ask for user password when we can't
connect to the server in the first place.

diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp
index 90dfa5e62f728285795321d2ebc23474cce4c54a..e5c871f84534c95d3aea39196bad4a762ee63efd 100644 (file)
--- a/src/gui/accountstate.cpp
+++ b/src/gui/accountstate.cpp
@@ -174,6 +174,14 @@ void AccountState::checkConnectivity()
         return;
     }
 
+    // If we never fetched credentials, do that now - otherwise connection attempts
+    // make little sense, we might be missing client certs.
+    if (!account()->credentials()->wasFetched()) {
+        _waitingForNewCredentials = true;
+        account()->credentials()->fetchFromKeychain();
+        return;
+    }
+
     // IF the account is connected the connection check can be skipped
     // if the last successful etag check job is not so long ago.
     ConfigFile cfg;
@@ -247,10 +255,11 @@ void AccountState::slotConnectionValidatorResult(ConnectionValidator::Status sta
         // much more likely, so keep trying to connect.
         setState(NetworkError);
         break;
-    case ConnectionValidator::CredentialsMissingOrWrong:
+    case ConnectionValidator::CredentialsWrong:
+    case ConnectionValidator::CredentialsNotReady:
         slotInvalidCredentials();
         break;
-    case ConnectionValidator::UserCanceledCredentials:
+    case ConnectionValidator::SslError:
         setState(SignedOut);
         break;
     case ConnectionValidator::ServiceUnavailable:
@@ -270,36 +279,33 @@ void AccountState::slotInvalidCredentials()
     if (isSignedOut() || _waitingForNewCredentials)
         return;
 
+    qCInfo(lcAccountState) << "Invalid credentials for" << _account->url().toString()
+                           << "asking user";
+
     if (account()->credentials()->ready())
         account()->credentials()->invalidateToken();
-    account()->credentials()->fetchFromKeychain();
+    account()->credentials()->askFromUser();
 
     setState(ConfigurationError);
     _waitingForNewCredentials = true;
 }
 
-void AccountState::slotCredentialsFetched(AbstractCredentials *credentials)
+void AccountState::slotCredentialsFetched(AbstractCredentials *)
 {
-    if (!credentials->ready()) {
-        // No exiting credentials found in the keychain
-        credentials->askFromUser();
-        return;
-    }
-
+    // Make a connection attempt, no matter whether the credentials are
+    // ready or not - we want to check whether we can get an SSL connection
+    // going before bothering the user for a password.
+    qCInfo(lcAccountState) << "Fetched credentials for" << _account->url().toString()
+                           << "attempting to connect";
     _waitingForNewCredentials = false;
-
-    if (_connectionValidator) {
-        // When new credentials become available we always want to restart the
-        // connection validation, even if it's currently running.
-        _connectionValidator->deleteLater();
-        _connectionValidator = 0;
-    }
-
     checkConnectivity();
 }
 
 void AccountState::slotCredentialsAsked(AbstractCredentials *credentials)
 {
+    qCInfo(lcAccountState) << "Credentials asked for" << _account->url().toString()
+                           << "are they ready?" << credentials->ready();
+
     _waitingForNewCredentials = false;
 
     if (!credentials->ready()) {

diff --git a/src/gui/creds/shibbolethcredentials.cpp b/src/gui/creds/shibbolethcredentials.cpp
index a827106df699d25e298f2d36d140dd43e610ef2f..cc3d6fb686dc63384807f334eb167d9a20328438 100644 (file)
--- a/src/gui/creds/shibbolethcredentials.cpp
+++ b/src/gui/creds/shibbolethcredentials.cpp
@@ -121,6 +121,8 @@ bool ShibbolethCredentials::ready() const
 
 void ShibbolethCredentials::fetchFromKeychain()
 {
+    _wasFetched = true;
+
     if (_user.isEmpty()) {
         _user = _account->credentialSetting(QLatin1String(userC)).toString();
     }

diff --git a/src/libsync/connectionvalidator.cpp b/src/libsync/connectionvalidator.cpp
index c39f3cd7e2d129ab9ce6ae3522bc5822c633d9b0..5816655be77e756fe8838189c058b6bf705ba00c 100644 (file)
--- a/src/libsync/connectionvalidator.cpp
+++ b/src/libsync/connectionvalidator.cpp
@@ -48,15 +48,17 @@ QString ConnectionValidator::statusString(Status stat)
     case Connected:
         return QLatin1String("Connected");
     case NotConfigured:
-        return QLatin1String("NotConfigured");
+        return QLatin1String("Not configured");
     case ServerVersionMismatch:
         return QLatin1String("Server Version Mismatch");
-    case CredentialsMissingOrWrong:
+    case CredentialsNotReady:
+        return QLatin1String("Credentials not ready");
+    case CredentialsWrong:
         return QLatin1String("Credentials Wrong");
+    case SslError:
+        return QLatin1String("SSL Error");
     case StatusNotFound:
         return QLatin1String("Status not found");
-    case UserCanceledCredentials:
-        return QLatin1String("User canceled credentials");
     case ServiceUnavailable:
         return QLatin1String("Service unavailable");
     case MaintenanceMode:
@@ -143,10 +145,7 @@ void ConnectionValidator::slotStatusFound(const QUrl &url, const QJsonObject &in
     }
 
     // now check the authentication
-    if (_account->credentials()->ready())
-        QTimer::singleShot(0, this, SLOT(checkAuthentication()));
-    else
-        reportResult(CredentialsMissingOrWrong);
+    QTimer::singleShot( 0, this, SLOT( checkAuthentication() ));
 }
 
 // status.php could not be loaded (network or server issue!).
@@ -154,11 +153,13 @@ void ConnectionValidator::slotNoStatusFound(QNetworkReply *reply)
 {
     auto job = qobject_cast<CheckServerJob *>(sender());
     qCWarning(lcConnectionValidator) << reply->error() << job->errorString() << reply->peek(1024);
-    if (!_account->credentials()->ready()) {
-        // This could be needed for SSL client certificates
-        // We need to load them from keychain and try
-        reportResult(CredentialsMissingOrWrong);
-    } else if (!_account->credentials()->stillValid(reply)) {
+    if (reply->error() == QNetworkReply::SslHandshakeFailedError) {
+        reportResult(SslError);
+        return;
+    }
+
+    if (!_account->credentials()->stillValid(reply)) {
+        // Note: Why would this happen on a status.php request?
         _errors.append(tr("Authentication error: Either username or password are wrong."));
     } else {
         //_errors.append(tr("Unable to connect to %1").arg(_account->url().toString()));
@@ -180,8 +181,9 @@ void ConnectionValidator::checkAuthentication()
 {
     AbstractCredentials *creds = _account->credentials();
 
-    if (!creds->ready()) { // The user canceled
-        reportResult(UserCanceledCredentials);
+    if (!creds->ready()) {
+        reportResult(CredentialsNotReady);
+        return;
     }
 
     // simply GET the webdav root, will fail if credentials are wrong.
@@ -200,10 +202,15 @@ void ConnectionValidator::slotAuthFailed(QNetworkReply *reply)
     auto job = qobject_cast<PropfindJob *>(sender());
     Status stat = Timeout;
 
-    if (reply->error() == QNetworkReply::AuthenticationRequiredError || !_account->credentials()->stillValid(reply)) {
+    if (reply->error() == QNetworkReply::SslHandshakeFailedError) {
+        _errors << job->errorStringParsingBody();
+        stat = SslError;
+
+    } else if (reply->error() == QNetworkReply::AuthenticationRequiredError
+        || !_account->credentials()->stillValid(reply)) {
         qCWarning(lcConnectionValidator) << "******** Password is wrong!" << reply->error() << job->errorString();
         _errors << tr("The provided credentials are not correct");
-        stat = CredentialsMissingOrWrong;
+        stat = CredentialsWrong;
 
     } else if (reply->error() != QNetworkReply::NoError) {
         _errors << job->errorStringParsingBody();

diff --git a/src/libsync/connectionvalidator.h b/src/libsync/connectionvalidator.h
index 7254c9bcf60225be233c55823f4babc76266b791..b8bca9af94fe941d19a1286d78126e839cd4c753 100644 (file)
--- a/src/libsync/connectionvalidator.h
+++ b/src/libsync/connectionvalidator.h
@@ -86,9 +86,10 @@ public:
         Connected,
         NotConfigured,
         ServerVersionMismatch, // The server version is too old
-        CredentialsMissingOrWrong, // Credentials aren't ready or AuthenticationRequiredError
+        CredentialsNotReady, // Credentials aren't ready
+        CredentialsWrong, // AuthenticationRequiredError
+        SslError, // SSL handshake error, certificate rejected by user?
         StatusNotFound, // Error retrieving status.php
-        UserCanceledCredentials, // checkAuthentication when credentials aren't ready
         ServiceUnavailable, // 503 on authed request
         MaintenanceMode, // maintenance enabled in status.php
         Timeout // actually also used for other errors on the authed request

diff --git a/src/libsync/creds/abstractcredentials.cpp b/src/libsync/creds/abstractcredentials.cpp
index 9fdef31734e4472fef007b97b7121becdf815098..2aca3a0f8b521fe2bd1a27327ed3cdf9d631f608 100644 (file)
--- a/src/libsync/creds/abstractcredentials.cpp
+++ b/src/libsync/creds/abstractcredentials.cpp
@@ -24,6 +24,7 @@ Q_LOGGING_CATEGORY(lcCredentials, "sync.credentials", QtInfoMsg)
 
 AbstractCredentials::AbstractCredentials()
     : _account(0)
+    , _wasFetched(false)
 {
 }
 

diff --git a/src/libsync/creds/abstractcredentials.h b/src/libsync/creds/abstractcredentials.h
index d73d4534e28ccf33a878802a7a4aee0ef02935d2..09ba50e3e453688485bc3c193b94beab2a9cc474 100644 (file)
--- a/src/libsync/creds/abstractcredentials.h
+++ b/src/libsync/creds/abstractcredentials.h
@@ -44,9 +44,25 @@ public:
     virtual QString authType() const = 0;
     virtual QString user() const = 0;
     virtual QNetworkAccessManager *getQNAM() const = 0;
+
+    /** Whether there are credentials that can be used for a connection attempt. */
     virtual bool ready() const = 0;
+
+    /** Whether fetchFromKeychain() was called before. */
+    bool wasFetched() const { return _wasFetched; }
+
+    /** Trigger (async) fetching of credential information
+     *
+     * Should set _wasFetched = true, and later emit fetched() when done.
+     */
     virtual void fetchFromKeychain() = 0;
+
+    /** Ask credentials from the user (typically async)
+     *
+     * Should emit asked() when done.
+     */
     virtual void askFromUser() = 0;
+
     virtual bool stillValid(QNetworkReply *reply) = 0;
     virtual void persist() = 0;
 
@@ -56,6 +72,8 @@ public:
      *
      * Note that sensitive data (like the password used to acquire the
      * session cookie) may be retained. See forgetSensitiveData().
+     *
+     * ready() must return false afterwards.
      */
     virtual void invalidateToken() = 0;
 
@@ -70,11 +88,23 @@ public:
     static QString keychainKey(const QString &url, const QString &user);
 
 Q_SIGNALS:
+    /** Emitted when fetchFromKeychain() is done.
+     *
+     * Note that ready() can be true or false, depending on whether there was useful
+     * data in the keychain.
+     */
     void fetched();
+
+    /** Emitted when askFromUser() is done.
+     *
+     * Note that ready() can be true or false, depending on whether the user provided
+     * data or not.
+     */
     void asked();
 
 protected:
     Account *_account;
+    bool _wasFetched;
 };
 
 } // namespace OCC

diff --git a/src/libsync/creds/dummycredentials.cpp b/src/libsync/creds/dummycredentials.cpp
index 1e34c72e1c408adaf9913666c54f7f9e3a44c195..c55e979ac12baa2c5a7041f09b03b4aa9a81967c 100644 (file)
--- a/src/libsync/creds/dummycredentials.cpp
+++ b/src/libsync/creds/dummycredentials.cpp
@@ -45,6 +45,7 @@ bool DummyCredentials::stillValid(QNetworkReply *reply)
 
 void DummyCredentials::fetchFromKeychain()
 {
+    _wasFetched = true;
     Q_EMIT(fetched());
 }
 

diff --git a/src/libsync/creds/httpcredentials.cpp b/src/libsync/creds/httpcredentials.cpp
index 5f5e880cc1768f792c8d4d677ee6a7652d8e1337..f5e3cc582068969198e454fd116e853badcacd5c 100644 (file)
--- a/src/libsync/creds/httpcredentials.cpp
+++ b/src/libsync/creds/httpcredentials.cpp
@@ -144,6 +144,8 @@ QString HttpCredentials::fetchUser()
 
 void HttpCredentials::fetchFromKeychain()
 {
+    _wasFetched = true;
+
     // User must be fetched from config file
     fetchUser();
 

diff --git a/src/libsync/creds/tokencredentials.cpp b/src/libsync/creds/tokencredentials.cpp
index 9e5e5b39e9ae2a6a2972dedafa4b81e8c1e2c566..2008542f55d007eae34ebee988c993589d30f306 100644 (file)
--- a/src/libsync/creds/tokencredentials.cpp
+++ b/src/libsync/creds/tokencredentials.cpp
@@ -119,6 +119,7 @@ bool TokenCredentials::ready() const
 
 void TokenCredentials::fetchFromKeychain()
 {
+    _wasFetched = true;
     Q_EMIT fetched();
 }