From 6d027ebd40c85f5732eba74583a9eace08842524 Mon Sep 17 00:00:00 2001 From: Jocelyn Turcotte Date: Sat, 5 Sep 2015 15:39:22 +0200 Subject: [PATCH] Separate the credential dialog from their fetch #3350 This moves the responsibility of asking the user or not for credentials from the Credentials classes back to the AccountState. fetch() now only extract credentials from the keychain, reports the result to the AccountState which then decides if askFromUser() should be called or not. The result is once more reported to the AccounState. This also replaces the HttpCredentials::queryPassword virtual which now lets HttpCredentialsGui and HttpCredentialsText do it the way that they prefer. --- src/cmd/cmd.cpp | 10 ++--- src/gui/accountstate.cpp | 43 +++++++++++++++++----- src/gui/accountstate.h | 7 +++- src/gui/application.cpp | 2 +- src/gui/creds/httpcredentialsgui.cpp | 26 +++++++++---- src/gui/creds/httpcredentialsgui.h | 3 +- src/gui/creds/shibbolethcredentials.cpp | 17 ++++----- src/gui/creds/shibbolethcredentials.h | 4 +- src/libsync/account.cpp | 7 ++++ src/libsync/account.h | 2 + src/libsync/connectionvalidator.cpp | 3 +- src/libsync/connectionvalidator.h | 6 +-- src/libsync/creds/abstractcredentials.h | 5 ++- src/libsync/creds/dummycredentials.cpp | 7 +++- src/libsync/creds/dummycredentials.h | 3 +- src/libsync/creds/httpcredentials.cpp | 49 ++++++------------------- src/libsync/creds/httpcredentials.h | 7 ++-- 17 files changed, 111 insertions(+), 90 deletions(-) diff --git a/src/cmd/cmd.cpp b/src/cmd/cmd.cpp index 73b93c807..06d29d853 100644 --- a/src/cmd/cmd.cpp +++ b/src/cmd/cmd.cpp @@ -114,11 +114,11 @@ public: _sslTrusted(false) {} - QString queryPassword(bool *ok, const QString&) Q_DECL_OVERRIDE { - if (ok) { - *ok = true; - } - return ::queryPassword(user()); + void askFromUser() Q_DECL_OVERRIDE { + _password = ::queryPassword(user()); + _ready = true; + persist(); + emit asked(); } void setSSLTrusted( bool isTrusted ) { diff --git a/src/gui/accountstate.cpp b/src/gui/accountstate.cpp index e389c276a..7d5460014 100644 --- a/src/gui/accountstate.cpp +++ b/src/gui/accountstate.cpp @@ -15,6 +15,7 @@ #include "accountmanager.h" #include "account.h" #include "creds/abstractcredentials.h" +#include "logger.h" #include #include @@ -28,6 +29,7 @@ AccountState::AccountState(AccountPtr account) , _state(AccountState::Disconnected) , _connectionStatus(ConnectionValidator::Undefined) , _waitingForNewCredentials(false) + , _credentialsFetchMode(Interactive) { qRegisterMetaType("AccountState*"); @@ -35,6 +37,8 @@ AccountState::AccountState(AccountPtr account) SLOT(slotInvalidCredentials())); connect(account.data(), SIGNAL(credentialsFetched(AbstractCredentials*)), SLOT(slotCredentialsFetched(AbstractCredentials*))); + connect(account.data(), SIGNAL(credentialsAsked(AbstractCredentials*)), + SLOT(slotCredentialsAsked(AbstractCredentials*))); } AccountState::~AccountState() @@ -79,7 +83,7 @@ void AccountState::setState(State state) _connectionStatus = ConnectionValidator::Undefined; _connectionErrors.clear(); } else if (oldState == SignedOut && _state == Disconnected) { - checkConnectivity(AbstractCredentials::Interactive); + checkConnectivity(Interactive); } } @@ -131,7 +135,7 @@ bool AccountState::isConnectedOrTemporarilyUnavailable() const return isConnected() || _state == ServiceUnavailable; } -void AccountState::checkConnectivity(AbstractCredentials::FetchMode credentialsFetchMode) +void AccountState::checkConnectivity(CredentialFetchMode credentialsFetchMode) { if (isSignedOut() || _waitingForNewCredentials) { return; @@ -141,7 +145,8 @@ void AccountState::checkConnectivity(AbstractCredentials::FetchMode credentialsF qDebug() << "ConnectionValidator already running, ignoring"; return; } - ConnectionValidator * conValidator = new ConnectionValidator(account(), credentialsFetchMode); + _credentialsFetchMode = credentialsFetchMode; + ConnectionValidator * conValidator = new ConnectionValidator(account()); _connectionValidator = conValidator; connect(conValidator, SIGNAL(connectionResult(ConnectionValidator::Status,QStringList)), SLOT(slotConnectionValidatorResult(ConnectionValidator::Status,QStringList))); @@ -231,6 +236,29 @@ void AccountState::slotInvalidCredentials() } void AccountState::slotCredentialsFetched(AbstractCredentials* credentials) +{ + if (!credentials->ready()) { + // No exiting credentials found in the keychain + if (_credentialsFetchMode == Interactive) + credentials->askFromUser(); + else { + Logger::instance()->postOptionalGuiLog(tr("Reauthentication required"), tr("You need to re-login to continue using the account %1.").arg(_account->displayName())); + setState(SignedOut); + _waitingForNewCredentials = false; + } + return; + } + + _waitingForNewCredentials = false; + + // When new credentials become available we always want to restart the + // connection validation, even if it's currently running. + delete _connectionValidator; + + checkConnectivity(_credentialsFetchMode); +} + +void AccountState::slotCredentialsAsked(AbstractCredentials* credentials) { _waitingForNewCredentials = false; @@ -242,14 +270,9 @@ void AccountState::slotCredentialsFetched(AbstractCredentials* credentials) // When new credentials become available we always want to restart the // connection validation, even if it's currently running. - if (_connectionValidator) { - delete _connectionValidator; - } + delete _connectionValidator; - // If we made it this far, it means that we either fetched credentials - // interactively, or that we already aborted for missing/invalid credentials. - Q_ASSERT(credentials->ready()); - checkConnectivity(AbstractCredentials::Interactive); + checkConnectivity(_credentialsFetchMode); } std::unique_ptr AccountState::settings() diff --git a/src/gui/accountstate.h b/src/gui/accountstate.h index 7280334d1..be1423950 100644 --- a/src/gui/accountstate.h +++ b/src/gui/accountstate.h @@ -19,6 +19,7 @@ #include #include "utility.h" #include "connectionvalidator.h" +#include "creds/abstractcredentials.h" #include class QSettings; @@ -27,7 +28,6 @@ namespace OCC { class AccountState; class Account; -class AbstractCredentials; /** * @brief Extra info about an ownCloud server account. @@ -59,6 +59,7 @@ public: /// An error like invalid credentials where retrying won't help. ConfigurationError }; + enum CredentialFetchMode { Interactive, NonInteractive }; /// The actual current connectivity status. typedef ConnectionValidator::Status ConnectionStatus; @@ -84,7 +85,7 @@ public: /// Triggers a ping to the server to update state and /// connection status and errors. - void checkConnectivity(AbstractCredentials::FetchMode credentialsFetchMode); + void checkConnectivity(CredentialFetchMode credentialsFetchMode); /** Returns a new settings object for this account, already in the right groups. */ std::unique_ptr settings(); @@ -104,6 +105,7 @@ protected Q_SLOTS: void slotConnectionValidatorResult(ConnectionValidator::Status status, const QStringList& errors); void slotInvalidCredentials(); void slotCredentialsFetched(AbstractCredentials* creds); + void slotCredentialsAsked(AbstractCredentials* creds); private: AccountPtr _account; @@ -111,6 +113,7 @@ private: ConnectionStatus _connectionStatus; QStringList _connectionErrors; bool _waitingForNewCredentials; + CredentialFetchMode _credentialsFetchMode; QPointer _connectionValidator; }; diff --git a/src/gui/application.cpp b/src/gui/application.cpp index 0b2460b1b..799f7ced0 100644 --- a/src/gui/application.cpp +++ b/src/gui/application.cpp @@ -223,7 +223,7 @@ void Application::slotCheckConnection() // when the error is permanent. if (state != AccountState::SignedOut && state != AccountState::ConfigurationError) { - accountState->checkConnectivity(AbstractCredentials::NonInteractive); + accountState->checkConnectivity(AccountState::NonInteractive); } } diff --git a/src/gui/creds/httpcredentialsgui.cpp b/src/gui/creds/httpcredentialsgui.cpp index 2357aade6..efbe52e4f 100644 --- a/src/gui/creds/httpcredentialsgui.cpp +++ b/src/gui/creds/httpcredentialsgui.cpp @@ -23,24 +23,34 @@ using namespace QKeychain; namespace OCC { - QString HttpCredentialsGui::queryPassword(bool *ok, const QString& hint) +void HttpCredentialsGui::askFromUser() { - if (!ok) { - return QString(); - } + // The rest of the code assumes that this will be done asynchronously + QMetaObject::invokeMethod(this, "askFromUserAsync", Qt::QueuedConnection); +} +void HttpCredentialsGui::askFromUserAsync() +{ QString msg = tr("Please enter %1 password:\n" "\n" "User: %2\n" "Account: %3\n") .arg(Theme::instance()->appNameGUI(), _user, _account->displayName()); - if (!hint.isEmpty()) { - msg += QLatin1String("\n") + hint + QLatin1String("\n"); + if (!_fetchErrorString.isEmpty()) { + msg += QLatin1String("\n") + tr("Reading from keychain failed with error: '%1'").arg( + _fetchErrorString) + QLatin1String("\n"); } - return QInputDialog::getText(0, tr("Enter Password"), msg, + bool ok = false; + QString pwd = QInputDialog::getText(0, tr("Enter Password"), msg, QLineEdit::Password, _previousPassword, - ok); + &ok); + if (ok) { + _password = pwd; + _ready = true; + persist(); + } + emit asked(); } } // namespace OCC diff --git a/src/gui/creds/httpcredentialsgui.h b/src/gui/creds/httpcredentialsgui.h index 22c9d4370..4c5908d43 100644 --- a/src/gui/creds/httpcredentialsgui.h +++ b/src/gui/creds/httpcredentialsgui.h @@ -28,7 +28,8 @@ class HttpCredentialsGui : public HttpCredentials { public: explicit HttpCredentialsGui() : HttpCredentials() {} HttpCredentialsGui(const QString& user, const QString& password, const QString& certificatePath, const QString& certificatePasswd) : HttpCredentials(user, password, certificatePath, certificatePasswd) {} - QString queryPassword(bool *ok, const QString& hint) Q_DECL_OVERRIDE; + void askFromUser() Q_DECL_OVERRIDE; + Q_INVOKABLE void askFromUserAsync(); }; } // namespace OCC diff --git a/src/gui/creds/shibbolethcredentials.cpp b/src/gui/creds/shibbolethcredentials.cpp index 2c6e80dd8..12b9b7d82 100644 --- a/src/gui/creds/shibbolethcredentials.cpp +++ b/src/gui/creds/shibbolethcredentials.cpp @@ -26,7 +26,6 @@ #include "accessmanager.h" #include "account.h" -#include "logger.h" #include "theme.h" #include "cookiejar.h" #include "syncengine.h" @@ -60,7 +59,6 @@ ShibbolethCredentials::ShibbolethCredentials(const QNetworkCookie& cookie) : _ready(true), _stillValid(true), _fetchJobInProgress(false), - _interactiveFetch(true), _browser(0), _shibCookie(cookie) { @@ -153,12 +151,11 @@ bool ShibbolethCredentials::ready() const return _ready; } -void ShibbolethCredentials::fetch(FetchMode mode) +void ShibbolethCredentials::fetchFromKeychain() { if(_fetchJobInProgress) { return; } - _interactiveFetch = mode == Interactive; if (_user.isEmpty()) { _user = _account->credentialSetting(QLatin1String(userC)).toString(); @@ -178,6 +175,11 @@ void ShibbolethCredentials::fetch(FetchMode mode) } } +void ShibbolethCredentials::askFromUser() +{ + showLoginWindow(); +} + bool ShibbolethCredentials::stillValid(QNetworkReply *reply) { Q_UNUSED(reply) @@ -262,7 +264,7 @@ void ShibbolethCredentials::slotUserFetched(const QString &user) _stillValid = true; _ready = true; _fetchJobInProgress = false; - Q_EMIT fetched(); + Q_EMIT asked(); } @@ -270,7 +272,7 @@ void ShibbolethCredentials::slotBrowserRejected() { _ready = false; _fetchJobInProgress = false; - Q_EMIT fetched(); + Q_EMIT asked(); } void ShibbolethCredentials::slotReadJobDone(QKeychain::Job *job) @@ -290,10 +292,7 @@ void ShibbolethCredentials::slotReadJobDone(QKeychain::Job *job) _stillValid = true; _fetchJobInProgress = false; Q_EMIT fetched(); - } else if (_interactiveFetch) { - showLoginWindow(); } else { - Logger::instance()->postOptionalGuiLog(tr("Reauthentication required"), tr("You need to re-login to continue using the account %1.").arg(_account->displayName())); _ready = false; _fetchJobInProgress = false; Q_EMIT fetched(); diff --git a/src/gui/creds/shibbolethcredentials.h b/src/gui/creds/shibbolethcredentials.h index f4cbd45a7..19bbbb25a 100644 --- a/src/gui/creds/shibbolethcredentials.h +++ b/src/gui/creds/shibbolethcredentials.h @@ -55,7 +55,8 @@ public: QString user() const Q_DECL_OVERRIDE; QNetworkAccessManager* getQNAM() const Q_DECL_OVERRIDE; bool ready() const Q_DECL_OVERRIDE; - void fetch(FetchMode mode = Interactive) Q_DECL_OVERRIDE; + void fetchFromKeychain() Q_DECL_OVERRIDE; + void askFromUser(); bool stillValid(QNetworkReply *reply) Q_DECL_OVERRIDE; void persist() Q_DECL_OVERRIDE; void invalidateToken() Q_DECL_OVERRIDE; @@ -88,7 +89,6 @@ private: bool _ready; bool _stillValid; bool _fetchJobInProgress; - bool _interactiveFetch; QPointer _browser; QNetworkCookie _shibCookie; QString _user; diff --git a/src/libsync/account.cpp b/src/libsync/account.cpp index 9ce6608e7..f1e0535d5 100644 --- a/src/libsync/account.cpp +++ b/src/libsync/account.cpp @@ -141,6 +141,8 @@ void Account::setCredentials(AbstractCredentials *cred) SIGNAL(proxyAuthenticationRequired(QNetworkProxy,QAuthenticator*))); connect(_credentials, SIGNAL(fetched()), SLOT(slotCredentialsFetched())); + connect(_credentials, SIGNAL(asked()), + SLOT(slotCredentialsAsked())); } QUrl Account::davUrl() const @@ -423,6 +425,11 @@ void Account::slotCredentialsFetched() emit credentialsFetched(_credentials); } +void Account::slotCredentialsAsked() +{ + emit credentialsAsked(_credentials); +} + void Account::handleInvalidCredentials() { emit invalidCredentials(); diff --git a/src/libsync/account.h b/src/libsync/account.h index 4f98aa839..63ae12166 100644 --- a/src/libsync/account.h +++ b/src/libsync/account.h @@ -163,6 +163,7 @@ signals: void propagatorNetworkActivity(); void invalidCredentials(); void credentialsFetched(AbstractCredentials* credentials); + void credentialsAsked(AbstractCredentials* credentials); /// Forwards from QNetworkAccessManager::proxyAuthenticationRequired(). void proxyAuthenticationRequired(const QNetworkProxy&, QAuthenticator*); @@ -173,6 +174,7 @@ signals: protected Q_SLOTS: void slotHandleSslErrors(QNetworkReply*,QList); void slotCredentialsFetched(); + void slotCredentialsAsked(); private: Account(QObject *parent = 0); diff --git a/src/libsync/connectionvalidator.cpp b/src/libsync/connectionvalidator.cpp index 2b715ff33..3a55c96e2 100644 --- a/src/libsync/connectionvalidator.cpp +++ b/src/libsync/connectionvalidator.cpp @@ -23,10 +23,9 @@ namespace OCC { -ConnectionValidator::ConnectionValidator(AccountPtr account, AbstractCredentials::FetchMode credentialsFetchMode, QObject *parent) +ConnectionValidator::ConnectionValidator(AccountPtr account, QObject *parent) : QObject(parent), _account(account), - _credentialsFetchMode(credentialsFetchMode), _isCheckingServerAndAuth(false) { } diff --git a/src/libsync/connectionvalidator.h b/src/libsync/connectionvalidator.h index afd598cf9..045adaf8f 100644 --- a/src/libsync/connectionvalidator.h +++ b/src/libsync/connectionvalidator.h @@ -20,7 +20,6 @@ #include #include #include "accountfwd.h" -#include "creds/abstractcredentials.h" namespace OCC { @@ -68,14 +67,14 @@ class OWNCLOUDSYNC_EXPORT ConnectionValidator : public QObject { Q_OBJECT public: - explicit ConnectionValidator(AccountPtr account, AbstractCredentials::FetchMode credentialsFetchMode, QObject *parent = 0); + explicit ConnectionValidator(AccountPtr account, QObject *parent = 0); enum Status { Undefined, Connected, NotConfigured, ServerVersionMismatch, - CredentialsWrong, + CredentialsMissingOrWrong, StatusNotFound, UserCanceledCredentials, ServiceUnavailable, @@ -114,7 +113,6 @@ private: QStringList _errors; AccountPtr _account; - AbstractCredentials::FetchMode _credentialsFetchMode; bool _isCheckingServerAndAuth; }; diff --git a/src/libsync/creds/abstractcredentials.h b/src/libsync/creds/abstractcredentials.h index 6e83febbd..607422641 100644 --- a/src/libsync/creds/abstractcredentials.h +++ b/src/libsync/creds/abstractcredentials.h @@ -30,7 +30,6 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject Q_OBJECT public: - enum FetchMode { Interactive, NonInteractive }; AbstractCredentials(); // No need for virtual destructor - QObject already has one. @@ -49,7 +48,8 @@ public: virtual QString user() const = 0; virtual QNetworkAccessManager* getQNAM() const = 0; virtual bool ready() const = 0; - virtual void fetch(FetchMode mode = Interactive) = 0; + virtual void fetchFromKeychain() = 0; + virtual void askFromUser() = 0; virtual bool stillValid(QNetworkReply *reply) = 0; virtual void persist() = 0; /** Invalidates auth token, or password for basic auth */ @@ -59,6 +59,7 @@ public: Q_SIGNALS: void fetched(); + void asked(); protected: Account* _account; diff --git a/src/libsync/creds/dummycredentials.cpp b/src/libsync/creds/dummycredentials.cpp index 3dbfaa0dd..b0a043235 100644 --- a/src/libsync/creds/dummycredentials.cpp +++ b/src/libsync/creds/dummycredentials.cpp @@ -56,11 +56,16 @@ bool DummyCredentials::stillValid(QNetworkReply *reply) return true; } -void DummyCredentials::fetch(FetchMode) +void DummyCredentials::fetchFromKeychain() { Q_EMIT(fetched()); } +void DummyCredentials::askFromUser() +{ + Q_EMIT(asked()); +} + void DummyCredentials::persist() {} diff --git a/src/libsync/creds/dummycredentials.h b/src/libsync/creds/dummycredentials.h index 8536d2042..4fa100b52 100644 --- a/src/libsync/creds/dummycredentials.h +++ b/src/libsync/creds/dummycredentials.h @@ -35,7 +35,8 @@ public: QNetworkAccessManager* getQNAM() const Q_DECL_OVERRIDE; bool ready() const Q_DECL_OVERRIDE; bool stillValid(QNetworkReply *reply) Q_DECL_OVERRIDE; - void fetch(FetchMode mode = Interactive) Q_DECL_OVERRIDE; + void fetchFromKeychain() Q_DECL_OVERRIDE; + void askFromUser() Q_DECL_OVERRIDE; void persist() Q_DECL_OVERRIDE; void invalidateToken() Q_DECL_OVERRIDE {} }; diff --git a/src/libsync/creds/httpcredentials.cpp b/src/libsync/creds/httpcredentials.cpp index 8ce1af2da..cfeba26a4 100644 --- a/src/libsync/creds/httpcredentials.cpp +++ b/src/libsync/creds/httpcredentials.cpp @@ -24,7 +24,6 @@ #include "account.h" #include "accessmanager.h" -#include "logger.h" #include "utility.h" #include "theme.h" #include "syncengine.h" @@ -83,24 +82,18 @@ const char authenticationFailedC[] = "owncloud-authentication-failed"; } // ns HttpCredentials::HttpCredentials() - : _user(), - _password(), - _certificatePath(), - _certificatePasswd(), - _ready(false), - _fetchJobInProgress(false), - _interactiveFetch(true) + : _ready(false), + _fetchJobInProgress(false) { } HttpCredentials::HttpCredentials(const QString& user, const QString& password, const QString& certificatePath, const QString& certificatePasswd) : _user(user), _password(password), + _ready(true), _certificatePath(certificatePath), _certificatePasswd(certificatePasswd), - _ready(true), - _fetchJobInProgress(false), - _interactiveFetch(true) + _fetchJobInProgress(false) { } @@ -202,12 +195,12 @@ QString HttpCredentials::fetchUser() return _user; } -void HttpCredentials::fetch(FetchMode mode) +void HttpCredentials::fetchFromKeychain() { + // FIXME: Should this check go if we check in AccountState instead? if (_fetchJobInProgress) { return; } - _interactiveFetch = mode == Interactive; // User must be fetched from config file fetchUser(); @@ -257,8 +250,8 @@ void HttpCredentials::slotReadJobDone(QKeychain::Job *job) QKeychain::Error error = job->error(); + _fetchJobInProgress = false; if( !_password.isEmpty() && error == NoError ) { - _fetchJobInProgress = false; // All cool, the keychain did not come back with error. // Still, the password can be empty which indicates a problem and @@ -268,31 +261,11 @@ void HttpCredentials::slotReadJobDone(QKeychain::Job *job) } else { // we come here if the password is empty or any other keychain // error happend. - // In all error conditions it should - // ask the user for the password interactively now. - // interactive password dialog starts here - - QString hint; - if (job->error() != EntryNotFound) { - hint = tr("Reading from keychain failed with error: '%1'").arg( - job->errorString()); - } - bool ok = false; - QString pwd; - if (_interactiveFetch) - pwd = queryPassword(&ok, hint); - else - Logger::instance()->postOptionalGuiLog(tr("Reauthentication required"), tr("You need to re-login to continue using the account %1.").arg(_account->displayName())); - _fetchJobInProgress = false; - if (ok) { - _password = pwd; - _ready = true; - persist(); - } else { - _password = QString::null; - _ready = false; - } + _fetchErrorString = job->error() != EntryNotFound ? job->errorString() : QString(); + + _password = QString(); + _ready = false; emit fetched(); } } diff --git a/src/libsync/creds/httpcredentials.h b/src/libsync/creds/httpcredentials.h index 685502d0e..bbd0a3d2c 100644 --- a/src/libsync/creds/httpcredentials.h +++ b/src/libsync/creds/httpcredentials.h @@ -44,12 +44,11 @@ public: QString authType() const Q_DECL_OVERRIDE; QNetworkAccessManager* getQNAM() const Q_DECL_OVERRIDE; bool ready() const Q_DECL_OVERRIDE; - void fetch(FetchMode mode = Interactive) Q_DECL_OVERRIDE; + void fetchFromKeychain() Q_DECL_OVERRIDE; bool stillValid(QNetworkReply *reply) Q_DECL_OVERRIDE; void persist() Q_DECL_OVERRIDE; QString user() const Q_DECL_OVERRIDE; QString password() const; - virtual QString queryPassword(bool *ok, const QString& hint) = 0; void invalidateToken() Q_DECL_OVERRIDE; QString fetchUser(); virtual bool sslIsTrusted() { return false; } @@ -68,13 +67,13 @@ protected: QString _user; QString _password; QString _previousPassword; + QString _fetchErrorString; + bool _ready; private: QString _certificatePath; QString _certificatePasswd; - bool _ready; bool _fetchJobInProgress; //True if the keychain job is in progress or the input dialog visible - bool _interactiveFetch; }; } // namespace OCC -- 2.30.2