Skip to content

Commit 8f6a10c

Browse files
DeepDiver1975kw-tmueller
DeepDiver1975
and
kw-tmueller
authored
fix: remove oc10 OAuth2 fallback (#12222)
* fix: remove oc10 OAuth2 fallback * fix: adjust oauth tests and test 307 response upon return of .well-known/openid-configuration * feat: add error state for oauth when no authorization url is known --------- Co-authored-by: Thomas Müller <[email protected]>
1 parent 34624e6 commit 8f6a10c

File tree

6 files changed

+99
-48
lines changed

6 files changed

+99
-48
lines changed

‎src/gui/creds/httpcredentialsgui.cpp‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ void HttpCredentialsGui::asyncAuthResult(OAuth::Result r, const QString &token,
6464
// should not happen after the initial setup
6565
Q_ASSERT(false);
6666
[[fallthrough]];
67+
case OAuth::ErrorIdPUnreachable:
68+
// TODO: add user facing error message that authentication is currently not possible - retry is the wrong advice
69+
[[fallthrough]];
6770
case OAuth::Error:
6871
Q_EMIT oAuthErrorOccurred();
6972
return;

‎src/gui/creds/oauth.cpp‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,11 @@ void OAuth::handleSocketReadyRead()
260260
// todo: #24 - this can be converted to an adapter
261261
void OAuth::getTokens()
262262
{
263+
if(_tokenEndpoint.isEmpty()) {
264+
Q_EMIT result(Error);
265+
return;
266+
}
267+
263268
auto postTokenReply = postTokenRequest({
264269
{QStringLiteral("grant_type"), QStringLiteral("authorization_code")},
265270
{QStringLiteral("code"), _queryArgs.queryItemValue(QStringLiteral("code"))},
@@ -343,8 +348,7 @@ void OAuth::checkUserInfo()
343348
// todo: #24 - I think this should also be an adapter. I also have a vague recollection that we have a job that does this already
344349
QNetworkReply *OAuth::postTokenRequest(QUrlQuery &&queryItems)
345350
{
346-
const QUrl requestTokenUrl =
347-
_tokenEndpoint.isEmpty() ? Utility::concatUrlPath(_serverUrl, QStringLiteral("/index.php/apps/oauth2/api/v1/token")) : _tokenEndpoint;
351+
const QUrl requestTokenUrl = _tokenEndpoint;
348352
QNetworkRequest req;
349353
req.setTransferTimeout(defaultTimeoutMs());
350354
switch (_endpointAuthMethod) {
@@ -377,6 +381,10 @@ QUrl OAuth::authorisationLink() const
377381
Q_ASSERT(_server.isListening());
378382
Q_ASSERT(_wellKnownFinished);
379383

384+
if (!_authEndpoint.isValid()) {
385+
return {}; // no auth endpoint, cannot continue
386+
}
387+
380388
const QByteArray code_challenge =
381389
QCryptographicHash::hash(_pkceCodeVerifier, QCryptographicHash::Sha256).toBase64(QByteArray::Base64UrlEncoding | QByteArray::OmitTrailingEquals);
382390
QUrlQuery query{{QStringLiteral("response_type"), QStringLiteral("code")}, {QStringLiteral("client_id"), _clientId},
@@ -393,11 +401,7 @@ QUrl OAuth::authorisationLink() const
393401
// todo: #20 oc10 as fallback!
394402
query.addQueryItem(QStringLiteral("user"), davUser);
395403
}
396-
const QUrl url = _authEndpoint.isValid() ? Utility::concatUrlPath(_authEndpoint, {}, query)
397-
// todo: #20 oc10!
398-
: Utility::concatUrlPath(_serverUrl, QStringLiteral("/index.php/apps/oauth2/authorize"), query);
399-
400-
return url;
404+
return Utility::concatUrlPath(_authEndpoint, {}, query);
401405
}
402406

403407
void OAuth::fetchWellKnown()
@@ -477,17 +481,21 @@ bool isUrlSchemeValid(const QUrl &url)
477481

478482
void OAuth::openBrowser()
479483
{
480-
Q_ASSERT(!authorisationLink().isEmpty());
481-
484+
auto authorisationURL = authorisationLink();
485+
if (authorisationURL.isEmpty()) {
486+
qCWarning(lcOauth) << "Authorization URL is unknown - well-known/openid-configuration endpoint did not return usable information";
487+
Q_EMIT result(ErrorIdPUnreachable, QString());
488+
return;
489+
}
482490
qCDebug(lcOauth) << "opening browser";
483491

484-
if (!isUrlSchemeValid(authorisationLink())) {
492+
if (!isUrlSchemeValid(authorisationURL)) {
485493
qCWarning(lcOauth) << "URL validation failed";
486494
Q_EMIT result(ErrorInsecureUrl, QString());
487495
return;
488496
}
489497

490-
if (!QDesktopServices::openUrl(authorisationLink())) {
498+
if (!QDesktopServices::openUrl(authorisationURL)) {
491499
qCWarning(lcOauth) << "QDesktopServices::openUrl Failed";
492500
// We cannot open the browser, then we claim we don't support OAuth.
493501
Q_EMIT result(NotSupported, QString());

‎src/gui/creds/oauth.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ class OAuth : public QObject
5454
{
5555
Q_OBJECT
5656
public:
57-
enum Result { NotSupported, LoggedIn, Error, ErrorInsecureUrl };
57+
enum Result { NotSupported, LoggedIn, Error, ErrorInsecureUrl, ErrorIdPUnreachable };
5858
Q_ENUM(Result)
5959
enum class TokenEndpointAuthMethods : char { client_secret_basic, client_secret_post };
6060
Q_ENUM(TokenEndpointAuthMethods)

‎src/gui/newaccountwizard/oauthpagecontroller.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,11 @@ void OAuthPageController::handleOauthResult(OAuth::Result result, const QString
286286
break;
287287
}
288288
case OAuth::Result::ErrorInsecureUrl: {
289-
handleError(tr("Oauth2 authentication requires a secured connection."));
289+
handleError(tr("OAuth2 authentication requires a secured connection."));
290+
break;
291+
}
292+
case OAuth::Result::ErrorIdPUnreachable: {
293+
handleError(tr("Authorization server unreachable."));
290294
break;
291295
}
292296
};

‎src/gui/newwizard/states/oauthcredentialssetupwizardstate.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,11 @@ void OAuthCredentialsSetupWizardState::handleOAuthResult(OAuth::Result result, c
6666
break;
6767
}
6868
case OAuth::Result::ErrorInsecureUrl: {
69-
Q_EMIT evaluationFailed(tr("Oauth2 authentication requires a secured connection."));
69+
Q_EMIT evaluationFailed(tr("OAuth2 authentication requires a secured connection."));
70+
break;
71+
}
72+
case OAuth::Result::ErrorIdPUnreachable: {
73+
Q_EMIT evaluationFailed(tr("Authorization server unreachable."));
7074
break;
7175
}
7276
};

‎test/testoauth.cpp‎

Lines changed: 66 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ class DesktopServiceHook : public QObject
2727
~DesktopServiceHook() { QDesktopServices::unsetUrlHandler(QStringLiteral("oauthtest")); }
2828
};
2929

30-
static const QUrl sOAuthTestServer(QStringLiteral("oauthtest://someserver/owncloud"));
30+
static const QUrl sOpenIdBaseURL(QStringLiteral("oauthtest://auth.example.com"));
31+
static const QUrl sOpenIdAuthURL(QStringLiteral("oauthtest://auth.example.com/realms/test/protocol/openid-connect/auth"));
3132

3233

3334
class FakePostReply : public QNetworkReply
@@ -115,8 +116,7 @@ class OAuthTestCase : public QObject
115116
CustomState } state = StartState;
116117
Q_ENUM(State);
117118

118-
// for oauth2 we use localhost, for oidc we use 127.0.0.1
119-
QString localHost = QStringLiteral("localhost");
119+
QString localHost = QStringLiteral("127.0.0.1");
120120
bool replyToBrowserOk = false;
121121
bool gotAuthOk = false;
122122
virtual bool done() const { return replyToBrowserOk && gotAuthOk; }
@@ -133,16 +133,18 @@ class OAuthTestCase : public QObject
133133
{
134134
fakeAm = new FakeAM({}, nullptr);
135135
account = Account::create(QUuid::createUuid());
136-
account->setUrl(sOAuthTestServer);
136+
account->setUrl(sOpenIdBaseURL);
137137
// the account seizes ownership over the qnam in account->setCredentials(...) by keeping a shared pointer on it
138138
// therefore, we should never call fakeAm->setThis(...)
139139
account->setCredentials(new FakeCredentials { fakeAm });
140140
fakeAm->setOverride([this](QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *device) {
141141
if (req.url().path().endsWith(QLatin1String(".well-known/openid-configuration"))) {
142142
return this->wellKnownReply(op, req);
143-
} else if (req.url().path().endsWith(QLatin1String("status.php"))) {
143+
}
144+
if (req.url().path().endsWith(QLatin1String("status.php"))) {
144145
return this->statusPhpReply(op, req);
145-
} else if (req.url().path().endsWith(QLatin1String("ocs/v2.php/cloud/user")) && req.url().query() == QLatin1String("format=json")) {
146+
}
147+
if (req.url().path().endsWith(QLatin1String("ocs/v2.php/cloud/user")) && req.url().query() == QLatin1String("format=json")) {
146148
return this->userInfoReply(op, req);
147149
}
148150
OC_ASSERT(device);
@@ -158,6 +160,13 @@ class OAuthTestCase : public QObject
158160
}
159161

160162
virtual void test()
163+
{
164+
runTest();
165+
166+
QTRY_VERIFY(done());
167+
}
168+
169+
virtual void runTest()
161170
{
162171
oauth = prepareOauth();
163172
oauth->startAuthentication();
@@ -166,15 +175,13 @@ class OAuthTestCase : public QObject
166175
if (spy.wait()) {
167176
oauth->openBrowser();
168177
}
169-
170-
QTRY_VERIFY(done());
171178
}
172179

173180
virtual void openBrowserHook(const QUrl &url) {
174181
QCOMPARE(state, StatusPhpState);
175182
state = BrowserOpened;
176-
QCOMPARE(url.path(), sOAuthTestServer.path() + QStringLiteral("/index.php/apps/oauth2/authorize"));
177-
QVERIFY(url.toString().startsWith(sOAuthTestServer.toString()));
183+
QCOMPARE(url.path(), sOpenIdAuthURL.path());
184+
QVERIFY(url.toString().startsWith(sOpenIdBaseURL.toString()));
178185
QUrlQuery query(url);
179186
QCOMPARE(query.queryItemValue(QStringLiteral("response_type")), QLatin1String("code"));
180187
QCOMPARE(query.queryItemValue(QStringLiteral("client_id")), _expectedClientId);
@@ -207,8 +214,7 @@ class OAuthTestCase : public QObject
207214
OC_ASSERT(state == BrowserOpened);
208215
state = TokenAsked;
209216
OC_ASSERT(op == QNetworkAccessManager::PostOperation);
210-
OC_ASSERT(req.url().toString().startsWith(sOAuthTestServer.toString()));
211-
OC_ASSERT(req.url().path() == sOAuthTestServer.path() + QStringLiteral("/index.php/apps/oauth2/api/v1/token"));
217+
212218
auto payload = std::make_unique<QBuffer>();
213219
payload->setData(tokenReplyPayload());
214220
return new FakePostReply(op, req, std::move(payload), fakeAm);
@@ -219,8 +225,8 @@ class OAuthTestCase : public QObject
219225
OC_ASSERT(state == StartState);
220226
state = StatusPhpState;
221227
OC_ASSERT(op == QNetworkAccessManager::GetOperation);
222-
OC_ASSERT(req.url().toString().startsWith(sOAuthTestServer.toString()));
223-
OC_ASSERT(req.url().path() == sOAuthTestServer.path() + QStringLiteral("/status.php"));
228+
OC_ASSERT(req.url().toString().startsWith(sOpenIdBaseURL.toString()));
229+
OC_ASSERT(req.url().path() == sOpenIdBaseURL.path() + QStringLiteral("/status.php"));
224230
auto payload = std::make_unique<QBuffer>();
225231
payload->setData(statusPhpPayload());
226232
return new FakePostReply(op, req, std::move(payload), fakeAm);
@@ -231,17 +237,22 @@ class OAuthTestCase : public QObject
231237
OC_ASSERT(state == TokenAsked);
232238
state = UserInfoFetched;
233239
OC_ASSERT(op == QNetworkAccessManager::GetOperation);
234-
OC_ASSERT(req.url().toString().startsWith(sOAuthTestServer.toString()));
235-
OC_ASSERT(req.url().path() == sOAuthTestServer.path() + QStringLiteral("/ocs/v2.php/cloud/user"));
240+
OC_ASSERT(req.url().toString().startsWith(sOpenIdBaseURL.toString()));
241+
OC_ASSERT(req.url().path() == sOpenIdBaseURL.path() + QStringLiteral("/ocs/v2.php/cloud/user"));
236242
OC_ASSERT(req.url().query() == QStringLiteral("format=json"));
237243
auto payload = std::make_unique<QBuffer>();
238244
payload->setData(userInfoPayload());
239245
return new FakePostReply(op, req, std::move(payload), fakeAm);
240246
}
241247

242-
virtual QNetworkReply *wellKnownReply(QNetworkAccessManager::Operation op, const QNetworkRequest &req)
243-
{
244-
return new FakeErrorReply(op, req, fakeAm, 404);
248+
virtual QNetworkReply * wellKnownReply(QNetworkAccessManager::Operation op, const QNetworkRequest & req) {
249+
OC_ASSERT(op == QNetworkAccessManager::GetOperation);
250+
QJsonDocument jsondata(QJsonObject{
251+
{QStringLiteral("authorization_endpoint"), sOpenIdAuthURL.toString()},
252+
{QStringLiteral("token_endpoint"), QStringLiteral("oauthtest://openidserver/token_endpoint")},
253+
{QStringLiteral("token_endpoint_auth_methods_supported"), QJsonArray{QStringLiteral("client_secret_post")}},
254+
});
255+
return new FakePayloadReply(op, req, jsondata.toJson(), fakeAm);
245256
}
246257

247258
virtual QByteArray tokenReplyPayload() const {
@@ -411,37 +422,58 @@ private Q_SLOTS:
411422
localHost = QStringLiteral("127.0.0.1");
412423
}
413424

414-
QNetworkReply * wellKnownReply(QNetworkAccessManager::Operation op, const QNetworkRequest & req) override {
415-
OC_ASSERT(op == QNetworkAccessManager::GetOperation);
416-
QJsonDocument jsondata(QJsonObject{
417-
{QStringLiteral("authorization_endpoint"),
418-
QJsonValue(QStringLiteral("oauthtest://openidserver") + sOAuthTestServer.path() + QStringLiteral("/index.php/apps/oauth2/authorize"))},
419-
{QStringLiteral("token_endpoint"), QStringLiteral("oauthtest://openidserver/token_endpoint")},
420-
{QStringLiteral("token_endpoint_auth_methods_supported"), QJsonArray{QStringLiteral("client_secret_post")}},
421-
});
422-
return new FakePayloadReply(op, req, jsondata.toJson(), fakeAm);
423-
}
424-
425425
void openBrowserHook(const QUrl & url) override {
426-
OC_ASSERT(url.host() == QStringLiteral("openidserver"));
426+
OC_ASSERT(url.host() == QStringLiteral("auth.example.com"));
427427
QUrl url2 = url;
428-
url2.setHost(sOAuthTestServer.host());
428+
url2.setHost(sOpenIdBaseURL.host());
429429
OAuthTestCase::openBrowserHook(url2);
430430
}
431431

432+
/*
432433
QNetworkReply *tokenReply(QNetworkAccessManager::Operation op, const QNetworkRequest &request, QIODevice *device) override
433434
{
434435
OC_ASSERT(browserReply);
435436
OC_ASSERT(request.url().toString().startsWith(QStringLiteral("oauthtest://openidserver/token_endpoint")));
436437
auto req = request;
437438
req.setUrl(QUrl(request.url().toString().replace(QLatin1String("oauthtest://openidserver/token_endpoint"),
438-
sOAuthTestServer.toString() + QStringLiteral("/index.php/apps/oauth2/api/v1/token"))));
439+
sOpenIdBaseURL.toString() + QStringLiteral("/index.php/apps/oauth2/api/v1/token"))));
439440
return OAuthTestCase::tokenReply(op, req, device);
440-
}
441+
}*/
441442
} test;
442443
test.test();
443444
}
444445

446+
void testWellKnown307() {
447+
struct Test : OAuthTestCase {
448+
Test()
449+
{
450+
localHost = QStringLiteral("127.0.0.1");
451+
}
452+
453+
QNetworkReply *wellKnownReply(QNetworkAccessManager::Operation op, const QNetworkRequest &req) override
454+
{
455+
return new FakeErrorReply(op, req, fakeAm, 404);
456+
}
457+
458+
void oauthResult(OAuth::Result result, const QString &token, const QString &refreshToken) override
459+
{
460+
QCOMPARE(result, OAuth::ErrorIdPUnreachable);
461+
QCOMPARE(state, StatusPhpState);
462+
QCOMPARE(token, QStringLiteral(""));
463+
QCOMPARE(refreshToken, QStringLiteral(""));
464+
}
465+
466+
void openBrowserHook(const QUrl & url) override {
467+
OC_ASSERT(url.host() == QStringLiteral("openidserver"));
468+
QUrl url2 = url;
469+
url2.setHost(sOpenIdBaseURL.host());
470+
OAuthTestCase::openBrowserHook(url2);
471+
}
472+
} test;
473+
test.runTest();
474+
QTRY_VERIFY(test.gotAuthOk == false);
475+
QTRY_VERIFY(test.replyToBrowserOk == false);
476+
}
445477

446478
void testTimeout()
447479
{

0 commit comments

Comments
 (0)