From: Christian Kamm Date: Thu, 27 Apr 2017 11:58:26 +0000 (+0200) Subject: Credentials: Simplify credential flow #5728 X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~719 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=d3b00532b1dd9f44cc606e6738b53345c37582cf;p=nextcloud-desktop.git 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 90dfa5e62..e5c871f84 100644 --- 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 a827106df..cc3d6fb68 100644 --- 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 c39f3cd7e..5816655be 100644 --- 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(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(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 7254c9bcf..b8bca9af9 100644 --- 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 9fdef3173..2aca3a0f8 100644 --- 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 d73d4534e..09ba50e3e 100644 --- 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 1e34c72e1..c55e979ac 100644 --- 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 5f5e880cc..f5e3cc582 100644 --- 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 9e5e5b39e..2008542f5 100644 --- 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(); }