Skip to content

feat: Add OAuth 2.0 support with PKCE flow - #6940

Open
Kota-Jagadeesh wants to merge 3 commits into
commons-app:mainfrom
Kota-Jagadeesh:feat/OAuth
Open

feat: Add OAuth 2.0 support with PKCE flow#6940
Kota-Jagadeesh wants to merge 3 commits into
commons-app:mainfrom
Kota-Jagadeesh:feat/OAuth

Conversation

@Kota-Jagadeesh

Copy link
Copy Markdown
Collaborator

Description (required)

Fixes #6885

  • implemented OAuth 2.0 authorization code grant flow with PKCE to enable thesecure web-based login via chrome custom tabs.
  • configured the custom scheme deep-linking (fr.free.nrw.commons://oauth-callback) in AndroidManifest.xml to intercept post-auth redirects.
  • and addedthe token exchange (/w/rest.php/oauth2/access_token) and profile fetching (/w/rest.php/oauth2/resource/profile) in LoginInterface, securely persisting access_token and refresh_token in SessionManager via Android the accountmanager.
  • updated the NetworkingModule to automatically attach Authorization: Bearer <access_token> headers to the outgoing API requests.
  • also added a "Login with OAuth" option to activity_login.xml

Tests performed (required)

Tested ProdDebug on Redmi Note 13 PRO with API level 36. Verified web authentication flow, deep-link callback execution, token persistence, and successful account contributions fetching.

Screenshots (for UI changes only)

Screenrecorder-2026-08-09-21-31-53-10.mp4
image

@RitikaPahwa4444

Copy link
Copy Markdown
Collaborator

I'm getting this error page on this branch:

screen-20260810-020943.mp4

@Kota-Jagadeesh

Kota-Jagadeesh commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

ahh, @RitikaPahwa4444 that is expected. as the OAuth 2.0 consumer keys aren't committed here(git) , build.gradle.kts falls back to empty strings if they aren't present in local.properties

@Kota-Jagadeesh

Copy link
Copy Markdown
Collaborator Author

I'm getting this error page on this branch:

I'll mail you and @nicolas-raoul my developer client keys in some time, so you can use them in local.properties and test the auth flow directly

@chrisdebian chrisdebian left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for building this out — the PKCE implementation itself is correct: SecureRandom with 32 bytes for the verifier, SHA-256 + base64url for the challenge, S256 as the method, all matching RFC 7636. Found three real issues around it though, all in how the tokens it protects get handled afterward.

1. Shipping a client_secret alongside PKCE defeats the reason to use PKCE at all. grantOAuthToken() sends client_secret = BuildConfig.OAUTH_CLIENT_SECRET, compiled in from local.properties. PKCE exists specifically so a public client — anything that can't keep a secret, which by definition includes a distributed Android APK — doesn't need one (RFC 8252, "OAuth 2.0 for Native Apps," is explicit that native apps must be treated as public clients and must not use client secrets). Whatever value ends up in OAUTH_CLIENT_SECRET is trivially recoverable from any built APK via jadx/dex2jar, so it provides no actual protection — it's PKCE doing the real work here regardless of whether the secret is present. Suggest dropping client_secret entirely and registering the app's OAuth consumer as a public client, matching what PKCE is for.

2. refresh_token is stored more weakly than access_token, and more weakly than this codebase's own existing convention. SessionManager.updateOAuthAccount() puts the access token in accountManager.setAuthToken() (the auth-token slot, correctly access-controlled) but the refresh token in accountManager.setUserData() — the general-purpose metadata bag, not designed for secrets. The refresh token is the longer-lived, more valuable of the two credentials (usable to mint fresh access tokens until revoked), so if anything it deserves more protection, not less. Worth noting the existing password-based login path already treats its one secret correctly, via accountManager.setPassword()/getPassword() — the strongest single-value slot AccountManager offers. The new OAuth path doesn't extend that same care to the refresh token.

3. The bearer token gets attached to every outgoing request on the shared OkHttpClient, not just requests to the host that issued it. The new interceptor in NetworkingModule.provideOkHttpClient() adds Authorization: Bearer <accessToken> unconditionally, with no check on chain.request().url.host. That OkHttpClient singleton is the same instance wired into provideOkHttpJsonApiClient() a few lines below, which serves tools_forge (Toolforge), WIKIDATA_SPARQL_QUERY_URL, and BuildConfig.WIKIMEDIA_CAMPAIGNS_URL — three genuinely different hosts, none of which are the OAuth resource server. As written, a logged-in user's access token would be sent as a header to all of them. Suggest scoping the interceptor to the specific host(s) the token is actually valid for before this merges.

Smaller note, not blocking: the authorization request in startOAuthLogin() has no state parameter. PKCE already covers the main authorization-code-injection threat this would traditionally guard against, so it's a defence-in-depth gap rather than an open hole, but worth adding for completeness per the usual OAuth 2.0 guidance (RFC 6749 §10.12).

Happy to go into more detail on any of these.

@github-actions

Copy link
Copy Markdown

✅ Generated APK variants!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add OAuth Support

3 participants