malpercio.dev / ezpds
An easy-to-host PDS on the ATProtocol, MacOS. Grandma-approved.
fork atom

fix: address Phase 5 code review feedback

Remove stale #[allow(dead_code)] annotations from items now used in production:
- TokenRequestForm fields: code, redirect_uri, client_id, code_verifier (used in handle_authorization_code)
- TokenResponse struct (used to construct response)
- OAuthTokenError::with_nonce method (used in handle_authorization_code)
- AuthCodeRow struct (used as consume_authorization_code return type)
- consume_authorization_code function (called from oauth_token.rs)
- store_oauth_refresh_token function (called from oauth_token.rs)
- DpopTokenEndpointError enum and validate_dpop_for_token_endpoint function (used in oauth_token.rs)
- issue_nonce, validate_and_consume_nonce, cleanup_expired_nonces functions (called from oauth_token.rs)

Keep #[allow(dead_code)] on genuinely unused items:
- refresh_token field on TokenRequestForm (used in Phase 6)
- MissingHeader variant (matched but never constructed)
- code_challenge_method field (stored but never read)

Minor fixes:
- Replace inline account insertion with insert_test_account helper in consume_authorization_code_returns_none_for_expired_code test
- Add RFC 7638 comment to dpop_thumbprint test helper noting lexicographic key order requirement
- Replace Arc wrapper with .clone() in consumed_code_returns_invalid_grant test
- Add AC comment annotations to Phase 4 tests (AC5.1, AC5.2, AC5.3, AC5.4) for consistency

malpercio.dev c163c214 ec27e84a

