Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Credentials: Use per-account keychain entries #5830 #6027

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Credentials: Use per-account keychain entries #5830 #6027

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bf1936
Credentials: Use per-account keychain entries #5830
ckamm Sep 12, 2017
0232546
HttpCreds: Remove migration code from 1.7
ckamm Sep 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 40 additions & 9 deletions src/gui/creds/shibbolethcredentials.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ ShibbolethCredentials::ShibbolethCredentials()
, _ready(false)
, _stillValid(false)
, _browser(0)
, _keychainMigration(false)
{
}

Expand All @@ -62,6 +63,7 @@ ShibbolethCredentials::ShibbolethCredentials(const QNetworkCookie &cookie)
, _stillValid(true)
, _browser(0)
, _shibCookie(cookie)
, _keychainMigration(false)
{
}

Expand Down Expand Up @@ -131,15 +133,22 @@ void ShibbolethCredentials::fetchFromKeychain()
Q_EMIT fetched();
} else {
_url = _account->url();
ReadPasswordJob *job = new ReadPasswordJob(Theme::instance()->appName());
job->setSettings(ConfigFile::settingsWithGroup(Theme::instance()->appName(), job).release());
job->setInsecureFallback(false);
job->setKey(keychainKey(_account->url().toString(), user()));
connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotReadJobDone(QKeychain::Job *)));
job->start();
_keychainMigration = false;
fetchFromKeychainHelper();
}
}

void ShibbolethCredentials::fetchFromKeychainHelper()
{
ReadPasswordJob *job = new ReadPasswordJob(Theme::instance()->appName());
job->setSettings(ConfigFile::settingsWithGroup(Theme::instance()->appName(), job).release());
job->setInsecureFallback(false);
job->setKey(keychainKey(_url.toString(), user(),
_keychainMigration ? QString() : _account->id()));
connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotReadJobDone(QKeychain::Job *)));
job->start();
}

void ShibbolethCredentials::askFromUser()
{
showLoginWindow();
Expand Down Expand Up @@ -242,6 +251,16 @@ void ShibbolethCredentials::slotBrowserRejected()

void ShibbolethCredentials::slotReadJobDone(QKeychain::Job *job)
{
// If we can't find the credentials at the keys that include the account id,
// try to read them from the legacy locations that don't have a account id.
if (!_keychainMigration && job->error() == QKeychain::EntryNotFound) {
qCWarning(lcShibboleth)
<< "Could not find keychain entry, attempting to read from legacy location";
_keychainMigration = true;
fetchFromKeychainHelper();
return;
}

if (job->error() == QKeychain::NoError) {
ReadPasswordJob *readJob = static_cast<ReadPasswordJob *>(job);
delete readJob->settings();
Expand All @@ -260,6 +279,19 @@ void ShibbolethCredentials::slotReadJobDone(QKeychain::Job *job)
_ready = false;
Q_EMIT fetched();
}


// If keychain data was read from legacy location, wipe these entries and store new ones
if (_keychainMigration && _ready) {
persist();

DeletePasswordJob *job = new DeletePasswordJob(Theme::instance()->appName());
job->setSettings(ConfigFile::settingsWithGroup(Theme::instance()->appName(), job).release());
job->setKey(keychainKey(_account->url().toString(), user(), QString()));
job->start();

qCWarning(lcShibboleth) << "Migrated old keychain entries";
}
}

void ShibbolethCredentials::showLoginWindow()
Expand Down Expand Up @@ -313,7 +345,7 @@ void ShibbolethCredentials::storeShibCookie(const QNetworkCookie &cookie)
job->setSettings(ConfigFile::settingsWithGroup(Theme::instance()->appName(), job).release());
// we don't really care if it works...
//connect(job, SIGNAL(finished(QKeychain::Job*)), SLOT(slotWriteJobDone(QKeychain::Job*)));
job->setKey(keychainKey(_account->url().toString(), user()));
job->setKey(keychainKey(_account->url().toString(), user(), _account->id()));
job->setTextData(QString::fromUtf8(cookie.toRawForm()));
job->start();
}
Expand All @@ -322,7 +354,7 @@ void ShibbolethCredentials::removeShibCookie()
{
DeletePasswordJob *job = new DeletePasswordJob(Theme::instance()->appName());
job->setSettings(ConfigFile::settingsWithGroup(Theme::instance()->appName(), job).release());
job->setKey(keychainKey(_account->url().toString(), user()));
job->setKey(keychainKey(_account->url().toString(), user(), _account->id()));
job->start();
}

Expand All @@ -336,5 +368,4 @@ void ShibbolethCredentials::addToCookieJar(const QNetworkCookie &cookie)
jar->blockSignals(false);
}


} // namespace OCC
5 changes: 5 additions & 0 deletions src/gui/creds/shibbolethcredentials.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ private Q_SLOTS:
void storeShibCookie(const QNetworkCookie &cookie);
void removeShibCookie();
void addToCookieJar(const QNetworkCookie &cookie);

