Oauth2 Token request does basic and post auth

[TuxCoder]

Pre-submission Checks

• [X] I checked for similar issues, but could not find any. I also checked the closed issues. I could not contribute additional information to any existing issue.
• [X] I will take the time to fill in all the required fields. I know that the bug report may be dismissed otherwise due to lack of information.

Describe the bug

client does send oauth client_id and client_secret in post.

Expected behavior

client doesnot send oauth client_id and client_secret in post.

Steps to reproduce the issue

login to an oauth2 server. enable dumping from POST <token_endpoint> request after getting consent.

Screenshots

No response

Logs

No response

Client version number

tested with: ownCloud ownCloud 2.10.1 Apr 5 2022 11:20:16 https://github.com/owncloud/client/commit/82eefdafa2ec7a1927df87c850d9e6f1e9dac6af Libraries Qt 5.15.3, OpenSSL 1.1.1q 5 Jul 2022 Using virtual files plugin: suffix debian-5.18.2

tested fix in github master

Desktop environment (Linux only)

debian testing

Client package version and origin (Linux only)

No response

Installation path (Windows only)

No response

Server information

ocis 2.0.0beta5 - nixos as host (ask if you need more info)

• custom openid connect server (ory hydra)

Additional context

ory hydra complains about auth via http-basic and via post data. This error totally makes sense as ether one should be use. ref: https://www.rfc-editor.org/rfc/rfc6749#section-2.3.1

The client request in the dynamic client registration "token_endpoint_auth_method"="client_secret_basic" i assume, that should be the way to auth.

this patch remove the unnecessary additional auth in the post-body:

--- a/src/libsync/creds/oauth.cpp
+++ b/src/libsync/creds/oauth.cpp
@@ -458,9 +458,7 @@ QNetworkReply *OAuth::postTokenRequest(const QList<QPair<QString, QString>> &que
     req.setAttribute(HttpCredentials::DontAddCredentialsAttribute, true);
 
     QUrlQuery arguments;
-    arguments.setQueryItems(QList<QPair<QString, QString>> { { QStringLiteral("client_id"), _clientId },
-                                { QStringLiteral("client_secret"), _clientSecret },
-                                { QStringLiteral("scope"), Theme::instance()->openIdConnectScopes() } }
+    arguments.setQueryItems(QList<QPair<QString, QString>> { { QStringLiteral("scope"), Theme::instance()->openIdConnectScopes() } }
         << queryItems);
     req.setUrl(requestTokenUrl);
     return _networkAccessManager->post(req, arguments.toString(QUrl::FullyEncoded).toUtf8());

[michaelstingl]

@TuxCoder thanks a lot for your feedback! We do this for older versions of the oC10 oauth2 app. Our assumption was, sending some additional data won't hurt.

@DeepDiver1975 @TheOneRing what are your thoughts?

/cc involving backend and the other clients… @wkloucek @felix-schwarz

[TheOneRing]

Sounds to me like hydra is a bit to strict here. We try to support as many idp's as possible and rely on them to not be too picky. The spec does not say that you must use either basic auth or post data so I think we are generally following specs here.

[TuxCoder]

@TheOneRing

Alternatively, the authorization server MAY support including the client credentials in the request-body using the following parameters:

Ok this can be interpreted ether way

Including the client credentials in the request-body using the two parameters is NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme (or other password-based HTTP authentication schemes). The parameters can only be transmitted in the request-body and MUST NOT be included in the request URI.

and its only NOT RECOMMENDED, but yeah

Also has every oauth client the setting "token_endpoint_auth_method" what has as value 1 and exactly 1 string what can be ether: "client_secret_basic" - default "client_secret_post" "none" ans some special ones: client_secret_jwt, ..

ref: https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

Hydra is probably strict, but also correct, what should it do if half of the information is there or there, it would be IMHO stupid to accept such a request, or try to process it. what information should be preferred, basic auth or post info, what if one part is missing? what if different? fallback combine? that would be a hack in my opinion.

Please remove the unneeded credentials, Android client also does not have that and it works!

[github-actions[bot]]

This issue was marked stale because it has been open for 30 days with no activity. Remove the stale label or comment or this will be closed in 7 days.

[github-actions[bot]]

The issue was marked as stale for 7 days and closed automatically.