+12 -26
+1 -5
crates/relay/src/auth/mod.rs
··· 228 228 /// 229 229 /// Returns a 22-character base64url string (16 random bytes). The nonce is 230 230 /// inserted into the store with an expiry of `Instant::now() + 5 minutes`. 231 - #[allow(dead_code)] 232 231 pub(crate) async fn issue_nonce(store: &DpopNonceStore) -> String { 233 232 let mut bytes = [0u8; 16]; 234 233 OsRng.fill_bytes(&mut bytes); ··· 243 242 /// Returns `true` if the nonce is present in the store and has not expired. 244 243 /// Removes the nonce unconditionally (whether valid or expired) to prevent reuse. 245 244 /// Returns `false` for unknown nonces. 246 - #[allow(dead_code)] 247 245 pub(crate) async fn validate_and_consume_nonce(store: &DpopNonceStore, nonce: &str) -> bool { 248 246 let mut map = store.lock().await; 249 247 match map.remove(nonce) { ··· 256 254 /// 257 255 /// Call this on every token request to prevent unbounded memory growth. 258 256 /// Under normal relay load (low request volume) this is sufficient without a background task. 259 - #[allow(dead_code)] 260 257 pub(crate) async fn cleanup_expired_nonces(store: &DpopNonceStore) { 261 258 let now = std::time::Instant::now(); 262 259 store.lock().await.retain(|_, expiry| *expiry > now); ··· 366 363 /// Error from DPoP validation at the token endpoint. 367 364 /// 368 365 /// Converted to `OAuthTokenError` by the handler in `routes/oauth_token.rs`. 369 - #[allow(dead_code)] 370 366 pub(crate) enum DpopTokenEndpointError { 371 367 /// `DPoP:` header is absent. 368 + #[allow(dead_code)] 372 369 MissingHeader, 373 370 /// DPoP proof is syntactically or semantically invalid. 374 371 InvalidProof(&'static str), ··· 385 382 /// 386 383 /// `htm` must be `"POST"`. `htu` must be the token endpoint URL (e.g. 387 384 /// `"https://relay.example.com/oauth/token"`). 388 - #[allow(dead_code)] 389 385 pub(crate) async fn validate_dpop_for_token_endpoint( 390 386 dpop_token: &str, 391 387 htm: &str,
+2 -11
crates/relay/src/db/oauth.rs
··· 164 164 } 165 165 166 166 /// A row read from `oauth_authorization_codes` during code exchange. 167 - #[allow(dead_code)] 168 167 pub struct AuthCodeRow { 169 168 pub client_id: String, 170 169 pub did: String, 171 170 pub code_challenge: String, 171 + #[allow(dead_code)] 172 172 pub code_challenge_method: String, 173 173 pub redirect_uri: String, 174 174 pub scope: String, ··· 181 181 /// 182 182 /// The code column stores the SHA-256 hex hash of the raw code bytes. Callers must 183 183 /// hash the presented code before calling this function (use `routes::token::sha256_hex`). 184 - #[allow(dead_code)] 185 184 pub async fn consume_authorization_code( 186 185 pool: &SqlitePool, 187 186 code_hash: &str, ··· 227 226 /// `scope` is always `'com.atproto.refresh'` for OAuth refresh tokens. 228 227 /// `jkt` is the DPoP key thumbprint binding this token to the client's keypair. 229 228 /// Expires 24 hours after insertion. 230 - #[allow(dead_code)] 231 229 pub async fn store_oauth_refresh_token( 232 230 pool: &SqlitePool, 233 231 token_hash: &str, ··· 494 492 .await 495 493 .unwrap(); 496 494 497 - sqlx::query( 498 - "INSERT INTO accounts (did, email, password_hash, created_at, updated_at) \ 499 - VALUES ('did:plc:testaccount000000000000', 'test@example.com', NULL, \ 500 - datetime('now'), datetime('now'))", 501 - ) 502 - .execute(&pool) 503 - .await 504 - .unwrap(); 495 + insert_test_account(&pool).await; 505 496 506 497 // Insert an already-expired auth code directly (bypassing store_authorization_code's +60s default). 507 498 sqlx::query(
+9 -10
crates/relay/src/routes/oauth_token.rs
··· 34 34 pub struct TokenRequestForm { 35 35 pub grant_type: Option<String>, 36 36 // authorization_code grant 37 - #[allow(dead_code)] 38 37 pub code: Option<String>, 39 - #[allow(dead_code)] 40 38 pub redirect_uri: Option<String>, 41 - #[allow(dead_code)] 42 39 pub client_id: Option<String>, 43 - #[allow(dead_code)] 44 40 pub code_verifier: Option<String>, 45 41 // refresh_token grant 46 42 #[allow(dead_code)] ··· 48 44 } 49 45 50 46 /// Successful token endpoint response body (RFC 6749 §5.1). 51 - #[allow(dead_code)] 52 47 #[derive(Debug, Serialize)] 53 48 pub struct TokenResponse { 54 49 pub access_token: String, ··· 79 74 } 80 75 } 81 76 82 - #[allow(dead_code)] 83 77 pub fn with_nonce(error: &'static str, error_description: &'static str, nonce: String) -> Self { 84 78 Self { 85 79 error, ··· 396 390 397 391 fn dpop_thumbprint(key: &SigningKey) -> String { 398 392 let jwk = dpop_key_to_jwk(key); 393 + // RFC 7638 requires keys to be in lexicographic order (crv, kty, x, y for EC keys). 394 + // Do NOT reorder these keys, or the thumbprint will differ silently. 399 395 let canonical = serde_json::to_string(&serde_json::json!({ 400 396 "crv": jwk["crv"], 401 397 "kty": jwk["kty"], ··· 489 485 490 486 #[tokio::test] 491 487 async fn unknown_grant_type_returns_400_unsupported() { 488 + // AC5.2 492 489 let resp = app(test_state().await) 493 490 .oneshot(post_token("grant_type=client_credentials")) 494 491 .await ··· 500 497 501 498 #[tokio::test] 502 499 async fn missing_grant_type_returns_400_invalid_request() { 500 + // AC5.3 503 501 let resp = app(test_state().await) 504 502 .oneshot(post_token("code=abc123")) 505 503 .await ··· 511 509 512 510 #[tokio::test] 513 511 async fn error_response_content_type_is_json() { 512 + // AC5.4 514 513 let resp = app(test_state().await) 515 514 .oneshot(post_token("grant_type=bad")) 516 515 .await ··· 526 525 527 526 #[tokio::test] 528 527 async fn error_response_has_error_and_error_description_fields() { 528 + // AC5.1 529 529 let resp = app(test_state().await) 530 530 .oneshot(post_token("grant_type=bad")) 531 531 .await ··· 537 537 538 538 #[tokio::test] 539 539 async fn get_token_endpoint_returns_405() { 540 + // Method routing (no AC) 540 541 let resp = app(test_state().await) 541 542 .oneshot( 542 543 Request::builder() ··· 870 871 Some(&nonce1), 871 872 now_secs(), 872 873 ); 873 - let state_arc = std::sync::Arc::new(state); 874 874 875 875 // Build the app twice using different oneshot calls on the same state. 876 876 // Clone state so the DB pool is shared across both calls. 877 - let state1 = (*state_arc).clone(); 878 - let resp1 = app(state1) 877 + let resp1 = app(state.clone()) 879 878 .oneshot(post_token_with_dpop(&body, &dpop1)) 880 879 .await 881 880 .unwrap(); 882 881 assert_eq!(resp1.status(), StatusCode::OK, "first use must succeed"); 883 882 884 883 // Second use — code was consumed. 885 - let state2 = (*state_arc).clone(); 884 + let state2 = state.clone(); 886 885 let nonce2 = issue_nonce(&state2.dpop_nonces).await; 887 886 let dpop2 = make_dpop_proof( 888 887 &key,