/// Reads data from keychain, progressing to slotReadJobDone
void fetchFromKeychainHelper();

QUrl _url;
QByteArray prepareCookieData() const;

Expand All @@ -92,6 +96,7 @@ private Q_SLOTS:
QPointer<ShibbolethWebView> _browser;
QNetworkCookie _shibCookie;
QString _user;
bool _keychainMigration;
};

} // namespace OCC
Expand Down
5 changes: 4 additions & 1 deletion src/libsync/creds/abstractcredentials.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ void AbstractCredentials::setAccount(Account *account)
_account = account;
}

QString AbstractCredentials::keychainKey(const QString &url, const QString &user)
QString AbstractCredentials::keychainKey(const QString &url, const QString &user, const QString &accountId)
{
QString u(url);
if (u.isEmpty()) {
Expand All @@ -51,6 +51,9 @@ QString AbstractCredentials::keychainKey(const QString &url, const QString &user
}

QString key = user + QLatin1Char(':') + u;
if (!accountId.isEmpty()) {
key += QLatin1Char(':') + accountId;
}
return key;
}
} // namespace OCC
2 changes: 1 addition & 1 deletion src/libsync/creds/abstractcredentials.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class OWNCLOUDSYNC_EXPORT AbstractCredentials : public QObject
*/
virtual void forgetSensitiveData() = 0;

static QString keychainKey(const QString &url, const QString &user);
static QString keychainKey(const QString &url, const QString &user, const QString &accountId);

