Add support for multiple clients#201
Conversation
d8529d8 to
90b43df
Compare
As part of Project Prism we are expanding the apps and will need them all to be able to add backup methods. This updates the backup service to respect the client name when determining the appropriate bundle identifiers for verification. I've added this header to all of the routes for completeness, but add_factor is the primary one we're interested in right now.
90b43df to
375b6b0
Compare
There was a problem hiding this comment.
Pull request overview
Updates the backup service’s OIDC token verification to support multiple client applications by selecting the expected Google/Apple audience based on an incoming client-name request header, and threads this through route handlers into the auth/verification layer.
Changes:
- Add
client-name/client-versionheader constants and plumbclient-namethrough relevant routes intoAuthHandlerandOidcTokenVerifier. - Select Google/Apple OIDC client IDs dynamically based on
client-name, with unit tests covering the mapping logic. - Update
cargo-denyadvisory ignores and bumprustls-webpkiinCargo.lock(alongside existing0.101.7usage).
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/environment.rs | Adds client-name-based selection for Google/Apple OIDC client IDs and unit tests. |
| src/routes/verify_factor.rs | Extracts client-name/client-version via constants and forwards client-name into auth verification. |
| src/routes/sync_backup.rs | Extracts and forwards client-name into auth verification. |
| src/routes/retrieve_metadata.rs | Extracts and forwards client-name into auth verification. |
| src/routes/retrieve_from_challenge.rs | Extracts client-name/client-version via constants and forwards client-name into auth verification. |
| src/routes/delete_factor.rs | Extracts and forwards client-name into auth verification. |
| src/routes/delete_backup.rs | Extracts and forwards client-name into auth verification. |
| src/routes/create_backup.rs | Extracts and forwards client-name into factor registration validation. |
| src/routes/add_sync_factor.rs | Extracts and forwards client-name into factor registration validation. |
| src/routes/add_factor.rs | Extracts and forwards client-name into new-factor registration validation. |
| src/oidc_token_verifier.rs | Accepts client-name to choose expected audience during OIDC verification; refactors verifier creation. |
| src/lib.rs | Exposes new headers module. |
| src/headers.rs | Defines header-name constants for reuse across routes. |
| src/auth.rs | Threads client_name through auth verification and OIDC validation paths. |
| deny.toml | Adds additional ignored RUSTSEC advisories for rustls-webpki via upstream deps. |
| Cargo.lock | Updates rustls-webpki (0.103.10 → 0.103.13) while retaining 0.101.7 via rustls 0.21.x. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Google backup is okay. Just need android-id in here so apple backup can be implemented on android devices down the road
| (Self::Staging, Some("ios-id")) => "org.world.staging.id", | ||
| (Self::Staging, Some("android-id")) => "org.world.id.staging", |
There was a problem hiding this comment.
is it expected for the id to before and after staging?
There was a problem hiding this comment.
Yeah this was one oddity where each team used a different bundle id format :(
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (1)
src/routes/delete_factor.rs:67
client_nameis borrowed fromheadersand then passed into an async call (auth_handler.verify(...).await). Borrowing fromHeaderMapacross await commonly fails to compile (self-referential future). Copy the header to an ownedStringfirst and passas_deref()/Option<String>onward.
let client_name = headers.get(&CLIENT_NAME).and_then(|v| v.to_str().ok());
// Step 1: Extract the factor IDs from the request
let factor_id = request.factor_id.clone();
let encryption_key = request.encryption_key.clone();
// Step 1.1 Validate there is no encryption key if deleting a `Sync` factor
if request.scope == FactorScope::Sync && encryption_key.is_some() {
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
As part of Project Prism we are expanding the apps and will need them all to be able to add backup methods. This updates the backup service to respect the client name when determining the appropriate bundle identifiers for verification.
I've added this header to all of the routes for completeness, but add_factor is the primary one we're interested in right now.