[BUG] OAuth2 client registration and subsequent requests use inconsistent authentication methods when OAuth2 authorization server supports client_secret_post

[yihanwu1024]

Introduction

There are two OAuth2 client-to-authorization-server authentication methods that do the same thing: client_secret_basic and client_secret_post. The former uses the Authorization: Basic header, while the latter puts the data in the body.

Actual behaviour

OAuth2 client registration and subsequent requests use inconsistent authentication methods, when the OAuth2 authorization server supports client_secret_post.

1. Without checking any server capabilities, the app registers an OAuth2 client that uses client_secret_basic.
   

   android/owncloudTestUtil/src/main/java/com/owncloud/android/testutil/oauth/ClientRegistrationRequest.kt

   Line 28 in 36ef69e

   tokenEndpointAuthMethod = "client_secret_basic",

2. The app then acts as this OAuth2 client, but instead speaks client_secret_post after checking that the server supports it. Otherwise it speaks client_secret_basic. If a server supports both, then client_secret_post is used. Keycloak supports both, but now requires that, if a client registered with client_secret_basic, it must keep using client_secret_basic, even if the authentication details are the same. As a result, adding an account fails 100% of the time on the latest Keycloak.
   

   android/owncloudApp/src/main/java/com/owncloud/android/presentation/authentication/LoginActivity.kt

   Line 670 in 36ef69e

   if (serverInfo.oidcServerConfiguration.isTokenEndpointAuthMethodSupportedClientSecretPost()) {

   android/owncloudApp/src/main/java/com/owncloud/android/presentation/authentication/AccountAuthenticator.java

   Line 378 in 36ef69e

   oidcServerConfigurationUseCaseResult.getDataOrNull().isTokenEndpointAuthMethodSupportedClientSecretPost()) {

Expected behaviour

The OAuth2 client authentication methods are used consistently.

Proposed fix: move server capability check to the client registration step, and persist client_secret_basic or client_secret_post.
There is no other good way to ensure the required consistency.

Steps to reproduce

Use an installation with the latest Keycloak (I can give you an account)

Environment data

Android version: 17

Device model: Pixel 10

Stock or customized system: stock

ownCloud app version: 4.8.1

ownCloud server version: 8.0.2 (irrelevant)

Logs

Web server error log

Keycloak records a client authentication failure event

ownCloud log (data/owncloud.log)

No significant request has hit ownCloud yet

[jesmrec]

Thanks for reporting @yihanwu1024!!. We will review your proposal and come back to here.

[jesmrec]

@yihanwu1024 i think your scratch point is not correct:

Without checking any server capabilities, the app registers an OAuth2 client that uses client_secret_basic.

android/owncloudTestUtil/src/main/java/com/owncloud/android/testutil/oauth/ClientRegistrationRequest.kt

Line 28 in 36ef69e

tokenEndpointAuthMethod = "client_secret_basic",

You are pointing to a testUtil class, just used for the unit tests, not for the production artifact.

client_secret_post is also involved, coming from OIDC discovery endpoint:

android/owncloudDomain/src/main/java/com/owncloud/android/domain/authentication/oauth/model/OIDCServerConfiguration.kt

Line 39 in 36ef69e

tokenEndpointAuthMethodsSupported?.any { it == "client_secret_post" } ?: false

We worked on a similar issue several months ago, our criteria is taking client_secret_post as prior if the server supports client_server_post and client_server_basic. If you are interested about it, here you have the issue discussion: #4575 (comment) to take a look 📃

For sure, if you have further objections, let us know. Always open to constructive discussions 😉

CC @joragua

[yihanwu1024]

Sorry @jesmrec, I posted the wrong source code! There are only 2 results when searching for client_secret_basic and I gave the wrong one. The other one should correctly point out the issue:

android/owncloudDomain/src/main/java/com/owncloud/android/domain/authentication/oauth/model/ClientRegistrationRequest.kt

Line 36 in 3b0b27f

private const val CLIENT_REGISTRATION_AUTH_METHOD = "client_secret_basic"

I also collected more concrete evidence:
In Keycloak, the newly registered client has Allowed authentication method: client_secret_basic, not Any or client_secret_post.
Keycloak also logged HTTP POST /realms/master/protocol/openid-connect/token, then client authenticator FAILED: client-secret.

[jesmrec]

thanks for the clarification @yihanwu1024!!

Take a look to my answer here: #4575 (comment)

If the backend exposes in the /openid-configuration endpoint that both client_secret_post and client-secret are supported, the client has no way (afaik) to distinguish in which cases (or clients) should be applied every of the exposed methods. Therefore, we had to set a priority order.

Anyway, if i miss something, open to listen to more ideas!

[yihanwu1024]

@jesmrec Do you know if the mechanism you described also exists in the client registration process?
The selection of one authentication method or the other can be arbitrary, but in case of Keycloak, it must be consistent.

[jesmrec]

Need to check.