Skip to content

Commit 03048de

Browse files
rcdaileyguruz
rcdailey
authored and
guruz
committed
fix: use non-nullable empty string for clientAuth in public PKCE clients
Address PR review feedback from @zerox80 to use empty string instead of nullable String? for clientAuth parameter. - Change clientAuth from String? to String across domain/data layers - Use empty string "" for public clients instead of null - Simplify header check to isNotEmpty() instead of null-safe chain - Add unit test for public PKCE client token exchange scenario
1 parent c14bf94 commit 03048de

7 files changed

Lines changed: 85 additions & 31 deletions

File tree

‎opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/AccountAuthenticator.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ private String refreshToken(
345345

346346
String clientIdForRequest = null;
347347
String clientSecretForRequest = null;
348-
String clientAuth = null;
348+
String clientAuth;
349349

350350
if (clientId == null) {
351351
Timber.d("Client Id not stored. Let's use the hardcoded one");
@@ -366,12 +366,12 @@ private String refreshToken(
366366
// RFC 7636: Public clients (token_endpoint_auth_method: none) must not send Authorization header
367367
if (oidcServerConfigurationUseCaseResult.getDataOrNull() != null &&
368368
oidcServerConfigurationUseCaseResult.getDataOrNull().isTokenEndpointAuthMethodNone()) {
369-
clientAuth = null;
369+
clientAuth = "";
370370
clientIdForRequest = clientId;
371371
} else if (oidcServerConfigurationUseCaseResult.getDataOrNull() != null &&
372372
oidcServerConfigurationUseCaseResult.getDataOrNull().isTokenEndpointAuthMethodSupportedClientSecretPost()) {
373373
// For client_secret_post, credentials go in body, not Authorization header
374-
clientAuth = null;
374+
clientAuth = "";
375375
clientIdForRequest = clientId;
376376
clientSecretForRequest = clientSecret;
377377
} else {

‎opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/LoginActivity.kt‎

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -669,38 +669,37 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
669669
// Use oidc discovery one, or build an oauth endpoint using serverBaseUrl + Setup string.
670670
val tokenEndPoint: String
671671

672-
var clientId: String? = null
673-
var clientSecret: String? = null
674-
var clientAuth: String? = null
672+
// Determine clientId and clientSecret
673+
val (clientId, clientSecret) = if (clientRegistrationInfo?.clientId != null && clientRegistrationInfo.clientSecret != null) {
674+
Pair(clientRegistrationInfo.clientId, clientRegistrationInfo.clientSecret as String)
675+
} else {
676+
Pair(getString(R.string.oauth2_client_id), getString(R.string.oauth2_client_secret))
677+
}
678+
679+
var clientIdForRequest: String? = null
680+
var clientSecretForRequest: String? = null
681+
var clientAuth: String
675682

676683
val serverInfo = authenticationViewModel.serverInfo.value?.peekContent()?.getStoredData()
677684
if (serverInfo is ServerInfo.OIDCServer) {
678685
tokenEndPoint = serverInfo.oidcServerConfiguration.tokenEndpoint
679686

680687
// RFC 7636: Public clients (token_endpoint_auth_method: none) must not send Authorization header
681688
if (serverInfo.oidcServerConfiguration.isTokenEndpointAuthMethodNone()) {
682-
clientAuth = null
683-
clientId = clientRegistrationInfo?.clientId ?: contextProvider.getString(R.string.oauth2_client_id)
689+
clientAuth = ""
690+
clientIdForRequest = clientId
684691
} else if (serverInfo.oidcServerConfiguration.isTokenEndpointAuthMethodSupportedClientSecretPost()) {
685692
// For client_secret_post, credentials go in body, not Authorization header
686-
clientAuth = null
687-
clientId = clientRegistrationInfo?.clientId ?: contextProvider.getString(R.string.oauth2_client_id)
688-
clientSecret = clientRegistrationInfo?.clientSecret ?: contextProvider.getString(R.string.oauth2_client_secret)
693+
clientAuth = ""
694+
clientIdForRequest = clientId
695+
clientSecretForRequest = clientSecret
689696
} else {
690697
// For other methods (e.g., client_secret_basic), use Basic auth header
691-
clientAuth = if (clientRegistrationInfo?.clientId != null && clientRegistrationInfo.clientSecret != null) {
692-
OAuthUtils.getClientAuth(clientRegistrationInfo.clientSecret as String, clientRegistrationInfo.clientId)
693-
} else {
694-
OAuthUtils.getClientAuth(getString(R.string.oauth2_client_secret), getString(R.string.oauth2_client_id))
695-
}
698+
clientAuth = OAuthUtils.getClientAuth(clientSecret, clientId)
696699
}
697700
} else {
698701
tokenEndPoint = "$serverBaseUrl${File.separator}${contextProvider.getString(R.string.oauth2_url_endpoint_access)}"
699-
clientAuth = if (clientRegistrationInfo?.clientId != null && clientRegistrationInfo.clientSecret != null) {
700-
OAuthUtils.getClientAuth(clientRegistrationInfo.clientSecret as String, clientRegistrationInfo.clientId)
701-
} else {
702-
OAuthUtils.getClientAuth(getString(R.string.oauth2_client_secret), getString(R.string.oauth2_client_id))
703-
}
702+
clientAuth = OAuthUtils.getClientAuth(clientSecret, clientId)
704703
}
705704

706705
val scope = resources.getString(R.string.oauth2_openid_scope)
@@ -710,8 +709,8 @@ class LoginActivity : AppCompatActivity(), SslUntrustedCertDialog.OnSslUntrusted
710709
tokenEndpoint = tokenEndPoint,
711710
clientAuth = clientAuth,
712711
scope = scope,
713-
clientId = clientId,
714-
clientSecret = clientSecret,
712+
clientId = clientIdForRequest,
713+
clientSecret = clientSecretForRequest,
715714
authorizationCode = authorizationCode,
716715
redirectUri = OAuthUtils.buildRedirectUri(applicationContext).toString(),
717716
codeVerifier = authenticationViewModel.codeVerifier

‎opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/oauth/TokenRequestRemoteOperation.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,8 @@ class TokenRequestRemoteOperation(
5454

5555
val postMethod = PostMethod(URL(tokenRequestParams.tokenEndpoint), requestBody)
5656

57-
tokenRequestParams.clientAuth?.takeIf { it.isNotEmpty() }?.let {
58-
postMethod.addRequestHeader(AUTHORIZATION_HEADER, it)
57+
if (tokenRequestParams.clientAuth.isNotEmpty()) {
58+
postMethod.addRequestHeader(AUTHORIZATION_HEADER, tokenRequestParams.clientAuth)
5959
}
6060

6161
val status = client.executeHttpMethod(postMethod)

‎opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/oauth/params/TokenRequestParams.kt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import okhttp3.RequestBody
3030

3131
sealed class TokenRequestParams(
3232
val tokenEndpoint: String,
33-
val clientAuth: String?,
33+
val clientAuth: String,
3434
val grantType: String,
3535
val scope: String,
3636
val clientId: String?,
@@ -40,7 +40,7 @@ sealed class TokenRequestParams(
4040

4141
class Authorization(
4242
tokenEndpoint: String,
43-
clientAuth: String?,
43+
clientAuth: String,
4444
grantType: String,
4545
scope: String,
4646
clientId: String?,
@@ -65,7 +65,7 @@ sealed class TokenRequestParams(
6565

6666
class RefreshToken(
6767
tokenEndpoint: String,
68-
clientAuth: String?,
68+
clientAuth: String,
6969
grantType: String,
7070
scope: String,
7171
clientId: String?,

‎opencloudData/src/test/java/eu/opencloud/android/data/oauth/datasources/implementation/OCRemoteOAuthDataSourceTest.kt‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,15 @@ import eu.opencloud.android.testutil.oauth.OC_CLIENT_REGISTRATION
3434
import eu.opencloud.android.testutil.oauth.OC_CLIENT_REGISTRATION_REQUEST
3535
import eu.opencloud.android.testutil.oauth.OC_OIDC_SERVER_CONFIGURATION
3636
import eu.opencloud.android.testutil.oauth.OC_TOKEN_REQUEST_ACCESS
37+
import eu.opencloud.android.testutil.oauth.OC_TOKEN_REQUEST_ACCESS_PUBLIC_CLIENT
3738
import eu.opencloud.android.testutil.oauth.OC_TOKEN_RESPONSE
3839
import eu.opencloud.android.utils.createRemoteOperationResultMock
3940
import io.mockk.every
4041
import io.mockk.mockk
4142
import io.mockk.verify
4243
import org.junit.Assert.assertEquals
4344
import org.junit.Assert.assertNotNull
45+
import org.junit.Assert.assertTrue
4446
import org.junit.Before
4547
import org.junit.Test
4648

@@ -102,6 +104,37 @@ class OCRemoteOAuthDataSourceTest {
102104
}
103105
}
104106

107+
/**
108+
* Test for public PKCE clients (RFC 7636).
109+
* Public clients MUST NOT send Authorization header during token exchange.
110+
* This test verifies that token requests work correctly with empty clientAuth.
111+
*/
112+
@Test
113+
fun `performTokenRequest with public PKCE client returns a TokenResponse`() {
114+
val tokenResponseResult: RemoteOperationResult<TokenResponse> =
115+
createRemoteOperationResultMock(data = OC_REMOTE_TOKEN_RESPONSE, isSuccess = true)
116+
117+
every {
118+
oidcService.performTokenRequest(ocClientMocked, any())
119+
} returns tokenResponseResult
120+
121+
// Verify the fixture has empty clientAuth for public clients
122+
assertTrue(
123+
"clientAuth should be empty for public clients",
124+
OC_TOKEN_REQUEST_ACCESS_PUBLIC_CLIENT.clientAuth.isEmpty()
125+
)
126+
127+
val tokenResponse = remoteOAuthDataSource.performTokenRequest(OC_TOKEN_REQUEST_ACCESS_PUBLIC_CLIENT)
128+
129+
assertNotNull(tokenResponse)
130+
assertEquals(OC_TOKEN_RESPONSE, tokenResponse)
131+
132+
verify(exactly = 1) {
133+
clientManager.getClientForAnonymousCredentials(OC_SECURE_BASE_URL, any())
134+
oidcService.performTokenRequest(ocClientMocked, any())
135+
}
136+
}
137+
105138
@Test
106139
fun `registerClient returns a ClientRegistrationInfo`() {
107140
val clientRegistrationResponse: RemoteOperationResult<ClientRegistrationResponse> =

‎opencloudDomain/src/main/java/eu/opencloud/android/domain/authentication/oauth/model/TokenRequest.kt‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ package eu.opencloud.android.domain.authentication.oauth.model
2424
sealed class TokenRequest(
2525
val baseUrl: String,
2626
val tokenEndpoint: String,
27-
val clientAuth: String?,
27+
val clientAuth: String,
2828
val grantType: String,
2929
val scope: String,
3030
val clientId: String?,
@@ -33,7 +33,7 @@ sealed class TokenRequest(
3333
class AccessToken(
3434
baseUrl: String,
3535
tokenEndpoint: String,
36-
clientAuth: String?,
36+
clientAuth: String,
3737
scope: String,
3838
clientId: String? = null,
3939
clientSecret: String? = null,
@@ -45,7 +45,7 @@ sealed class TokenRequest(
4545
class RefreshToken(
4646
baseUrl: String,
4747
tokenEndpoint: String,
48-
clientAuth: String?,
48+
clientAuth: String,
4949
scope: String,
5050
clientId: String? = null,
5151
clientSecret: String? = null,

‎opencloudTestUtil/src/main/java/eu/opencloud/android/testutil/oauth/TokenRequest.kt‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,3 +43,25 @@ val OC_TOKEN_REQUEST_ACCESS = TokenRequest.AccessToken(
4343
redirectUri = OC_REDIRECT_URI,
4444
codeVerifier = "A high-entropy cryptographic random STRING using the unreserved characters"
4545
)
46+
47+
/**
48+
* Test fixtures for public PKCE clients (RFC 7636).
49+
* Public clients MUST NOT send Authorization header during token exchange.
50+
*/
51+
val OC_TOKEN_REQUEST_REFRESH_PUBLIC_CLIENT = TokenRequest.RefreshToken(
52+
baseUrl = OC_SECURE_BASE_URL,
53+
tokenEndpoint = OC_TOKEN_ENDPOINT,
54+
clientAuth = "", // Empty for public clients per RFC 7636
55+
scope = OC_SCOPE,
56+
refreshToken = OC_REFRESH_TOKEN
57+
)
58+
59+
val OC_TOKEN_REQUEST_ACCESS_PUBLIC_CLIENT = TokenRequest.AccessToken(
60+
baseUrl = OC_SECURE_BASE_URL,
61+
tokenEndpoint = OC_TOKEN_ENDPOINT,
62+
clientAuth = "", // Empty for public clients per RFC 7636
63+
scope = OC_SCOPE,
64+
authorizationCode = "4uth0r1z4t10nC0d3",
65+
redirectUri = OC_REDIRECT_URI,
66+
codeVerifier = "A high-entropy cryptographic random STRING using the unreserved characters"
67+
)

0 commit comments

Comments
 (0)