add Qwen OAuth support with browser authentication flow#80
add Qwen OAuth support with browser authentication flow#80daog1 wants to merge 2 commits intojnsahaj:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces Qwen OAuth provider support via a device code flow with PKCE authentication, credential caching, and token refresh mechanisms. New dependencies for cryptography and HTTP operations are added, along with a dedicated OAuth module and provider backend variant to handle Qwen-specific API interactions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as LumenCLI
participant OAuth as QwenOAuthManager
participant DeviceAPI as Device Auth API
participant TokenAPI as Token API
participant Cache as Credential Cache
rect rgb(200, 220, 255)
Note over User,Cache: OAuth Device Code Flow (First Run)
User->>CLI: run command with Qwen provider
CLI->>OAuth: ensure_valid_credentials()
OAuth->>Cache: load_cached_credentials()
Cache-->>OAuth: none (first run)
OAuth->>DeviceAPI: request_device_authorization(PKCE)
DeviceAPI-->>OAuth: device_code, user_code, verification_uri
OAuth-->>CLI: return (user code & URI displayed)
CLI-->>User: "Visit https://... and enter code: XXXX"
User->>DeviceAPI: visit URI, enter user_code, authorize
end
rect rgb(220, 240, 220)
Note over OAuth,TokenAPI: Poll for Token & Cache
loop adaptive backoff until token received
OAuth->>TokenAPI: poll_device_token(device_code)
TokenAPI-->>OAuth: authorization_pending or success
end
TokenAPI-->>OAuth: access_token, refresh_token, resource_url
OAuth->>Cache: save_credentials(QwenCredentials)
OAuth-->>CLI: QwenCredentials
end
rect rgb(240, 220, 220)
Note over CLI,TokenAPI: Token Refresh (Subsequent Runs)
CLI->>OAuth: ensure_valid_credentials()
OAuth->>Cache: load_cached_credentials()
Cache-->>OAuth: cached credentials (expired)
OAuth->>TokenAPI: refresh_access_token(refresh_token)
TokenAPI-->>OAuth: new access_token, updated expiry
OAuth->>Cache: save_credentials(updated)
OAuth-->>CLI: valid QwenCredentials
end
sequenceDiagram
participant Provider as LumenProvider
participant QwenOAuth as ProviderBackend::QwenOAuth
participant OAuth as QwenOAuthManager
participant Qwen as Qwen API
Provider->>QwenOAuth: complete(system, user_prompt)
activate QwenOAuth
QwenOAuth->>OAuth: ensure_valid_credentials()
OAuth-->>QwenOAuth: QwenCredentials (token, resource_url)
QwenOAuth->>QwenOAuth: derive endpoint from resource_url
QwenOAuth->>QwenOAuth: map model to Qwen variant
QwenOAuth->>QwenOAuth: update overrides (token, base_url)
QwenOAuth->>Qwen: POST /compatible-mode/v1/chat/completions
Note over QwenOAuth,Qwen: Authorization: Bearer {access_token}
Qwen-->>QwenOAuth: {choices: [{message: {content: "..."}]}}
QwenOAuth->>QwenOAuth: extract_openai_content()
QwenOAuth-->>Provider: Result<String, ProviderError>
deactivate QwenOAuth
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/command/mod.rs (1)
49-57: Behavioral change confirmed: Draft now uses unstaged diff instead of staged diff.The
stagedparameter changed fromtruetofalsein commit 23b12c2. This creates an inconsistency: the CLI help text states "Generate a commit message for the staged changes", but the implementation generates messages from unstaged changes.Unlike the
Explaincommand (which accepts a--stagedflag to let users control this behavior), theDraftcommand has no such flag and always uses unstaged changes. If the intent is to generate messages for staged changes, the hardcodedfalseshould betrue, or a--stagedflag should be added to matchExplain's behavior.
🧹 Nitpick comments (5)
src/qwen_oauth.rs (3)
130-132: Inconsistent indentation forendpoint_formethod.This method has no indentation despite being inside the
impl QwenOAuthManagerblock. While it compiles correctly, the formatting is inconsistent with the rest of the implementation.🔎 Suggested fix
-pub fn endpoint_for(credentials: &QwenCredentials) -> String { - normalize_endpoint(credentials.resource_url.as_deref()) - } + pub fn endpoint_for(credentials: &QwenCredentials) -> String { + normalize_endpoint(credentials.resource_url.as_deref()) + }
172-180: Poll interval reset behavior may cause unnecessary requests.When the server responds with
authorization_pending(notslow_down), the interval resets to 2 seconds. This could cause rapid polling if the user takes time to authorize. Consider maintaining the current interval instead of resetting.🔎 Suggested fix
TokenPollResult::Pending { slow_down } => { if slow_down { let next = poll_interval.as_millis() as u64 * 3 / 2; poll_interval = Duration::from_millis(std::cmp::min(next, MAX_POLL_INTERVAL_MS)); - } else { - poll_interval = Duration::from_secs(2); } + // Keep current interval when not asked to slow down }
392-401: Tokens without expiry are treated as invalid.When
expiry_dateisNone, the function returnsfalse, forcing re-authentication. If Qwen's API sometimes omitsexpires_in, valid tokens would be discarded. Consider treating missing expiry as valid (with a fallback lifetime) or documenting this assumption.src/provider/mod.rs (2)
248-287: QwenOAuth bypasses the genaiCliententirely.The
QwenOAuthbackend stores aClient(line 37), but thecomplete()method usesqwen_oauth_chat_completion()directly instead. The genaiClientand itsServiceTargetResolverare never used. This creates unnecessary complexity and memory overhead.Consider either:
- Remove the unused
clientfromQwenOAuthbackend- Or use the genai client for consistency with other backends
🔎 Option 1: Simplify backend structure
enum ProviderBackend { GenAI { client: Client, model: String }, QwenOAuth { - client: Client, model: String, oauth: Mutex<QwenOAuthManager>, overrides: Arc<StdMutex<QwenOverrideState>>, }, }And remove the client construction in the initialization (lines 87-116).
379-392: Creating a new HTTP client per request is inefficient.
reqwest::Clientis designed to be reused - it manages connection pooling internally. Creating a new client per request loses these benefits.🔎 Suggested fix: Accept client as parameter
async fn qwen_oauth_chat_completion( + client: &reqwest::Client, base_url: &str, access_token: &str, user_agent: &str, model: &str, system_prompt: &str, user_prompt: &str, ) -> Result<String, ProviderError> { let url = format!("{base_url}chat/completions"); if std::env::var("LUMEN_QWEN_DEBUG").ok().as_deref() == Some("1") { eprintln!("[qwen-oauth] direct_request_url={url}"); } let body = serde_json::json!({ "model": model, "messages": [ { "role": "system", "content": system_prompt }, { "role": "user", "content": user_prompt } ] }); - let client = reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(90)) - .build()?; - let response = client .post(&url)Then store a
reqwest::Clientin theQwenOAuthbackend and pass it to this function.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlsrc/command/configure.rssrc/command/mod.rssrc/config/cli.rssrc/main.rssrc/provider/mod.rssrc/qwen_oauth.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/command/mod.rs (1)
src/git_entity/diff.rs (1)
from_working_tree(26-44)
src/qwen_oauth.rs (2)
release.sh (1)
error(24-24)src/provider/mod.rs (1)
from(315-317)
src/provider/mod.rs (1)
src/qwen_oauth.rs (1)
endpoint_for(130-132)
🔇 Additional comments (18)
src/command/mod.rs (1)
44-46: LGTM - formatting adjustment.src/qwen_oauth.rs (5)
12-24: Constants are well-defined for OAuth flow.The hardcoded client ID is acceptable for public OAuth clients using the device code flow (no client secret). Token refresh buffer of 30 seconds provides reasonable margin for proactive refresh.
25-47: Error handling and credentials struct are well-designed.
95-128: Token management logic is sound.The cascade from cache → refresh → device flow is appropriate. Note that refresh failures are silently handled (line 113), falling through to re-authenticate via device flow. This is reasonable UX behavior.
353-378: URL sanitization with host allowlist is a good security practice.The allowlist approach (
dashscope.aliyuncs.com,dashscope-intl.aliyuncs.com,portal.qwen.ai) prevents potential SSRF or credential leakage to unauthorized hosts.
380-390: PKCE implementation follows RFC 7636 correctly.Using
OsRngfor cryptographic randomness with SHA-256 and URL-safe base64 encoding is the correct approach for PKCE S256 challenge method.src/main.rs (1)
18-18: LGTM - module inclusion for Qwen OAuth.src/config/cli.rs (1)
39-39: LGTM - Qwen provider type addition follows existing pattern.Also applies to: 56-56
src/command/configure.rs (4)
8-13: Good design:needs_api_keycleanly separates authentication modes.This flag provides a scalable way to handle different authentication mechanisms (API keys vs OAuth vs local).
76-81: LGTM - Qwen provider configuration with OAuth indicator.
115-127: Clear user messaging for different authentication flows.The conditional messages for Qwen OAuth browser flow vs Ollama local execution provide good UX guidance.
169-175: Removingapi_keyfor Qwen ensures OAuth-only authentication.Explicitly removing the key prevents confusion if user previously configured a different provider with an API key.
src/provider/mod.rs (5)
4-8: Dual Mutex types are appropriate for mixed sync/async contexts.Using
std::sync::Mutexfor the synchronousServiceTargetResolverclosure andtokio::sync::Mutexfor the asyncQwenOAuthManageris the correct pattern.Also applies to: 29-32
87-111: ServiceTargetResolver correctly injects Qwen OAuth credentials dynamically.The resolver pattern allows updating auth headers without rebuilding the client. The custom DashScope headers follow Qwen's API requirements.
Note: Line 92 uses
expect()which panics on lock poisoning. While lock poisoning is rare, consider if this behavior is acceptable or if a fallback error should be returned.
332-349: Model mapping logic for portal endpoint is clear.The prefix-based mapping handles the known model aliases for
portal.qwen.ai. Non-portal endpoints correctly bypass mapping.
415-435: Content extraction handles multiple response formats correctly.Supporting both string content and array-of-parts format (with
textfield or direct strings) covers the various OpenAI-compatible response structures.
314-318: LGTM - error conversion implementation.Cargo.toml (1)
18-21: Dependencies are appropriate for OAuth PKCE flow.The added dependencies align with the OAuth implementation requirements:
base64,rand,sha2for PKCE code verifier/challenge generationopenfor launching browser during device authorizationAll versions are current or within acceptable patches of the latest releases (base64 0.22.1, rand 0.9.2, sha2 0.10.9, spinoff 0.8.0). The pinned versions provide stability while supporting the intended functionality.
| fn save_credentials(credentials: &QwenCredentials) -> Result<(), QwenOAuthError> { | ||
| let path = credential_path()?; | ||
| if let Some(parent) = path.parent() { | ||
| fs::create_dir_all(parent)?; | ||
| } | ||
| let data = serde_json::to_string_pretty(credentials)?; | ||
| fs::write(path, data)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Credential file lacks restricted permissions.
The OAuth credentials (access/refresh tokens) are written without setting restrictive file permissions. On systems with permissive umask, other users could read these tokens.
🔎 Suggested fix for Unix systems
+#[cfg(unix)]
+use std::os::unix::fs::PermissionsExt;
+
fn save_credentials(credentials: &QwenCredentials) -> Result<(), QwenOAuthError> {
let path = credential_path()?;
if let Some(parent) = path.parent() {
fs::create_dir_all(parent)?;
+ #[cfg(unix)]
+ {
+ let mut perms = fs::metadata(parent)?.permissions();
+ perms.set_mode(0o700);
+ fs::set_permissions(parent, perms)?;
+ }
}
let data = serde_json::to_string_pretty(credentials)?;
fs::write(&path, data)?;
+ #[cfg(unix)]
+ {
+ let mut perms = fs::metadata(&path)?.permissions();
+ perms.set_mode(0o600);
+ fs::set_permissions(&path, perms)?;
+ }
Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn save_credentials(credentials: &QwenCredentials) -> Result<(), QwenOAuthError> { | |
| let path = credential_path()?; | |
| if let Some(parent) = path.parent() { | |
| fs::create_dir_all(parent)?; | |
| } | |
| let data = serde_json::to_string_pretty(credentials)?; | |
| fs::write(path, data)?; | |
| Ok(()) | |
| } | |
| #[cfg(unix)] | |
| use std::os::unix::fs::PermissionsExt; | |
| fn save_credentials(credentials: &QwenCredentials) -> Result<(), QwenOAuthError> { | |
| let path = credential_path()?; | |
| if let Some(parent) = path.parent() { | |
| fs::create_dir_all(parent)?; | |
| #[cfg(unix)] | |
| { | |
| let mut perms = fs::metadata(parent)?.permissions(); | |
| perms.set_mode(0o700); | |
| fs::set_permissions(parent, perms)?; | |
| } | |
| } | |
| let data = serde_json::to_string_pretty(credentials)?; | |
| fs::write(&path, data)?; | |
| #[cfg(unix)] | |
| { | |
| let mut perms = fs::metadata(&path)?.permissions(); | |
| perms.set_mode(0o600); | |
| fs::set_permissions(&path, perms)?; | |
| } | |
| Ok(()) | |
| } |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.