fix: remove Authorization header for public clients in OAuth2 token exchange

[rcdailey]

Summary

Public OAuth2 clients using PKCE should not send Authorization headers during token exchange per RFC 7636. This was causing token refresh failures with external OIDC providers like Authelia and Zitadel.

Changes

• Made clientAuth nullable in TokenRequest and TokenRequestParams
• Added conditional Authorization header logic in TokenRequestRemoteOperation (only adds header when clientAuth is non-null and non-empty)
• Added isTokenEndpointAuthMethodNone() helper method in OIDCServerConfiguration to detect public clients
• Updated LoginActivity to set clientAuth = null for public clients based on token_endpoint_auth_method from OIDC discovery
• Updated AccountAuthenticator refresh token flow with same logic for consistent behavior

Testing

Tested with Authelia OIDC provider configured with token_endpoint_auth_method: none. Both initial token exchange and token refresh now work correctly without sending the incorrect Authorization header.

RFC Compliance

This change ensures compliance with RFC 7636 (PKCE) and RFC 6749 (OAuth 2.0), which specify that public clients should only send form parameters (client_id, code, code_verifier, redirect_uri, grant_type) during token exchange, with no Authorization header.

Related to #55

[guruz]

@rcdailey I was just looking at this one. Is this superseded by something else?

[zerox80]

@rcdailey Hi, thanks for this PR and sorry for the delayed response, are u still available?

In exchangeAuthorizationCodeForTokens(), there appears to be a variable shadowing issue:

val (clientId, clientSecret) = if (...) { ... } else { ... }
// ...
var clientId: String? = null      // Shadows the outer clientId
var clientSecret: String? = null  // Shadows the outer clientSecret

The inner clientId/clientSecret variables shadow the outer ones from the destructuring. This could lead to unexpected behavior. Consider renaming them to clientIdForRequest and clientSecretForRequest (like in AccountAuthenticator.java).

It would be great to add unit tests for the public PKCE client scenario to ensure the empty/null clientAuth handling works correctly. Something like:

@Test
fun `performTokenRequest with public PKCE client returns a TokenResponse`() {
    // Test with clientAuth = null/empty
    assertTrue(tokenRequest.clientAuth == null || tokenRequest.clientAuth.isEmpty())
    // Verify no Authorization header is sent
}

Alternative Approach
I implemented a similar fix using empty String instead of nullable String? to avoid future API breakages:

zerox80@ea94fb4
zerox80@a520371
Would you mind adjusting your code accordingly so we can merge it? Also, why have you closed the PR?

[rcdailey]

Hey folks, sorry for the delay in getting back with you. I closed the PR due to lack of response for about 2 months; I wasn't sure if my PR was interesting so I decided to close it so I could move on.

If you feel the change has merit, I'd be happy to reopen and make the suggested updates. Please let me know. My time is split between a few other projects but I'll try to prioritize this.

I appreciate the reply.

[zerox80]

Yes we are almost done, would be nice if we could just merge this one

[streaminganger]

the owncloud android app has also landed a hacky fix for this but with an overly verbose explicit flag everywhere

i like this patch as it better reflects RFC 7636

[guruz]

Tested with Authelia OIDC provider configured with token_endpoint_auth_method: none. Both initial token exchange and token refresh now work correctly without sending the incorrect Authorization header.

@kulmann @schweigisito If I merge any of those commits, Which test instance can I use to try it out?

[zerox80]

theres none yet, u can try my branch from my commit i mentioned, mine should work fine aswell

[zerox80]

https://github.com/zerox80/android/tree/feature/fix-oauth-pkce

can make a new PR, or we wait for @rcdailey to implement testing, he said he is kinda busy so what way should we go?

[zerox80]

Hey @rcdailey, thanks for the original work! Since you mentioned being busy, I went ahead and created a new PR based on your approach, adding unit tests and some refactoring.

My apologies regarding my previous comment about variable shadowing – upon closer review, I realized that issue was actually introduced in my own local refactoring of your code, not in your original PR. Your logic was sound! I've incorporated your approach but refactored it slightly (to use destructuring, which is where I introduced and then fixed the shadowing) and added the tests.

I've credited you in the PR description. Let me know if you'd like me to add you as a co-author on the commits as well

[guruz]

Tested with Authelia OIDC provider configured with token_endpoint_auth_method: none. Both initial token exchange and token refresh now work correctly without sending the incorrect Authorization header.

@kulmann @schweigisito If I merge any of those commits, Which test instance can I use to try it out?

FYI I've tested this PR here (by @rcdailey ) with demo.opencloud.eu and at least that still works.

[guruz]

@zerox80 If the original approach by @rcdailey is good, maybe you could keep his commit, but then add your tests/improvements as a second commit on top and we merge that?
That way both your contributions are visible. (and good if you review each others code!)

So basically you fork this branch/PR and add your fix as a second commit (without stashing)

[zerox80]

Can we leave it like that?

[rcdailey]

Thanks for the detailed feedback, @zerox80. I've pushed a follow-up commit (37a1bc250) addressing your suggestions:

• Changed clientAuth from String? to non-nullable String across the domain and data layers
• Replaced null with empty string "" for public PKCE clients
• Simplified the authorization header check to use isNotEmpty() instead of the null-safe chain
• Added unit test for the public PKCE client token exchange scenario
• Renamed shadowed variables in LoginActivity.kt (clientIdForRequest/clientSecretForRequest)

I've also rebased on main to pull in the latest changes.

Regarding the alternative PR/implementation: my earlier comment about other obligations wasn't me stepping away from this PR. I'm committed to seeing it through and responsive to feedback, so I'd prefer we continue iterating here rather than duplicating effort. Happy to incorporate any additional suggestions you have.

[Copilot]

Pull request overview

This pull request fixes token exchange for public OAuth2 clients using PKCE by conditionally omitting the Authorization header when not required. This resolves token refresh failures with OIDC providers like Authelia and Zitadel that use token_endpoint_auth_method: none.

Changes:

• Added conditional Authorization header logic in TokenRequestRemoteOperation to skip the header when clientAuth is empty
• Added isTokenEndpointAuthMethodNone() helper method to detect public clients
• Updated authentication flows in LoginActivity and AccountAuthenticator to set empty clientAuth for public clients based on OIDC discovery metadata
• Added test fixtures and test coverage for public client scenarios

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
opencloudTestUtil/src/main/java/eu/opencloud/android/testutil/oauth/TokenRequest.kt	Added test fixtures for public PKCE clients with empty clientAuth
opencloudDomain/src/main/java/eu/opencloud/android/domain/authentication/oauth/model/OIDCServerConfiguration.kt	Added helper method to detect public clients (token_endpoint_auth_method: none)
opencloudData/src/test/java/eu/opencloud/android/data/oauth/datasources/implementation/OCRemoteOAuthDataSourceTest.kt	Added test for token requests with public PKCE clients
opencloudComLibrary/src/main/java/eu/opencloud/android/lib/resources/oauth/TokenRequestRemoteOperation.kt	Made Authorization header conditional - only added when clientAuth is non-empty
opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/LoginActivity.kt	Updated token exchange to handle public clients, client_secret_post, and client_secret_basic auth methods
opencloudApp/src/main/java/eu/opencloud/android/presentation/authentication/AccountAuthenticator.java	Updated token refresh flow with same auth method handling logic

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[zerox80]

lol