Q_SIGNALS:
/** Emitted when fetchFromKeychain() is done.
Expand Down
86 changes: 66 additions & 20 deletions src/libsync/creds/httpcredentials.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ static void addSettingsToJob(Account *account, QKeychain::Job *job)

HttpCredentials::HttpCredentials()
: _ready(false)
, _keychainMigration(false)
{
}

Expand All @@ -113,6 +114,7 @@ HttpCredentials::HttpCredentials(const QString &user, const QString &password, c
, _ready(true)
, _clientSslKey(key)
, _clientSslCertificate(certificate)
, _keychainMigration(false)
{
}

Expand Down Expand Up @@ -174,21 +176,43 @@ void HttpCredentials::fetchFromKeychain()
return;
}

const QString kck = keychainKey(_account->url().toString(), _user);

if (_ready) {
Q_EMIT fetched();
} else {
// Read client cert from keychain
const QString kck = keychainKey(_account->url().toString(), _user + clientCertificatePEMC);
ReadPasswordJob *job = new ReadPasswordJob(Theme::instance()->appName());
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
job->setKey(kck);
_keychainMigration = false;
fetchFromKeychainHelper();
}
}

connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotReadClientCertPEMJobDone(QKeychain::Job *)));
void HttpCredentials::fetchFromKeychainHelper()
{
// Read client cert from keychain
const QString kck = keychainKey(
_account->url().toString(),
_user + clientCertificatePEMC,
_keychainMigration ? QString() : _account->id());

ReadPasswordJob *job = new ReadPasswordJob(Theme::instance()->appName());
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
job->setKey(kck);
connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotReadClientCertPEMJobDone(QKeychain::Job *)));
job->start();
}

void HttpCredentials::deleteOldKeychainEntries()
{
auto startDeleteJob = [this](QString user) {
DeletePasswordJob *job = new DeletePasswordJob(Theme::instance()->appName());
addSettingsToJob(_account, job);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In HttpCredentials::invalidateToken, there is a job2 to remove the password from a "deprecated location" which does not set the settings.

Should we not do the same here? (in case of upgrading from a 1.7 client? maybe this is anyway no longer supported)

But in any case, we can remove this job2 from HttpCredentials::invalidateToken

job->setInsecureFallback(true);
job->setKey(keychainKey(_account->url().toString(), user, QString()));
job->start();
}
};

startDeleteJob(_user);
startDeleteJob(_user + clientKeyPEMC);
startDeleteJob(_user + clientCertificatePEMC);
}

void HttpCredentials::slotReadClientCertPEMJobDone(QKeychain::Job *incoming)
Expand All @@ -203,12 +227,15 @@ void HttpCredentials::slotReadClientCertPEMJobDone(QKeychain::Job *incoming)
}

// Load key too
const QString kck = keychainKey(_account->url().toString(), _user + clientKeyPEMC);
const QString kck = keychainKey(
_account->url().toString(),
_user + clientKeyPEMC,
_keychainMigration ? QString() : _account->id());

ReadPasswordJob *job = new ReadPasswordJob(Theme::instance()->appName());
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
job->setKey(kck);

connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotReadClientKeyPEMJobDone(QKeychain::Job *)));
job->start();
}
Expand Down Expand Up @@ -238,12 +265,15 @@ void HttpCredentials::slotReadClientKeyPEMJobDone(QKeychain::Job *incoming)
}

// Now fetch the actual server password
const QString kck = keychainKey(_account->url().toString(), _user);
const QString kck = keychainKey(
_account->url().toString(),
_user,
_keychainMigration ? QString() : _account->id());

ReadPasswordJob *job = new ReadPasswordJob(Theme::instance()->appName());
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
job->setKey(kck);

connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotReadJobDone(QKeychain::Job *)));
job->start();
}
Expand All @@ -260,6 +290,17 @@ bool HttpCredentials::stillValid(QNetworkReply *reply)
void HttpCredentials::slotReadJobDone(QKeychain::Job *incomingJob)
{
QKeychain::ReadPasswordJob *job = static_cast<ReadPasswordJob *>(incomingJob);
QKeychain::Error error = job->error();

// If we can't find the credentials at the keys that include the account id,
// try to read them from the legacy locations that don't have a account id.
if (!_keychainMigration && error == QKeychain::EntryNotFound) {
qCWarning(lcHttpCredentials)
<< "Could not find keychain entries, attempting to read from legacy locations";
_keychainMigration = true;
fetchFromKeychainHelper();
return;
}

bool isOauth = _account->credentialSetting(QLatin1String(isOAuthC)).toBool();
if (isOauth) {
Expand All @@ -272,8 +313,6 @@ void HttpCredentials::slotReadJobDone(QKeychain::Job *incomingJob)
qCWarning(lcHttpCredentials) << "Strange: User is empty!";
}

QKeychain::Error error = job->error();

if (!_refreshToken.isEmpty() && error == NoError) {
refreshAccessToken();
} else if (!_password.isEmpty() && error == NoError) {
Expand All @@ -292,6 +331,13 @@ void HttpCredentials::slotReadJobDone(QKeychain::Job *incomingJob)
_ready = false;
emit fetched();
}

// If keychain data was read from legacy location, wipe these entries and store new ones
if (_keychainMigration && _ready) {
persist();
deleteOldKeychainEntries();
qCWarning(lcHttpCredentials) << "Migrated old keychain entries";
}
}

bool HttpCredentials::refreshAccessToken()
Expand Down Expand Up @@ -344,7 +390,7 @@ void HttpCredentials::invalidateToken()
// User must be fetched from config file to generate a valid key
fetchUser();

const QString kck = keychainKey(_account->url().toString(), _user);
const QString kck = keychainKey(_account->url().toString(), _user, _account->id());
if (kck.isEmpty()) {
qCWarning(lcHttpCredentials) << "InvalidateToken: User is empty, bailing out!";
return;
Expand Down Expand Up @@ -406,7 +452,7 @@ void HttpCredentials::persist()
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotWriteClientCertPEMJobDone(QKeychain::Job *)));
job->setKey(keychainKey(_account->url().toString(), _user + clientCertificatePEMC));
job->setKey(keychainKey(_account->url().toString(), _user + clientCertificatePEMC, _account->id()));
job->setBinaryData(_clientSslCertificate.toPem());
job->start();
}
Expand All @@ -419,7 +465,7 @@ void HttpCredentials::slotWriteClientCertPEMJobDone(Job *incomingJob)
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotWriteClientKeyPEMJobDone(QKeychain::Job *)));
job->setKey(keychainKey(_account->url().toString(), _user + clientKeyPEMC));
job->setKey(keychainKey(_account->url().toString(), _user + clientKeyPEMC, _account->id()));
job->setBinaryData(_clientSslKey.toPem());
job->start();
}
Expand All @@ -431,7 +477,7 @@ void HttpCredentials::slotWriteClientKeyPEMJobDone(Job *incomingJob)
addSettingsToJob(_account, job);
job->setInsecureFallback(false);
connect(job, SIGNAL(finished(QKeychain::Job *)), SLOT(slotWriteJobDone(QKeychain::Job *)));
job->setKey(keychainKey(_account->url().toString(), _user));
job->setKey(keychainKey(_account->url().toString(), _user, _account->id()));
job->setTextData(isUsingOAuth() ? _refreshToken : _password);
job->start();
}
Expand Down
13 changes: 13 additions & 0 deletions src/libsync/creds/httpcredentials.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,18 @@ private Q_SLOTS:
void slotWriteJobDone(QKeychain::Job *);

protected:
/** Reads data from keychain locations
*
* Goes through
* slotReadClientCertPEMJobDone to
* slotReadClientCertPEMJobDone to
* slotReadJobDone
*/
void fetchFromKeychainHelper();

/// Wipes legacy keychain locations
void deleteOldKeychainEntries();

QString _user;
QString _password; // user's password, or access_token for OAuth
QString _refreshToken; // OAuth _refreshToken, set if OAuth is used.
Expand All @@ -128,6 +140,7 @@ private Q_SLOTS:
bool _ready;
QSslKey _clientSslKey;
QSslCertificate _clientSslCertificate;
bool _keychainMigration;
};


Expand Down